[PATCH v2] iio: adc: ad7124: Micro-optimize channel disabling

[PATCH v2] iio: adc: ad7124: Micro-optimize channel disabling

[Uwe Kleine-König]

The key objective in ad7124_disable_one() is clearing the
AD7124_CHANNEL_EN_MSK bit in the channel register. However there is no
advantage to keep the other bits in that register because when the
channel is used next time, all fields are rewritten anyhow. So instead
of using ad7124_spi_write_mask() (which is a register read plus a
register write) use a simple register write clearing the complete
register.

Also do the same in the .disable_all() callback by using the
.disable_one() callback there.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,

in (implicit) v1 I only adapted ad7124_disable_one() which resulted in
the very legitimate question why this wasn't done for .disable_all(). I
haven't checked because I wrongly assumed that .disable_all() used
.disable_one(). This v2 now makes the latter true and so .disable_all()
now also benefits from the micro optimisation.

Best regards
Uwe

 drivers/iio/adc/ad7124.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 6ae27cdd3250..2fdeb3247952 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -540,6 +540,14 @@ static int ad7124_append_status(struct ad_sigma_delta *sd, bool append)
 	return 0;
 }
 
+static int ad7124_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
+{
+	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
+
+	/* The relevant thing here is that AD7124_CHANNEL_EN_MSK is cleared. */
+	return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(chan), 2, 0);
+}
+
 static int ad7124_disable_all(struct ad_sigma_delta *sd)
 {
 	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
@@ -547,7 +555,7 @@ static int ad7124_disable_all(struct ad_sigma_delta *sd)
 	int i;
 
 	for (i = 0; i < st->num_channels; i++) {
-		ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_EN_MSK, 0, 2);
+		ret = ad7124_disable_one(sd, i);
 		if (ret < 0)
 			return ret;
 	}
@@ -555,13 +563,6 @@ static int ad7124_disable_all(struct ad_sigma_delta *sd)
 	return 0;
 }
 
-static int ad7124_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
-{
-	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
-
-	return ad7124_spi_write_mask(st, AD7124_CHANNEL(chan), AD7124_CHANNEL_EN_MSK, 0, 2);
-}
-
 static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
 	.set_channel = ad7124_set_channel,
 	.append_status = ad7124_append_status,

base-commit: b323d8e7bc03d27dec646bfdccb7d1a92411f189
-- 
2.47.1

Re: [PATCH v2] iio: adc: ad7124: Micro-optimize channel disabling

[Nuno Sá]

On Mon, 2025-01-20 at 15:07 +0100, Uwe Kleine-König wrote:
> The key objective in ad7124_disable_one() is clearing the
> AD7124_CHANNEL_EN_MSK bit in the channel register. However there is no
> advantage to keep the other bits in that register because when the
> channel is used next time, all fields are rewritten anyhow. So instead
> of using ad7124_spi_write_mask() (which is a register read plus a
> register write) use a simple register write clearing the complete
> register.
> 
> Also do the same in the .disable_all() callback by using the
> .disable_one() callback there.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

> Hello,
> 
> in (implicit) v1 I only adapted ad7124_disable_one() which resulted in
> the very legitimate question why this wasn't done for .disable_all(). I
> haven't checked because I wrongly assumed that .disable_all() used
> .disable_one(). This v2 now makes the latter true and so .disable_all()
> now also benefits from the micro optimisation.
> 
> Best regards
> Uwe
> 
>  drivers/iio/adc/ad7124.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 6ae27cdd3250..2fdeb3247952 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -540,6 +540,14 @@ static int ad7124_append_status(struct ad_sigma_delta
> *sd, bool append)
>  	return 0;
>  }
>  
> +static int ad7124_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
> +{
> +	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
> +
> +	/* The relevant thing here is that AD7124_CHANNEL_EN_MSK is cleared.
> */
> +	return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(chan), 2, 0);
> +}
> +
>  static int ad7124_disable_all(struct ad_sigma_delta *sd)
>  {
>  	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
> @@ -547,7 +555,7 @@ static int ad7124_disable_all(struct ad_sigma_delta *sd)
>  	int i;
>  
>  	for (i = 0; i < st->num_channels; i++) {
> -		ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i),
> AD7124_CHANNEL_EN_MSK, 0, 2);
> +		ret = ad7124_disable_one(sd, i);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -555,13 +563,6 @@ static int ad7124_disable_all(struct ad_sigma_delta *sd)
>  	return 0;
>  }
>  
> -static int ad7124_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
> -{
> -	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
> -
> -	return ad7124_spi_write_mask(st, AD7124_CHANNEL(chan),
> AD7124_CHANNEL_EN_MSK, 0, 2);
> -}
> -
>  static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
>  	.set_channel = ad7124_set_channel,
>  	.append_status = ad7124_append_status,
> 
> base-commit: b323d8e7bc03d27dec646bfdccb7d1a92411f189

Re: [PATCH v2] iio: adc: ad7124: Micro-optimize channel disabling

[Jonathan Cameron]

On Mon, 20 Jan 2025 15:52:20 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2025-01-20 at 15:07 +0100, Uwe Kleine-König wrote:
> > The key objective in ad7124_disable_one() is clearing the
> > AD7124_CHANNEL_EN_MSK bit in the channel register. However there is no
> > advantage to keep the other bits in that register because when the
> > channel is used next time, all fields are rewritten anyhow. So instead
> > of using ad7124_spi_write_mask() (which is a register read plus a
> > register write) use a simple register write clearing the complete
> > register.
> > 
> > Also do the same in the .disable_all() callback by using the
> > .disable_one() callback there.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > ---  
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Applied.  Thanks,