Message ID | 83ec9558211389896f21d9682e9824cd7979466c.1380550490.git.michal.simek@xilinx.com |
---|---|
State | Superseded |
Headers | show |
> + 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?
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
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
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
> > 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 --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; }