| Message ID | 5696378B.6070704@nvidia.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 13, 2016 at 05:09:55PM +0530, Laxman Dewangan wrote: > Yaah, this is something new and I think Mark also wanted to do this cleanup > on his comment. I did not understand it previously. Looks cool now. I did, please don't just ignore review comments. > However, I have requirement to configure PMIC with some inital setting based > on init_data (parsed) before regulator registration happened. Once I move to > this new mecahinsm, the inti_data parsing and regulator init happen in the > same function. So I need to have one callback from regulator_register to > driver. > Something below. This will do the pre configuration before > regulator_register happena dn machine constraint sets. > I can not use the init_data->regulator_init() as I need to have the init > data passed to the driver. Why do you have this requirement?
On Wednesday 13 January 2016 05:27 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Wed, Jan 13, 2016 at 05:09:55PM +0530, Laxman Dewangan wrote: > >> Yaah, this is something new and I think Mark also wanted to do this cleanup >> on his comment. I did not understand it previously. Looks cool now. > I did, please don't just ignore review comments. Sure, I respect all comments and thanks for review. Seems your mail got lost somewhere (still not seeing in my inbox). > >> However, I have requirement to configure PMIC with some inital setting based >> on init_data (parsed) before regulator registration happened. Once I move to >> this new mecahinsm, the inti_data parsing and regulator init happen in the >> same function. So I need to have one callback from regulator_register to >> driver. >> Something below. This will do the pre configuration before >> regulator_register happena dn machine constraint sets. >> I can not use the init_data->regulator_init() as I need to have the init >> data passed to the driver. > Why do you have this requirement? This is require to - configure FPS_SRC based on platform data for each rail, - If FPS_SRC enabled then enable it always. - If FPS_SRC is disabled (NONE) then based on init data constraint, set it to desired state. - Power mode and slew rate init based on default configuration from the register if platform does not want to set it. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 13, 2016 at 05:35:33PM +0530, Laxman Dewangan wrote: > On Wednesday 13 January 2016 05:27 PM, Mark Brown wrote: > >>Something below. This will do the pre configuration before > >>regulator_register happena dn machine constraint sets. > >>I can not use the init_data->regulator_init() as I need to have the init > >>data passed to the driver. > >Why do you have this requirement? > This is require to > - configure FPS_SRC based on platform data for each rail, > - If FPS_SRC enabled then enable it always. > - If FPS_SRC is disabled (NONE) then based on init data constraint, > set it to desired state. What is FPS_SRC and why is it set from init_data? A driver should never be looking at init_data. > - Power mode and slew rate init based on default configuration from the > register if platform does not want to set it. Same here, why is init_data needed here?
On Wednesday 13 January 2016 06:01 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Wed, Jan 13, 2016 at 05:35:33PM +0530, Laxman Dewangan wrote: >> On Wednesday 13 January 2016 05:27 PM, Mark Brown wrote: >> This is require to >> - configure FPS_SRC based on platform data for each rail, >> - If FPS_SRC enabled then enable it always. >> - If FPS_SRC is disabled (NONE) then based on init data constraint, >> set it to desired state. > What is FPS_SRC and why is it set from init_data? A driver should never > be looking at init_data. FPS: Flexible Power Sequence. Max77620 has three FPS source configuration FPS0,1 2 and can be configured with different digital input signals EN0, EN1 and EN2. Then rails and few GPIOs can configured with these FPS SRC with different slots so that it can be controlled externally. When FPS_SRC is set to NONE (this is needed from platform data) then based on constraint like boot enable/always enable, it need to be on desired state. Otherwise we may endup with disabling the rail when setting to NONE and create issue. I need to set the FPS src properly for each rails before callback happen from regulator init so that enable/disable/is_enable can handle it properly. >> - Power mode and slew rate init based on default configuration from the >> register if platform does not want to set it. > Same here, why is init_data needed here? > Here, init data is not needed. I was thinking to configure slew rate in preinit but can be done later also through regulator->ops. But I will need the driver specific DT data (customized flag) to configure the PMIC like slots. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 13, 2016 at 06:07:51PM +0530, Laxman Dewangan wrote: > On Wednesday 13 January 2016 06:01 PM, Mark Brown wrote: > >What is FPS_SRC and why is it set from init_data? A driver should never > >be looking at init_data. > When FPS_SRC is set to NONE (this is needed from platform data) then based > on constraint like boot enable/always enable, it need to be on desired > state. Otherwise we may endup with disabling the rail when setting to NONE > and create issue. What is "it" and why can't we check what the current configuration is while setting FPS to NONE? > I need to set the FPS src properly for each rails before callback happen > from regulator init so that enable/disable/is_enable can handle it properly. Why not just reorder the callback so it happens before the constraints are applied?
On Wednesday 13 January 2016 06:39 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Wed, Jan 13, 2016 at 06:07:51PM +0530, Laxman Dewangan wrote: >> On Wednesday 13 January 2016 06:01 PM, Mark Brown wrote: >>> What is FPS_SRC and why is it set from init_data? A driver should never >>> be looking at init_data. >> When FPS_SRC is set to NONE (this is needed from platform data) then based >> on constraint like boot enable/always enable, it need to be on desired >> state. Otherwise we may endup with disabling the rail when setting to NONE >> and create issue. > What is "it" and why can't we check what the current configuration is > while setting FPS to NONE? I will have platform specific data in callback of enable/disable only. Which call back do I need to set this? enable/disable or post register? >> I need to set the FPS src properly for each rails before callback happen >> from regulator init so that enable/disable/is_enable can handle it properly. > Why not just reorder the callback so it happens before the constraints > are applied? > which callback I need to reorder for getting the context for configruation? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 13, 2016 at 06:41:11PM +0530, Laxman Dewangan wrote: > On Wednesday 13 January 2016 06:39 PM, Mark Brown wrote: > >What is "it" and why can't we check what the current configuration is > >while setting FPS to NONE? > I will have platform specific data in callback of enable/disable only. > Which call back do I need to set this? enable/disable or post register? I'm sorry but I can't parse what you're saying here. > >>I need to set the FPS src properly for each rails before callback happen > >>from regulator init so that enable/disable/is_enable can handle it properly. > >Why not just reorder the callback so it happens before the constraints > >are applied? > which callback I need to reorder for getting the context for configruation? The existing call to parse the DT.
On Wednesday 13 January 2016 07:50 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Wed, Jan 13, 2016 at 06:41:11PM +0530, Laxman Dewangan wrote: >> On Wednesday 13 January 2016 06:39 PM, Mark Brown wrote: >>> What is "it" and why can't we check what the current configuration is >>> while setting FPS to NONE? >> I will have platform specific data in callback of enable/disable only. >> Which call back do I need to set this? enable/disable or post register? > I'm sorry but I can't parse what you're saying here. Here, before machine constraint applies, I need to configure FPS on desired source. >>>> I need to set the FPS src properly for each rails before callback happen >>> >from regulator init so that enable/disable/is_enable can handle it properly. >>> Why not just reorder the callback so it happens before the constraints >>> are applied? >> which callback I need to reorder for getting the context for configruation? > The existing call to parse the DT. Are we allow to do i2c transfer in the existing callback for parsing the DT? By code, nothing is stopping but want to know if there is such limitation on policy/design of this API. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 13, 2016 at 08:12:10PM +0530, Laxman Dewangan wrote: > On Wednesday 13 January 2016 07:50 PM, Mark Brown wrote: > >On Wed, Jan 13, 2016 at 06:41:11PM +0530, Laxman Dewangan wrote: > >>On Wednesday 13 January 2016 06:39 PM, Mark Brown wrote: > >>>What is "it" and why can't we check what the current configuration is > >>>while setting FPS to NONE? > >>I will have platform specific data in callback of enable/disable only. > >>Which call back do I need to set this? enable/disable or post register? > >I'm sorry but I can't parse what you're saying here. > Here, before machine constraint applies, I need to configure FPS on desired > source. I'm a bit confused here - I had thought you needed things the other way around? Can you describe in concrete terms what you want to configure? > >>>>I need to set the FPS src properly for each rails before callback happen > >>>>from regulator init so that enable/disable/is_enable can handle it properly. > >>>Why not just reorder the callback so it happens before the constraints > >>>are applied? > >>which callback I need to reorder for getting the context for configruation? > >The existing call to parse the DT. > Are we allow to do i2c transfer in the existing callback for parsing the DT? Yes, of course.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 3308c6b..aa2ca20 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -3899,6 +3899,17 @@ regulator_register(const struct regulator_desc *regulator_desc, rdev->dev.of_node = of_node_get(config->of_node); } + if (init_data && regulator_desc->pre_register_init) { + ret = regulator_desc->pre_register_init(regulator_desc, + config, init_data); + if (ret < 0) { + dev_err(dev, "Pre register failed: %d\n", ret); + kfree(rdev); + kfree(config); + return ERR_PTR(ret); + } + } + mutex_lock(®ulator_list_mutex); mutex_init(&rdev->mutex); diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index 16ac9e1..fd1d989 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -280,6 +280,9 @@ struct regulator_desc { int (*of_parse_cb)(struct device_node *, const struct regulator_desc *, struct regulator_config *); + int (*pre_register_init)(const struct regulator_desc *, + struct regulator_config *, + struct regulator_init_data *); int id; bool continuous_voltage_range; unsigned n_voltages;