Skip to contentSkip to conversation

mmc: usdhi6rol0: UHS support

PATCH Superseded Lars Persson 3 participants 9 messages Apr 18, 2016 – Apr 20, 2016 2 messages not in the archive

Subsystems (3)

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Lars Persson

author maintainer
#0 lore raw

v2 cover mmc: usdhi6rol0: UHS support

This patch set adds UHS support to the usdhi6rol0 driver, with control of the regulator and pin state settings.

Changes since v1:
- Use mmc_regulator_set_vqmmc().

Lars Persson (3):
mmc: dt: usdhi6rol0: add optional pinctrl binding
mmc: usdhi6rol0: add support for UHS modes
mmc: usdhi6rol0: add pinctrl to set pin drive strength

 .../devicetree/bindings/mmc/usdhi6rol0.txt         |  4 ++
 drivers/mmc/host/usdhi6rol0.c                      | 51 ++++++++++++++++++++++
 2 files changed, 55 insertions(+)
Signature · 1 line
2.1.4

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

PATCH v2 1/3 mmc: dt: usdhi6rol0: add optional pinctrl binding

Add a pinctrl binding to specify different pin settings for high speed modes and UHS modes.

---
 Documentation/devicetree/bindings/mmc/usdhi6rol0.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/mmc/usdhi6rol0.txt +4 -0
index 8babdaa..9dd9478 100644
@@ -12,6 +12,10 @@ Optional properties:
 
 - vmmc-supply:	a phandle of a regulator, supplying Vcc to the card
 - vqmmc-supply:	a phandle of a regulator, supplying VccQ to the card
+- pinctrl-names: Must contain a "default" entry and optionally a "pins_uhs"
+                 entry for the UHS pin configuration.
+- pinctrl-N: One property for each name listed in pinctrl-names, see
+             ../pinctrl/pinctrl-bindings.txt.
 
 Additionally any standard mmc bindings from mmc.txt can be used.
 
Signature · 6 lines
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#3 lore raw

Re: [PATCH v2 1/3] mmc: dt: usdhi6rol0: add optional pinctrl binding

Quoted 13 lines · Lars Persson

Add a pinctrl binding to specify different pin settings for high speed modes and UHS modes.

---
 Documentation/devicetree/bindings/mmc/usdhi6rol0.txt | 4 ++++
 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/mmc/usdhi6rol0.txt +0 -0
index 8babdaa..9dd9478 100644

Documentation/devicetree/bindings/mmc/usdhi6rol0.txt Optional properties: · line 15

Quoted 4 lines
Documentation/devicetree/bindings/mmc/usdhi6rol0.txt +1 -0
@@ -12,6 +12,10 @@ Optional properties:
 
 - vmmc-supply: a phandle of a regulator, supplying Vcc to the card
 - vqmmc-supply:        a phandle of a regulator, supplying VccQ to the card
+- pinctrl-names: Must contain a "default" entry and optionally a "pins_uhs"

"state_uhs" is becoming more and more widely deployed, I suggest you use that as well.

Moreover, I am reviewing patch 3/3 and I think some more clarification is needed here, on whether what is required and optional. See those comment on patch 3/3.

Quoted 9 lines
Documentation/devicetree/bindings/mmc/usdhi6rol0.txt +3 -1
+                 entry for the UHS pin configuration.
+- pinctrl-N: One property for each name listed in pinctrl-names, see
+             ../pinctrl/pinctrl-bindings.txt.
 
 Additionally any standard mmc bindings from mmc.txt can be used.
 
--

2.1.4

Kind regards
Uffe

author maintainer
#4 lore raw

PATCH v2 2/3 mmc: usdhi6rol0: add support for UHS modes

In reply to Lars Persson

Add a start_signal_voltage_switch() operation to support enabling of UHS modes.

---
 drivers/mmc/host/usdhi6rol0.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
drivers/mmc/host/usdhi6rol0.c +13 -0
index 807c06e..2585ea4 100644
@@ -1147,12 +1147,25 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	}
 }
 
+static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	int ret;
+
+	ret = mmc_regulator_set_vqmmc(mmc, ios);
+
+	if (ret < 0)
+		dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+
+	return ret;
+}
+
 static struct mmc_host_ops usdhi6_ops = {
 	.request	= usdhi6_request,
 	.set_ios	= usdhi6_set_ios,
 	.get_cd		= usdhi6_get_cd,
 	.get_ro		= usdhi6_get_ro,
 	.enable_sdio_irq = usdhi6_enable_sdio_irq,
+	.start_signal_voltage_switch = usdhi6_sig_volt_switch,
 };
 
 /*			State machine handlers				*/
Signature · 1 line
2.1.4
author maintainer
#5 lore raw

PATCH v2 3/3 mmc: usdhi6rol0: add pinctrl to set pin drive strength

In reply to Lars Persson

Some boards need different pin drive strength for the UHS mode. Add an optional pinctrl setting with two pin states covering UHS speeds and other speeds.

---
 drivers/mmc/host/usdhi6rol0.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
drivers/mmc/host/usdhi6rol0.c +39 -1
index 2585ea4..e8a5c8f 100644
@@ -22,6 +22,7 @@
 #include <linux/mmc/sdio.h>
 #include <linux/module.h>
 #include <linux/pagemap.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/scatterlist.h>
 #include <linux/string.h>
@@ -198,6 +199,11 @@ struct usdhi6_host {
 	struct dma_chan *chan_rx;
 	struct dma_chan *chan_tx;
 	bool dma_active;
+
+	/* Pin control */
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_uhs;
 };
 
 /*			I/O primitives					*/
@@ -1147,14 +1153,39 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
 	}
 }
 
+static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage)
+{
+	if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) ||
+	    IS_ERR(host->pins_uhs))
+		return 0;
+
+	switch (voltage) {
+	case MMC_SIGNAL_VOLTAGE_180:
+	case MMC_SIGNAL_VOLTAGE_120:
+		return pinctrl_select_state(host->pinctrl,
+					    host->pins_uhs);
+
+	default:
+		return pinctrl_select_state(host->pinctrl,
+					    host->pins_default);
+	}
+}
+
 static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	int ret;
 
 	ret = mmc_regulator_set_vqmmc(mmc, ios);
 
-	if (ret < 0)
+	if (ret < 0) {
 		dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage);
+	if (ret)
+		dev_warn_once(mmc_dev(mmc),
+			      "Failed to set pinstate err=%d\n", ret);
 
 	return ret;
 }
@@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev)
 		mmc->f_max = host->imclk;
 	mmc->f_min = host->imclk / 512;
 
+	host->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!IS_ERR(host->pinctrl)) {
+		host->pins_default = pinctrl_lookup_state(host->pinctrl,
+							  PINCTRL_STATE_DEFAULT);
+		host->pins_uhs     = pinctrl_lookup_state(host->pinctrl,
+							  "pins_uhs");
+	}
 	platform_set_drvdata(pdev, host);
 
 	ret = mmc_add_host(mmc);
Signature · 1 line
2.1.4
#6 lore raw

Re: [PATCH v2 3/3] mmc: usdhi6rol0: add pinctrl to set pin drive strength

Quoted 34 lines · Lars Persson

Some boards need different pin drive strength for the UHS mode. Add an optional pinctrl setting with two pin states covering UHS speeds and other speeds.

---
 drivers/mmc/host/usdhi6rol0.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
drivers/mmc/host/usdhi6rol0.c +6 -0
index 2585ea4..e8a5c8f 100644
@@ -22,6 +22,7 @@
 #include <linux/mmc/sdio.h>
 #include <linux/module.h>
 #include <linux/pagemap.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/scatterlist.h>
 #include <linux/string.h>
@@ -198,6 +199,11 @@ struct usdhi6_host {
        struct dma_chan *chan_rx;
        struct dma_chan *chan_tx;
        bool dma_active;
+
+       /* Pin control */
+       struct pinctrl *pinctrl;
+       struct pinctrl_state *pins_default;
+       struct pinctrl_state *pins_uhs;
 };
 
 /*                     I/O primitives                                  */

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1159

Quoted 7 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
@@ -1147,14 +1153,39 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
        }
 }
 
+static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage)
+{
+       if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) ||
+           IS_ERR(host->pins_uhs))

So this means, if you have UHS support you need both default and uhs pins. That makes sense but it's not according to the DT documentation in patch 1/3.

Quoted 9 lines
drivers/mmc/host/usdhi6rol0.c +9 -0
+               return 0;
+
+       switch (voltage) {
+       case MMC_SIGNAL_VOLTAGE_180:
+       case MMC_SIGNAL_VOLTAGE_120:
+               return pinctrl_select_state(host->pinctrl,
+                                           host->pins_uhs);
+
+       default:

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1178

Quoted 10 lines
drivers/mmc/host/usdhi6rol0.c +5 -0
+               return pinctrl_select_state(host->pinctrl,
+                                           host->pins_default);
+       }
+}
+
 static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 {
        int ret;
 
        ret = mmc_regulator_set_vqmmc(mmc, ios);

What if you don't have pins for default and uhs state? Should you verify that's the case before deciding to switch voltage?

Quoted 15 lines
drivers/mmc/host/usdhi6rol0.c +8 -1
-       if (ret < 0)
+       if (ret < 0) {
                dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+               return ret;
+       }
+
+       ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage);
+       if (ret)
+               dev_warn_once(mmc_dev(mmc),
+                             "Failed to set pinstate err=%d\n", ret);
 
        return ret;
 }

drivers/mmc/host/usdhi6rol0.c static int usdhi6_probe(struct platform_device *pdev) · line 1854

Quoted 7 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
@@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev)
                mmc->f_max = host->imclk;
        mmc->f_min = host->imclk / 512;
 
+       host->pinctrl = devm_pinctrl_get(&pdev->dev);
+       if (!IS_ERR(host->pinctrl)) {
+               host->pins_default = pinctrl_lookup_state(host->pinctrl,
+                                                         PINCTRL_STATE_DEFAULT);

According to the DT documentation in patch 1/3, the default pins should be required and not optional. I assume you want them to be optional, but required when supporting UHS, right?

Quoted 9 lines
drivers/mmc/host/usdhi6rol0.c +3 -1
+               host->pins_uhs     = pinctrl_lookup_state(host->pinctrl,
+                                                         "pins_uhs");
+       }
        platform_set_drvdata(pdev, host);
 
        ret = mmc_add_host(mmc);
--

2.1.4

Kind regards
Uffe

author maintainer
#7 lore raw

Re: [PATCH v2 3/3] mmc: usdhi6rol0: add pinctrl to set pin drive strength

Quoted 41 lines
Quoted 38 lines · 19 apr. 2016 at 11:45 Ulf Hansson <[email protected]> wrote:

On 18 April 2016 at 15:44, Lars Persson <[email protected]> wrote:
Some boards need different pin drive strength for the UHS mode. Add an optional pinctrl setting with two pin states covering UHS speeds and other speeds.

---
drivers/mmc/host/usdhi6rol0.c | 40 +++++++++++++++++++++++++++++++++++++++-

1 file changed, 39 insertions(+), 1 deletion(-)
drivers/mmc/host/usdhi6rol0.c +6 -0
index 2585ea4..e8a5c8f 100644
@@ -22,6 +22,7 @@
 #include <linux/mmc/sdio.h>
 #include <linux/module.h>
 #include <linux/pagemap.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/scatterlist.h>
 #include <linux/string.h>
@@ -198,6 +199,11 @@ struct usdhi6_host {
       struct dma_chan *chan_rx;
       struct dma_chan *chan_tx;
       bool dma_active;
+
+       /* Pin control */
+       struct pinctrl *pinctrl;
+       struct pinctrl_state *pins_default;
+       struct pinctrl_state *pins_uhs;
 };
 
 /*                     I/O primitives                                  */
@@ -1147,14 +1153,39 @@ static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable)
       }
 }
 
Quoted 8 lines

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1159

Quoted 4 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
+static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage)
+{
+       if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) ||
+           IS_ERR(host->pins_uhs))

So this means, if you have UHS support you need both default and uhs pins. That makes sense but it's not according to the DT documentation in patch 1/3.

No. It means that if this particular board requires distinct settings for UHS then both pin states need to be defined. Otherwise we assume that the default settings are fine also for UHS.

Quoted 19 lines
Quoted 19 lines
drivers/mmc/host/usdhi6rol0.c +14 -0
+               return 0;
+
+       switch (voltage) {
+       case MMC_SIGNAL_VOLTAGE_180:
+       case MMC_SIGNAL_VOLTAGE_120:
+               return pinctrl_select_state(host->pinctrl,
+                                           host->pins_uhs);
+
+       default:
+               return pinctrl_select_state(host->pinctrl,
+                                           host->pins_default);
+       }
+}
+
 static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 {
       int ret;
 
Quoted 4 lines

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1178

Quoted 1 line
drivers/mmc/host/usdhi6rol0.c +0 -0
       ret = mmc_regulator_set_vqmmc(mmc, ios);

What if you don't have pins for default and uhs state? Should you verify that's the case before deciding to switch voltage

No see my reply above.

Quoted 19 lines
Quoted 19 lines
drivers/mmc/host/usdhi6rol0.c +8 -1
-       if (ret < 0)
+       if (ret < 0) {
               dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+               return ret;
+       }
+
+       ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage);
+       if (ret)
+               dev_warn_once(mmc_dev(mmc),
+                             "Failed to set pinstate err=%d\n", ret);
 
       return ret;
 }
@@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev)
               mmc->f_max = host->imclk;
       mmc->f_min = host->imclk / 512;
 
Quoted 8 lines

drivers/mmc/host/usdhi6rol0.c static int usdhi6_probe(struct platform_device *pdev) · line 1854

Quoted 4 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
+       host->pinctrl = devm_pinctrl_get(&pdev->dev);
+       if (!IS_ERR(host->pinctrl)) {
+               host->pins_default = pinctrl_lookup_state(host->pinctrl,
+                                                         PINCTRL_STATE_DEFAULT);

According to the DT documentation in patch 1/3, the default pins should be required and not optional. I assume you want them to be optional, but required when supporting UHS, right?

Yes the default is required only when a UHS pin state is defined. Hence all the pinctrl parameters are listed as optional, but the presence of a pinctrl-names parameter makes the default pins required.

Thanks.

- Lars

#8 lore raw

Re: [PATCH v2 3/3] mmc: usdhi6rol0: add pinctrl to set pin drive strength

[...]

Quoted 11 lines
Quoted 9 lines

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1159

Quoted 5 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
+static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage)
+{
+       if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) ||
+           IS_ERR(host->pins_uhs))

So this means, if you have UHS support you need both default and uhs pins. That makes sense but it's not according to the DT documentation in patch 1/3.

No. It means that if this particular board requires distinct settings for UHS then both pin states need to be defined. Otherwise we assume that the default settings are fine also for UHS.

I see.

I would rather see that you bail out in ->probe() when IS_ERR(host->pinctrl) instead of testing for that here.

Conforming to the policy you describe here, you should only need to test for IS_ERR(host->pins_uhs), as host->pins_default shall then be implicitly covered by that test.

Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
drivers/mmc/host/usdhi6rol0.c +14 -0
+               return 0;
+
+       switch (voltage) {
+       case MMC_SIGNAL_VOLTAGE_180:
+       case MMC_SIGNAL_VOLTAGE_120:
+               return pinctrl_select_state(host->pinctrl,
+                                           host->pins_uhs);
+
+       default:
+               return pinctrl_select_state(host->pinctrl,
+                                           host->pins_default);
+       }
+}
+

static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)

{
       int ret;
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
       ret = mmc_regulator_set_vqmmc(mmc, ios);

What if you don't have pins for default and uhs state? Should you verify that's the case before deciding to switch voltage

No see my reply above.

Okay!

Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
drivers/mmc/host/usdhi6rol0.c +8 -1
-       if (ret < 0)
+       if (ret < 0) {
               dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+               return ret;
+       }
+
+       ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage);
+       if (ret)
+               dev_warn_once(mmc_dev(mmc),
+                             "Failed to set pinstate err=%d\n", ret);
 
       return ret;

}

drivers/mmc/host/usdhi6rol0.c +0 -0
@@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev)
               mmc->f_max = host->imclk;
       mmc->f_min = host->imclk / 512;
 
Quoted 8 lines

drivers/mmc/host/usdhi6rol0.c static int usdhi6_probe(struct platform_device *pdev) · line 1854

Quoted 4 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
+       host->pinctrl = devm_pinctrl_get(&pdev->dev);
+       if (!IS_ERR(host->pinctrl)) {
+               host->pins_default = pinctrl_lookup_state(host->pinctrl,
+                                                         PINCTRL_STATE_DEFAULT);

According to the DT documentation in patch 1/3, the default pins should be required and not optional. I assume you want them to be optional, but required when supporting UHS, right?

Quoted 1 line

Yes the default is required only when a UHS pin state is defined. Hence all the pinctrl parameters are listed as optional, but the presence of a pinctrl-names parameter makes the default pins required.

I am not sure why you say default state is required when pinctrl-names is defined. It's rather a recommendation and it's being selected by the driver core if it exist.

Please update the patch for DT documentation to reflect the policy. Moreover you need to deploy this policy properly here and you also need to add error handling.

In more detail.
1. Check for errors from devm_pinctrl_get() and propagate them.
2. Check if "state_uhs" can be fetched and if so try also to fetch default state. If the latter fails you need propagate that as an error as well.

Does that make sense?

Kind regards
Uffe

A message that is not in the archive (sent off-list or not yet received): CAPDyKFoV7h73K+7HhkdS7BC1RUAo3dcum_+cRikY=_F2YBeNSA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#10 lore raw

Re: [PATCH v2 3/3] mmc: usdhi6rol0: add pinctrl to set pin drive strength

Quoted 14 lines · Ulf Hansson

[...]

Quoted 11 lines
Quoted 9 lines

drivers/mmc/host/usdhi6rol0.c static void usdhi6_enable_sdio_irq(struct mmc_host *mmc, int enable) · line 1159

Quoted 5 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
+static int usdhi6_set_pinstates(struct usdhi6_host *host, int voltage)
+{
+       if (IS_ERR(host->pinctrl) || IS_ERR(host->pins_default) ||
+           IS_ERR(host->pins_uhs))

So this means, if you have UHS support you need both default and uhs pins. That makes sense but it's not according to the DT documentation in patch 1/3.

No. It means that if this particular board requires distinct settings for UHS then both pin states need to be defined. Otherwise we assume that the default settings are fine also for UHS.

Quoted 8 lines

I see.

I would rather see that you bail out in ->probe() when IS_ERR(host->pinctrl) instead of testing for that here.

Conforming to the policy you describe here, you should only need to test for IS_ERR(host->pins_uhs), as host->pins_default shall then be implicitly covered by that test.

Ok.

Quoted 59 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
drivers/mmc/host/usdhi6rol0.c +14 -0
+               return 0;
+
+       switch (voltage) {
+       case MMC_SIGNAL_VOLTAGE_180:
+       case MMC_SIGNAL_VOLTAGE_120:
+               return pinctrl_select_state(host->pinctrl,
+                                           host->pins_uhs);
+
+       default:
+               return pinctrl_select_state(host->pinctrl,
+                                           host->pins_default);
+       }
+}
+

static int usdhi6_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)

{
        int ret;
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
        ret = mmc_regulator_set_vqmmc(mmc, ios);

What if you don't have pins for default and uhs state? Should you verify that's the case before deciding to switch voltage

No see my reply above.

Okay!

Quoted 29 lines
Quoted 28 lines
Quoted 17 lines
drivers/mmc/host/usdhi6rol0.c +8 -1
-       if (ret < 0)
+       if (ret < 0) {
                dev_warn(mmc_dev(mmc), "Voltage switch failed: %d\n", ret);
+               return ret;
+       }
+
+       ret = usdhi6_set_pinstates(mmc_priv(mmc), ios->signal_voltage);
+       if (ret)
+               dev_warn_once(mmc_dev(mmc),
+                             "Failed to set pinstate err=%d\n", ret);
 
        return ret;

}

drivers/mmc/host/usdhi6rol0.c static int usdhi6_probe(struct platform_device *pdev) · line 1854

Quoted 7 lines
drivers/mmc/host/usdhi6rol0.c +4 -0
@@ -1817,6 +1848,13 @@ static int usdhi6_probe(struct platform_device *pdev)
                mmc->f_max = host->imclk;
        mmc->f_min = host->imclk / 512;
 
+       host->pinctrl = devm_pinctrl_get(&pdev->dev);
+       if (!IS_ERR(host->pinctrl)) {
+               host->pins_default = pinctrl_lookup_state(host->pinctrl,
+                                                         PINCTRL_STATE_DEFAULT);

According to the DT documentation in patch 1/3, the default pins should be required and not optional. I assume you want them to be optional, but required when supporting UHS, right?

Quoted 5 lines
Quoted 1 line

Yes the default is required only when a UHS pin state is defined. Hence all the pinctrl parameters are listed as optional, but the presence of a pinctrl-names parameter makes the default pins required.

I am not sure why you say default state is required when pinctrl-names is defined. It's rather a recommendation and it's being selected by the driver core if it exist.

I considered the two use-cases for pinctrl in this driver:
- Only using default and letting the core select it.
- Using it for UHS in which case default and pins_uhs are required.

Both cases need the default, that is why it was listed as required. But I will change this based on your feedback.

Quoted 5 lines

Please update the patch for DT documentation to reflect the policy. Moreover you need to deploy this policy properly here and you also need to add error handling.

Quoted 7 lines

In more detail.
1. Check for errors from devm_pinctrl_get() and propagate them.
2. Check if "state_uhs" can be fetched and if so try also to fetch default state. If the latter fails you need propagate that as an error as well.

Does that make sense?

OK, I will implement this error checking.

- Lars

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