Re: [PATCH] mtd: rawnand: ingenic: Convert the driver to exec_op()

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Tue, 19 May 2020 17:04:37 +0200
Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:

> Le lun. 18 mai 2020 à 21:35, Boris Brezillon 
> <boris.brezillon@xxxxxxxxxxxxx> a écrit :
> > On Mon, 18 May 2020 21:24:22 +0200
> > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
> >   
> >>  On Mon, 18 May 2020 19:50:04 +0200
> >>  Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> >>   
> >>  > Hi Boris,
> >>  >
> >>  > Le lun. 18 mai 2020 à 18:56, Boris Brezillon
> >>  > <boris.brezillon@xxxxxxxxxxxxx> a écrit :  
> >>  > > Let's convert the driver to exec_op() to have one less driver   
> >> relying  
> >>  > > on the legacy interface.  
> >>  >
> >>  > Great work, thanks for that.
> >>  >
> >>  > However it does not work :( nand_scan() returns error -145.  
> >> 
> >>  Looks like the R/B signal is inverted. Can you try with the
> >>  following diff applied?  
> > 
> > I checked the DT, and the GPIO is indeed declared GPIO_ACTIVE_LOW,
> > which explain why the test is inverted. Because of DT ABI stability it
> > might not be an option to change that, but the signal should actually 
> > be
> > declared GPIO_ACTIVE_HIGH.  
> 
> It depends what you consider what is the active state, is it when 
> "busy" or "ready"? ;)

True, this should really be documented in the generic binding part. As
you probably guessed from this discussion, all other drivers (and the
framework) is assuming "ready" is the state we're monitoring, so it's
effectively active high.

> 
> I can fix it in the devicetree, and the driver would return 
> (gpiod_get_value_cansleep(gpiod) ^ gpiod_is_active_low(gpiod)) for 
> compatibility with the old devicetree.

Or you could read the raw value (gpiod_get_raw_value_cansleep()),
but that still means you can't move away from the old semantics without
breaking the existing DT with the erroneous active-low. I mean,
active-low is still valid if someone has the R/B signal inverted,
but you can't discriminate when it's valid and when it's not.

I guess having a custom helper, and documenting that the active state
for ingenic is BUSY would be okay. Unless you'd be willing to break
the backward compat for the only board that has a rb-gpios property
defined.

[1]https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L4166

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/