[7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than request and disable

Commit Message

Jonathan Cameron April 2, 2021, 6:45 p.m. UTC

From: Jonathan Cameron <[e-mail]>

These devices are not able to mask the signal used as a data ready
interrupt.  As such they previously requested the irq then immediately
disabled it.  Now we can avoid the potential of a spurious interrupt
by avoiding the irq being auto enabled in the first place.

I'm not sure how this code could have been called with the irq already
disabled, so I believe the conditional would always have been true and
have removed it.

Signed-off-by: Jonathan Cameron <[e-mail]>
Cc: Lars-Peter Clausen <[e-mail]>
Cc: Alexandru Ardelean <[e-mail]>
---
 drivers/iio/adc/ad_sigma_delta.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Song Bao Hua (Barry Song) April 2, 2021, 8:30 p.m. UTC | #1

> -----Original Message-----
> From: Jonathan Cameron [mailto:[e-mail]]
> Sent: Saturday, April 3, 2021 7:46 AM
> To: [e-mail]
> Cc: Song Bao Hua (Barry Song) <[e-mail]>; Jonathan Cameron
> <[e-mail]>; Lars-Peter Clausen <[e-mail]>;
> Alexandru Ardelean <[e-mail]>
> Subject: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than
> request and disable
> 
> From: Jonathan Cameron <[e-mail]>
> 
> These devices are not able to mask the signal used as a data ready
> interrupt.  As such they previously requested the irq then immediately
> disabled it.  Now we can avoid the potential of a spurious interrupt
> by avoiding the irq being auto enabled in the first place.
> 
> I'm not sure how this code could have been called with the irq already
> disabled, so I believe the conditional would always have been true and
> have removed it.
> 

If irq_dis is true, it seems the original code assumed interrupt was
open. Now the code is moving to disable-irq for true irq_dis. For
false irq_dis, this patch has no side effect so looks correct.

A safer way might be as below?

if(!sigma_delta->irq_dis)
	irq_flags = sigma_delta->info->irq_flags | IRQF_NO_AUTOEN;

request_irq(irq_flags);

sigma_delta->irq_dis =  true;

> Signed-off-by: Jonathan Cameron <[e-mail]>
> Cc: Lars-Peter Clausen <[e-mail]>
> Cc: Alexandru Ardelean <[e-mail]>
> ---
>  drivers/iio/adc/ad_sigma_delta.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c
> b/drivers/iio/adc/ad_sigma_delta.c
> index 9289812c0a94..e777ec718973 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -485,18 +485,15 @@ static int ad_sd_probe_trigger(struct iio_dev *indio_dev)
>  	sigma_delta->trig->ops = &ad_sd_trigger_ops;
>  	init_completion(&sigma_delta->completion);
> 
> +	sigma_delta->irq_dis = true;
>  	ret = request_irq(sigma_delta->spi->irq,
>  			  ad_sd_data_rdy_trig_poll,
> -			  sigma_delta->info->irq_flags,
> +			  sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
>  			  indio_dev->name,
>  			  sigma_delta);
>  	if (ret)
>  		goto error_free_trig;
> 
> -	if (!sigma_delta->irq_dis) {
> -		sigma_delta->irq_dis = true;
> -		disable_irq_nosync(sigma_delta->spi->irq);
> -	}
>  	iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
> 
>  	ret = iio_trigger_register(sigma_delta->trig);
> --
> 2.31.1

Thanks
Barry

Jonathan Cameron April 5, 2021, 2:51 p.m. UTC | #2

On Fri, 2 Apr 2021 20:30:17 +0000
"Song Bao Hua (Barry Song)" <[e-mail]> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron [mailto:[e-mail]]
> > Sent: Saturday, April 3, 2021 7:46 AM
> > To: [e-mail]
> > Cc: Song Bao Hua (Barry Song) <[e-mail]>; Jonathan Cameron
> > <[e-mail]>; Lars-Peter Clausen <[e-mail]>;
> > Alexandru Ardelean <[e-mail]>
> > Subject: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than
> > request and disable
> > 
> > From: Jonathan Cameron <[e-mail]>
> > 
> > These devices are not able to mask the signal used as a data ready
> > interrupt.  As such they previously requested the irq then immediately
> > disabled it.  Now we can avoid the potential of a spurious interrupt
> > by avoiding the irq being auto enabled in the first place.
> > 
> > I'm not sure how this code could have been called with the irq already
> > disabled, so I believe the conditional would always have been true and
> > have removed it.
> >   
> 
> If irq_dis is true, it seems the original code assumed interrupt was
> open. Now the code is moving to disable-irq for true irq_dis. For
> false irq_dis, this patch has no side effect so looks correct.
Hi Barry,

As far as I can tell (and I looked closely :), at the point where
ad_sd_probe_trigger() can be called, irq_dis is always false,
so the original check is misleading by implying
otherwise.

Taken in isolation of what is visible in this patch I'd agree
the change you suggest makes sense, but I'd also like to clean
up the wrong impression the original check was giving.  It is
also worth noting that ad_sigma_delta.h lists irq_dis as a
private member of the structure that should only be used in the
library (reducing chances of any drivers in future doing something
odd with it).

The other checks on irq_dis look suspicious as well, but removing
those would rather spread the scope of this patch.

As Lars has given an Reviewed-by and this is his code, I'm going
to press ahead with this as it stands.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to see if we missed anything.

Thanks,

Jonathan

> 
> A safer way might be as below?
> 
> if(!sigma_delta->irq_dis)
> 	irq_flags = sigma_delta->info->irq_flags | IRQF_NO_AUTOEN;
> 
> request_irq(irq_flags);
> 
> sigma_delta->irq_dis =  true;
> 
> > Signed-off-by: Jonathan Cameron <[e-mail]>
> > Cc: Lars-Peter Clausen <[e-mail]>
> > Cc: Alexandru Ardelean <[e-mail]>
> > ---
> >  drivers/iio/adc/ad_sigma_delta.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > b/drivers/iio/adc/ad_sigma_delta.c
> > index 9289812c0a94..e777ec718973 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -485,18 +485,15 @@ static int ad_sd_probe_trigger(struct iio_dev *indio_dev)
> >  	sigma_delta->trig->ops = &ad_sd_trigger_ops;
> >  	init_completion(&sigma_delta->completion);
> > 
> > +	sigma_delta->irq_dis = true;
> >  	ret = request_irq(sigma_delta->spi->irq,
> >  			  ad_sd_data_rdy_trig_poll,
> > -			  sigma_delta->info->irq_flags,
> > +			  sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> >  			  indio_dev->name,
> >  			  sigma_delta);
> >  	if (ret)
> >  		goto error_free_trig;
> > 
> > -	if (!sigma_delta->irq_dis) {
> > -		sigma_delta->irq_dis = true;
> > -		disable_irq_nosync(sigma_delta->spi->irq);
> > -	}
> >  	iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
> > 
> >  	ret = iio_trigger_register(sigma_delta->trig);
> > --
> > 2.31.1  
> 
> Thanks
> Barry
>

Song Bao Hua (Barry Song) April 5, 2021, 10:56 p.m. UTC | #3

> -----Original Message-----
> From: Jonathan Cameron [mailto:[e-mail]]
> Sent: Tuesday, April 6, 2021 2:52 AM
> To: Song Bao Hua (Barry Song) <[e-mail]>
> Cc: [e-mail]; Jonathan Cameron <[e-mail]>;
> Lars-Peter Clausen <[e-mail]>; Alexandru Ardelean
> <[e-mail]>; tiantao (H) <[e-mail]>
> Subject: Re: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than
> request and disable
> 
> On Fri, 2 Apr 2021 20:30:17 +0000
> "Song Bao Hua (Barry Song)" <[e-mail]> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron [mailto:[e-mail]]
> > > Sent: Saturday, April 3, 2021 7:46 AM
> > > To: [e-mail]
> > > Cc: Song Bao Hua (Barry Song) <[e-mail]>; Jonathan Cameron
> > > <[e-mail]>; Lars-Peter Clausen <[e-mail]>;
> > > Alexandru Ardelean <[e-mail]>
> > > Subject: [PATCH 7/7] iio:adc:ad_sigma_delta: Use IRQF_NO_AUTOEN rather than
> > > request and disable
> > >
> > > From: Jonathan Cameron <[e-mail]>
> > >
> > > These devices are not able to mask the signal used as a data ready
> > > interrupt.  As such they previously requested the irq then immediately
> > > disabled it.  Now we can avoid the potential of a spurious interrupt
> > > by avoiding the irq being auto enabled in the first place.
> > >
> > > I'm not sure how this code could have been called with the irq already
> > > disabled, so I believe the conditional would always have been true and
> > > have removed it.
> > >
> >
> > If irq_dis is true, it seems the original code assumed interrupt was
> > open. Now the code is moving to disable-irq for true irq_dis. For
> > false irq_dis, this patch has no side effect so looks correct.
> Hi Barry,
> 
> As far as I can tell (and I looked closely :), at the point where
> ad_sd_probe_trigger() can be called, irq_dis is always false,
> so the original check is misleading by implying
> otherwise.

I didn't realize that. Thanks for clarification.

> 
> Taken in isolation of what is visible in this patch I'd agree
> the change you suggest makes sense, but I'd also like to clean
> up the wrong impression the original check was giving.  It is
> also worth noting that ad_sigma_delta.h lists irq_dis as a
> private member of the structure that should only be used in the
> library (reducing chances of any drivers in future doing something
> odd with it).
> 

Yes. It makes sense.

> The other checks on irq_dis look suspicious as well, but removing
> those would rather spread the scope of this patch.
> 
> As Lars has given an Reviewed-by and this is his code, I'm going
> to press ahead with this as it stands.
> 
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to see if we missed anything.
> 
> Thanks,
> 
> Jonathan
> 
> >
> > A safer way might be as below?
> >
> > if(!sigma_delta->irq_dis)
> > 	irq_flags = sigma_delta->info->irq_flags | IRQF_NO_AUTOEN;
> >
> > request_irq(irq_flags);
> >
> > sigma_delta->irq_dis =  true;
> >
> > > Signed-off-by: Jonathan Cameron <[e-mail]>
> > > Cc: Lars-Peter Clausen <[e-mail]>
> > > Cc: Alexandru Ardelean <[e-mail]>
> > > ---
> > >  drivers/iio/adc/ad_sigma_delta.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > > b/drivers/iio/adc/ad_sigma_delta.c
> > > index 9289812c0a94..e777ec718973 100644
> > > --- a/drivers/iio/adc/ad_sigma_delta.c
> > > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > > @@ -485,18 +485,15 @@ static int ad_sd_probe_trigger(struct iio_dev
> *indio_dev)
> > >  	sigma_delta->trig->ops = &ad_sd_trigger_ops;
> > >  	init_completion(&sigma_delta->completion);
> > >
> > > +	sigma_delta->irq_dis = true;
> > >  	ret = request_irq(sigma_delta->spi->irq,
> > >  			  ad_sd_data_rdy_trig_poll,
> > > -			  sigma_delta->info->irq_flags,
> > > +			  sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> > >  			  indio_dev->name,
> > >  			  sigma_delta);
> > >  	if (ret)
> > >  		goto error_free_trig;
> > >
> > > -	if (!sigma_delta->irq_dis) {
> > > -		sigma_delta->irq_dis = true;
> > > -		disable_irq_nosync(sigma_delta->spi->irq);
> > > -	}
> > >  	iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
> > >
> > >  	ret = iio_trigger_register(sigma_delta->trig);
> > > --
> > > 2.31.1
> >

Thanks
Barry

Patch

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 9289812c0a94..e777ec718973 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -485,18 +485,15 @@  static int ad_sd_probe_trigger(struct iio_dev *indio_dev)
 	sigma_delta->trig->ops = &ad_sd_trigger_ops;
 	init_completion(&sigma_delta->completion);
 
+	sigma_delta->irq_dis = true;
 	ret = request_irq(sigma_delta->spi->irq,
 			  ad_sd_data_rdy_trig_poll,
-			  sigma_delta->info->irq_flags,
+			  sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
 			  indio_dev->name,
 			  sigma_delta);
 	if (ret)
 		goto error_free_trig;
 
-	if (!sigma_delta->irq_dis) {
-		sigma_delta->irq_dis = true;
-		disable_irq_nosync(sigma_delta->spi->irq);
-	}
 	iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
 
 	ret = iio_trigger_register(sigma_delta->trig);