Message-ID: <20240824122111.425fa689@jic23-huawei>
Date: Sat, 24 Aug 2024 12:21:11 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Esteban Blanc <eblanc@...libre.com>
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 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.

> +		};
> +	} rx_data __aligned(IIO_DMA_MINALIGN);

This one is always fun.  You shouldn't need to mark this second buffer
because normally SPI controllers don't shoot themselves in the foot
and it isn't normal software flow to fill rx_data whilst the SPI
controller is working on tx_data (I didn't notice you doing that
here either).

As such, we don't need tx_buf and rx_buf to be in different cachelines.
It is sufficient to mark just the first one (as that forces these
to start at a new cacheline) the second buffer can then sit later
in that same cacheline if the sizes end up so that happens.

So drop __aligned(IIO_DMA_MINALIGN from rx_data.


> +};
> +
> +#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.

> +	.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?

> +	 */
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(val, &st->rx_data.raw[reg_size], val_size);
> +
> +	return ret;
> +}

> +
> +static int ad4030_get_chan_gain(struct iio_dev *indio_dev, int ch, int *val,
> +				int *val2)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	u16 gain;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(ch),
> +			       st->rx_data.raw, AD4030_REG_GAIN_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	gain = be16_to_cpu(*(u16 *)st->rx_data.raw);

As below, I'd be tempted to use the unaligned getters to avoid the cast.
This isn't a performance path afterall so even if it's painful on
some arch, we don't really care.

> +
> +	/* From datasheet: multiplied output = input × gain word/0x8000 */
> +	*val = gain / 0x8000;
> +	*val2 = mul_u64_u32_div(gain % 0x8000, MICRO, 0x8000);
> +
> +	return 0;
> +}
> +
> +/*
> + * @brief Returns the offset where 1 LSB = (VREF/2^precision_bits - 1)/gain

Not kernel, doc so drop the @brief, or fully document it.

> + */
> +static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
> +			       st->rx_data.raw, AD4030_REG_OFFSET_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	switch (st->chip->precision_bits) {
> +	case 16:
> +		*val = sign_extend32(get_unaligned_be16(st->rx_data.raw), 15);
> +		break;
return 0;
> +
> +	case 24:
> +		*val = sign_extend32(get_unaligned_be24(st->rx_data.raw), 23);
> +		break;
return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
I'll always prefer early returns because I don't like scrolling
to check what happens after switch statements.
So move the returns up and drop this.

> +}
> +
> +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
> +				int gain_frac)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be16 *val = (__be16 *)st->rx_data.raw;

Hmm. This is a bit ugly. Maybe a union for that
buffer so you can reference it directly without the type casting?
Or be lazy and do a put_unaligned_be16 even though it's aligned ;)

> +	u64 gain;
> +
> +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> +
> +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> +		return -EINVAL;
> +
> +	*val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), val,
> +			  AD4030_REG_GAIN_BYTES_NB);
> +}

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


> +
> +	return 0;
> +}

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



> +
> +		case IIO_CHAN_INFO_CALIBSCALE:
> +			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
> +						   val, val2);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +
> +		case IIO_CHAN_INFO_CALIBBIAS:
> +			ret = ad4030_get_chan_offset(indio_dev, chan->channel,
> +						     val);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	unreachable();
> +}
> +



> +
> +static int ad4030_reset(struct ad4030_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct gpio_desc *reset;
> +	int ret;
> +
> +	/* Use GPIO if available ... */

Obvious comment given code. I'd drop it.

> +	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 */
likewise. Feels obvious enough to not benefit from comment.
Maybe that's just me having seen far too many drivers though
so if you really think it is useful then fine to leave these.

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


> +	fsleep(AD4030_TRESET_COM_DELAY_MS);
> +
> +	/* After reset, conversion mode is enabled. Switch to reg access */

This comment is good as not obvious without datasheet diving.
So one to definitely keep.


> +	return ad4030_enter_config_mode(st);
> +}
> +
> +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?
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)

Superficially it seems to be matched with a particular compatible
so it seems likely.

> +
> +	return 0;
> +}
> +


>
> +static const struct spi_device_id ad4030_id_table[] = {
> +	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> +	{}
As below.

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.


> +};
> +MODULE_DEVICE_TABLE(spi, ad4030_id_table);
> +
> +static const struct of_device_id ad4030_of_match[] = {
> +	{ .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
> +	{}

I'm aiming for a bit more consistency in formatting of these.
As such made an arbitrary choice to prefer
 	{ }
for the terminators. Please go with that for new code.

> +};
> +MODULE_DEVICE_TABLE(of, ad4030_of_match);

Powered by blists - more mailing lists