mmc_of_parse() adaptations, DT support for Sheevaplugs

In addition to just logging errors encountered during DT parsing or allocating GPIO slots for CD/WP, mmc_of_parse() now returns with an error.

Signed-off-by Simon Baatz <[email protected]>

---
 drivers/mmc/core/host.c  |   24 +++++++++++++++++++-----
 include/linux/mmc/host.h |    2 +-
 2 files changed, 20 insertions(+), 6 deletions(-)

drivers/mmc/core/host.c +19 -5

index 2a3593d..45122cc 100644
@@ -306,7 +306,7 @@ static inline void mmc_host_clk_sysfs_init(struct mmc_host *host)
  * parse the properties and set respective generic mmc-host flags and
  * parameters.
  */
-void mmc_of_parse(struct mmc_host *host)
+int mmc_of_parse(struct mmc_host *host)
 {
        struct device_node *np;
        u32 bus_width;

@@ -315,7 +315,7 @@ void mmc_of_parse(struct mmc_host *host)
        int len, ret, gpio;
 
        if (!host->parent || !host->parent->of_node)
-               return;
+               return 0;
 
        np = host->parent->of_node;
 

@@ -338,6 +338,7 @@ void mmc_of_parse(struct mmc_host *host)
        default:
                dev_err(host->parent,
                        "Invalid \"bus-width\" value %ud!\n", bus_width);
+               return -EINVAL;
        }
 
        /* f_max is obtained from the optional "max-frequency" property */

@@ -372,13 +373,15 @@ void mmc_of_parse(struct mmc_host *host)
                                gpio_inv_cd = true;
 
                        ret = mmc_gpio_request_cd(host, gpio);
-                       if (ret < 0)
+                       if (ret < 0) {
                                dev_err(host->parent,
                                        "Failed to request CD GPIO #%d: %d!\n",
                                        gpio, ret);
-                       else
+                               return ret;
+                       } else {
                                dev_info(host->parent, "Got CD GPIO #%d.\n",
                                         gpio);
+                       }
                }
 
                if (explicit_inv_cd ^ gpio_inv_cd)

@@ -394,9 +397,14 @@ void mmc_of_parse(struct mmc_host *host)
                        gpio_inv_wp = true;
 
                ret = mmc_gpio_request_ro(host, gpio);
-               if (ret < 0)
+               if (ret < 0) {
                        dev_err(host->parent,
                                "Failed to request WP GPIO: %d!\n", ret);
+                       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;

@@ -413,6 +421,12 @@ void mmc_of_parse(struct mmc_host *host)
                host->pm_caps |= MMC_PM_KEEP_POWER;
        if (of_find_property(np, "enable-sdio-wakeup", &len))
                host->pm_caps |= MMC_PM_WAKE_SDIO_IRQ;
+
+       return 0;
+
+out:
+       mmc_gpio_free_cd(host);
+       return ret;
 }
 
 EXPORT_SYMBOL(mmc_of_parse);

include/linux/mmc/host.h +1 -2

index e326ae2..c8c4fbc 100644
@@ -369,7 +369,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *);
 int mmc_add_host(struct mmc_host *);
 void mmc_remove_host(struct mmc_host *);
 void mmc_free_host(struct mmc_host *);
-void mmc_of_parse(struct mmc_host *host);
+int mmc_of_parse(struct mmc_host *host);
 
 static inline void *mmc_priv(struct mmc_host *host)
 {
--

1.7.9.5

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

Signed-off-by Simon Baatz <[email protected]>

---
 drivers/mmc/host/sdhci-pxav3.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/mmc/host/sdhci-pxav3.c +5 -2

index 1ae358e..67ea388 100644
@@ -252,7 +252,9 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 
 	match = of_match_device(of_match_ptr(sdhci_pxav3_of_match), &pdev->dev);
 	if (match) {
-		mmc_of_parse(host->mmc);
+		ret = mmc_of_parse(host->mmc);
+		if (ret)
+			goto err_of_parse;
 		sdhci_get_of_property(pdev);
 		pdata = pxav3_get_mmc_pdata(dev);
 	} else if (pdata) {

@@ -313,10 +315,11 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_of_parse:
+err_cd_req:
 err_add_host:
 	clk_disable_unprepare(clk);
 	clk_put(clk);
-err_cd_req:
 err_clk_get:
 	sdhci_pltfm_free(pdev);
 	kfree(pxa);

Signature · 1 line

1.7.9.5

In order to prepare the switch to the standard MMC device tree parser for mvsdio, adapt all current uses of mvsdio in the dts files to the standard format.

Signed-off-by Simon Baatz <[email protected]>

---

Quoted 6 lines · Simon Baatz

In order to prepare the switch to the standard MMC device tree parser for mvsdio, adapt all current uses of mvsdio in the dts files to the standard format.

Signed-off-by Simon Baatz <[email protected]>

---

...

arch/arm/boot/dts/kirkwood-mplcec4.dts line 139

Quoted 10 lines

arch/arm/boot/dts/kirkwood-mplcec4.dts +1 -1

index 7588241..bf3a58c 100644
@@ -136,7 +136,7 @@
 			pinctrl-0 = <&pmx_sdio &pmx_sdio_cd>;
 			pinctrl-names = "default";
 			status = "okay";
-			cd-gpios = <&gpio1 15 0>;
+			cd-gpios = <&gpio1 15 1>;

Is this a bugfix that should be backported?

In order to prepare the switch to the standard MMC device tree parser for mvsdio, adapt all current uses of mvsdio in the dts files to the standard format.

Signed-off-by Simon Baatz <[email protected]>

---
 arch/arm/boot/dts/armada-370-db.dts                |    1 +
 arch/arm/boot/dts/armada-370-mirabox.dts           |    1 +
 arch/arm/boot/dts/armada-370-rd.dts                |    1 +
 arch/arm/boot/dts/armada-370-xp.dtsi               |    4 ++++
 arch/arm/boot/dts/armada-xp-db.dts                 |    1 +
 arch/arm/boot/dts/kirkwood-dreamplug.dts           |    1 +
 .../arm/boot/dts/kirkwood-guruplug-server-plus.dts |    2 ++
 arch/arm/boot/dts/kirkwood-mplcec4.dts             |    2 +-
 arch/arm/boot/dts/kirkwood-topkick.dts             |    1 +
 arch/arm/boot/dts/kirkwood.dtsi                    |    4 ++++
 10 files changed, 17 insertions(+), 1 deletion(-)

Signed-off-by Simon Baatz <[email protected]>

---
 arch/arm/boot/dts/Makefile                        |    2 +
 arch/arm/boot/dts/kirkwood-sheevaplug-common.dtsi |   97 +++++++++++++++++++++
 arch/arm/boot/dts/kirkwood-sheevaplug-esata.dts   |   40 +++++++++
 arch/arm/boot/dts/kirkwood-sheevaplug.dts         |   45 ++++++++++
 4 files changed, 184 insertions(+)
 create mode 100644 arch/arm/boot/dts/kirkwood-sheevaplug-common.dtsi
 create mode 100644 arch/arm/boot/dts/kirkwood-sheevaplug-esata.dts
 create mode 100644 arch/arm/boot/dts/kirkwood-sheevaplug.dts

Signed-off-by Simon Baatz <[email protected]>

---
 arch/arm/mach-kirkwood/Kconfig            |    7 +++++++
 arch/arm/mach-kirkwood/Makefile           |    1 +
 arch/arm/mach-kirkwood/board-dt.c         |    4 ++++
 arch/arm/mach-kirkwood/board-sheevaplug.c |   27 +++++++++++++++++++++++++++
 arch/arm/mach-kirkwood/common.h           |    5 +++++
 5 files changed, 44 insertions(+)
 create mode 100644 arch/arm/mach-kirkwood/board-sheevaplug.c

While adding DT support for the Sheevaplugs by Globalscale Technologies (Kirkwood), it turned out that the DT binding of mvsdio lacked features to properly support the hardware (active high/low of CD and WP pins could not be described in DT).

This is standard functionality provided by the mmc_of_parse() helper function. However, mmc_of_parse() may allocate GPIO lines. If the allocation fails, it outputs an error, but does not return an error to its caller. Therefore, a proposal to handle errors in mmc_of_parse() is made.

The patch set is structured as follows:

1 Adapt mmc_of_parse() to return errors
2-6 Handle errors in current drivers using mmc_of_parse() (compile tested
only)
7-8 Convert mvsdio and respective dts files to mmc_of_parse() (tested on
kirkwood)
9 Add dts files for (eSATA) Sheevaplug
10 Add DT support for (eSATA) Sheevaplug

I could only test on an eSATA Sheevaplug. I found patches with different LEDs for the Sheevaplug. Thus, I would highly appreciate if someone with the hardware could give this a spin on a non-eSATA version. Some additional testing of the change detect and write protect behaviour for mvsdio can't hurt either. I hope that there aren't board revisions with different CD/WP pins out there.

Simon Baatz (10):
mmc: return mmc_of_parse() errors to caller
mmc: sh_mmcif: handle mmc_of_parse() errors during probe
mmc: tmio-mmc: handle mmc_of_parse() errors during probe
mmc: mxcmmc: handle mmc_of_parse() errors during probe
mmc: sdhi-pxav3: handle mmc_of_parse() errors during probe
mmc: tegra: handle mmc_of_parse() errors during probe
ARM: mvebu: Use standard MMC binding for all users of mvsdio
mmc: mvsdio: use standard MMC device-tree binding parser

    mmc_of_parse()

ARM: Kirkwood: Add dts files for Sheevaplug and eSATA Sheevaplug
ARM: Kirkwood: add DT support for Sheevaplug and Sheevaplug eSATA

Quoted 9 lines · Simon Baatz

While adding DT support for the Sheevaplugs by Globalscale Technologies (Kirkwood), it turned out that the DT binding of mvsdio lacked features to properly support the hardware (active high/low of CD and WP pins could not be described in DT).

This is standard functionality provided by the mmc_of_parse() helper function. However, mmc_of_parse() may allocate GPIO lines. If the allocation fails, it outputs an error, but does not return an error to its caller. Therefore, a proposal to handle errors in mmc_of_parse() is made.

Thanks for the patches. In principle I'm fine either way. It is a policy decision IMHO. E.g. consider a situation. You have a DT with an SD-card slot, where card-detection is performed by a GPIO. OTOH the same pin is used on some other (optional) interface on the same board. If that other competing interface is unused, the driver isn't loaded, you can use the GPIO for card-detection. However, if that other interface is used, your attempt to get the card-detect pin will fail, but you still can use the interface in polling mode. No, I don't think this is a good example of hardware design :) User experience would depend on driver probing order, but in principle it is imaginable. So, with the current mmc_of_parse() you're more tolerant. You get a warning in the log, but the interface might still be usable. And if you're surprised why your write protection status hasn't been properly detected - just look in the log.

You're proposing a change in behaviour. After your change any failure to obtain a resource in mmc_of_parse() will fail interface probing. Personally I don't think there are any users around, who would notice this change, still, we have to be aware of it. And I don't know whether we should do that. You know, we don't break user-space ;-)

So, technically I'm ok with the changes, but policy-wise I'm not sure how severe this change would be. We could go a bit slower and first add a return code as in your patch #1, but not fail drivers, like in your patches #2, 3,... but WARN_ON(mmc_of_parse() < 0); and continue for now. In 3.12 we could then replace those warnings with real failures. But maybe I'm just overcautious and we can just go ahead with your patches. You can add my

Acked-by Guennadi Liakhovetski <[email protected]>

for patches 1-3, and let's see what Chris thinks about this change.

Quoted 12 lines · Guennadi Liakhovetski

Quoted 9 lines · Simon Baatz

While adding DT support for the Sheevaplugs by Globalscale Technologies (Kirkwood), it turned out that the DT binding of mvsdio lacked features to properly support the hardware (active high/low of CD and WP pins could not be described in DT).

This is standard functionality provided by the mmc_of_parse() helper function. However, mmc_of_parse() may allocate GPIO lines. If the allocation fails, it outputs an error, but does not return an error to its caller. Therefore, a proposal to handle errors in mmc_of_parse() is made.

Quoted 13 lines

Thanks for the patches. In principle I'm fine either way. It is a policy decision IMHO. E.g. consider a situation. You have a DT with an SD-card slot, where card-detection is performed by a GPIO. OTOH the same pin is used on some other (optional) interface on the same board. If that other competing interface is unused, the driver isn't loaded, you can use the GPIO for card-detection. However, if that other interface is used, your attempt to get the card-detect pin will fail, but you still can use the interface in polling mode. No, I don't think this is a good example of hardware design :) User experience would depend on driver probing order, but in principle it is imaginable. So, with the current mmc_of_parse() you're more tolerant. You get a warning in the log, but the interface might still be usable. And if you're surprised why your write protection status hasn't been properly detected - just look in the log.

Yes, there is value in both ways. As should be clear by now, I prefer being more strict here :-). But in the end, it is a policy decision as you say.