| Message ID | 1603441103-17735-1-git-send-email-peng.fan@nxp.com |
|---|---|
| State | Needs Review / ACK |
| Headers | show |
| Series | [1/2] i2c: imx: use devm_request_threaded_irq to simplify code | expand |
On Fri, 23 Oct 2020 at 10:27, <peng.fan@nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > Use devm_request_threaded_irq to simplify code > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/i2c/busses/i2c-imx.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index e6f8d6e45a15..ba9d639223ec 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -1216,8 +1216,8 @@ static int i2c_imx_probe(struct platform_device *pdev) > goto rpm_disable; > > /* Request IRQ */ > - ret = request_threaded_irq(irq, i2c_imx_isr, NULL, IRQF_SHARED, > - pdev->name, i2c_imx); > + ret = devm_request_threaded_irq(&pdev->dev, irq, i2c_imx_isr, NULL, IRQF_SHARED, > + pdev->name, i2c_imx); Really? You silently revert commit e50e4f0b85be ("i2c: imx: Fix external abort on interrupt in exit paths"). This is not a simplification but serious change. NAK. At least without proper reasoning of why this is suddenly safe. Best regards, Krzysztof Best regards, Krzysztof
> Subject: Re: [PATCH 1/2] i2c: imx: use devm_request_threaded_irq to simplify > code > > On Fri, 23 Oct 2020 at 10:27, <peng.fan@nxp.com> wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > Use devm_request_threaded_irq to simplify code > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/i2c/busses/i2c-imx.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx.c > > b/drivers/i2c/busses/i2c-imx.c index e6f8d6e45a15..ba9d639223ec 100644 > > --- a/drivers/i2c/busses/i2c-imx.c > > +++ b/drivers/i2c/busses/i2c-imx.c > > @@ -1216,8 +1216,8 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > goto rpm_disable; > > > > /* Request IRQ */ > > - ret = request_threaded_irq(irq, i2c_imx_isr, NULL, IRQF_SHARED, > > - pdev->name, i2c_imx); > > + ret = devm_request_threaded_irq(&pdev->dev, irq, i2c_imx_isr, > NULL, IRQF_SHARED, > > + pdev->name, i2c_imx); > > Really? You silently revert commit e50e4f0b85be ("i2c: imx: Fix external abort > on interrupt in exit paths"). This is not a simplification but serious change. NAK. > At least without proper reasoning of why this is suddenly safe. Oh, I need look at git history before. But Is it because i2c interrupt enabled too early? I'll try your case on i.MX8M platform. Regards, Peng. > > Best regards, > Krzysztof > Best regards, > Krzysztof
On Sat, Oct 24, 2020 at 07:39:47AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH 1/2] i2c: imx: use devm_request_threaded_irq to simplify > > code > > > > On Fri, 23 Oct 2020 at 10:27, <peng.fan@nxp.com> wrote: > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > Use devm_request_threaded_irq to simplify code > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > drivers/i2c/busses/i2c-imx.c | 10 +++------- > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-imx.c > > > b/drivers/i2c/busses/i2c-imx.c index e6f8d6e45a15..ba9d639223ec 100644 > > > --- a/drivers/i2c/busses/i2c-imx.c > > > +++ b/drivers/i2c/busses/i2c-imx.c > > > @@ -1216,8 +1216,8 @@ static int i2c_imx_probe(struct platform_device > > *pdev) > > > goto rpm_disable; > > > > > > /* Request IRQ */ > > > - ret = request_threaded_irq(irq, i2c_imx_isr, NULL, IRQF_SHARED, > > > - pdev->name, i2c_imx); > > > + ret = devm_request_threaded_irq(&pdev->dev, irq, i2c_imx_isr, > > NULL, IRQF_SHARED, > > > + pdev->name, i2c_imx); > > > > Really? You silently revert commit e50e4f0b85be ("i2c: imx: Fix external abort > > on interrupt in exit paths"). This is not a simplification but serious change. NAK. > > At least without proper reasoning of why this is suddenly safe. > > Oh, I need look at git history before. But > Is it because i2c interrupt enabled too early? I'll try your case on i.MX8M platform. Shortly, it is because shared interrupts do not go well or at all with devm-managed interrupt handlers. Probably they could be made working with additional code, e.g. devm_free_irq() call or some additional checks in interrupt handlers. But in that case there won't be much of simplification. Best regards, Krzysztof
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index e6f8d6e45a15..ba9d639223ec 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -1216,8 +1216,8 @@ static int i2c_imx_probe(struct platform_device *pdev) goto rpm_disable; /* Request IRQ */ - ret = request_threaded_irq(irq, i2c_imx_isr, NULL, IRQF_SHARED, - pdev->name, i2c_imx); + ret = devm_request_threaded_irq(&pdev->dev, irq, i2c_imx_isr, NULL, IRQF_SHARED, + pdev->name, i2c_imx); if (ret) { dev_err(&pdev->dev, "can't claim irq %d\n", irq); goto rpm_disable; @@ -1265,7 +1265,6 @@ static int i2c_imx_probe(struct platform_device *pdev) clk_notifier_unregister: clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb); - free_irq(irq, i2c_imx); rpm_disable: pm_runtime_put_noidle(&pdev->dev); pm_runtime_disable(&pdev->dev); @@ -1278,7 +1277,7 @@ static int i2c_imx_probe(struct platform_device *pdev) static int i2c_imx_remove(struct platform_device *pdev) { struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev); - int irq, ret; + int ret; ret = pm_runtime_get_sync(&pdev->dev); if (ret < 0) @@ -1298,9 +1297,6 @@ static int i2c_imx_remove(struct platform_device *pdev) imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR); clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb); - irq = platform_get_irq(pdev, 0); - if (irq >= 0) - free_irq(irq, i2c_imx); clk_disable_unprepare(i2c_imx->clk); pm_runtime_put_noidle(&pdev->dev);