[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 <marcel.ziswiler at toradex.com>
>
> ---
>
> 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 = &dev_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(&dev_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(&dev_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&D Institute Poland
Samsung Electronics
p.marczak at samsung.com