[PATCH 1/2] spi: sh-msiof: Use devm_* managed allocators

[PATCH 1/2] spi: sh-msiof: Use devm_* managed allocators

[Laurent Pinchart]

This simplifies error and cleanup code paths.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/spi/spi-sh-msiof.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index c74298c..541e1ec 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -635,8 +635,7 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
 	master = spi_alloc_master(&pdev->dev, sizeof(struct sh_msiof_spi_priv));
 	if (master == NULL) {
 		dev_err(&pdev->dev, "failed to allocate spi master\n");
-		ret = -ENOMEM;
-		goto err0;
+		return -ENOMEM;
 	}
 
 	p = spi_master_get_devdata(master);
@@ -655,32 +654,32 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
 
 	init_completion(&p->done);
 
-	p->clk = clk_get(&pdev->dev, NULL);
+	p->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(p->clk)) {
 		dev_err(&pdev->dev, "cannot get clock\n");
 		ret = PTR_ERR(p->clk);
 		goto err1;
 	}
 
-	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	i = platform_get_irq(pdev, 0);
-	if (!r || i < 0) {
-		dev_err(&pdev->dev, "cannot get platform resources\n");
+	if (i < 0) {
+		dev_err(&pdev->dev, "cannot get platform IRQ\n");
 		ret = -ENOENT;
-		goto err2;
+		goto err1;
 	}
-	p->mapbase = ioremap_nocache(r->start, resource_size(r));
-	if (!p->mapbase) {
-		dev_err(&pdev->dev, "unable to ioremap\n");
-		ret = -ENXIO;
-		goto err2;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	p->mapbase = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(p->mapbase)) {
+		ret = PTR_ERR(p->mapbase);
+		goto err1;
 	}
 
-	ret = request_irq(i, sh_msiof_spi_irq, 0,
-			  dev_name(&pdev->dev), p);
+	ret = devm_request_irq(&pdev->dev, i, sh_msiof_spi_irq, 0,
+			       dev_name(&pdev->dev), p);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to request irq\n");
-		goto err3;
+		goto err1;
 	}
 
 	p->pdev = pdev;
@@ -719,13 +718,8 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
 		return 0;
 
 	pm_runtime_disable(&pdev->dev);
- err3:
-	iounmap(p->mapbase);
- err2:
-	clk_put(p->clk);
  err1:
 	spi_master_put(master);
- err0:
 	return ret;
 }
 
@@ -737,9 +731,6 @@ static int sh_msiof_spi_remove(struct platform_device *pdev)
 	ret = spi_bitbang_stop(&p->bitbang);
 	if (!ret) {
 		pm_runtime_disable(&pdev->dev);
-		free_irq(platform_get_irq(pdev, 0), p);
-		iounmap(p->mapbase);
-		clk_put(p->clk);
 		spi_master_put(p->bitbang.master);
 	}
 	return ret;
-- 
1.8.3.2

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

[PATCH 2/2] spi: sh-msiof: Convert to clk_prepare/unprepare

[Laurent Pinchart]

Get the driver ready for the migration to the common clock framework by
calling clk_prepare() and clk_unprepare(). The calls are added in the
probe and remove handlers as the clk_enable() and clk_disable() calls
are located in atomic context and there's no callback function in
non-atomic context that can be used to prepare/unprepare the clock.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/spi/spi-sh-msiof.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 541e1ec..fbbe949 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -682,6 +682,12 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
 		goto err1;
 	}
 
+	ret = clk_prepare(p->clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "unable to prepare clock\n");
+		goto err1;
+	}
+
 	p->pdev = pdev;
 	pm_runtime_enable(&pdev->dev);
 
@@ -718,6 +724,7 @@ static int sh_msiof_spi_probe(struct platform_device *pdev)
 		return 0;
 
 	pm_runtime_disable(&pdev->dev);
+	clk_unprepare(p->clk);
  err1:
 	spi_master_put(master);
 	return ret;
@@ -731,6 +738,7 @@ static int sh_msiof_spi_remove(struct platform_device *pdev)
 	ret = spi_bitbang_stop(&p->bitbang);
 	if (!ret) {
 		pm_runtime_disable(&pdev->dev);
+		clk_unprepare(p->clk);
 		spi_master_put(p->bitbang.master);
 	}
 	return ret;
-- 
1.8.3.2

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

Re: [PATCH 1/2] spi: sh-msiof: Use devm_* managed allocators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 132 bytes --]

On Thu, Nov 28, 2013 at 02:39:42AM +0100, Laurent Pinchart wrote:
> This simplifies error and cleanup code paths.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: sh-msiof: Convert to clk_prepare/unprepare

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Thu, Nov 28, 2013 at 02:39:43AM +0100, Laurent Pinchart wrote:

> Get the driver ready for the migration to the common clock framework by
> calling clk_prepare() and clk_unprepare(). The calls are added in the
> probe and remove handlers as the clk_enable() and clk_disable() calls
> are located in atomic context and there's no callback function in
> non-atomic context that can be used to prepare/unprepare the clock.

Applied, thanks.  However it does seem like you should be able to move
the prepare and disable to runtime PM rather than leaving it prepared
for the entire time the driver is loaded.  In most systems this won't
make any difference since prepare won't do anything but it's a bit
neater.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] spi: sh-msiof: Convert to clk_prepare/unprepare

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

Hi Mark,

On Thursday 28 November 2013 10:34:07 Mark Brown wrote:
> On Thu, Nov 28, 2013 at 02:39:43AM +0100, Laurent Pinchart wrote:
> > Get the driver ready for the migration to the common clock framework by
> > calling clk_prepare() and clk_unprepare(). The calls are added in the
> > probe and remove handlers as the clk_enable() and clk_disable() calls
> > are located in atomic context and there's no callback function in
> > non-atomic context that can be used to prepare/unprepare the clock.
> 
> Applied, thanks.  However it does seem like you should be able to move
> the prepare and disable to runtime PM rather than leaving it prepared
> for the entire time the driver is loaded.  In most systems this won't
> make any difference since prepare won't do anything but it's a bit
> neater.

It would be, but I believe the driver is currently broken in that regard. It 
calls the runtime PM sync API with a spinlock held, which if I'm not mistaken 
isn't right. That should be fixed, but is a bit out of scope of this work, 
hence the simple solution.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/2] spi: sh-msiof: Convert to clk_prepare/unprepare

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On Thu, Nov 28, 2013 at 03:16:19PM +0100, Laurent Pinchart wrote:
> On Thursday 28 November 2013 10:34:07 Mark Brown wrote:

> > Applied, thanks.  However it does seem like you should be able to move
> > the prepare and disable to runtime PM rather than leaving it prepared
> > for the entire time the driver is loaded.  In most systems this won't
> > make any difference since prepare won't do anything but it's a bit
> > neater.

> It would be, but I believe the driver is currently broken in that regard. It 
> calls the runtime PM sync API with a spinlock held, which if I'm not mistaken 
> isn't right. That should be fixed, but is a bit out of scope of this work, 
> hence the simple solution.

Yeah, that's broken and should be fixed - like I say I doubt it'll
matter either way in practical systems if the clock is prepared all the
time anyway.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]