Re: [PATCH 08/21] regulator: max77686: Pass descriptor instead of GPIO number

[Krzysztof Kozlowski]

On Mon, Feb 12, 2018 at 2:17 PM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> Instead of passing a global GPIO number, pass a descriptor looked
> up from the device tree configuration node.
>
> Cc: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
> Cc: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/regulator/max77686-regulator.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/regulator/max77686-regulator.c b/drivers/regulator/max77686-regulator.c
> index c301f3733475..5ebd06f47e6a 100644
> --- a/drivers/regulator/max77686-regulator.c
> +++ b/drivers/regulator/max77686-regulator.c
> @@ -25,8 +25,7 @@
>  #include <linux/kernel.h>
>  #include <linux/bug.h>
>  #include <linux/err.h>
> -#include <linux/gpio.h>
> -#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/driver.h>
> @@ -90,6 +89,7 @@ enum max77686_ramp_rate {
>  };
>
>  struct max77686_data {
> +       struct device *dev;
>         DECLARE_BITMAP(gpio_enabled, MAX77686_REGULATORS);
>
>         /* Array indexed by regulator id */
> @@ -269,16 +269,20 @@ static int max77686_of_parse_cb(struct device_node *np,
>         case MAX77686_BUCK8:
>         case MAX77686_BUCK9:
>         case MAX77686_LDO20 ... MAX77686_LDO22:
> -               config->ena_gpio = of_get_named_gpio(np,
> -                                       "maxim,ena-gpios", 0);
> -               config->ena_gpio_flags = GPIOF_OUT_INIT_HIGH;
> -               config->ena_gpio_initialized = true;
> +               config->ena_gpiod = devm_gpiod_get_from_of_node(max77686->dev,
> +                                                               np,
> +                                                               "maxim,ena",
> +                                                               0,
> +                                                               GPIOD_OUT_HIGH,
> +                                                               "max77686-LDO");

This is also for bucks so how about naming it "max77686-regulator"?

> +               if (IS_ERR(config->ena_gpiod))
> +                       return PTR_ERR(config->ena_gpiod);

This is not equivalent (although I did not look at previous patches in
the series). In case of of_get_named_gpio() failure, old code would
just set ena_gpio to invalid value and the ena-gpio method would not
be used. All other init data from Device Tree would be parsed.

In new code, failure of devm_gpiod_get_from_of_node() would cause
error of entire regulator_of_get_init_data() therefore using default
driver's init data.

This is not a huge difference in practice because both are result of
bad DT. However discarding entire init data on one property error
seems quite drastic.
On the other hand, what would happen on PROBE_DEFER?

Best regards,
Krzysztof

>                 break;
>         default:
>                 return 0;
>         }
>
> -       if (gpio_is_valid(config->ena_gpio)) {
> +       if (config->ena_gpiod) {
>                 set_bit(desc->id, max77686->gpio_enabled);
>
>                 return regmap_update_bits(config->regmap, desc->enable_reg,
> @@ -521,6 +525,7 @@ static int max77686_pmic_probe(struct platform_device *pdev)
>         if (!max77686)
>                 return -ENOMEM;
>
> +       max77686->dev = &pdev->dev;
>         config.dev = iodev->dev;
>         config.regmap = iodev->regmap;
>         config.driver_data = max77686;
> --
> 2.14.3
>