| Message ID | 20240916121516.3102-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: qcom-geni: Keep comment why interrupts start disabled | expand |
Hi Wolfram, On Mon, Sep 16, 2024 at 02:15:17PM GMT, Wolfram Sang wrote: > The to-be-fixed commit rightfully reduced a race window, but also > removed a comment which is still helpful after the fix. Bring the > comment back. > > Fixes: e2c85d85a05f ("i2c: qcom-geni: Use IRQF_NO_AUTOEN flag in request_irq()") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 4c9050a4d58e..03c05dcc2202 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -818,6 +818,8 @@ static int geni_i2c_probe(struct platform_device *pdev) > init_completion(&gi2c->done); > spin_lock_init(&gi2c->lock); > platform_set_drvdata(pdev, gi2c); > + > + /* Disable the interrupt so that the system can enter low-power mode */ Thanks for the patch! However, I wouldn’t typically consider this a fix, and I don’t think it would qualify for stable release inclusion. That said, I agree that a comment should be added back. The original comment no longer fits as well as it did before. A more appropriate comment would be: /* * Do not enable interrupts by default to allow the system to enter * low-power mode. The driver will explicitly enable interrupts when * needed using enable_irq(). */ Does it make sense? Thanks, Andi PS If you want I can add it to the current i2c-host-fixes and retrigger the pull request. > ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN, > dev_name(dev), gi2c); > if (ret) { > -- > 2.45.2 >
> Thanks for the patch! However, I wouldn’t typically consider this > a fix, and I don’t think it would qualify for stable release > inclusion. Okay, I agree. > That said, I agree that a comment should be added back. The original > comment no longer fits as well as it did before. A more > appropriate comment would be: > > /* > * Do not enable interrupts by default to allow the system to enter > * low-power mode. The driver will explicitly enable interrupts when > * needed using enable_irq(). > */ > > Does it make sense? Frankly, I think this is too detailed, we can't have kernel driver 101 in each and every driver. But I will happily stand back if you insist because we are entering a bike-shedding area. My proposal would be: /* Keep interrupts disabled initially to allow for low-power modes */ Chose what you prefer! Happy hacking, Wolfram
Hi Wolfram, ... > > That said, I agree that a comment should be added back. The original > > comment no longer fits as well as it did before. A more > > appropriate comment would be: > > > > /* > > * Do not enable interrupts by default to allow the system to enter > > * low-power mode. The driver will explicitly enable interrupts when > > * needed using enable_irq(). > > */ > > > > Does it make sense? > > Frankly, I think this is too detailed, we can't have kernel driver 101 > in each and every driver. But I will happily stand back if you insist > because we are entering a bike-shedding area. My proposal would be: > > /* Keep interrupts disabled initially to allow for low-power modes */ Ack! > Chose what you prefer! > > Happy hacking, Thanks, Andi > Wolfram
On Mon, Sep 16, 2024 at 02:15:17PM GMT, Wolfram Sang wrote: > The to-be-fixed commit rightfully reduced a race window, but also > removed a comment which is still helpful after the fix. Bring the > comment back. > > Fixes: e2c85d85a05f ("i2c: qcom-geni: Use IRQF_NO_AUTOEN flag in request_irq()") > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index 4c9050a4d58e..03c05dcc2202 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -818,6 +818,8 @@ static int geni_i2c_probe(struct platform_device *pdev) > init_completion(&gi2c->done); > spin_lock_init(&gi2c->lock); > platform_set_drvdata(pdev, gi2c); > + > + /* Disable the interrupt so that the system can enter low-power mode */ I'm uncertain about the correctness of this comment. Seems more likely that we're concerns about lingering interrupts from operation of the bus during boot. Perhaps I'm missing something obvious, or perhaps there's a need for reviewing the code written here under the documented premise? I think there's value in keeping the comment in the code, and then try to update it with a more detailed description. Reviewed-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn > ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN, > dev_name(dev), gi2c); > if (ret) { > -- > 2.45.2 > >
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 4c9050a4d58e..03c05dcc2202 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -818,6 +818,8 @@ static int geni_i2c_probe(struct platform_device *pdev) init_completion(&gi2c->done); spin_lock_init(&gi2c->lock); platform_set_drvdata(pdev, gi2c); + + /* Disable the interrupt so that the system can enter low-power mode */ ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN, dev_name(dev), gi2c); if (ret) {
The to-be-fixed commit rightfully reduced a race window, but also removed a comment which is still helpful after the fix. Bring the comment back. Fixes: e2c85d85a05f ("i2c: qcom-geni: Use IRQF_NO_AUTOEN flag in request_irq()") Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-qcom-geni.c | 2 ++ 1 file changed, 2 insertions(+)