Message-ID: <5d932a4a-790e-ca95-c5de-c2267e1f365c@gmail.com>
Date:   Mon, 2 May 2022 11:53:31 +0300
From:   Cosmin Tanislav <demonsingur@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-iio@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH v3 2/2] iio: adc: ad4130: add AD4130 driver



On 5/1/22 19:08, Jonathan Cameron wrote:
> On Tue, 19 Apr 2022 18:08:28 +0300
> Cosmin Tanislav <demonsingur@...il.com> wrote:
> 
>> AD4130-8 is an ultra-low power, high precision, measurement solution for
>> low bandwidth battery operated applications.
>>
>> The fully integrated AFE (Analog Front-End) includes a multiplexer for up
>> to 16 single-ended or 8 differential inputs, PGA (Programmable Gain
>> Amplifier), 24-bit Sigma-Delta ADC, on-chip reference and oscillator,
>> selectable filter options, smart sequencer, sensor biasing and excitation
>> options, diagnostics, and a FIFO buffer.
>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@...log.com>
> 
> Hi Cosmin,
> 
> Sorry this one took me a while to get back to.  Last week was a little
> crazy on the day job front.
> 
> Anyhow, whilst reading this I realised we have some unclear ABI documentation
> and hence divergence in interpretation of the units of hwfifo_watermark.
> See below. I've not done a thorough audit as don't have enough time today,
> but we have at least some drivers treating it as being in scans (+ the core
> does this for watermark) and some treating it as being in individual channel
> readings...  My intent IIRC was that it was in scans as otherwise you run into
> the problem you are working around with needing to tweak it to match scans
> within each driver.
> 
> A few other trivial things inline including the alignment issue I emailed
> the list about but haven't sent patches out for yet (about 90 driver are
> affected... *sigh*).
> 
> Thanks,
> 
> Jonathan
>  >
>> ---
> 
> ...
> 
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 39d806f6d457..c3aa7e4eca2f 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -6,6 +6,7 @@
>>   # When adding new entries keep the list in alphabetical order
>>   obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>>   obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>> +obj-$(CONFIG_AD4130) += ad4130.o
>>   obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o
>>   obj-$(CONFIG_AD7124) += ad7124.o
>>   obj-$(CONFIG_AD7192) += ad7192.o
>> diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
>> new file mode 100644
>> index 000000000000..dd2f18734cba
>> --- /dev/null
>> +++ b/drivers/iio/adc/ad4130.c
> 
> ...
> 
>> +struct ad4130_state {
>> +	const struct ad4130_chip_info	*chip_info;
>> +	struct spi_device		*spi;
>> +	struct regmap			*regmap;
>> +	struct clk			*mclk;
>> +	struct regulator_bulk_data	regulators[4];
>> +	u32				irq_trigger;
>> +	u32				inv_irq_trigger;
>> +
>> +	/*
>> +	 * Synchronize access to members of driver state, and ensure atomicity
>> +	 * of consecutive regmap operations.
>> +	 */
>> +	struct mutex			lock;
>> +	struct completion		completion;
>> +
>> +	struct iio_chan_spec		chans[AD4130_MAX_CHANNELS];
>> +	struct ad4130_chan_info		chans_info[AD4130_MAX_CHANNELS];
>> +	struct ad4130_slot_info		slots_info[AD4130_MAX_SETUPS];
>> +	enum ad4130_pin_function	pins_fn[AD4130_MAX_ANALOG_PINS];
>> +	u32				vbias_pins[AD4130_MAX_ANALOG_PINS];
>> +	u32				num_vbias_pins;
>> +	int				scale_tbls[AD4130_REF_SEL_MAX][AD4130_PGA_NUM][2];
>> +	struct gpio_chip		gc;
>> +	unsigned int			gpio_offsets[AD4130_MAX_GPIOS];
>> +	unsigned int			num_gpios;
>> +
>> +	u32			int_pin_sel;
>> +	u32			int_ref_uv;
>> +	u32			mclk_sel;
>> +	bool			int_ref_en;
>> +	bool			bipolar;
>> +
>> +	unsigned int		num_enabled_channels;
>> +	unsigned int		effective_watermark;
>> +	unsigned int		watermark;
>> +
>> +	struct spi_message	fifo_msg;
>> +	struct spi_transfer	fifo_xfer[2];
>> +
>> +	/*
>> +	 * DMA (thus cache coherency maintenance) requires the
>> +	 * transfer buffers to live in their own cache lines.
>> +	 */
>> +	u8			reset_buf[AD4130_RESET_BUF_SIZE] ____cacheline_aligned;
> 
> This needs a fix along with the other set that I'll hopefully get this weekend.
> In meantime if you change this to __aligned(IIO_ALIGN); then the fix I have
> coming shortly will change that define to be large enough for the odd ARM systems
> with 128 byte requirement for DMA and 64 byte L1 cache alignment (which is what
> ___cacheline_aligned guarantees.
> 

Sure thing.

> 
>> +	u8			reg_write_tx_buf[4];
>> +	u8			reg_read_tx_buf[1];
>> +	u8			reg_read_rx_buf[3];
>> +	u8			fifo_tx_buf[2];
>> +	u8			fifo_rx_buf[AD4130_FIFO_SIZE *
>> +					    AD4130_FIFO_MAX_SAMPLE_SIZE];
>> +};
>> +
> 
>> +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.

>> +
>> +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;

>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int ad4310_parse_fw(struct iio_dev *indio_dev)
>> +{
>> +	struct ad4130_state *st = iio_priv(indio_dev);
>> +	struct device *dev = &st->spi->dev;
>> +	u32 ext_clk_freq = AD4130_MCLK_FREQ_76_8KHZ;
>> +	bool int_clk_out = false;
>> +	unsigned int i;
>> +	int avdd_uv;
>> +	int irq;
>> +	int ret;
>> +
> 
> ...
> 
>> +
>> +	device_property_read_u32(dev, "adi,ext-clk-freq", &ext_clk_freq);
> 
> I'll note this in reviewing the binding, but the naming should incorporate the
> units.
> 
>> +	if (ext_clk_freq != AD4130_MCLK_FREQ_153_6KHZ &&
>> +	    ext_clk_freq != AD4130_MCLK_FREQ_76_8KHZ)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Invalid external clock frequency %u\n",
>> +				     ext_clk_freq);
>> +
> 
> ...
> 
>> +
>> +	return 0;
>> +}
>> +
> 
>> +
>> +static int ad4130_setup(struct iio_dev *indio_dev)
>> +{
>> +	struct ad4130_state *st = iio_priv(indio_dev);
>> +	struct device *dev = &st->spi->dev;
>> +	unsigned int int_ref_val;
>> +	unsigned long rate = AD4130_MCLK_FREQ_76_8KHZ;
>> +	unsigned int val;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	if (st->mclk_sel == AD4130_MCLK_153_6KHZ_EXT)
>> +		rate = AD4130_MCLK_FREQ_153_6KHZ;
>> +
>> +	ret = clk_set_rate(st->mclk, rate);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = clk_prepare_enable(st->mclk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_add_action_or_reset(dev, ad4130_clk_disable_unprepare,
>> +				       st->mclk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (st->int_ref_uv == AD4130_INT_REF_2_5V)
>> +		int_ref_val = AD4130_INT_REF_VAL_2_5V;
>> +	else
>> +		int_ref_val = AD4130_INT_REF_VAL_1_25V;
>> +
>> +	/* Switch to SPI 4-wire mode. */
>> +	val = AD4130_CSB_EN_MASK;
> 
> Trivial but I'd prefer to see this treated as a field as well for consistency.
> 	val = FIELD_PREP(AD4130_CSB_EN_MASK, 1);

Already did, but I waited for your input before pushing the new
patchset.

> >> +	val |= FIELD_PREP(AD4130_BIPOLAR_MASK, st->bipolar);
>> +	val |= FIELD_PREP(AD4130_INT_REF_EN_MASK, st->int_ref_en);
>> +	val |= FIELD_PREP(AD4130_MODE_MASK, AD4130_MODE_IDLE);
>> +	val |= FIELD_PREP(AD4130_MCLK_SEL_MASK, st->mclk_sel);
>> +	val |= FIELD_PREP(AD4130_INT_REF_VAL_MASK, int_ref_val);
>> +
>> +	ret = regmap_write(st->regmap, AD4130_REG_ADC_CONTROL, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = FIELD_PREP(AD4130_INT_PIN_SEL_MASK, st->int_pin_sel);
>> +	for (i = 0; i < st->num_gpios; i++)
>> +		val |= BIT(st->gpio_offsets[i]);
>> +
>> +	ret = regmap_write(st->regmap, AD4130_REG_IO_CONTROL, val);
>> +	if (ret)
>> +		return ret;
>> +
> 

Powered by blists - more mailing lists