When during a measurement two channels are enabled, two measurements are
done that are reported sequencially in the DATA register. As the code
triggered by reading one of the sysfs properties expects that only one
channel is enabled it only reads the first data set which might or might
not belong to the intended channel.

To prevent this situation disable all channels during probe. This fixes
a problem in practise because the reset default for channel 0 is
enabled. So all measurements before the first measurement on channel 0
(which disables channel 0 at the end) might report wrong values.

Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels")
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,

this patch was part of a series before. The remaining patches are still
under discussion. As this is a fix orthogonal to the other patches of
the series (apart from the other relevant change there also being
necessary to make the ad7124 work for me) it IMHO makes sense to apply
this one already now. There are machines that don't suffer from the
other issue (i.e. the device irq becoming pending by spi traffic), so
this fix is also valuable stand alone. It's IMHO good enough to go in
before v6.12.

The previous submission is available at
https://lore.kernel.org/linux-iio/20241028160748.489596-10-u.kleine-koenig@baylibre.com/

b4 ignored Nuno's Reviewed-by tag with

	NOTE: some trailers ignored due to from/email mismatches:
	    ! Trailer: Reviewed-by: Nuno Sa <nuno.sa@analog.com>
	     Msg From: Nuno Sá <noname.nuno@gmail.com>

I wonder if other maintainers use b4 apply's -S by default, because I
often run into this issue but don't see others mentioning that.
I added the tag here anyhow.

 drivers/iio/adc/ad7124.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index a5d91933f505..749304d38415 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -917,6 +917,9 @@ static int ad7124_setup(struct ad7124_state *st)
 		 * set all channels to this default value.
 		 */
 		ad7124_set_channel_odr(st, i, 10);
+
+		/* Disable all channels to prevent unintended conversions. */
+		ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, 0);
 	}
 
 	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);

base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
-- 
2.45.2

On Mon,  4 Nov 2024 11:19:04 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> When during a measurement two channels are enabled, two measurements are
> done that are reported sequencially in the DATA register. As the code
> triggered by reading one of the sysfs properties expects that only one
> channel is enabled it only reads the first data set which might or might
> not belong to the intended channel.
> 
> To prevent this situation disable all channels during probe. This fixes
> a problem in practise because the reset default for channel 0 is
> enabled. So all measurements before the first measurement on channel 0
> (which disables channel 0 at the end) might report wrong values.
> 
> Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels")
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
> Hello,
> 
> this patch was part of a series before. The remaining patches are still
> under discussion. As this is a fix orthogonal to the other patches of
> the series (apart from the other relevant change there also being
> necessary to make the ad7124 work for me) it IMHO makes sense to apply
> this one already now. There are machines that don't suffer from the
> other issue (i.e. the device irq becoming pending by spi traffic), so
> this fix is also valuable stand alone. It's IMHO good enough to go in
> before v6.12.
> 
> The previous submission is available at
> https://lore.kernel.org/linux-iio/20241028160748.489596-10-u.kleine-koenig@baylibre.com/
> 
> b4 ignored Nuno's Reviewed-by tag with
> 
> 	NOTE: some trailers ignored due to from/email mismatches:
> 	    ! Trailer: Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 	     Msg From: Nuno Sá <noname.nuno@gmail.com>
> 
> I wonder if other maintainers use b4 apply's -S by default, because I
> often run into this issue but don't see others mentioning that.
I only use -S when Nuno has given a tag.  It tends to be a bit too generous
on applying tags in general.

Thanks for picking it up here.

Sadly this has probably missed 6.12, but I have queued it up as a fix
for early next cycle and marked it for stable.

Thanks,

Jonathan

> I added the tag here anyhow.
> 
>  drivers/iio/adc/ad7124.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index a5d91933f505..749304d38415 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -917,6 +917,9 @@ static int ad7124_setup(struct ad7124_state *st)
>  		 * set all channels to this default value.
>  		 */
>  		ad7124_set_channel_odr(st, i, 10);
> +
> +		/* Disable all channels to prevent unintended conversions. */
> +		ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, 0);
>  	}
>  
>  	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
> 
> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc

Hello Jonathan,

On Sat, Nov 09, 2024 at 03:24:38PM +0000, Jonathan Cameron wrote:
> Sadly this has probably missed 6.12, but I have queued it up as a fix
> for early next cycle and marked it for stable.

I interpreted "early next cycle" as "This will go into v6.13-rc1.". But
that didn't work and didn't hit the mainline yet (as of cdd30ebb1b9f).

This patch was included in next as
64612ec9b909b699293b7220c634f67a9fc12e06 between next-20241111 and
next-20241128 and then disappeared from there.

What is wrong here?

Best regards
Uwe

On 12/6/24 5:04 AM, Uwe Kleine-König wrote:
> Hello Jonathan,
> 
> On Sat, Nov 09, 2024 at 03:24:38PM +0000, Jonathan Cameron wrote:
>> Sadly this has probably missed 6.12, but I have queued it up as a fix
>> for early next cycle and marked it for stable.
> 
> I interpreted "early next cycle" as "This will go into v6.13-rc1.". But
> that didn't work and didn't hit the mainline yet (as of cdd30ebb1b9f).
> 
> This patch was included in next as
> 64612ec9b909b699293b7220c634f67a9fc12e06 between next-20241111 and
> next-20241128 and then disappeared from there.
> 
> What is wrong here?
> 
> Best regards
> Uwe

FYI, the iio tree is currently missing from linux-next due to [1].

[1]: https://lore.kernel.org/all/d707cb3b-1569-45d9-bdc3-dcc98eb88bc4@sirena.org.uk/

On Fri, Dec 06, 2024 at 09:35:15AM -0600, David Lechner wrote:
> On 12/6/24 5:04 AM, Uwe Kleine-König wrote:
> > Hello Jonathan,
> > 
> > On Sat, Nov 09, 2024 at 03:24:38PM +0000, Jonathan Cameron wrote:
> >> Sadly this has probably missed 6.12, but I have queued it up as a fix
> >> for early next cycle and marked it for stable.
> > 
> > I interpreted "early next cycle" as "This will go into v6.13-rc1.". But
> > that didn't work and didn't hit the mainline yet (as of cdd30ebb1b9f).
> > 
> > This patch was included in next as
> > 64612ec9b909b699293b7220c634f67a9fc12e06 between next-20241111 and
> > next-20241128 and then disappeared from there.
> > 
> > What is wrong here?
> > 
> > Best regards
> > Uwe
> 
> FYI, the iio tree is currently missing from linux-next due to [1].
> 
> [1]: https://lore.kernel.org/all/d707cb3b-1569-45d9-bdc3-dcc98eb88bc4@sirena.org.uk/

I first thought that couldn't be the explanation because that conflict
happened to late, but indeed next-20241203 is the first next tag after
next-20241128 and

	git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git fixes-togreg

still contains my patch. Thanks for that hint.

Masahiro pointed out a merge fix in the linked thread, but I think it's
incomplete/wrong.

When merging the above branch (currently at
1694dea95b02eff1a64c893ffee4626df533b2ab) into ceb8bf2ceaa7 ("module:
Convert default symbol namespace to string literal") I'd do:

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 215731bd3c7d..ef9875d3b79d 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -100,7 +100,7 @@ const struct regmap_config inv_icm42600_spi_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 	.use_single_write = true,
 };
-EXPORT_SYMBOL_NS_GPL(inv_icm42600_spi_regmap_config, IIO_ICM42600);
+EXPORT_SYMBOL_NS_GPL(inv_icm42600_spi_regmap_config, "IIO_ICM42600");
 
 struct inv_icm42600_hw {
 	uint8_t whoami;

The MODULE_IMPORT_NS line for
drivers/iio/imu/inv_icm42600/inv_icm42600_core.c was already fixed in 
commit cdd30ebb1b9f ("module: Convert symbol namespace to string
literal").

Best regards
Uwe

On Fri, 6 Dec 2024 17:11:44 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> On Fri, Dec 06, 2024 at 09:35:15AM -0600, David Lechner wrote:
> > On 12/6/24 5:04 AM, Uwe Kleine-König wrote:  
> > > Hello Jonathan,
> > > 
> > > On Sat, Nov 09, 2024 at 03:24:38PM +0000, Jonathan Cameron wrote:  
> > >> Sadly this has probably missed 6.12, but I have queued it up as a fix
> > >> for early next cycle and marked it for stable.  
> > > 
> > > I interpreted "early next cycle" as "This will go into v6.13-rc1.". But
> > > that didn't work and didn't hit the mainline yet (as of cdd30ebb1b9f).
> > > 
> > > This patch was included in next as
> > > 64612ec9b909b699293b7220c634f67a9fc12e06 between next-20241111 and
> > > next-20241128 and then disappeared from there.
> > > 
> > > What is wrong here?
> > > 
> > > Best regards
> > > Uwe  
> > 
> > FYI, the iio tree is currently missing from linux-next due to [1].
> > 
> > [1]: https://lore.kernel.org/all/d707cb3b-1569-45d9-bdc3-dcc98eb88bc4@sirena.org.uk/  
> 
> I first thought that couldn't be the explanation because that conflict
> happened to late, but indeed next-20241203 is the first next tag after
> next-20241128 and
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git fixes-togreg
> 
> still contains my patch. Thanks for that hint.
> 
> Masahiro pointed out a merge fix in the linked thread, but I think it's
> incomplete/wrong.
> 
> When merging the above branch (currently at
> 1694dea95b02eff1a64c893ffee4626df533b2ab) into ceb8bf2ceaa7 ("module:
> Convert default symbol namespace to string literal") I'd do:
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 215731bd3c7d..ef9875d3b79d 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -100,7 +100,7 @@ const struct regmap_config inv_icm42600_spi_regmap_config = {
>  	.cache_type = REGCACHE_RBTREE,
>  	.use_single_write = true,
>  };
> -EXPORT_SYMBOL_NS_GPL(inv_icm42600_spi_regmap_config, IIO_ICM42600);
> +EXPORT_SYMBOL_NS_GPL(inv_icm42600_spi_regmap_config, "IIO_ICM42600");
>  
>  struct inv_icm42600_hw {
>  	uint8_t whoami;
> 
> The MODULE_IMPORT_NS line for
> drivers/iio/imu/inv_icm42600/inv_icm42600_core.c was already fixed in 
> commit cdd30ebb1b9f ("module: Convert symbol namespace to string
> literal").
> 
> Best regards
> Uwe

Thanks for the heads up. This had passed me by completely.... 
I normally rebase on rc1 whereas these we a tiny bit after.
I've rebased on char-misc/char-misc-next and char-misc/char-misc-linus that
should clean up the dependencies as Greg has dragged in the couple of
changes just after rc1. I'm a little confused why Linus did them then
as opposed to just before tagging rc1. Ah well.

Jonathan


Jonathan