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

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