diff mbox

[v2,3/3] i2c: xilinx: Use devm_* functions

Message ID 83ec9558211389896f21d9682e9824cd7979466c.1380550490.git.michal.simek@xilinx.com
State Superseded
Headers show

Commit Message

Michal Simek Sept. 30, 2013, 2:15 p.m. UTC 
From: Kedareswara rao Appana <appana.durga.rao@xilinx.com>

Simplified the probe and remove functions using devm_* functions

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2: None

 drivers/i2c/busses/i2c-xiic.c | 69 +++++++++++--------------------------------
 1 file changed, 18 insertions(+), 51 deletions(-)

--
1.8.2.3

Comments

Wolfram Sang Oct. 4, 2013, 5:33 a.m. UTC | #1 
> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(i2c->base)) {
> +		dev_err(&pdev->dev, "Could not allocate iomem\n");

devm_ioremap_resource already prints error messages.

> +	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);

This is too early. Can you find out why?

> +	pdata = (struct xiic_i2c_platform_data *)dev_get_platdata(&pdev->dev);

Casting a void pointer?
Michal Simek Oct. 4, 2013, 9:16 a.m. UTC | #2 
On 10/04/2013 07:33 AM, Wolfram Sang wrote:
> 
>> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(i2c->base)) {
>> +		dev_err(&pdev->dev, "Could not allocate iomem\n");
> 
> devm_ioremap_resource already prints error messages.

you are right.

> 
>> +	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);
> 
> This is too early. Can you find out why?

Why do you think that it is too early?
I am looking at origin code again and I think that the code
is also problematic because in xiic_reinit() interrupts are enabled
but they are requested later.
Shouldn't be there a logic that interrupts should be enabled when
interrupts are registered by the kernel?

>> +	pdata = (struct xiic_i2c_platform_data *)dev_get_platdata(&pdev->dev);
> 
> Casting a void pointer?

No problem with that.

Thanks,
Michal
Wolfram Sang Oct. 4, 2013, 11:58 a.m. UTC | #3 
On Fri, Oct 04, 2013 at 11:16:20AM +0200, Michal Simek wrote:
> On 10/04/2013 07:33 AM, Wolfram Sang wrote:
> > 
> >> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
> >> +	if (IS_ERR(i2c->base)) {
> >> +		dev_err(&pdev->dev, "Could not allocate iomem\n");
> > 
> > devm_ioremap_resource already prints error messages.
> 
> you are right.
> 
> > 
> >> +	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);
> > 
> > This is too early. Can you find out why?
> 
> Why do you think that it is too early?

The ISR uses spinlocks which are not initialized by then.

> I am looking at origin code again and I think that the code
> is also problematic because in xiic_reinit() interrupts are enabled
> but they are requested later.
> Shouldn't be there a logic that interrupts should be enabled when
> interrupts are registered by the kernel?

First register the handler, then activate interrupts. You are right,
this needs to be fixed, too.

Regards,

   Wolfram
Michal Simek Oct. 4, 2013, 12:04 p.m. UTC | #4 
On 10/04/2013 01:58 PM, Wolfram Sang wrote:
> On Fri, Oct 04, 2013 at 11:16:20AM +0200, Michal Simek wrote:
>> On 10/04/2013 07:33 AM, Wolfram Sang wrote:
>>>
>>>> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);
>>>> +	if (IS_ERR(i2c->base)) {
>>>> +		dev_err(&pdev->dev, "Could not allocate iomem\n");
>>>
>>> devm_ioremap_resource already prints error messages.
>>
>> you are right.
>>
>>>
>>>> +	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);
>>>
>>> This is too early. Can you find out why?
>>
>> Why do you think that it is too early?
> 
> The ISR uses spinlocks which are not initialized by then.

And waitqueue too.  Ok I will keep that request irq in current location
and I will add xiic_reinit() below this code.

>> I am looking at origin code again and I think that the code
>> is also problematic because in xiic_reinit() interrupts are enabled
>> but they are requested later.
>> Shouldn't be there a logic that interrupts should be enabled when
>> interrupts are registered by the kernel?
> 
> First register the handler, then activate interrupts. You are right,
> this needs to be fixed, too.

Do you want me to create separate patch just about moving request irq in front of
xiic_reinit()? And then devm_ conversion?

Thanks,
Michal
Wolfram Sang Oct. 5, 2013, 5:59 a.m. UTC | #5 
> > First register the handler, then activate interrupts. You are right,
> > this needs to be fixed, too.
> 
> Do you want me to create separate patch just about moving request irq in front of
> xiic_reinit()? And then devm_ conversion?

Yes, seperate, please.
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 44e6ae7..6dd3212 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -32,6 +32,7 @@ 
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/errno.h>
+#include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/i2c.h>
@@ -698,32 +699,28 @@  static int xiic_i2c_probe(struct platform_device *pdev)
 	int ret, irq;
 	u8 i;

+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		goto resource_missing;
+	i2c->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(i2c->base)) {
+		dev_err(&pdev->dev, "Could not allocate iomem\n");
+		return PTR_ERR(i2c->base);
+	}

 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
-		goto resource_missing;
-
-	pdata = (struct xiic_i2c_platform_data *)dev_get_platdata(&pdev->dev);
+		return irq;

-	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
-	if (!i2c)
-		return -ENOMEM;
-
-	if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
-		dev_err(&pdev->dev, "Memory region busy\n");
-		ret = -EBUSY;
-		goto request_mem_failed;
+	ret = devm_request_irq(&pdev->dev, irq, xiic_isr, 0, pdev->name, i2c);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Cannot claim IRQ\n");
+		return ret;
 	}

-	i2c->base = ioremap(res->start, resource_size(res));
-	if (!i2c->base) {
-		dev_err(&pdev->dev, "Unable to map registers\n");
-		ret = -EIO;
-		goto map_failed;
-	}
+	pdata = (struct xiic_i2c_platform_data *)dev_get_platdata(&pdev->dev);

 	/* hook up driver to tree */
 	platform_set_drvdata(pdev, i2c);
@@ -736,17 +733,13 @@  static int xiic_i2c_probe(struct platform_device *pdev)

 	spin_lock_init(&i2c->lock);
 	init_waitqueue_head(&i2c->wait);
-	ret = request_irq(irq, xiic_isr, 0, pdev->name, i2c);
-	if (ret) {
-		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		goto request_irq_failed;
-	}

 	/* add i2c adapter to i2c tree */
 	ret = i2c_add_adapter(&i2c->adap);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add adapter\n");
-		goto add_adapter_failed;
+		xiic_deinit(i2c);
+		return ret;
 	}

 	if (pdata) {
@@ -756,43 +749,17 @@  static int xiic_i2c_probe(struct platform_device *pdev)
 	}

 	return 0;
-
-add_adapter_failed:
-	free_irq(irq, i2c);
-request_irq_failed:
-	xiic_deinit(i2c);
-	iounmap(i2c->base);
-map_failed:
-	release_mem_region(res->start, resource_size(res));
-request_mem_failed:
-	kfree(i2c);
-
-	return ret;
-resource_missing:
-	dev_err(&pdev->dev, "IRQ or Memory resource is missing\n");
-	return -ENOENT;
 }

 static int xiic_i2c_remove(struct platform_device *pdev)
 {
 	struct xiic_i2c *i2c = platform_get_drvdata(pdev);
-	struct resource *res;

 	/* remove adapter & data */
 	i2c_del_adapter(&i2c->adap);

 	xiic_deinit(i2c);

-	free_irq(platform_get_irq(pdev, 0), i2c);
-
-	iounmap(i2c->base);
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res)
-		release_mem_region(res->start, resource_size(res));
-
-	kfree(i2c);
-
 	return 0;
 }