Message ID | 20230823104010.79107-4-balamanikandan.gunasundar@microchip.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | mmc: atmel-mci: Convert to gpio descriptors | expand |
Hi Balamanikandan, thanks for your patch! On Wed, Aug 23, 2023 at 12:40 PM Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com> wrote: > The polarity of the card detection gpio is handled by the "cd-inverted" > property in the device tree. Move this inversion logic to gpiolib to avoid > reading the gpio raw value. > > Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com> (...) > drivers/gpio/gpiolib-of.c | 7 +++++++ Patching here is the right approach! > +#if IS_ENABLED(CONFIG_MMC_ATMELMCI) > + if (of_device_is_compatible(np->parent, "atmel,hsmci") && > + !strcmp(propname, "cd-gpios")) { > + active_high = of_property_read_bool(np, "cd-inverted"); > + of_gpio_quirk_polarity(np, active_high, flags); > + } > +#endif > for (i = 0; i < ARRAY_SIZE(gpios); i++) { > if (of_device_is_compatible(np, gpios[i].compatible) && > !strcmp(propname, gpios[i].gpio_propname)) { This duplicates the code right below. Can't you just add an entry to the existing gpios[] array? I would imagine: static const struct { const char *compatible; const char *gpio_propname; const char *polarity_propname; } gpios[] = { #if IS_ENABLED(CONFIG_MMC_ATMELMCI) { "atmel,hsmci", "cd-gpios", "cd-inverted" }, #endif (...) Yours, Linus Walleij
Hi Linus, Thanks for reviewing the patch. On 23/08/23 6:11 pm, Linus Walleij wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Balamanikandan, > > thanks for your patch! > > On Wed, Aug 23, 2023 at 12:40 PM Balamanikandan Gunasundar > <balamanikandan.gunasundar@microchip.com> wrote: > >> The polarity of the card detection gpio is handled by the "cd-inverted" >> property in the device tree. Move this inversion logic to gpiolib to avoid >> reading the gpio raw value. >> >> Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com> > (...) >> drivers/gpio/gpiolib-of.c | 7 +++++++ > > Patching here is the right approach! > >> +#if IS_ENABLED(CONFIG_MMC_ATMELMCI) >> + if (of_device_is_compatible(np->parent, "atmel,hsmci") && The only difference above is "np->parent" while the existing code uses "np". This is because the compatible string is defined in the parent node here while the others have it in the same node. mmc0: mmc@f0000000 { compatible = "atmel,hsmci"; ... slot@0 { cd-gpios = <&pioE 0 GPIO_ACTIVE_LOW>; cd-inverted; } } >> + !strcmp(propname, "cd-gpios")) { >> + active_high = of_property_read_bool(np, "cd-inverted"); >> + of_gpio_quirk_polarity(np, active_high, flags); >> + } >> +#endif >> for (i = 0; i < ARRAY_SIZE(gpios); i++) { >> if (of_device_is_compatible(np, gpios[i].compatible) && >> !strcmp(propname, gpios[i].gpio_propname)) { > > This duplicates the code right below. Can't you just add an entry to the > existing gpios[] array? Yes! I attempted to do so. But as explained above in this particular case, of_device_is_compatible(np->parent, ..) should be called. Adding a condition check here looked clumsy. > > I would imagine: > > static const struct { > const char *compatible; > const char *gpio_propname; > const char *polarity_propname; > } gpios[] = { > #if IS_ENABLED(CONFIG_MMC_ATMELMCI) > { "atmel,hsmci", "cd-gpios", "cd-inverted" }, > #endif > (...) > I think with the above entry I can also add a check like ... if (np->parent) { if (of_device_is_compatible(np->parent, "atmel,hsmci") && !strcmp(propname, gpios[i].gpio_propname)) active_high = of_property_read_bool(); } Please let me know your thoughts. Thanks, Balamanikandan > > Yours, > Linus Walleij
On Thu, Aug 24, 2023 at 8:39 AM <Balamanikandan.Gunasundar@microchip.com> wrote: > >> +#if IS_ENABLED(CONFIG_MMC_ATMELMCI) > >> + if (of_device_is_compatible(np->parent, "atmel,hsmci") && > > The only difference above is "np->parent" while the existing code uses > "np". This is because the compatible string is defined in the parent > node here while the others have it in the same node. Aha. What about this right before the for-loop then: /* The Atmel MSMCI has the property in a child node of the device */ if (IS_ENABLED(CONFIG_MMC_ATMELMCI) && of_device_is_compatible(np->parent, "atmel,hsmci")) np = parent->np; Yours, Linus Walleij
On 24/08/23 2:03 pm, Linus Walleij wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Thu, Aug 24, 2023 at 8:39 AM <Balamanikandan.Gunasundar@microchip.com> wrote: > >>>> +#if IS_ENABLED(CONFIG_MMC_ATMELMCI) >>>> + if (of_device_is_compatible(np->parent, "atmel,hsmci") && >> >> The only difference above is "np->parent" while the existing code uses >> "np". This is because the compatible string is defined in the parent >> node here while the others have it in the same node. > > Aha. What about this right before the for-loop > then: > > /* The Atmel MSMCI has the property in a child node of the device */ > if (IS_ENABLED(CONFIG_MMC_ATMELMCI) && > of_device_is_compatible(np->parent, "atmel,hsmci")) > np = parent->np; > This looks good! I will send a v6. Thanks, Balamanikandan > Yours, > Linus Walleij
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 1436cdb5fa26..87d652c62339 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -257,6 +257,13 @@ static void of_gpio_set_polarity_by_property(const struct device_node *np, unsigned int i; bool active_high; +#if IS_ENABLED(CONFIG_MMC_ATMELMCI) + if (of_device_is_compatible(np->parent, "atmel,hsmci") && + !strcmp(propname, "cd-gpios")) { + active_high = of_property_read_bool(np, "cd-inverted"); + of_gpio_quirk_polarity(np, active_high, flags); + } +#endif for (i = 0; i < ARRAY_SIZE(gpios); i++) { if (of_device_is_compatible(np, gpios[i].compatible) && !strcmp(propname, gpios[i].gpio_propname)) { diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c index 6f815818dd22..535783c43105 100644 --- a/drivers/mmc/host/atmel-mci.c +++ b/drivers/mmc/host/atmel-mci.c @@ -206,7 +206,6 @@ enum atmci_pdc_buf { * @bus_width: Number of data lines wired up the slot * @detect_pin: GPIO pin wired to the card detect switch * @wp_pin: GPIO pin wired to the write protect sensor - * @detect_is_active_high: The state of the detect pin when it is active * @non_removable: The slot is not removable, only detect once * * If a given slot is not present on the board, @bus_width should be @@ -222,7 +221,6 @@ struct mci_slot_pdata { unsigned int bus_width; struct gpio_desc *detect_pin; struct gpio_desc *wp_pin; - bool detect_is_active_high; bool non_removable; }; @@ -405,7 +403,6 @@ struct atmel_mci { * available. * @wp_pin: GPIO pin used for card write protect sending, or negative * if not available. - * @detect_is_active_high: The state of the detect pin when it is active. * @detect_timer: Timer used for debouncing @detect_pin interrupts. */ struct atmel_mci_slot { @@ -426,7 +423,6 @@ struct atmel_mci_slot { struct gpio_desc *detect_pin; struct gpio_desc *wp_pin; - bool detect_is_active_high; struct timer_list detect_timer; }; @@ -644,6 +640,7 @@ atmci_of_init(struct platform_device *pdev) struct device_node *cnp; struct mci_platform_data *pdata; u32 slot_id; + int err; if (!np) { dev_err(&pdev->dev, "device node not found\n"); @@ -675,11 +672,12 @@ atmci_of_init(struct platform_device *pdev) pdata->slot[slot_id].detect_pin = devm_fwnode_gpiod_get(&pdev->dev, of_fwnode_handle(cnp), "cd", GPIOD_IN, "cd-gpios"); - if (IS_ERR(pdata->slot[slot_id].detect_pin)) + err = PTR_ERR_OR_ZERO(pdata->slot[slot_id].detect_pin); + if (err) { + if (err != -ENOENT) + return ERR_PTR(err); pdata->slot[slot_id].detect_pin = NULL; - - pdata->slot[slot_id].detect_is_active_high = - of_property_read_bool(cnp, "cd-inverted"); + } pdata->slot[slot_id].non_removable = of_property_read_bool(cnp, "non-removable"); @@ -687,8 +685,12 @@ atmci_of_init(struct platform_device *pdev) pdata->slot[slot_id].wp_pin = devm_fwnode_gpiod_get(&pdev->dev, of_fwnode_handle(cnp), "wp", GPIOD_IN, "wp-gpios"); - if (IS_ERR(pdata->slot[slot_id].wp_pin)) + err = PTR_ERR_OR_ZERO(pdata->slot[slot_id].wp_pin); + if (err) { + if (err != -ENOENT) + return ERR_PTR(err); pdata->slot[slot_id].wp_pin = NULL; + } } return pdata; @@ -1566,8 +1568,7 @@ static int atmci_get_cd(struct mmc_host *mmc) struct atmel_mci_slot *slot = mmc_priv(mmc); if (slot->detect_pin) { - present = !(gpiod_get_raw_value(slot->detect_pin) ^ - slot->detect_is_active_high); + present = gpiod_get_value_cansleep(slot->detect_pin); dev_dbg(&mmc->class_dev, "card is %spresent\n", present ? "" : "not "); } @@ -1680,8 +1681,7 @@ static void atmci_detect_change(struct timer_list *t) return; enable_irq(gpiod_to_irq(slot->detect_pin)); - present = !(gpiod_get_raw_value(slot->detect_pin) ^ - slot->detect_is_active_high); + present = gpiod_get_value_cansleep(slot->detect_pin); present_old = test_bit(ATMCI_CARD_PRESENT, &slot->flags); dev_vdbg(&slot->mmc->class_dev, "detect change: %d (was %d)\n", @@ -2272,7 +2272,6 @@ static int atmci_init_slot(struct atmel_mci *host, slot->host = host; slot->detect_pin = slot_data->detect_pin; slot->wp_pin = slot_data->wp_pin; - slot->detect_is_active_high = slot_data->detect_is_active_high; slot->sdc_reg = sdc_reg; slot->sdio_irq = sdio_irq; @@ -2280,7 +2279,7 @@ static int atmci_init_slot(struct atmel_mci *host, "slot[%u]: bus_width=%u, detect_pin=%d, " "detect_is_active_high=%s, wp_pin=%d\n", id, slot_data->bus_width, desc_to_gpio(slot_data->detect_pin), - slot_data->detect_is_active_high ? "true" : "false", + !gpiod_is_active_low(slot_data->detect_pin) ? "true" : "false", desc_to_gpio(slot_data->wp_pin)); mmc->ops = &atmci_ops; @@ -2318,10 +2317,8 @@ static int atmci_init_slot(struct atmel_mci *host, /* Assume card is present initially */ set_bit(ATMCI_CARD_PRESENT, &slot->flags); if (slot->detect_pin) { - if (gpiod_get_raw_value(slot->detect_pin) ^ - slot->detect_is_active_high) { + if (!gpiod_get_value_cansleep(slot->detect_pin)) clear_bit(ATMCI_CARD_PRESENT, &slot->flags); - } } else { dev_dbg(&mmc->class_dev, "no detect pin available\n"); }
The polarity of the card detection gpio is handled by the "cd-inverted" property in the device tree. Move this inversion logic to gpiolib to avoid reading the gpio raw value. Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com> --- drivers/gpio/gpiolib-of.c | 7 +++++++ drivers/mmc/host/atmel-mci.c | 33 +++++++++++++++------------------ 2 files changed, 22 insertions(+), 18 deletions(-)