diff mbox series

[3/5] mmc: slot-gpio: use gpiod_set_active_[low|high]()

Message ID 20230913115001.23183-4-brgl@bgdev.pl
State New
Headers
Series gpio: remove gpiod_toggle_active_low() |

Commit Message

Bartosz Golaszewski Sept. 13, 2023, 11:49 a.m. UTC 
  From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We have new, less cumbersome and clearer interfaces for controlling GPIO
polarity. Use them in the MMC code.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/mmc/core/slot-gpio.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Bartosz Golaszewski Sept. 13, 2023, 12:39 p.m. UTC | #1 
On Wed, Sep 13, 2023 at 2:24 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Sep 13, 2023 at 1:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We have new, less cumbersome and clearer interfaces for controlling GPIO
> > polarity. Use them in the MMC code.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> I like the looks of the code better, obviously but this looks like this for
> a reason unfortunately.
>
> See the following from
> Documentation/devicetree/bindings/mmc/mmc-controller.yaml:
>
>   # CD and WP lines can be implemented on the hardware in one of two
>   # ways: as GPIOs, specified in cd-gpios and wp-gpios properties, or
>   # as dedicated pins. Polarity of dedicated pins can be specified,
>   # using *-inverted properties. GPIO polarity can also be specified
>   # using the GPIO_ACTIVE_LOW flag. This creates an ambiguity in the
>   # latter case. We choose to use the XOR logic for GPIO CD and WP
>   # lines.  This means, the two properties are "superimposed," for
>   # example leaving the GPIO_ACTIVE_LOW flag clear and specifying the
>   # respective *-inverted property property results in a
>   # double-inversion and actually means the "normal" line polarity is
>   # in effect.
>

I hate it, thanks. :)

> Will you still provide the desired "double inversion" after this patch?
>

Not in the current form. Would it work to go:

if (override_active_level) {
    if (!(host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH))
        gpiod_set_active_high(desc);
    else
        gpiod_set_active_low(desc);
} else {
    if (host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
        gpiod_set_active_high(desc);
    else
        gpiod_set_active_low(desc);
}

?

Alternatively we could reimplement the toggle semantics locally in a
helper function in order to get rid of it from GPIOLIB.

Bart
Linus Walleij Sept. 14, 2023, 8:31 a.m. UTC | #2 
On Wed, Sep 13, 2023 at 2:39 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Wed, Sep 13, 2023 at 2:24 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Wed, Sep 13, 2023 at 1:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > We have new, less cumbersome and clearer interfaces for controlling GPIO
> > > polarity. Use them in the MMC code.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > I like the looks of the code better, obviously but this looks like this for
> > a reason unfortunately.
> >
> > See the following from
> > Documentation/devicetree/bindings/mmc/mmc-controller.yaml:
> >
> >   # CD and WP lines can be implemented on the hardware in one of two
> >   # ways: as GPIOs, specified in cd-gpios and wp-gpios properties, or
> >   # as dedicated pins. Polarity of dedicated pins can be specified,
> >   # using *-inverted properties. GPIO polarity can also be specified
> >   # using the GPIO_ACTIVE_LOW flag. This creates an ambiguity in the
> >   # latter case. We choose to use the XOR logic for GPIO CD and WP
> >   # lines.  This means, the two properties are "superimposed," for
> >   # example leaving the GPIO_ACTIVE_LOW flag clear and specifying the
> >   # respective *-inverted property property results in a
> >   # double-inversion and actually means the "normal" line polarity is
> >   # in effect.
> >
>
> I hate it, thanks. :)
>
> > Will you still provide the desired "double inversion" after this patch?
> >
>
> Not in the current form. Would it work to go:
>
> if (override_active_level) {
>     if (!(host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH))
>         gpiod_set_active_high(desc);
>     else
>         gpiod_set_active_low(desc);
> } else {
>     if (host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
>         gpiod_set_active_high(desc);
>     else
>         gpiod_set_active_low(desc);
> }
>
> ?

I *think* so but my boolean parser i known to be flawed since I have
screwed up double inversions repeatedly over the years, so it should
not be trusted at all.

> Alternatively we could reimplement the toggle semantics locally in a
> helper function in order to get rid of it from GPIOLIB.

I don't know about that, the flag is inside gpio_desc so we cannot
access it (struct is private to gpiolib...)

Yours,
Linus Walleij
Linus Walleij Sept. 14, 2023, 8:38 a.m. UTC | #3 
On Thu, Sep 14, 2023 at 10:31 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 13, 2023 at 2:39 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > Alternatively we could reimplement the toggle semantics locally in a
> > helper function in order to get rid of it from GPIOLIB.
>
> I don't know about that, the flag is inside gpio_desc so we cannot
> access it (struct is private to gpiolib...)

Actually I think the way the toggle call came about was for this one
MMC usecase.

Then other subsystems have used it without asking the GPIO
maintainers or without implementing the more proper accessors
or patching drivers/gpio/gpiolib-of.c because why not, probably
thinking something like "hey weird that it is just toggle I guess they
are not so smart, but it works, ship it".

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 2a2d949a9344..a6fea6559a5e 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -204,12 +204,11 @@  int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
 	}
 
 	/* override forces default (active-low) polarity ... */
-	if (override_active_level && !gpiod_is_active_low(desc))
-		gpiod_toggle_active_low(desc);
-
+	if (override_active_level)
+		gpiod_set_active_low(desc);
 	/* ... or active-high */
-	if (host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
-		gpiod_toggle_active_low(desc);
+	else if (host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
+		gpiod_set_active_high(desc);
 
 	ctx->cd_gpio = desc;
 
@@ -256,7 +255,7 @@  int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
 	}
 
 	if (host->caps2 & MMC_CAP2_RO_ACTIVE_HIGH)
-		gpiod_toggle_active_low(desc);
+		gpiod_set_active_high(desc);
 
 	ctx->ro_gpio = desc;