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

[David Collins <collinsd@codeaurora.org>]

On 04/19/2018 09:16 AM, Doug Anderson wrote:
> On Wed, Apr 18, 2018 at 4:30 PM, David Collins <collinsd@codeaurora.org> 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, &uV);
>>>> +               if (!ret) {
>>>> +                       range = &vreg->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,
>>>> +       };
>>>
>>> You're sticking a negative value in an array of unsigned inits.  Here
>>> and in other similar functions.
>>>
>>> I know, I know.  The function is defined to return an unsigned int.
>>> It's wrong.  of_regulator.c clearly puts the return code in a signed
>>> int.  First attempt at fixing this is at
>>> <https://patchwork.kernel.org/patch/10346081/>.
>>
>> I can change the error cases to use REGULATOR_MODE_INVALID which is added
>> by this change still under review [2].
> 
> I haven't seen Mark NAK it (yet), so for lack of a better option I'd
> start using it in your patch and document in the commit message that
> it depends on my patch.

Your patch was accepted.  I'll switch to using REGULATOR_MODE_INVALID.

Thanks,
David

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