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

[Janani Sunil]

On 9/13/26 22:51, Jonathan Cameron wrote:
> > +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 |
> > +				 AD7768_POWER_MODE_MCLK_DIV_MSK,
> > +				 FIELD_PREP(AD7768_POWER_MODE_POWER_MODE_MSK,
> > +					    mode_info->mode) |
> > +				 FIELD_PREP(AD7768_POWER_MODE_MCLK_DIV_MSK,
> > +					    mode_info->mode));
> Sashiko:
> [Severity: High]
> When configuring MEDIAN mode via ad7768_set_power_mode(), does this code
> inadvertently configure the MCLK_DIV bitfield using the power mode
> register value instead of the correct clock divider register code?
>
> For MEDIAN power mode, mode_info->mode is 0x2. Writing 0x2 to the MCLK_DIV
> bitfield sets the hardware divider to 4 (MCLK/4), instead of the expected
> divider of 8 (MCLK/8, which requires register value 0x1).
>
> Since ad7768_power_modes correctly defines mclk_div = 8 for MEDIAN mode, the
> software will calculate output rates and delays based on a divider of 8, while
> the hardware runs twice as fast.
> -
> I haven't looked into this one so please check it out.

Both Median power mode and its corresponding MCLK divider use 0b10 register encoding. So using mode_info->mode for both the fields is intentional and logically right.

Best Regards,
Jan