| Message ID | 20230913115001.23183-4-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | gpio: remove gpiod_toggle_active_low() | expand |
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. Will you still provide the desired "double inversion" after this patch? Yours, Linus Walleij
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
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
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 --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;