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

20 Sep 2023


      Hi
Il mer 20 set 2023, 08:13 Alexander Dahl ada@thorsis.com ha scritto:
...
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@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@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@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.
Yes the suggestion above is what is the right path to follow.
Michael
...
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@amarulasolutions.com
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com