Re: [PATCH v3 2/2] iio: adc: add driver for the ti-adc084s021 chip

On 03/05/17 13:01, Mårten Lindahl wrote:

From: Mårten Lindahl <redacted>

This adds support for the Texas Instruments ADC084S021 ADC chip.

Signed-off-by: Mårten Lindahl <redacted>

A few more minor bits and pieces.

Jonathan

You only need to do this for tx_buf as then rx_buf will at worst end up
in the same cacheline as it.  Fairly implausible that this would cause
issues given they will be read before a transfer and written after.
Doing it twice results in potential waste of say 64 bytes depending on
the cacheline length.

+};
+
+#define ADC084S021_VOLTAGE_CHANNEL(num)                  \
+	{                                                      \
+		.type = IIO_VOLTAGE,                                 \
+		.channel = (num),                                    \
+		.indexed = 1,                                        \
+		.scan_index = (num),                                 \
+		.scan_type = {                                       \
+			.sign = 'u',                                       \
+			.realbits = 8,                                     \
+			.storagebits = 16,                                 \
+			.shift = 4,                                        \
+			.endianness = IIO_BE,                              \

They aren't at the moment as you are doing the endian conversion in kernel.
Drop that in the push to buffers path.

+		},                                                   \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
+	}
+
+static const struct iio_chan_spec adc084s021_channels[] = {
+	ADC084S021_VOLTAGE_CHANNEL(0),
+	ADC084S021_VOLTAGE_CHANNEL(1),
+	ADC084S021_VOLTAGE_CHANNEL(2),
+	ADC084S021_VOLTAGE_CHANNEL(3),
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static int adc084s021_power_on(struct adc084s021 *adc)
+{
+	int ret;
+
+	ret = regulator_enable(adc->reg);
+	if (ret) {
+		dev_warn(&adc->spi->dev,
+			"Failed to enable supply voltage\n");
+	}
+
+	return ret;

These two little functions do seem a little unnecessary.
A regulator fail should be pretty unlikely so I'd be tempted
to just have the regulator_enable / disable inline instead
of off in these functions.

+}
+
+static int adc084s021_power_off(struct adc084s021 *adc)
+{
+	int ret;
+
+	ret = regulator_disable(adc->reg);
+	if (ret) {
+		dev_warn(&adc->spi->dev,
+			"Failed to disable supply voltage\n");
+	}
+
+	return ret;
+}
+
+/**
+ * Read an ADC channel and return its value.
+ *
+ * @adc: The ADC SPI data.
+ * @data: Buffer for converted data.
+ */
+static int adc084s021_adc_conversion(struct adc084s021 *adc, void *data)
+{
+	int n_words = (adc->spi_trans.len >> 1) - 1; /* Discard first word */
+	int ret, i = 0;
+	u16 *p = data;
+
+	/* Do the transfer */
+	ret = spi_sync(adc->spi, &adc->message);
+	if (ret < 0)
+		return ret;
+
+	for (; i < n_words; i++)
+		*(p + i) = be16_to_cpu(adc->rx_buf[i + 1]);

Should only do the endian conversion in kernel if not
using it for buffered output.  In buffered output we can
save a few cycles by leaving it up to userspace.

+
+	return ret;
+}
+
+static int adc084s021_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *channel, int *val,
+			   int *val2, long mask)
+{
+	struct adc084s021 *adc = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+
+		ret = adc084s021_power_on(adc);
+		if (ret) {
+			iio_device_release_direct_mode(indio_dev);
+			return ret;
+		}
+
+		adc->tx_buf[0] = channel->channel << 3;
+		ret = adc084s021_adc_conversion(adc, val);
+		iio_device_release_direct_mode(indio_dev);
+		adc084s021_power_off(adc);
+		if (ret < 0)
+			return ret;
+
+		*val = (*val >> channel->scan_type.shift) & 0xff;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = adc084s021_power_on(adc);
+		if (ret)
+			return ret;
+
+		ret = regulator_get_voltage(adc->reg);
+		adc084s021_power_off(adc);
+		if (ret < 0)
+			return ret;
+
+		*val = ret / 1000;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+/**
+ * Read enabled ADC channels and push data to the buffer.
+ *
+ * @irq: The interrupt number (not used).
+ * @pollfunc: Pointer to the poll func.
+ */
+static irqreturn_t adc084s021_buffer_trigger_handler(int irq, void *pollfunc)
+{
+	struct iio_poll_func *pf = pollfunc;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adc084s021 *adc = iio_priv(indio_dev);
+	__be16 data[8] = {0}; /* 4 * 16-bit words of data + 8 bytes timestamp */
+
+	mutex_lock(&adc->lock);
+
+	if (adc084s021_adc_conversion(adc, &data) < 0)
+		dev_err(&adc->spi->dev, "Failed to read data\n");

hmm. Not sure I'm keen on pushing garbage to the buffer.
I might fix this up as well... Depends on what else there is ;)

+
+	iio_push_to_buffers_with_timestamp(indio_dev, data,
+					   iio_get_time_ns(indio_dev));
+	mutex_unlock(&adc->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int adc084s021_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct adc084s021 *adc = iio_priv(indio_dev);
+	int scan_index;
+	int i = 0;
+
+	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		const struct iio_chan_spec *channel =
+			&indio_dev->channels[scan_index];
+		adc->tx_buf[i++] = channel->channel << 3;
+	}
+	adc->spi_trans.len = 2 + (i * sizeof(__be16)); /* Trash + channels */
+
+	return adc084s021_power_on(adc);
+}
+
+static int adc084s021_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct adc084s021 *adc = iio_priv(indio_dev);
+
+	adc->spi_trans.len = 4; /* Trash + single channel */
+
+	return adc084s021_power_off(adc);
+}
+
+static const struct iio_info adc084s021_info = {
+	.read_raw = adc084s021_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static const struct iio_buffer_setup_ops adc084s021_buffer_setup_ops = {
+	.preenable = adc084s021_buffer_preenable,
+	.postenable = iio_triggered_buffer_postenable,
+	.predisable = iio_triggered_buffer_predisable,
+	.postdisable = adc084s021_buffer_postdisable,
+};
+
+static int adc084s021_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct adc084s021 *adc;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+	if (!indio_dev) {
+		dev_err(&spi->dev, "Failed to allocate IIO device\n");
+		return -ENOMEM;
+	}
+
+	adc = iio_priv(indio_dev);
+	adc->spi = spi;
+
+	/* Connect the SPI device and the iio dev */
+	spi_set_drvdata(spi, indio_dev);
+
+	/* Initiate the Industrial I/O device */
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.of_node = spi->dev.of_node;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &adc084s021_info;
+	indio_dev->channels = adc084s021_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adc084s021_channels);
+
+	/* Create SPI transfer for channel reads */
+	adc->spi_trans.tx_buf = adc->tx_buf;
+	adc->spi_trans.rx_buf = adc->rx_buf;
+	adc->spi_trans.len = 4; /* Trash + single channel */
+	spi_message_init_with_transfers(&adc->message, &adc->spi_trans, 1);
+
+	adc->reg = devm_regulator_get(&spi->dev, "vref");
+	if (IS_ERR(adc->reg))
+		return PTR_ERR(adc->reg);
+
+	mutex_init(&adc->lock);
+
+	/* Setup triggered buffer with pollfunction */
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
+					    adc084s021_buffer_trigger_handler,
+					    &adc084s021_buffer_setup_ops);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to setup triggered buffer\n");
+		return ret;
+	}
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+
+	return ret;

return devm_iio_deivce_register(...

If this all there is I'll fix it up.

+}
+
+static const struct of_device_id adc084s021_of_match[] = {
+	{ .compatible = "ti,adc084s021", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, adc084s021_of_match);
+
+static const struct spi_device_id adc084s021_id[] = {
+	{ ADC084S021_DRIVER_NAME, 0},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, adc084s021_id);
+
+static struct spi_driver adc084s021_driver = {
+	.driver = {
+		.name = ADC084S021_DRIVER_NAME,
+		.of_match_table = of_match_ptr(adc084s021_of_match),
+	},
+	.probe = adc084s021_probe,
+	.id_table = adc084s021_id,
+};
+module_spi_driver(adc084s021_driver);
+
+MODULE_AUTHOR("Mårten Lindahl [off-list ref]");
+MODULE_DESCRIPTION("Texas Instruments ADC084S021");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("1.0");

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html