[PATCH] Input: ambakmi - Use managed interfaces

[PATCH] Input: ambakmi - Use managed interfaces

[Himangi Saraogi]

This patch introduces the use of managed interfaces like devm_clk_get,
devm_kalloc etc. devm_ioremap_resource is used instead of
amba_request_regions and devm_ioremap. It also does away with the functions
to free the allocated memory in probe and remove functions. Also, some
labels in the probe function are removed.

Signed-off-by: Himangi Saraogi <himangi774@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
 drivers/input/serio/ambakmi.c | 44 ++++++++++---------------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/drivers/input/serio/ambakmi.c b/drivers/input/serio/ambakmi.c
index 8b748d9..405a175 100644
--- a/drivers/input/serio/ambakmi.c
+++ b/drivers/input/serio/ambakmi.c
@@ -23,6 +23,7 @@
 #include <linux/clk.h>
 
 #include <asm/io.h>
+#include <linux/io.h>
 #include <asm/irq.h>
 
 #define KMI_BASE	(kmi->base)
@@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
 {
 	struct amba_kmi_port *kmi;
 	struct serio *io;
-	int ret;
-
-	ret = amba_request_regions(dev, NULL);
-	if (ret)
-		return ret;
-
-	kmi = kzalloc(sizeof(struct amba_kmi_port), GFP_KERNEL);
-	io = kzalloc(sizeof(struct serio), GFP_KERNEL);
-	if (!kmi || !io) {
-		ret = -ENOMEM;
-		goto out;
-	}
 
+	io = devm_kzalloc(&dev->dev, sizeof(struct serio), GFP_KERNEL);
+	if (!io)
+		return -ENOMEM;
 
 	io->id.type	= SERIO_8042;
 	io->write	= amba_kmi_write;
@@ -135,32 +127,20 @@ static int amba_kmi_probe(struct amba_device *dev,
 	io->port_data	= kmi;
 	io->dev.parent	= &dev->dev;
 
+	kmi->base	= devm_ioremap_resource(&dev->dev, &dev->res);
+	if (IS_ERR(kmi->base))
+		return PTR_ERR(kmi->base);
 	kmi->io		= io;
-	kmi->base	= ioremap(dev->res.start, resource_size(&dev->res));
-	if (!kmi->base) {
-		ret = -ENOMEM;
-		goto out;
-	}
 
-	kmi->clk = clk_get(&dev->dev, "KMIREFCLK");
-	if (IS_ERR(kmi->clk)) {
-		ret = PTR_ERR(kmi->clk);
-		goto unmap;
-	}
+	kmi->clk = devm_clk_get(&dev->dev, "KMIREFCLK");
+	if (IS_ERR(kmi->clk))
+		return PTR_ERR(kmi->clk);
 
 	kmi->irq = dev->irq[0];
 	amba_set_drvdata(dev, kmi);
 
 	serio_register_port(kmi->io);
 	return 0;
-
- unmap:
-	iounmap(kmi->base);
- out:
-	kfree(kmi);
-	kfree(io);
-	amba_release_regions(dev);
-	return ret;
 }
 
 static int amba_kmi_remove(struct amba_device *dev)
@@ -168,10 +148,6 @@ static int amba_kmi_remove(struct amba_device *dev)
 	struct amba_kmi_port *kmi = amba_get_drvdata(dev);
 
 	serio_unregister_port(kmi->io);
-	clk_put(kmi->clk);
-	iounmap(kmi->base);
-	kfree(kmi);
-	amba_release_regions(dev);
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH] Input: ambakmi - Use managed interfaces

[Russell King - ARM Linux]

On Sun, Jul 13, 2014 at 11:00:51PM +0530, Himangi Saraogi wrote:
> @@ -23,6 +23,7 @@
>  #include <linux/clk.h>
>  
>  #include <asm/io.h>
> +#include <linux/io.h>

NAK - please include either linux/io.h _or_ asm/io.h but not both.

> @@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
>  {
>  	struct amba_kmi_port *kmi;
>  	struct serio *io;
> -	int ret;
> -
> -	ret = amba_request_regions(dev, NULL);
> -	if (ret)
> -		return ret;

I'm /really/ not happy about that going.

> -
> -	kmi = kzalloc(sizeof(struct amba_kmi_port), GFP_KERNEL);
> -	io = kzalloc(sizeof(struct serio), GFP_KERNEL);
> -	if (!kmi || !io) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
>  
> +	io = devm_kzalloc(&dev->dev, sizeof(struct serio), GFP_KERNEL);
> +	if (!io)
> +		return -ENOMEM;
>  
>  	io->id.type	= SERIO_8042;
>  	io->write	= amba_kmi_write;
> @@ -135,32 +127,20 @@ static int amba_kmi_probe(struct amba_device *dev,
>  	io->port_data	= kmi;
>  	io->dev.parent	= &dev->dev;
>  
> +	kmi->base	= devm_ioremap_resource(&dev->dev, &dev->res);
> +	if (IS_ERR(kmi->base))
> +		return PTR_ERR(kmi->base);

NAK.  Look carefully through the above changes to work out why.  Please,
if you don't understand what you're doing, or don't even bother with a
build test of the code you've modified, don't even modify it in the
first place.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] Input: ambakmi - Use managed interfaces

[Julia Lawall]

On Sun, 13 Jul 2014, Russell King - ARM Linux wrote:

> On Sun, Jul 13, 2014 at 11:00:51PM +0530, Himangi Saraogi wrote:
> > @@ -23,6 +23,7 @@
> >  #include <linux/clk.h>
> >  
> >  #include <asm/io.h>
> > +#include <linux/io.h>
> 
> NAK - please include either linux/io.h _or_ asm/io.h but not both.
> 
> > @@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
> >  {
> >  	struct amba_kmi_port *kmi;
> >  	struct serio *io;
> > -	int ret;
> > -
> > -	ret = amba_request_regions(dev, NULL);
> > -	if (ret)
> > -		return ret;
> 
> I'm /really/ not happy about that going.

Could you explain why?  I looked at the code several times, and I couldn't 
see how it was different than request_mem_region, which is merged into 
devm_ioremap_resource.

thanks,
julia

> 
> > -
> > -	kmi = kzalloc(sizeof(struct amba_kmi_port), GFP_KERNEL);
> > -	io = kzalloc(sizeof(struct serio), GFP_KERNEL);
> > -	if (!kmi || !io) {
> > -		ret = -ENOMEM;
> > -		goto out;
> > -	}
> >  
> > +	io = devm_kzalloc(&dev->dev, sizeof(struct serio), GFP_KERNEL);
> > +	if (!io)
> > +		return -ENOMEM;
> >  
> >  	io->id.type	= SERIO_8042;
> >  	io->write	= amba_kmi_write;
> > @@ -135,32 +127,20 @@ static int amba_kmi_probe(struct amba_device *dev,
> >  	io->port_data	= kmi;
> >  	io->dev.parent	= &dev->dev;
> >  
> > +	kmi->base	= devm_ioremap_resource(&dev->dev, &dev->res);
> > +	if (IS_ERR(kmi->base))
> > +		return PTR_ERR(kmi->base);
> 
> NAK.  Look carefully through the above changes to work out why.  Please,
> if you don't understand what you're doing, or don't even bother with a
> build test of the code you've modified, don't even modify it in the
> first place.
> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.
> 

Re: [PATCH] Input: ambakmi - Use managed interfaces

[Russell King - ARM Linux]

On Sun, Jul 13, 2014 at 08:11:29PM +0200, Julia Lawall wrote:
> On Sun, 13 Jul 2014, Russell King - ARM Linux wrote:
> 
> > On Sun, Jul 13, 2014 at 11:00:51PM +0530, Himangi Saraogi wrote:
> > > @@ -23,6 +23,7 @@
> > >  #include <linux/clk.h>
> > >  
> > >  #include <asm/io.h>
> > > +#include <linux/io.h>
> > 
> > NAK - please include either linux/io.h _or_ asm/io.h but not both.
> > 
> > > @@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
> > >  {
> > >  	struct amba_kmi_port *kmi;
> > >  	struct serio *io;
> > > -	int ret;
> > > -
> > > -	ret = amba_request_regions(dev, NULL);
> > > -	if (ret)
> > > -		return ret;
> > 
> > I'm /really/ not happy about that going.
> 
> Could you explain why?  I looked at the code several times, and I couldn't 
> see how it was different than request_mem_region, which is merged into 
> devm_ioremap_resource.

Check what gets used for the name of the resource.

Now, consider that most devices when they are registered have their
resource names set to the device name.

Then realise that devm_ioremap_resource() uses the resource name or
the device name again.  So, what you end up with in /proc/iomem is
a load of stupidity - you don't get to see there which drivers are
making use of the resources, only a load of duplicated information
about what devices are using the regions.

This is contary to other bus types (like PCI) where the _device_ takes
the non-busy parent resource, and the driver takes the busy resource
using the _driver_ name, not the device name.  The exception to this
is network drivers which have in the past used the network device
name.

Here's an example.  x86:

  fc000000-fc01ffff : 0000:00:19.0	<--- device name
    fc000000-fc01ffff : e1000e		<--- driver name
  fc020000-fc023fff : 0000:00:1b.0	<--- device name
    fc020000-fc023fff : ICH HD audio	<--- driver name
  fc024000-fc024fff : 0000:00:03.3
  fc025000-fc025fff : 0000:00:19.0	<--- device name
    fc025000-fc025fff : e1000e		<--- driver name
  fc226000-fc2267ff : 0000:00:1f.2	<--- device name
    fc226000-fc2267ff : ahci		<--- driver name

etc.  When using devm_ioremap_resource() this ends up as:

02184000-021841ff : /soc/aips-bus@02100000/usb@02184000
  02184000-021841ff : /soc/aips-bus@02100000/usb@02184000
02184200-021843ff : /soc/aips-bus@02100000/usb@02184200
  02184200-021843ff : /soc/aips-bus@02100000/usb@02184200

which is really pointless duplicating the resource name like that.  It
conveys no additional useful information.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] Input: ambakmi - Use managed interfaces

[Julia Lawall]

On Sun, 13 Jul 2014, Russell King - ARM Linux wrote:

> On Sun, Jul 13, 2014 at 08:11:29PM +0200, Julia Lawall wrote:
> > On Sun, 13 Jul 2014, Russell King - ARM Linux wrote:
> > 
> > > On Sun, Jul 13, 2014 at 11:00:51PM +0530, Himangi Saraogi wrote:
> > > > @@ -23,6 +23,7 @@
> > > >  #include <linux/clk.h>
> > > >  
> > > >  #include <asm/io.h>
> > > > +#include <linux/io.h>
> > > 
> > > NAK - please include either linux/io.h _or_ asm/io.h but not both.
> > > 
> > > > @@ -112,19 +113,10 @@ static int amba_kmi_probe(struct amba_device *dev,
> > > >  {
> > > >  	struct amba_kmi_port *kmi;
> > > >  	struct serio *io;
> > > > -	int ret;
> > > > -
> > > > -	ret = amba_request_regions(dev, NULL);
> > > > -	if (ret)
> > > > -		return ret;
> > > 
> > > I'm /really/ not happy about that going.
> > 
> > Could you explain why?  I looked at the code several times, and I couldn't 
> > see how it was different than request_mem_region, which is merged into 
> > devm_ioremap_resource.
> 
> Check what gets used for the name of the resource.
> 
> Now, consider that most devices when they are registered have their
> resource names set to the device name.
> 
> Then realise that devm_ioremap_resource() uses the resource name or
> the device name again.  So, what you end up with in /proc/iomem is
> a load of stupidity - you don't get to see there which drivers are
> making use of the resources, only a load of duplicated information
> about what devices are using the regions.
> 
> This is contary to other bus types (like PCI) where the _device_ takes
> the non-busy parent resource, and the driver takes the busy resource
> using the _driver_ name, not the device name.  The exception to this
> is network drivers which have in the past used the network device
> name.
> 
> Here's an example.  x86:
> 
>   fc000000-fc01ffff : 0000:00:19.0	<--- device name
>     fc000000-fc01ffff : e1000e		<--- driver name
>   fc020000-fc023fff : 0000:00:1b.0	<--- device name
>     fc020000-fc023fff : ICH HD audio	<--- driver name
>   fc024000-fc024fff : 0000:00:03.3
>   fc025000-fc025fff : 0000:00:19.0	<--- device name
>     fc025000-fc025fff : e1000e		<--- driver name
>   fc226000-fc2267ff : 0000:00:1f.2	<--- device name
>     fc226000-fc2267ff : ahci		<--- driver name
> 
> etc.  When using devm_ioremap_resource() this ends up as:
> 
> 02184000-021841ff : /soc/aips-bus@02100000/usb@02184000
>   02184000-021841ff : /soc/aips-bus@02100000/usb@02184000
> 02184200-021843ff : /soc/aips-bus@02100000/usb@02184200
>   02184200-021843ff : /soc/aips-bus@02100000/usb@02184200
> 
> which is really pointless duplicating the resource name like that.  It
> conveys no additional useful information.

OK, thanks very much.

julia