Re: [PATCH 00/13 v2] Regulator ena_gpiod fixups

[Marek Szyprowski]

Hi Linus,

On 2018-12-04 10:31, Linus Walleij wrote:
> Hi Marek,
>
> first, thanks a *lot* for testing this, it is is much, much appreciated!
>
> On Mon, Dec 3, 2018 at 3:35 PM Marek Szyprowski
> <m.szyprowski@xxxxxxxxxxx> wrote:
>
>> The idea is good imho, but it looks that there are some missing cases in
>> the code. Here are some logs from the boards I have access to:
> OK let's fix this!
>
>> Artik5 evaluation board (arch/arm/boot/dts/exynos3250-artik5-eval.dtb):
>> s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12
> Question: this is supposed to fail, right? It is something
> like a probe deferral or nonexisting GPIO controller?

It looks that the issue has been introduced earlier, but I didn't notice it.

gpiod_get_from_of_node() doesn't handle GPIOD_FLAGS_BIT_NONEXCLUSIVE
flag, the rest is just a result of it.


Here we have a case, where 2 regulators provided by s2mps11 driver have
a common gpio enable line (by PMIC design), so s2mps11 calls
devm_gpiod_get_from_of_node() 2 times for exactly the same gpio descriptor.


Fixing gpiod_get_from_of_node() for GPIOD_FLAGS_BIT_NONEXCLUSIVE is trivial:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cd84315ad586..ace194665b19 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4192,6 +4192,8 @@ struct gpio_desc *gpiod_get_from_of_node(struct
device_node *node,
ÂÂÂÂÂÂÂ transitory = flags & OF_GPIO_TRANSITORY;

ÂÂÂÂÂÂÂ ret = gpiod_request(desc, label);
+ÂÂÂÂÂÂ if (ret == -EBUSY && (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return desc;
ÂÂÂÂÂÂÂ if (ret)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ERR_PTR(ret);


With the above fix I still however get 2 warnings from devres functions,
but this is probably caused by adding the same entry 2 times to the list
without proper refcounting... I will check that later.


> I look in the upstream tree:
> arch/arm/boot/dts/exynos3250-artik5.dtsi
> where s2mps14 is defined:
>
> ldo12_reg: LDO12 {
>     /* VDD72 ~ VDD73 */
>     regulator-name = "VLDO12_2.8V";
>     regulator-min-microvolt = <2800000>;
>     regulator-max-microvolt = <2800000>;
>     samsung,ext-control-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>;
> };
>
> I didn't really change anything about this, so this missing
> GPIO descriptor looks worrysome.
>
> Anyways what happens is this:
>
> gpio[reg] = devm_gpiod_get_from_of_node(...)
> if (IS_ERR(gpio[reg]))
> (...)
>             continue;
>
> So this IS_ERR descriptor is left around. So we should
> probably handle erronoeus or NULL descriptors in
> gpiod_unhinge().
>
> If you add this on top, does it start working?
>
> diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
> index 5864e758d7f2..e35751bf0ea8 100644
> --- a/drivers/gpio/gpiolib-devres.c
> +++ b/drivers/gpio/gpiolib-devres.c
> @@ -332,6 +332,8 @@ EXPORT_SYMBOL(devm_gpiod_put);
>
>  void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc)
>  {
> +       if (IS_ERR_OR_NULL(desc))
> +               return;
>         WARN_ON(devres_destroy(dev, devm_gpiod_release,
>                                devm_gpiod_match, desc));
>  }
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland