Re: [PATCH v4 08/14] iio: adc: ad7768: Add per-channel conversion delay

[Janani Sunil]

On 8/24/26 09:10, Andy Shevchenko wrote:
> On Fri, Aug 21, 2026 at 04:07:01PM +0200, Janani Sunil wrote:
> > Expose the per-channel synchronization phase offset through the IIO
> > conversion-delay attribute.
> >
> > Derive the delay resolution from MCLK, power mode and decimation rate.
> > Validate the requested delay and program the corresponding phase
> > register when applying the active channel configuration.
> ...
>
> > +static int ad7768_get_convdelay_params(struct ad7768_state *st, unsigned int ch,
> > +				       struct ad7768_convdelay_params *params)
> > +{
> > +	struct ad7768_freq_config f_cfg;
> > +	unsigned int dec_rate;
> > +	unsigned int mclk_div;
> > +	unsigned int mult;
> > +	u64 mclk;
> > +	int ret;
> > +
> > +	ret = ad7768_get_freq_cfg(st, st->ch_freq[ch], &f_cfg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dec_rate = ad7768_dec_rate[f_cfg.dec_rate];
> > +	switch (dec_rate) {
> > +	case 32:
> > +		params->shift = 3;
> > +		params->max_raw = 31;
> > +		mult = 1;
> > +		break;
> > +	case 64:
> > +		params->shift = 2;
> > +		params->max_raw = 63;
> > +		mult = 1;
> > +		break;
> > +	case 128:
> > +		params->shift = 1;
> > +		params->max_raw = 127;
> > +		mult = 1;
> > +		break;
> > +	case 256:
> > +		params->shift = 0;
> > +		params->max_raw = 255;
> > +		mult = 1;
> > +		break;
> > +	case 512:
> > +		params->shift = 0;
> > +		params->max_raw = 255;
> > +		mult = 2;
> > +		break;
> > +	case 1024:
> > +		params->shift = 0;
> > +		params->max_raw = 255;
> > +		mult = 4;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> So, this is just a bit twiddling of the dec_rate.
>
> Can be written like
>
> 	/* Optional, perhaps shouldn't appear */
> 	if (!dec_rate)
> 		return -EINVAL;
>
> 	/* Same? What does table allow to have? */
> 	if (!is_power_of_two(dec_rate))
> 		return -EINVAL;
>
> 	mult = ilog2(dec_rate);
> 	if (mult > 10) {
> 		return -EINVAL;
> 	} else if (mult > 8) {
> 		params->shift = 0;
> 		params->max_raw = 255;
> 		mult = BIT(mult - 8);
> 	} else if (mult > 5) {
> 		params->shift = 8 - mult;
> 		params->max_raw = dec_rate - 1;
> 		mult = 1;
> 	} else {
> 		return -EINVAL;
> 	}
>
> TBH, I don't know which looks easier to read. It all depends on what datasheet
> says about these parameters and multiplier and what the table allows to have.
I kept the explicit switch case because it the device supports only the 6 discrete decimation ratios listed in the table 32.
The switch directly mirrors the phase resolution, number of steps and register bit mappings from that table, which I find easier to verify against the datasheet.

The arithmetic version is more compact, but the proposed conditions would reject the valid x32 case because ilog2(32) is 5. A corrected arithmetic version would be functionally equivalent, but I do not think it improves the readability here.

Regards,
Jan