[PATCH 2/2] mtd: nand: raw: atmel: Add error handling when rb-gpios missing

[Alexander Dahl]

Hello Eugen, hello Michael,

Am Wed, Aug 23, 2023 at 01:59:58PM +0300 schrieb Eugen Hristev:
> On 8/23/23 09:54, Michael Nazzareno Trimarchi wrote:
> > Hi
> > 
> > On Wed, Aug 23, 2023 at 8:28 AM Eugen Hristev
> > <eugen.hristev at collabora.com> wrote:
> > > 
> > > Hi,
> > > 
> > > On 8/8/23 18:03, Alexander Dahl wrote:
> > > > Hello Michael,
> > > > 
> > > > Am Tue, Aug 08, 2023 at 03:49:45PM +0200 schrieb Michael Nazzareno Trimarchi:
> > > > > Hi
> > > > > 
> > > > > On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl <ada at thorsis.com> wrote:
> > > > > > 
> > > > > > Adapt behaviour to Linux kernel driver.
> > > > > > 
> > > > > > The return value of gpio_request_by_name_nodev() was not checked before,
> > > > > > and thus in case 'rb-gpios' was missing in DT, rb.type was set to
> > > > > > ATMEL_NAND_GPIO_RB nevertheless, leading to output like this for
> > > > > > example (on sam9x60-curiosity with the line removed from dts):
> > > > > > 
> > > > > >       NAND:  Could not find valid ONFI parameter page; aborting
> > > > > >       device found, Manufacturer ID: 0xc2, Chip ID: 0xdc
> > > > > >       Macronix NAND 512MiB 3,3V 8-bit
> > > > > >       512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 64
> > > > > >       atmel-nand-controller nand-controller: NAND scan failed: -22
> > > > > >       Failed to probe nand driver (err = -22)
> > > > > >       Failed to initialize NAND controller. (error -22)
> > > > > >       0 MiB
> > > > > > 
> > > > > > Note: not having that gpio assigned in dts is fine, the driver does not
> > > > > > override nand_chip->dev_ready() then and a generic solution is used.
> > > > > > 
> > > > > > Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver")
> > > > > > Signed-off-by: Alexander Dahl <ada at thorsis.com>
> > > > > > ---
> > > > > >    drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++----
> > > > > >    1 file changed, 7 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
> > > > > > index 2b29c8def6..8e745a5111 100644
> > > > > > --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> > > > > > +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c
> > > > > > @@ -1600,10 +1600,13 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc,
> > > > > >                           nand->cs[i].rb.type = ATMEL_NAND_NATIVE_RB;
> > > > > >                           nand->cs[i].rb.id = val;
> > > > > >                   } else {
> > > > > > -                       gpio_request_by_name_nodev(np, "rb-gpios", 0,
> > > > > > -                                                  &nand->cs[i].rb.gpio,
> > > > > > -                                                  GPIOD_IS_IN);
> > > > > > -                       nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB;
> > > > > > +                       ret = gpio_request_by_name_nodev(np, "rb-gpios", 0,
> > > > > > +                                                        &nand->cs[i].rb.gpio,
> > > > > > +                                                        GPIOD_IS_IN);
> > > > > > +                       if (ret)
> > > > > > +                               dev_err(nc->dev, "Failed to get R/B gpio (err = %d)\n", ret);
> > > > > 
> > > > > Should not then an error here
> > > > 
> > > > Different log level or no message at all?
> > > > 
> > > > Note: Linux prints the same message with error level in that case.
> > > > 
> > > > Greets
> > > > Alex
> > > > 
> > > 
> > > Since the rb-gpios is optional, we can continue probing without it.
> > > Throwing an error message is optional and pure informative. So I am fine
> > > with it
> > > 
> > 
> > Yes ok, but I'm not sure linux give an error if the gpio is get as
> > optional and condition
> > is IS_ERR. Am I right?
> 
> 
> 			if (IS_ERR(gpio) && PTR_ERR(gpio) != -ENOENT) {
> 				dev_err(nc->dev,
> 					"Failed to get R/B gpio (err = %ld)\n",
> 					PTR_ERR(gpio));
> 				return ERR_CAST(gpio);
> 			}
> 
> So Linux throws the message if IS_ERR . If the property is missing (ENOENT)
> it moves on.
> 
> Can we replicate the same behavior or this behavior does not suit us in
> U-boot ?
> 
> Basically I think it should be :
> 		
> 		if (ret && ret != -ENOENT)
> 			dev_err(...)
> 		if (!ret)
> 			rb.type = ATMEL_NAND_GPIO_RB;
> 
> Is this what you had in mind Michael ?

The discussion between you guys somehow got stuck.  I still have this
patch on my TODO list, but I'm not sure how to proceed.  Please advice
in which direction to go forward, there's still an error condition
(described in the initial commit message) here, which needs to be
handled, otherwise the NAND flash might not be usable.

Greets
Alex

> 
> Eugen
> 
> > 
> > For the rest is fine
> > 
> > Michael
> > 
> > > What I wanted to ask is what happens with nand->cs[i].rb.type , is it 0
> > > by default ?
> > > 
> > > Other than that, I can apply this patch, Michael, do you have any more
> > > comments on it ?
> > > 
> > > Thanks,
> > > Eugen
> > > > > 
> > > > > Michael
> > > > > 
> > > > > > +                       else
> > > > > > +                               nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB;
> > > > > >                   }
> > > > > > 
> > > > > >                   gpio_request_by_name_nodev(np, "cs-gpios", 0,
> > > > > > --
> > > > > > 2.30.2
> > > > > > 
> > > > > 
> > > > > 
> > > > > --
> > > > > Michael Nazzareno Trimarchi
> > > > > Co-Founder & Chief Executive Officer
> > > > > M. +39 347 913 2170
> > > > > michael at amarulasolutions.com
> > > > > __________________________________
> > > > > 
> > > > > Amarula Solutions BV
> > > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > > T. +31 (0)85 111 9172
> > > > > info at amarulasolutions.com
> > > > > www.amarulasolutions.com
> > > 
> > 
> > 
>