RE: [PATCH v2 2/2] iio: adc: max14001: New driver

[Paller, Kim Seer]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Tuesday, June 6, 2023 3:24 AM
> To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx>
> Cc: lars@xxxxxxxxxx; krzysztof.kozlowski@xxxxxxxxxx; broonie@xxxxxxxxxx;
> lgirdwood@xxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/2] iio: adc: max14001: New driver
> 
> [External]
> 
> On Mon, 5 Jun 2023 21:07:55 +0800
> Kim Seer Paller <kimseer.paller@xxxxxxxxxx> wrote:
> 
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx>
> > ---
> ...
> 
> Hi Kim,
> 
> A few comments inline.
> 
> > diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> > new file mode 100644 index 000000000..7c5272756
> > --- /dev/null
> > +++ b/drivers/iio/adc/max14001.c
> > @@ -0,0 +1,333 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
> > +/*
> > + * Analog Devices MAX14001 ADC driver
> > + *
> > + * Copyright 2023 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitrev.h>
> > +#include <linux/device.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h> #include <linux/spi/spi.h>
> > +#include <linux/slab.h> #include <linux/types.h>
> > +
> > +#include <asm/unaligned.h>
> > +
> > +/* MAX14001 Registers Address */
> > +#define MAX14001_ADC			0x00
> > +#define MAX14001_FADC			0x01
> > +#define MAX14001_FLAGS			0x02
> > +#define MAX14001_FLTEN			0x03
> > +#define MAX14001_THL			0x04
> > +#define MAX14001_THU			0x05
> > +#define MAX14001_INRR			0x06
> > +#define MAX14001_INRT			0x07
> > +#define MAX14001_INRP			0x08
> > +#define MAX14001_CFG			0x09
> > +#define MAX14001_ENBL			0x0A
> > +#define MAX14001_ACT			0x0B
> > +#define MAX14001_WEN			0x0C
> > +
> > +#define MAX14001_VERIFICATION_REG(x)	((x) + 0x10)
> > +
> > +#define MAX14001_CFG_EXRF		BIT(5)
> > +
> > +#define MAX14001_ADDR_MASK		GENMASK(15, 11)
> > +#define MAX14001_DATA_MASK		GENMASK(9, 0)
> > +#define MAX14001_FILTER_MASK		GENMASK(3, 2)
> > +
> > +#define MAX14001_SET_WRITE_BIT		BIT(10)
> > +#define MAX14001_WRITE_WEN		0x294
> > +
> > +struct max14001_state {
> > +	struct spi_device	*spi;
> > +	/* lock protect agains multiple concurrent accesses */
> 
> To what?  Here I suspect it's RMW sequence on device and perhaps more
> importantly the buffers below.
> 
> > +	struct mutex		lock;
> > +	struct regmap		*regmap;
> > +	int			vref_mv;
> > +	/*
> > +	 * DMA (thus cache coherency maintenance) requires the
> > +	 * transfer buffers to live in their own cache lines.
> 
> You are looking at an old kernel I guess - we fixed all of these - and
> introduced IIO_DMA_MINALIGN for __aligned(IIO_DMA_MINALIGN) to
> make it easier to fix any such problems in future.
> 
> Upshot is that ___cacheline_aligned aligns to the l1 cacheline length.
> Some fun systems (such as the big servers I use in my dayjob) have higher
> cacheline sizes for their larger / further from CPU caches.
> One group of SoCs out there is known to both do non coherent DMA and
> have a larger line size for the bit relevant to that than ___cacheline_aligned
> gives you. So on that rare platform this is currently broken.

It's good to know. Given this information, is there anything specific that I 
need to change in the code or implementation related to 
the ___cacheline_aligned part?
> 
> > +	 */
> > +	__be16			spi_tx_buffer ____cacheline_aligned;
> > +	__be16			spi_rx_buffer;
> > +};
> > +
> > +static int max14001_read(void *context, unsigned int reg_addr,
> > +					unsigned int *data)
> > +{
> > +	struct max14001_state *st = context;
> > +	u16 tx = 0;
> > +	int ret;
> > +
> > +	struct spi_transfer xfers[] = {
> > +		{
> > +			.tx_buf = &st->spi_tx_buffer,
> > +			.len = 2,
> > +			.cs_change = 1,
> > +		}, {
> > +			.rx_buf = &st->spi_rx_buffer,
> > +			.len = 2,
> > +		},
> > +	};
> > +
> > +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> > +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> > +
> > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > +	if (ret)
> > +		return ret;
> > +
> > +	*data = bitrev16(be16_to_cpu(st->spi_rx_buffer)) &
> > +MAX14001_DATA_MASK;
> > +
> > +	return 0;
> > +}
> > +
> > +static int max14001_write(void *context, unsigned int reg_addr,
> > +					unsigned int data)
> > +{
> > +	struct max14001_state *st = context;
> > +	u16 tx = 0;
> > +
> > +	tx = FIELD_PREP(MAX14001_ADDR_MASK, reg_addr);
> > +	tx |= FIELD_PREP(MAX14001_SET_WRITE_BIT, 1);
> > +	tx |= FIELD_PREP(MAX14001_DATA_MASK, data);
> > +
> > +	st->spi_tx_buffer = bitrev16(cpu_to_be16(tx));
> > +
> > +	return spi_write(st->spi, &st->spi_tx_buffer, 2); }
> > +
> > +static int max14001_write_verification_reg(struct max14001_state *st,
> > +				     unsigned int reg_addr)
> > +{
> > +	unsigned int reg_data;
> > +	int ret;
> > +
> > +	ret = max14001_read(st, reg_addr, &reg_data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return max14001_write(st,
> MAX14001_VERIFICATION_REG(reg_addr),
> > +				reg_data);
> 
> Even though this is a bit unusual, I'd still expect this to use the regmap_read /
> regmap_write interfaces not directly use the callbacks.
> 
> > +}
> > +
> > +static int max14001_reg_update(struct max14001_state *st,
> > +				unsigned int reg_addr,
> > +				unsigned int mask,
> > +				unsigned int val)
> > +{
> > +	int ret;
> > +
> > +	/* Enable SPI Registers Write */
> > +	ret = max14001_write(st, MAX14001_WEN,
> MAX14001_WRITE_WEN);
> 
> Mixing regmap and non regmap rather defeats the point of having a standard
> interface.  Use regmap_read and regmap_write throughout or not at all.

I found it difficult to implement the regmap interface due to the timing diagram 
requirements. The chip select needs to be changed between transfers, which, 
as far as I know, does not work with regmap. Perhaps, I will consider sticking 
to the non-regmap approach.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(st->regmap, reg_addr, mask, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = max14001_write_verification_reg(st, reg_addr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Disable SPI Registers Write */
> > +	return max14001_write(st, MAX14001_WEN, 0); }
> > +
> > +static int max14001_read_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int *val, int *val2, long mask) {
> > +	struct max14001_state *st = iio_priv(indio_dev);
> > +	unsigned int data;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&st->lock);
> > +		ret = max14001_read(st, MAX14001_ADC, &data);
> > +		mutex_unlock(&st->lock);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = data;
> > +
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = st->vref_mv;
> > +		*val2 = 10;
> > +
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct regmap_config max14001_regmap_config = {
> > +	.reg_read = max14001_read,
> > +	.reg_write = max14001_write,
> 
> I'd keep this up by the callbacks, so all the regmap setup stuff is in one place.
> 
> > +};
> > +
> > +static const struct iio_info max14001_info = {
> > +	.read_raw = max14001_read_raw,
> > +};
> > +
> 
> ...
> 
> > +static int max14001_probe(struct spi_device *spi) {
> 
> ...
> 
> > +
> > +	vref = devm_regulator_get_optional(&spi->dev, "vref");
> > +	if (IS_ERR(vref)) {
> > +		if (PTR_ERR(vref) != -ENODEV)
> > +			return dev_err_probe(&spi->dev, PTR_ERR(vref),
> > +					     "Failed to get vref regulator");
> > +
> > +		/* internal reference */
> > +		st->vref_mv = 1250;
> > +	} else {
> > +		ret = regulator_enable(vref);
> > +		if (ret)
> > +			return dev_err_probe(&spi->dev, ret,
> > +					"Failed to enable vref regulators\n");
> > +
> > +		ret = devm_add_action_or_reset(&spi->dev,
> > +					       max14001_regulator_disable,
> > +					       vref);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* enable external voltage reference */
> 
> use external voltage reference?
> 
> It's enabled by the regulator_enable() above, not this line.

What I meant was to enable the CFG register to utilize the external voltage 
source within the ADC. I've missed to specify the comment on this one.

> 
> > +		ret = max14001_reg_update(st, MAX14001_CFG,
> > +					  MAX14001_CFG_EXRF, 1);
> > +
> > +		ret = regulator_get_voltage(vref);
> > +		if (ret < 0)
> > +			return dev_err_probe(&spi->dev, ret,
> > +					     "Failed to get vref\n");
> > +
> > +		st->vref_mv = ret / 1000;
> > +	}
> > +
> > +	mutex_init(&st->lock);
> > +
> > +	return devm_iio_device_register(&spi->dev, indio_dev); }