RE: [PATCH v2] iio: adc: ad_sigma_delta: fix CS held asserted after single conversion

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Friday, May 15, 2026 4:27 PM
> To: Sabau, Radu bogdan <Radu.Sabau@xxxxxxxxxx>; Jonathan Cameron
> <jic23@xxxxxxxxxx>; Radu Sabau via B4 Relay
> <devnull+radu.sabau.analog.com@xxxxxxxxxx>
> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; Andy
> Shevchenko <andy@xxxxxxxxxx>; Uwe Kleine König <u.kleine-
> koenig@xxxxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] iio: adc: ad_sigma_delta: fix CS held asserted after
> single conversion
> 
> [External]
> 
> On 5/15/26 4:20 AM, Sabau, Radu bogdan wrote:
> >> -----Original Message-----
> >> From: David Lechner <dlechner@xxxxxxxxxxxx>
> >> Sent: Thursday, May 14, 2026 5:57 PM
> >
> > ...
> >
> >>>> I'm struggling a bit on how the max11205 works at all as there seems
> >>>> to be a status register read on a device which claims to have no registers.
> >>>> ad_sigma_delta_clear_pending_event() as the binding suggests this
> device
> >>>> doesn't have a drdy_gpio
> >>>>
> >>>> Anyhow, please take a look at the feedback and if it's wrong please
> provide
> >>>> an explanation of why in this thread.
> >>>>
> >>>
> >>> Hi Jonathan,
> >>>
> >>> First of all sorry for forgetting about Sashiko. I had a look at max11205 and
> it
> >>> seems like the device doesn't have a CS so therefore the concern Sashiko
> >>> raises regarding CS may not be valid, I will make sure to mention
> >>> that in the commit message.
> >>>
> >>> On the other hand, I see the IC has a shared pin for DRDY and DOUT and
> >>> the bindings specify some interrupt though no specific rdy-gpios are
> >>> mentioned there. The device reads a register that doesn't exist which
> >>> in this case means it blindly clocks out whatever comes on MISO...
> >>>
> >>> However I may have a fix for this and would appear in a second commit.
> >>> In clear_pending_event where rdy_gpiod is checked there could be yet
> >>> another else branch that could simply return 0 and here is why I think this
> >> would work:
> >>>
> >>> Since the IRQ is requested with IRQF_NO_AUTOEN and
> IRQ_DISABLE_UNLAZY,
> >> the latter
> >>> keeps the IRQ hardware-unmasked even while software-disabled, so any
> >> falling edge
> >>
> >> This should depend on the IRQ controller. There are some past discussions
> >> on this on the mailing list. The ideal behavior should be that if the
> >> IRQ controller can fully disable the interrupt so that we don't get the
> >> spurious interrupt on enable (from the normal SPI data, not DRDY). If
> >> the interrupt controller can't do this, then it requires the rdy-gpios
> >> to be able to distinguish DRDY from SPI data.
> >>
> >
> > rdy-gpios added to yaml of devices that don’t have them and also don't have
> > registers is a solution too. Though I think what you mean by this is that the
> > rdy-gpios should be something optional to be added by the user depending
> > on his IRQ controller capabilities, right? Perhaps this is the case here already.
> >
> >> I have used this on ad7173 on a ZedBoard without rdy-gpios and the
> >> interrupt was working correctly. So unless something changed with
> >> the interrupt config in this code since the last time I was using,
> >> I would not expect to see this problem on _all_ interrupt controllers.
> >>
> >
> > Yes but ad7173 has registers, max11205 and few others don't.
> > In the cases where no rdy-gpios, although registers are present or not,
> > it "reads" the status register which in these cases means clocking 1 byte
> 
> There is already sigma_delta->info->has_registers. Why is this not
> used to prevent reading the status register when there are no
> registers?

That's pretty much what that new 'else' would be doing, handling devices
without registers.

> 
> > from the DOUT line, and here 2 cases appear:
> >
> > 1. DOUT/RDY is still high which means DOUT reads 0xFF
> >
> > - "status reg" = 0xFF
> > - pending_event = !(0xFF & 0x80) = !(0x80) = false
> > - returns 0, nothing bad happens -> Works by accident
> >
> > 2. DOUT reads 0x00 (line LOW) , perhaps RDY dropped already.
> >
> > - The 1 byte read already consumed 8 bits of the actual conversion result and
> > data stream is already corrupted.
> > - "status reg" = 0x00
> > - pending_event = !(0x00 & 0x80) = !(0) = true
> > - Continues into the drain path
> > - kzalloc(0 + 1) -> allocates 1 byte
> > - memset(data + 2, 0xff, 0 - 1) -> data_read_len is unsigned int, so 0 - 1 =
> SIZE_MAX
> > - memeset of SIZE_MAX bytes -> heap corruption, kernel crash -> Worst
> case, but
> > still possible
> >
> > I would then say that the solutions are 1 of :
> > - add rdy-gpios as well where has_registers are false.
> > - add another else with return 0, since if the DRDY is not low, it will be after
> enable_irq,
> > and if it is low, it will be triggered afterwards and clock data correctly. For
> these devices
> > as far as I can tell, there should be no spurious signals, only the DRDY
> interrupt unless
> > clocking data. I may be wrong though, and perhaps the rdy-gpios is the safer
> move,
> > though this would mean less churn.
> 
> I would expect the need for rdy-gpios to be the same whether not not
> the chip has registers (only depends on the interrupt controller).

I would too, would then add rdy-gpios where needed instead.

> 
> >
> > What do you guys think?
> >
> > Thanks,
> > Radu
> >