Messages in this thread Patch in this message | | | Subject | Re: [PATCH 00/13 v2] Regulator ena_gpiod fixups | From | Marek Szyprowski <> | Date | Tue, 4 Dec 2018 11:33:29 +0100 |
| |
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@samsung.com> 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
| |