[PATCH] i2c: qcom-geni: Keep comment why interrupts start disabled

[PATCH] i2c: qcom-geni: Keep comment why interrupts start disabled

[Wolfram Sang]

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 */
 	ret = devm_request_irq(dev, gi2c->irq, geni_i2c_irq, IRQF_NO_AUTOEN,
 			       dev_name(dev), gi2c);
 	if (ret) {
-- 
2.45.2

Re: [PATCH] i2c: qcom-geni: Keep comment why interrupts start disabled

[Andi Shyti]

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
> 

Re: [PATCH] i2c: qcom-geni: Keep comment why interrupts start disabled

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]


> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: qcom-geni: Keep comment why interrupts start disabled

[Andi Shyti]

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

Re: [PATCH] i2c: qcom-geni: Keep comment why interrupts start disabled

[Bjorn Andersson]

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
> 
>