[3/4,v2] mmc: host: switch OF parser to use gpio descriptors
diff mbox

Message ID 1409137253-25189-3-git-send-email-linus.walleij@linaro.org
State Not Applicable
Delegated to: Linus Walleij
Headers show

Commit Message

Linus Walleij Aug. 27, 2014, 11 a.m. UTC
This switches the central MMC OF parser to use gpio descriptors
instead of grabbing GPIOs explicitly from the device tree.
This strips out an unecessary use of the integer-based GPIO
API that we want to get rid of, cuts down on code as the
gpio descriptor code will handle active low flags.

Acked-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Restore error reporting as done in previous stand-alone
  patch.
---
 drivers/mmc/core/host.c | 68 +++++++++++++++++--------------------------------
 1 file changed, 23 insertions(+), 45 deletions(-)

Comments

Ulf Hansson Aug. 29, 2014, 12:16 p.m. UTC | #1
On 27 August 2014 13:00, Linus Walleij <linus.walleij@linaro.org> wrote:
> This switches the central MMC OF parser to use gpio descriptors
> instead of grabbing GPIOs explicitly from the device tree.
> This strips out an unecessary use of the integer-based GPIO
> API that we want to get rid of, cuts down on code as the
> gpio descriptor code will handle active low flags.
>
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks! Applied for next.

Kind regards
Uffe


> ---
> ChangeLog v1->v2:
> - Restore error reporting as done in previous stand-alone
>   patch.
> ---
>  drivers/mmc/core/host.c | 68 +++++++++++++++++--------------------------------
>  1 file changed, 23 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 95cceae96944..6f7ed9c50346 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -310,9 +310,7 @@ int mmc_of_parse(struct mmc_host *host)
>  {
>         struct device_node *np;
>         u32 bus_width;
> -       bool explicit_inv_wp, gpio_inv_wp = false;
> -       enum of_gpio_flags flags;
> -       int len, ret, gpio;
> +       int len, ret;
>
>         if (!host->parent || !host->parent->of_node)
>                 return 0;
> @@ -360,60 +358,40 @@ int mmc_of_parse(struct mmc_host *host)
>         if (of_find_property(np, "non-removable", &len)) {
>                 host->caps |= MMC_CAP_NONREMOVABLE;
>         } else {
> -               bool explicit_inv_cd, gpio_inv_cd = false;
> -
> -               explicit_inv_cd = of_property_read_bool(np, "cd-inverted");
> +               if (of_property_read_bool(np, "cd-inverted"))
> +                       host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
>
>                 if (of_find_property(np, "broken-cd", &len))
>                         host->caps |= MMC_CAP_NEEDS_POLL;
>
> -               gpio = of_get_named_gpio_flags(np, "cd-gpios", 0, &flags);
> -               if (gpio == -EPROBE_DEFER)
> -                       return gpio;
> -               if (gpio_is_valid(gpio)) {
> -                       if (!(flags & OF_GPIO_ACTIVE_LOW))
> -                               gpio_inv_cd = true;
> -
> -                       ret = mmc_gpio_request_cd(host, gpio, 0);
> -                       if (ret < 0) {
> -                               dev_err(host->parent,
> -                                       "Failed to request CD GPIO #%d: %d!\n",
> -                                       gpio, ret);
> +               ret = mmc_gpiod_request_cd(host, "cd", 0, false, 0);
> +               if (ret) {
> +                       if (ret == -EPROBE_DEFER)
>                                 return ret;
> -                       } else {
> -                               dev_info(host->parent, "Got CD GPIO #%d.\n",
> -                                        gpio);
> +                       if (ret != -ENOENT) {
> +                               dev_err(host->parent,
> +                                       "Failed to request CD GPIO: %d\n",
> +                                       ret);
>                         }
> -               }
> -
> -               if (explicit_inv_cd ^ gpio_inv_cd)
> -                       host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> +               } else
> +                       dev_info(host->parent, "Got CD GPIO\n");
>         }
>
>         /* Parse Write Protection */
> -       explicit_inv_wp = of_property_read_bool(np, "wp-inverted");
> -
> -       gpio = of_get_named_gpio_flags(np, "wp-gpios", 0, &flags);
> -       if (gpio == -EPROBE_DEFER) {
> -               ret = -EPROBE_DEFER;
> -               goto out;
> -       }
> -       if (gpio_is_valid(gpio)) {
> -               if (!(flags & OF_GPIO_ACTIVE_LOW))
> -                       gpio_inv_wp = true;
> +       if (of_property_read_bool(np, "wp-inverted"))
> +               host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>
> -               ret = mmc_gpio_request_ro(host, gpio);
> -               if (ret < 0) {
> -                       dev_err(host->parent,
> -                               "Failed to request WP GPIO: %d!\n", ret);
> +       ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0);
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER)
>                         goto out;
> -               } else {
> -                               dev_info(host->parent, "Got WP GPIO #%d.\n",
> -                                        gpio);
> +               if (ret != -ENOENT) {
> +                       dev_err(host->parent,
> +                               "Failed to request WP GPIO: %d\n",
> +                               ret);
>                 }
> -       }
> -       if (explicit_inv_wp ^ gpio_inv_wp)
> -               host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
> +       } else
> +               dev_info(host->parent, "Got WP GPIO\n");
>
>         if (of_find_property(np, "cap-sd-highspeed", &len))
>                 host->caps |= MMC_CAP_SD_HIGHSPEED;
> --
> 1.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Sept. 9, 2014, 7:05 a.m. UTC | #2
On Wed, Aug 27, 2014 at 1:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> This switches the central MMC OF parser to use gpio descriptors
> instead of grabbing GPIOs explicitly from the device tree.
> This strips out an unecessary use of the integer-based GPIO
> API that we want to get rid of, cuts down on code as the
> gpio descriptor code will handle active low flags.
>
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Restore error reporting as done in previous stand-alone
>   patch.

Hi Ulf,

the v2 version of this patch set should be working fine on
v3.17-rc4, as I merged a patch setting up the correct
prototypes for systems without gpiolib.

Fenguang's build tests are working on it atleast, I promise...

Tell me if you want me to resend the patch set or if you can
take the same patches again, but on top of -rc4. There is
no change in them.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Sept. 9, 2014, 12:29 p.m. UTC | #3
On 9 September 2014 09:05, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Aug 27, 2014 at 1:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> This switches the central MMC OF parser to use gpio descriptors
>> instead of grabbing GPIOs explicitly from the device tree.
>> This strips out an unecessary use of the integer-based GPIO
>> API that we want to get rid of, cuts down on code as the
>> gpio descriptor code will handle active low flags.
>>
>> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> ChangeLog v1->v2:
>> - Restore error reporting as done in previous stand-alone
>>   patch.
>
> Hi Ulf,
>
> the v2 version of this patch set should be working fine on
> v3.17-rc4, as I merged a patch setting up the correct
> prototypes for systems without gpiolib.
>
> Fenguang's build tests are working on it atleast, I promise...
>
> Tell me if you want me to resend the patch set or if you can
> take the same patches again, but on top of -rc4. There is
> no change in them.
>

Hi Linus,

I have re-applied them again, let's hope for better luck this time. :-)

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas Sept. 30, 2014, 11:30 a.m. UTC | #4
Hello Linus,

On Wed, Aug 27, 2014 at 1:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> This switches the central MMC OF parser to use gpio descriptors
> instead of grabbing GPIOs explicitly from the device tree.
> This strips out an unecessary use of the integer-based GPIO
> API that we want to get rid of, cuts down on code as the
> gpio descriptor code will handle active low flags.
>
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>

Card detection is failing in some boards because this patch changes
some semantics. I tried to come up with a fix but I didn't find an
obvious way to do it (more on that below).

>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 95cceae96944..6f7ed9c50346 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -310,9 +310,7 @@ int mmc_of_parse(struct mmc_host *host)
>  {
>         struct device_node *np;
>         u32 bus_width;
> -       bool explicit_inv_wp, gpio_inv_wp = false;
> -       enum of_gpio_flags flags;
> -       int len, ret, gpio;
> +       int len, ret;
>
>         if (!host->parent || !host->parent->of_node)
>                 return 0;
> @@ -360,60 +358,40 @@ int mmc_of_parse(struct mmc_host *host)
>         if (of_find_property(np, "non-removable", &len)) {
>                 host->caps |= MMC_CAP_NONREMOVABLE;
>         } else {
> -               bool explicit_inv_cd, gpio_inv_cd = false;
> -
> -               explicit_inv_cd = of_property_read_bool(np, "cd-inverted");
> +               if (of_property_read_bool(np, "cd-inverted"))
> +                       host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
>

The card-detect signal active high flag is set if the "cd-inverted"
property is found but in the original code MMC_CAP2_CD_ACTIVE_HIGH was
set only if "cd-inverted" was true and the card detect GPIO flag was
OF_GPIO_ACTIVE_HIGH.

>                 if (of_find_property(np, "broken-cd", &len))
>                         host->caps |= MMC_CAP_NEEDS_POLL;
>
> -               gpio = of_get_named_gpio_flags(np, "cd-gpios", 0, &flags);
> -               if (gpio == -EPROBE_DEFER)
> -                       return gpio;
> -               if (gpio_is_valid(gpio)) {
> -                       if (!(flags & OF_GPIO_ACTIVE_LOW))
> -                               gpio_inv_cd = true;
> -
> -                       ret = mmc_gpio_request_cd(host, gpio, 0);
> -                       if (ret < 0) {
> -                               dev_err(host->parent,
> -                                       "Failed to request CD GPIO #%d: %d!\n",
> -                                       gpio, ret);
> +               ret = mmc_gpiod_request_cd(host, "cd", 0, false, 0);

Now false is passed as the override_active_level bool parameter but
mmc_gpio_request_cd() sets ctx->override_cd_active_level = true. Which
means that after this patch, mmc_gpio_get_cd() does not take into
account if host->caps2 has MMC_CAP2_CD_ACTIVE_HIGH set.

> +               if (ret) {
> +                       if (ret == -EPROBE_DEFER)
>                                 return ret;
> -                       } else {
> -                               dev_info(host->parent, "Got CD GPIO #%d.\n",
> -                                        gpio);
> +                       if (ret != -ENOENT) {
> +                               dev_err(host->parent,
> +                                       "Failed to request CD GPIO: %d\n",
> +                                       ret);
>                         }
> -               }
> -
> -               if (explicit_inv_cd ^ gpio_inv_cd)
> -                       host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> +               } else
> +                       dev_info(host->parent, "Got CD GPIO\n");
>         }
>
>         /* Parse Write Protection */
> -       explicit_inv_wp = of_property_read_bool(np, "wp-inverted");
> -
> -       gpio = of_get_named_gpio_flags(np, "wp-gpios", 0, &flags);
> -       if (gpio == -EPROBE_DEFER) {
> -               ret = -EPROBE_DEFER;
> -               goto out;
> -       }
> -       if (gpio_is_valid(gpio)) {
> -               if (!(flags & OF_GPIO_ACTIVE_LOW))
> -                       gpio_inv_wp = true;
> +       if (of_property_read_bool(np, "wp-inverted"))
> +               host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>
> -               ret = mmc_gpio_request_ro(host, gpio);
> -               if (ret < 0) {
> -                       dev_err(host->parent,
> -                               "Failed to request WP GPIO: %d!\n", ret);
> +       ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0);
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER)
>                         goto out;
> -               } else {
> -                               dev_info(host->parent, "Got WP GPIO #%d.\n",
> -                                        gpio);
> +               if (ret != -ENOENT) {
> +                       dev_err(host->parent,
> +                               "Failed to request WP GPIO: %d\n",
> +                               ret);
>                 }
> -       }
> -       if (explicit_inv_wp ^ gpio_inv_wp)
> -               host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;

Same issue with write-protect signal active high. After this patch
MMC_CAP2_RO_ACTIVE_HIGH is always set while before it was
conditionally set depending of the write-protect GPIO flag value.

> +       } else
> +               dev_info(host->parent, "Got WP GPIO\n");
>
>         if (of_find_property(np, "cap-sd-highspeed", &len))
>                 host->caps |= MMC_CAP_SD_HIGHSPEED;
> --

Passing true to mmc_gpiod_request_cd() and not setting
MMC_CAP2_RO_ACTIVE_HIGH makes card detection work again on this board.
The former is a one-liner but the later is more complicated since
AFAIU the idea of the GPIO descriptor code is to handle the flags but
in this case this detail is needed.

Should we set MMC_CAP2_{CD,RO}_ACTIVE_HIGH in
mmc_gpiod_request_{cd,ro} or add functions to get the flags for these
cd and wp GPIO in order to set the host->caps2 flags according to the
old logic?

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Sept. 30, 2014, 2:01 p.m. UTC | #5
On Tue, Sep 30, 2014 at 1:30 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> On Wed, Aug 27, 2014 at 1:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> This switches the central MMC OF parser to use gpio descriptors
>> instead of grabbing GPIOs explicitly from the device tree.
>> This strips out an unecessary use of the integer-based GPIO
>> API that we want to get rid of, cuts down on code as the
>> gpio descriptor code will handle active low flags.
>>
>> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Card detection is failing in some boards because this patch changes
> some semantics. I tried to come up with a fix but I didn't find an
> obvious way to do it (more on that below).

I think some of this may be weird semantics: the CAP flags in the
MMC subsystem, MMC_CAP2_CD_ACTIVE_HIGH and
MMC_CAP2_RO_ACTIVE_HIGH are somewhat confusing since
the GPIO subsystem nowadays has its own concept of a line
active high or low.

The MMC caps make a lot of sense if the host has dedicated lines
to read CD and WP, and does not use a GPIO for this.

When a GPIO descriptor is in use, the thing gets a bit convoluted:
what if both the GPIO *and* the CAP flag is set to inversion?

So the old code did this with XOR:

        if (explicit_inv_cd ^ gpio_inv_cd)
            host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;

Meaning double inversion -> no inversion.

I guess I was sort of hoping that nobody did this crazy thing.
Turns out I was wrong, so have to restore the old semantics...

I'm sending a patch for you to test.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 95cceae96944..6f7ed9c50346 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -310,9 +310,7 @@  int mmc_of_parse(struct mmc_host *host)
 {
 	struct device_node *np;
 	u32 bus_width;
-	bool explicit_inv_wp, gpio_inv_wp = false;
-	enum of_gpio_flags flags;
-	int len, ret, gpio;
+	int len, ret;
 
 	if (!host->parent || !host->parent->of_node)
 		return 0;
@@ -360,60 +358,40 @@  int mmc_of_parse(struct mmc_host *host)
 	if (of_find_property(np, "non-removable", &len)) {
 		host->caps |= MMC_CAP_NONREMOVABLE;
 	} else {
-		bool explicit_inv_cd, gpio_inv_cd = false;
-
-		explicit_inv_cd = of_property_read_bool(np, "cd-inverted");
+		if (of_property_read_bool(np, "cd-inverted"))
+			host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
 
 		if (of_find_property(np, "broken-cd", &len))
 			host->caps |= MMC_CAP_NEEDS_POLL;
 
-		gpio = of_get_named_gpio_flags(np, "cd-gpios", 0, &flags);
-		if (gpio == -EPROBE_DEFER)
-			return gpio;
-		if (gpio_is_valid(gpio)) {
-			if (!(flags & OF_GPIO_ACTIVE_LOW))
-				gpio_inv_cd = true;
-
-			ret = mmc_gpio_request_cd(host, gpio, 0);
-			if (ret < 0) {
-				dev_err(host->parent,
-					"Failed to request CD GPIO #%d: %d!\n",
-					gpio, ret);
+		ret = mmc_gpiod_request_cd(host, "cd", 0, false, 0);
+		if (ret) {
+			if (ret == -EPROBE_DEFER)
 				return ret;
-			} else {
-				dev_info(host->parent, "Got CD GPIO #%d.\n",
-					 gpio);
+			if (ret != -ENOENT) {
+				dev_err(host->parent,
+					"Failed to request CD GPIO: %d\n",
+					ret);
 			}
-		}
-
-		if (explicit_inv_cd ^ gpio_inv_cd)
-			host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+		} else
+			dev_info(host->parent, "Got CD GPIO\n");
 	}
 
 	/* Parse Write Protection */
-	explicit_inv_wp = of_property_read_bool(np, "wp-inverted");
-
-	gpio = of_get_named_gpio_flags(np, "wp-gpios", 0, &flags);
-	if (gpio == -EPROBE_DEFER) {
-		ret = -EPROBE_DEFER;
-		goto out;
-	}
-	if (gpio_is_valid(gpio)) {
-		if (!(flags & OF_GPIO_ACTIVE_LOW))
-			gpio_inv_wp = true;
+	if (of_property_read_bool(np, "wp-inverted"))
+		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
 
-		ret = mmc_gpio_request_ro(host, gpio);
-		if (ret < 0) {
-			dev_err(host->parent,
-				"Failed to request WP GPIO: %d!\n", ret);
+	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
 			goto out;
-		} else {
-				dev_info(host->parent, "Got WP GPIO #%d.\n",
-					 gpio);
+		if (ret != -ENOENT) {
+			dev_err(host->parent,
+				"Failed to request WP GPIO: %d\n",
+				ret);
 		}
-	}
-	if (explicit_inv_wp ^ gpio_inv_wp)
-		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
+	} else
+		dev_info(host->parent, "Got WP GPIO\n");
 
 	if (of_find_property(np, "cap-sd-highspeed", &len))
 		host->caps |= MMC_CAP_SD_HIGHSPEED;