Re: [PATCH v5 09/10] iio: adc: ad4080: add driver support

[David Lechner <dlechner@baylibre.com>]

On 5/9/25 5:50 AM, Antoniu Miclaus wrote:
> Add support for AD4080 high-speed, low noise, low distortion,
> 20-bit, Easy Drive, successive approximation register (SAR)
> analog-to-digital converter (ADC).
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---

...

> +static const char *const ad4080_filter_type_iio_enum[] = {
> +	[FILTER_NONE]      = "none",
> +	[SINC_1]           = "sinc1",
> +	[SINC_5]           = "sinc5",
> +	[SINC_5_COMP]      = "sinc5+pf1",
> +};
> +
> +static const int ad4080_dec_rate_iio_enum[] = {

Would make more sense to call this _avail instead of _iio_enum.

> +	2, 4, 8, 16, 32, 64, 128, 256, 512, 1024,
> +};
> +
> +static const char * const ad4080_power_supplies[] = {
> +	"vdd33", "vdd11", "vddldo", "iovdd", "vrefin",
> +};
> +
> +struct ad4080_chip_info {
> +	const char *name;
> +	unsigned int product_id;
> +	int num_scales;
> +	const unsigned int (*scale_table)[2];
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
> +};
> +
> +struct ad4080_state {
> +	struct regmap			*regmap;
> +	struct clk			*clk;

clk isn't used outside of probe and can be removed.

> +	struct iio_backend		*back;
> +	const struct ad4080_chip_info	*info;
> +	/*
> +	 * Synchronize access to members the of driver state, and ensure
> +	 * atomicity of consecutive regmap operations.
> +	 */
> +	struct mutex			lock;
> +	unsigned int			num_lanes;
> +	unsigned int			dec_rate;
> +	unsigned long			clk_rate;
> +	enum ad4080_filter_type		filter_type;
> +	bool				lvds_cnv_en;
> +};
> +

...

> +static int ad4080_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long m)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	unsigned int dec_rate;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad4080_get_scale(st, val, val2);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		dec_rate = ad4080_get_dec_rate(indio_dev, chan);

Need to check return value for negative error code.

> +		if (st->filter_type == SINC_5_COMP)
> +			dec_rate *= 2;
> +		if (st->filter_type)
> +			*val = DIV_ROUND_CLOSEST(st->clk_rate, dec_rate);
> +		else
> +			*val = st->clk_rate;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = ad4080_get_dec_rate(indio_dev, chan);

Ditto.

Also, should set *val = 1 in the case of st->filter_type == FILTER_NONE.

> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4080_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:

This has no effect when st->filter_type == FILTER_NONE and should only
allow 1 in that case.

> +		return ad4080_set_dec_rate(indio_dev, chan, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

...

> +static int ad4080_set_filter_type(struct iio_dev *dev,
> +				  const struct iio_chan_spec *chan,
> +				  unsigned int mode)
> +{
> +	struct ad4080_state *st = iio_priv(dev);
> +	unsigned int dec_rate;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	dec_rate = ad4080_get_dec_rate(dev, chan);

Again, need to check for negative error code.

> +
> +	if (mode >= SINC_5 && dec_rate >= 512)
> +		return -EINVAL;
> +
> +	ret = iio_backend_filter_type_set(st->back, mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4080_REG_FILTER_CONFIG,
> +				 AD4080_FILTER_CONFIG_FILTER_SEL_MSK,
> +				 FIELD_PREP(AD4080_FILTER_CONFIG_FILTER_SEL_MSK,
> +					    mode));
> +	if (ret)
> +		return ret;
> +
> +	st->filter_type = mode;
> +
> +	return 0;
> +}
> +
> +static int ad4080_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:

Would make sense to have a separate case for st->filter_type == FILTER_NONE
simialar to suggestions above. 1 is the only available value in that case.

Also, in the st->filter_type != FILTER_NONE case, would make sense to set
*length based on st->filter_type since 256 is the max value for the sinc5
filters.

> +		*vals = ad4080_dec_rate_iio_enum;
> +		*length = ARRAY_SIZE(ad4080_dec_rate_iio_enum);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

...

> +
> +static int ad4080_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->regmap);
> +	unsigned int id;
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> +			   AD4080_INTERFACE_CONFIG_A_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> +			   AD4080_INTERFACE_CONFIG_A_SDO_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD4080_REG_CHIP_TYPE, &id);
> +	if (ret)
> +		return ret;
> +
> +	if (id != AD4080_CHIP_ID)
> +		dev_info(dev, "Unrecognized CHIP_ID 0x%X\n", id);
> +
> +	ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
> +			      AD4080_GPIO_CONFIG_A_GPO_1_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
> +			   FIELD_PREP(AD4080_GPIO_CONFIG_B_GPIO_1_SEL_MSK, 3));

Would be nice to have a macro or comment to explain what 3 is here so that
we don't have to look at the datasheet to figure out that this is configuring
the pin as "Filter Result Ready (Active Low)".

I would also expect to have some devictree bindings that describe that GPIO 1
on the ADC is connected to the sync_n input of the ad408x (AXI ADC) IP block.
Or at the very least, some comments here explaining that this is the assumed
default and if we ever add another backend or someone wants to wire up a part
a bit differently, then we might need to make this more generic.


> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> +	if (ret)
> +		return ret;
> +
> +	if (!st->lvds_cnv_en)
> +		return 0;
> +
> +	ret = regmap_update_bits(st->regmap,
> +				 AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +				 AD4080_ADC_DATA_INTF_CONFIG_B_LVDS_CNV_CLK_CNT_MSK,
> +				 FIELD_PREP(AD4080_ADC_DATA_INTF_CONFIG_B_LVDS_CNV_CLK_CNT_MSK, 7));

Similar here, it looks like the 7 here is the result of some calulation
based on the external CLK signal, So I would expec to see that caluclation
done in code using st->clk_rate or explain why hard-coding 7 is OK and
how that value was calculated.

> +	if (ret)
> +		return ret;
> +
> +	if (st->num_lanes > 1) {
> +		ret = regmap_set_bits(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				      AD4080_ADC_DATA_INTF_CONFIG_A_SPI_LVDS_LANES);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_set_bits(st->regmap,
> +			      AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +			      AD4080_ADC_DATA_INTF_CONFIG_B_LVDS_CNV_EN);
> +	if (ret)
> +		return ret;
> +
> +	return ad4080_lvds_sync_write(st);
> +}
> +