[PATCH 1/2] mmc: mxcmmc: Convert to devm-* API

[PATCH 1/2] mmc: mxcmmc: Convert to devm-* API

[Alexander Shiyan]

Replace existing resource handling in the driver with managed
device resource, this ensures more consistent error values and
simplifies error paths.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mmc/host/mxcmmc.c | 101 +++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 63 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index f7199c8..84d630e 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -124,9 +124,8 @@ enum mxcmci_type {
 
 struct mxcmci_host {
 	struct mmc_host		*mmc;
-	struct resource		*res;
 	void __iomem		*base;
-	int			irq;
+	dma_addr_t		phys_base;
 	int			detect_irq;
 	struct dma_chan		*dma;
 	struct dma_async_tx_descriptor *desc;
@@ -241,33 +240,26 @@ static inline void mxcmci_writew(struct mxcmci_host *host, u16 val, int reg)
 
 static void mxcmci_set_clk_rate(struct mxcmci_host *host, unsigned int clk_ios);
 
-static inline void mxcmci_init_ocr(struct mxcmci_host *host)
+static void mxcmci_init_ocr(struct mxcmci_host *host)
 {
-	host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc");
-
+	host->vcc = devm_regulator_get(mmc_dev(host->mmc), "vmmc");
 	if (IS_ERR(host->vcc)) {
-		host->vcc = NULL;
+		if (host->pdata && host->pdata->ocr_avail)
+			host->mmc->ocr_avail = host->pdata->ocr_avail;
+		else
+			host->mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
 	} else {
 		host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc);
 		if (host->pdata && host->pdata->ocr_avail)
 			dev_warn(mmc_dev(host->mmc),
 				"pdata->ocr_avail will not be used\n");
 	}
-
-	if (host->vcc == NULL) {
-		/* fall-back to platform data */
-		if (host->pdata && host->pdata->ocr_avail)
-			host->mmc->ocr_avail = host->pdata->ocr_avail;
-		else
-			host->mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
-	}
 }
 
-static inline void mxcmci_set_power(struct mxcmci_host *host,
-				    unsigned char power_mode,
-				    unsigned int vdd)
+static void mxcmci_set_power(struct mxcmci_host *host, unsigned char power_mode,
+			     unsigned int vdd)
 {
-	if (host->vcc) {
+	if (!IS_ERR(host->vcc)) {
 		if (power_mode == MMC_POWER_UP)
 			mmc_regulator_set_ocr(host->mmc, host->vcc, vdd);
 		else if (power_mode == MMC_POWER_OFF)
@@ -299,7 +291,6 @@ static void mxcmci_softreset(struct mxcmci_host *host)
 
 	mxcmci_writew(host, 0xff, MMC_REG_RES_TO);
 }
-static int mxcmci_setup_dma(struct mmc_host *mmc);
 
 #if IS_ENABLED(CONFIG_PPC_MPC512x)
 static inline void buffer_swap32(u32 *buf, int len)
@@ -868,8 +859,8 @@ static int mxcmci_setup_dma(struct mmc_host *mmc)
 	struct mxcmci_host *host = mmc_priv(mmc);
 	struct dma_slave_config *config = &host->dma_slave_config;
 
-	config->dst_addr = host->res->start + MMC_REG_BUFFER_ACCESS;
-	config->src_addr = host->res->start + MMC_REG_BUFFER_ACCESS;
+	config->dst_addr = host->phys_base + MMC_REG_BUFFER_ACCESS;
+	config->src_addr = host->phys_base + MMC_REG_BUFFER_ACCESS;
 	config->dst_addr_width = 4;
 	config->src_addr_width = 4;
 	config->dst_maxburst = host->burstlen;
@@ -1040,8 +1031,8 @@ static const struct mmc_host_ops mxcmci_ops = {
 static int mxcmci_probe(struct platform_device *pdev)
 {
 	struct mmc_host *mmc;
-	struct mxcmci_host *host = NULL;
-	struct resource *iores, *r;
+	struct mxcmci_host *host;
+	struct resource *res;
 	int ret = 0, irq;
 	bool dat3_card_detect = false;
 	dma_cap_mask_t mask;
@@ -1052,21 +1043,25 @@ static int mxcmci_probe(struct platform_device *pdev)
 
 	of_id = of_match_device(mxcmci_of_match, &pdev->dev);
 
-	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(pdev, 0);
-	if (!iores || irq < 0)
+	if (irq < 0)
 		return -EINVAL;
 
-	r = request_mem_region(iores->start, resource_size(iores), pdev->name);
-	if (!r)
-		return -EBUSY;
+	mmc = mmc_alloc_host(sizeof(*host), &pdev->dev);
+	if (!mmc)
+		return -ENOMEM;
 
-	mmc = mmc_alloc_host(sizeof(struct mxcmci_host), &pdev->dev);
-	if (!mmc) {
-		ret = -ENOMEM;
-		goto out_release_mem;
+	host = mmc_priv(mmc);
+
+	host->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(host->base)) {
+		ret = PTR_ERR(host->base);
+		goto out_free;
 	}
 
+	host->phys_base = res->start;
+
 	ret = mmc_of_parse(mmc);
 	if (ret)
 		goto out_free;
@@ -1084,13 +1079,6 @@ static int mxcmci_probe(struct platform_device *pdev)
 	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
 	mmc->max_seg_size = mmc->max_req_size;
 
-	host = mmc_priv(mmc);
-	host->base = ioremap(r->start, resource_size(r));
-	if (!host->base) {
-		ret = -ENOMEM;
-		goto out_free;
-	}
-
 	if (of_id) {
 		const struct platform_device_id *id_entry = of_id->data;
 		host->devtype = id_entry->driver_data;
@@ -1120,19 +1108,16 @@ static int mxcmci_probe(struct platform_device *pdev)
 	else
 		host->default_irq_mask = 0;
 
-	host->res = r;
-	host->irq = irq;
-
 	host->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
 	if (IS_ERR(host->clk_ipg)) {
 		ret = PTR_ERR(host->clk_ipg);
-		goto out_iounmap;
+		goto out_free;
 	}
 
 	host->clk_per = devm_clk_get(&pdev->dev, "per");
 	if (IS_ERR(host->clk_per)) {
 		ret = PTR_ERR(host->clk_per);
-		goto out_iounmap;
+		goto out_free;
 	}
 
 	clk_prepare_enable(host->clk_per);
@@ -1159,9 +1144,9 @@ static int mxcmci_probe(struct platform_device *pdev)
 	if (!host->pdata) {
 		host->dma = dma_request_slave_channel(&pdev->dev, "rx-tx");
 	} else {
-		r = platform_get_resource(pdev, IORESOURCE_DMA, 0);
-		if (r) {
-			host->dmareq = r->start;
+		res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
+		if (res) {
+			host->dmareq = res->start;
 			host->dma_data.peripheral_type = IMX_DMATYPE_SDHC;
 			host->dma_data.priority = DMA_PRIO_LOW;
 			host->dma_data.dma_request = host->dmareq;
@@ -1178,7 +1163,8 @@ static int mxcmci_probe(struct platform_device *pdev)
 
 	INIT_WORK(&host->datawork, mxcmci_datawork);
 
-	ret = request_irq(host->irq, mxcmci_irq, 0, DRIVER_NAME, host);
+	ret = devm_request_irq(&pdev->dev, irq, mxcmci_irq, 0,
+			       dev_name(&pdev->dev), host);
 	if (ret)
 		goto out_free_dma;
 
@@ -1188,7 +1174,7 @@ static int mxcmci_probe(struct platform_device *pdev)
 		ret = host->pdata->init(&pdev->dev, mxcmci_detect_irq,
 				host->mmc);
 		if (ret)
-			goto out_free_irq;
+			goto out_free_dma;
 	}
 
 	init_timer(&host->watchdog);
@@ -1199,20 +1185,17 @@ static int mxcmci_probe(struct platform_device *pdev)
 
 	return 0;
 
-out_free_irq:
-	free_irq(host->irq, host);
 out_free_dma:
 	if (host->dma)
 		dma_release_channel(host->dma);
+
 out_clk_put:
 	clk_disable_unprepare(host->clk_per);
 	clk_disable_unprepare(host->clk_ipg);
-out_iounmap:
-	iounmap(host->base);
+
 out_free:
 	mmc_free_host(mmc);
-out_release_mem:
-	release_mem_region(iores->start, resource_size(iores));
+
 	return ret;
 }
 
@@ -1223,23 +1206,15 @@ static int mxcmci_remove(struct platform_device *pdev)
 
 	mmc_remove_host(mmc);
 
-	if (host->vcc)
-		regulator_put(host->vcc);
-
 	if (host->pdata && host->pdata->exit)
 		host->pdata->exit(&pdev->dev, mmc);
 
-	free_irq(host->irq, host);
-	iounmap(host->base);
-
 	if (host->dma)
 		dma_release_channel(host->dma);
 
 	clk_disable_unprepare(host->clk_per);
 	clk_disable_unprepare(host->clk_ipg);
 
-	release_mem_region(host->res->start, resource_size(host->res));
-
 	mmc_free_host(mmc);
 
 	return 0;
-- 
1.8.3.2

[PATCH 2/2] mmc: mxcmmc: Convert to SIMPLE_DEV_PM_OPS

[Alexander Shiyan]

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/mmc/host/mxcmmc.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 84d630e..86401c0 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -1220,8 +1220,7 @@ static int mxcmci_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int mxcmci_suspend(struct device *dev)
+static int __maybe_unused mxcmci_suspend(struct device *dev)
 {
 	struct mmc_host *mmc = dev_get_drvdata(dev);
 	struct mxcmci_host *host = mmc_priv(mmc);
@@ -1231,7 +1230,7 @@ static int mxcmci_suspend(struct device *dev)
 	return 0;
 }
 
-static int mxcmci_resume(struct device *dev)
+static int __maybe_unused mxcmci_resume(struct device *dev)
 {
 	struct mmc_host *mmc = dev_get_drvdata(dev);
 	struct mxcmci_host *host = mmc_priv(mmc);
@@ -1241,11 +1240,7 @@ static int mxcmci_resume(struct device *dev)
 	return 0;
 }
 
-static const struct dev_pm_ops mxcmci_pm_ops = {
-	.suspend	= mxcmci_suspend,
-	.resume		= mxcmci_resume,
-};
-#endif
+static SIMPLE_DEV_PM_OPS(mxcmci_pm_ops, mxcmci_suspend, mxcmci_resume);
 
 static struct platform_driver mxcmci_driver = {
 	.probe		= mxcmci_probe,
@@ -1254,9 +1249,7 @@ static struct platform_driver mxcmci_driver = {
 	.driver		= {
 		.name		= DRIVER_NAME,
 		.owner		= THIS_MODULE,
-#ifdef CONFIG_PM
 		.pm	= &mxcmci_pm_ops,
-#endif
 		.of_match_table	= mxcmci_of_match,
 	}
 };
-- 
1.8.3.2

Re: [PATCH 2/2] mmc: mxcmmc: Convert to SIMPLE_DEV_PM_OPS

[Fabio Estevam]

On Sat, Feb 22, 2014 at 11:46 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  drivers/mmc/host/mxcmmc.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 84d630e..86401c0 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -1220,8 +1220,7 @@ static int mxcmci_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> -#ifdef CONFIG_PM
> -static int mxcmci_suspend(struct device *dev)

Doesn't this need to be protected by CONFIG_PM_SLEEP?

Re: [PATCH 2/2] mmc: mxcmmc: Convert to SIMPLE_DEV_PM_OPS

[Alexander Shiyan]

Суббота, 22 февраля 2014, 11:51 -03:00 от Fabio Estevam <festevam@gmail.com>:
> On Sat, Feb 22, 2014 at 11:46 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  drivers/mmc/host/mxcmmc.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> > index 84d630e..86401c0 100644
> > --- a/drivers/mmc/host/mxcmmc.c
> > +++ b/drivers/mmc/host/mxcmmc.c
> > @@ -1220,8 +1220,7 @@ static int mxcmci_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > -#ifdef CONFIG_PM
> > -static int mxcmci_suspend(struct device *dev)
> 
> Doesn't this need to be protected by CONFIG_PM_SLEEP?

No, this already protected by SIMPLE_DEV_PM_OPS(),
see include/linux/pm.h.

---

Re: [PATCH 2/2] mmc: mxcmmc: Convert to SIMPLE_DEV_PM_OPS

[Fabio Estevam]

On Sat, Feb 22, 2014 at 11:57 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> Суббота, 22 февраля 2014, 11:51 -03:00 от Fabio Estevam <festevam@gmail.com>:
>> On Sat, Feb 22, 2014 at 11:46 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
>> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
>> > ---
>> >  drivers/mmc/host/mxcmmc.c | 13 +++----------
>> >  1 file changed, 3 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
>> > index 84d630e..86401c0 100644
>> > --- a/drivers/mmc/host/mxcmmc.c
>> > +++ b/drivers/mmc/host/mxcmmc.c
>> > @@ -1220,8 +1220,7 @@ static int mxcmci_remove(struct platform_device *pdev)
>> >         return 0;
>> >  }
>> >
>> > -#ifdef CONFIG_PM
>> > -static int mxcmci_suspend(struct device *dev)
>>
>> Doesn't this need to be protected by CONFIG_PM_SLEEP?
>
> No, this already protected by SIMPLE_DEV_PM_OPS(),
> see include/linux/pm.h.

but then you would get a warning saying that "mxcmci_suspend/resume"
are defined by not used" when CONFIG_PM_SLEEP=n

Re: [PATCH 2/2] mmc: mxcmmc: Convert to SIMPLE_DEV_PM_OPS

[Alexander Shiyan]

Суббота, 22 февраля 2014, 11:59 -03:00 от Fabio Estevam <festevam@gmail.com>:
> On Sat, Feb 22, 2014 at 11:57 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> > Суббота, 22 февраля 2014, 11:51 -03:00 от Fabio Estevam <festevam@gmail.com>:
> >> On Sat, Feb 22, 2014 at 11:46 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> >> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> >> > ---
> >> >  drivers/mmc/host/mxcmmc.c | 13 +++----------
> >> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> >> > index 84d630e..86401c0 100644
> >> > --- a/drivers/mmc/host/mxcmmc.c
> >> > +++ b/drivers/mmc/host/mxcmmc.c
> >> > @@ -1220,8 +1220,7 @@ static int mxcmci_remove(struct platform_device *pdev)
> >> >         return 0;
> >> >  }
> >> >
> >> > -#ifdef CONFIG_PM
> >> > -static int mxcmci_suspend(struct device *dev)
> >>
> >> Doesn't this need to be protected by CONFIG_PM_SLEEP?
> >
> > No, this already protected by SIMPLE_DEV_PM_OPS(),
> > see include/linux/pm.h.
> 
> but then you would get a warning saying that "mxcmci_suspend/resume"
> are defined by not used" when CONFIG_PM_SLEEP=n

Please see the patch first,
this warnings is avoided by "__maybe_unused" attributes.

---

Re: [PATCH 2/2] mmc: mxcmmc: Convert to SIMPLE_DEV_PM_OPS

[Fabio Estevam]

On Sat, Feb 22, 2014 at 12:02 PM, Alexander Shiyan <shc_work@mail.ru> wrote:

>> but then you would get a warning saying that "mxcmci_suspend/resume"
>> are defined by not used" when CONFIG_PM_SLEEP=n
>
> Please see the patch first,
> this warnings is avoided by "__maybe_unused" attributes.

Ok, I see it now ;-)

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>

Re: [PATCH 1/2] mmc: mxcmmc: Convert to devm-* API

[Arnd Bergmann]

On Saturday 22 February 2014 18:46:11 Alexander Shiyan wrote:
> Replace existing resource handling in the driver with managed
> device resource, this ensures more consistent error values and
> simplifies error paths.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Nice cleanup!

>  drivers/mmc/host/mxcmmc.c | 101 +++++++++++++++++-----------------------------
>  1 file changed, 38 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index f7199c8..84d630e 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -124,9 +124,8 @@ enum mxcmci_type {
>  
>  struct mxcmci_host {
>         struct mmc_host         *mmc;
> -       struct resource         *res;
>         void __iomem            *base;
> -       int                     irq;
> +       dma_addr_t              phys_base;
>         int                     detect_irq;
>         struct dma_chan         *dma;
>         struct dma_async_tx_descriptor *desc;

Just nitpicking, but I think phys_base should be either phys_addr_t or
resource_size_t. dma_addr_t is what you get out of the dma-mapping API,
not what you use for ioremap. In theory they may be different, but I
don't know how we'd handle that case for the dmaengine API, since we
don't currently have a way to convert between the two, other than for
doing DMA on memory pages.

	Arnd

Re: [PATCH 2/2] mmc: mxcmmc: Convert to SIMPLE_DEV_PM_OPS

[Arnd Bergmann]

On Saturday 22 February 2014 12:05:43 Fabio Estevam wrote:
> On Sat, Feb 22, 2014 at 12:02 PM, Alexander Shiyan <shc_work@mail.ru> wrote:
> 
> >> but then you would get a warning saying that "mxcmci_suspend/resume"
> >> are defined by not used" when CONFIG_PM_SLEEP=n
> >
> > Please see the patch first,
> > this warnings is avoided by "__maybe_unused" attributes.
> 
> Ok, I see it now 
> 
> Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
> 

Unfortunately, everyone else uses the #ifdef here, and a lot of people
don't understand how to use __maybe_unused for this. IMHO all
drivers should do it the way that Alexander does it here, or
we should change the SIMPLE_DEV_PM_OPS() definition in a way that
doesn't need the #ifdef.

I have in the past tried to come up with a way to handle this more
naturally, but couldn't figure out how to migrate to it. If
SIMPLE_DEV_PM_OPS() defines an unused variable pointing to the
operations, we need neither the #ifdef nor the __maybe_unused
marker, but then it fails to build when there is an #ifdef.

	Arnd

Re: [PATCH 1/2] mmc: mxcmmc: Convert to devm-* API

[Alexander Shiyan]

Суббота, 22 февраля 2014, 16:37 +01:00 от Arnd Bergmann <arnd@arndb.de>:
> On Saturday 22 February 2014 18:46:11 Alexander Shiyan wrote:
> > Replace existing resource handling in the driver with managed
> > device resource, this ensures more consistent error values and
> > simplifies error paths.
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> 
> Nice cleanup!
...
> > +++ b/drivers/mmc/host/mxcmmc.c
> > @@ -124,9 +124,8 @@ enum mxcmci_type {
> >  
> >  struct mxcmci_host {
> >         struct mmc_host         *mmc;
> > -       struct resource         *res;
> >         void __iomem            *base;
> > -       int                     irq;
> > +       dma_addr_t              phys_base;
> >         int                     detect_irq;
> >         struct dma_chan         *dma;
> >         struct dma_async_tx_descriptor *desc;
> 
> Just nitpicking, but I think phys_base should be either phys_addr_t or
> resource_size_t. dma_addr_t is what you get out of the dma-mapping API,
> not what you use for ioremap. In theory they may be different, but I
> don't know how we'd handle that case for the dmaengine API, since we
> don't currently have a way to convert between the two, other than for
> doing DMA on memory pages.

Maybe I'll think of something in the subsequent cleanup of the driver.
Currently, many things look in the driver just scary, here for example:

static inline void mxcmci_writel(struct mxcmci_host *host, u32 val, int reg)
{
	if (IS_ENABLED(CONFIG_PPC_MPC512x))
		iowrite32be(val, host->base + reg);
	else
		writel(val, host->base + reg);
}

---