i2c: qcom-geni: Keep comment why interrupts start disabled

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(+)

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
>

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