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(-)
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");
}
--
2.25.1Hi 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
© 2016 - 2025 Red Hat, Inc.