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

[Keerthy]

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.

>>
>> 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?
>
>>
>> Best regards,
>>