[PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Janusz Krzysztofik]

Avoid replication of error code conversion in non-DT GPIO consumers'
code by returning -EPROBE_DEFER from gpiod_find() in case a chip
identified by its label in a registered lookup table is not ready.

See https://lkml.org/lkml/2018/5/30/176 for example case.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
If accepted, please add
	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
if Boris doesn't mind.

Thanks,
Janusz

 drivers/gpio/gpiolib.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..15dc77c80328 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 		chip = find_chip_by_name(p->chip_label);
 
 		if (!chip) {
-			dev_err(dev, "cannot find GPIO chip %s\n",
-				p->chip_label);
-			return ERR_PTR(-ENODEV);
+			/*
+			 * As the lookup table indicates a chip with
+			 * p->chip_label should exist, assume it may
+			 * still appear latar and let the interested
+			 * consumer be probed again or let the Deferred
+			 * Probe infrastructure handle the error.
+			 */
+			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
+				 p->chip_label);
+			return ERR_PTR(-EPROBE_DEFER);
 		}
 
 		if (chip->ngpio <= p->chip_hwnum) {
-- 
2.16.4

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Boris Brezillon]

Hi Janusz,

On Tue,  3 Jul 2018 19:26:35 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Avoid replication of error code conversion in non-DT GPIO consumers'
> code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> identified by its label in a registered lookup table is not ready.
> 
> See https://lkml.org/lkml/2018/5/30/176 for example case.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
> If accepted, please add
> 	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> if Boris doesn't mind.
> 
> Thanks,
> Janusz
> 
>  drivers/gpio/gpiolib.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e11a3bb03820..15dc77c80328 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>  		chip = find_chip_by_name(p->chip_label);
>  
>  		if (!chip) {
> -			dev_err(dev, "cannot find GPIO chip %s\n",
> -				p->chip_label);
> -			return ERR_PTR(-ENODEV);
> +			/*
> +			 * As the lookup table indicates a chip with
> +			 * p->chip_label should exist, assume it may
> +			 * still appear latar and let the interested

					^ later

> +			 * consumer be probed again or let the Deferred
> +			 * Probe infrastructure handle the error.
> +			 */
> +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> +				 p->chip_label);
> +			return ERR_PTR(-EPROBE_DEFER);
>  		}
>  
>  		if (chip->ngpio <= p->chip_hwnum) {

Looks good otherwise. Let's hope we're not breaking implementations
testing for -ENODEV...

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

Regards,

Boris

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Janusz Krzysztofik]

On Tuesday, July 3, 2018 7:31:41 PM CEST Boris Brezillon wrote:
> Hi Janusz,
> 
> On Tue,  3 Jul 2018 19:26:35 +0200
> 
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > Avoid replication of error code conversion in non-DT GPIO consumers'
> > code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> > identified by its label in a registered lookup table is not ready.
> > 
> > See https://lkml.org/lkml/2018/5/30/176 for example case.
> > 
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > ---
> > If accepted, please add
> > 
> > 	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > 
> > if Boris doesn't mind.
> > 
> > Thanks,
> > Janusz
> > 
> >  drivers/gpio/gpiolib.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index e11a3bb03820..15dc77c80328 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct device
> > *dev, const char *con_id,> 
> >  		chip = find_chip_by_name(p->chip_label);
> >  		
> >  		if (!chip) {
> > 
> > -			dev_err(dev, "cannot find GPIO chip %s\n",
> > -				p->chip_label);
> > -			return ERR_PTR(-ENODEV);
> > +			/*
> > +			 * As the lookup table indicates a chip with
> > +			 * p->chip_label should exist, assume it may
> > +			 * still appear latar and let the interested
> 
> 					^ later
> 
> > +			 * consumer be probed again or let the Deferred
> > +			 * Probe infrastructure handle the error.
> > +			 */
> > +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> > +				 p->chip_label);
> > +			return ERR_PTR(-EPROBE_DEFER);
> > 
> >  		}
> >  		
> >  		if (chip->ngpio <= p->chip_hwnum) {
> 
> Looks good otherwise. Let's hope we're not breaking implementations
> testing for -ENODEV...

I've reviewed them all and found two which I think may be affected:
- drivers/mfd/arizona-core.c,
- drivers/i2c/busses/i2c-imx.c.
As far as I can understand the code, both depend on error != -EPROBE_DEFER in 
order to continue in degraded mode. I'm adding their maintainers to the loop.

Thanks,
Janusz

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Uwe Kleine-König]

Hello,

On Wed, Jul 04, 2018 at 09:13:42PM +0200, Janusz Krzysztofik wrote:
> On Tuesday, July 3, 2018 7:31:41 PM CEST Boris Brezillon wrote:
> > Hi Janusz,
> > 
> > On Tue,  3 Jul 2018 19:26:35 +0200
> > 
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > > Avoid replication of error code conversion in non-DT GPIO consumers'
> > > code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> > > identified by its label in a registered lookup table is not ready.
> > > 
> > > See https://lkml.org/lkml/2018/5/30/176 for example case.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > > ---
> > > If accepted, please add
> > > 
> > > 	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > 
> > > if Boris doesn't mind.
> > > 
> > > Thanks,
> > > Janusz
> > > 
> > >  drivers/gpio/gpiolib.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index e11a3bb03820..15dc77c80328 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct device
> > > *dev, const char *con_id,> 
> > >  		chip = find_chip_by_name(p->chip_label);
> > >  		
> > >  		if (!chip) {
> > > 
> > > -			dev_err(dev, "cannot find GPIO chip %s\n",
> > > -				p->chip_label);
> > > -			return ERR_PTR(-ENODEV);
> > > +			/*
> > > +			 * As the lookup table indicates a chip with
> > > +			 * p->chip_label should exist, assume it may
> > > +			 * still appear latar and let the interested
> > 
> > 					^ later
> > 
> > > +			 * consumer be probed again or let the Deferred
> > > +			 * Probe infrastructure handle the error.
> > > +			 */
> > > +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> > > +				 p->chip_label);
> > > +			return ERR_PTR(-EPROBE_DEFER);
> > > 
> > >  		}
> > >  		
> > >  		if (chip->ngpio <= p->chip_hwnum) {
> > 
> > Looks good otherwise. Let's hope we're not breaking implementations
> > testing for -ENODEV...
> 
> I've reviewed them all and found two which I think may be affected:
> - drivers/mfd/arizona-core.c,
> - drivers/i2c/busses/i2c-imx.c.
> As far as I can understand the code, both depend on error != -EPROBE_DEFER in 
> order to continue in degraded mode. I'm adding their maintainers to the loop.

TL;DR: Either I don't understand the implication for
drivers/i2c/busses/i2c-imx.c or everything is fine.

Given that only i2c_imx_init_recovery_info() uses gpio functions I assume
you mean:

        rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
        rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH);

        if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
            PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
                return -EPROBE_DEFER;
        } else if (IS_ERR(rinfo->sda_gpiod) ||
                   IS_ERR(rinfo->scl_gpiod) ||
                   IS_ERR(i2c_imx->pinctrl_pins_default) ||
                   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
                dev_dbg(&pdev->dev, "recovery information incomplete\n");
                return 0;
        }

So if a patch changes devm_gpiod_get() to return -EPROBE_DEFER in more
cases that doesn't seem to hurt. Moreover TTBOMK this driver should only
be used by dt-machines today such that changing gpio* for non-DT users
shouldn't affect it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Lee Jones]

On Wed, 04 Jul 2018, Janusz Krzysztofik wrote:

> On Tuesday, July 3, 2018 7:31:41 PM CEST Boris Brezillon wrote:
> > Hi Janusz,
> > 
> > On Tue,  3 Jul 2018 19:26:35 +0200
> > 
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > > Avoid replication of error code conversion in non-DT GPIO consumers'
> > > code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> > > identified by its label in a registered lookup table is not ready.
> > > 
> > > See https://lkml.org/lkml/2018/5/30/176 for example case.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > > ---
> > > If accepted, please add
> > > 
> > > 	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > 
> > > if Boris doesn't mind.
> > > 
> > > Thanks,
> > > Janusz
> > > 
> > >  drivers/gpio/gpiolib.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index e11a3bb03820..15dc77c80328 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct device
> > > *dev, const char *con_id,> 
> > >  		chip = find_chip_by_name(p->chip_label);
> > >  		
> > >  		if (!chip) {
> > > 
> > > -			dev_err(dev, "cannot find GPIO chip %s\n",
> > > -				p->chip_label);
> > > -			return ERR_PTR(-ENODEV);
> > > +			/*
> > > +			 * As the lookup table indicates a chip with
> > > +			 * p->chip_label should exist, assume it may
> > > +			 * still appear latar and let the interested
> > 
> > 					^ later
> > 
> > > +			 * consumer be probed again or let the Deferred
> > > +			 * Probe infrastructure handle the error.
> > > +			 */
> > > +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> > > +				 p->chip_label);
> > > +			return ERR_PTR(-EPROBE_DEFER);
> > > 
> > >  		}
> > >  		
> > >  		if (chip->ngpio <= p->chip_hwnum) {
> > 
> > Looks good otherwise. Let's hope we're not breaking implementations
> > testing for -ENODEV...
> 
> I've reviewed them all and found two which I think may be affected:
> - drivers/mfd/arizona-core.c,
> - drivers/i2c/busses/i2c-imx.c.
> As far as I can understand the code, both depend on error != -EPROBE_DEFER in 
> order to continue in degraded mode. I'm adding their maintainers to the loop.

>From a quick glance, the -EPROBE_DEFER handing in Arizona Core appears
to be correct.  Would you mind explaining what your concerns are in
more detail please?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Lee Jones]

On Wed, 04 Jul 2018, Janusz Krzysztofik wrote:

> On Tuesday, July 3, 2018 7:31:41 PM CEST Boris Brezillon wrote:
> > Hi Janusz,
> > 
> > On Tue,  3 Jul 2018 19:26:35 +0200
> > 
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > > Avoid replication of error code conversion in non-DT GPIO consumers'
> > > code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> > > identified by its label in a registered lookup table is not ready.
> > > 
> > > See https://lkml.org/lkml/2018/5/30/176 for example case.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > > ---
> > > If accepted, please add
> > > 
> > > 	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > 
> > > if Boris doesn't mind.
> > > 
> > > Thanks,
> > > Janusz
> > > 
> > >  drivers/gpio/gpiolib.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index e11a3bb03820..15dc77c80328 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct device
> > > *dev, const char *con_id,> 
> > >  		chip = find_chip_by_name(p->chip_label);
> > >  		
> > >  		if (!chip) {
> > > 
> > > -			dev_err(dev, "cannot find GPIO chip %s\n",
> > > -				p->chip_label);
> > > -			return ERR_PTR(-ENODEV);
> > > +			/*
> > > +			 * As the lookup table indicates a chip with
> > > +			 * p->chip_label should exist, assume it may
> > > +			 * still appear latar and let the interested
> > 
> > 					^ later
> > 
> > > +			 * consumer be probed again or let the Deferred
> > > +			 * Probe infrastructure handle the error.
> > > +			 */
> > > +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> > > +				 p->chip_label);
> > > +			return ERR_PTR(-EPROBE_DEFER);
> > > 
> > >  		}
> > >  		
> > >  		if (chip->ngpio <= p->chip_hwnum) {
> > 
> > Looks good otherwise. Let's hope we're not breaking implementations
> > testing for -ENODEV...
> 
> I've reviewed them all and found two which I think may be affected:
> - drivers/mfd/arizona-core.c,
> - drivers/i2c/busses/i2c-imx.c.
> As far as I can understand the code, both depend on error != -EPROBE_DEFER in 
> order to continue in degraded mode. I'm adding their maintainers to the loop.

From a quick glance, the -EPROBE_DEFER handing in Arizona Core appears
to be correct.  Would you mind explaining what your concerns are in
more detail please?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Janusz Krzysztofik]

On Thursday, July 5, 2018 7:50:37 AM CEST Lee Jones wrote:
> On Wed, 04 Jul 2018, Janusz Krzysztofik wrote:
> > On Tuesday, July 3, 2018 7:31:41 PM CEST Boris Brezillon wrote:
> > > Hi Janusz,
> > > 
> > > On Tue,  3 Jul 2018 19:26:35 +0200
> > > 
> > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > > > Avoid replication of error code conversion in non-DT GPIO consumers'
> > > > code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> > > > identified by its label in a registered lookup table is not ready.
> > > > 
> > > > See https://lkml.org/lkml/2018/5/30/176 for example case.
> > > > 
> > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > > > ---
> > > > If accepted, please add
> > > > 
> > > > 	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > 
> > > > if Boris doesn't mind.
> > > > 
> > > > Thanks,
> > > > Janusz
> > > > 
> > > >  drivers/gpio/gpiolib.c | 13 ++++++++++---
> > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > index e11a3bb03820..15dc77c80328 100644
> > > > --- a/drivers/gpio/gpiolib.c
> > > > +++ b/drivers/gpio/gpiolib.c
> > > > @@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct
> > > > device
> > > > *dev, const char *con_id,>
> > > > 
> > > >  		chip = find_chip_by_name(p->chip_label);
> > > >  		
> > > >  		if (!chip) {
> > > > 
> > > > -			dev_err(dev, "cannot find GPIO chip %s\n",
> > > > -				p->chip_label);
> > > > -			return ERR_PTR(-ENODEV);
> > > > +			/*
> > > > +			 * As the lookup table indicates a chip with
> > > > +			 * p->chip_label should exist, assume it may
> > > > +			 * still appear latar and let the interested
> > > > 
> > > 					^ later
> > > > 
> > > > +			 * consumer be probed again or let the Deferred
> > > > +			 * Probe infrastructure handle the error.
> > > > +			 */
> > > > +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> > > > +				 p->chip_label);
> > > > +			return ERR_PTR(-EPROBE_DEFER);
> > > > 
> > > >  		}
> > > >  		
> > > >  		if (chip->ngpio <= p->chip_hwnum) {
> > > 
> > > Looks good otherwise. Let's hope we're not breaking implementations
> > > testing for -ENODEV...
> > 
> > I've reviewed them all and found two which I think may be affected:
> > - drivers/mfd/arizona-core.c,
> > - drivers/i2c/busses/i2c-imx.c.
> > As far as I can understand the code, both depend on error != -EPROBE_DEFER
> > in order to continue in degraded mode. I'm adding their maintainers to
> > the loop.
> From a quick glance, the -EPROBE_DEFER handing in Arizona Core appears
> to be correct.  Would you mind explaining what your concerns are in
> more detail please?

Hi

That's more about handling -ENODEV rather than -EPROBE_DEFER.

Before the change, if GPIO chip supposed to provide "reset" pin was not ready 
during  arizona_dev_init(), devm_gpiod_get() returned -ENODEV and device was 
initialized in degraded mode, i.e., with no control over the "reset" pin.
After the change, gpiod_get() will return -EPROBE_DEFER in such case and 
arizona_dev_init() won't succeed in case the GPIO chip doesn't appear later 
for some reason.

Thanks,
Januszz

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Lee Jones]

On Thu, 05 Jul 2018, Janusz Krzysztofik wrote:
> On Thursday, July 5, 2018 7:50:37 AM CEST Lee Jones wrote:
> > On Wed, 04 Jul 2018, Janusz Krzysztofik wrote:
> > > On Tuesday, July 3, 2018 7:31:41 PM CEST Boris Brezillon wrote:
> > > > Hi Janusz,
> > > > 
> > > > On Tue,  3 Jul 2018 19:26:35 +0200
> > > > 
> > > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > > > > Avoid replication of error code conversion in non-DT GPIO consumers'
> > > > > code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> > > > > identified by its label in a registered lookup table is not ready.
> > > > > 
> > > > > See https://lkml.org/lkml/2018/5/30/176 for example case.
> > > > > 
> > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > > > > ---
> > > > > If accepted, please add
> > > > > 
> > > > > 	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > 
> > > > > if Boris doesn't mind.
> > > > > 
> > > > > Thanks,
> > > > > Janusz
> > > > > 
> > > > >  drivers/gpio/gpiolib.c | 13 ++++++++++---
> > > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > > > index e11a3bb03820..15dc77c80328 100644
> > > > > --- a/drivers/gpio/gpiolib.c
> > > > > +++ b/drivers/gpio/gpiolib.c
> > > > > @@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct
> > > > > device
> > > > > *dev, const char *con_id,>
> > > > > 
> > > > >  		chip = find_chip_by_name(p->chip_label);
> > > > >  		
> > > > >  		if (!chip) {
> > > > > 
> > > > > -			dev_err(dev, "cannot find GPIO chip %s\n",
> > > > > -				p->chip_label);
> > > > > -			return ERR_PTR(-ENODEV);
> > > > > +			/*
> > > > > +			 * As the lookup table indicates a chip with
> > > > > +			 * p->chip_label should exist, assume it may
> > > > > +			 * still appear latar and let the interested
> > > > > 
> > > > 					^ later
> > > > > 
> > > > > +			 * consumer be probed again or let the Deferred
> > > > > +			 * Probe infrastructure handle the error.
> > > > > +			 */
> > > > > +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> > > > > +				 p->chip_label);
> > > > > +			return ERR_PTR(-EPROBE_DEFER);
> > > > > 
> > > > >  		}
> > > > >  		
> > > > >  		if (chip->ngpio <= p->chip_hwnum) {
> > > > 
> > > > Looks good otherwise. Let's hope we're not breaking implementations
> > > > testing for -ENODEV...
> > > 
> > > I've reviewed them all and found two which I think may be affected:
> > > - drivers/mfd/arizona-core.c,
> > > - drivers/i2c/busses/i2c-imx.c.
> > > As far as I can understand the code, both depend on error != -EPROBE_DEFER
> > > in order to continue in degraded mode. I'm adding their maintainers to
> > > the loop.
> > From a quick glance, the -EPROBE_DEFER handing in Arizona Core appears
> > to be correct.  Would you mind explaining what your concerns are in
> > more detail please?
> 
> Hi
> 
> That's more about handling -ENODEV rather than -EPROBE_DEFER.
> 
> Before the change, if GPIO chip supposed to provide "reset" pin was not ready 
> during  arizona_dev_init(), devm_gpiod_get() returned -ENODEV and device was 
> initialized in degraded mode, i.e., with no control over the "reset" pin.
> After the change, gpiod_get() will return -EPROBE_DEFER in such case and 
> arizona_dev_init() won't succeed in case the GPIO chip doesn't appear later 
> for some reason.

Yes, I see that now.  Thanks for your explanation.

I'm bringing in the big guns (CC'ed).

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Richard Fitzgerald]

On 05/07/18 21:56, Janusz Krzysztofik wrote:
> On Thursday, July 5, 2018 7:50:37 AM CEST Lee Jones wrote:
>> On Wed, 04 Jul 2018, Janusz Krzysztofik wrote:
>>> On Tuesday, July 3, 2018 7:31:41 PM CEST Boris Brezillon wrote:
>>>> Hi Janusz,
>>>>
>>>> On Tue,  3 Jul 2018 19:26:35 +0200
>>>>
>>>> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
>>>>> Avoid replication of error code conversion in non-DT GPIO consumers'
>>>>> code by returning -EPROBE_DEFER from gpiod_find() in case a chip
>>>>> identified by its label in a registered lookup table is not ready.
>>>>>
>>>>> See https://lkml.org/lkml/2018/5/30/176 for example case.
>>>>>
>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>>>> ---
>>>>> If accepted, please add
>>>>>
>>>>> 	Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
>>>>>
>>>>> if Boris doesn't mind.
>>>>>
>>>>> Thanks,
>>>>> Janusz
>>>>>
>>>>>   drivers/gpio/gpiolib.c | 13 ++++++++++---
>>>>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>>>> index e11a3bb03820..15dc77c80328 100644
>>>>> --- a/drivers/gpio/gpiolib.c
>>>>> +++ b/drivers/gpio/gpiolib.c
>>>>> @@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct
>>>>> device
>>>>> *dev, const char *con_id,>
>>>>>
>>>>>   		chip = find_chip_by_name(p->chip_label);
>>>>>   		
>>>>>   		if (!chip) {
>>>>>
>>>>> -			dev_err(dev, "cannot find GPIO chip %s\n",
>>>>> -				p->chip_label);
>>>>> -			return ERR_PTR(-ENODEV);
>>>>> +			/*
>>>>> +			 * As the lookup table indicates a chip with
>>>>> +			 * p->chip_label should exist, assume it may
>>>>> +			 * still appear latar and let the interested
>>>>>
>>>> 					^ later
>>>>>
>>>>> +			 * consumer be probed again or let the Deferred
>>>>> +			 * Probe infrastructure handle the error.
>>>>> +			 */
>>>>> +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
>>>>> +				 p->chip_label);
>>>>> +			return ERR_PTR(-EPROBE_DEFER);
>>>>>
>>>>>   		}
>>>>>   		
>>>>>   		if (chip->ngpio <= p->chip_hwnum) {
>>>>
>>>> Looks good otherwise. Let's hope we're not breaking implementations
>>>> testing for -ENODEV...
>>>
>>> I've reviewed them all and found two which I think may be affected:
>>> - drivers/mfd/arizona-core.c,
>>> - drivers/i2c/busses/i2c-imx.c.
>>> As far as I can understand the code, both depend on error != -EPROBE_DEFER
>>> in order to continue in degraded mode. I'm adding their maintainers to
>>> the loop.
>>  From a quick glance, the -EPROBE_DEFER handing in Arizona Core appears
>> to be correct.  Would you mind explaining what your concerns are in
>> more detail please?
> 
> Hi
> 
> That's more about handling -ENODEV rather than -EPROBE_DEFER.
> 
> Before the change, if GPIO chip supposed to provide "reset" pin was not ready
> during  arizona_dev_init(), devm_gpiod_get() returned -ENODEV and device was
> initialized in degraded mode, i.e., with no control over the "reset" pin.
> After the change, gpiod_get() will return -EPROBE_DEFER in such case and
> arizona_dev_init() won't succeed in case the GPIO chip doesn't appear later
> for some reason.
> 
> Thanks,
> Januszz
> 
> 

The intention is that if the DT node is missing, the Arizona driver can run
using only soft reset, though there are limitations in that mode.
This should return -ENOENT so that the Arizona driver will continue without
a GPIO.

If the DT defines a GPIO it is effectively saying that this GPIO is required so
it is valid for the Arizona driver never to come up if the GPIO it is defined to
depend on doesn't come up.

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Janusz Krzysztofik]

> >> On Wed, 04 Jul 2018, Janusz Krzysztofik wrote:
> >>> On Tuesday, July 3, 2018 7:31:41 PM CEST Boris Brezillon wrote:
> >>>> On Tue,  3 Jul 2018 19:26:35 +0200 Janusz Krzysztofik wrote:
> >>>>>   		chip = find_chip_by_name(p->chip_label);
> >>>>>   		if (!chip) {
> >>>>> -			dev_err(dev, "cannot find GPIO chip %s\n",
> >>>>> -				p->chip_label);
> >>>>> -			return ERR_PTR(-ENODEV);
> >>>>> +			/*
> >>>>> +			 * As the lookup table indicates a chip with
> >>>>> +			 * p->chip_label should exist, assume it may
> >>>>> +			 * still appear latar and let the interested
> >>>>> +			 * consumer be probed again or let the Deferred
> >>>>> +			 * Probe infrastructure handle the error.
> >>>>> +			 */
> >>>>> +			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> >>>>> +				 p->chip_label);
> >>>>> +			return ERR_PTR(-EPROBE_DEFER);
> >>>>>   		}
> >>>> 
> >>>> Looks good otherwise. Let's hope we're not breaking implementations
> >>>> testing for -ENODEV...
> >>> 
> >>> I've reviewed them all and found two which I think may be affected:
> >>> - drivers/mfd/arizona-core.c,
> >>> - drivers/i2c/busses/i2c-imx.c.

On Thursday, July 5, 2018 7:23:46 AM CEST Uwe Kleine-König wrote:
> TL;DR: Either I don't understand the implication for
> drivers/i2c/busses/i2c-imx.c or everything is fine.
> ...
> So if a patch changes devm_gpiod_get() to return -EPROBE_DEFER in more
> cases that doesn't seem to hurt. Moreover TTBOMK this driver should only
> be used by dt-machines today such that changing gpio* for non-DT users
> shouldn't affect it.

On Friday, July 6, 2018 11:03:53 AM CEST Richard Fitzgerald wrote:
> The intention is that if the DT node is missing, the Arizona driver can run
> using only soft reset, though there are limitations in that mode.
> This should return -ENOENT so that the Arizona driver will continue without
> a GPIO.
> 
> If the DT defines a GPIO it is effectively saying that this GPIO is required
> so it is valid for the Arizona driver never to come up if the GPIO it is
> defined to depend on doesn't come up.

Uwe, Richard, thanks for clarifications.

I think we can assume the change is safe for all current implementations.

Thanks,
Janusz

Re: [PATCH] gpiolib: Defer on non-DT find_chip_by_name() failure

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 8:26 PM, Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> Avoid replication of error code conversion in non-DT GPIO consumers'
> code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> identified by its label in a registered lookup table is not ready.
>
> See https://lkml.org/lkml/2018/5/30/176 for example case.

> +                       /*
> +                        * As the lookup table indicates a chip with
> +                        * p->chip_label should exist, assume it may
> +                        * still appear latar and let the interested

latar -> later

> +                        * consumer be probed again or let the Deferred
> +                        * Probe infrastructure handle the error.
> +                        */
> +                       dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
> +                                p->chip_label);
> +                       return ERR_PTR(-EPROBE_DEFER);
>                 }

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] gpiolib: Defer on non-DT find_chip_by_name() failure

[Janusz Krzysztofik]

Avoid replication of error code conversion in non-DT GPIO consumers'
code by returning -EPROBE_DEFER from gpiod_find() in case a chip
identified by its label in a registered lookup table is not ready.

See https://lkml.org/lkml/2018/5/30/176 for example case.

Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changelog
v2: fix typo (latar -> later) - thanks Boris and Andy for catching this

I'm not sure if adding both Suggested-by: and Reviewed-by: heades both
with the same person name is in line with good practices, please remove
one if not.

Thanks,
Janusz

 drivers/gpio/gpiolib.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..01295c03b315 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3639,9 +3639,16 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 		chip = find_chip_by_name(p->chip_label);
 
 		if (!chip) {
-			dev_err(dev, "cannot find GPIO chip %s\n",
-				p->chip_label);
-			return ERR_PTR(-ENODEV);
+			/*
+			 * As the lookup table indicates a chip with
+			 * p->chip_label should exist, assume it may
+			 * still appear later and let the interested
+			 * consumer be probed again or let the Deferred
+			 * Probe infrastructure handle the error.
+			 */
+			dev_warn(dev, "cannot find GPIO chip %s, deferring\n",
+				 p->chip_label);
+			return ERR_PTR(-EPROBE_DEFER);
 		}
 
 		if (chip->ngpio <= p->chip_hwnum) {
-- 
2.16.4

Re: [PATCH v2] gpiolib: Defer on non-DT find_chip_by_name() failure

[Boris Brezillon]

On Wed,  4 Jul 2018 00:18:19 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Avoid replication of error code conversion in non-DT GPIO consumers'
> code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> identified by its label in a registered lookup table is not ready.
> 
> See https://lkml.org/lkml/2018/5/30/176 for example case.
> 
> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Changelog
> v2: fix typo (latar -> later) - thanks Boris and Andy for catching this
> 
> I'm not sure if adding both Suggested-by: and Reviewed-by: heades both
> with the same person name is in line with good practices, please remove
> one if not.

I don't think that's a problem. Suggesting a solution and agreeing on
the implementation are 2 different things, so both are not mutually
exclusive IMO.

Re: [PATCH v2] gpiolib: Defer on non-DT find_chip_by_name() failure

[Linus Walleij]

On Wed, Jul 4, 2018 at 12:18 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Avoid replication of error code conversion in non-DT GPIO consumers'
> code by returning -EPROBE_DEFER from gpiod_find() in case a chip
> identified by its label in a registered lookup table is not ready.
>
> See https://lkml.org/lkml/2018/5/30/176 for example case.
>
> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Changelog
> v2: fix typo (latar -> later) - thanks Boris and Andy for catching this

Patch applied as the discussion seems to conclude this should
work fine, I hope we will notice if it doesn't!

Yours,
Linus Walleij