lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJKOXPdUUD_ht=S6Koee_mE+6X+7XMzQs+FTGEhVgc+_LbpMkQ@mail.gmail.com>
Date:   Mon, 12 Feb 2018 14:37:11 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org, Chanwoo Choi <cw00.choi@...sung.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [PATCH 08/21] regulator: max77686: Pass descriptor instead of
 GPIO number

On Mon, Feb 12, 2018 at 2:17 PM, Linus Walleij <linus.walleij@...aro.org> wrote:
> Instead of passing a global GPIO number, pass a descriptor looked
> up from the device tree configuration node.
>
> Cc: Chanwoo Choi <cw00.choi@...sung.com>
> Cc: Krzysztof Kozlowski <krzk@...nel.org>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
>  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
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ