Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Mark Brown]

On Wed, Apr 25, 2018 at 02:04:56PM -0700, David Collins wrote:

> > Using -EAGAIN for "I can't ever read the configuration from this
> > regulator" doesn't seem right - it's not like any number of retries
> > will ever manage to read the value back.

> In this case, the _regulator_get_voltage() call can succeed, but only
> after a voltage is explicitly requested from the framework side.  The

...

> Do you still have reservations about using -EAGAIN for this purpose?  If
> so, which error code would you suggest using?

Yes, that's clearly a problem - -EAGAIN is more for situations where you
can just immediately retry like signal interruptions.  If the caller
repetedly sits and tries to read the voltage it'll never succeed unless
something else comes along and sets something.


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Mark Brown]

On Wed, Apr 25, 2018 at 02:04:56PM -0700, David Collins wrote:

> > Using -EAGAIN for "I can't ever read the configuration from this
> > regulator" doesn't seem right - it's not like any number of retries
> > will ever manage to read the value back.

> In this case, the _regulator_get_voltage() call can succeed, but only
> after a voltage is explicitly requested from the framework side.  The

...

> Do you still have reservations about using -EAGAIN for this purpose?  If
> so, which error code would you suggest using?

Yes, that's clearly a problem - -EAGAIN is more for situations where you
can just immediately retry like signal interruptions.  If the caller
repetedly sits and tries to read the voltage it'll never succeed unless
something else comes along and sets something.


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

>>> I think that's probably only OK if we have a specific error code for the
>>> regulator being limited in this way otherwise our error handling for I/O
>>> problems involves us trying to reconfigure supplies which seems like it
>>> would be risky.  

>> Would you be ok with -EAGAIN being used for this purpose?

> Using -EAGAIN for "I can't ever read the configuration from this
> regulator" doesn't seem right - it's not like any number of retries
> will ever manage to read the value back.

In this case, the _regulator_get_voltage() call can succeed, but only
after a voltage is explicitly requested from the framework side.  The
intention here would then be to call _regulator_do_set_voltage() with the
constraint min_uV to max_uV range.  After that, subsequent
_regulator_get_voltage() calls will be successful.

Here is the general idea:

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 65f9b7c..e61983d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -910,6 +910,19 @@ static int machine_constraints_voltage(struct
regulator_dev *rdev,
rdev->constraints->min_uV && rdev->constraints->max_uV) {
int target_min, target_max;
int current_uV = _regulator_get_voltage(rdev);
+   if (current_uV == -EAGAIN) {
+   /*
+* Regulator voltage cannot be read until after
+* configuration; try setting constraint range.
+*/
+   rdev_info(rdev, "Setting %d-%duV\n",
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+   _regulator_do_set_voltage(rdev,
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+   current_uV = _regulator_get_voltage(rdev);
+   }
if (current_uV < 0) {
rdev_err(rdev,
 "failed to get the current voltage(%d)\n",

Do you still have reservations about using -EAGAIN for this purpose?  If
so, which error code would you suggest using?

Thanks,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

>>> I think that's probably only OK if we have a specific error code for the
>>> regulator being limited in this way otherwise our error handling for I/O
>>> problems involves us trying to reconfigure supplies which seems like it
>>> would be risky.  

>> Would you be ok with -EAGAIN being used for this purpose?

> Using -EAGAIN for "I can't ever read the configuration from this
> regulator" doesn't seem right - it's not like any number of retries
> will ever manage to read the value back.

In this case, the _regulator_get_voltage() call can succeed, but only
after a voltage is explicitly requested from the framework side.  The
intention here would then be to call _regulator_do_set_voltage() with the
constraint min_uV to max_uV range.  After that, subsequent
_regulator_get_voltage() calls will be successful.

Here is the general idea:

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 65f9b7c..e61983d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -910,6 +910,19 @@ static int machine_constraints_voltage(struct
regulator_dev *rdev,
rdev->constraints->min_uV && rdev->constraints->max_uV) {
int target_min, target_max;
int current_uV = _regulator_get_voltage(rdev);
+   if (current_uV == -EAGAIN) {
+   /*
+* Regulator voltage cannot be read until after
+* configuration; try setting constraint range.
+*/
+   rdev_info(rdev, "Setting %d-%duV\n",
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+   _regulator_do_set_voltage(rdev,
+ rdev->constraints->min_uV,
+ rdev->constraints->max_uV);
+   current_uV = _regulator_get_voltage(rdev);
+   }
if (current_uV < 0) {
rdev_err(rdev,
 "failed to get the current voltage(%d)\n",

Do you still have reservations about using -EAGAIN for this purpose?  If
so, which error code would you suggest using?

Thanks,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Mark Brown]

On Tue, Apr 24, 2018 at 02:09:47PM -0700, David Collins wrote:
> On 04/24/2018 10:45 AM, Mark Brown wrote:

> > I think that's probably only OK if we have a specific error code for the
> > regulator being limited in this way otherwise our error handling for I/O
> > problems involves us trying to reconfigure supplies which seems like it
> > would be risky.  

> Would you be ok with -EAGAIN being used for this purpose?

Using -EAGAIN for "I can't ever read the configuration from this
regulator" doesn't seem right - it's not like any number of retries
will ever manage to read the value back.


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Mark Brown]

On Tue, Apr 24, 2018 at 02:09:47PM -0700, David Collins wrote:
> On 04/24/2018 10:45 AM, Mark Brown wrote:

> > I think that's probably only OK if we have a specific error code for the
> > regulator being limited in this way otherwise our error handling for I/O
> > problems involves us trying to reconfigure supplies which seems like it
> > would be risky.  

> Would you be ok with -EAGAIN being used for this purpose?

Using -EAGAIN for "I can't ever read the configuration from this
regulator" doesn't seem right - it's not like any number of retries
will ever manage to read the value back.


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

On 04/24/2018 10:45 AM, Mark Brown wrote:
>>> You'd need to ask Mark if he's OK with it, but a option #3 is to add a
>>> patch to your series fix the regulator framework to try setting the
>>> voltage if _regulator_get_voltage() fails.  Presumably in
>>> machine_constraints_voltage() you'd now do something like:
>>>
>>>   int target_min, target_max;
>>>   int current_uV = _regulator_get_voltage(rdev);
>>>   if (current_uV < 0) {
>>> /* Maybe this regulator's hardware can't be read and needs to be 
>>> initted */
>>> _regulator_do_set_voltage(
>>>   rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
>>> current_uV = _regulator_get_voltage(rdev);
>>>   }
>>>   if (current_uV < 0) {
>>> rdev_err(rdev,
>>>   "failed to get the current voltage(%d)\n",
>>>   current_uV);
>>>   return current_uV;
>>>   }
> 
>>> If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
>>> but this needs to be documented _somewhere_ (unlike for bypass it's
>>> not obvious, so you need to find someplace to put it).  I'd rather not
>>> hack the DT to deal with our software limitations.
> 
>> I'm not opposed to your option #3 though it does seem a little hacky and
>> tailored to the qcom_rpmh-regulator specific case.  Note that I think it
>> would be better to vote for min_uV to max_uV than min_uV to min_uV though.
> 
>> Mark, what are your thoughts on the best way to handle this situation?
> 
> I think that's probably only OK if we have a specific error code for the
> regulator being limited in this way otherwise our error handling for I/O
> problems involves us trying to reconfigure supplies which seems like it
> would be risky.  

Would you be ok with -EAGAIN being used for this purpose?

Thanks,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

On 04/24/2018 10:45 AM, Mark Brown wrote:
>>> You'd need to ask Mark if he's OK with it, but a option #3 is to add a
>>> patch to your series fix the regulator framework to try setting the
>>> voltage if _regulator_get_voltage() fails.  Presumably in
>>> machine_constraints_voltage() you'd now do something like:
>>>
>>>   int target_min, target_max;
>>>   int current_uV = _regulator_get_voltage(rdev);
>>>   if (current_uV < 0) {
>>> /* Maybe this regulator's hardware can't be read and needs to be 
>>> initted */
>>> _regulator_do_set_voltage(
>>>   rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
>>> current_uV = _regulator_get_voltage(rdev);
>>>   }
>>>   if (current_uV < 0) {
>>> rdev_err(rdev,
>>>   "failed to get the current voltage(%d)\n",
>>>   current_uV);
>>>   return current_uV;
>>>   }
> 
>>> If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
>>> but this needs to be documented _somewhere_ (unlike for bypass it's
>>> not obvious, so you need to find someplace to put it).  I'd rather not
>>> hack the DT to deal with our software limitations.
> 
>> I'm not opposed to your option #3 though it does seem a little hacky and
>> tailored to the qcom_rpmh-regulator specific case.  Note that I think it
>> would be better to vote for min_uV to max_uV than min_uV to min_uV though.
> 
>> Mark, what are your thoughts on the best way to handle this situation?
> 
> I think that's probably only OK if we have a specific error code for the
> regulator being limited in this way otherwise our error handling for I/O
> problems involves us trying to reconfigure supplies which seems like it
> would be risky.  

Would you be ok with -EAGAIN being used for this purpose?

Thanks,
David

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Mark Brown]

On Fri, Apr 20, 2018 at 03:08:57PM -0700, David Collins wrote:
> On 04/19/2018 09:16 AM, Doug Anderson wrote:
> > On Wed, Apr 18, 2018 at 4:30 PM, David Collins  
> > wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> > You'd need to ask Mark if he's OK with it, but a option #3 is to add a
> > patch to your series fix the regulator framework to try setting the
> > voltage if _regulator_get_voltage() fails.  Presumably in
> > machine_constraints_voltage() you'd now do something like:
> > 
> >   int target_min, target_max;
> >   int current_uV = _regulator_get_voltage(rdev);
> >   if (current_uV < 0) {
> > /* Maybe this regulator's hardware can't be read and needs to be 
> > initted */
> > _regulator_do_set_voltage(
> >   rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
> > current_uV = _regulator_get_voltage(rdev);
> >   }
> >   if (current_uV < 0) {
> > rdev_err(rdev,
> >   "failed to get the current voltage(%d)\n",
> >   current_uV);
> >   return current_uV;
> >   }

> > If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
> > but this needs to be documented _somewhere_ (unlike for bypass it's
> > not obvious, so you need to find someplace to put it).  I'd rather not
> > hack the DT to deal with our software limitations.

> I'm not opposed to your option #3 though it does seem a little hacky and
> tailored to the qcom_rpmh-regulator specific case.  Note that I think it
> would be better to vote for min_uV to max_uV than min_uV to min_uV though.

> Mark, what are your thoughts on the best way to handle this situation?

I think that's probably only OK if we have a specific error code for the
regulator being limited in this way otherwise our error handling for I/O
problems involves us trying to reconfigure supplies which seems like it
would be risky.  


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Mark Brown]

On Fri, Apr 20, 2018 at 03:08:57PM -0700, David Collins wrote:
> On 04/19/2018 09:16 AM, Doug Anderson wrote:
> > On Wed, Apr 18, 2018 at 4:30 PM, David Collins  
> > wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> > You'd need to ask Mark if he's OK with it, but a option #3 is to add a
> > patch to your series fix the regulator framework to try setting the
> > voltage if _regulator_get_voltage() fails.  Presumably in
> > machine_constraints_voltage() you'd now do something like:
> > 
> >   int target_min, target_max;
> >   int current_uV = _regulator_get_voltage(rdev);
> >   if (current_uV < 0) {
> > /* Maybe this regulator's hardware can't be read and needs to be 
> > initted */
> > _regulator_do_set_voltage(
> >   rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
> > current_uV = _regulator_get_voltage(rdev);
> >   }
> >   if (current_uV < 0) {
> > rdev_err(rdev,
> >   "failed to get the current voltage(%d)\n",
> >   current_uV);
> >   return current_uV;
> >   }

> > If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
> > but this needs to be documented _somewhere_ (unlike for bypass it's
> > not obvious, so you need to find someplace to put it).  I'd rather not
> > hack the DT to deal with our software limitations.

> I'm not opposed to your option #3 though it does seem a little hacky and
> tailored to the qcom_rpmh-regulator specific case.  Note that I think it
> would be better to vote for min_uV to max_uV than min_uV to min_uV though.

> Mark, what are your thoughts on the best way to handle this situation?

I think that's probably only OK if we have a specific error code for the
regulator being limited in this way otherwise our error handling for I/O
problems involves us trying to reconfigure supplies which seems like it
would be risky.  


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

On 04/19/2018 09:16 AM, Doug Anderson wrote:
> On Wed, Apr 18, 2018 at 4:30 PM, David Collins  
> wrote:
 + * @drms_mode: Array of regulator framework modes which 
 can
 + * be configured dynamically for this 
 regulator
 + * via the set_load() callback.
>>>
>>> Using the singular for something that is an array is confusing.  Why
>>> not "drms_modes" or "drms_mode_arr"?  In the past review you said
>>> 'Perhaps something along the lines of "drms_modes"'.
>>
>> It seems awkward to me to use a plural for arrays as it leads to indexing
>> like this: vreg->drms_modes[i].  "mode i" seems better than "modes i".
>> However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
>> if that style is preferred.
> 
> I'd very much like a plural here.

Ok.  I'll change this to be plural.


 +   prop = "qcom,regulator-initial-voltage";
 +   ret = of_property_read_u32(node, prop, );
 +   if (!ret) {
 +   range = >hw_data->voltage_range;
 +   selector = DIV_ROUND_UP(uV - range->min_uV,
 +   range->uV_step) + range->min_sel;
 +   if (uV < range->min_uV || selector > 
 range->max_sel) {
 +   dev_err(dev, "%s: %s=%u is invalid\n",
 +   node->name, prop, uV);
 +   return -EINVAL;
 +   }
 +
 +   vreg->voltage_selector = selector;
 +
 +   cmd[cmd_count].addr
 +   = vreg->addr + 
 RPMH_REGULATOR_REG_VRM_VOLTAGE;
 +   cmd[cmd_count++].data
 +   = DIV_ROUND_UP(selector * range->uV_step
 +   + range->min_uV, 1000);
 +   }
>>>
>>> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
>>> Otherwise "get_voltage_sel" will return selector 0 before the first
>>> set, right?
>>>
>>> Previously Mark said: "If the driver can't read values it should
>>> return an appropriate error code."
>>> ...and previously you said: "I'll try this out and see if the
>>> regulator framework complains during regulator registration."
>>
>> I tested out what happens when vreg->voltage_selector = -EINVAL is set
>> when qcom,regulator-initial-voltage is not present.  This results in
>> devm_regulator_register() failing and subsequently causing the
>> qcom_rpmh-regulator probe to fail.  The error happens in
>> machine_constraints_voltage() [1].
>>
>> This leaves two courses of action:
>> 1. (current patch set) allow voltage_selector to stay 0 if uninitialized
>> 2. Set voltage_selector = -EINVAL by default and specify in DT binding
>> documentation that qcom,regulator-initial-voltage is required for VRM
>> managed RPMh regulator resources which have regulator-min-microvolt and
>> regulator-max-microvolt specified.
>>
>> Are you ok with the DT implications of option #2?
> 
> You'd need to ask Mark if he's OK with it, but a option #3 is to add a
> patch to your series fix the regulator framework to try setting the
> voltage if _regulator_get_voltage() fails.  Presumably in
> machine_constraints_voltage() you'd now do something like:
> 
>   int target_min, target_max;
>   int current_uV = _regulator_get_voltage(rdev);
>   if (current_uV < 0) {
> /* Maybe this regulator's hardware can't be read and needs to be initted 
> */
> _regulator_do_set_voltage(
>   rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
> current_uV = _regulator_get_voltage(rdev);
>   }
>   if (current_uV < 0) {
> rdev_err(rdev,
>   "failed to get the current voltage(%d)\n",
>   current_uV);
>   return current_uV;
>   }
> 
> If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
> but this needs to be documented _somewhere_ (unlike for bypass it's
> not obvious, so you need to find someplace to put it).  I'd rather not
> hack the DT to deal with our software limitations.

I'm not opposed to your option #3 though it does seem a little hacky and
tailored to the qcom_rpmh-regulator specific case.  Note that I think it
would be better to vote for min_uV to max_uV than min_uV to min_uV though.

Mark, what are your thoughts on the best way to handle this situation?


 +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int 
 mode)
 +{
 +   static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = 
 {
 +   [RPMH_REGULATOR_MODE_RET]  = -EINVAL,
 +   [RPMH_REGULATOR_MODE_LPM]  = REGULATOR_MODE_IDLE,
 +   [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
 +   [RPMH_REGULATOR_MODE_HPM]  = 

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

On 04/19/2018 09:16 AM, Doug Anderson wrote:
> On Wed, Apr 18, 2018 at 4:30 PM, David Collins  
> wrote:
 + * @drms_mode: Array of regulator framework modes which 
 can
 + * be configured dynamically for this 
 regulator
 + * via the set_load() callback.
>>>
>>> Using the singular for something that is an array is confusing.  Why
>>> not "drms_modes" or "drms_mode_arr"?  In the past review you said
>>> 'Perhaps something along the lines of "drms_modes"'.
>>
>> It seems awkward to me to use a plural for arrays as it leads to indexing
>> like this: vreg->drms_modes[i].  "mode i" seems better than "modes i".
>> However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
>> if that style is preferred.
> 
> I'd very much like a plural here.

Ok.  I'll change this to be plural.


 +   prop = "qcom,regulator-initial-voltage";
 +   ret = of_property_read_u32(node, prop, );
 +   if (!ret) {
 +   range = >hw_data->voltage_range;
 +   selector = DIV_ROUND_UP(uV - range->min_uV,
 +   range->uV_step) + range->min_sel;
 +   if (uV < range->min_uV || selector > 
 range->max_sel) {
 +   dev_err(dev, "%s: %s=%u is invalid\n",
 +   node->name, prop, uV);
 +   return -EINVAL;
 +   }
 +
 +   vreg->voltage_selector = selector;
 +
 +   cmd[cmd_count].addr
 +   = vreg->addr + 
 RPMH_REGULATOR_REG_VRM_VOLTAGE;
 +   cmd[cmd_count++].data
 +   = DIV_ROUND_UP(selector * range->uV_step
 +   + range->min_uV, 1000);
 +   }
>>>
>>> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }".
>>> Otherwise "get_voltage_sel" will return selector 0 before the first
>>> set, right?
>>>
>>> Previously Mark said: "If the driver can't read values it should
>>> return an appropriate error code."
>>> ...and previously you said: "I'll try this out and see if the
>>> regulator framework complains during regulator registration."
>>
>> I tested out what happens when vreg->voltage_selector = -EINVAL is set
>> when qcom,regulator-initial-voltage is not present.  This results in
>> devm_regulator_register() failing and subsequently causing the
>> qcom_rpmh-regulator probe to fail.  The error happens in
>> machine_constraints_voltage() [1].
>>
>> This leaves two courses of action:
>> 1. (current patch set) allow voltage_selector to stay 0 if uninitialized
>> 2. Set voltage_selector = -EINVAL by default and specify in DT binding
>> documentation that qcom,regulator-initial-voltage is required for VRM
>> managed RPMh regulator resources which have regulator-min-microvolt and
>> regulator-max-microvolt specified.
>>
>> Are you ok with the DT implications of option #2?
> 
> You'd need to ask Mark if he's OK with it, but a option #3 is to add a
> patch to your series fix the regulator framework to try setting the
> voltage if _regulator_get_voltage() fails.  Presumably in
> machine_constraints_voltage() you'd now do something like:
> 
>   int target_min, target_max;
>   int current_uV = _regulator_get_voltage(rdev);
>   if (current_uV < 0) {
> /* Maybe this regulator's hardware can't be read and needs to be initted 
> */
> _regulator_do_set_voltage(
>   rdev, rdev->constraints->min_uV, rdev->constraints->min_uV);
> current_uV = _regulator_get_voltage(rdev);
>   }
>   if (current_uV < 0) {
> rdev_err(rdev,
>   "failed to get the current voltage(%d)\n",
>   current_uV);
>   return current_uV;
>   }
> 
> If Mark doesn't like that then I guess I'd be OK w/ initting it to 0
> but this needs to be documented _somewhere_ (unlike for bypass it's
> not obvious, so you need to find someplace to put it).  I'd rather not
> hack the DT to deal with our software limitations.

I'm not opposed to your option #3 though it does seem a little hacky and
tailored to the qcom_rpmh-regulator specific case.  Note that I think it
would be better to vote for min_uV to max_uV than min_uV to min_uV though.

Mark, what are your thoughts on the best way to handle this situation?


 +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int 
 mode)
 +{
 +   static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = 
 {
 +   [RPMH_REGULATOR_MODE_RET]  = -EINVAL,
 +   [RPMH_REGULATOR_MODE_LPM]  = REGULATOR_MODE_IDLE,
 +   [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL,
 +   [RPMH_REGULATOR_MODE_HPM]  = REGULATOR_MODE_FAST,
 +   };

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Doug Anderson]

Hi,

On Wed, Apr 18, 2018 at 4:30 PM, David Collins  wrote:
>>> + * @drms_mode: Array of regulator framework modes which can
>>> + * be configured dynamically for this regulator
>>> + * via the set_load() callback.
>>
>> Using the singular for something that is an array is confusing.  Why
>> not "drms_modes" or "drms_mode_arr"?  In the past review you said
>> 'Perhaps something along the lines of "drms_modes"'.
>
> It seems awkward to me to use a plural for arrays as it leads to indexing
> like this: vreg->drms_modes[i].  "mode i" seems better than "modes i".
> However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
> if that style is preferred.

I'd very much like a plural here.


>>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>>> +   unsigned int mode, bool bypassed)
>>> +{
>>> +   struct tcs_cmd cmd = {
>>> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>>> +   };
>>
>> Please add:
>>
>> if (mode & ~(REGULATOR_MODE_STANDBY |
>>  REGULATOR_MODE_IDLE |
>>  REGULATOR_MODE_NORMAL |
>>  REGULATOR_MODE_FAST))
>>   return -EINVAL;
>>
>> That way if someone adds a new mode you don't index off the end of
>> your array.  Ah, I see, you have this in rpmh_regulator_vrm_set_mode
>> by checking if mode > REGULATOR_MODE_STANDBY.  That works.  Can you
>> move it here so it's closer to where the array access is?
>
> Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in
> rpmh_regulator_vrm_set_mode() shouldn't be necessary at all.  I felt safer
> leaving it in though.  The framework ensures that no mode values may be
> passed into rpmh_regulator_vrm_set_mode() which is not identified in
> constraints.valid_modes_mask.  Similar sanitization happens for internal
> rpmh_regulator_vrm_set_mode() calls.
>
> I'll move the (mode > REGULATOR_MODE_STANDBY) check into
> rpmh_regulator_vrm_set_mode_bypass().

Ah, good point about the valid_modes_mask!  I'm happy with moving the
test here.  I wouldn't mind a comment saying that the check is
probably overkill because the framework already checks
valid_modes_mask but shouldn't hurt.


>>> +
>>> +   return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
>>
>> Might not hurt to have a comment saying that this calls
>> rpmh_regulator_vrm_set_mode() instead of calling
>> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
>> to change the mode returned by a future call to get_mode().
>
> This seems pretty clear on inspection of the very closely spaced
> functions.  I don't see the need for a comment about it.

It wasn't clear to me--I thought it might have just been because you
didn't want to manually pass in the current bypass state.  Then I
looked at the function and thought there might have been a bug because
it was saving the mode.  Then I looked back at the regulator framework
and finally came to the conclusion that set_load() is supposed to
change the mode (AKA: you'd expect that calling get_mode() after
set_load() would show you the mode you ended up at).

I guess this is all perhaps obvious to any regulator framework
experts, but since I spent 5 minutes digging through all that it
seemed like it deserved a comment to save the next person the 5
minutes.  ...but I won't insist.


>>> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
>>> +   bool *enable)
>>> +{
>>> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>> +
>>> +   *enable = vreg->bypassed;
>>> +
>>> +   return 0;
>>
>> Should you return an error code if nobody has ever called set_bypass?
>> ...or is it OK to just return "not bypassed"?  Please document this in
>> the code.
>
> I think it is fine to return "not bypassed" by default if there is no
> configuration in place to enable bypassing.  How are you suggesting that
> this be documented in the code?

I guess I wish the function had comments and then it could be
documented there.  ...but none of the functions in this file do...

I guess you're right that it's clear enough without a comment, so
let's just leave it as is.


>>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
>>> +   struct device *dev, struct device_node 
>>> *node)
>>> +{
>>> +   const char *prop;
>>> +   int i, len, ret, mode;
>>> +   u32 *buf;
>>> +
>>> +   /* qcom,allowed-drms-modes is optional */
>>> +   prop = "qcom,allowed-drms-modes";
>>> +   len = of_property_count_elems_of_size(node, prop, sizeof(u32));
>>> +   if (len < 0)
>>> +   return 0;
>>> +
>>> +   vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
>>> +   GFP_KERNEL);
>>> +   vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
>>> +  

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Doug Anderson]

Hi,

On Wed, Apr 18, 2018 at 4:30 PM, David Collins  wrote:
>>> + * @drms_mode: Array of regulator framework modes which can
>>> + * be configured dynamically for this regulator
>>> + * via the set_load() callback.
>>
>> Using the singular for something that is an array is confusing.  Why
>> not "drms_modes" or "drms_mode_arr"?  In the past review you said
>> 'Perhaps something along the lines of "drms_modes"'.
>
> It seems awkward to me to use a plural for arrays as it leads to indexing
> like this: vreg->drms_modes[i].  "mode i" seems better than "modes i".
> However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
> if that style is preferred.

I'd very much like a plural here.


>>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>>> +   unsigned int mode, bool bypassed)
>>> +{
>>> +   struct tcs_cmd cmd = {
>>> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>>> +   };
>>
>> Please add:
>>
>> if (mode & ~(REGULATOR_MODE_STANDBY |
>>  REGULATOR_MODE_IDLE |
>>  REGULATOR_MODE_NORMAL |
>>  REGULATOR_MODE_FAST))
>>   return -EINVAL;
>>
>> That way if someone adds a new mode you don't index off the end of
>> your array.  Ah, I see, you have this in rpmh_regulator_vrm_set_mode
>> by checking if mode > REGULATOR_MODE_STANDBY.  That works.  Can you
>> move it here so it's closer to where the array access is?
>
> Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in
> rpmh_regulator_vrm_set_mode() shouldn't be necessary at all.  I felt safer
> leaving it in though.  The framework ensures that no mode values may be
> passed into rpmh_regulator_vrm_set_mode() which is not identified in
> constraints.valid_modes_mask.  Similar sanitization happens for internal
> rpmh_regulator_vrm_set_mode() calls.
>
> I'll move the (mode > REGULATOR_MODE_STANDBY) check into
> rpmh_regulator_vrm_set_mode_bypass().

Ah, good point about the valid_modes_mask!  I'm happy with moving the
test here.  I wouldn't mind a comment saying that the check is
probably overkill because the framework already checks
valid_modes_mask but shouldn't hurt.


>>> +
>>> +   return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
>>
>> Might not hurt to have a comment saying that this calls
>> rpmh_regulator_vrm_set_mode() instead of calling
>> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
>> to change the mode returned by a future call to get_mode().
>
> This seems pretty clear on inspection of the very closely spaced
> functions.  I don't see the need for a comment about it.

It wasn't clear to me--I thought it might have just been because you
didn't want to manually pass in the current bypass state.  Then I
looked at the function and thought there might have been a bug because
it was saving the mode.  Then I looked back at the regulator framework
and finally came to the conclusion that set_load() is supposed to
change the mode (AKA: you'd expect that calling get_mode() after
set_load() would show you the mode you ended up at).

I guess this is all perhaps obvious to any regulator framework
experts, but since I spent 5 minutes digging through all that it
seemed like it deserved a comment to save the next person the 5
minutes.  ...but I won't insist.


>>> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
>>> +   bool *enable)
>>> +{
>>> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>>> +
>>> +   *enable = vreg->bypassed;
>>> +
>>> +   return 0;
>>
>> Should you return an error code if nobody has ever called set_bypass?
>> ...or is it OK to just return "not bypassed"?  Please document this in
>> the code.
>
> I think it is fine to return "not bypassed" by default if there is no
> configuration in place to enable bypassing.  How are you suggesting that
> this be documented in the code?

I guess I wish the function had comments and then it could be
documented there.  ...but none of the functions in this file do...

I guess you're right that it's clear enough without a comment, so
let's just leave it as is.


>>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
>>> +   struct device *dev, struct device_node 
>>> *node)
>>> +{
>>> +   const char *prop;
>>> +   int i, len, ret, mode;
>>> +   u32 *buf;
>>> +
>>> +   /* qcom,allowed-drms-modes is optional */
>>> +   prop = "qcom,allowed-drms-modes";
>>> +   len = of_property_count_elems_of_size(node, prop, sizeof(u32));
>>> +   if (len < 0)
>>> +   return 0;
>>> +
>>> +   vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode),
>>> +   GFP_KERNEL);
>>> +   vreg->drms_mode_max_uA = devm_kcalloc(dev, len,
>>> +  sizeof(*vreg->drms_mode_max_uA), 
>>> 

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Stephen Boyd]

Quoting David Collins (2018-04-18 16:30:26)
> On 04/17/2018 01:02 PM, Doug Anderson wrote:
> > On Fri, Apr 13, 2018 at 7:50 PM, David Collins  
> > wrote:
> >> +#define RPMH_REGULATOR_DISABLE 0x0
> >> +#define RPMH_REGULATOR_ENABLE  0x1
> > 
> > In the last version Stephen said he didn't like the above two #defines
> > and you said you'd remove them, yet they are still here.  Explanation?
> 
> I thought that he was referring to the comments above the constants since
> his review comment was immediately following the second of two comments as
> opposed to these constants.  I'll let him follow-up on this point if
> necessary.
> 

I think I was talking about the comments. The defines don't look super
useful either though.

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Stephen Boyd]

Quoting David Collins (2018-04-18 16:30:26)
> On 04/17/2018 01:02 PM, Doug Anderson wrote:
> > On Fri, Apr 13, 2018 at 7:50 PM, David Collins  
> > wrote:
> >> +#define RPMH_REGULATOR_DISABLE 0x0
> >> +#define RPMH_REGULATOR_ENABLE  0x1
> > 
> > In the last version Stephen said he didn't like the above two #defines
> > and you said you'd remove them, yet they are still here.  Explanation?
> 
> I thought that he was referring to the comments above the constants since
> his review comment was immediately following the second of two comments as
> opposed to these constants.  I'll let him follow-up on this point if
> necessary.
> 

I think I was talking about the comments. The defines don't look super
useful either though.

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

On 04/17/2018 01:02 PM, Doug Anderson wrote:
> On Fri, Apr 13, 2018 at 7:50 PM, David Collins  
> wrote:
>> +#define RPMH_REGULATOR_DISABLE 0x0
>> +#define RPMH_REGULATOR_ENABLE  0x1
> 
> In the last version Stephen said he didn't like the above two #defines
> and you said you'd remove them, yet they are still here.  Explanation?

I thought that he was referring to the comments above the constants since
his review comment was immediately following the second of two comments as
opposed to these constants.  I'll let him follow-up on this point if
necessary.


>> + * @drms_mode: Array of regulator framework modes which can
>> + * be configured dynamically for this regulator
>> + * via the set_load() callback.
> 
> Using the singular for something that is an array is confusing.  Why
> not "drms_modes" or "drms_mode_arr"?  In the past review you said
> 'Perhaps something along the lines of "drms_modes"'.

It seems awkward to me to use a plural for arrays as it leads to indexing
like this: vreg->drms_modes[i].  "mode i" seems better than "modes i".
However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
if that style is preferred.


>> +struct rpmh_vreg {
>> +   struct rpmh_client  *rpmh_client;
>> +   u32 addr;
>> +   struct regulator_desc   rdesc;
>> +   const struct rpmh_vreg_hw_data  *hw_data;
>> +   enum rpmh_regulator_typeregulator_type;
>> +   boolalways_wait_for_ack;
>> +   unsigned int*drms_mode;
>> +   int *drms_mode_max_uA;
>> +   size_t  drms_mode_count;
>> +
>> +   boolenabled;
>> +   int voltage_selector;
>> +   unsigned intmode;
>> +   boolbypassed;
> 
> nit: stick next to "enabled" to make slightly better structure packing...

Ok


>> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
>> +   unsigned int selector)
>> +{
>> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +   struct tcs_cmd cmd = {
>> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
>> +   };
>> +   int ret;
>> +
>> +   /* VRM voltage control register is set with voltage in millivolts. */
>> +   cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
>> +   selector), 1000);
>> +
>> +   ret = rpmh_regulator_send_request(vreg, , 1,
>> + selector > vreg->voltage_selector);
> 
> If you init vreg->voltage_selector to an error as I suggest in
> rpmh_regulator_load_default_parameters() you'll need to handle it
> here.  See below.

Ok


>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>> +   unsigned int mode, bool bypassed)
>> +{
>> +   struct tcs_cmd cmd = {
>> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>> +   };
> 
> Please add:
> 
> if (mode & ~(REGULATOR_MODE_STANDBY |
>  REGULATOR_MODE_IDLE |
>  REGULATOR_MODE_NORMAL |
>  REGULATOR_MODE_FAST))
>   return -EINVAL;
> 
> That way if someone adds a new mode you don't index off the end of
> your array.  Ah, I see, you have this in rpmh_regulator_vrm_set_mode
> by checking if mode > REGULATOR_MODE_STANDBY.  That works.  Can you
> move it here so it's closer to where the array access is?

Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in
rpmh_regulator_vrm_set_mode() shouldn't be necessary at all.  I felt safer
leaving it in though.  The framework ensures that no mode values may be
passed into rpmh_regulator_vrm_set_mode() which is not identified in
constraints.valid_modes_mask.  Similar sanitization happens for internal
rpmh_regulator_vrm_set_mode() calls.

I'll move the (mode > REGULATOR_MODE_STANDBY) check into
rpmh_regulator_vrm_set_mode_bypass().


>> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int 
>> load_uA)
>> +{
>> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +   int i;
>> +
>> +   for (i = 0; i < vreg->drms_mode_count - 1; i++)
>> +   if (load_uA < vreg->drms_mode_max_uA[i])
>> +   break;
> 
> Can you put a warning here if the requested load_uA is larger than the
> largest supported, and/or perhaps consider it an error case?

I'll add a warning.


>> +
>> +   return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
> 
> Might not hurt to have a comment saying that this calls
> rpmh_regulator_vrm_set_mode() instead of calling
> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
> to change the mode 

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[David Collins]

On 04/17/2018 01:02 PM, Doug Anderson wrote:
> On Fri, Apr 13, 2018 at 7:50 PM, David Collins  
> wrote:
>> +#define RPMH_REGULATOR_DISABLE 0x0
>> +#define RPMH_REGULATOR_ENABLE  0x1
> 
> In the last version Stephen said he didn't like the above two #defines
> and you said you'd remove them, yet they are still here.  Explanation?

I thought that he was referring to the comments above the constants since
his review comment was immediately following the second of two comments as
opposed to these constants.  I'll let him follow-up on this point if
necessary.


>> + * @drms_mode: Array of regulator framework modes which can
>> + * be configured dynamically for this regulator
>> + * via the set_load() callback.
> 
> Using the singular for something that is an array is confusing.  Why
> not "drms_modes" or "drms_mode_arr"?  In the past review you said
> 'Perhaps something along the lines of "drms_modes"'.

It seems awkward to me to use a plural for arrays as it leads to indexing
like this: vreg->drms_modes[i].  "mode i" seems better than "modes i".
However, I'm willing to change this to be drms_modes and drms_mode_max_uAs
if that style is preferred.


>> +struct rpmh_vreg {
>> +   struct rpmh_client  *rpmh_client;
>> +   u32 addr;
>> +   struct regulator_desc   rdesc;
>> +   const struct rpmh_vreg_hw_data  *hw_data;
>> +   enum rpmh_regulator_typeregulator_type;
>> +   boolalways_wait_for_ack;
>> +   unsigned int*drms_mode;
>> +   int *drms_mode_max_uA;
>> +   size_t  drms_mode_count;
>> +
>> +   boolenabled;
>> +   int voltage_selector;
>> +   unsigned intmode;
>> +   boolbypassed;
> 
> nit: stick next to "enabled" to make slightly better structure packing...

Ok


>> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
>> +   unsigned int selector)
>> +{
>> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +   struct tcs_cmd cmd = {
>> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
>> +   };
>> +   int ret;
>> +
>> +   /* VRM voltage control register is set with voltage in millivolts. */
>> +   cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
>> +   selector), 1000);
>> +
>> +   ret = rpmh_regulator_send_request(vreg, , 1,
>> + selector > vreg->voltage_selector);
> 
> If you init vreg->voltage_selector to an error as I suggest in
> rpmh_regulator_load_default_parameters() you'll need to handle it
> here.  See below.

Ok


>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
>> +   unsigned int mode, bool bypassed)
>> +{
>> +   struct tcs_cmd cmd = {
>> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
>> +   };
> 
> Please add:
> 
> if (mode & ~(REGULATOR_MODE_STANDBY |
>  REGULATOR_MODE_IDLE |
>  REGULATOR_MODE_NORMAL |
>  REGULATOR_MODE_FAST))
>   return -EINVAL;
> 
> That way if someone adds a new mode you don't index off the end of
> your array.  Ah, I see, you have this in rpmh_regulator_vrm_set_mode
> by checking if mode > REGULATOR_MODE_STANDBY.  That works.  Can you
> move it here so it's closer to where the array access is?

Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in
rpmh_regulator_vrm_set_mode() shouldn't be necessary at all.  I felt safer
leaving it in though.  The framework ensures that no mode values may be
passed into rpmh_regulator_vrm_set_mode() which is not identified in
constraints.valid_modes_mask.  Similar sanitization happens for internal
rpmh_regulator_vrm_set_mode() calls.

I'll move the (mode > REGULATOR_MODE_STANDBY) check into
rpmh_regulator_vrm_set_mode_bypass().


>> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int 
>> load_uA)
>> +{
>> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
>> +   int i;
>> +
>> +   for (i = 0; i < vreg->drms_mode_count - 1; i++)
>> +   if (load_uA < vreg->drms_mode_max_uA[i])
>> +   break;
> 
> Can you put a warning here if the requested load_uA is larger than the
> largest supported, and/or perhaps consider it an error case?

I'll add a warning.


>> +
>> +   return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);
> 
> Might not hurt to have a comment saying that this calls
> rpmh_regulator_vrm_set_mode() instead of calling
> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
> to change the mode returned by a future call to 

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Doug Anderson]

Hi,

On Fri, Apr 13, 2018 at 7:50 PM, David Collins  wrote:
> +#define RPMH_REGULATOR_DISABLE 0x0
> +#define RPMH_REGULATOR_ENABLE  0x1

In the last version Stephen said he didn't like the above two #defines
and you said you'd remove them, yet they are still here.  Explanation?


> + * @drms_mode: Array of regulator framework modes which can
> + * be configured dynamically for this regulator
> + * via the set_load() callback.

Using the singular for something that is an array is confusing.  Why
not "drms_modes" or "drms_mode_arr"?  In the past review you said
'Perhaps something along the lines of "drms_modes"'.


> +struct rpmh_vreg {
> +   struct rpmh_client  *rpmh_client;
> +   u32 addr;
> +   struct regulator_desc   rdesc;
> +   const struct rpmh_vreg_hw_data  *hw_data;
> +   enum rpmh_regulator_typeregulator_type;
> +   boolalways_wait_for_ack;
> +   unsigned int*drms_mode;
> +   int *drms_mode_max_uA;
> +   size_t  drms_mode_count;
> +
> +   boolenabled;
> +   int voltage_selector;
> +   unsigned intmode;
> +   boolbypassed;

nit: stick next to "enabled" to make slightly better structure packing...


> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
> +   unsigned int selector)
> +{
> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +   struct tcs_cmd cmd = {
> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> +   };
> +   int ret;
> +
> +   /* VRM voltage control register is set with voltage in millivolts. */
> +   cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
> +   selector), 1000);
> +
> +   ret = rpmh_regulator_send_request(vreg, , 1,
> + selector > vreg->voltage_selector);

If you init vreg->voltage_selector to an error as I suggest in
rpmh_regulator_load_default_parameters() you'll need to handle it
here.  See below.


> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
> +   unsigned int mode, bool bypassed)
> +{
> +   struct tcs_cmd cmd = {
> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> +   };

Please add:

if (mode & ~(REGULATOR_MODE_STANDBY |
 REGULATOR_MODE_IDLE |
 REGULATOR_MODE_NORMAL |
 REGULATOR_MODE_FAST))
  return -EINVAL;

That way if someone adds a new mode you don't index off the end of
your array.  Ah, I see, you have this in rpmh_regulator_vrm_set_mode
by checking if mode > REGULATOR_MODE_STANDBY.  That works.  Can you
move it here so it's closer to where the array access is?


> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int 
> load_uA)
> +{
> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +   int i;
> +
> +   for (i = 0; i < vreg->drms_mode_count - 1; i++)
> +   if (load_uA < vreg->drms_mode_max_uA[i])
> +   break;

Can you put a warning here if the requested load_uA is larger than the
largest supported, and/or perhaps consider it an error case?


> +
> +   return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);

Might not hurt to have a comment saying that this calls
rpmh_regulator_vrm_set_mode() instead of calling
rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
to change the mode returned by a future call to get_mode().


> +static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
> +   bool enable)
> +{
> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +   int ret;
> +
> +   if (vreg->bypassed == enable)
> +   return 0;

Just like for enable, remove this check.  The regulator core does it for you.


> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
> +   bool *enable)
> +{
> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> +   *enable = vreg->bypassed;
> +
> +   return 0;

Should you return an error code if nobody has ever called set_bypass?
...or is it OK to just return "not bypassed"?  Please document this in
the code.


> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
> +   struct device *dev, struct device_node *node)
> +{
> +   const char *prop;
> +   int i, len, ret, mode;
> +   u32 *buf;
> +
> +   /* qcom,allowed-drms-modes is optional */
> +   prop = "qcom,allowed-drms-modes";
> +   len = 

Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver

[Doug Anderson]

Hi,

On Fri, Apr 13, 2018 at 7:50 PM, David Collins  wrote:
> +#define RPMH_REGULATOR_DISABLE 0x0
> +#define RPMH_REGULATOR_ENABLE  0x1

In the last version Stephen said he didn't like the above two #defines
and you said you'd remove them, yet they are still here.  Explanation?


> + * @drms_mode: Array of regulator framework modes which can
> + * be configured dynamically for this regulator
> + * via the set_load() callback.

Using the singular for something that is an array is confusing.  Why
not "drms_modes" or "drms_mode_arr"?  In the past review you said
'Perhaps something along the lines of "drms_modes"'.


> +struct rpmh_vreg {
> +   struct rpmh_client  *rpmh_client;
> +   u32 addr;
> +   struct regulator_desc   rdesc;
> +   const struct rpmh_vreg_hw_data  *hw_data;
> +   enum rpmh_regulator_typeregulator_type;
> +   boolalways_wait_for_ack;
> +   unsigned int*drms_mode;
> +   int *drms_mode_max_uA;
> +   size_t  drms_mode_count;
> +
> +   boolenabled;
> +   int voltage_selector;
> +   unsigned intmode;
> +   boolbypassed;

nit: stick next to "enabled" to make slightly better structure packing...


> +static int rpmh_regulator_vrm_set_voltage_sel(struct regulator_dev *rdev,
> +   unsigned int selector)
> +{
> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +   struct tcs_cmd cmd = {
> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE,
> +   };
> +   int ret;
> +
> +   /* VRM voltage control register is set with voltage in millivolts. */
> +   cmd.data = DIV_ROUND_UP(regulator_list_voltage_linear_range(rdev,
> +   selector), 1000);
> +
> +   ret = rpmh_regulator_send_request(vreg, , 1,
> + selector > vreg->voltage_selector);

If you init vreg->voltage_selector to an error as I suggest in
rpmh_regulator_load_default_parameters() you'll need to handle it
here.  See below.


> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg,
> +   unsigned int mode, bool bypassed)
> +{
> +   struct tcs_cmd cmd = {
> +   .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE,
> +   };

Please add:

if (mode & ~(REGULATOR_MODE_STANDBY |
 REGULATOR_MODE_IDLE |
 REGULATOR_MODE_NORMAL |
 REGULATOR_MODE_FAST))
  return -EINVAL;

That way if someone adds a new mode you don't index off the end of
your array.  Ah, I see, you have this in rpmh_regulator_vrm_set_mode
by checking if mode > REGULATOR_MODE_STANDBY.  That works.  Can you
move it here so it's closer to where the array access is?


> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int 
> load_uA)
> +{
> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +   int i;
> +
> +   for (i = 0; i < vreg->drms_mode_count - 1; i++)
> +   if (load_uA < vreg->drms_mode_max_uA[i])
> +   break;

Can you put a warning here if the requested load_uA is larger than the
largest supported, and/or perhaps consider it an error case?


> +
> +   return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]);

Might not hurt to have a comment saying that this calls
rpmh_regulator_vrm_set_mode() instead of calling
rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed
to change the mode returned by a future call to get_mode().


> +static int rpmh_regulator_vrm_set_bypass(struct regulator_dev *rdev,
> +   bool enable)
> +{
> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +   int ret;
> +
> +   if (vreg->bypassed == enable)
> +   return 0;

Just like for enable, remove this check.  The regulator core does it for you.


> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev,
> +   bool *enable)
> +{
> +   struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
> +
> +   *enable = vreg->bypassed;
> +
> +   return 0;

Should you return an error code if nobody has ever called set_bypass?
...or is it OK to just return "not bypassed"?  Please document this in
the code.


> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg,
> +   struct device *dev, struct device_node *node)
> +{
> +   const char *prop;
> +   int i, len, ret, mode;
> +   u32 *buf;
> +
> +   /* qcom,allowed-drms-modes is optional */
> +   prop = "qcom,allowed-drms-modes";
> +   len =