From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keerthy Date: Thu, 15 Sep 2016 16:46:02 +0530 Subject: [U-Boot] [PATCH] power: regulator: Add limits checking while setting voltage and current In-Reply-To: References: <1473929671-354-1-git-send-email-j-keerthy@ti.com> <57DA7E70.2020900@samsung.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> --- >>> 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, >>