There's a small issue with setting oversampling-ratio that seems to have
been there since the driver was in staging.
Trying to set an oversampling value of '2' will set an oversampling value
of '1'. This is because find_closest() does an average + rounding of 1 + 2,
and we get '1'.

This is the only issue with find_closest(), at least in this setup. The
other values (above 2) work reasonably well. Setting 3, rounds to 2, so a
quick fix is to round 'val' to 3 (if userspace provides 2).

Fixes 41f71e5e7daf: ("staging: iio: adc: ad7606: Use find_closest() macro")
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index ae49f4ba50d9..d0fe9fd65f3f 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -748,6 +748,9 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		if (val2)
 			return -EINVAL;
+		/* Minor trick, so that OS of 2, doesn't get rounded to 1 */
+		if (val == 2)
+			val++;
 		i = find_closest(val, st->oversampling_avail,
 				 st->num_os_ratios);
 		ret = st->write_os(indio_dev, i);
-- 
2.46.1

On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> There's a small issue with setting oversampling-ratio that seems to have
> been there since the driver was in staging.
> Trying to set an oversampling value of '2' will set an oversampling value
> of '1'. This is because find_closest() does an average + rounding of 1 + 2,
> and we get '1'.
> 
> This is the only issue with find_closest(), at least in this setup. The
> other values (above 2) work reasonably well. Setting 3, rounds to 2, so a
> quick fix is to round 'val' to 3 (if userspace provides 2).

This sounds like a bug in find_closest() instead of in this driver.

If there is an exact match in the list, it seems reasonable to expect
that the exact match is returned by find_closest().

On 10/21/24 2:03 PM, David Lechner wrote:
> On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
>> There's a small issue with setting oversampling-ratio that seems to have
>> been there since the driver was in staging.
>> Trying to set an oversampling value of '2' will set an oversampling value
>> of '1'. This is because find_closest() does an average + rounding of 1 + 2,
>> and we get '1'.
>>
>> This is the only issue with find_closest(), at least in this setup. The
>> other values (above 2) work reasonably well. Setting 3, rounds to 2, so a
>> quick fix is to round 'val' to 3 (if userspace provides 2).
> 
> This sounds like a bug in find_closest() instead of in this driver.
> 
> If there is an exact match in the list, it seems reasonable to expect
> that the exact match is returned by find_closest().
> 

Likely also affected by this bug since they have values 1, 2 in the list:

* rtq6056_adc_set_average()
* si1133_scale_to_swgain()

On Mon, Oct 21, 2024 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 10/21/24 2:03 PM, David Lechner wrote:
> > On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> >> There's a small issue with setting oversampling-ratio that seems to have
> >> been there since the driver was in staging.
> >> Trying to set an oversampling value of '2' will set an oversampling value
> >> of '1'. This is because find_closest() does an average + rounding of 1 + 2,
> >> and we get '1'.
> >>
> >> This is the only issue with find_closest(), at least in this setup. The
> >> other values (above 2) work reasonably well. Setting 3, rounds to 2, so a
> >> quick fix is to round 'val' to 3 (if userspace provides 2).
> >
> > This sounds like a bug in find_closest() instead of in this driver.
> >

Adding Bart (the original author of find_closest()).

> > If there is an exact match in the list, it seems reasonable to expect
> > that the exact match is returned by find_closest().
> >
>
> Likely also affected by this bug since they have values 1, 2 in the list:
>
> * rtq6056_adc_set_average()
> * si1133_scale_to_swgain()

Yeah.
I forgot to mention this sooner.
But this patch is more of an RFC patch about how to handle this
situation with find_closest().

For monotonic values with an increment of 1, find_closest() is a bit buggy.
Will try to fix find_closest()

>

On Tue, 2024-10-22 at 09:31 +0300, Alexandru Ardelean wrote:
> On Mon, Oct 21, 2024 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote:
> > 
> > On 10/21/24 2:03 PM, David Lechner wrote:
> > > On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > > > There's a small issue with setting oversampling-ratio that seems to have
> > > > been there since the driver was in staging.
> > > > Trying to set an oversampling value of '2' will set an oversampling
> > > > value
> > > > of '1'. This is because find_closest() does an average + rounding of 1 +
> > > > 2,
> > > > and we get '1'.
> > > > 
> > > > This is the only issue with find_closest(), at least in this setup. The
> > > > other values (above 2) work reasonably well. Setting 3, rounds to 2, so
> > > > a
> > > > quick fix is to round 'val' to 3 (if userspace provides 2).
> > > 
> > > This sounds like a bug in find_closest() instead of in this driver.
> > > 
> 
> Adding Bart (the original author of find_closest()).
> 
> > > If there is an exact match in the list, it seems reasonable to expect
> > > that the exact match is returned by find_closest().
> > > 
> > 
> > Likely also affected by this bug since they have values 1, 2 in the list:
> > 
> > * rtq6056_adc_set_average()
> > * si1133_scale_to_swgain()
> 
> Yeah.
> I forgot to mention this sooner.
> But this patch is more of an RFC patch about how to handle this
> situation with find_closest().
> 
> For monotonic values with an increment of 1, find_closest() is a bit buggy.
> Will try to fix find_closest()
> 
> > 

FWIW, I'm not a fan of using find_closest() in this situation. We have an
available attr wich outputs the supported values. To me, -EINVAL is the way to
go if some user writes an invalid value.

I feel the usage of find_closest() in these case is likely to make the code
easier. Maybe an helper analogous to match_string() would be seen with good eyes
(like match_value()).

But yeah, I guess that changing the behavior now could break some userspace
users.

- Nuno Sá