From: Bartosz Golaszewski <[email protected]>
Use the new, less cumbersome interface for setting the GPIO as
active-high that doesn't require first checking the current state.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
index 6748226b8bd1..c055133c45fe 100644
--- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
+++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
@@ -388,9 +388,8 @@ static int ingenic_nand_init_chip(struct platform_device *pdev,
* here for older DTs so we can re-use the generic nand_gpio_waitrdy()
* helper, and be consistent with what other drivers do.
*/
- if (of_machine_is_compatible("qi,lb60") &&
- gpiod_is_active_low(nand->busy_gpio))
- gpiod_toggle_active_low(nand->busy_gpio);
+ if (of_machine_is_compatible("qi,lb60"))
+ gpiod_set_active_high(nand->busy_gpio);
nand->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_LOW);
--
2.39.2
Hi,
Le mercredi 13 septembre 2023 à 13:49 +0200, Bartosz Golaszewski a
écrit :
> From: Bartosz Golaszewski <[email protected]>
>
> Use the new, less cumbersome interface for setting the GPIO as
> active-high that doesn't require first checking the current state.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Works for me.
Acked-by: Paul Cercueil <[email protected]>
Cheers,
-Paul
> ---
> drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> index 6748226b8bd1..c055133c45fe 100644
> --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand_drv.c
> @@ -388,9 +388,8 @@ static int ingenic_nand_init_chip(struct
> platform_device *pdev,
> * here for older DTs so we can re-use the generic
> nand_gpio_waitrdy()
> * helper, and be consistent with what other drivers do.
> */
> - if (of_machine_is_compatible("qi,lb60") &&
> - gpiod_is_active_low(nand->busy_gpio))
> - gpiod_toggle_active_low(nand->busy_gpio);
> + if (of_machine_is_compatible("qi,lb60"))
> + gpiod_set_active_high(nand->busy_gpio);
>
> nand->wp_gpio = devm_gpiod_get_optional(dev, "wp",
> GPIOD_OUT_LOW);
>
On Wed, Sep 13, 2023 at 10:05 PM Andy Shevchenko
<[email protected]> wrote:
> On Wed, Sep 13, 2023 at 2:50 PM Bartosz Golaszewski <[email protected]> wrote:
> >
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Use the new, less cumbersome interface for setting the GPIO as
> > active-high that doesn't require first checking the current state.
>
> ...
>
> > * here for older DTs so we can re-use the generic nand_gpio_waitrdy()
> > * helper, and be consistent with what other drivers do.
> > */
> > - if (of_machine_is_compatible("qi,lb60") &&
> > - gpiod_is_active_low(nand->busy_gpio))
> > - gpiod_toggle_active_low(nand->busy_gpio);
> > + if (of_machine_is_compatible("qi,lb60"))
> > + gpiod_set_active_high(nand->busy_gpio);
>
> Why not moving this quirk to gpiolib-of.c?
That's a better idea here I think, it's clearly a quirk for a
buggy device tree.
Yours,
Linus Walleij
Hi Andy,
[email protected] wrote on Wed, 13 Sep 2023 22:12:40 +0200:
> On Wed, Sep 13, 2023 at 10:05 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, Sep 13, 2023 at 2:50 PM Bartosz Golaszewski <[email protected]> wrote:
> > >
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > Use the new, less cumbersome interface for setting the GPIO as
> > > active-high that doesn't require first checking the current state.
> >
> > ...
> >
> > > * here for older DTs so we can re-use the generic nand_gpio_waitrdy()
> > > * helper, and be consistent with what other drivers do.
> > > */
> > > - if (of_machine_is_compatible("qi,lb60") &&
> > > - gpiod_is_active_low(nand->busy_gpio))
> > > - gpiod_toggle_active_low(nand->busy_gpio);
> > > + if (of_machine_is_compatible("qi,lb60"))
> > > + gpiod_set_active_high(nand->busy_gpio);
> >
> > Why not moving this quirk to gpiolib-of.c?
>
> That's a better idea here I think, it's clearly a quirk for a
> buggy device tree.
Agreed, it's just for backward compatibility purposes in a single
driver. I believe it should stay here.
Thanks,
Miquèl
On Wed, Sep 13, 2023 at 2:50 PM Bartosz Golaszewski <[email protected]> wrote:
>
> From: Bartosz Golaszewski <[email protected]>
>
> Use the new, less cumbersome interface for setting the GPIO as
> active-high that doesn't require first checking the current state.
...
> * here for older DTs so we can re-use the generic nand_gpio_waitrdy()
> * helper, and be consistent with what other drivers do.
> */
> - if (of_machine_is_compatible("qi,lb60") &&
> - gpiod_is_active_low(nand->busy_gpio))
> - gpiod_toggle_active_low(nand->busy_gpio);
> + if (of_machine_is_compatible("qi,lb60"))
> + gpiod_set_active_high(nand->busy_gpio);
Why not moving this quirk to gpiolib-of.c?
--
With Best Regards,
Andy Shevchenko
Hi Bartosz,
[email protected] wrote on Wed, 13 Sep 2023 14:01:47 +0200:
> Hi,
>
> Le mercredi 13 septembre 2023 à 13:49 +0200, Bartosz Golaszewski a
> écrit :
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Use the new, less cumbersome interface for setting the GPIO as
> > active-high that doesn't require first checking the current state.
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> Works for me.
>
> Acked-by: Paul Cercueil <[email protected]>
For me as well, the new API looks much better.
I saw Hans is fine with you merging the platform/x86 patch. I am also
fine if all maintainers agree on that. In this case:
Acked-by: Miquel Raynal <[email protected]>
However, if you finally need to produce an immutable tag, let me know
and I will take the patch through our tree.
Thanks,
Miquèl
On Thu, Sep 14, 2023 at 10:02 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Sep 13, 2023 at 11:23 PM Miquel Raynal
> <[email protected]> wrote:
> > [email protected] wrote on Wed, 13 Sep 2023 22:12:40 +0200:
> > > On Wed, Sep 13, 2023 at 10:05 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Wed, Sep 13, 2023 at 2:50 PM Bartosz Golaszewski <[email protected]> wrote:
>
> ...
>
> > > > Why not moving this quirk to gpiolib-of.c?
> > >
> > > That's a better idea here I think, it's clearly a quirk for a
> > > buggy device tree.
> >
> > Agreed, it's just for backward compatibility purposes in a single
> > driver. I believe it should stay here.
>
> I believe Linus was for moving.
>
> gpiolib-of.c contains a lot of quirks, including this one. Calling
To be clear:
"including one for the same issue"
> these new (or old) APIs for overriding polarity in many cases
> shouldn't be needed if there were no issues with DT or something like that.
To be clear:
The less we call these APIs from drivers the better. Ideally these
APIs shouldn't have existed.
--
With Best Regards,
Andy Shevchenko
Hi,
Le jeudi 14 septembre 2023 à 10:02 +0300, Andy Shevchenko a écrit :
> On Wed, Sep 13, 2023 at 11:23 PM Miquel Raynal
> <[email protected]> wrote:
> > [email protected] wrote on Wed, 13 Sep 2023 22:12:40 +0200:
> > > On Wed, Sep 13, 2023 at 10:05 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Wed, Sep 13, 2023 at 2:50 PM Bartosz Golaszewski
> > > > <[email protected]> wrote:
>
> ...
>
> > > > Why not moving this quirk to gpiolib-of.c?
> > >
> > > That's a better idea here I think, it's clearly a quirk for a
> > > buggy device tree.
> >
> > Agreed, it's just for backward compatibility purposes in a single
> > driver. I believe it should stay here.
>
> I believe Linus was for moving.
Which Linus? Because the one who's also the gpio maintainer just wrote
above that it was better to keep it in the driver.
Cheers,
-Paul
>
> gpiolib-of.c contains a lot of quirks, including this one. Calling
> these new (or old) APIs for overriding polarity in many cases
> shouldn't be needed if were no issues with DT or something like that.
>
On Thu, Sep 14, 2023 at 10:30 AM Paul Cercueil <[email protected]> wrote:
>
> Hi,
>
> Le jeudi 14 septembre 2023 à 10:02 +0300, Andy Shevchenko a écrit :
> > On Wed, Sep 13, 2023 at 11:23 PM Miquel Raynal
> > <[email protected]> wrote:
> > > [email protected] wrote on Wed, 13 Sep 2023 22:12:40 +0200:
> > > > On Wed, Sep 13, 2023 at 10:05 PM Andy Shevchenko
> > > > <[email protected]> wrote:
> > > > > On Wed, Sep 13, 2023 at 2:50 PM Bartosz Golaszewski
> > > > > <[email protected]> wrote:
> >
> > ...
> >
> > > > > Why not moving this quirk to gpiolib-of.c?
> > > >
> > > > That's a better idea here I think, it's clearly a quirk for a
> > > > buggy device tree.
> > >
> > > Agreed, it's just for backward compatibility purposes in a single
> > > driver. I believe it should stay here.
> >
> > I believe Linus was for moving.
>
> Which Linus? Because the one who's also the gpio maintainer just wrote
> above that it was better to keep it in the driver.
>
I'm also under the impression that Linus meant moving it to gpiolib-of.c. Let's
Linus: Could you clarify?
Bart
> Cheers,
> -Paul
>
> >
> > gpiolib-of.c contains a lot of quirks, including this one. Calling
> > these new (or old) APIs for overriding polarity in many cases
> > shouldn't be needed if were no issues with DT or something like that.
> >
>
On Wed, Sep 13, 2023 at 11:23 PM Miquel Raynal
<[email protected]> wrote:
> [email protected] wrote on Wed, 13 Sep 2023 22:12:40 +0200:
> > On Wed, Sep 13, 2023 at 10:05 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Wed, Sep 13, 2023 at 2:50 PM Bartosz Golaszewski <[email protected]> wrote:
...
> > > Why not moving this quirk to gpiolib-of.c?
> >
> > That's a better idea here I think, it's clearly a quirk for a
> > buggy device tree.
>
> Agreed, it's just for backward compatibility purposes in a single
> driver. I believe it should stay here.
I believe Linus was for moving.
gpiolib-of.c contains a lot of quirks, including this one. Calling
these new (or old) APIs for overriding polarity in many cases
shouldn't be needed if were no issues with DT or something like that.
--
With Best Regards,
Andy Shevchenko
On Thu, Sep 14, 2023 at 11:30 AM Bartosz Golaszewski <[email protected]> wrote:
> On Thu, Sep 14, 2023 at 10:30 AM Paul Cercueil <[email protected]> wrote:
> > > I believe Linus was for moving.
Yes.
> > Which Linus? Because the one who's also the gpio maintainer just wrote
> > above that it was better to keep it in the driver.
What. No. I expressed myself unclearly:
> > Why not moving this quirk to gpiolib-of.c?
>
> That's a better idea here I think, it's clearly a quirk for a
> buggy device tree.
"That's a better idea here I think"
means
"That's a better idea [IN THIS CASE] I think"
i.e. in this case it is a better idea to move it into gpiolib-of.c
> I'm also under the impression that Linus meant moving it to gpiolib-of.c. Let's
>
> Linus: Could you clarify?
Yes.
I invented that thing so I'm a fan of it.
Yours,
Linus Walleij