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

[Jonathan Cameron]

> 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.
> 
> Signed-off-by: Janani Sunil <janani.sunil@xxxxxxxxxx>
>
A few things in here from sashiko as well. + one from me.

> diff --git a/drivers/iio/adc/ad7768.c b/drivers/iio/adc/ad7768.c
> index 802666f7abd4..9c6bebb1857f 100644
> --- a/drivers/iio/adc/ad7768.c
> +++ b/drivers/iio/adc/ad7768.c

>  
> +static int ad7768_get_convdelay_params(struct ad7768_state *st, unsigned int ch,
> +				       struct ad7768_convdelay_params *params)
> +{
> +	const struct ad7768_freq_config *freq_cfg;
> +	unsigned int dec_rate;
> +	unsigned int mclk_div;
> +	u64 mclk;
> +	u64 mult;
> +
> +	freq_cfg = ad7768_find_freq_config(st, st->power_mode_idx,
> +					   st->ch_freq[ch]);
> +	if (!freq_cfg)
> +		return -EINVAL;
> +
> +	dec_rate = ad7768_dec_rate[freq_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;
> +	}
> +
> +	mclk = clk_get_rate(st->mclk);
> +	if (!mclk)
> +		return -EINVAL;
> +
> +	mclk_div = ad7768_power_modes[st->power_mode_idx].mclk_div;
> +	params->step_ps =
> +		DIV_ROUND_CLOSEST_ULL(mult * PSEC_PER_SEC * mclk_div, mclk);

Sashiko:
[Severity: Low]
Is it safe to pass the 64-bit variable mclk as the divisor here?

The DIV_ROUND_CLOSEST_ULL() macro explicitly requires a 32-bit divisor.
Internally, it invokes do_div(), which casts the divisor to a 32-bit
integer.

While this might not cause a functional issue here because mclk is bounded
and fits safely within 32 bits, it violates the API contract and can
trigger static analysis warnings due to the implicit downcasting of the
u64 type.

-
Not a major thing but given the docs for DIV_ROUND_CLOSEST_ULL call
out that it 'must' be a 32 bit number, I'd add some checks to confirm
it is.

> +
> +	return 0;
> +}
> +
> +static int ad7768_set_channel_convdelay(struct ad7768_state *st,
> +					unsigned int ch)
> +{
> +	u64 delay_ps = st->ch_convdelay_ps[ch];
> +	struct ad7768_convdelay_params params;
> +	u64 max_delay_ps;
> +	u64 raw;
> +	int ret;
> +
> +	ret = ad7768_get_convdelay_params(st, ch, &params);
> +	if (ret)
> +		return ret;
> +
> +	max_delay_ps = (u64)params.max_raw * params.step_ps;
> +	if (delay_ps > max_delay_ps)
> +		return -EINVAL;

Add a comment here in why you need to do the check fist in
the step_ps multiplied space then again after division.

> +
> +	raw = DIV_ROUND_CLOSEST_ULL(delay_ps, params.step_ps);

Sashiko:

[Severity: Low]
Does this have the same type safety issue as above?

The params.step_ps struct member is declared as a u64, but is passed
to DIV_ROUND_CLOSEST_ULL() which requires a 32-bit divisor.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>