Re: [PATCH v3 2/2] iio: adc: ad4130: add AD4130 driver

[Jonathan Cameron <jic23@xxxxxxxxxx>]

> >   
> >> +static int ad4130_set_fifo_watermark(struct iio_dev *indio_dev, unsigned int val)
> >> +{
> >> +	struct ad4130_state *st = iio_priv(indio_dev);
> >> +	unsigned int eff;
> >> +	int ret;
> >> +
> >> +	if (val > AD4130_FIFO_SIZE)
> >> +		return -EINVAL;
> >> +
> >> +	/*
> >> +	 * Always set watermark to a multiple of the number of enabled channels
> >> +	 * to avoid making the FIFO unaligned.
> >> +	 */
> >> +	eff = rounddown(val, st->num_enabled_channels);
> >> +
> >> +	mutex_lock(&st->lock);
> >> +
> >> +	ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> >> +				 AD4130_WATERMARK_MASK,
> >> +				 FIELD_PREP(AD4130_WATERMARK_MASK,
> >> +					    ad4130_watermark_reg_val(eff)));
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	st->effective_watermark = eff;
> >> +	st->watermark = val;  
> > 
> > Hmm this is a potential inconsistency in the IIO ABI.
> > ABI docs describes watermark as being number of 'scan elements' which is
> > not the clearest text we could have gone with...
> > 
> > Now I may well have made a mistake in the following as it's rather a long time
> > since I last looked at the core handling for this...
> > 
> > The core treats it as number datum (which is same as a scan) when using
> > it for the main watermark attribute and also when using watermarks with the
> > kfifo (the IIO fifo is made up of objects each of which is a scan. So kfifo_len()
> > returns the number of scans.
> >   
> > Looking very quickly at a few other drivers
> > adxl367 seems to use number of samples.
> > adxl372 is using number of scans.
> > bmc150 hardware seems to work on basis of frame count which I 'think' is probably scans.
> > fxls8962 uses 'samples count' which is not clearly defined in the datasheet but there
> > is an example showing that it's scans (I think)...
> > lsm6dsx - some of the fifos used with this are based on tagged data so the connection to
> > what hits the front end buffers is non obvious.
> > 
> > So, not great for consistency :(
> > 
> > Going forwards i think we should standardize the hardware fifo watermark on what is being
> > used for the software watermark which I believe is number of scans.
> > Not necessary much we can do about old drivers though due to risk of breaking ABI...
> > We should make the documentation clearer though.
> >   
> 
> I was confused too, but this seemed more logical to me at the time, and
> since you didn't say anything regarding it on ADXL367, I did it the same
> way here. I guess we can't go back and change it now on ADXL367, I'm
> sorry for this. I'll fix it.

I missed it.  Review is never perfect (mine definitely aren't!)

Thinking more on the adxl367. We still have a window to  fix that as
the driver isn't yet in a release kernel.  Would you mind spinning a
patch to fix that one?  Even if we miss the rc cycle (it's a bit tight
timing wise) we can sneak it in as an early fix in stable without
significant risk of breaking anyone's userspace.

There might be other drivers that have that interpretation we can't
fix but if we can reduce the scope of the problem by changing the adxl367
that would be great.

We should also definitely improve the docs and perhaps add a note to say
that due to need to maintain ABI, a few drivers use scans * number of channels
rather than scans.

> 
> >> +
> >> +out:
> >> +	mutex_unlock(&st->lock);
> >> +
> >> +	return ret;
> >> +}
> >> +  
> > 
> > 
> > ...
> >   
> >> +
> >> +static int ad4130_parse_fw_channel(struct iio_dev *indio_dev,
> >> +				   struct fwnode_handle *child)
> >> +{
> >> +	struct ad4130_state *st = iio_priv(indio_dev);
> >> +	unsigned int index = indio_dev->num_channels++;
> >> +	struct device *dev = &st->spi->dev;
> >> +	struct ad4130_chan_info *chan_info;
> >> +	struct iio_chan_spec *chan;
> >> +	u32 pins[2];
> >> +	int ret;
> >> +
> >> +	if (index >= AD4130_MAX_CHANNELS)
> >> +		return dev_err_probe(dev, -EINVAL, "Too many channels\n");
> >> +
> >> +	chan = &st->chans[index];
> >> +	chan_info = &st->chans_info[index];
> >> +
> >> +	*chan = ad4130_channel_template;
> >> +	chan->scan_type.realbits = st->chip_info->resolution;
> >> +	chan->scan_type.storagebits = st->chip_info->resolution;
> >> +	chan->scan_index = index;
> >> +
> >> +	chan_info->slot = AD4130_INVALID_SLOT;
> >> +	chan_info->setup.fs = AD4130_FS_MIN;
> >> +	chan_info->initialized = true;
> >> +
> >> +	ret = fwnode_property_read_u32_array(child, "diff-channels", pins,
> >> +					     ARRAY_SIZE(pins));
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = ad4130_validate_diff_channels(st, pins, ARRAY_SIZE(pins));
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	chan->channel = pins[0];
> >> +	chan->channel2 = pins[1];
> >> +
> >> +	ret = ad4130_parse_fw_setup(st, child, &chan_info->setup);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	fwnode_property_read_u32(child, "adi,excitation-pin-0",
> >> +				 &chan_info->iout0);
> >> +	if (chan_info->setup.iout0_val != AD4130_IOUT_OFF) {  
> > 
> > It would be slightly better to set an explicit default value here as the fact it
> > is 0 is hidden by the enum. e.g.
> > 	chan_info->iout0 = AD4130_IOUT_OFF;
> > 	fwnode_property_read_u32(child, "adi,excitation-pin-0",
> > 			 	 &chan_info->iout0);
> > 	if (chan_info->....
> > That would save reviewers wondering what the default is and having to go
> > check the enum (and I'm lazy :)  
> 
> I understand the idea, but the default value for iout0 is not
> AD4130_IOUT_OFF. iout0 is the pin that iout0_val current is
> applied to, and AD4130_IOUT_OFF is a value for iout0_val.
> Look at ad4130_parse_fw_setup.
> 
> For iout0, I guess I could do
> #define AD4130_AIN0	0x0
> ...
> chan_info->iout0 = AD4130_AIN0;

Oops.  I got confused.  Code is fine as it is.  Adding the define isn't going to make
it much clearer.
 
> 
> >> +		}
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +