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

["Nuno Sá" <noname.nuno@gmail.com>]

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