Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib

[Boris Brezillon]

Hi Brian,

On Wed, 2 Dec 2015 20:45:44 -0800
Brian Norris  wrote:

> (to be clear, this branch of discussion isn't directly regarding the TI
> changes; we can handle any generic handling afterward, as long as we get
> the DT binding right now)
> 
> On Tue, Oct 27, 2015 at 09:28:32AM +0100, Boris Brezillon wrote:
> > On Mon, 26 Oct 2015 13:49:00 -0700
> > Brian Norris  wrote:
> > > On Fri, Sep 18, 2015 at 05:53:40PM +0300, Roger Quadros wrote:
> > > > @@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device 
> > > > *pdev)
> > > > info->reg = pdata->reg;
> > > > info->of_node = pdata->of_node;
> > > > info->ecc_opt = pdata->ecc_opt;
> > > > -   info->dev_ready = pdata->dev_ready;
> > > > +   if (pdata->dev_ready)
> > > > +   dev_info(>dev, "pdata->dev_ready is 
> > > > deprecated\n");
> > > > +
> > > > info->xfer_type = pdata->xfer_type;
> > > > info->devsize = pdata->devsize;
> > > > info->elm_of_node = pdata->elm_of_node;
> > > > @@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct 
> > > > platform_device *pdev)
> > > > nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
> > > > nand_chip->cmd_ctrl  = omap_hwcontrol;
> > > >  
> > > > +   info->ready_gpiod = devm_gpiod_get_optional(>dev, "ready",
> > > > +   GPIOD_IN);
> > > 
> > > Others have been looking at using GPIOs for the ready/busy pin too. At a
> > > minimum, we need an updated DT binding doc for this, since I see you're
> > > adding this via device tree in a later patch (I don't see any DT binding
> > > patch for this; but I could just be overlooking it). It'd also be great
> > > if this support was moved to nand_dt_init() so other platforms can
> > > benefit, but I won't require that.
> > 
> > Actually I started to work on a generic solution parsing the DT and
> > creating CS, WP and RB gpios when they are provided, but I think it's a
> > bit more complicated than just moving the rb-gpios parsing into
> > nand_dt_init().
> > First you'll need something to store your gpio_desc pointers, which
> > means you'll have to allocate a table of gpio_desc pointers (or a table
> > of struct embedding a gpio_desc pointer).
> 
> I'm not sure what you mean by table. It seems like we would just have a
> few gpio_desc pointers in struct nand_chip, no?

Nope, because a single nand_chip can use several CS and R/B lines,
hence the gpio_desc array.

> 
> > The other blocking point is that when nand_scan_ident() is called, the
> > caller is supposed to have filled the ->dev_ready() or ->waitfunc()
> > fields, and to choose how to implement it he may need to know
> > which kind of RB handler should be used (this is the case in the sunxi
> > driver, where the user can either use a GPIO or native R/B pin directly
> > connected to the controller).
> 
> Right, I was thinking about this one.
> 
> > All this makes me think that maybe nand_dt_init() should be called
> > separately or in a different helper (nand_init() ?) taking care of the
> > basic nand_chip initializations/allocations without interacting with
> > the NAND itself.
> 
> Yeah, I feel like there have been other good reasons for something like
> this before, and we just have worked around them so far. Maybe something
> more like the alloc/add split in many other subsystems? e.g.,
> platform_device_{alloc,add}, spi_{alloc,add}_device. Now we might want
> nand_alloc()?
> 
> On a slight tangent, it seems silly that nand_scan_tail() doesn't
> register the MTD, but nand_release() unregisters it. That shouldn't be.

Yep, that's one of the things we should address too.

> 
> > Another solution would be to add an ->init() function to nand_chip
> > and call it after the generic initialization has been done (but before
> > NAND chip detection). This way the NAND controller driver could adapt
> > some fields and parse controller specific properties.
> 
> That could work too, I guess.

Hm, all these suggestions have been made before I started my
nand_controller/nand_chip separation work, and I think we can get
something way simpler if we switch to a model where the core
implements most of the initialization steps (including parsing of
generic DT properties) and ask the controller to do its specific
initialization (as is done in the SPI subsystem for example).

If you want to have an idea of where I'm heading to, you can have a
look at the last commits in this branch [1]. The generic DT parsing is
not yet automated but it could easily be places here [2], so that when
controller->ops->add() is called everything is in place and the driver
can overload the default behavior with its own implementation.

Any objection to this approach?

Best Regards,

Boris

[1]https://github.com/bbrezillon/linux-sunxi/commits/nand/layering-rework

Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib

[Brian Norris]

(to be clear, this branch of discussion isn't directly regarding the TI
changes; we can handle any generic handling afterward, as long as we get
the DT binding right now)

On Tue, Oct 27, 2015 at 09:28:32AM +0100, Boris Brezillon wrote:
> On Mon, 26 Oct 2015 13:49:00 -0700
> Brian Norris  wrote:
> > On Fri, Sep 18, 2015 at 05:53:40PM +0300, Roger Quadros wrote:
> > > @@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device 
> > > *pdev)
> > >   info->reg = pdata->reg;
> > >   info->of_node = pdata->of_node;
> > >   info->ecc_opt = pdata->ecc_opt;
> > > - info->dev_ready = pdata->dev_ready;
> > > + if (pdata->dev_ready)
> > > + dev_info(>dev, "pdata->dev_ready is 
> > > deprecated\n");
> > > +
> > >   info->xfer_type = pdata->xfer_type;
> > >   info->devsize = pdata->devsize;
> > >   info->elm_of_node = pdata->elm_of_node;
> > > @@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device 
> > > *pdev)
> > >   nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
> > >   nand_chip->cmd_ctrl  = omap_hwcontrol;
> > >  
> > > + info->ready_gpiod = devm_gpiod_get_optional(>dev, "ready",
> > > + GPIOD_IN);
> > 
> > Others have been looking at using GPIOs for the ready/busy pin too. At a
> > minimum, we need an updated DT binding doc for this, since I see you're
> > adding this via device tree in a later patch (I don't see any DT binding
> > patch for this; but I could just be overlooking it). It'd also be great
> > if this support was moved to nand_dt_init() so other platforms can
> > benefit, but I won't require that.
> 
> Actually I started to work on a generic solution parsing the DT and
> creating CS, WP and RB gpios when they are provided, but I think it's a
> bit more complicated than just moving the rb-gpios parsing into
> nand_dt_init().
> First you'll need something to store your gpio_desc pointers, which
> means you'll have to allocate a table of gpio_desc pointers (or a table
> of struct embedding a gpio_desc pointer).

I'm not sure what you mean by table. It seems like we would just have a
few gpio_desc pointers in struct nand_chip, no?

> The other blocking point is that when nand_scan_ident() is called, the
> caller is supposed to have filled the ->dev_ready() or ->waitfunc()
> fields, and to choose how to implement it he may need to know
> which kind of RB handler should be used (this is the case in the sunxi
> driver, where the user can either use a GPIO or native R/B pin directly
> connected to the controller).

Right, I was thinking about this one.

> All this makes me think that maybe nand_dt_init() should be called
> separately or in a different helper (nand_init() ?) taking care of the
> basic nand_chip initializations/allocations without interacting with
> the NAND itself.

Yeah, I feel like there have been other good reasons for something like
this before, and we just have worked around them so far. Maybe something
more like the alloc/add split in many other subsystems? e.g.,
platform_device_{alloc,add}, spi_{alloc,add}_device. Now we might want
nand_alloc()?

On a slight tangent, it seems silly that nand_scan_tail() doesn't
register the MTD, but nand_release() unregisters it. That shouldn't be.

> Another solution would be to add an ->init() function to nand_chip
> and call it after the generic initialization has been done (but before
> NAND chip detection). This way the NAND controller driver could adapt
> some fields and parse controller specific properties.

That could work too, I guess.

> What do you think?

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib

[Roger Quadros]

On 26/10/15 22:49, Brian Norris wrote:
> + others
> 
> A few comments below.
> 
> On Fri, Sep 18, 2015 at 05:53:40PM +0300, Roger Quadros wrote:
>> The GPMC WAIT pin status are now available over gpiolib.
>> Update the omap_dev_ready() function to use gpio instead of
>> directly accessing GPMC register space.
>>
>> Signed-off-by: Roger Quadros 
>> ---
>>  drivers/mtd/nand/omap2.c | 29 
>> +---
>>  include/linux/platform_data/mtd-nand-omap2.h |  2 +-
>>  2 files changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index 228f498..d0f2620 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -12,6 +12,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -184,6 +185,8 @@ struct omap_nand_info {
>>  /* fields specific for BCHx_HW ECC scheme */
>>  struct device   *elm_dev;
>>  struct device_node  *of_node;
>> +/* NAND ready gpio */
>> +struct gpio_desc*ready_gpiod;
>>  };
>>  
>>  /**
>> @@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct 
>> nand_chip *chip)
>>  }
>>  
>>  /**
>> - * omap_dev_ready - calls the platform specific dev_ready function
>> + * omap_dev_ready - checks the NAND Ready GPIO line
>>   * @mtd: MTD device structure
>> + *
>> + * Returns true if ready and false if busy.
>>   */
>>  static int omap_dev_ready(struct mtd_info *mtd)
>>  {
>> -unsigned int val = 0;
>>  struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>>  mtd);
>>  
>> -val = readl(info->reg.gpmc_status);
>> -
>> -if ((val & 0x100) == 0x100) {
>> -return 1;
>> -} else {
>> -return 0;
>> -}
>> +return gpiod_get_value(info->ready_gpiod);
>>  }
>>  
>>  /**
>> @@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device 
>> *pdev)
>>  info->reg = pdata->reg;
>>  info->of_node = pdata->of_node;
>>  info->ecc_opt = pdata->ecc_opt;
>> -info->dev_ready = pdata->dev_ready;
>> +if (pdata->dev_ready)
>> +dev_info(>dev, "pdata->dev_ready is 
>> deprecated\n");
>> +
>>  info->xfer_type = pdata->xfer_type;
>>  info->devsize = pdata->devsize;
>>  info->elm_of_node = pdata->elm_of_node;
>> @@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device 
>> *pdev)
>>  nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
>>  nand_chip->cmd_ctrl  = omap_hwcontrol;
>>  
>> +info->ready_gpiod = devm_gpiod_get_optional(>dev, "ready",
>> +GPIOD_IN);
> 
> Others have been looking at using GPIOs for the ready/busy pin too. At a
> minimum, we need an updated DT binding doc for this, since I see you're
> adding this via device tree in a later patch (I don't see any DT binding
> patch for this; but I could just be overlooking it). It'd also be great
> if this support was moved to nand_dt_init() so other platforms can
> benefit, but I won't require that.
> 
> Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio'
> (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't
> really care, but the name should be going into a doc, so we can choose
> the same one everywhere.
> 
> EDIT: looks like the discussion was partly here [1] and it seems we're
> landing on "rb-gpios" in the latest version [2]. Can we stick with that?

Why should it be "rb-gpios" and not "rb-gpio"?
I don't think there are multiple gpios for r/b# function.

cheers,
-roger

> 
> Regards,
> Brian
> 
> [0] "Previous" may be subject to debate, as both series have been going
> for several revisions.
> [1] http://patchwork.ozlabs.org/patch/515327/
> [2] http://patchwork.ozlabs.org/patch/526819/
> 
>> +if (IS_ERR(info->ready_gpiod)) {
>> +dev_err(dev, "failed to get ready gpio\n");
>> +return PTR_ERR(info->ready_gpiod);
>> +}
>> +
>>  /*
>>   * If RDY/BSY line is connected to OMAP then use the omap ready
>>   * function and the generic nand_wait function which reads the status
>> @@ -1822,7 +1829,7 @@ static int omap_nand_probe(struct platform_device 
>> *pdev)
>>   * chip delay which is slightly more than tR (AC Timing) of the NAND
>>   * device and read status register until you get a failure or success
>>   */
>> -if (info->dev_ready) {
>> +if (info->ready_gpiod) {
>>  nand_chip->dev_ready = omap_dev_ready;
>>  nand_chip->chip_delay = 0;
>>  } else {
>> diff --git a/include/linux/platform_data/mtd-nand-omap2.h 
>> b/include/linux/platform_data/mtd-nand-omap2.h
>> index ff27e5a..19e509d 100644
>> --- a/include/linux/platform_data/mtd-nand-omap2.h
>> +++ 

Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib

[Boris Brezillon]

Hi Roger,

On Tue, 27 Oct 2015 10:03:02 +0200
Roger Quadros  wrote:

> On 26/10/15 22:49, Brian Norris wrote:
> > 
> > Others have been looking at using GPIOs for the ready/busy pin too. At a
> > minimum, we need an updated DT binding doc for this, since I see you're
> > adding this via device tree in a later patch (I don't see any DT binding
> > patch for this; but I could just be overlooking it). It'd also be great
> > if this support was moved to nand_dt_init() so other platforms can
> > benefit, but I won't require that.
> > 
> > Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio'
> > (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't
> > really care, but the name should be going into a doc, so we can choose
> > the same one everywhere.
> > 
> > EDIT: looks like the discussion was partly here [1] and it seems we're
> > landing on "rb-gpios" in the latest version [2]. Can we stick with that?
> 
> Why should it be "rb-gpios" and not "rb-gpio"?
> I don't think there are multiple gpios for r/b# function.

Because it's supposed to be a generic binding, and some NAND chips
embed several dies, thus exposing several CS and RB pins, hence the
rb-gpios name.
Also, as described here [1], the convention is to name your property
-gpios even if you only need one gpio.

Best Regards,

Boris

[1]http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/gpio/gpio.txt#L16



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib

[Boris Brezillon]

Hi Brian,

On Mon, 26 Oct 2015 13:49:00 -0700
Brian Norris  wrote:

> + others
> 
> A few comments below.
> 
> On Fri, Sep 18, 2015 at 05:53:40PM +0300, Roger Quadros wrote:
> > The GPMC WAIT pin status are now available over gpiolib.
> > Update the omap_dev_ready() function to use gpio instead of
> > directly accessing GPMC register space.
> > 
> > Signed-off-by: Roger Quadros 
> > ---
> >  drivers/mtd/nand/omap2.c | 29 
> > +---
> >  include/linux/platform_data/mtd-nand-omap2.h |  2 +-
> >  2 files changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index 228f498..d0f2620 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> > @@ -12,6 +12,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -184,6 +185,8 @@ struct omap_nand_info {
> > /* fields specific for BCHx_HW ECC scheme */
> > struct device   *elm_dev;
> > struct device_node  *of_node;
> > +   /* NAND ready gpio */
> > +   struct gpio_desc*ready_gpiod;
> >  };
> >  
> >  /**
> > @@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct 
> > nand_chip *chip)
> >  }
> >  
> >  /**
> > - * omap_dev_ready - calls the platform specific dev_ready function
> > + * omap_dev_ready - checks the NAND Ready GPIO line
> >   * @mtd: MTD device structure
> > + *
> > + * Returns true if ready and false if busy.
> >   */
> >  static int omap_dev_ready(struct mtd_info *mtd)
> >  {
> > -   unsigned int val = 0;
> > struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> > mtd);
> >  
> > -   val = readl(info->reg.gpmc_status);
> > -
> > -   if ((val & 0x100) == 0x100) {
> > -   return 1;
> > -   } else {
> > -   return 0;
> > -   }
> > +   return gpiod_get_value(info->ready_gpiod);
> >  }
> >  
> >  /**
> > @@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device 
> > *pdev)
> > info->reg = pdata->reg;
> > info->of_node = pdata->of_node;
> > info->ecc_opt = pdata->ecc_opt;
> > -   info->dev_ready = pdata->dev_ready;
> > +   if (pdata->dev_ready)
> > +   dev_info(>dev, "pdata->dev_ready is 
> > deprecated\n");
> > +
> > info->xfer_type = pdata->xfer_type;
> > info->devsize = pdata->devsize;
> > info->elm_of_node = pdata->elm_of_node;
> > @@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device 
> > *pdev)
> > nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
> > nand_chip->cmd_ctrl  = omap_hwcontrol;
> >  
> > +   info->ready_gpiod = devm_gpiod_get_optional(>dev, "ready",
> > +   GPIOD_IN);
> 
> Others have been looking at using GPIOs for the ready/busy pin too. At a
> minimum, we need an updated DT binding doc for this, since I see you're
> adding this via device tree in a later patch (I don't see any DT binding
> patch for this; but I could just be overlooking it). It'd also be great
> if this support was moved to nand_dt_init() so other platforms can
> benefit, but I won't require that.

Actually I started to work on a generic solution parsing the DT and
creating CS, WP and RB gpios when they are provided, but I think it's a
bit more complicated than just moving the rb-gpios parsing into
nand_dt_init().
First you'll need something to store your gpio_desc pointers, which
means you'll have to allocate a table of gpio_desc pointers (or a table
of struct embedding a gpio_desc pointer).
The other blocking point is that when nand_scan_ident() is called, the
caller is supposed to have filled the ->dev_ready() or ->waitfunc()
fields, and to choose how to implement it he may need to know
which kind of RB handler should be used (this is the case in the sunxi
driver, where the user can either use a GPIO or native R/B pin directly
connected to the controller).

All this makes me think that maybe nand_dt_init() should be called
separately or in a different helper (nand_init() ?) taking care of the
basic nand_chip initializations/allocations without interacting with
the NAND itself.

Another solution would be to add an ->init() function to nand_chip
and call it after the generic initialization has been done (but before
NAND chip detection). This way the NAND controller driver could adapt
some fields and parse controller specific properties.

What do you think?

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib

[Roger Quadros]

Boris,

On 27/10/15 10:12, Boris Brezillon wrote:
> Hi Roger,
> 
> On Tue, 27 Oct 2015 10:03:02 +0200
> Roger Quadros  wrote:
> 
>> On 26/10/15 22:49, Brian Norris wrote:
>>>
>>> Others have been looking at using GPIOs for the ready/busy pin too. At a
>>> minimum, we need an updated DT binding doc for this, since I see you're
>>> adding this via device tree in a later patch (I don't see any DT binding
>>> patch for this; but I could just be overlooking it). It'd also be great
>>> if this support was moved to nand_dt_init() so other platforms can
>>> benefit, but I won't require that.
>>>
>>> Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio'
>>> (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't
>>> really care, but the name should be going into a doc, so we can choose
>>> the same one everywhere.
>>>
>>> EDIT: looks like the discussion was partly here [1] and it seems we're
>>> landing on "rb-gpios" in the latest version [2]. Can we stick with that?
>>
>> Why should it be "rb-gpios" and not "rb-gpio"?
>> I don't think there are multiple gpios for r/b# function.
> 
> Because it's supposed to be a generic binding, and some NAND chips
> embed several dies, thus exposing several CS and RB pins, hence the
> rb-gpios name.
> Also, as described here [1], the convention is to name your property
> -gpios even if you only need one gpio.

Makes sense now. Thanks for the explanation.
I'll update this patch to use rb-gpios and update the binding doc as well.

--
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib

[Brian Norris]

+ others

A few comments below.

On Fri, Sep 18, 2015 at 05:53:40PM +0300, Roger Quadros wrote:
> The GPMC WAIT pin status are now available over gpiolib.
> Update the omap_dev_ready() function to use gpio instead of
> directly accessing GPMC register space.
> 
> Signed-off-by: Roger Quadros 
> ---
>  drivers/mtd/nand/omap2.c | 29 
> +---
>  include/linux/platform_data/mtd-nand-omap2.h |  2 +-
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 228f498..d0f2620 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -184,6 +185,8 @@ struct omap_nand_info {
>   /* fields specific for BCHx_HW ECC scheme */
>   struct device   *elm_dev;
>   struct device_node  *of_node;
> + /* NAND ready gpio */
> + struct gpio_desc*ready_gpiod;
>  };
>  
>  /**
> @@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct 
> nand_chip *chip)
>  }
>  
>  /**
> - * omap_dev_ready - calls the platform specific dev_ready function
> + * omap_dev_ready - checks the NAND Ready GPIO line
>   * @mtd: MTD device structure
> + *
> + * Returns true if ready and false if busy.
>   */
>  static int omap_dev_ready(struct mtd_info *mtd)
>  {
> - unsigned int val = 0;
>   struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>   mtd);
>  
> - val = readl(info->reg.gpmc_status);
> -
> - if ((val & 0x100) == 0x100) {
> - return 1;
> - } else {
> - return 0;
> - }
> + return gpiod_get_value(info->ready_gpiod);
>  }
>  
>  /**
> @@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
>   info->reg = pdata->reg;
>   info->of_node = pdata->of_node;
>   info->ecc_opt = pdata->ecc_opt;
> - info->dev_ready = pdata->dev_ready;
> + if (pdata->dev_ready)
> + dev_info(>dev, "pdata->dev_ready is 
> deprecated\n");
> +
>   info->xfer_type = pdata->xfer_type;
>   info->devsize = pdata->devsize;
>   info->elm_of_node = pdata->elm_of_node;
> @@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device 
> *pdev)
>   nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
>   nand_chip->cmd_ctrl  = omap_hwcontrol;
>  
> + info->ready_gpiod = devm_gpiod_get_optional(>dev, "ready",
> + GPIOD_IN);

Others have been looking at using GPIOs for the ready/busy pin too. At a
minimum, we need an updated DT binding doc for this, since I see you're
adding this via device tree in a later patch (I don't see any DT binding
patch for this; but I could just be overlooking it). It'd also be great
if this support was moved to nand_dt_init() so other platforms can
benefit, but I won't require that.

Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio'
(the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't
really care, but the name should be going into a doc, so we can choose
the same one everywhere.

EDIT: looks like the discussion was partly here [1] and it seems we're
landing on "rb-gpios" in the latest version [2]. Can we stick with that?

Regards,
Brian

[0] "Previous" may be subject to debate, as both series have been going
for several revisions.
[1] http://patchwork.ozlabs.org/patch/515327/
[2] http://patchwork.ozlabs.org/patch/526819/

> + if (IS_ERR(info->ready_gpiod)) {
> + dev_err(dev, "failed to get ready gpio\n");
> + return PTR_ERR(info->ready_gpiod);
> + }
> +
>   /*
>* If RDY/BSY line is connected to OMAP then use the omap ready
>* function and the generic nand_wait function which reads the status
> @@ -1822,7 +1829,7 @@ static int omap_nand_probe(struct platform_device *pdev)
>* chip delay which is slightly more than tR (AC Timing) of the NAND
>* device and read status register until you get a failure or success
>*/
> - if (info->dev_ready) {
> + if (info->ready_gpiod) {
>   nand_chip->dev_ready = omap_dev_ready;
>   nand_chip->chip_delay = 0;
>   } else {
> diff --git a/include/linux/platform_data/mtd-nand-omap2.h 
> b/include/linux/platform_data/mtd-nand-omap2.h
> index ff27e5a..19e509d 100644
> --- a/include/linux/platform_data/mtd-nand-omap2.h
> +++ b/include/linux/platform_data/mtd-nand-omap2.h
> @@ -70,7 +70,6 @@ struct omap_nand_platform_data {
>   int cs;
>   struct mtd_partition*parts;
>   int nr_parts;
> - booldev_ready;
>   boolflash_bbt;
>   

[PATCH v3 18/27] mtd: nand: omap2: Implement NAND ready using gpiolib

[Roger Quadros]

The GPMC WAIT pin status are now available over gpiolib.
Update the omap_dev_ready() function to use gpio instead of
directly accessing GPMC register space.

Signed-off-by: Roger Quadros 
---
 drivers/mtd/nand/omap2.c | 29 +---
 include/linux/platform_data/mtd-nand-omap2.h |  2 +-
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 228f498..d0f2620 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -184,6 +185,8 @@ struct omap_nand_info {
/* fields specific for BCHx_HW ECC scheme */
struct device   *elm_dev;
struct device_node  *of_node;
+   /* NAND ready gpio */
+   struct gpio_desc*ready_gpiod;
 };
 
 /**
@@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct 
nand_chip *chip)
 }
 
 /**
- * omap_dev_ready - calls the platform specific dev_ready function
+ * omap_dev_ready - checks the NAND Ready GPIO line
  * @mtd: MTD device structure
+ *
+ * Returns true if ready and false if busy.
  */
 static int omap_dev_ready(struct mtd_info *mtd)
 {
-   unsigned int val = 0;
struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
mtd);
 
-   val = readl(info->reg.gpmc_status);
-
-   if ((val & 0x100) == 0x100) {
-   return 1;
-   } else {
-   return 0;
-   }
+   return gpiod_get_value(info->ready_gpiod);
 }
 
 /**
@@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
info->reg = pdata->reg;
info->of_node = pdata->of_node;
info->ecc_opt = pdata->ecc_opt;
-   info->dev_ready = pdata->dev_ready;
+   if (pdata->dev_ready)
+   dev_info(>dev, "pdata->dev_ready is 
deprecated\n");
+
info->xfer_type = pdata->xfer_type;
info->devsize = pdata->devsize;
info->elm_of_node = pdata->elm_of_node;
@@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device *pdev)
nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
nand_chip->cmd_ctrl  = omap_hwcontrol;
 
+   info->ready_gpiod = devm_gpiod_get_optional(>dev, "ready",
+   GPIOD_IN);
+   if (IS_ERR(info->ready_gpiod)) {
+   dev_err(dev, "failed to get ready gpio\n");
+   return PTR_ERR(info->ready_gpiod);
+   }
+
/*
 * If RDY/BSY line is connected to OMAP then use the omap ready
 * function and the generic nand_wait function which reads the status
@@ -1822,7 +1829,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 * chip delay which is slightly more than tR (AC Timing) of the NAND
 * device and read status register until you get a failure or success
 */
-   if (info->dev_ready) {
+   if (info->ready_gpiod) {
nand_chip->dev_ready = omap_dev_ready;
nand_chip->chip_delay = 0;
} else {
diff --git a/include/linux/platform_data/mtd-nand-omap2.h 
b/include/linux/platform_data/mtd-nand-omap2.h
index ff27e5a..19e509d 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -70,7 +70,6 @@ struct omap_nand_platform_data {
int cs;
struct mtd_partition*parts;
int nr_parts;
-   booldev_ready;
boolflash_bbt;
enum nand_ioxfer_type;
int devsize;
@@ -81,5 +80,6 @@ struct omap_nand_platform_data {
/* deprecated */
struct gpmc_nand_regs   reg;
struct device_node  *of_node;
+   booldev_ready;
 };
 #endif
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html