Message-ID: <bcf599e1-7816-4580-a2f9-039b0d3c0c99@baylibre.com>
Date: Sun, 13 Jul 2025 11:35:06 -0500
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adc: ad7173: prevent scan if too many setups
 requested

On 7/13/25 9:58 AM, Jonathan Cameron wrote:
> On Wed, 09 Jul 2025 11:35:52 -0500
> David Lechner <dlechner@...libre.com> wrote:
> 
>> Add a check to ad7173_update_scan_mode() to ensure that we didn't exceed
>> the maximum number of unique channel configurations.
>>
>> In the AD7173 family of chips, there are some chips that have 16
>> CHANNELx registers but only 8 setups (combination of CONFIGx, FILTERx,
>> GAINx and OFFSETx registers). Since commit 2233378a8c60 ("iio: adc:
>> ad7173: fix num_slots"), it is possible to have more than 8 channels
>> enabled in a scan at the same time, so it is possible to get a bad
>> configuration where more than 8 channels are using unique configurations.
>> This happens because the algorithm to allocate the setup slots only
>> takes into account which slot has been least recently used and doesn't
>> know about the maximum number of slots available.
>>
>> Since the algorithm to allocate the setup slots is quite complex, it is
>> simpler to check after the fact if the current state is valid or not.
>> So this patch adds a check in ad7173_update_scan_mode() after setting up
>> all of the configurations to make sure that the actual setup still
>> matches the requested setup for each enabled channel. If not, we prevent
>> the scan from being enabled and return an error.
>>
>> The setup comparison is ad7173_setup_equal() is refactored to a separate
>> function since we need to call it in two places now.
>>
>> Fixes: 2233378a8c60 ("iio: adc: ad7173: fix num_slots")
>> Signed-off-by: David Lechner <dlechner@...libre.com>
>> ---
>> I know this isn't pretty, but after puzzling over it for a day, this was
>> the best I could come up with that didn't involve a complete rewrite of
>> the setup allocation algorithm.
>>
>> I don't really understand why we care about which setup was the least
>> recently used - it isn't like we are going to wear out one setup by
>> using it too much. 
>> Maybe it was just to reduce the number of SPI xfers?
> 
> Been a while, so I may be remembering the intent here wrong.
> The challenge of these allocators is exactly what you have called out.
> How do we cope if too many configs are needed to deliver the mix of
> channel configs requested.  I think the LRU thing was an attempt to
> reduce the amount of reconfiguring needed.  That's mostly relevant of
> single channel reads I think...
> 
> 
>> Anyway, ad7124 has a similar setup allocation algorithm, so it could be
>> nice to eventually replace both of these with something common that is
>> a bit simpler, e.g. always use SETUP 0 for single transfers and allocate
>> the rest of the setups in order for buffered reads with more than one
>> channel enabled.
> 
> So don't use setup 0 for buffered reads?  That sounds odd.

I didn't word that well. All setups would be used for buffered
reads.

> 
>> And just always re-write the setup each time so we
>> don't have to try to keep track of what each slot is programmed with.
> 
> Fair enough as a simplification.
> 
> If you've stopped using the lru, why are things like the _lru() functions still used?

This is just speculation on how we could do this differently in
the future, not what the actual change is here. A complete rewrite
seemed too big of a change for Fixes, so I've started with just
this patch for now and may consider doing what I've suggested above
in the future.

Powered by blists - more mailing lists