Re: [PATCH V2] gpio: Fix return value mismatch of function gpiod_get_from_of_node()

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Thu, 20 Jun 2019 at 23:37, Waibel Georg
<Georg.Waibel@xxxxxxxxxxxxxxxxx> wrote:
>
> In case the requested gpio property is not found in the device tree, some
> callers of gpiod_get_from_of_node() expect a return value of NULL, others
> expect -ENOENT.
> In particular devm_fwnode_get_index_gpiod_from_child() expects -ENOENT.
> Currently it gets a NULL, which breaks the loop that tries all
> gpio_suffixes. The result is that a gpio property is not found, even
> though it is there.
>
> This patch changes gpiod_get_from_of_node() to return -ENOENT instead
> of NULL when the requested gpio property is not found in the device
> tree. Additionally it modifies all calling functions to properly
> evaluate the return value.
>
> Another approach would be to leave the return value of
> gpiod_get_from_of_node() as is and fix the bug in
> devm_fwnode_get_index_gpiod_from_child(). Other callers would still need
> to be reworked. The effort would be the same as with the chosen solution.
>
> Signed-off-by: Georg Waibel <georg.waibel@xxxxxxxxxxxxxxxxx>
> ---
>
> V2: Rebased on top of [PATCH] regulator: s2mps11: Fix ERR_PTR dereference on GPIO lookup failure
>
> ---
>  drivers/gpio/gpiolib.c                 | 6 +-----
>  drivers/regulator/da9211-regulator.c   | 2 ++
>  drivers/regulator/s2mps11.c            | 4 +++-
>  drivers/regulator/s5m8767.c            | 4 +++-
>  drivers/regulator/tps65090-regulator.c | 7 ++++---
>  5 files changed, 13 insertions(+), 10 deletions(-)

You missed my tag from v1:

For s2mps11 and s5m8767 bits:
Reviewed-by: Krzysztof Kozlowski <krzk@xxxxxxxxxx>

Best regards,
Krzysztof

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e013d41..be1d1d2 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4244,8 +4244,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_index);
>   *
>   * Returns:
>   * On successful request the GPIO pin is configured in accordance with
> - * provided @dflags. If the node does not have the requested GPIO
> - * property, NULL is returned.
> + * provided @dflags.
>   *
>   * In case of error an ERR_PTR() is returned.
>   */
> @@ -4267,9 +4266,6 @@ struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
>                                         index, &flags);
>
>         if (!desc || IS_ERR(desc)) {
> -               /* If it is not there, just return NULL */
> -               if (PTR_ERR(desc) == -ENOENT)
> -                       return NULL;
>                 return desc;
>         }
>
> diff --git a/drivers/regulator/da9211-regulator.c b/drivers/regulator/da9211-regulator.c
> index da37b4c..0309823 100644
> --- a/drivers/regulator/da9211-regulator.c
> +++ b/drivers/regulator/da9211-regulator.c
> @@ -289,6 +289,8 @@ static struct da9211_pdata *da9211_parse_regulators_dt(
>                                   0,
>                                   GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
>                                   "da9211-enable");
> +               if (IS_ERR(pdata->gpiod_ren[n]))
> +                       pdata->gpiod_ren[n] = NULL;
>                 n++;
>         }
>
> diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
> index af9bf10..209d1ff 100644
> --- a/drivers/regulator/s2mps11.c
> +++ b/drivers/regulator/s2mps11.c
> @@ -821,7 +821,9 @@ static void s2mps14_pmic_dt_parse_ext_control_gpio(struct platform_device *pdev,
>                                 0,
>                                 GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
>                                 "s2mps11-regulator");
> -               if (IS_ERR(gpio[reg])) {
> +               if (PTR_ERR(gpio[reg]) == -ENOENT)
> +                       gpio[reg] = NULL;
> +               else if (IS_ERR(gpio[reg])) {
>                         dev_err(&pdev->dev, "Failed to get control GPIO for %d/%s\n",
>                                 reg, rdata[reg].name);
>                         gpio[reg] = NULL;
> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> index bb9d1a0..6ca27e9 100644
> --- a/drivers/regulator/s5m8767.c
> +++ b/drivers/regulator/s5m8767.c
> @@ -574,7 +574,9 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>                         0,
>                         GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
>                         "s5m8767");
> -               if (IS_ERR(rdata->ext_control_gpiod))
> +               if (PTR_ERR(rdata->ext_control_gpiod) == -ENOENT)
> +                       rdata->ext_control_gpiod = NULL;
> +               else if (IS_ERR(rdata->ext_control_gpiod))
>                         return PTR_ERR(rdata->ext_control_gpiod);
>
>                 rdata->id = i;
> diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
> index ca39b3d..10ea4b5 100644
> --- a/drivers/regulator/tps65090-regulator.c
> +++ b/drivers/regulator/tps65090-regulator.c
> @@ -371,11 +371,12 @@ static struct tps65090_platform_data *tps65090_parse_dt_reg_data(
>                                                                     "dcdc-ext-control-gpios", 0,
>                                                                     gflags,
>                                                                     "tps65090");
> -                       if (IS_ERR(rpdata->gpiod))
> -                               return ERR_CAST(rpdata->gpiod);
> -                       if (!rpdata->gpiod)
> +                       if (PTR_ERR(rpdata->gpiod) == -ENOENT) {
>                                 dev_err(&pdev->dev,
>                                         "could not find DCDC external control GPIO\n");
> +                               rpdata->gpiod = NULL;
> +                       } else if (IS_ERR(rpdata->gpiod))
> +                               return ERR_CAST(rpdata->gpiod);
>                 }
>
>                 if (of_property_read_u32(tps65090_matches[idx].of_node,
> --
> 2.7.4
>