Re: [U-Boot] [PATCH v3 3/5] regulator: fixed: honour optionality of enable gpio

[Stephen Warren]

On 09/15/2016 03:30 AM, John Keeping wrote:

On Thu, 15 Sep 2016 09:27:22 +0200, Marcel Ziswiler wrote:


According to the binding documentation the fixed regulator enable GPIO
is optional. However so far registration thereof failed if no enable
GPIO was specified. Fix this by making it entirely optional whether an
enable GPIO is used.



@@ -98,8 +100,9 @@ static int fixed_regulator_set_enable(struct udevice *dev, 
bool enable)
struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
int ret;

+   /* Enable GPIO is optional */
if (!dev_pdata->gpio.dev)
-   return -ENOSYS;
+   return 0;


I'm not sure about this change, the current behaviour seems correct to
me.  After this we're pretending that fixed_set_enable(dev, false) has
succeeded, when it has not.


That should probably be:

if (!dev_pdata->gpio.dev) {
if (!enable)
return -ENOSYS
return 0;
}

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/5] regulator: fixed: honour optionality of enable gpio

[Przemyslaw Marczak]

Hello Marcel,

On 09/15/2016 09:27 AM, Marcel Ziswiler wrote:

According to the binding documentation the fixed regulator enable GPIO
is optional. However so far registration thereof failed if no enable
GPIO was specified. Fix this by making it entirely optional whether an
enable GPIO is used.

Signed-off-by: Marcel Ziswiler 

---

Changes in v3:
- Introduce new patch to honour optionality of fixed regulator enable
   GPIO.

Changes in v2: None

  drivers/power/regulator/fixed.c | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/power/regulator/fixed.c b/drivers/power/regulator/fixed.c
index 37b8400..c68da70 100644
--- a/drivers/power/regulator/fixed.c
+++ b/drivers/power/regulator/fixed.c
@@ -37,11 +37,12 @@ static int fixed_regulator_ofdata_to_platdata(struct 
udevice *dev)
/* Set type to fixed */
uc_pdata->type = REGULATOR_TYPE_FIXED;
  
-	/* Get fixed regulator gpio desc */

+   /* Get fixed regulator optional enable GPIO desc */
gpio = _pdata->gpio;
ret = gpio_request_by_name(dev, "gpio", 0, gpio, GPIOD_IS_OUT);
if (ret)
-   debug("Fixed regulator gpio - not found! Error: %d", ret);
+   debug("Fixed reg optional enable GPIO - not found! Error: %d",
+ ret);


The "reg" is usually threated as a "register", so full word is better here.

  
  	/* Get optional ramp up delay */

dev_pdata->startup_delay_us = fdtdec_get_uint(gd->fdt_blob,
@@ -87,8 +88,9 @@ static bool fixed_regulator_get_enable(struct udevice *dev)
  {
struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
  
+	/* Enable GPIO is optional */

if (!dev_pdata->gpio.dev)
-   return false;
+   return true;


That is good fix :)

  
  	return dm_gpio_get_value(_pdata->gpio);

  }
@@ -98,8 +100,9 @@ static int fixed_regulator_set_enable(struct udevice *dev, 
bool enable)
struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
int ret;
  
+	/* Enable GPIO is optional */

if (!dev_pdata->gpio.dev)
-   return -ENOSYS;
+   return 0;
  
  	ret = dm_gpio_set_value(_pdata->gpio, enable);

if (ret) {


I don't think, that returning the 0 (success) is a good idea.

We should return some error value, because this is checked
by the regulator's command code.

User can be informed about what was happened - instead of getting
command succeed - but without any results behind the command.

Maybe the -ENOSYS is not the best here. What about -EPERM?

We could assume, that such operation is not permitted for this device.

Best regards,
--
Przemyslaw Marczak
Samsung R Institute Poland
Samsung Electronics
p.marc...@samsung.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 3/5] regulator: fixed: honour optionality of enable gpio

[John Keeping]

On Thu, 15 Sep 2016 09:27:22 +0200, Marcel Ziswiler wrote:

> According to the binding documentation the fixed regulator enable GPIO
> is optional. However so far registration thereof failed if no enable
> GPIO was specified. Fix this by making it entirely optional whether an
> enable GPIO is used.
> 
> Signed-off-by: Marcel Ziswiler 
> 
> ---
> 
> Changes in v3:
> - Introduce new patch to honour optionality of fixed regulator enable
>   GPIO.
> 
> Changes in v2: None
> 
>  drivers/power/regulator/fixed.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/regulator/fixed.c b/drivers/power/regulator/fixed.c
> index 37b8400..c68da70 100644
> --- a/drivers/power/regulator/fixed.c
> +++ b/drivers/power/regulator/fixed.c
> @@ -37,11 +37,12 @@ static int fixed_regulator_ofdata_to_platdata(struct 
> udevice *dev)
>   /* Set type to fixed */
>   uc_pdata->type = REGULATOR_TYPE_FIXED;
>  
> - /* Get fixed regulator gpio desc */
> + /* Get fixed regulator optional enable GPIO desc */
>   gpio = _pdata->gpio;
>   ret = gpio_request_by_name(dev, "gpio", 0, gpio, GPIOD_IS_OUT);
>   if (ret)
> - debug("Fixed regulator gpio - not found! Error: %d", ret);
> + debug("Fixed reg optional enable GPIO - not found! Error: %d",
> +   ret);

If we're changing this, should we tighten up the error handling?  It's
only ENOENT that we want to ignore here, other errors should probably
make fixed_regulator_ofdata_to_platdata() fail.

>   /* Get optional ramp up delay */
>   dev_pdata->startup_delay_us = fdtdec_get_uint(gd->fdt_blob,
> @@ -87,8 +88,9 @@ static bool fixed_regulator_get_enable(struct udevice *dev)
>  {
>   struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
>  
> + /* Enable GPIO is optional */
>   if (!dev_pdata->gpio.dev)
> - return false;
> + return true;

This looks much better to me (I thought I had a similar change locally
but I can't find it now).

>   return dm_gpio_get_value(_pdata->gpio);
>  }
> @@ -98,8 +100,9 @@ static int fixed_regulator_set_enable(struct udevice *dev, 
> bool enable)
>   struct fixed_regulator_platdata *dev_pdata = dev_get_platdata(dev);
>   int ret;
>  
> + /* Enable GPIO is optional */
>   if (!dev_pdata->gpio.dev)
> - return -ENOSYS;
> + return 0;

I'm not sure about this change, the current behaviour seems correct to
me.  After this we're pretending that fixed_set_enable(dev, false) has
succeeded, when it has not.

>   ret = dm_gpio_set_value(_pdata->gpio, enable);
>   if (ret) {
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot