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

[Keerthy]

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.

>
> Regards,
> Simon
>