Use the devm_* for the memory region allocation, interrupt request,
clock handler request.
By doing this, it does not require to explicitly free it and hence
saving some code.
Signed-off-by: Laxman Dewangan <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 64 ++++++++++-----------------------------
1 files changed, 17 insertions(+), 47 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 18067b3..390d379 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -106,7 +106,6 @@
* @adapter: core i2c layer adapter information
* @clk: clock reference for i2c controller
* @i2c_clk: clock reference for i2c bus
- * @iomem: memory resource for registers
* @base: ioremapped registers cookie
* @cont_id: i2c controller id, used for for packet header
* @irq: irq number of transfer complete interrupt
@@ -124,7 +123,6 @@ struct tegra_i2c_dev {
struct i2c_adapter adapter;
struct clk *clk;
struct clk *i2c_clk;
- struct resource *iomem;
void __iomem *base;
int cont_id;
int irq;
@@ -573,7 +571,6 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
struct tegra_i2c_dev *i2c_dev;
struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
struct resource *res;
- struct resource *iomem;
struct clk *clk;
struct clk *i2c_clk;
const unsigned int *prop;
@@ -586,50 +583,41 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "no mem resource\n");
return -EINVAL;
}
- iomem = request_mem_region(res->start, resource_size(res), pdev->name);
- if (!iomem) {
- dev_err(&pdev->dev, "I2C region already claimed\n");
- return -EBUSY;
- }
- base = ioremap(iomem->start, resource_size(iomem));
+ base = devm_request_and_ioremap(&pdev->dev, res);
if (!base) {
- dev_err(&pdev->dev, "Cannot ioremap I2C region\n");
- return -ENOMEM;
+ dev_err(&pdev->dev, "Cannot request/ioremap I2C registers\n");
+ return -EADDRNOTAVAIL;
}
res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (!res) {
dev_err(&pdev->dev, "no irq resource\n");
- ret = -EINVAL;
- goto err_iounmap;
+ return -EINVAL;
}
irq = res->start;
- clk = clk_get(&pdev->dev, NULL);
+ clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(clk)) {
dev_err(&pdev->dev, "missing controller clock");
- ret = PTR_ERR(clk);
- goto err_release_region;
+ return PTR_ERR(clk);
}
- i2c_clk = clk_get(&pdev->dev, "i2c");
+ i2c_clk = devm_clk_get(&pdev->dev, "i2c");
if (IS_ERR(i2c_clk)) {
dev_err(&pdev->dev, "missing bus clock");
- ret = PTR_ERR(i2c_clk);
- goto err_clk_put;
+ return PTR_ERR(i2c_clk);
}
- i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
+ i2c_dev = devm_kzalloc(&pdev->dev, sizeof(*i2c_dev), GFP_KERNEL);
if (!i2c_dev) {
- ret = -ENOMEM;
- goto err_i2c_clk_put;
+ dev_err(&pdev->dev, "Could not allocate struct tegra_i2c_dev");
+ return -ENOMEM;
}
i2c_dev->base = base;
i2c_dev->clk = clk;
i2c_dev->i2c_clk = i2c_clk;
- i2c_dev->iomem = iomem;
i2c_dev->adapter.algo = &tegra_i2c_algo;
i2c_dev->irq = irq;
i2c_dev->cont_id = pdev->id;
@@ -658,13 +646,14 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
ret = tegra_i2c_init(i2c_dev);
if (ret) {
dev_err(&pdev->dev, "Failed to initialize i2c controller");
- goto err_free;
+ return ret;
}
- ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
+ ret = devm_request_irq(&pdev->dev, i2c_dev->irq,
+ tegra_i2c_isr, 0, pdev->name, i2c_dev);
if (ret) {
dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
- goto err_free;
+ return ret;
}
clk_enable(i2c_dev->i2c_clk);
@@ -682,38 +671,19 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
if (ret) {
dev_err(&pdev->dev, "Failed to add I2C adapter\n");
- goto err_free_irq;
+ clk_disable(i2c_dev->i2c_clk);
+ return ret;
}
of_i2c_register_devices(&i2c_dev->adapter);
return 0;
-err_free_irq:
- free_irq(i2c_dev->irq, i2c_dev);
-err_free:
- kfree(i2c_dev);
-err_i2c_clk_put:
- clk_put(i2c_clk);
-err_clk_put:
- clk_put(clk);
-err_release_region:
- release_mem_region(iomem->start, resource_size(iomem));
-err_iounmap:
- iounmap(base);
- return ret;
}
static int __devexit tegra_i2c_remove(struct platform_device *pdev)
{
struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
i2c_del_adapter(&i2c_dev->adapter);
- free_irq(i2c_dev->irq, i2c_dev);
- clk_put(i2c_dev->i2c_clk);
- clk_put(i2c_dev->clk);
- release_mem_region(i2c_dev->iomem->start,
- resource_size(i2c_dev->iomem));
- iounmap(i2c_dev->base);
- kfree(i2c_dev);
return 0;
}
--
1.7.1.1
The notification of the transfer complete by calling complete()
should be done after clearing all interrupt status.
This may avoid the race condition of misconfigure the i2c controller
in multi-core environment.
Signed-off-by: Laxman Dewangan <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 390d379..16820a5 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -399,8 +399,6 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
disable_irq_nosync(i2c_dev->irq);
i2c_dev->irq_disabled = 1;
}
-
- complete(&i2c_dev->msg_complete);
goto err;
}
@@ -409,7 +407,6 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
i2c_dev->msg_err |= I2C_ERR_NO_ACK;
if (status & I2C_INT_ARBITRATION_LOST)
i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
- complete(&i2c_dev->msg_complete);
goto err;
}
@@ -427,14 +424,14 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
}
+ i2c_writel(i2c_dev, status, I2C_INT_STATUS);
+ if (i2c_dev->is_dvc)
+ dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
+
if (status & I2C_INT_PACKET_XFER_COMPLETE) {
BUG_ON(i2c_dev->msg_buf_remaining);
complete(&i2c_dev->msg_complete);
}
-
- i2c_writel(i2c_dev, status, I2C_INT_STATUS);
- if (i2c_dev->is_dvc)
- dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
return IRQ_HANDLED;
err:
/* An error occurred, mask all interrupts */
@@ -444,6 +441,8 @@ err:
i2c_writel(i2c_dev, status, I2C_INT_STATUS);
if (i2c_dev->is_dvc)
dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
+
+ complete(&i2c_dev->msg_complete);
return IRQ_HANDLED;
}
--
1.7.1.1
Following two changes:
1. Using devm_* for all allocation.
This will reduce the code size for freeing resources.
2. Notifying transfer complete after clearing the interrupt status.
This is to avoid the misconfiguration of i2c register in multi-core
environment.
Laxman Dewangan (2):
i2c: tegra: make all resource allocation through devm_*
i2c: tegra: notify transfer-complete after clearing status.
drivers/i2c/busses/i2c-tegra.c | 79 +++++++++++++---------------------------
1 files changed, 25 insertions(+), 54 deletions(-)
On 05/07/2012 12:46 AM, Laxman Dewangan wrote:
> Use the devm_* for the memory region allocation, interrupt request,
> clock handler request.
> By doing this, it does not require to explicitly free it and hence
> saving some code.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
Acked-by: Stephen Warren <[email protected]>
Note thought that devm_clk_get() appears to have been introduced very
recently in linux-next, so in order to merge this patch one of the
following has to happen:
a) This patch goes through the tree that merged the addition of
devm_clk_get() (which I think is Russell King's ARM tree)
b) Whichever tree takes this patch must first merge a branch that
contains that patch that adds devm_clk_get().
On 05/07/2012 12:46 AM, Laxman Dewangan wrote:
> The notification of the transfer complete by calling complete()
> should be done after clearing all interrupt status.
> This may avoid the race condition of misconfigure the i2c controller
> in multi-core environment.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
Acked-by: Stephen Warren <[email protected]>
This patch appears to be independent from the first patch in the series,
so I assume can be checked in immediately.
On Mon, May 07, 2012 at 12:16:19PM +0530, Laxman Dewangan wrote:
> The notification of the transfer complete by calling complete()
> should be done after clearing all interrupt status.
> This may avoid the race condition of misconfigure the i2c controller
> in multi-core environment.
"may avoid" or "does avoid"? I.e. does it fix any bug that really
occured? Then I'd add a stable tag.
So far, applied to next.
> if (status & I2C_INT_PACKET_XFER_COMPLETE) {
> BUG_ON(i2c_dev->msg_buf_remaining);
> complete(&i2c_dev->msg_complete);
> }
Side note: If you see a way to handle the BUG invocations more
gracefully, that would be appreciated.
Thanks,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Saturday 12 May 2012 06:37 PM, Wolfram Sang wrote:
> * PGP Signed by an unknown key
>
> On Mon, May 07, 2012 at 12:16:19PM +0530, Laxman Dewangan wrote:
>> The notification of the transfer complete by calling complete()
>> should be done after clearing all interrupt status.
>> This may avoid the race condition of misconfigure the i2c controller
>> in multi-core environment.
> "may avoid" or "does avoid"? I.e. does it fix any bug that really
> occured? Then I'd add a stable tag.
>
> So far, applied to next.
"does avoid".
We had some of stability issue in long hour stress test with i2c and
this is one the fix towards the stability issue. Most of fixes are
already there in the mainline.
It is fine to stable tag on this change.
>> if (status& I2C_INT_PACKET_XFER_COMPLETE) {
>> BUG_ON(i2c_dev->msg_buf_remaining);
>> complete(&i2c_dev->msg_complete);
>> }
> Side note: If you see a way to handle the BUG invocations more
> gracefully, that would be appreciated.
>
Ok, I will work towards that.
Thanks,
Laxman
> It is fine to stable tag on this change.
Done.
> >Side note: If you see a way to handle the BUG invocations more
> >gracefully, that would be appreciated.
> >
> Ok, I will work towards that.
Thanks!
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, May 07, 2012 at 10:18:45AM -0600, Stephen Warren wrote:
> On 05/07/2012 12:46 AM, Laxman Dewangan wrote:
> > Use the devm_* for the memory region allocation, interrupt request,
> > clock handler request.
> > By doing this, it does not require to explicitly free it and hence
> > saving some code.
> >
> > Signed-off-by: Laxman Dewangan <[email protected]>
>
> Acked-by: Stephen Warren <[email protected]>
>
> Note thought that devm_clk_get() appears to have been introduced very
> recently in linux-next, so in order to merge this patch one of the
> following has to happen:
>
> a) This patch goes through the tree that merged the addition of
> devm_clk_get() (which I think is Russell King's ARM tree)
I think it would be way more fitting if those go in via I2C.
> b) Whichever tree takes this patch must first merge a branch that
> contains that patch that adds devm_clk_get().
I have been pointed recently to subtle bugs when converting to devm_*.
Even more, there is still the ongoing idea of platform_devm_* to
simplify further. I need to scratch my head about those in general, but
I won't do this before my 10 day vacation starting tomorrow. Until I am
done with that, the devm_get_clock is probably in mainline. While devm_*
patches are nice, they are not really critical. So, I prefer to take
some extra time to get it right.
Thanks,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |