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

[David Lechner]

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?

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

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