Re: [PATCH v3 0/9] iio: adc: Add TI ADS126X ADC family support

On Sat Aug 8, 2026 at 1:37 PM -05, David Lechner wrote:
> On 8/7/26 10:58 PM, Kurt Borja wrote:
>
> ...
>
>>   - @David: I added support for the monitor channels, but I prefer to
>>     parse them from DT instead of making them static (similar to the
>>     ad4170-4 approach too :p).
>
> Why? Unless there really is some property that depends on how the
> system is wired up, it seems like this is just making unnecessary
> work for users to be able to use the monitor channels. And if someone
> decided later that they do in fact want to use the monitoring channel
> and it wasn't in the devicetree, sometimes it can be very difficult
> to actually change the devicetree.

The only thing I can think of is the reference source. The datasheet
says "Measure the supply monitor readings using either the internal or
an external reference".

I saw that the ti-ads112c14 also allows the monitors to be referenced
externally but you didn't implement support for it. In my case I think
it's okay to leave it unimplemented too and make the channels static.

>
> The monitor inputs also have many restrictions compared to a
> normal input that it would be really hard to describe correctly
> in the bindings without allowing things that should not actually
> be allowed. (can't have excitation current or burnout, temperature
> channel requires internal reference, most should be single-channel,
> etc.)

Good point.

>
>> 
>>   - @David: About filters... As I mentioned in the previous version, the
>>     data_rate configuration takes precedence over the filter selection.
>>     If an incompatible filter (given a data rate) is selected, the chip
>>     resorts to a sane compatible one when doing conversions (either
>>     SINC1 or plain SINC5).
>> 
>>     Now, I don't know how to expose this in userspace. Should I limit
>>     the sampling_frequency_available attribute (given a filter)? Or
>>     should it be the other way around, limit the filter_type_available
>>     attribute (given a data rate)?.
>  I figured that the filter type selection would be more important than
> the rate so when I implemented it for ADS112C14, I made it so that
> one has to pick the filter first and everything else flows from that.
> (I didn't expose sampling frequency until the same time as filter type.)
>
> The thinking behind this is that if you do care about filtering, then
> you are picking filter type and sampling rate to get certain notches
> and/or frequency response of the filter rather than trying to get a
> faster or slower sample rate.

I think this makes a lot of sense in your chip because there is no
plain "data rate" register. The data rate ends up being a consequence of
the modulator divider + OSR/filter settings.

>
> And the driver also allows using an hrtimer trigger to do single-shot
> samples for cases where one doesn't want to sample as fast as possible
> in continuous mode. This would be more useful to someone who just cares
> about sample rate and not about filtering.

Why did you go for this instead of just leaving the continuous mode
running and reading on each trigger?

>
> Just posted the series yesterday:
> https://lore.kernel.org/linux-iio/20260807-iio-adc-ti-ads112c14-filter-support-v1-0-4d3ba00caf18@baylibre.com/T/#t

Can you Cc me this series too? The settlingtime stuff is something I'll
implement too.

>
> ADS126X seems a little less complicated in this regard though
> as the same sampling rates are available for all filters with
> the exception of the FIR filter having a limited subset. So I
> would go with the option to limit sampling rate based on filter
> type, not the other way around. If a higher rate is selected
> when changing to the FIR filter type, just have it go to the
> max (20 SPS).

I'll go for this!

>

Thank you very much for your review and tags :)

-- 
Thanks,
 ~ Kurt