Message-Id: <D3QUGZYL7INK.R3U3WQR0OCUS@baylibre.com>
Date: Tue, 27 Aug 2024 18:45:49 +0200
From: "Esteban Blanc" <eblanc@...libre.com>
To: "Jonathan Cameron" <jic23@...nel.org>
Cc: "Lars-Peter Clausen" <lars@...afoo.de>, "Michael Hennerich"
 <Michael.Hennerich@...log.com>, "Rob Herring" <robh@...nel.org>, "Krzysztof
 Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>,
 "Nuno Sa" <nuno.sa@...log.com>, "Jonathan Corbet" <corbet@....net>,
 <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "David Lechner" <dlechner@...libre.com>,
 <linux-doc@...r.kernel.org>
Subject: Re: [PATCH 2/6] iio: adc: ad4030: add driver for ad4030-24

On Sat Aug 24, 2024 at 1:21 PM CEST, Jonathan Cameron wrote:
> On Thu, 22 Aug 2024 14:45:18 +0200
> Esteban Blanc <eblanc@...libre.com> wrote:
>
> > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > 
> > The driver implements basic support for the AD4030-24 1 channel
> > differential ADC with hardware gain and offset control.
> > 
> > Signed-off-by: Esteban Blanc <eblanc@...libre.com>
> Hi Esteban
>
> Some additional comments.  David did a good review already so
> I've tried not to duplicate too much of that.
>
> The big one in here is don't use extended_name.
> It's effectively deprecated for new drivers plus
> it would have required you add a lot of ABI docs as every
> sysfs file would have a strange name.
>
> > diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> > new file mode 100644
> > index 000000000000..a981dce988e5
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4030.c
>
> > +struct ad4030_state {
> > +	struct spi_device *spi;
> > +	struct regmap *regmap;
> > +	const struct ad4030_chip_info *chip;
> > +	struct gpio_desc *cnv_gpio;
> > +	int vref_uv;
> > +	int vio_uv;
> > +	int offset_avail[3];
> > +	u32 conversion_speed_hz;
> > +	enum ad4030_out_mode mode;
> > +
> > +	/*
> > +	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> > +	 * to live in their own cache lines.
> > +	 */
> > +	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> > +	struct {
> > +		union {
> > +			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> > +			struct {
> > +				s32 val;
> > +				u32 common;
> > +			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
>
> David pointed out this doesn't need to be packed.
> Given you have a union here, add __beXX as needed to avoid casts below.

They also pointed out that I should reduce the size for the common field.
I was planing to use an u32 bitfield here, 8 bits for common and 24 bits for
padding. As far as I understood, the C standard is quite flexible on the
size used for bitfield, so I should probably keep the __packed, right?

> > +};
> > +
> > +#define AD4030_CHAN_CMO(_idx)  {					\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _idx * 2 + 2,					\
> > +	.scan_index = _idx * 2 + 1,					\
> > +	.extend_name = "Channel" #_idx " common byte part",		\
>
> We more or less never use extend name any more because it makes writing
> userspace code much harder.  Use the label callback to assign a label instead.
>
> If we were still using this, it would need to be a lot simpler than that
> and no spaces etc as it ends up int he sysfs file names.

> > +	.scan_type = {							\
> > +		.sign = 'u',						\
> > +		.storagebits = 32,					\
> > +		.realbits = 8,						\
> > +		.endianness = IIO_BE,					\
> > +	},								\
> > +}
> > +
> > +#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) {			\
> > +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),		\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
> > +		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
> > +		BIT(IIO_CHAN_INFO_RAW),					\
> > +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
> > +		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _idx * 2,						\
> > +	.channel2 = _idx * 2 + 1,					\
> > +	.scan_index = _idx * 2,						\
> > +	.extend_name = "Channel" #_idx " differential part",		\
>
> As above, no to this for same reason.
> This will generate a crazy ABI so I'm a bit surprised that didn't show
> up in your testing.  Would have needed a lot of docs even if we did
> still do things this way.

I'm using ADI IIO oscilloscope to check the signals so I didn't get
impacted by the sysfs change. Anyway I will use labels.

> > +	.differential = true,						\
> > +	.scan_type = {							\
> > +		.sign = 's',						\
> > +		.storagebits = _storage,				\
> > +		.realbits = _real,					\
> > +		.shift = _shift,					\
> > +		.endianness = IIO_BE,					\
> > +	},								\
> > +}
> > +
> > +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> > +			   void *val, size_t val_size)
> > +{
> > +	struct ad4030_state *st = context;
> > +
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = st->tx_data,
> > +		.rx_buf = st->rx_data.raw,
> > +		.len = reg_size + val_size,
> > +	};
> > +	int ret;
> > +
> > +	memcpy(st->tx_data, reg, reg_size);
> > +
> > +	/*
> > +	 * This should use spi_write_the_read but when doing so, CS never get
> > +	 * deasserted.
>
> I'm confused.  As a single transfer it won't be deasserted in the transfer
> whereas spi_write_then_read() will. So is this comment backwards or
> is it referring to something else?

So, with a single transfer (what is done now), the transfer is working
as expected: CS goes low, the data is transferred, CS goes high again.
With spi_write_then_read(), CS goes low, data is transferred but CS never
goes high again. After some time I get a timeout error in the kernel logs.

> > +static int ad4030_conversion(struct ad4030_state *st,
> > +			     const struct iio_chan_spec *chan)
> > +{
> > +	unsigned int bytes_to_read;
> > +	unsigned char byte_index;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/* Number of bytes for one differential channel */
> > +	bytes_to_read = BITS_TO_BYTES(chan->scan_type.realbits);
> > +	/* Add one byte if we are using a differential + common byte mode */
> > +	bytes_to_read += (st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
> > +			st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ? 1 : 0;
> > +	/* Mulitiply by the number of hardware channels */
> > +	bytes_to_read *= st->chip->num_channels;
> > +
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> > +	ndelay(AD4030_TCNVH_NS);
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > +	ndelay(st->chip->tcyc);
> > +
> > +	ret = spi_read(st->spi, st->rx_data.raw, bytes_to_read);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> > +		return 0;
> > +
> > +	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> > +	for (i = 0; i < st->chip->num_channels; i++)
> > +		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> break line after =.
>
> When it doesn't significantly hurt readability we still try to keep to 80
> chars for IIO drivers.  People have big screens but a lot of kernel devs
> love to have lots of windows across them - or have bad eyesight due to
> years of code review!

I keep forgeting that checkpatch now defaults at 100 chars...

> > +static int ad4030_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan, int *val,
> > +			   int *val2, long info)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > +		switch (info) {
> > +		case IIO_CHAN_INFO_RAW:
> > +			return ad4030_single_conversion(indio_dev, chan, val);
> > +
> > +		case IIO_CHAN_INFO_SCALE:
> > +			*val = (st->vref_uv * 2) / MILLI;
> > +			*val2 = st->chip->precision_bits;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
>
> No reason you can't read this whilst buffered capture in progress.
> Maybe it's not worth the effort of special casing though.
>
> It is the one thing people do read whilst doing buffered capture
> though because they didn't cache it before starting the buffer
> and it's needed for data interpretation unlike all the other controls.
>
> Maybe just do a
> 	if (info == IIO_CHAN_INFO_SCALE) {
> 	}
> block at top of function?

Good catch. I will check for IIO_CHAN_INFO_SCALE before the whole block

> > +static int ad4030_reset(struct ad4030_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	struct gpio_desc *reset;
> > +	int ret;
> > +
> > +	/* Use GPIO if available ... */
> > +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset))
> > +		return dev_err_probe(dev, PTR_ERR(reset),
> > +				"Failed to get reset GPIO\n");
> > +
> > +	if (reset) {
> > +		ndelay(50);
> > +		gpiod_set_value_cansleep(reset, 0);
> > +	} else {
> > +		/* ... falback to software reset otherwise */
> > +		ret = ad4030_enter_config_mode(st);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
> > +				   AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	/* Wait for reset to complete before communicating to it */
>
> I'd rather see a reference for the value than a generic comment
> like this.  Also pull the actual value down here. Not particularly
> useful to have a define for what is a real time unless you are going
>  to have some combined docs for a bunch of timings (i.e a datasheet
> table reference)

I will put the real value in fsleep call directly. When you say "I'd
rather see a reference for the value", you ment a reference to the place
the value is defined in the datasheet, right?

> > +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> > +{
> > +	unsigned int grade;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> > +	if (ret)
> > +		return ret;
> > +
> > +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> > +	if (grade != st->chip->grade)
> > +		return dev_err_probe(&st->spi->dev, -EINVAL,
> > +					"Unknown grade(0x%x) for %s\n", grade,
> > +					st->chip->name);
>
> Is this similar to a missmatch on a whoami value?

Yes. It also saved me multiple hours of debuging when the eval board
was not connected porperly and the SPI link was just not working.

> I.e. should we print a message and carry on in the interests of providing
> some degree of support for newer devices on older kernel?
> (fallback compatibles in DT)

Ok, let's go with a warning then.

> > +static const struct spi_device_id ad4030_id_table[] = {
> > +	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> > +	{}
>
> I'm going to assume you have a bunch of other parts you plan to
> support soon. Otherwise we normally don't add the chip specific
> support until it is needed.  It tends to complicate initial driver
> review a little and experience says that sometimes no other devices
> are ever added.

I'm sending the other devices in the same series (patch 4 and 5).
For the sake of reducing noise in the later patches, I've put it in
the initial driver. If you feel like I should wait and do it in the
following patch (patch 4), I can do that.

Thanks for your time,

-- 
Esteban Blanc
BayLibre

Powered by blists - more mailing lists