| Message ID | 20240404175126.110208-1-markus.burri@mt.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v1] iio: adc: ad_sigma_delta: Clear pending interrupts before enable | expand |
On Thu, 4 Apr 2024 19:51:26 +0200 Markus Burri <markus.burri@mt.com> wrote: Hi Markus, > For device will enable and disable irq contiously like AD7195, > it use DOUT/RDY pin for both SPI transfer and data ready. > It will disable irq during SPI transfer, and re-eanble irq after SPI transfer. > That may cause irq status bit set to 1 during spi transfer. Superficially that sounds like it might be an irq driver bug to me... Or just possibly an irq chip doing lazy disabling? > > When the active condition has been detected, the corresponding bit > remains set until cleared by software. Status flags are cleared > by writing a 1 to the corresponding bit position. > > Signed-off-by: Markus Burri <markus.burri@mt.com> I'll need an appropriate ADI ack for this one. It seems highly unusual to be calling a generic irqchip related function in a driver (there are no other such users). So this seems unlikely to be the right fix for this particular problem. > --- > drivers/iio/adc/ad_sigma_delta.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c > index a602429cdde4..59544c39642a 100644 > --- a/drivers/iio/adc/ad_sigma_delta.c > +++ b/drivers/iio/adc/ad_sigma_delta.c > @@ -515,6 +515,10 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p) > > irq_handled: > iio_trigger_notify_done(indio_dev->trig); > + /* > + * ACK pending interrupts from spi transfer. Single line comment format would be fine here. > + */ > + irq_gc_ack_set_bit(irq_get_irq_data(sigma_delta->spi->irq)); > sigma_delta->irq_dis = false; > enable_irq(sigma_delta->spi->irq); >
On Sat, 2024-04-06 at 17:17 +0100, Jonathan Cameron wrote: > On Thu, 4 Apr 2024 19:51:26 +0200 > Markus Burri <markus.burri@mt.com> wrote: > > Hi Markus, > > > > For device will enable and disable irq contiously like AD7195, > > it use DOUT/RDY pin for both SPI transfer and data ready. > > It will disable irq during SPI transfer, and re-eanble irq after SPI > > transfer. > > That may cause irq status bit set to 1 during spi transfer. > > Superficially that sounds like it might be an irq driver bug to me... > Or just possibly an irq chip doing lazy disabling? Yes, this sounds odd as we are already explicitly disabling lazy disabling: https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad_sigma_delta.c#L589 > > > > > When the active condition has been detected, the corresponding bit > > remains set until cleared by software. Status flags are cleared > > by writing a 1 to the corresponding bit position. > > > > Signed-off-by: Markus Burri <markus.burri@mt.com> > > I'll need an appropriate ADI ack for this one. > Yeah, I wanted to reply to this one Friday but then completely forgot. I can't really ack this one. I would need some insights from someone with more core IRQ knowledge. But... > It seems highly unusual to be calling a generic irqchip related function in a > driver (there are no other such users). So this seems unlikely to be > the right fix for this particular problem. > Yes, and this (I think) would not even fix (if a fix is needed) this for all irqchips which to me already sounds not the way to go. - Nuno Sá >
On Mon, 08 Apr 2024 10:31:21 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sat, 2024-04-06 at 17:17 +0100, Jonathan Cameron wrote: > > On Thu, 4 Apr 2024 19:51:26 +0200 > > Markus Burri <markus.burri@mt.com> wrote: > > > > Hi Markus, > > > > > > > For device will enable and disable irq contiously like AD7195, > > > it use DOUT/RDY pin for both SPI transfer and data ready. > > > It will disable irq during SPI transfer, and re-eanble irq after SPI > > > transfer. > > > That may cause irq status bit set to 1 during spi transfer. > > > > Superficially that sounds like it might be an irq driver bug to me... > > Or just possibly an irq chip doing lazy disabling? > > Yes, this sounds odd as we are already explicitly disabling lazy disabling: > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad_sigma_delta.c#L589 Markus, can you confirm you are running no a kernel with that line present? Maybe this is just an old kernel not having the fix? > > > > > > > > > When the active condition has been detected, the corresponding bit > > > remains set until cleared by software. Status flags are cleared > > > by writing a 1 to the corresponding bit position. > > > > > > Signed-off-by: Markus Burri <markus.burri@mt.com> > > > > I'll need an appropriate ADI ack for this one. > > > > Yeah, I wanted to reply to this one Friday but then completely forgot. I can't > really ack this one. I would need some insights from someone with more core IRQ > knowledge. But... > > > It seems highly unusual to be calling a generic irqchip related function in a > > driver (there are no other such users). So this seems unlikely to be > > the right fix for this particular problem. > > > > Yes, and this (I think) would not even fix (if a fix is needed) this for all > irqchips which to me already sounds not the way to go. > > - Nuno Sá > >
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index a602429cdde4..59544c39642a 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -515,6 +515,10 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p) irq_handled: iio_trigger_notify_done(indio_dev->trig); + /* + * ACK pending interrupts from spi transfer. + */ + irq_gc_ack_set_bit(irq_get_irq_data(sigma_delta->spi->irq)); sigma_delta->irq_dis = false; enable_irq(sigma_delta->spi->irq);
For device will enable and disable irq contiously like AD7195, it use DOUT/RDY pin for both SPI transfer and data ready. It will disable irq during SPI transfer, and re-eanble irq after SPI transfer. That may cause irq status bit set to 1 during spi transfer. When the active condition has been detected, the corresponding bit remains set until cleared by software. Status flags are cleared by writing a 1 to the corresponding bit position. Signed-off-by: Markus Burri <markus.burri@mt.com> --- drivers/iio/adc/ad_sigma_delta.c | 4 ++++ 1 file changed, 4 insertions(+)