Re: [PATCH v4 05/14] iio: adc: Add AD7768 and AD7768-4 core support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Sat, Aug 22, 2026 at 12:30:05PM -0500, David Lechner wrote:
> On 8/21/26 9:06 AM, Janani Sunil wrote:

...

> > +static int ad7768_update_scan_mode(struct iio_dev *indio_dev,
> > +				   const unsigned long *scan_mask)
> > +{
> > +	struct ad7768_state *st = iio_priv(indio_dev);
> > +	unsigned int channel_mask;
> > +	unsigned int standby_mask;
> > +	int ret;
> > +
> > +	channel_mask = ad7768_all_standby_mask(st);
> > +	standby_mask = channel_mask;
> 
> Would be helpful to have some comments to explain why XTAL gets special
> handling so that we don't have to look at the datasheet. (I see something
> similar later in the patch too.)
> 
> > +	if (st->clock_source == AD7768_CLOCK_SOURCE_XTAL)
> > +		standby_mask &= ~BIT(st->chip_info->num_channels / 2);
> > +
> > +	for (unsigned int c = 0; c < st->chip_info->num_channels; c++) {
> > +		if (test_bit(c, scan_mask))
> > +			standby_mask &= ~BIT(c);
> > +	}

Make _mask:s to be unsigned long, and use direct operations on top, No need to
have for-loop I think.

> > +	ret = regmap_update_bits(st->regmap, AD7768_REG_CH_STANDBY,
> > +				 channel_mask, standby_mask);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (unsigned int c = 0; c < st->chip_info->num_channels; c++) {
> > +		if (test_bit(c, scan_mask))
> > +			ret = iio_backend_chan_enable(st->back, c);
> > +		else
> > +			ret = iio_backend_chan_disable(st->back, c);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}

...

> > +	/* ADC start-up time after reset: 1.66 ms max (datasheet Table 1) */
> > +	fsleep(2000);

2 * USEC_PER_MSEC

...

> > +	clock_name = ad7768_clock_names[st->clock_source];
> > +	if (st->clock_source == AD7768_CLOCK_SOURCE_LVDS)
> > +		st->mclk = devm_clk_get(dev, clock_name);
> 
> This needs an explanation why we don't enable the LVDS clock right away.
> 
> > +	else if (device_property_present(dev, "clock-names"))
> > +		st->mclk = devm_clk_get_enabled(dev, clock_name);
> > +	else
> > +		st->mclk = devm_clk_get_enabled(dev, NULL);
> 
> The NULL option should work in all cases since there should only be one clock.

I disagree on such an approach in general. We should not encourage NULL cases
for the clocks. Some drivers (IRL we have such cases) might have need more
clocks in the future and this becomes a problem. I think the NULL must be
simply dropped. Always require the named clock.

> > +	if (IS_ERR(st->mclk))
> > +		return dev_err_probe(dev, PTR_ERR(st->mclk),
> > +				     "Failed to get master clock\n");

...

> > +	/*
> > +	 * The datasheet does not specify a wake-up time. Allow 20 ms for the
> > +	 * ADC and digital clocks to restart.
> > +	 */
> > +	fsleep(20000);

20 * USEC_PER_MSEC (don't forget to include time.h for these multipliers)

-- 
With Best Regards,
Andy Shevchenko