[3/9] I2C: mv64xxx: use devm_ioremap_resource()

Commit Message

Russell King May 16, 2013, 8:33 p.m. UTC

Eliminate reg_base_p and reg_size, mv64xxx_i2c_unmap_regs() and an
unchecked ioremap() return from this driver by using the devm_*
API for requesting and ioremapping resources.

Signed-off-by: Russell King <[e-mail]>
---
 drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
 1 files changed, 6 insertions(+), 40 deletions(-)

Comments

Jean-Francois Moine May 17, 2013, 9:23 a.m. UTC | #1

On Thu, 16 May 2013 21:33:09 +0100
Russell King <[e-mail]> wrote:

> Eliminate reg_base_p and reg_size, mv64xxx_i2c_unmap_regs() and an
> unchecked ioremap() return from this driver by using the devm_*
> API for requesting and ioremapping resources.
> 
> Signed-off-by: Russell King <[e-mail]>
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
>  1 files changed, 6 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 0339cd8..19cc9bf 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
	[snip]
> @@ -704,7 +671,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
>  
>  	rc = i2c_del_adapter(&drv_data->adapter);
>  	free_irq(drv_data->irq, drv_data);
> -	mv64xxx_i2c_unmap_regs(drv_data);
>  #if defined(CONFIG_HAVE_CLK)
>  	/* Not all platforms have a clk */
>  	if (!IS_ERR(drv_data->clk)) {

The patch does not apply: it seems it lacks:

+	int rc;

-	i2c_del_adapter(&drv_data->adapter);
+	rc = i2c_del_adapter(&drv_data->adapter);

-	return 0;
+	return rc;

BTW, is this return code useful?

Russell King - ARM Linux May 17, 2013, 9:34 a.m. UTC | #2

On Fri, May 17, 2013 at 11:23:16AM +0200, Jean-Francois Moine wrote:
> On Thu, 16 May 2013 21:33:09 +0100
> Russell King <[e-mail]> wrote:
> 
> > Eliminate reg_base_p and reg_size, mv64xxx_i2c_unmap_regs() and an
> > unchecked ioremap() return from this driver by using the devm_*
> > API for requesting and ioremapping resources.
> > 
> > Signed-off-by: Russell King <[e-mail]>
> > ---
> >  drivers/i2c/busses/i2c-mv64xxx.c |   46 +++++---------------------------------
> >  1 files changed, 6 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> > index 0339cd8..19cc9bf 100644
> > --- a/drivers/i2c/busses/i2c-mv64xxx.c
> > +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> 	[snip]
> > @@ -704,7 +671,6 @@ mv64xxx_i2c_remove(struct platform_device *dev)
> >  
> >  	rc = i2c_del_adapter(&drv_data->adapter);
> >  	free_irq(drv_data->irq, drv_data);
> > -	mv64xxx_i2c_unmap_regs(drv_data);
> >  #if defined(CONFIG_HAVE_CLK)
> >  	/* Not all platforms have a clk */
> >  	if (!IS_ERR(drv_data->clk)) {
> 
> The patch does not apply: it seems it lacks:

I should've mentioned that the patches are against v3.9, not v3.10-rc1.

> +	int rc;
> 
> -	i2c_del_adapter(&drv_data->adapter);
> +	rc = i2c_del_adapter(&drv_data->adapter);
> 
> -	return 0;
> +	return rc;
> 
> BTW, is this return code useful?

No it isn't.  Removals *can't* fail.  Once the remove function is called,
the device _will_ be unbound no matter what the return value of that
function is.  The module will also be unloaded (if that's why it's being
unbound) whether or not you return an error.

So, removals must always succeed.  Why their return type hasn't been void
from the start I've no idea - but that's a question for Patrick Mochel
who implemented the original driver model design.

Patch

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 0339cd8..19cc9bf 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -92,8 +92,6 @@  struct mv64xxx_i2c_data {
 	u32			aborting;
 	u32			cntl_bits;
 	void __iomem		*reg_base;
-	u32			reg_base_p;
-	u32			reg_size;
 	u32			addr1;
 	u32			addr2;
 	u32			bytes_left;
@@ -495,40 +493,6 @@  static const struct i2c_algorithm mv64xxx_i2c_algo = {
  *
  *****************************************************************************
  */
-static int
-mv64xxx_i2c_map_regs(struct platform_device *pd,
-	struct mv64xxx_i2c_data *drv_data)
-{
-	int size;
-	struct resource	*r = platform_get_resource(pd, IORESOURCE_MEM, 0);
-
-	if (!r)
-		return -ENODEV;
-
-	size = resource_size(r);
-
-	if (!request_mem_region(r->start, size, drv_data->adapter.name))
-		return -EBUSY;
-
-	drv_data->reg_base = ioremap(r->start, size);
-	drv_data->reg_base_p = r->start;
-	drv_data->reg_size = size;
-
-	return 0;
-}
-
-static void
-mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
-{
-	if (drv_data->reg_base) {
-		iounmap(drv_data->reg_base);
-		release_mem_region(drv_data->reg_base_p, drv_data->reg_size);
-	}
-
-	drv_data->reg_base = NULL;
-	drv_data->reg_base_p = 0;
-}
-
 #ifdef CONFIG_OF
 static int
 mv64xxx_calc_freq(const int tclk, const int n, const int m)
@@ -610,6 +574,7 @@  mv64xxx_i2c_probe(struct platform_device *pd)
 {
 	struct mv64xxx_i2c_data		*drv_data;
 	struct mv64xxx_i2c_pdata	*pdata = pd->dev.platform_data;
+	struct resource	*r;
 	int	rc;
 
 	if ((!pdata && !pd->dev.of_node))
@@ -619,9 +584,12 @@  mv64xxx_i2c_probe(struct platform_device *pd)
 	if (!drv_data)
 		return -ENOMEM;
 
-	rc = mv64xxx_i2c_map_regs(pd, drv_data);
-	if (rc)
+	r = platform_get_resource(pd, IORESOURCE_MEM, 0);
+	drv_data->reg_base = devm_ioremap_resource(&pd->dev, r);
+	if (IS_ERR(drv_data->reg_base)) {
+		rc = PTR_ERR(drv_data->reg_base);
 		goto exit_kfree;
+	}
 
 	strlcpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
 		sizeof(drv_data->adapter.name));
@@ -690,7 +658,6 @@  mv64xxx_i2c_probe(struct platform_device *pd)
 		clk_unprepare(drv_data->clk);
 	}
 #endif
-		mv64xxx_i2c_unmap_regs(drv_data);
 	exit_kfree:
 		kfree(drv_data);
 	return rc;
@@ -704,7 +671,6 @@  mv64xxx_i2c_remove(struct platform_device *dev)
 
 	rc = i2c_del_adapter(&drv_data->adapter);
 	free_irq(drv_data->irq, drv_data);
-	mv64xxx_i2c_unmap_regs(drv_data);
 #if defined(CONFIG_HAVE_CLK)
 	/* Not all platforms have a clk */
 	if (!IS_ERR(drv_data->clk)) {