From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 15 Sep 2016 10:15:55 +0200 Subject: [U-Boot] [PATCH v3 3/5] regulator: fixed: honour optionality of enable gpio In-Reply-To: <1473924445-14456-4-git-send-email-marcel.ziswiler@toradex.com> References: <1473924445-14456-1-git-send-email-marcel.ziswiler@toradex.com> <1473924445-14456-4-git-send-email-marcel.ziswiler@toradex.com> Message-ID: <57DA58BB.10908@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 = &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