Re: [PATCH v7 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>

This is just highlighting some feedback from sashiko.
I have very little idea what it is talking about!

If it is garbage, just reply to say so.

Thanks,

Jonathan

>
> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> index 44ba19fb2096..be815b8c6766 100644
> --- a/drivers/iio/adc/ad7768.c
> +++ b/drivers/iio/adc/ad7768.c
> @@ -41,6 +41,8 @@
>  #define AD7768_REG_POWER_MODE				0x04
>  #define   AD7768_SLEEP_MODE_MSK				BIT(7)
>  #define   AD7768_POWER_MODE_POWER_MODE_MSK		GENMASK(5, 4)
> +#define     AD7768_POWER_MODE_POWER_MODE_LOW		0x0
> +#define     AD7768_POWER_MODE_POWER_MODE_MEDIAN		0x2
>  #define     AD7768_POWER_MODE_POWER_MODE_FAST		0x3
>  #define   AD7768_POWER_MODE_LVDS_ENABLE			BIT(3)
>  #define   AD7768_POWER_MODE_MCLK_DIV_MSK		GENMASK(1, 0)
> @@ -102,6 +104,17 @@ enum ad7768_clock_source {
>  	AD7768_CLOCK_SOURCE_LVDS,
>  };
>  
> +struct ad7768_power_mode_info {
> +	unsigned int mode;
> +	unsigned int mclk_div;
> +};
> +
> +static const struct ad7768_power_mode_info ad7768_power_modes[] = {
> +	{ .mode = AD7768_POWER_MODE_POWER_MODE_LOW, .mclk_div = 32 },
> +	{ .mode = AD7768_POWER_MODE_POWER_MODE_MEDIAN, .mclk_div = 8 },
> +	{ .mode = AD7768_POWER_MODE_POWER_MODE_FAST, .mclk_div = 4 },
> +};
> +
>  struct ad7768_precharge_config {
>  	bool prebufp_en;
>  	bool prebufn_en;
> @@ -127,6 +140,7 @@ struct ad7768_state {
>  	const struct ad7768_chip_info *chip_info;
>  	struct iio_backend *back;
>  	unsigned int vref_uV[2];
> +	unsigned int power_mode_idx;
>  
>  	__be16 d16 __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -330,6 +344,32 @@ static int ad7768_sync(struct ad7768_state *st)
>  			       AD7768_DATA_CONTROL_SPI_SYNC);
>  }
>  
> +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.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>