[5/6,v2] mtd: rawnand: ams-delta: use GPIO lookup table

Message ID 20180525222046.11200-1-jmkrzyszt@gmail.com
State Changes Requested
Headers show
Series
  • Untitled series #46774
Related show

Commit Message

Janusz Krzysztofik May 25, 2018, 10:20 p.m.
Now as the Amstrad Delta board provides GPIO lookup tables, switch from
GPIO numbers to GPIO descriptors and use the table to locate required
GPIO pins.

Declare static variables for storing GPIO descriptors and replace
gpio_ functions with their gpiod_ equivalents. Return -EPROBE_DEFER
if the GPIO pins are not yet available so device initialization is
postponed instead of aborted.

Pin naming used by the driver should be followed while respective GPIO
lookup table is initialized by a board init code.

Created and tested against linux-4.17-rc3, on top of patch 1/6 "ARM:
OMAP1: ams-delta: add GPIO lookup tables" (already applied to
omap-for-v4.18/soc tree).

Changes since v1:
- fix handling of devm_gpiod_get_optional() return values - thanks to
  Andy Shevchenko.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/mtd/nand/raw/ams-delta.c | 120 +++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 56 deletions(-)

Comments

Boris Brezillon May 30, 2018, 9:05 a.m. | #1
Hi Janusz,

On Sat, 26 May 2018 00:20:45 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Now as the Amstrad Delta board provides GPIO lookup tables, switch from
> GPIO numbers to GPIO descriptors and use the table to locate required
> GPIO pins.
> 
> Declare static variables for storing GPIO descriptors and replace
> gpio_ functions with their gpiod_ equivalents. Return -EPROBE_DEFER
> if the GPIO pins are not yet available so device initialization is
> postponed instead of aborted.
> 
> Pin naming used by the driver should be followed while respective GPIO
> lookup table is initialized by a board init code.
> 
> Created and tested against linux-4.17-rc3, on top of patch 1/6 "ARM:
> OMAP1: ams-delta: add GPIO lookup tables" (already applied to
> omap-for-v4.18/soc tree).
> 
> Changes since v1:
> - fix handling of devm_gpiod_get_optional() return values - thanks to
>   Andy Shevchenko.

Can you put the changelog after the "---" separator so that it does not
appear in the final commit message?

> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/mtd/nand/raw/ams-delta.c | 120 +++++++++++++++++++++------------------
>  1 file changed, 64 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
> index 37a3cc21c7bc..524ceaf12de0 100644
> --- a/drivers/mtd/nand/raw/ams-delta.c
> +++ b/drivers/mtd/nand/raw/ams-delta.c
> @@ -20,23 +20,28 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/rawnand.h>
>  #include <linux/mtd/partitions.h>
> -#include <linux/gpio.h>
>  #include <linux/platform_data/gpio-omap.h>
>  
>  #include <asm/io.h>
>  #include <asm/sizes.h>
>  
> -#include <mach/board-ams-delta.h>
> -
>  #include <mach/hardware.h>
>  
>  /*
>   * MTD structure for E3 (Delta)
>   */
>  static struct mtd_info *ams_delta_mtd = NULL;
> +static struct gpio_desc *gpiod_rdy;
> +static struct gpio_desc *gpiod_nce;
> +static struct gpio_desc *gpiod_nre;
> +static struct gpio_desc *gpiod_nwp;
> +static struct gpio_desc *gpiod_nwe;
> +static struct gpio_desc *gpiod_ale;
> +static struct gpio_desc *gpiod_cle;
>  
>  /*
>   * Define partitions for flash devices
> @@ -70,9 +75,9 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
>  
>  	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
>  	writew(byte, this->IO_ADDR_W);
> -	gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NWE, 0);
> +	gpiod_set_value(gpiod_nwe, 0);
>  	ndelay(40);
> -	gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NWE, 1);
> +	gpiod_set_value(gpiod_nwe, 1);
>  }
>  
>  static u_char ams_delta_read_byte(struct mtd_info *mtd)
> @@ -81,11 +86,11 @@ static u_char ams_delta_read_byte(struct mtd_info *mtd)
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	void __iomem *io_base = (void __iomem *)nand_get_controller_data(this);
>  
> -	gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NRE, 0);
> +	gpiod_set_value(gpiod_nre, 0);
>  	ndelay(40);
>  	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
>  	res = readw(this->IO_ADDR_R);
> -	gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NRE, 1);
> +	gpiod_set_value(gpiod_nre, 1);
>  
>  	return res;
>  }
> @@ -120,12 +125,9 @@ static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
>  {
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
> -		gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NCE,
> -				(ctrl & NAND_NCE) == 0);
> -		gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_CLE,
> -				(ctrl & NAND_CLE) != 0);
> -		gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_ALE,
> -				(ctrl & NAND_ALE) != 0);
> +		gpiod_set_value(gpiod_nce, !(ctrl & NAND_NCE));
> +		gpiod_set_value(gpiod_cle, !!(ctrl & NAND_CLE));
> +		gpiod_set_value(gpiod_ale, !!(ctrl & NAND_ALE));
>  	}
>  
>  	if (cmd != NAND_CMD_NONE)
> @@ -134,41 +136,9 @@ static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
>  
>  static int ams_delta_nand_ready(struct mtd_info *mtd)
>  {
> -	return gpio_get_value(AMS_DELTA_GPIO_PIN_NAND_RB);
> +	return gpiod_get_value(gpiod_rdy);
>  }
>  
> -static const struct gpio _mandatory_gpio[] = {
> -	{
> -		.gpio	= AMS_DELTA_GPIO_PIN_NAND_NCE,
> -		.flags	= GPIOF_OUT_INIT_HIGH,
> -		.label	= "nand_nce",
> -	},
> -	{
> -		.gpio	= AMS_DELTA_GPIO_PIN_NAND_NRE,
> -		.flags	= GPIOF_OUT_INIT_HIGH,
> -		.label	= "nand_nre",
> -	},
> -	{
> -		.gpio	= AMS_DELTA_GPIO_PIN_NAND_NWP,
> -		.flags	= GPIOF_OUT_INIT_HIGH,
> -		.label	= "nand_nwp",
> -	},
> -	{
> -		.gpio	= AMS_DELTA_GPIO_PIN_NAND_NWE,
> -		.flags	= GPIOF_OUT_INIT_HIGH,
> -		.label	= "nand_nwe",
> -	},
> -	{
> -		.gpio	= AMS_DELTA_GPIO_PIN_NAND_ALE,
> -		.flags	= GPIOF_OUT_INIT_LOW,
> -		.label	= "nand_ale",
> -	},
> -	{
> -		.gpio	= AMS_DELTA_GPIO_PIN_NAND_CLE,
> -		.flags	= GPIOF_OUT_INIT_LOW,
> -		.label	= "nand_cle",
> -	},
> -};
>  
>  /*
>   * Main initialization routine
> @@ -216,12 +186,17 @@ static int ams_delta_init(struct platform_device *pdev)
>  	this->write_buf = ams_delta_write_buf;
>  	this->read_buf = ams_delta_read_buf;
>  	this->cmd_ctrl = ams_delta_hwcontrol;
> -	if (gpio_request(AMS_DELTA_GPIO_PIN_NAND_RB, "nand_rdy") == 0) {
> -		this->dev_ready = ams_delta_nand_ready;
> -	} else {
> -		this->dev_ready = NULL;
> -		pr_notice("Couldn't request gpio for Delta NAND ready.\n");
> +
> +	gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
> +	if (IS_ERR(gpiod_rdy)) {
> +		err = PTR_ERR(gpiod_rdy);
> +		dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err);
> +		goto err_gpiod;
>  	}
> +
> +	if (gpiod_rdy)
> +		this->dev_ready = ams_delta_nand_ready;
> +
>  	/* 25 us command delay time */
>  	this->chip_delay = 30;
>  	this->ecc.mode = NAND_ECC_SOFT;
> @@ -230,7 +205,44 @@ static int ams_delta_init(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, io_base);
>  
>  	/* Set chip enabled, but  */
> -	err = gpio_request_array(_mandatory_gpio, ARRAY_SIZE(_mandatory_gpio));
> +	gpiod_nwp = devm_gpiod_get(&pdev->dev, "nwp", GPIOD_OUT_HIGH);
> +	if (IS_ERR(gpiod_nwp)) {
> +		err = PTR_ERR(gpiod_nwp);
> +		dev_err(&pdev->dev, "NWP GPIO request failed (%d)\n", err);
> +		goto err_gpiod;
> +	}
> +	gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
> +	if (IS_ERR(gpiod_nce)) {
> +		err = PTR_ERR(gpiod_nce);
> +		dev_err(&pdev->dev, "NCE GPIO request failed (%d)\n", err);
> +		goto err_gpiod;
> +	}
> +	gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
> +	if (IS_ERR(gpiod_nre)) {
> +		err = PTR_ERR(gpiod_nre);
> +		dev_err(&pdev->dev, "NRE GPIO request failed (%d)\n", err);
> +		goto err_gpiod;
> +	}
> +	gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
> +	if (IS_ERR(gpiod_nwe)) {
> +		err = PTR_ERR(gpiod_nwe);
> +		dev_err(&pdev->dev, "NWE GPIO request failed (%d)\n", err);
> +		goto err_gpiod;
> +	}
> +	gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod_ale)) {
> +		err = PTR_ERR(gpiod_ale);
> +		dev_err(&pdev->dev, "ALE GPIO request failed (%d)\n", err);
> +		goto err_gpiod;
> +	}
> +	gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod_cle)) {
> +		err = PTR_ERR(gpiod_cle);
> +		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
> +	}
> +err_gpiod:
> +	if (err == -ENODEV || err == -ENOENT)
> +		err = -EPROBE_DEFER;

Hm, isn't it better to make gpiod_find() return ERR_PTR(-EPROBE_DEFER)
here [1]? At least, ENOENT should not be turned into EPROBE_DEFER,
because it's returned when there's no entry matching the requested gpio
in the lookup table, and deferring the probe won't solve this problem.

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/gpio/gpiolib.c#L3525
Janusz Krzysztofik May 30, 2018, 5:43 p.m. | #2
On Wednesday, May 30, 2018 11:05:00 AM CEST Boris Brezillon wrote:
> Hi Janusz,

Hi Boris,

> On Sat, 26 May 2018 00:20:45 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > ...
> > Changes since v1:
> > - fix handling of devm_gpiod_get_optional() return values - thanks to
> >   Andy Shevchenko.
> 
> Can you put the changelog after the "---" separator so that it does not
> appear in the final commit message?

Yes, sure, sorry for that.

> > +err_gpiod:
> > +	if (err == -ENODEV || err == -ENOENT)
> > +		err = -EPROBE_DEFER;
> 
> Hm, isn't it better to make gpiod_find() return ERR_PTR(-EPROBE_DEFER)
> here [1]? At least, ENOENT should not be turned into EPROBE_DEFER,
> because it's returned when there's no entry matching the requested gpio
> in the lookup table, and deferring the probe won't solve this problem.

ENOENT is also returned when no matching lookup table is found. That may 
happen if consumer dev_name stored in the table differs from dev_name assigned 
to the consumer by its bus, the platform bus in this case. For that reason I 
think the consumer dev_name should be initialized in the table after the 
device is registered, when its actual dev_name can be obtained. If that device 
registration happens after the driver is already registered, e.g., at 
late_initcall, the device is probed before its lookup table is ready. For that 
reason returning EPROBE_DEFER seems better to me even in the ENOENT case.

Thanks,
Janusz
Boris Brezillon May 30, 2018, 5:52 p.m. | #3
On Wed, 30 May 2018 19:43:09 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> On Wednesday, May 30, 2018 11:05:00 AM CEST Boris Brezillon wrote:
> > Hi Janusz,  
> 
> Hi Boris,
> 
> > On Sat, 26 May 2018 00:20:45 +0200
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:  
> > > ...
> > > Changes since v1:
> > > - fix handling of devm_gpiod_get_optional() return values - thanks to
> > >   Andy Shevchenko.  
> > 
> > Can you put the changelog after the "---" separator so that it does not
> > appear in the final commit message?  
> 
> Yes, sure, sorry for that.
> 
> > > +err_gpiod:
> > > +	if (err == -ENODEV || err == -ENOENT)
> > > +		err = -EPROBE_DEFER;  
> > 
> > Hm, isn't it better to make gpiod_find() return ERR_PTR(-EPROBE_DEFER)
> > here [1]? At least, ENOENT should not be turned into EPROBE_DEFER,
> > because it's returned when there's no entry matching the requested gpio
> > in the lookup table, and deferring the probe won't solve this problem.  
> 
> ENOENT is also returned when no matching lookup table is found. That may 
> happen if consumer dev_name stored in the table differs from dev_name assigned 
> to the consumer by its bus, the platform bus in this case. For that reason I 
> think the consumer dev_name should be initialized in the table after the 
> device is registered, when its actual dev_name can be obtained. If that device 
> registration happens after the driver is already registered, e.g., at 
> late_initcall, the device is probed before its lookup table is ready. For that 
> reason returning EPROBE_DEFER seems better to me even in the ENOENT case.

Sorry, I don't get it. Aren't GPIO lookup tables supposed to be declared
in board files, especially if the GPIO is used by a platform device?
When would you have a lookup table registered later in the init/boot
process?
Janusz Krzysztofik May 30, 2018, 8:39 p.m. | #4
On Wednesday, May 30, 2018 7:52:20 PM CEST Boris Brezillon wrote:
> On Wed, 30 May 2018 19:43:09 +0200
> 
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > On Wednesday, May 30, 2018 11:05:00 AM CEST Boris Brezillon wrote:
> > > Hi Janusz,
> > 
> > Hi Boris,
> > 
> > > On Sat, 26 May 2018 00:20:45 +0200
> > > 
> > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > > > ...
> > > > Changes since v1:
> > > > - fix handling of devm_gpiod_get_optional() return values - thanks to
> > > > 
> > > >   Andy Shevchenko.
> > > 
> > > Can you put the changelog after the "---" separator so that it does not
> > > appear in the final commit message?
> > 
> > Yes, sure, sorry for that.
> > 
> > > > +err_gpiod:
> > > > +	if (err == -ENODEV || err == -ENOENT)
> > > > +		err = -EPROBE_DEFER;
> > > 
> > > Hm, isn't it better to make gpiod_find() return ERR_PTR(-EPROBE_DEFER)
> > > here [1]? At least, ENOENT should not be turned into EPROBE_DEFER,
> > > because it's returned when there's no entry matching the requested gpio
> > > in the lookup table, and deferring the probe won't solve this problem.
> > 
> > ENOENT is also returned when no matching lookup table is found. That may
> > happen if consumer dev_name stored in the table differs from dev_name
> > assigned to the consumer by its bus, the platform bus in this case. For
> > that reason I think the consumer dev_name should be initialized in the
> > table after the device is registered, when its actual dev_name can be
> > obtained. If that device registration happens after the driver is already
> > registered, e.g., at late_initcall, the device is probed before its
> > lookup table is ready. For that reason returning EPROBE_DEFER seems
> > better to me even in the ENOENT case.
> Sorry, I don't get it. Aren't GPIO lookup tables supposed to be declared
> in board files, especially if the GPIO is used by a platform device?
> When would you have a lookup table registered later in the init/boot
> process?

When e.g. I'd like to register my GPIO consumer platform device at 
late_initcall for some reason, and I'm not sure what exact dev_name my 
consomer will be registered with by the platform bus. In that case I think I 
should assign dev_name to the lookup table after the consumer device is 
registered and its exact dev_name can be obtained, then register the table,

Am I missing something?

Thanks,
Janusz
Boris Brezillon June 4, 2018, 9:55 a.m. | #5
On Wed, 30 May 2018 22:39:03 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> On Wednesday, May 30, 2018 7:52:20 PM CEST Boris Brezillon wrote:
> > On Wed, 30 May 2018 19:43:09 +0200
> > 
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:  
> > > On Wednesday, May 30, 2018 11:05:00 AM CEST Boris Brezillon wrote:  
> > > > Hi Janusz,  
> > > 
> > > Hi Boris,
> > >   
> > > > On Sat, 26 May 2018 00:20:45 +0200
> > > > 
> > > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:  
> > > > > ...
> > > > > Changes since v1:
> > > > > - fix handling of devm_gpiod_get_optional() return values - thanks to
> > > > > 
> > > > >   Andy Shevchenko.  
> > > > 
> > > > Can you put the changelog after the "---" separator so that it does not
> > > > appear in the final commit message?  
> > > 
> > > Yes, sure, sorry for that.
> > >   
> > > > > +err_gpiod:
> > > > > +	if (err == -ENODEV || err == -ENOENT)
> > > > > +		err = -EPROBE_DEFER;  
> > > > 
> > > > Hm, isn't it better to make gpiod_find() return ERR_PTR(-EPROBE_DEFER)
> > > > here [1]? At least, ENOENT should not be turned into EPROBE_DEFER,
> > > > because it's returned when there's no entry matching the requested gpio
> > > > in the lookup table, and deferring the probe won't solve this problem.  
> > > 
> > > ENOENT is also returned when no matching lookup table is found. That may
> > > happen if consumer dev_name stored in the table differs from dev_name
> > > assigned to the consumer by its bus, the platform bus in this case. For
> > > that reason I think the consumer dev_name should be initialized in the
> > > table after the device is registered, when its actual dev_name can be
> > > obtained. If that device registration happens after the driver is already
> > > registered, e.g., at late_initcall, the device is probed before its
> > > lookup table is ready. For that reason returning EPROBE_DEFER seems
> > > better to me even in the ENOENT case.  
> > Sorry, I don't get it. Aren't GPIO lookup tables supposed to be declared
> > in board files, especially if the GPIO is used by a platform device?
> > When would you have a lookup table registered later in the init/boot
> > process?  
> 
> When e.g. I'd like to register my GPIO consumer platform device at 
> late_initcall for some reason, and I'm not sure what exact dev_name my 
> consomer will be registered with by the platform bus.

You should know the name before the device is registered.

> In that case I think I 
> should assign dev_name to the lookup table after the consumer device is 
> registered and its exact dev_name can be obtained, then register the table,

I'm pretty sure it's not supposed to work like that. Resources attached
to a device should be defined before the device is registered, not
after, simply because when you call platform_device_register(), the
device might be directly bind to the driver before the
platform_device_register() calls return, and the driver will fail to
probe the device if it doesn't find the GPIO it needs.
Janusz Krzysztofik June 4, 2018, 4:48 p.m. | #6
On Monday, June 4, 2018 11:55:43 AM CEST Boris Brezillon wrote:
> On Wed, 30 May 2018 22:39:03 +0200
> 
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > On Wednesday, May 30, 2018 7:52:20 PM CEST Boris Brezillon wrote:
> > > On Wed, 30 May 2018 19:43:09 +0200
> > > 
> > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > > > On Wednesday, May 30, 2018 11:05:00 AM CEST Boris Brezillon wrote:
> > > > > Hi Janusz,
> > > > 
> > > > Hi Boris,
> > > > 
> > > > > On Sat, 26 May 2018 00:20:45 +0200
> > > > > 
> > > > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> > > > > > ...
> > > > > > Changes since v1:
> > > > > > - fix handling of devm_gpiod_get_optional() return values - thanks
> > > > > > to
> > > > > > 
> > > > > >   Andy Shevchenko.
> > > > > 
> > > > > Can you put the changelog after the "---" separator so that it does
> > > > > not
> > > > > appear in the final commit message?
> > > > 
> > > > Yes, sure, sorry for that.
> > > > 
> > > > > > +err_gpiod:
> > > > > > +	if (err == -ENODEV || err == -ENOENT)
> > > > > > +		err = -EPROBE_DEFER;
> > > > > 
> > > > > Hm, isn't it better to make gpiod_find() return
> > > > > ERR_PTR(-EPROBE_DEFER)
> > > > > here [1]? At least, ENOENT should not be turned into EPROBE_DEFER,
> > > > > because it's returned when there's no entry matching the requested
> > > > > gpio
> > > > > in the lookup table, and deferring the probe won't solve this
> > > > > problem.
> > > > 
> > > > ENOENT is also returned when no matching lookup table is found. That
> > > > may
> > > > happen if consumer dev_name stored in the table differs from dev_name
> > > > assigned to the consumer by its bus, the platform bus in this case.
> > > > For
> > > > that reason I think the consumer dev_name should be initialized in the
> > > > table after the device is registered, when its actual dev_name can be
> > > > obtained. If that device registration happens after the driver is
> > > > already
> > > > registered, e.g., at late_initcall, the device is probed before its
> > > > lookup table is ready. For that reason returning EPROBE_DEFER seems
> > > > better to me even in the ENOENT case.
> > > 
> > > Sorry, I don't get it. Aren't GPIO lookup tables supposed to be declared
> > > in board files, especially if the GPIO is used by a platform device?
> > > When would you have a lookup table registered later in the init/boot
> > > process?
> > 
> > When e.g. I'd like to register my GPIO consumer platform device at
> > late_initcall for some reason, and I'm not sure what exact dev_name my
> > consomer will be registered with by the platform bus.
> 
> You should know the name before the device is registered.

What if I use PLATFORM_DEVID_AUTO?

For other cases, if bus specific names of devices were supposed to be known 
before registration, bus drivers should export functions returning those names 
from initialized bus specific device structures or their components while they 
don't. Under such circumstances, we end up hardcoding device names based on 
our knowledge of bus internals if we need to specify them in advance, and 
those internals are not guaranteed to never change.

> > In that case I think I
> > should assign dev_name to the lookup table after the consumer device is
> > registered and its exact dev_name can be obtained, then register the
> > table,
> 
> I'm pretty sure it's not supposed to work like that. Resources attached
> to a device should be defined before the device is registered, not
> after,

What do you mean by resources attached to a device? I don't think we should 
consider GPIO lookup tables as consumer device resources. Those tables are 
registered separately from consumer device registration and I know of no 
requirement for registering them in advance. Maybe I'm missing something.

Let's have a look at regulators. There are no separately registered regulator 
lookup tables, instead regulator consumer supply tables are attached to bus 
specific device structures of regulator devices, not their consumers, hence 
registered together with providers, not consumers. Will you still call those 
tables 'resources attached to' consumers? 

As far as I can see, regulator_get() never returns -EINVAL, only -ENODEV or -
EPROBE_DEFER. However, gpiod_get() can also return -EINVAL. Maybe it 
shouldn't, but it does, and I'm just trying to adopt to that in order to not 
break a driver I'm trying to update. 

> simply because when you call platform_device_register(), the
> device might be directly bind to the driver before the
> platform_device_register() calls return, and the driver will fail to
> probe the device if it doesn't find the GPIO it needs.

That's exactly the case I'm talking about, but my conclusion is different: the 
driver should fail softly so the device is probed again later, as long as I'm 
not wrong the no requirement exists for registering GPIO lookup tables before 
related consumers are registered.

If I' missing something or you are still not convinced, I'll try to resolve 
issues with the device I see in a different way and submit a new patch that 
hopefully matches your requirements.

Thanks,
Janusz
Boris Brezillon June 4, 2018, 11:09 p.m. | #7
On Mon, 04 Jun 2018 18:48:08 +0200
Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> On Monday, June 4, 2018 11:55:43 AM CEST Boris Brezillon wrote:
> > On Wed, 30 May 2018 22:39:03 +0200
> > 
> > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:  
> > > On Wednesday, May 30, 2018 7:52:20 PM CEST Boris Brezillon wrote:  
> > > > On Wed, 30 May 2018 19:43:09 +0200
> > > > 
> > > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:  
> > > > > On Wednesday, May 30, 2018 11:05:00 AM CEST Boris Brezillon wrote:  
> > > > > > Hi Janusz,  
> > > > > 
> > > > > Hi Boris,
> > > > >   
> > > > > > On Sat, 26 May 2018 00:20:45 +0200
> > > > > > 
> > > > > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:  
> > > > > > > ...
> > > > > > > Changes since v1:
> > > > > > > - fix handling of devm_gpiod_get_optional() return values - thanks
> > > > > > > to
> > > > > > > 
> > > > > > >   Andy Shevchenko.  
> > > > > > 
> > > > > > Can you put the changelog after the "---" separator so that it does
> > > > > > not
> > > > > > appear in the final commit message?  
> > > > > 
> > > > > Yes, sure, sorry for that.
> > > > >   
> > > > > > > +err_gpiod:
> > > > > > > +	if (err == -ENODEV || err == -ENOENT)
> > > > > > > +		err = -EPROBE_DEFER;  
> > > > > > 
> > > > > > Hm, isn't it better to make gpiod_find() return
> > > > > > ERR_PTR(-EPROBE_DEFER)
> > > > > > here [1]? At least, ENOENT should not be turned into EPROBE_DEFER,
> > > > > > because it's returned when there's no entry matching the requested
> > > > > > gpio
> > > > > > in the lookup table, and deferring the probe won't solve this
> > > > > > problem.  
> > > > > 
> > > > > ENOENT is also returned when no matching lookup table is found. That
> > > > > may
> > > > > happen if consumer dev_name stored in the table differs from dev_name
> > > > > assigned to the consumer by its bus, the platform bus in this case.
> > > > > For
> > > > > that reason I think the consumer dev_name should be initialized in the
> > > > > table after the device is registered, when its actual dev_name can be
> > > > > obtained. If that device registration happens after the driver is
> > > > > already
> > > > > registered, e.g., at late_initcall, the device is probed before its
> > > > > lookup table is ready. For that reason returning EPROBE_DEFER seems
> > > > > better to me even in the ENOENT case.  
> > > > 
> > > > Sorry, I don't get it. Aren't GPIO lookup tables supposed to be declared
> > > > in board files, especially if the GPIO is used by a platform device?
> > > > When would you have a lookup table registered later in the init/boot
> > > > process?  
> > > 
> > > When e.g. I'd like to register my GPIO consumer platform device at
> > > late_initcall for some reason, and I'm not sure what exact dev_name my
> > > consomer will be registered with by the platform bus.  
> > 
> > You should know the name before the device is registered.  
> 
> What if I use PLATFORM_DEVID_AUTO?

In this case, the id assigned to the device will be dependent on the
device registration order, which in case of C board files should be
predictable. Am I missing something?

> 
> For other cases, if bus specific names of devices were supposed to be known 
> before registration, bus drivers should export functions returning those names 
> from initialized bus specific device structures or their components while they 
> don't. Under such circumstances, we end up hardcoding device names based on 
> our knowledge of bus internals if we need to specify them in advance, and 
> those internals are not guaranteed to never change.

But, when it comes to C board files, device names are known ahead of
time, right? And the only use case I see for those gpio_lookup_table is
non-DT platforms.

I had a quick look at a few call-sites of gpiod_add_lookup_table() and
couldn't find an example where the gpio lookup table was registered
after the platform device it was meant to be used by. If you have such
an example, can you point it to me?

> 
> > > In that case I think I
> > > should assign dev_name to the lookup table after the consumer device is
> > > registered and its exact dev_name can be obtained, then register the
> > > table,  
> > 
> > I'm pretty sure it's not supposed to work like that. Resources attached
> > to a device should be defined before the device is registered, not
> > after,  
> 
> What do you mean by resources attached to a device? I don't think we should 
> consider GPIO lookup tables as consumer device resources.

Why? If the device requires a GPIO to be controlled by the SoC, why
shouldn't we consider the GPIO as a resource. Sure, it's not part of
the struct resource array attached to the platform_device, but it's
still a resource that needs to be available for the driver to correctly
probe the device.

> Those tables are 
> registered separately from consumer device registration and I know of no 
> requirement for registering them in advance. Maybe I'm missing something.

But that doesn't make sense. Why would you register a device, and only
then attach it the description of the GPIOs it needs. Think about
the DT case, and imagine you support DT overlays, it's like having an
overlay that defines your device, and then another overlay that adds the
xx-gpios props to this device on top.

> 
> Let's have a look at regulators. There are no separately registered regulator 
> lookup tables, instead regulator consumer supply tables are attached to bus 
> specific device structures of regulator devices, not their consumers, hence 
> registered together with providers, not consumers. Will you still call those 
> tables 'resources attached to' consumers?

Except the gpio-lookup tables are not here to describe GPIOs, but to
give consumer-oriented friendly names so that the consumer can then do

	 devm_gpiod_get(&pdev->dev, "foo", GPIOD_OUT_LOW);

Here "foo" is only meaningful to pdev, and not globally exposed, and
another driver might pretty well request the exact same GPIO using a
different name.

> 
> As far as I can see, regulator_get() never returns -EINVAL, only -ENODEV or -
> EPROBE_DEFER. However, gpiod_get() can also return -EINVAL. Maybe it 
> shouldn't, but it does, and I'm just trying to adopt to that in order to not 
> break a driver I'm trying to update.

Well, IMO it should return EINVAL if no entry in the registered lookup
tables is matching the requested "dev_name(dev) + con_id" pair. I'm
bringing back my analogy with the DT case. What you're suggesting
would be similar to waiting for a new xxx-gpios prop to magically
appear in the DT.

Note that the GPIO itself is not necessarily ready to be used when the
consumer calls devm_gpio_get(), because the GPIO chip might not be
probed yet or it might be missing a dependency. And that's all fine, we
have EPROBE_DEFER for that. But that's different than saying "we don't
have a GPIO description attached to the device, let's wait for someone
to create one".

> 
> > simply because when you call platform_device_register(), the
> > device might be directly bind to the driver before the
> > platform_device_register() calls return, and the driver will fail to
> > probe the device if it doesn't find the GPIO it needs.  
> 
> That's exactly the case I'm talking about, but my conclusion is different: the 
> driver should fail softly so the device is probed again later, as long as I'm 
> not wrong the no requirement exists for registering GPIO lookup tables before 
> related consumers are registered.

How long. Why should we wait only after late init calls? What if the
lookup table is created after that?

> 
> If I' missing something or you are still not convinced, I'll try to resolve 
> issues with the device I see in a different way and submit a new patch that 
> hopefully matches your requirements.

Maybe I'm wrong, but I still think that registering a lookup table
after the pdev it will be used by is a bad idea.

Regards,

Boris
Boris Brezillon June 4, 2018, 11:30 p.m. | #8
On Tue, 5 Jun 2018 01:09:32 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Mon, 04 Jun 2018 18:48:08 +0200
> Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> 
> > On Monday, June 4, 2018 11:55:43 AM CEST Boris Brezillon wrote:  
> > > On Wed, 30 May 2018 22:39:03 +0200
> > > 
> > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:    
> > > > On Wednesday, May 30, 2018 7:52:20 PM CEST Boris Brezillon wrote:    
> > > > > On Wed, 30 May 2018 19:43:09 +0200
> > > > > 
> > > > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:    
> > > > > > On Wednesday, May 30, 2018 11:05:00 AM CEST Boris Brezillon wrote:    
> > > > > > > Hi Janusz,    
> > > > > > 
> > > > > > Hi Boris,
> > > > > >     
> > > > > > > On Sat, 26 May 2018 00:20:45 +0200
> > > > > > > 
> > > > > > > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:    
> > > > > > > > ...
> > > > > > > > Changes since v1:
> > > > > > > > - fix handling of devm_gpiod_get_optional() return values - thanks
> > > > > > > > to
> > > > > > > > 
> > > > > > > >   Andy Shevchenko.    
> > > > > > > 
> > > > > > > Can you put the changelog after the "---" separator so that it does
> > > > > > > not
> > > > > > > appear in the final commit message?    
> > > > > > 
> > > > > > Yes, sure, sorry for that.
> > > > > >     
> > > > > > > > +err_gpiod:
> > > > > > > > +	if (err == -ENODEV || err == -ENOENT)
> > > > > > > > +		err = -EPROBE_DEFER;    
> > > > > > > 
> > > > > > > Hm, isn't it better to make gpiod_find() return
> > > > > > > ERR_PTR(-EPROBE_DEFER)
> > > > > > > here [1]? At least, ENOENT should not be turned into EPROBE_DEFER,
> > > > > > > because it's returned when there's no entry matching the requested
> > > > > > > gpio
> > > > > > > in the lookup table, and deferring the probe won't solve this
> > > > > > > problem.    
> > > > > > 
> > > > > > ENOENT is also returned when no matching lookup table is found. That
> > > > > > may
> > > > > > happen if consumer dev_name stored in the table differs from dev_name
> > > > > > assigned to the consumer by its bus, the platform bus in this case.
> > > > > > For
> > > > > > that reason I think the consumer dev_name should be initialized in the
> > > > > > table after the device is registered, when its actual dev_name can be
> > > > > > obtained. If that device registration happens after the driver is
> > > > > > already
> > > > > > registered, e.g., at late_initcall, the device is probed before its
> > > > > > lookup table is ready. For that reason returning EPROBE_DEFER seems
> > > > > > better to me even in the ENOENT case.    
> > > > > 
> > > > > Sorry, I don't get it. Aren't GPIO lookup tables supposed to be declared
> > > > > in board files, especially if the GPIO is used by a platform device?
> > > > > When would you have a lookup table registered later in the init/boot
> > > > > process?    
> > > > 
> > > > When e.g. I'd like to register my GPIO consumer platform device at
> > > > late_initcall for some reason, and I'm not sure what exact dev_name my
> > > > consomer will be registered with by the platform bus.    
> > > 
> > > You should know the name before the device is registered.    
> > 
> > What if I use PLATFORM_DEVID_AUTO?

Just had a quick look at board-ams-delta.c and you don't have a single
device setting pdev->id to PLATFORM_DEVID_AUTO. It's either set to
PLATFORM_DEVID_NONE (-1) or assigned a specific id, so the problem does
not exist, really. Just set the name .dev_id to the appropriate value
at declaration time and register the lookup tables before registering
the pdevs.

Patch

diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c
index 37a3cc21c7bc..524ceaf12de0 100644
--- a/drivers/mtd/nand/raw/ams-delta.c
+++ b/drivers/mtd/nand/raw/ams-delta.c
@@ -20,23 +20,28 @@ 
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
-#include <linux/gpio.h>
 #include <linux/platform_data/gpio-omap.h>
 
 #include <asm/io.h>
 #include <asm/sizes.h>
 
-#include <mach/board-ams-delta.h>
-
 #include <mach/hardware.h>
 
 /*
  * MTD structure for E3 (Delta)
  */
 static struct mtd_info *ams_delta_mtd = NULL;
+static struct gpio_desc *gpiod_rdy;
+static struct gpio_desc *gpiod_nce;
+static struct gpio_desc *gpiod_nre;
+static struct gpio_desc *gpiod_nwp;
+static struct gpio_desc *gpiod_nwe;
+static struct gpio_desc *gpiod_ale;
+static struct gpio_desc *gpiod_cle;
 
 /*
  * Define partitions for flash devices
@@ -70,9 +75,9 @@  static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte)
 
 	writew(0, io_base + OMAP_MPUIO_IO_CNTL);
 	writew(byte, this->IO_ADDR_W);
-	gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NWE, 0);
+	gpiod_set_value(gpiod_nwe, 0);
 	ndelay(40);
-	gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NWE, 1);
+	gpiod_set_value(gpiod_nwe, 1);
 }
 
 static u_char ams_delta_read_byte(struct mtd_info *mtd)
@@ -81,11 +86,11 @@  static u_char ams_delta_read_byte(struct mtd_info *mtd)
 	struct nand_chip *this = mtd_to_nand(mtd);
 	void __iomem *io_base = (void __iomem *)nand_get_controller_data(this);
 
-	gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NRE, 0);
+	gpiod_set_value(gpiod_nre, 0);
 	ndelay(40);
 	writew(~0, io_base + OMAP_MPUIO_IO_CNTL);
 	res = readw(this->IO_ADDR_R);
-	gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NRE, 1);
+	gpiod_set_value(gpiod_nre, 1);
 
 	return res;
 }
@@ -120,12 +125,9 @@  static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
 {
 
 	if (ctrl & NAND_CTRL_CHANGE) {
-		gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NCE,
-				(ctrl & NAND_NCE) == 0);
-		gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_CLE,
-				(ctrl & NAND_CLE) != 0);
-		gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_ALE,
-				(ctrl & NAND_ALE) != 0);
+		gpiod_set_value(gpiod_nce, !(ctrl & NAND_NCE));
+		gpiod_set_value(gpiod_cle, !!(ctrl & NAND_CLE));
+		gpiod_set_value(gpiod_ale, !!(ctrl & NAND_ALE));
 	}
 
 	if (cmd != NAND_CMD_NONE)
@@ -134,41 +136,9 @@  static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd,
 
 static int ams_delta_nand_ready(struct mtd_info *mtd)
 {
-	return gpio_get_value(AMS_DELTA_GPIO_PIN_NAND_RB);
+	return gpiod_get_value(gpiod_rdy);
 }
 
-static const struct gpio _mandatory_gpio[] = {
-	{
-		.gpio	= AMS_DELTA_GPIO_PIN_NAND_NCE,
-		.flags	= GPIOF_OUT_INIT_HIGH,
-		.label	= "nand_nce",
-	},
-	{
-		.gpio	= AMS_DELTA_GPIO_PIN_NAND_NRE,
-		.flags	= GPIOF_OUT_INIT_HIGH,
-		.label	= "nand_nre",
-	},
-	{
-		.gpio	= AMS_DELTA_GPIO_PIN_NAND_NWP,
-		.flags	= GPIOF_OUT_INIT_HIGH,
-		.label	= "nand_nwp",
-	},
-	{
-		.gpio	= AMS_DELTA_GPIO_PIN_NAND_NWE,
-		.flags	= GPIOF_OUT_INIT_HIGH,
-		.label	= "nand_nwe",
-	},
-	{
-		.gpio	= AMS_DELTA_GPIO_PIN_NAND_ALE,
-		.flags	= GPIOF_OUT_INIT_LOW,
-		.label	= "nand_ale",
-	},
-	{
-		.gpio	= AMS_DELTA_GPIO_PIN_NAND_CLE,
-		.flags	= GPIOF_OUT_INIT_LOW,
-		.label	= "nand_cle",
-	},
-};
 
 /*
  * Main initialization routine
@@ -216,12 +186,17 @@  static int ams_delta_init(struct platform_device *pdev)
 	this->write_buf = ams_delta_write_buf;
 	this->read_buf = ams_delta_read_buf;
 	this->cmd_ctrl = ams_delta_hwcontrol;
-	if (gpio_request(AMS_DELTA_GPIO_PIN_NAND_RB, "nand_rdy") == 0) {
-		this->dev_ready = ams_delta_nand_ready;
-	} else {
-		this->dev_ready = NULL;
-		pr_notice("Couldn't request gpio for Delta NAND ready.\n");
+
+	gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN);
+	if (IS_ERR(gpiod_rdy)) {
+		err = PTR_ERR(gpiod_rdy);
+		dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err);
+		goto err_gpiod;
 	}
+
+	if (gpiod_rdy)
+		this->dev_ready = ams_delta_nand_ready;
+
 	/* 25 us command delay time */
 	this->chip_delay = 30;
 	this->ecc.mode = NAND_ECC_SOFT;
@@ -230,7 +205,44 @@  static int ams_delta_init(struct platform_device *pdev)
 	platform_set_drvdata(pdev, io_base);
 
 	/* Set chip enabled, but  */
-	err = gpio_request_array(_mandatory_gpio, ARRAY_SIZE(_mandatory_gpio));
+	gpiod_nwp = devm_gpiod_get(&pdev->dev, "nwp", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpiod_nwp)) {
+		err = PTR_ERR(gpiod_nwp);
+		dev_err(&pdev->dev, "NWP GPIO request failed (%d)\n", err);
+		goto err_gpiod;
+	}
+	gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpiod_nce)) {
+		err = PTR_ERR(gpiod_nce);
+		dev_err(&pdev->dev, "NCE GPIO request failed (%d)\n", err);
+		goto err_gpiod;
+	}
+	gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpiod_nre)) {
+		err = PTR_ERR(gpiod_nre);
+		dev_err(&pdev->dev, "NRE GPIO request failed (%d)\n", err);
+		goto err_gpiod;
+	}
+	gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpiod_nwe)) {
+		err = PTR_ERR(gpiod_nwe);
+		dev_err(&pdev->dev, "NWE GPIO request failed (%d)\n", err);
+		goto err_gpiod;
+	}
+	gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW);
+	if (IS_ERR(gpiod_ale)) {
+		err = PTR_ERR(gpiod_ale);
+		dev_err(&pdev->dev, "ALE GPIO request failed (%d)\n", err);
+		goto err_gpiod;
+	}
+	gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW);
+	if (IS_ERR(gpiod_cle)) {
+		err = PTR_ERR(gpiod_cle);
+		dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err);
+	}
+err_gpiod:
+	if (err == -ENODEV || err == -ENOENT)
+		err = -EPROBE_DEFER;
 	if (err)
 		goto out_gpio;
 
@@ -246,9 +258,7 @@  static int ams_delta_init(struct platform_device *pdev)
 	goto out;
 
  out_mtd:
-	gpio_free_array(_mandatory_gpio, ARRAY_SIZE(_mandatory_gpio));
 out_gpio:
-	gpio_free(AMS_DELTA_GPIO_PIN_NAND_RB);
 	iounmap(io_base);
 out_free:
 	kfree(this);
@@ -266,8 +276,6 @@  static int ams_delta_cleanup(struct platform_device *pdev)
 	/* Release resources, unregister device */
 	nand_release(ams_delta_mtd);
 
-	gpio_free_array(_mandatory_gpio, ARRAY_SIZE(_mandatory_gpio));
-	gpio_free(AMS_DELTA_GPIO_PIN_NAND_RB);
 	iounmap(io_base);
 
 	/* Free the MTD device structure */