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

[Simon Glass]

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?

Regards,
Simon