Hi Keerthy,
On 15 September 2016 at 05:16, Keerthy <a0393...@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-keer...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
