Re: [PATCH v6 07/17] iio: adc: ad7768: Add power mode helper

[Jonathan Cameron]

> Describe each power mode and its internal master-clock divisor in a
> table. Replace the open-coded fast-mode setup with a helper that records
> the active mode, preparing for dynamic mode selection.
> 
> Signed-off-by: Janani Sunil <janani.sunil@xxxxxxxxxx>

A sashiko comment that if nothing else suggest a nice little
optimization if there are no ordering issues. I haven't checked
the datasheet to be sure if there are or not.

>
> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> index 707f3ae70a95..71db57a2b9dd 100644
> --- a/drivers/iio/adc/ad7768.c
> +++ b/drivers/iio/adc/ad7768.c
...

>  
> +static int ad7768_set_power_mode(struct ad7768_state *st,
> +				 unsigned int mode_idx)
> +{
> +	const struct ad7768_power_mode_info *mode_info;
> +	int ret;
> +
> +	mode_info = &ad7768_power_modes[mode_idx];
> +	ret = regmap_update_bits(st->regmap, AD7768_REG_POWER_MODE,
> +				 AD7768_POWER_MODE_POWER_MODE_MSK,
> +				 FIELD_PREP(AD7768_POWER_MODE_POWER_MODE_MSK,
> +					    mode_info->mode));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD7768_REG_POWER_MODE,
> +				 AD7768_POWER_MODE_MCLK_DIV_MSK,
> +				 FIELD_PREP(AD7768_POWER_MODE_MCLK_DIV_MSK,
> +					    mode_info->mode));

Sashiko:
[Severity: Medium]
Does splitting this register update into two consecutive read-modify-write
operations to AD7768_REG_POWER_MODE create a problematic transient hardware
state?

Updating the analog power mode first and the digital clock divider second might
leave them temporarily mismatched. If the analog power mode is updated to a
lower state while the clock divider remains fast, could this violate internal
timing constraints and risk ADC glitches or unpredictable behavior?

Could these two fields be updated in a single regmap_update_bits() call to
avoid both the transient hardware state and the redundant SPI bus overhead?

-

Is there a reason to split these?  If not even if the transient
state doesn't matter, it would be nice to avoid the double write
and update both fields in one go.  You used to do that in the default
state setting that was removed, so good to keep that approach if
possible here.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>