[PATCH 0/7] mmc: pxamci: cleanups and minor DT tweaks

[PATCH 0/7] mmc: pxamci: cleanups and minor DT tweaks

[Daniel Mack]

Here is a series of small patches for pxamci that do some cleanups.

The pxa tree has a patch for 4.19 [1] that this series builds upon,
so if possible, we'd like to let this series go through Robert's tree
after it has been reviewed. If that's not okay for the mmc folks,
teh series has to wait until 4.20-rc1.

[1] https://github.com/rjarzmik/linux/commit/6b3348f9e6e


Thanks,
Daniel

Daniel Mack (7):
  mmc: pxamci: remove irq from private context
  mmc: pxamci: remove dma resources from private context
  mmc: pxamci: remove dead code from pxamci_remove()
  mmc: pxamci: fix indenting
  mmc: pxamci: call mmc_of_parse()
  mmc: pxamci: remove pxa-mmc,gpio-power from devicetree bindings
  mmc: pxamci: let mmc core handle regulators

 .../devicetree/bindings/mmc/pxa-mmc.txt       |   1 -
 drivers/mmc/host/pxamci.c                     | 111 ++++++++----------
 2 files changed, 47 insertions(+), 65 deletions(-)

-- 
2.17.1

[PATCH 1/7] mmc: pxamci: remove irq from private context

[Daniel Mack]

This seems to be a left-over from times before the IRQ was handled by devm
functions. Remove it.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/mmc/host/pxamci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 6c94474e36f4..f0968882dbd3 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -58,7 +58,6 @@ struct pxamci_host {
 	void __iomem		*base;
 	struct clk		*clk;
 	unsigned long		clkrate;
-	int			irq;
 	unsigned int		clkrt;
 	unsigned int		cmdat;
 	unsigned int		imask;
@@ -711,7 +710,6 @@ static int pxamci_probe(struct platform_device *pdev)
 
 	spin_lock_init(&host->lock);
 	host->res = r;
-	host->irq = irq;
 	host->imask = MMC_I_MASK_ALL;
 
 	host->base = devm_ioremap_resource(&pdev->dev, r);
@@ -729,7 +727,7 @@ static int pxamci_probe(struct platform_device *pdev)
 	writel(64, host->base + MMC_RESTO);
 	writel(host->imask, host->base + MMC_I_MASK);
 
-	ret = devm_request_irq(&pdev->dev, host->irq, pxamci_irq, 0,
+	ret = devm_request_irq(&pdev->dev, irq, pxamci_irq, 0,
 			       DRIVER_NAME, host);
 	if (ret)
 		goto out;
-- 
2.17.1

Re: [PATCH 1/7] mmc: pxamci: remove irq from private context

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> This seems to be a left-over from times before the IRQ was handled by devm
> functions. Remove it.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

[PATCH 2/7] mmc: pxamci: remove dma resources from private context

[Daniel Mack]

These members are no longer in use, so let's remove them.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/mmc/host/pxamci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index f0968882dbd3..a826c0e8a096 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -71,12 +71,8 @@ struct pxamci_host {
 	struct dma_chan		*dma_chan_rx;
 	struct dma_chan		*dma_chan_tx;
 	dma_cookie_t		dma_cookie;
-	dma_addr_t		sg_dma;
 	unsigned int		dma_len;
-
 	unsigned int		dma_dir;
-	unsigned int		dma_drcmrrx;
-	unsigned int		dma_drcmrtx;
 
 	struct regulator	*vcc;
 };
-- 
2.17.1

Re: [PATCH 2/7] mmc: pxamci: remove dma resources from private context

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> These members are no longer in use, so let's remove them.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

[PATCH 3/7] mmc: pxamci: remove dead code from pxamci_remove()

[Daniel Mack]

These gpio assignments don't make sense, as they are not used anywhere.
Remove the dead code.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/mmc/host/pxamci.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index a826c0e8a096..ee1c32862ef8 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -806,18 +806,12 @@ static int pxamci_probe(struct platform_device *pdev)
 static int pxamci_remove(struct platform_device *pdev)
 {
 	struct mmc_host *mmc = platform_get_drvdata(pdev);
-	int gpio_cd = -1, gpio_ro = -1, gpio_power = -1;
 
 	if (mmc) {
 		struct pxamci_host *host = mmc_priv(mmc);
 
 		mmc_remove_host(mmc);
 
-		if (host->pdata) {
-			gpio_cd = host->pdata->gpio_card_detect;
-			gpio_ro = host->pdata->gpio_card_ro;
-			gpio_power = host->pdata->gpio_power;
-		}
 		if (host->pdata && host->pdata->exit)
 			host->pdata->exit(&pdev->dev, mmc);
 
@@ -833,6 +827,7 @@ static int pxamci_remove(struct platform_device *pdev)
 
 		mmc_free_host(mmc);
 	}
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH 3/7] mmc: pxamci: remove dead code from pxamci_remove()

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> These gpio assignments don't make sense, as they are not used anywhere.
> Remove the dead code.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

[PATCH 4/7] mmc: pxamci: fix indenting

[Daniel Mack]

pxamci_of_init() had some weird indenting.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/mmc/host/pxamci.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index ee1c32862ef8..3df60c7babce 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -593,16 +593,16 @@ MODULE_DEVICE_TABLE(of, pxa_mmc_dt_ids);
 
 static int pxamci_of_init(struct platform_device *pdev)
 {
-        struct device_node *np = pdev->dev.of_node;
-        struct pxamci_platform_data *pdata;
-        u32 tmp;
+	struct device_node *np = pdev->dev.of_node;
+	struct pxamci_platform_data *pdata;
+	u32 tmp;
 
-        if (!np)
-                return 0;
+	if (!np)
+		return 0;
 
-        pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
-        if (!pdata)
-                return -ENOMEM;
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
 
 	pdata->gpio_card_detect =
 		of_get_named_gpio(np, "cd-gpios", 0);
@@ -616,9 +616,9 @@ static int pxamci_of_init(struct platform_device *pdev)
 	if (of_property_read_u32(np, "pxa-mmc,detect-delay-ms", &tmp) == 0)
 		pdata->detect_delay_ms = tmp;
 
-        pdev->dev.platform_data = pdata;
+	pdev->dev.platform_data = pdata;
 
-        return 0;
+	return 0;
 }
 #else
 static int pxamci_of_init(struct platform_device *pdev)
-- 
2.17.1

Re: [PATCH 4/7] mmc: pxamci: fix indenting

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> pxamci_of_init() had some weird indenting.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

[PATCH 5/7] mmc: pxamci: call mmc_of_parse()

[Daniel Mack]

Call into mmc_of_parse() from pxamci_of_init(). As it needs a pointer to a
struct mmc_host, refactor the code a bit.

This allows all generic MMC properties to be set that are described in
Documentation/devicetree/bindings/mmc/mmc.txt.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/mmc/host/pxamci.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 3df60c7babce..e15708aa77a2 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -591,11 +591,13 @@ static const struct of_device_id pxa_mmc_dt_ids[] = {
 
 MODULE_DEVICE_TABLE(of, pxa_mmc_dt_ids);
 
-static int pxamci_of_init(struct platform_device *pdev)
+static int pxamci_of_init(struct platform_device *pdev,
+			  struct mmc_host *mmc)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct pxamci_platform_data *pdata;
 	u32 tmp;
+	int ret;
 
 	if (!np)
 		return 0;
@@ -616,12 +618,17 @@ static int pxamci_of_init(struct platform_device *pdev)
 	if (of_property_read_u32(np, "pxa-mmc,detect-delay-ms", &tmp) == 0)
 		pdata->detect_delay_ms = tmp;
 
+	ret = mmc_of_parse(mmc);
+	if (ret < 0)
+		return ret;
+
 	pdev->dev.platform_data = pdata;
 
 	return 0;
 }
 #else
-static int pxamci_of_init(struct platform_device *pdev)
+static int pxamci_of_init(struct platform_device *pdev,
+			  struct mmc_host *mmc)
 {
         return 0;
 }
@@ -634,10 +641,6 @@ static int pxamci_probe(struct platform_device *pdev)
 	struct resource *r;
 	int ret, irq, gpio_cd = -1, gpio_ro = -1, gpio_power = -1;
 
-	ret = pxamci_of_init(pdev);
-	if (ret)
-		return ret;
-
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -672,6 +675,10 @@ static int pxamci_probe(struct platform_device *pdev)
 	 */
 	mmc->max_blk_count = 65535;
 
+	ret = pxamci_of_init(pdev, mmc);
+	if (ret)
+		return ret;
+
 	host = mmc_priv(mmc);
 	host->mmc = mmc;
 	host->pdata = pdev->dev.platform_data;
-- 
2.17.1

Re: [PATCH 5/7] mmc: pxamci: call mmc_of_parse()

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> Call into mmc_of_parse() from pxamci_of_init(). As it needs a pointer to a
> struct mmc_host, refactor the code a bit.
>
> This allows all generic MMC properties to be set that are described in
> Documentation/devicetree/bindings/mmc/mmc.txt.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>

I'm a bit worried about this one :
 - mmc_of_init() will request the gpios from "cd-gpios" and "wp-gpios"
 - pdata->gpio_card_detect will be used to request the same gpio in
 pxa_mci_of_init()

So the gpio is acquired twice, isn't it ? Do you know this works by test proof,
and doesn't it make sense to remove the "pdata->gpio* =" statements from
pxamci_of_init() ?

Cheers.

-- 
Robert

Re: [PATCH 5/7] mmc: pxamci: call mmc_of_parse()

[Daniel Mack]

On Saturday, June 30, 2018 05:25 PM, Robert Jarzmik wrote:
> Daniel Mack <daniel@zonque.org> writes:
> 
>> Call into mmc_of_parse() from pxamci_of_init(). As it needs a pointer to a
>> struct mmc_host, refactor the code a bit.
>>
>> This allows all generic MMC properties to be set that are described in
>> Documentation/devicetree/bindings/mmc/mmc.txt.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
> 
> I'm a bit worried about this one :
>   - mmc_of_init() will request the gpios from "cd-gpios" and "wp-gpios"
>   - pdata->gpio_card_detect will be used to request the same gpio in
>   pxa_mci_of_init()
> 
> So the gpio is acquired twice, isn't it ? Do you know this works by test proof,
> and doesn't it make sense to remove the "pdata->gpio* =" statements from
> pxamci_of_init() ?

Ah, yes. Thanks for spotting this, you're right! Will fix in v2!


Thanks,
Daniel

[PATCH 6/7] mmc: pxamci: remove pxa-mmc,gpio-power from devicetree bindings

[Daniel Mack]

Devicetree-enabled boards should use proper regulators to control the
power of cards, not GPIOs, so let's remove this property. The regulator
properties are are supported by the MMC core and are described in the
generic MMC document:

  Documentation/devicetree/bindings/mmc/mmc.txt

Note that devicetree support for PXA platforms hasn't fully landed yet,
so this binding does not have any users at this point.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 Documentation/devicetree/bindings/mmc/pxa-mmc.txt | 1 -
 drivers/mmc/host/pxamci.c                         | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/pxa-mmc.txt b/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
index b7025de7dced..359b13977f24 100644
--- a/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
@@ -8,7 +8,6 @@ Required properties:
 
 Optional properties:
 - marvell,detect-delay-ms: sets the detection delay timeout in ms.
-- marvell,gpio-power: GPIO spec for the card power enable pin
 
 This file documents differences between the core properties in mmc.txt
 and the properties used by the pxa-mmc driver.
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index e15708aa77a2..c01d69904e2a 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -612,9 +612,6 @@ static int pxamci_of_init(struct platform_device *pdev,
 		of_get_named_gpio(np, "wp-gpios", 0);
 
 	/* pxa-mmc specific */
-	pdata->gpio_power =
-		of_get_named_gpio(np, "pxa-mmc,gpio-power", 0);
-
 	if (of_property_read_u32(np, "pxa-mmc,detect-delay-ms", &tmp) == 0)
 		pdata->detect_delay_ms = tmp;
 
-- 
2.17.1

Re: [PATCH 6/7] mmc: pxamci: remove pxa-mmc,gpio-power from devicetree bindings

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> Devicetree-enabled boards should use proper regulators to control the
> power of cards, not GPIOs, so let's remove this property. The regulator
> properties are are supported by the MMC core and are described in the
s/are are/are/

> Note that devicetree support for PXA platforms hasn't fully landed yet,
> so this binding does not have any users at this point.
No users in-tree to be more specific.
> diff --git a/Documentation/devicetree/bindings/mmc/pxa-mmc.txt b/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
> index b7025de7dced..359b13977f24 100644
> --- a/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/pxa-mmc.txt
> @@ -8,7 +8,6 @@ Required properties:
>  
>  Optional properties:
>  - marvell,detect-delay-ms: sets the detection delay timeout in ms.
> -- marvell,gpio-power: GPIO spec for the card power enable pin
Okay, I don't have a big devicetree knowledge, but wouldn't it make sense to
augment the example in this file to show the proper usage of card power (and at
it wp-gpio and cd-gpio) ?

Cheers.

-- 
Robert

[PATCH 7/7] mmc: pxamci: let mmc core handle regulators

[Daniel Mack]

Strip some code by letting the mmc core handle the regulators. The old
.gpio_power pdata handling is kept around for now.

This also set the voltage on the regulator and handles -EPROBE_DEFER
correctly.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/mmc/host/pxamci.c | 54 ++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 32 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index c01d69904e2a..f40550c4a7a1 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -73,58 +73,46 @@ struct pxamci_host {
 	dma_cookie_t		dma_cookie;
 	unsigned int		dma_len;
 	unsigned int		dma_dir;
-
-	struct regulator	*vcc;
 };
 
-static inline void pxamci_init_ocr(struct pxamci_host *host)
+static int pxamci_init_ocr(struct pxamci_host *host)
 {
-#ifdef CONFIG_REGULATOR
-	host->vcc = devm_regulator_get_optional(mmc_dev(host->mmc), "vmmc");
-
-	if (IS_ERR(host->vcc))
-		host->vcc = NULL;
-	else {
-		host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc);
-		if (host->pdata && host->pdata->ocr_mask)
-			dev_warn(mmc_dev(host->mmc),
-				"ocr_mask/setpower will not be used\n");
-	}
-#endif
-	if (host->vcc == NULL) {
+	struct mmc_host *mmc = host->mmc;
+	int ret;
+
+	ret = mmc_regulator_get_supply(mmc);
+	if (ret < 0)
+		return ret;
+
+	if (IS_ERR(mmc->supply.vmmc)) {
 		/* fall-back to platform data */
-		host->mmc->ocr_avail = host->pdata ?
+		mmc->ocr_avail = host->pdata ?
 			host->pdata->ocr_mask :
 			MMC_VDD_32_33 | MMC_VDD_33_34;
 	}
+
+	return 0;
 }
 
 static inline int pxamci_set_power(struct pxamci_host *host,
 				    unsigned char power_mode,
 				    unsigned int vdd)
 {
+	struct mmc_host *mmc = host->mmc;
+	struct regulator *supply = mmc->supply.vmmc;
 	int on;
 
-	if (host->vcc) {
-		int ret;
+	if (!IS_ERR(supply))
+		return mmc_regulator_set_ocr(mmc, supply, vdd);
 
-		if (power_mode == MMC_POWER_UP) {
-			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
-			if (ret)
-				return ret;
-		} else if (power_mode == MMC_POWER_OFF) {
-			ret = mmc_regulator_set_ocr(host->mmc, host->vcc, 0);
-			if (ret)
-				return ret;
-		}
-	}
-	if (!host->vcc && host->pdata &&
+	if (host->pdata &&
 	    gpio_is_valid(host->pdata->gpio_power)) {
 		on = ((1 << vdd) & host->pdata->ocr_mask);
 		gpio_set_value(host->pdata->gpio_power,
 			       !!on ^ host->pdata->gpio_power_invert);
 	}
-	if (!host->vcc && host->pdata && host->pdata->setpower)
+
+	if (host->pdata && host->pdata->setpower)
 		return host->pdata->setpower(mmc_dev(host->mmc), vdd);
 
 	return 0;
@@ -696,7 +684,9 @@ static int pxamci_probe(struct platform_device *pdev)
 	mmc->f_min = (host->clkrate + 63) / 64;
 	mmc->f_max = (mmc_has_26MHz()) ? 26000000 : host->clkrate;
 
-	pxamci_init_ocr(host);
+	ret = pxamci_init_ocr(host);
+	if (ret < 0)
+		return ret;
 
 	mmc->caps = 0;
 	host->cmdat = 0;
-- 
2.17.1

Re: [PATCH 7/7] mmc: pxamci: let mmc core handle regulators

[Robert Jarzmik]

Daniel Mack <daniel@zonque.org> writes:

> Strip some code by letting the mmc core handle the regulators. The old
> .gpio_power pdata handling is kept around for now.
>
> This also set the voltage on the regulator and handles -EPROBE_DEFER
> correctly.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
I'll take it as it was tested, right ?

Therefore:
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

--
Robert

Re: [PATCH 7/7] mmc: pxamci: let mmc core handle regulators

[Daniel Mack]

On Saturday, June 30, 2018 05:25 PM, Robert Jarzmik wrote:
> Daniel Mack <daniel@zonque.org> writes:
> 
>> Strip some code by letting the mmc core handle the regulators. The old
>> .gpio_power pdata handling is kept around for now.
>>
>> This also set the voltage on the regulator and handles -EPROBE_DEFER
>> correctly.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
> I'll take it as it was tested, right ?

Yes, with a non-removable SDIO device.

Thanks for your reviews and Acked-bys!


Daniel