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

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

[Keerthy]

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@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;
-- 
1.9.1

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

[Przemyslaw Marczak]

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@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);

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.

Best regards,

-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak@samsung.com

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

[Keerthy]

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@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);
>
> 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,
>

[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@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,
>>

[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@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@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

[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@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@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
>

[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@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@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
>>