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

[Paul Cercueil <paul@crapouillou.net>]

Le mar. 19 mai 2020 à 19:28, Boris Brezillon 
<boris.brezillon@collabora.com> a écrit :
> On Tue, 19 May 2020 17:04:37 +0200
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
>>  Le lun. 18 mai 2020 à 21:35, Boris Brezillon
>>  <boris.brezillon@collabora.com> a écrit :
>>  > On Mon, 18 May 2020 21:24:22 +0200
>>  > Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>  >
>>  >>  On Mon, 18 May 2020 19:50:04 +0200
>>  >>  Paul Cercueil <paul@crapouillou.net> wrote:
>>  >>
>>  >>  > Hi Boris,
>>  >>  >
>>  >>  > Le lun. 18 mai 2020 à 18:56, Boris Brezillon
>>  >>  > <boris.brezillon@collabora.com> 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);
}

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.

Cheers,
-Paul



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