Message-ID: <20220424165143.068ba254@jic23-huawei>
Date:   Sun, 24 Apr 2022 16:51:43 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Cosmin Tanislav <demonsingur@...il.com>
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 v1 3/3] iio: adc: ad4130: add AD4130 driver

On Sun, 17 Apr 2022 13:26:38 +0300
Cosmin Tanislav <demonsingur@...il.com> wrote:

> On 4/16/22 19:21, Jonathan Cameron wrote:
> > On Wed, 13 Apr 2022 12:40:11 +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,
> > 
> > I've only glanced at Andy's comments, so may well overlap in places
> > though I'll try and avoid too much repetition if I happen to remember
> > Andy commented on something already.
> > 
> > Only a few minor things from me.  For such a complex device this
> > is looking pretty good for a first version posted.
> > 
> > Jonathan
> > 
> >   
> >> ---
> >>   MAINTAINERS              |    8 +
> >>   drivers/iio/adc/Kconfig  |   13 +
> >>   drivers/iio/adc/Makefile |    1 +
> >>   drivers/iio/adc/ad4130.c | 2072 ++++++++++++++++++++++++++++++++++++++
> >>   4 files changed, 2094 insertions(+)
> >>   create mode 100644 drivers/iio/adc/ad4130.c
> >>  
> > 
> > ...
> >   
> >> diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
> >> new file mode 100644
> >> index 000000000000..89fb9b413ff0
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/ad4130.c
> >> @@ -0,0 +1,2072 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/*
> >> + * AD4130 SPI ADC driver
> >> + *
> >> + * Copyright 2022 Analog Devices Inc.
> >> + */
> >> +#include <asm/div64.h>
> >> +#include <asm/unaligned.h>
> >> +#include <linux/bitfield.h>
> >> +#include <linux/bitops.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/device.h>
> >> +#include <linux/err.h>
> >> +#include <linux/gpio/driver.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/irq.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/iio/buffer.h>
> >> +#include <linux/iio/kfifo_buf.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of_irq.h>
> >> +#include <linux/property.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/regulator/consumer.h>
> >> +#include <linux/spi/spi.h>
> >> +
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/iio/sysfs.h>
> >> +
> >> +#define AD4130_8_NAME			"ad4130-8"
> >> +
> >> +#define AD4130_COMMS_READ_MASK		BIT(6)
> >> +
> >> +#define AD4130_REG_STATUS		0x00
> >> +#define AD4130_STATUS_POR_FLAG_MASK	BIT(4)
> >> +
> >> +#define AD4130_REG_ADC_CONTROL		0x01
> >> +#define AD4130_BIPOLAR_MASK		BIT(14)  
> > where possibly it is good to name register fields such that it's
> > obvious which register they are fields of.  Makes it easier
> > to be sure we have the right one.
> > (I fell into this trap myself this week and wasted an hour or
> > so before I figured out that there were two different registers
> > with fields with exactly the same name ;)
> > 
> > Lots of different conventions for this one and I don't mind
> > which one you pick. e.g.  This works, but isn't perfect by
> > any means.
> > 
> > #define AD4130_ADC_CTRL_REG
> > #define  AD4130_ADC_CTRL_BIPOLAR_MASK  
> >  > Note I quite like the subtle indenting to make it easier  
> > to read these definitions as well.
> >   
> 
> Well. It's not late to change it now, if you insist.
> 
> If you look at my past drivers, I kept the register prefix
> for masks, but it seemed kind of redundant and I dropped it
> for this one.

To a certain extent this is about consistency.  Even if it's
not necessary for clarity in this particular driver I'd like
to keep that clarity of definition in all drivers if possible
to provide good examples for cases where maybe it's more
important.

> 
> By subtle indenting, you mean, making the masks look like
> sub-definitions of the register?

Sort of - I mean the extra space as in the example above between
define and the name.

> 
> >> +#define AD4130_INT_REF_VAL_MASK		BIT(13)
> >> +#define AD4130_INT_REF_2_5V		2500000
> >> +#define AD4130_INT_REF_1_25V		1250000
> >> +#define AD4130_CSB_EN_MASK		BIT(9)
> >> +#define AD4130_INT_REF_EN_MASK		BIT(8)
> >> +#define AD4130_MODE_MASK		GENMASK(5, 2)
> >> +#define AD4130_MCLK_SEL_MASK		GENMASK(1, 0)  


> > ...
> >   
> >> +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_setup_info	setups_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;
> >> +	bool			int_ref_en;
> >> +	u32			int_ref_uv;
> >> +	u32			mclk_sel;
> >> +	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;
> >> +	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];  
> > 
> > This is quite a large buffer.  Perhaps it would be better to drain the fifo
> > in multiple steps if it is very full?  I guess that could be added
> > later if anyone ever ran into a problem with the buffer size.
> >   
> 
> We're quite time-constrained when receiving the FIFO watermark
> interrupt, I'm not sure two separate transfers would be any better.

Potential issue is that you get an SPI master that can't do such a bit
transfer.  There are a few out there which are quite limited because
they aren't DMA based. As stated, perhaps this is one to fix only
when someone runs into the problem.

> 
> >   
> >> +};  
> >   
> >> +
> >> +static const struct iio_info ad4130_info = {
> >> +	.read_raw = ad4130_read_raw,
> >> +	.read_avail = ad4130_read_avail,
> >> +	.write_raw_get_fmt = ad4130_write_raw_get_fmt,
> >> +	.write_raw = ad4130_write_raw,
> >> +	.update_scan_mode = ad4130_update_scan_mode,
> >> +	.hwfifo_set_watermark = ad4130_set_fifo_watermark,
> >> +	.debugfs_reg_access = ad4130_reg_access,
> >> +};
> >> +
> >> +static int ad4130_buffer_postenable(struct iio_dev *indio_dev)
> >> +{
> >> +	struct ad4130_state *st = iio_priv(indio_dev);
> >> +	int ret;
> >> +
> >> +	mutex_lock(&st->lock);
> >> +
> >> +	ret = ad4130_set_watermark_interrupt_en(st, true);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	/* When the chip enters FIFO mode, IRQ polarity is inversed. */  
> > 
> > That is downright odd :)  Perhaps a datasheet section reference is
> > appropriate here.  
> 
> Page 65, FIFO Watermark Interrupt section.
> 
> +
> 
> Page 71, Bit Descriptions for STATUS Register, RDYB.
> 
> I'll add them as a comment.

Great.

...

...

> >> +	ret = ad4130_parse_fw_children(indio_dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	for (i = 0; i < AD4130_MAX_GPIOS; i++) {
> >> +		if (st->pins_fn[i + AD4130_AIN2_P1] != AD4130_PIN_FN_NONE)
> >> +			continue;  
> > 
> > I'm a bit confused. pins_fn seems to be for the Analog pins, yet here is being
> > used for the GPIOs?  Maybe some explanatory comments
> >   
> 
> AIN2 = P1, AIN3 = P2, AIN4 = P3, AIN5 = P4. I'll add some comments.

Ah. I'd missed that relationship.

> 
> >> +
> >> +		st->gpio_offsets[st->num_gpios++] = i;
> >> +	}
> >> +
> >> +	return 0;
> >> +}  
...

> >> +	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;
> >> +	val |= st->bipolar ? AD4130_BIPOLAR_MASK : 0;  
> > 
> > Prefer field PREP even for these single bit cases >  
> 
> Do you want this for the places where I used `status ? mask : 0`
> inside regmap_update_bits() calls too?

That would be great.   Though probably not for the gpio one as
that is used in a more complex fashion so would be more confusing
done with two FIELD_PREP() calls.

> 
> >> +	val |= st->int_ref_en ? AD4130_INT_REF_EN_MASK : 0;

Sorry I didn't get back to this earlier (I see you sent a v2 and v3).
Fun week of spec review against a short timescale so I've not had any
time to get much IIO mailing list reading done!

Thanks,

Jonathan

Powered by blists - more mailing lists