Re: [PATCH 2/2] iio: adc: ti-ads1298: Add driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

Took me a while to get back to this. Incorporated all of your comments, except that I seem to be unable to count characters. Most places, I have more than 80 characters in a line it I don't indent as I did. I'll try to improve my indents...


On 13-12-2023 15:55, Andy Shevchenko wrote:
On Wed, Dec 13, 2023 at 10:47:22AM +0100, Mike Looijmans wrote:
Skeleton driver for the TI ADS1298 medical ADC. This device is
typically used for ECG and similar measurements. Supports data
acquisition at configurable scale and sampling frequency.
...

+config TI_ADS1298
+	tristate "Texas Instruments ADS1298"
+	depends on SPI
+	select IIO_BUFFER
+	default y
Huh?!

+	help
+	  If you say yes here you get support for Texas Instruments ADS1298
+	  medical ADC chips
+
+	  This driver can also be built as a module. If so, the module will be
+	  called ti-ads1298.
...

+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
Is it used as a proxy? (At least for array_size.h)
Please use real headers in such case.

+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
This is interesting grouping, but okay, I understand the point.

+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
...

+#define ADS1298_CLK_RATE	2048000
Units? _HZ ?

...

+/* Outputs status word and 8 samples of 24 bits each, plus the command byte */
/* Outputs status word and 8 24-bit samples, plus the command byte */

a bit shorter.

+#define ADS1298_SPI_RDATA_BUFFER_SIZE	((ADS1298_MAX_CHANNELS + 1) * 3 + 1)
...

+#define ADS1298_CHAN(index)					\
+{								\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = index,					\
+	.address = 3 * index + 4,				\
Hmm... does this 3 have a distinct definition?

+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
Can be written as below

	.info_mask_separate =					\
		BIT(IIO_CHAN_INFO_RAW) |			\
		BIT(IIO_CHAN_INFO_SCALE),			\

+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
	.info_mask_shared_by_all =				\
		BIT(IIO_CHAN_INFO_SAMP_FREQ) |			\
		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),		\

+	.scan_index = index,					\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = 24,					\
+		.storagebits = 32,				\
+		.endianness = IIO_BE,				\
+	},							\
+}
...

+static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
+{
+	struct spi_transfer cmd_xfer = {
+		.tx_buf = priv->cmd_buffer,
+		.rx_buf = priv->cmd_buffer,
+		.len = 1,
		sizeof(command) ?

+		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
+		.delay.value = 2,
+		.delay.unit = SPI_DELAY_UNIT_USECS,
		.delay = {
			.value = ...
			.unit = ...
		},

+	};
+	priv->cmd_buffer[0] = command;
+
+	return spi_sync_transfer(priv->spi, &cmd_xfer, 1);
+}
...

+	/* Cannot take longer than 40ms (250Hz) */
+	ret = wait_for_completion_timeout(&priv->completion,
+					  msecs_to_jiffies(50));
One line?

+	if (!ret)
+		return -ETIMEDOUT;
...

+	if (cfg & ADS1298_MASK_CONFIG1_HR)
+		rate >>= 6; /* HR mode */
+	else
+		rate >>= 7; /* LP mode */
Are those magic numbers defined?

...

+	factor = (rate >> 6) / val;
Is it the same 6 semantically as above?

...

+	return IIO_VAL_FRACTIONAL_LOG2;
+}
+
+
One blank line is enough.

+		*val = sign_extend32(get_unaligned_be24(
+					priv->rx_buffer + chan->address), 23);
Strange indentation.

		*val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
				     23);

Doesn't fit, first line is 83 characters by my count...


...

+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val);
+		if (!ret) {
Why not using standard pattern?

		if (ret)
			return ret;

(see below)

+			ret = IIO_VAL_INT;
+			*val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));
Outer parentheses are redundant.

+		}
+		break;
		return IIO_VAL_INT;


+	default:
+		ret = -EINVAL;
+		break;
return directly.

+	}
+	return ret;
It will gone.

...

+static int ads1298_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct ads1298_private *priv = iio_priv(indio_dev);
+	int ret;
No need, just return directly.

+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = ads1298_set_samp_freq(priv, val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
...

+static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			      unsigned int writeval, unsigned int *readval)
+{
+	struct ads1298_private *priv = iio_priv(indio_dev);
+	if (!readval)
Perhaps positive conditional?

	if (readval)
		return readval;
	return writeval;

+		return regmap_write(priv->regmap, reg, writeval);
+
+	return regmap_read(priv->regmap, reg, readval);
+}
...

+	/* Power down channels that aren't in use */
+	for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
+		regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
+				   ADS1298_MASK_CH_PD,
+				   test_bit(i, scan_mask) ?
+							0 : ADS1298_MASK_CH_PD);
Broken indentation.

+	}
...

+static void ads1298_rdata_unmark_busy(struct ads1298_private *priv)
+{
+	unsigned long flags;
+
+	/* Notify we're no longer waiting for the SPI transfer to complete */
+	spin_lock_irqsave(&priv->irq_busy_lock, flags);
+	priv->rdata_xfer_busy = false;
+	spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
Perhaps switch to use guard()?
(And scoped_guard() where it makes sense.)

+}
+		/* Transfer 24-bit value into 32-bit array */
+		memcpy(bounce + 1, data, 3);
Hmm... Wouldn't get_unaligned_..24() work here better?

+		bounce += 4;
If so, you can iterate over u32 members directly without this += 4.

...

+static const char *ads1298_family_name(unsigned int id)
+{
+	switch (id & 0xe0) {
GENMASK() ?

+	case 0x80:
+		return "ADS129x";
+	case 0xc0:
+		return "ADS129xR";
Can we have these all magics be defined?

+	default:
+		return "(unknown)";
+	}
+}
...

+	if ((val & 0x18) == 0x10) {
Ditto.

+		dev_info(dev, "Found %s, %d channels\n",
+			 ads1298_family_name(val),
+			 4 + 2 * (val & 0x07));
Ditto for 0x07.

+	} else {
+		dev_err(dev, "Unknown ID: 0x%x\n", val);
+		return -ENODEV;
+	}
...

+	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
+	if (indio_dev == NULL)
We do !indio_dev in this case.

+		return -ENOMEM;
...

+		ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
+					       priv->reg_vref);
One line?


+		if (ret)
+			return ret;
...

+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "Failed to get clk\n");
Ditto.

...

+		return dev_err_probe(dev, ret,
+				     "Failed to enable avdd regulator\n");
Ditto.

...

+	ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
+				       priv->reg_avdd);
Ditto.

...

+	priv->regmap = devm_regmap_init(dev, NULL, priv,
+					&ads1298_regmap_config);
Ditto.

...

+	/* Avoid giving all the same name, iio scope doesn't handle that well */
IIO

+	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "%s@%s",
+					 spi_get_device_id(spi)->name,
+					 dev_name(dev));
No error check?

+	if (reset_gpio) {
+		udelay(1); /* Minimum pulse width is 2 clocks at 2MHz */
How do you know it's 2MHz? Is it always the same on all platforms / setups?

+		gpiod_set_value(reset_gpio, 0);
+	} else {
+		ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
+		if (ret)
+			return dev_err_probe(dev, ret, "RESET failed\n");
+	}
+	/* Wait 18 clock cycles for reset command to complete */
Ditto.

+	udelay(9);
...

+	ret = devm_request_irq(&spi->dev, spi->irq, &ads1298_interrupt,
&spi->dev is different to dev?

+			       IRQF_TRIGGER_FALLING, indio_dev->name,
+			       indio_dev);
...

+	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
+					  &ads1298_setup_ops);
Ditto.

...

+static const struct spi_device_id ads1298_id[] = {
+	{ "ads1298",  },
Inner comma is not needed.

+	{ }
+};
...

+static const struct of_device_id ads1298_of_table[] = {
+	{ .compatible = "ti,ads1298" },
+	{ },
Drop the comma in terminator entry.

+};


--
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@xxxxxxxx
W: www.topic.nl








[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux