Re: [PATCH v2 2/2] iio: adc: ti-ads1298: Add driver

[Mike Looijmans <mike.looijmans@topic.nl>]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail
On 04-02-2024 16:54, Jonathan Cameron wrote:
> On Fri, 2 Feb 2024 11:59:01 +0100
> Mike Looijmans<mike.looijmans@topic.nl>  wrote:
>
>> Skeleton driver for the TI ADS1298 medical ADC. This device is
>> typically used for ECG and similar measurements. Supports data
>> acquisition at configurable scale and sampling frequency.
>>
>> Signed-off-by: Mike Looijmans<mike.looijmans@topic.nl>
>>
> Hi Mike,
>
> A few minor things I'd missed before.
>
> I'm still interested in why more standard interrupt handling isn't
> good enough here (see reply to v1 thread) but if we can't get to the bottom
> of that (or do figure it out and we can't fix it) then this doesn't look
> too bad so I'll accept the complex handling.

I think one of the key elements was the IRQF_ONESHOT usage. The DRDY 
signal on this chip isn't a "level" signal as most chips have, it will 
de-assert at any rising edge of the SPI clock, without regarding 
chip-select. There's no other indication of "data ready", so the only 
way is to keep the interrupt active on edge detect.

Keeping things in hard IRQ handlers reduces the number of context 
switches, and the amount of work done is minimal. A worker thread would 
wake at every DRDY signal, and after the corresponding SPI transaction. 
This doesn't account for much overhead, but the interrupt rate is double 
the sampling frequency. Most importantly, the device doesn't have to 
compete with other threads in the system.

If I have time and hardware available, I try to get some timing info 
with an oscilloscope...

Assume "yes" to all other suggestions...

>> +	ret = devm_request_irq(dev, spi->irq, &ads1298_interrupt,
>> +			       IRQF_TRIGGER_FALLING, indio_dev->name,
> I missed this before (and we've gotten it wrong a bunch of times in the past
> so plenty of bad examples to copy that we can't fix without possible
> regressions) but we generally now leave irq direction to the firmware description.
> People have an annoying habit of putting not gates and similar in the path
> to interrupt pins.  Fine to have the binding state the expected form though
> (as you do).  So basically not flags here.

I'd be happy to leave the IRQ direction to firmware (indeed, inverters 
happen...), but afaik that wasn't possible with interrupts. I'll dig 
into the docs to see if that has changed recently.


> I'm still curious to understand more of where the delays that lead to
> needing to do this complex handling came from, but I guess it's not too bad
> if we can't get to the bottom of that so I'll take the driver anyway
> (after a little more time on list for others to review!)
>
>> +			       indio_dev);

(PS - sorry for sending HTML, consider me appropriately punisched for that)

-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E:mike.looijmans@topic.nl
W:www.topic.nl