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

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Tue, 19 May 2020 23:12:21 +0200
Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:

> Le mar. 19 mai 2020 à 19:28, Boris Brezillon 
> <boris.brezillon@xxxxxxxxxxxxx> a écrit :
> > 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.  
> 
> What I suggest, in the probe:
> 
> if (of_machine_is_compatible("qi,lb60") && 
> gpiod_is_active_low(nand->busy_gpio)) {
>     gpiod_toggle_active_low(nand->busy_gpio);
> }
> 

Oh, I didn't notice this gpiod_toggle_active_low() in the API.

> Then it's backward-compatible, would allow me to fix the rb-gpios in 
> devicetree, and wouldn't require a custom nand_gpio_waitrdy() function.

That's indeed the best option IMHO. I'll add a patch doing that in my
v2. Thanks for the suggestion.

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