Re: [PATCH v4 01/17] staging: iio: resolver: ad2s1210: do not use fault register for dummy read

From: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: linux-iio@vger.kernel.org, linux-staging@lists.linux.dev,
	"Michael Hennerich" <Michael.Hennerich@analog.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Axel Haslam" <ahaslam@baylibre.com>,
	"Philip Molloy" <pmolloy@baylibre.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 01/17] staging: iio: resolver: ad2s1210: do not use fault register for dummy read
Date: Tue, 10 Oct 2023 16:38:23 +0100	[thread overview]
Message-ID: <20231010163823.00612bdd@jic23-huawei> (raw)
In-Reply-To: <20231005-ad2s1210-mainline-v4-1-ec00746840fc@baylibre.com>

On Thu,  5 Oct 2023 19:50:18 -0500
David Lechner <dlechner@baylibre.com> wrote:

> When reading registers on the AD2S1210 chip, we have to supply a "dummy"
> address for the second SPI tx byte so that we don't accidentally write
> to a register. This register will be read and the value discarded on the
> next regmap read or write call.
> 
> Reading the fault register has a side-effect of clearing the faults
> so we should not use this register for the dummy read.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
ouch.

Applied to the togreg branch of iio.git and pushed out as testing for 0day
to take a look at it.

Thanks,

Jonathan

> ---
> 
> v4 changes: New patch
> 
> (this probably should have been done before "staging: iio: resolver:
> ad2s1210: use regmap for config registers" but was overlooked until now)
> 
>  drivers/staging/iio/resolver/ad2s1210.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 67d8af0dd7ae..8fbde9517fe9 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -166,9 +166,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
>  	st->tx[0] = reg;
>  	/*
>  	 * Must be valid register address here otherwise this could write data.
> -	 * It doesn't matter which one.
> +	 * It doesn't matter which one as long as reading doesn't have side-
> +	 * effects.
>  	 */
> -	st->tx[1] = AD2S1210_REG_FAULT;
> +	st->tx[1] = AD2S1210_REG_CONTROL;
>  
>  	ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
>  	if (ret < 0)
> 