Re: [PATCH v3 7/7] iio: adc: ad7625: add LTC2387 and ADAQ2387x support

[Nicolae-Daniel Deaconescu <Nicolae-daniel.Deaconescu@analog.com>]

Hi Jonathan,

On Sat, 7 Nov 2026 02:20:06 +0000, Jonathan Cameron wrote:
>
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > Add the LTC2387-16, LTC2387-18, ADAQ23875, ADAQ23876 and ADAQ23878 as chip
> > data of this driver. The same two PWMs and IIO backend read them. Chip data
>
> PWMs don't tend to read anything. Rephrase as something like 
>
> "The same two PWMs and IIO backend are used to read from them."

Will fix in v4: "The same two PWMs and IIO backend are used to read from
them."

>
> > and a few quirks cover the differences, see the binding patch for the list:
>
> The 'quirks' here concerns me.  We want everything to be covered
> by chip data even if some of it is 'apply quirk' :)

Agreed. It is the same point as the scale: ddr_timing stands in for "the
new parts" in several places. In v4 each of those will get its own chip
info member (gain for the scale, EN pins, second PWM name, supplies), and
ddr_timing will stay for the DDR timing, PWM step and lane code.

>
> > - Set the backend number of lanes for the new parts. Skipping is allowed
> >   only if the backend can't set it and one lane is used.
> > - Limit the default sampling frequency of the new parts to the PWM clock
> >   rate.
> > - Skip the EN pin mode setup for the new parts, as they have no EN pins.
> > - Include math64.h, minmax.h, property.h and types.h for the new code.
> > - Add the new parts, sorted by part number, to the Kconfig help and
> >   Documentation/iio/ad7625.rst.
> > - Make the Kconfig prompt, the top comment of the driver and
> >   MODULE_DESCRIPTION() generic, as the driver supports more than the
> >   AD7625.
> > - Add new maintainer entries.
>
> That is a massive list that is more or less describing the patch.  Trim
> it down to just those bits that are non obvious or need to be highlighted.
> No need to mention everything.  I'm going to guess perhaps an LLM was involved?
> If so they need poking to avoid doing this sort of list.

Agreed, I'll cut it down in v4 to what needs highlighting. An LLM helped
draft it, as the cover letter says, and I'll keep it from listing the whole
patch.

>
> > @@ -4,8 +4,9 @@
> >  AD7625 driver
> >  ====================
> > -ADC driver for Analog Devices Inc. AD7625, AD7626, AD7960, and AD7961
> > -devices. The module name is ``ad7625``.
> > +ADC driver for Analog Devices Inc. AD7625, AD7626, AD7960, AD7961, LTC2387-16,
> > +LTC2387-18, ADAQ23875, ADAQ23876 and ADAQ23878 devices. The module name is
> > +``ad7625``.
>
> The next section says what is supproted. I'd be tempted to avoid
> repeating that here in a fashion that is churn heavy.  Use something like
> 'and fairly similar' to avoid naming more than the one the driver is named after.

I'll change it in v4 to: "ADC driver for Analog Devices Inc. AD7625 and
fairly similar devices. The module name is ``ad7625``."

>
> > +	st->step = step;
> > +
> > +	if (!freq || freq > st->info->max_sample_freq_hz ||
> > +	    freq > st->step.clk_rate_Hz)
> > +		return -EINVAL;
> > +
> > +	cycles_per_step = st->step.clk_cycles_per_step;
> > +
> > +	/* Everything but lastclk and burst_end is in steps of the PWMs. */
> > +	gate = ad7625_ddr_burst_pulses(st) / cycles_per_step;
> > +	cnvh = ad7625_ddr_ns_to_cycles(st, t->cnv_high_min_ns);
> > +	cnvh = max(ad7625_ddr_cycles_to_steps(st, cnvh), 1U);
>
> Similar to offset.
> > +	offset = ad7625_ddr_ns_to_cycles(st, t->first_clk_min_ns);
>
> Maybe use a different local variable for offset_cycles just to avoid
> the units of the variable changing in two lines.  

I'll do this in v4, for offset and cnvh:

	offset_cycles = ad7625_ddr_ns_to_cycles(st, t->first_clk_min_ns);
	offset = ad7625_ddr_cycles_to_steps(st, offset_cycles);

>
> > +	 * CNV and may run into the next period, but has to end within
> > +	 * t_LASTCLK of the next CNV. It must also leave the gate low for at
> > +	 * least one step per period.
> > +	 */
> > +	burst_end = (offset + gate) * cycles_per_step;
> > +	min_period = burst_end > lastclk ?
> > +		     DIV_ROUND_UP(burst_end - lastclk, cycles_per_step) : 0;
> > +	period = DIV_ROUND_UP(st->step.clk_rate_Hz, freq);
> > +	period = max3(DIV_ROUND_UP(period, cycles_per_step), gate + 1, min_period);
> > +
> > +	cnv_wf.period_length_ns = ad7625_ddr_steps_to_ns(st, period);
> > +	cnv_wf.duty_length_ns = ad7625_ddr_steps_to_ns(st, cnvh);
>
> Perhaps set the whole thing down here with 
> 	cnv_wf = (struct pwm_waveform) {
> 		.period_length_ns = ....
> 		.duty_lenght_ns = ...
> 	};
> rather that clear it then fill stuff in down here.
> Maybe also explicitly set the offset to 0 to show it's the baseline
> (I assume) for the other one that follows?
>
> > +
> > +	clk_gate_wf.period_length_ns = cnv_wf.period_length_ns;
> > +	clk_gate_wf.duty_length_ns = ad7625_ddr_steps_to_ns(st, gate);
> > +	clk_gate_wf.duty_offset_ns = ad7625_ddr_steps_to_ns(st, offset % period);
>
> Similar for this one.

Yes, offset 0 is the baseline the clk gate offset is relative to. I'll set
it down there in v4:

	cnv_wf = (struct pwm_waveform) {
		.period_length_ns = ad7625_ddr_steps_to_ns(st, period),
		.duty_length_ns = ad7625_ddr_steps_to_ns(st, cnvh),
		.duty_offset_ns = 0,
	};
	clk_gate_wf = (struct pwm_waveform) {
		.period_length_ns = cnv_wf.period_length_ns,
		.duty_length_ns = ad7625_ddr_steps_to_ns(st, gate),
		.duty_offset_ns = ad7625_ddr_steps_to_ns(st, offset % period),
	};

>
> >  	unsigned long target;
> >  	int ret;
> > +	if (st->info->ddr_timing)
> > +		return ad7625_set_sampling_freq_ddr(st, freq);
> > +
> >  	if (!freq)
> >  		return -EINVAL;
> > @@ -239,6 +469,14 @@ static int ad7625_read_raw(struct iio_dev *indio_dev,
> >  		return IIO_VAL_INT;
> >  	case IIO_CHAN_INFO_SCALE:
> > +		if (st->info->ddr_timing) {
> > +			/* Referred to the input of the ADAQ front end, so divided by its gain. */
>
> Why does something called DDR timing have anything to do with the scaling
> here?  Is it being used as a proxy for parts that happen to have some other
> characteristic?

It is a proxy for "the new parts", see above. In v4 the scale will use a
chip info member for the gain.

>
> > +	ret = devm_regulator_get_enable_optional(dev, "vs-n");
> > +	if (ret && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "failed to enable VS-\n");
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	if (ret == -ENODEV) {
> > +		/* Without ref-supply the internal reference is used. */
> > +		st->vref_mV = AD7625_INTERNAL_REF_MV;
> > +		return 0;
> > +	}
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
> > +	if (ret == 0)
> > +		return dev_err_probe(dev, -EINVAL, "REF voltage must not be 0\n");
>
> Obviously it's nonsense if it is 0, but why do we exclude 0 but not 1 which also
> ends up being scaled to 0?  The function succeeded so I'm not sure we need
> to check for 0 explicitly unless it indicates some other failure path I'm not
> thinking of.

Agreed, I'll drop it in v4. The call succeeding is enough.

>
> > +			return dev_err_probe(dev, ret, "failed to set the number of lanes\n");
> > +	}
> > +
> > @@ -699,6 +1047,6 @@ static struct platform_driver ad7625_driver = {
> >  module_platform_driver(ad7625_driver);
> >  MODULE_AUTHOR("Trevor Gamblin <tgamblin@baylibre.com>");
> > -MODULE_DESCRIPTION("Analog Devices AD7625 ADC");
> > +MODULE_DESCRIPTION("Analog Devices high speed LVDS ADC");
>
> We tend to keep some part indication of the first part supported
> in these because at somepoint I'm sure another LVDS part from analog
> will come along that is too different to roll in here.  Designers
> are a creative bunch and tend to eventually manage to make generic
> drivers impossible.
>
> "Analog Devices high speed LVDS ADCs similar to the AD7625"
> or something like that.

Will do, and I'll use the same wording for the Kconfig prompt and the top
comment.

Thanks,
Daniel