[PATCH 1/4] mmc: slot-gpio: switch to use flags when getting GPIO

[PATCH 1/4] mmc: slot-gpio: switch to use flags when getting GPIO

[Linus Walleij]

When the slot GPIO driver gets the GPIO to be used for card
detect, it is now possible to specify a flag to have the line
set up as input. Get rid of the explicit setup call for input
and use the flag.

The extra argument works as there are transition varargs
macros in place in the <linux/gpio/consumer.h> header, in
the future we will make the flags argument compulsory.

Cc: Alexandre Courbot <gnurou@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/slot-gpio.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 5f89cb83d5f0..908c2b29e79f 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -308,14 +308,10 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
 	if (!con_id)
 		con_id = ctx->cd_label;
 
-	desc = devm_gpiod_get_index(host->parent, con_id, idx);
+	desc = devm_gpiod_get_index(host->parent, con_id, idx, GPIOD_IN);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
-	ret = gpiod_direction_input(desc);
-	if (ret < 0)
-		return ret;
-
 	if (debounce) {
 		ret = gpiod_set_debounce(desc, debounce);
 		if (ret < 0)
-- 
1.9.3

[PATCH 2/4] mmc: slot-gpio: add gpiod variant to get wp GPIO

[Linus Walleij]

This makes it possible to get the write protect (read only)
GPIO line from a GPIO descriptor. Written to exactly mirror
the card detect function.

Cc: Alexandre Courbot <gnurou@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/slot-gpio.c  | 48 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/slot-gpio.h |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 908c2b29e79f..e3fce4493fab 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -326,6 +326,54 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
 EXPORT_SYMBOL(mmc_gpiod_request_cd);
 
 /**
+ * mmc_gpiod_request_ro - request a gpio descriptor for write protection
+ * @host: mmc host
+ * @con_id: function within the GPIO consumer
+ * @idx: index of the GPIO to obtain in the consumer
+ * @override_active_level: ignore %GPIO_ACTIVE_LOW flag
+ * @debounce: debounce time in microseconds
+ *
+ * Use this function in place of mmc_gpio_request_ro() to use the GPIO
+ * descriptor API.  Note that it is paired with mmc_gpiod_free_ro() not
+ * mmc_gpio_free_ro().
+ *
+ * Returns zero on success, else an error.
+ */
+int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
+			 unsigned int idx, bool override_active_level,
+			 unsigned int debounce)
+{
+	struct mmc_gpio *ctx;
+	struct gpio_desc *desc;
+	int ret;
+
+	ret = mmc_gpio_alloc(host);
+	if (ret < 0)
+		return ret;
+
+	ctx = host->slot.handler_priv;
+
+	if (!con_id)
+		con_id = ctx->ro_label;
+
+	desc = devm_gpiod_get_index(host->parent, con_id, idx, GPIOD_IN);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	if (debounce) {
+		ret = gpiod_set_debounce(desc, debounce);
+		if (ret < 0)
+			return ret;
+	}
+
+	ctx->override_ro_active_level = override_active_level;
+	ctx->ro_gpio = desc;
+
+	return 0;
+}
+EXPORT_SYMBOL(mmc_gpiod_request_ro);
+
+/**
  * mmc_gpiod_free_cd - free the card-detection gpio descriptor
  * @host: mmc host
  *
diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
index d2433381e828..a0d0442c15bf 100644
--- a/include/linux/mmc/slot-gpio.h
+++ b/include/linux/mmc/slot-gpio.h
@@ -25,6 +25,9 @@ void mmc_gpio_free_cd(struct mmc_host *host);
 int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
 			 unsigned int idx, bool override_active_level,
 			 unsigned int debounce);
+int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
+			 unsigned int idx, bool override_active_level,
+			 unsigned int debounce);
 void mmc_gpiod_free_cd(struct mmc_host *host);
 void mmc_gpiod_request_cd_irq(struct mmc_host *host);
 
-- 
1.9.3

Re: [PATCH 2/4] mmc: slot-gpio: add gpiod variant to get wp GPIO

[Alexandre Courbot]

On Tue, Aug 12, 2014 at 10:25 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> This makes it possible to get the write protect (read only)
> GPIO line from a GPIO descriptor. Written to exactly mirror
> the card detect function.

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 2/4] mmc: slot-gpio: add gpiod variant to get wp GPIO

[Ulf Hansson]

On 12 August 2014 19:25, Linus Walleij <linus.walleij@linaro.org> wrote:
> This makes it possible to get the write protect (read only)
> GPIO line from a GPIO descriptor. Written to exactly mirror
> the card detect function.
>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks! Applied for next.

Kind regards
Uffe


> ---
>  drivers/mmc/core/slot-gpio.c  | 48 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/slot-gpio.h |  3 +++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 908c2b29e79f..e3fce4493fab 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -326,6 +326,54 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
>  EXPORT_SYMBOL(mmc_gpiod_request_cd);
>
>  /**
> + * mmc_gpiod_request_ro - request a gpio descriptor for write protection
> + * @host: mmc host
> + * @con_id: function within the GPIO consumer
> + * @idx: index of the GPIO to obtain in the consumer
> + * @override_active_level: ignore %GPIO_ACTIVE_LOW flag
> + * @debounce: debounce time in microseconds
> + *
> + * Use this function in place of mmc_gpio_request_ro() to use the GPIO
> + * descriptor API.  Note that it is paired with mmc_gpiod_free_ro() not
> + * mmc_gpio_free_ro().
> + *
> + * Returns zero on success, else an error.
> + */
> +int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
> +                        unsigned int idx, bool override_active_level,
> +                        unsigned int debounce)
> +{
> +       struct mmc_gpio *ctx;
> +       struct gpio_desc *desc;
> +       int ret;
> +
> +       ret = mmc_gpio_alloc(host);
> +       if (ret < 0)
> +               return ret;
> +
> +       ctx = host->slot.handler_priv;
> +
> +       if (!con_id)
> +               con_id = ctx->ro_label;
> +
> +       desc = devm_gpiod_get_index(host->parent, con_id, idx, GPIOD_IN);
> +       if (IS_ERR(desc))
> +               return PTR_ERR(desc);
> +
> +       if (debounce) {
> +               ret = gpiod_set_debounce(desc, debounce);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       ctx->override_ro_active_level = override_active_level;
> +       ctx->ro_gpio = desc;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(mmc_gpiod_request_ro);
> +
> +/**
>   * mmc_gpiod_free_cd - free the card-detection gpio descriptor
>   * @host: mmc host
>   *
> diff --git a/include/linux/mmc/slot-gpio.h b/include/linux/mmc/slot-gpio.h
> index d2433381e828..a0d0442c15bf 100644
> --- a/include/linux/mmc/slot-gpio.h
> +++ b/include/linux/mmc/slot-gpio.h
> @@ -25,6 +25,9 @@ void mmc_gpio_free_cd(struct mmc_host *host);
>  int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
>                          unsigned int idx, bool override_active_level,
>                          unsigned int debounce);
> +int mmc_gpiod_request_ro(struct mmc_host *host, const char *con_id,
> +                        unsigned int idx, bool override_active_level,
> +                        unsigned int debounce);
>  void mmc_gpiod_free_cd(struct mmc_host *host);
>  void mmc_gpiod_request_cd_irq(struct mmc_host *host);
>
> --
> 1.9.3
>

[PATCH 3/4] mmc: host: switch OF parser to use gpio descriptors

[Linus Walleij]

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.

As it is in no way a bug not to supply CD/WP GPIOs the messages
about them not being specified have been depromoted from
dev_err() to dev_dbg().

Cc: Alexandre Courbot <gnurou@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/host.c | 68 +++++++++++++++----------------------------------
 1 file changed, 21 insertions(+), 47 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 95cceae96944..048c6d687cc9 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,36 @@ 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 (explicit_inv_cd ^ gpio_inv_cd)
-			host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
+			dev_dbg(host->parent,
+				"Failed to request CD GPIO: %d\n",
+				ret);
+		} else
+			dev_info(host->parent, "Got CD GPIO\n");
 	}
 
 	/* Parse Write Protection */
-	explicit_inv_wp = of_property_read_bool(np, "wp-inverted");
+	if (of_property_read_bool(np, "wp-inverted"))
+		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
 
-	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;
-
-		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 (explicit_inv_wp ^ gpio_inv_wp)
-		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
+		dev_dbg(host->parent,
+			"Failed to request WP GPIO: %d\n",
+			ret);
+	} 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

Re: [PATCH 3/4] mmc: host: switch OF parser to use gpio descriptors

[Alexandre Courbot]

On Tue, Aug 12, 2014 at 10:25 AM, 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.
>
> As it is in no way a bug not to supply CD/WP GPIOs the messages
> about them not being specified have been depromoted from
> dev_err() to dev_dbg().

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 3/4] mmc: host: switch OF parser to use gpio descriptors

[Ulf Hansson]

On 12 August 2014 19:25, 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.
>
> As it is in no way a bug not to supply CD/WP GPIOs the messages
> about them not being specified have been depromoted from
> dev_err() to dev_dbg().
>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks! Applied for next.

Kind regards
Uffe


> ---
>  drivers/mmc/core/host.c | 68 +++++++++++++++----------------------------------
>  1 file changed, 21 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 95cceae96944..048c6d687cc9 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,36 @@ 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 (explicit_inv_cd ^ gpio_inv_cd)
> -                       host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
> +                       dev_dbg(host->parent,
> +                               "Failed to request CD GPIO: %d\n",
> +                               ret);
> +               } else
> +                       dev_info(host->parent, "Got CD GPIO\n");
>         }
>
>         /* Parse Write Protection */
> -       explicit_inv_wp = of_property_read_bool(np, "wp-inverted");
> +       if (of_property_read_bool(np, "wp-inverted"))
> +               host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
>
> -       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;
> -
> -               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 (explicit_inv_wp ^ gpio_inv_wp)
> -               host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
> +               dev_dbg(host->parent,
> +                       "Failed to request WP GPIO: %d\n",
> +                       ret);
> +       } 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
>

Re: [PATCH 3/4] mmc: host: switch OF parser to use gpio descriptors

[Simon Baatz]

Hi Linus,

On Tue, Aug 12, 2014 at 07:25:54PM +0200, Linus Walleij 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.
> 
> As it is in no way a bug not to supply CD/WP GPIOs the messages
> about them not being specified have been depromoted from
> dev_err() to dev_dbg().

I think the intention of the current code is to issue an
error if a valid GPIO was specified but could not be obtained.  It
should not issue an error just because the CD/WP GPIOs are not
specified.

This patch changes this behaviour. If the GPIO is specified
explicitly but cannot be obtained for some reason, the driver's probe
calling mmc_of_parse() will probably be successful.  Since debug
level messages are usually not enabled, the problem with the GPIO may
not even be visible in the logs.  For example, if I specify the same
GPIO for CD and WP in the DTS:

[    2.093442] kirkwood-pinctrl f1010000.pin-controller: request pin
44 (PIN44) for mvebu-gpio:44
[    2.093459] mvsdio f1090000.mvsdio: Got CD GPIO
[    2.118734] mv643xx_eth_port mv643xx_eth_port.0 eth0: port 0 with
MAC address 00:50:43:c3:55:55

One can only infer from the missing "Got WP GPIO" that something went
wrong.  Previously, the driver would not even load:

...
[    1.973402] mvsdio f1090000.mvsdio: Failed to request WP GPIO: -16!
[    1.979831] mvsdio: probe of f1090000.mvsdio failed with error -16

One can argue whether the current behaviour is too strict (and we did
that in the past, leading to the current code), but issuing only a
debug message when allocating an explicitly requested resource fails
is too tolerant, IMHO.


- Simon

Re: [PATCH 3/4] mmc: host: switch OF parser to use gpio descriptors

[Linus Walleij]

On Mon, Aug 18, 2014 at 2:46 PM, Simon Baatz <gmbnomis@gmail.com> wrote:
> On Tue, Aug 12, 2014 at 07:25:54PM +0200, Linus Walleij 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.
>>
>> As it is in no way a bug not to supply CD/WP GPIOs the messages
>> about them not being specified have been depromoted from
>> dev_err() to dev_dbg().
>
> I think the intention of the current code is to issue an
> error if a valid GPIO was specified but could not be obtained.

OK I'll post a fixup.

Yours,
Linus Walleij

[PATCH 4/4] mmc: mmci: augment driver to handle gpio descriptors

[Linus Walleij]

Currently the MMCI driver will only handle GPIO descriptors
implicitly through the device tree probe glue in mmc_of_init(),
but devices instatiated other ways such as through board files
and passing descriptors using the GPIO descriptor table will
not be able to exploit descriptors.

Augment the driver to look for a GPIO descriptor if device
tree is not used for the device, and if that doesn't work,
fall back to platform data GPIO assignment using the old
API. The end goal is to get rid of the platform data integer
GPIO assingments from the kernel.

This enable the MMCI-embedding platforms to be converted to
GPIO descritor tables.

Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 53 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 7ad463e9741c..24a83af6d153 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1552,16 +1552,49 @@ static int mmci_probe(struct amba_device *dev,
 	writel(0, host->base + MMCIMASK1);
 	writel(0xfff, host->base + MMCICLEAR);
 
-	/* If DT, cd/wp gpios must be supplied through it. */
-	if (!np && gpio_is_valid(plat->gpio_cd)) {
-		ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
-		if (ret)
-			goto clk_disable;
-	}
-	if (!np && gpio_is_valid(plat->gpio_wp)) {
-		ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
-		if (ret)
-			goto clk_disable;
+	/*
+	 * If:
+	 * - not using DT but using a descriptor table, or
+	 * - using a table of descriptors ALONGSIDE DT, or
+	 * look up these descriptors named "cd" and "wp" right here, fail
+	 * silently of these do not exist and proceed to try platform data
+	 */
+	if (!np) {
+		ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);
+		if (!ret)
+			dev_info(mmc_dev(mmc), "got CD GPIO (table)\n");
+		else {
+			if (ret == -EPROBE_DEFER)
+				goto clk_disable;
+			else if (gpio_is_valid(plat->gpio_cd)) {
+				ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
+				if (ret)
+					goto clk_disable;
+				else
+					dev_info(mmc_dev(mmc), "got CD GPIO (pdata)\n");
+			} else {
+				dev_dbg(mmc_dev(mmc),
+					"no CD GPIO in DT, table or pdata\n");
+			}
+		}
+
+		ret = mmc_gpiod_request_ro(mmc, "wp", 0, false, 0);
+		if (!ret)
+			dev_info(mmc_dev(mmc), "got WP GPIO (table)\n");
+		else {
+			if (ret == -EPROBE_DEFER)
+				goto clk_disable;
+			else if (gpio_is_valid(plat->gpio_wp)) {
+				ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
+				if (ret)
+					goto clk_disable;
+				else
+					dev_info(mmc_dev(mmc), "got WP GPIO (pdata)\n");
+			} else {
+				dev_dbg(mmc_dev(mmc),
+					"no WP GPIO in DT, table or pdata\n");
+			}
+		}
 	}
 
 	ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
-- 
1.9.3

Re: [PATCH 4/4] mmc: mmci: augment driver to handle gpio descriptors

[Alexandre Courbot]

On Tue, Aug 12, 2014 at 10:25 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> -       /* If DT, cd/wp gpios must be supplied through it. */
> -       if (!np && gpio_is_valid(plat->gpio_cd)) {
> -               ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
> -               if (ret)
> -                       goto clk_disable;
> -       }
> -       if (!np && gpio_is_valid(plat->gpio_wp)) {
> -               ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
> -               if (ret)
> -                       goto clk_disable;
> +       /*
> +        * If:
> +        * - not using DT but using a descriptor table, or
> +        * - using a table of descriptors ALONGSIDE DT, or
> +        * look up these descriptors named "cd" and "wp" right here, fail
> +        * silently of these do not exist and proceed to try platform data
> +        */
> +       if (!np) {
> +               ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);

I am not familiar with this driver, but since mmc_gpiod_request_cd()
uses gpiod_get(), can't you call it outside of the (!np) condition?
You should not have to do this kind of check when using the gpiod API.

> +               if (!ret)
> +                       dev_info(mmc_dev(mmc), "got CD GPIO (table)\n");
> +               else {

I think you want to add brackets around the dev_info() line to make
checkpatch.pl happy.

> +                       if (ret == -EPROBE_DEFER)
> +                               goto clk_disable;
> +                       else if (gpio_is_valid(plat->gpio_cd)) {
> +                               ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
> +                               if (ret)
> +                                       goto clk_disable;
> +                               else
> +                                       dev_info(mmc_dev(mmc), "got CD GPIO (pdata)\n");
> +                       } else {
> +                               dev_dbg(mmc_dev(mmc),
> +                                       "no CD GPIO in DT, table or pdata\n");
> +                       }
> +               }
> +
> +               ret = mmc_gpiod_request_ro(mmc, "wp", 0, false, 0);
> +               if (!ret)
> +                       dev_info(mmc_dev(mmc), "got WP GPIO (table)\n");
> +               else {

Same remark as above.

> +                       if (ret == -EPROBE_DEFER)
> +                               goto clk_disable;
> +                       else if (gpio_is_valid(plat->gpio_wp)) {
> +                               ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
> +                               if (ret)
> +                                       goto clk_disable;
> +                               else
> +                                       dev_info(mmc_dev(mmc), "got WP GPIO (pdata)\n");
> +                       } else {
> +                               dev_dbg(mmc_dev(mmc),
> +                                       "no WP GPIO in DT, table or pdata\n");
> +                       }
> +               }

I wonder (again, without knowing better) if this gpiod/gpio fallback
logic should not be put directly into mmc_gpio_request_ro/cd instead
of having two functions? Couldn't other drivers also benefit from it
that way?

Re: [PATCH 4/4] mmc: mmci: augment driver to handle gpio descriptors

[Linus Walleij]

On Thu, Aug 14, 2014 at 9:56 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Aug 12, 2014 at 10:25 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> -       /* If DT, cd/wp gpios must be supplied through it. */
>> -       if (!np && gpio_is_valid(plat->gpio_cd)) {
>> -               ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
>> -               if (ret)
>> -                       goto clk_disable;
>> -       }
>> -       if (!np && gpio_is_valid(plat->gpio_wp)) {
>> -               ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
>> -               if (ret)
>> -                       goto clk_disable;
>> +       /*
>> +        * If:
>> +        * - not using DT but using a descriptor table, or
>> +        * - using a table of descriptors ALONGSIDE DT, or
>> +        * look up these descriptors named "cd" and "wp" right here, fail
>> +        * silently of these do not exist and proceed to try platform data
>> +        */
>> +       if (!np) {
>> +               ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);
>
> I am not familiar with this driver, but since mmc_gpiod_request_cd()
> uses gpiod_get(), can't you call it outside of the (!np) condition?
> You should not have to do this kind of check when using the gpiod API.
(...)
> I wonder (again, without knowing better) if this gpiod/gpio fallback
> logic should not be put directly into mmc_gpio_request_ro/cd instead
> of having two functions? Couldn't other drivers also benefit from it
> that way?

So this is a side-effect of the fact that in the MMC subsystem, the
gpio descriptor is only used in the device tree parsing code.
mmc_of_parse() in drivers/mmc/core/host.c

So if you're *not* using device tree (as some of the MMCI platforms)
they need to make these calls elsewhere.

I agree that there may be a refactoring lurking here, that would
move this out of the DT parse code and into the MMC core.
It should then be called *before* parsing the DT as DT adds
additional options to the GPIO (explicit inversion).

It can be done on top of this patch set though, once these things
are in place.

Yours,
Linus Walleij

Re: [PATCH 1/4] mmc: slot-gpio: switch to use flags when getting GPIO

[Alexandre Courbot]

On Tue, Aug 12, 2014 at 10:25 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> When the slot GPIO driver gets the GPIO to be used for card
> detect, it is now possible to specify a flag to have the line
> set up as input. Get rid of the explicit setup call for input
> and use the flag.
>
> The extra argument works as there are transition varargs
> macros in place in the <linux/gpio/consumer.h> header, in
> the future we will make the flags argument compulsory.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks - I actually have a bunch of patches that update some other
drivers, will try to send them shortly...

Re: [PATCH 1/4] mmc: slot-gpio: switch to use flags when getting GPIO

[Ulf Hansson]

On 12 August 2014 19:25, Linus Walleij <linus.walleij@linaro.org> wrote:
> When the slot GPIO driver gets the GPIO to be used for card
> detect, it is now possible to specify a flag to have the line
> set up as input. Get rid of the explicit setup call for input
> and use the flag.
>
> The extra argument works as there are transition varargs
> macros in place in the <linux/gpio/consumer.h> header, in
> the future we will make the flags argument compulsory.
>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks! Applied for next.

Kind regards
Uffe

> ---
>  drivers/mmc/core/slot-gpio.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 5f89cb83d5f0..908c2b29e79f 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -308,14 +308,10 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
>         if (!con_id)
>                 con_id = ctx->cd_label;
>
> -       desc = devm_gpiod_get_index(host->parent, con_id, idx);
> +       desc = devm_gpiod_get_index(host->parent, con_id, idx, GPIOD_IN);
>         if (IS_ERR(desc))
>                 return PTR_ERR(desc);
>
> -       ret = gpiod_direction_input(desc);
> -       if (ret < 0)
> -               return ret;
> -
>         if (debounce) {
>                 ret = gpiod_set_debounce(desc, debounce);
>                 if (ret < 0)
> --
> 1.9.3
>

[PATCH 1/4] mmc: slot-gpio: switch to use flags when getting GPIO

[Linus Walleij]

When the slot GPIO driver gets the GPIO to be used for card
detect, it is now possible to specify a flag to have the line
set up as input. Get rid of the explicit setup call for input
and use the flag.

The extra argument works as there are transition varargs
macros in place in the <linux/gpio/consumer.h> header, in
the future we will make the flags argument compulsory.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/core/slot-gpio.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 5f89cb83d5f0..908c2b29e79f 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -308,14 +308,10 @@ int mmc_gpiod_request_cd(struct mmc_host *host, const char *con_id,
 	if (!con_id)
 		con_id = ctx->cd_label;
 
-	desc = devm_gpiod_get_index(host->parent, con_id, idx);
+	desc = devm_gpiod_get_index(host->parent, con_id, idx, GPIOD_IN);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
-	ret = gpiod_direction_input(desc);
-	if (ret < 0)
-		return ret;
-
 	if (debounce) {
 		ret = gpiod_set_debounce(desc, debounce);
 		if (ret < 0)
-- 
1.9.3

[PATCH 4/4] mmc: mmci: augment driver to handle gpio descriptors

[Linus Walleij]

Currently the MMCI driver will only handle GPIO descriptors
implicitly through the device tree probe glue in mmc_of_init(),
but devices instatiated other ways such as through board files
and passing descriptors using the GPIO descriptor table will
not be able to exploit descriptors.

Augment the driver to look for a GPIO descriptor if device
tree is not used for the device, and if that doesn't work,
fall back to platform data GPIO assignment using the old
API. The end goal is to get rid of the platform data integer
GPIO assingments from the kernel.

This enable the MMCI-embedding platforms to be converted to
GPIO descritor tables.

Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mmc/host/mmci.c | 53 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index e4d470704150..37a6542f056c 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1658,16 +1658,49 @@ static int mmci_probe(struct amba_device *dev,
 	writel(0, host->base + MMCIMASK1);
 	writel(0xfff, host->base + MMCICLEAR);
 
-	/* If DT, cd/wp gpios must be supplied through it. */
-	if (!np && gpio_is_valid(plat->gpio_cd)) {
-		ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
-		if (ret)
-			goto clk_disable;
-	}
-	if (!np && gpio_is_valid(plat->gpio_wp)) {
-		ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
-		if (ret)
-			goto clk_disable;
+	/*
+	 * If:
+	 * - not using DT but using a descriptor table, or
+	 * - using a table of descriptors ALONGSIDE DT, or
+	 * look up these descriptors named "cd" and "wp" right here, fail
+	 * silently of these do not exist and proceed to try platform data
+	 */
+	if (!np) {
+		ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);
+		if (!ret)
+			dev_info(mmc_dev(mmc), "got CD GPIO (table)\n");
+		else {
+			if (ret == -EPROBE_DEFER)
+				goto clk_disable;
+			else if (gpio_is_valid(plat->gpio_cd)) {
+				ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
+				if (ret)
+					goto clk_disable;
+				else
+					dev_info(mmc_dev(mmc), "got CD GPIO (pdata)\n");
+			} else {
+				dev_dbg(mmc_dev(mmc),
+					"no CD GPIO in DT, table or pdata\n");
+			}
+		}
+
+		ret = mmc_gpiod_request_ro(mmc, "wp", 0, false, 0);
+		if (!ret)
+			dev_info(mmc_dev(mmc), "got WP GPIO (table)\n");
+		else {
+			if (ret == -EPROBE_DEFER)
+				goto clk_disable;
+			else if (gpio_is_valid(plat->gpio_wp)) {
+				ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
+				if (ret)
+					goto clk_disable;
+				else
+					dev_info(mmc_dev(mmc), "got WP GPIO (pdata)\n");
+			} else {
+				dev_dbg(mmc_dev(mmc),
+					"no WP GPIO in DT, table or pdata\n");
+			}
+		}
 	}
 
 	ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
-- 
1.9.3

Re: [PATCH 4/4] mmc: mmci: augment driver to handle gpio descriptors

[Ulf Hansson]

On 27 August 2014 13:00, Linus Walleij <linus.walleij@linaro.org> wrote:
> Currently the MMCI driver will only handle GPIO descriptors
> implicitly through the device tree probe glue in mmc_of_init(),
> but devices instatiated other ways such as through board files
> and passing descriptors using the GPIO descriptor table will
> not be able to exploit descriptors.
>
> Augment the driver to look for a GPIO descriptor if device
> tree is not used for the device, and if that doesn't work,
> fall back to platform data GPIO assignment using the old
> API. The end goal is to get rid of the platform data integer
> GPIO assingments from the kernel.
>
> This enable the MMCI-embedding platforms to be converted to
> GPIO descritor tables.
>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 53 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index e4d470704150..37a6542f056c 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1658,16 +1658,49 @@ static int mmci_probe(struct amba_device *dev,
>         writel(0, host->base + MMCIMASK1);
>         writel(0xfff, host->base + MMCICLEAR);
>
> -       /* If DT, cd/wp gpios must be supplied through it. */
> -       if (!np && gpio_is_valid(plat->gpio_cd)) {
> -               ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
> -               if (ret)
> -                       goto clk_disable;
> -       }
> -       if (!np && gpio_is_valid(plat->gpio_wp)) {
> -               ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
> -               if (ret)
> -                       goto clk_disable;
> +       /*
> +        * If:
> +        * - not using DT but using a descriptor table, or
> +        * - using a table of descriptors ALONGSIDE DT, or
> +        * look up these descriptors named "cd" and "wp" right here, fail
> +        * silently of these do not exist and proceed to try platform data
> +        */
> +       if (!np) {
> +               ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0);
> +               if (!ret)
> +                       dev_info(mmc_dev(mmc), "got CD GPIO (table)\n");
> +               else {
> +                       if (ret == -EPROBE_DEFER)
> +                               goto clk_disable;
> +                       else if (gpio_is_valid(plat->gpio_cd)) {
> +                               ret = mmc_gpio_request_cd(mmc, plat->gpio_cd, 0);
> +                               if (ret)
> +                                       goto clk_disable;
> +                               else
> +                                       dev_info(mmc_dev(mmc), "got CD GPIO (pdata)\n");
> +                       } else {
> +                               dev_dbg(mmc_dev(mmc),
> +                                       "no CD GPIO in DT, table or pdata\n");
> +                       }
> +               }
> +
> +               ret = mmc_gpiod_request_ro(mmc, "wp", 0, false, 0);
> +               if (!ret)
> +                       dev_info(mmc_dev(mmc), "got WP GPIO (table)\n");
> +               else {
> +                       if (ret == -EPROBE_DEFER)
> +                               goto clk_disable;
> +                       else if (gpio_is_valid(plat->gpio_wp)) {
> +                               ret = mmc_gpio_request_ro(mmc, plat->gpio_wp);
> +                               if (ret)
> +                                       goto clk_disable;
> +                               else
> +                                       dev_info(mmc_dev(mmc), "got WP GPIO (pdata)\n");
> +                       } else {
> +                               dev_dbg(mmc_dev(mmc),
> +                                       "no WP GPIO in DT, table or pdata\n");
> +                       }

Just a nitpick; and unless it's important to you, could you remove all
the dev_info|dbg - we didn't need them before, why now?

Kind regards
Uffe

> +               }
>         }
>
>         ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
> --
> 1.9.3
>