Re: [PATCH v2 5/5] iio: adc: ad7625: add LTC2387 and ADAQ2387x support

[Nicolae-Daniel Deaconescu]

Hi Andy,

Thanks for the review. v3 is out with your comments addressed:
https://lore.kernel.org/all/20261106160529.876251-1-Nicolae-daniel.Deaconescu@xxxxxxxxxx/

My answers are inline. Patch numbers are those of v3.

On Wed, Nov 04, 2026 at 10:22:07PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 04, 2026 at 03:21:43PM +0200, Nicolae-Daniel Deaconescu wrote:
>
> >  config AD7625
> > -	tristate "Analog Devices AD7625/AD7626 High Speed ADC driver"
> > +	tristate "Analog Devices AD762x, AD796x, LTC2387 and ADAQ2387x ADC driver"
>
> This does not scale. Please, make it more generic and make sure the text in the
> top comment of the driver along with MODULE_DESCRIPTION() is quite similar to
> this (maybe without "driver" word).

Done. The Kconfig prompt, the comment at the top of the driver and
MODULE_DESCRIPTION() now all describe it as a high speed LVDS ADC.

>
> > +	  * LTC2387-18 18-Bit, 15 MSPS SAR Analog-to-Digital Converter
> > +	  * ADAQ23875 and ADAQ23876 16-Bit, 15 MSPS uModule Data Acquisition
> > +	    Systems
> > +	  * ADAQ23878 18-Bit, 15 MSPS uModule Data Acquisition System
>
> Why not keep this list sorted by a part number?

Fixed, the list is now sorted by part number in Kconfig, and in
Documentation/iio/ad7625.rst as well.

>
> >  	const unsigned int max_sample_freq_hz;
> >  	const bool has_power_down_state;
> >  	const bool has_bandwidth_control;
> >  	const bool has_internal_vref;
>
> Side note: What the point in const to these (plain booleans, integers)?
> Chip info as a whole should be const, no?

I left the existing members alone to keep the series about the new parts.
The members I add don't use const.

>
> > +	 * They are the parts with @ddr_timing set.
> > +	 */
> > +	const struct ad7625_ddr_timing *ddr_timing;
> > +	const char * const *supplies;
>
> Again, why the second const here...

That const is on the strings, not on the member: supplies points to an
array declared as 'static const char * const x[]'. Without it the
assignment discards a qualifier, and a non-const array gets a warning from
checkpatch.

>
> > +	int num_supplies;
> > +	/* Gain * 1000 of the ADAQ front end, 0 if the board has to provide it. */
> > +	const u16 default_gain_milli;
>
> ...and the const here?

Dropped. The field is a plain u16 default_gain now.

>
> milli is multiplier and not a unit. But if you think it's better this way, I am
> not going to object.

The C names are now gain and default_gain, set with 1 * MILLI. The 
devicetree property keeps the name adi,gain-milli, as ad4000 and
ad7380 already use it.

>
> > +static const char * const adaq2387x_supplies[] = {
> > +	"vdd", "vio", "vs-p", "vs-n"
>
> When not on a single line, add a trailing comma.

Fixed

>
> > +static const struct ad7625_chip_info ltc2387_16_chip_info = {
> > +	.name = "ltc2387-16",
> > +	.max_sample_freq_hz = 15 * MEGA,
>
> No, we have HZ_PER_MHZ for this.

Fixed

>
> > +	.ddr_timing = &ltc2387_ddr_timing,
> > +	.supplies = ltc2387_supplies,
> > +	.num_supplies = ARRAY_SIZE(ltc2387_supplies),
> > +	.default_gain_milli = 1000,
>
> As I mentioned above this multiplier without unit is confusing.
> Simply
>
> 	.default_gain = 1 * MILLI,
>
> ?

Fixed, for all the added chip infos.

>
> > +
> > +	ret = pwm_round_waveform_might_sleep(st->clk_gate_pwm, &clk_gate_wf);
> > +	if (ret)
> > +		return ret < 0 ? ret : -EINVAL;
>
> Don't nest branches, you can just move ret == 0 check out of that in both cases.

Done in both rounding checks: 
'if (ret < 0) return ret; if (ret) return -EINVAL;'

>
> > +static int ad7625_parse_ddr(struct device *dev, struct ad7625_state *st)
> > +{
> > +	int ret, i;
>
> Why is 'i' signed and why can't it be defined inside for-loop?

Fixed

>
> > +
> > +	ret = device_property_read_u16(dev, "adi,gain-milli", &st->gain_milli);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to read adi,gain-milli\n");
>
> 	const char *propname;
> 	...
> 	propname = "adi,gain-milli";
>
> 	// Why do we even need the presence check?
> 	if (!device_property_present(dev, propname))
> 		return dev_err_probe(dev, -EINVAL, "%s must be set\n", propname);
>
> 	ret = device_property_read_u16(dev, propname, &st->gain_milli);
> 	if (ret)
> 		return dev_err_probe(dev, ret, "failed to read %s\n", propname);

I dropped the presence check, as reading a missing property fails anyway
with 'failed to read adi,gain-milli'.

>
> > +		return dev_err_probe(dev, ret, "failed to enable supplies\n");
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	if (ret < 0 && ret != -ENODEV)
>
> Too many ENODEV checks, see below how to deduplicate.

Reworked with early returns: -ENODEV selects the internal reference, then
an error and a 0 V reference are checked on their own. I did not use the
else-if chain, as checkpatch warns about else after return.

>
> > -
> > -		if (ret)
> > -			return dev_err_probe(dev, -EINVAL,
> > -					     "failed to set EN pins\n");
>
> AFAICS the above can be moved to a helper in the separate patch and moved as a
> couple of lines only in this patch.

Done. Patch 5 only moves the code into ad7625_set_initial_mode() and the
patch with the new parts adds an early return to it.

>
> > +		/* Without the op, a backend can only be left on one lane. */
> > +		if (ret && (ret != -EOPNOTSUPP || st->two_lanes))
> > +			return dev_err_probe(dev, ret, "failed to set the number of lanes\n");
> > +	}
>
> Same problem, too many (hidden) branches makes code unreadable.

Fixed

>
> > +	/* The DDR parts reject rates above the clock of the PWMs. */
> > +	if (st->info->ddr_timing)
> > +		default_sample_freq = min_t(u32, default_sample_freq, st->step.clk_rate_hz);
>
> No min_t().

Switched to min().

Thanks,
Daniel