Re: [PATCH v10 8/8] iio: adc: ad4851: add ad485x driver

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 17 Jan 2025 15:45:35 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/17/25 7:07 AM, Antoniu Miclaus wrote:
> > Add support for the AD485X a fully buffered, 8-channel simultaneous
> > sampling, 16/20-bit, 1 MSPS data acquisition system (DAS) with
> > differential, wide common-mode range inputs.
> > 
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---  
> 
> ...
> 
> > +static void __ad4851_get_scale(struct iio_dev *indio_dev, int scale_tbl,
> > +			       unsigned int *val, unsigned int *val2)
> > +{
> > +	const struct iio_scan_type *scan_type;
> > +	unsigned int tmp;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, &indio_dev->channels[0]);  
> 
> This is ignoring possible error return.
> 
> > +
> > +	tmp = ((u64)scale_tbl * MICRO) >> scan_type->realbits;
> > +	*val = tmp / MICRO;
> > +	*val2 = tmp % MICRO;
> > +}
> > +...  
> 
> > +static int ad4851_read_raw(struct iio_dev *indio_dev,
> > +			   const struct iio_chan_spec *chan,
> > +			   int *val, int *val2, long info)
> > +{
> > +	struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		*val = st->cnv_trigger_rate_hz / st->osr;  
> 
> Needs to be:
> 
> 		*val = st->cnv_trigger_rate_hz;
> 		*val2 = st->osr;
> 
> for IIO_VAL_FRACTIONAL.
> 
> > +		return IIO_VAL_FRACTIONAL;
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		return ad4851_get_calibscale(st, chan->channel, val, val2);
> > +	case IIO_CHAN_INFO_SCALE:
> > +		return ad4851_get_scale(indio_dev, chan, val, val2);
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		return ad4851_get_calibbias(st, chan->channel, val);
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		return ad4851_get_oversampling_ratio(st, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int ad4851_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int val, int val2, long info)
> > +{
> > +	struct ad4851_state *st = iio_priv(indio_dev);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		return ad4851_set_sampling_freq(st, val * st->osr);  
> 
> Since we are using IIO_VAL_FRACTIONAL on read, we should handle val2 here to
> allow for a decimal component.
> 
> 	val * st->osr + val2 * st->osr / MICRO
> 
> 
> Also, should return -EINVAL if (val < 0 || val2 < 0) to avoid negative values
> being turned into large numbers when casting to unsigned.
> 
> 
> > +	case IIO_CHAN_INFO_SCALE:
> > +		return ad4851_set_scale(indio_dev, chan, val, val2);
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		return ad4851_set_calibscale(st, chan->channel, val, val2);
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		return ad4851_set_calibbias(st, chan->channel, val);
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		return ad4851_set_oversampling_ratio(indio_dev, chan, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +  
> 
> ...
> 
> > +
> > +#define AD4851_IIO_CHANNEL(index, ch, diff)					\
> > +	.type = IIO_VOLTAGE,							\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
> > +		BIT(IIO_CHAN_INFO_CALIBBIAS) |					\
> > +		BIT(IIO_CHAN_INFO_SCALE),					\
> > +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),		\
> > +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
> > +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),				\
> > +	.info_mask_shared_by_all_available =					\
> > +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),				\
> > +	.indexed = 1,								\
> > +	.differential = diff,							\
> > +	.channel = ch,								\
> > +	.channel2 = ch,								\
> > +	.scan_index = index,  
> 
> channel, channel2 and scan_index get written over in ad4851_parse_channels()
> so can be omitted here.
> 
> > +
> > +/*
> > + * In case of AD4858_IIO_CHANNEL the scan_type is handled dynamically during the
> > + * parse_channels function.
> > + */
> > +#define AD4858_IIO_CHANNEL(index, ch, diff)					\
> > +{										\
> > +	AD4851_IIO_CHANNEL(index, ch, diff)					\
> > +}
> > +
> > +#define AD4857_IIO_CHANNEL(index, ch, diff)					\
> > +{										\
> > +	AD4851_IIO_CHANNEL(index, ch, diff)					\
> > +	.scan_type = {								\
> > +		.sign = 'u',							\
> > +		.realbits = 16,							\
> > +		.storagebits = 16,						\
> > +	},									\
> > +}
> > +
> > +static int ad4851_parse_channels(struct iio_dev *indio_dev,
> > +				 struct iio_chan_spec **ad4851_channels,
> > +				 const struct iio_chan_spec ad4851_chan,
> > +				 const struct iio_chan_spec ad4851_chan_diff)
> > +{
> > +	struct ad4851_state *st = iio_priv(indio_dev);
> > +	struct device *dev = &st->spi->dev;
> > +	struct iio_chan_spec *channels;
> > +	unsigned int num_channels, reg;
> > +	unsigned int index = 0;
> > +	int ret;
> > +
> > +	num_channels = device_get_child_node_count(dev);
> > +	if (num_channels > AD4851_MAX_CH_NR)
> > +		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> > +				     num_channels);
> > +
> > +	channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> > +	if (!channels)
> > +		return -ENOMEM;
> > +
> > +	indio_dev->channels = channels;
> > +	indio_dev->num_channels = num_channels;
> > +
> > +	device_for_each_child_node_scoped(dev, child) {
> > +		ret = fwnode_property_read_u32(child, "reg", &reg);
> > +		if (ret || reg >= AD4851_MAX_CH_NR)
> > +			return dev_err_probe(dev, ret,
> > +					     "Missing/Invalid channel number\n");  
> 
> This allows returning 0 on error which will lead to an invalid state. Probably
> best to split this into two messages/returns.
> 
> > +		if (fwnode_property_present(child, "diff-channels")) {
> > +			*channels = ad4851_chan_diff;
> > +			channels->scan_index = index++;
> > +			channels->channel = reg;
> > +			channels->channel2 = reg;  
> 
> Typically we don't set channel == channel2 for differential channels.
So i guess this is tripping up on these being dedicated pairs labelled
+IN1,-IN1 on the datasheet.  The binding documents those as matching
the diff-channels - hence both channels and reg are the same.
So maybe best bet is to enforce that in the driver by checking it is
true.

It is a slightly weird description but only alternative would be to
invent some more channel numbers for the negative sides which is
less than ideal.  We could go that way though.

Some comments alongside a sanity check is probably the best way to
handle this and no surprise us in the future.

> 
> > +
> > +		} else {
> > +			*channels = ad4851_chan;
> > +			channels->scan_index = index++;
> > +			channels->channel = reg;  
> 
> These two lines are the same in each branch, so can be pulled out of the if
> statement to avoid duplicating them.
> 
> > +		}
> > +		channels++;
> > +
> > +		st->bipolar_ch[reg] = fwnode_property_read_bool(child, "bipolar");
> > +
> > +		if (st->bipolar_ch[reg]) {
> > +			channels->scan_type.sign = 's';
> > +		} else {
> > +			ret = regmap_write(st->regmap, AD4851_REG_CHX_SOFTSPAN(reg),
> > +					   AD4851_SOFTSPAN_0V_40V);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +	}
> > +
> > +	*ad4851_channels = channels;  
> 
> At this point, channels is pointing to memory we didn't allocate (because of
> channels++). As in the previous review, I suggest we just get rid of the output
> parameter since indio_dev->channels already has the correct pointer.
> 
> It's less chance for mistakes like this and avoids needing to provide an unused
> arg in ad4857_parse_channels().
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad4857_parse_channels(struct iio_dev *indio_dev)
> > +{
> > +	struct iio_chan_spec *ad4851_channels;
> > +	const struct iio_chan_spec ad4851_chan = AD4857_IIO_CHANNEL(0, 0, 0);
> > +	const struct iio_chan_spec ad4851_chan_diff = AD4857_IIO_CHANNEL(0, 0, 1);  
> 
> The only difference between these is the diff bit, so seems simpler to just
> have one template and dynamically set the bit instead.
> 
> > +
> > +	return ad4851_parse_channels(indio_dev, &ad4851_channels, ad4851_chan,
> > +				     ad4851_chan_diff);
> > +}
> > +...  
> 
> > +static const struct iio_info ad4851_iio_info = {
> > +	.debugfs_reg_access = ad4851_reg_access,
> > +	.read_raw = ad4851_read_raw,
> > +	.write_raw = ad4851_write_raw,
> > +	.update_scan_mode = ad4851_update_scan_mode,
> > +	.get_current_scan_type = &ad4851_get_current_scan_type,  
> 
> Inconsistent use of & on function pointer.
> 
> > +	.read_avail = ad4851_read_avail,
> > +};
> > +