On 04/19/2018 09:16 AM, Doug Anderson wrote:
> On Wed, Apr 18, 2018 at 4:30 PM, David Collins <collinsd@xxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
