[U-Boot] [PATCH] power: regulator: Add limits checking while setting voltage and current

[Keerthy]

On Wednesday 12 October 2016 11:14 AM, Keerthy wrote:
>
>
> On Tuesday 11 October 2016 05:49 AM, Simon Glass wrote:
>> Hi Keerthy,
>>
>> On 15 September 2016 at 05:16, Keerthy <a0393675 at ti.com> wrote:
>>>
>>>
>>> On Thursday 15 September 2016 04:38 PM, Keerthy wrote:
>>>>
>>>>
>>>>
>>>> On Thursday 15 September 2016 04:26 PM, Przemyslaw Marczak wrote:
>>>>>
>>>>> Hello Keerthy,
>>>>>
>>>>> On 09/15/2016 10:54 AM, Keerthy wrote:
>>>>>>
>>>>>> Currently the specific set ops functions are directly
>>>>>> called without any check for voltage/current limits for a regulator.
>>>>>> Check for them and proceed.
>>>>>>
>>>>>> Signed-off-by: Keerthy <j-keerthy at ti.com>
>>>>>> ---
>>>>>>   drivers/power/regulator/regulator-uclass.c | 10 ++++++++++
>>>>>>   1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/power/regulator/regulator-uclass.c
>>>>>> b/drivers/power/regulator/regulator-uclass.c
>>>>>> index 4434e36..089455e 100644
>>>>>> --- a/drivers/power/regulator/regulator-uclass.c
>>>>>> +++ b/drivers/power/regulator/regulator-uclass.c
>>>>>> @@ -41,6 +41,11 @@ int regulator_get_value(struct udevice *dev)
>>>>>>   int regulator_set_value(struct udevice *dev, int uV)
>>>>>>   {
>>>>>>       const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
>>>>>> +    struct dm_regulator_uclass_platdata *uc_pdata;
>>>>>> +
>>>>>> +    uc_pdata = dev_get_uclass_platdata(dev);
>>>>>> +    if (uV < uc_pdata->min_uV || uV > uc_pdata->max_uV)
>>>>>> +        return -EINVAL;
>>>>>>         if (!ops || !ops->set_value)
>>>>>>           return -ENOSYS;
>>>>>> @@ -61,6 +66,11 @@ int regulator_get_current(struct udevice *dev)
>>>>>>   int regulator_set_current(struct udevice *dev, int uA)
>>>>>>   {
>>>>>>       const struct dm_regulator_ops *ops = dev_get_driver_ops(dev);
>>>>>> +    struct dm_regulator_uclass_platdata *uc_pdata;
>>>>>> +
>>>>>> +    uc_pdata = dev_get_uclass_platdata(dev);
>>>>>> +    if (uA < uc_pdata->min_uA || uA > uc_pdata->max_uA)
>>>>>> +        return -EINVAL;
>>>>>>         if (!ops || !ops->set_current)
>>>>>>           return -ENOSYS;
>>>>>
>>>>>
>>>>> Adding those two checks will conflict with "force" option for
>>>>> cmd/regulator.c:283.
>>>>> There was value range checking in the command's code, but it was left
>>>>> unchecked
>>>>> for regulator direct calls.
>>>>>
>>>>> This change is good, but then maybe the "force" option should be
>>>>> removed
>>>>> from command,
>>>>> or API's prototypes should be updated by force flag argument?
>>>>>
>>>>> I assumed that this option could be useful for quick over-voltage
>>>>> setting (until reboot),
>>>>> since usually (min_uV == max_uV) - the voltage can't be changed in any
>>>>> range.
>>>>>
>>>>> The driver should take care about ignore it or not, however probably
>>>>> nobody used this.
>>>>>
>>>>> Of course this could potentially damage the device by wrong use,
>>>>> which can be also made by passing the force flag as an argument - by
>>>>> mistake.
>>>>>
>>>>> What do you thing about, update the dm_regulator_ops by:
>>>>>
>>>>>  - int (*set_value)(struct udevice *dev, int uV, int flag);
>>>>>  - int (*set_current)(struct udevice *dev, int uA, int flag);
>>>
>>>
>>> I personally do not like setting an extra flag everywhere just
>>> because we
>>> want to support force option.
>>>
>>> I would rather have 2 functions like:
>>>
>>> int (*force_set_value)(struct udevice *dev, int uV);
>>> int (*force_set_current)(struct udevice *dev, int uA);
>>>
>>> Where we can set the value ignoring the limits. But again that must
>>> be used
>>> with at most caution as setting higher voltages might end up damaging
>>> the
>>> device.
>>
>> That seems OK to me.
>>
>>>
>>>
>>>>>
>>>>> and also new flag to the present defined:
>>>>>
>>>>>  - REGULATOR_FLAG_FORCE = 1 << 2
>>>>>
>>>>> This requires more work, but will provide the functionality in a
>>>>> proper
>>>>> way.
>>>>
>>>>
>>>> I do not know cmd_regulator is the right place to check for min_uV and
>>>> max_uV. dm_regulator_uclass_platdata has both the limits and this i
>>>> feel
>>>> is a perfectly valid check in generic place else we would be again
>>>> checking for the same condition in every possible regulator specific
>>>> drivers.
>>>>
>>>> As far as the force option is concerned that i believe is more for
>>>> testing and like you said can be implemented by setting a flag.
>>>>
>>>> Just take a simple case of say a driver like mmc which unknowingly
>>>> requests a wrong voltage and the base driver has no check against min
>>>> and max voltages and assuming the regulator driver goes ahead and
>>>> sets a
>>>> very high voltage. That can be catastrophic right?
>>
>> What is the status of this patch please?
>>
>> Also it breaks tests (make tests) - can you please take a  look?
>
> Sure Simon. It was hanging somehow. I will redo and repost it.
Simon,

https://patchwork.ozlabs.org/patch/686932/
https://patchwork.ozlabs.org/patch/686933/
https://patchwork.ozlabs.org/patch/686934/

Posted the above 3 as a rework for this patch. Let me know if these are 
fine.

Regards,
Keerthy

>
>>
>> Regards,
>> Simon
>>