On 25/09/15 07:29, Sean Nyekjaer wrote:
> This patch adds support for the Texas Intruments ADS868x ADC.
>
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@xxxxxxxxx>
> Reviewed-by: Martin Hundebøll <martin.hundeboll@xxxxxxxxx>
Hi
The driver is fundamentally good, but I think a few small changes would make
it less complex to read which is always a good thing!
Comments inline.
Jonathan
> ---
> Changes since v1:
> - Now possible to read and write the actual offset and scale values
> - Removed unused includes
> - Removed unused buffer references
>
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads868x.c | 456 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 467 insertions(+)
> create mode 100644 drivers/iio/adc/ti-ads868x.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index deea62c..39924d5 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -322,6 +322,16 @@ config TI_ADC128S052
> This driver can also be built as a module. If so, the module will be
> called ti-adc128s052.
>
> +config TI_ADS868X
> + tristate "Texas Instruments ADS8684/8"
> + depends on SPI && OF
> + help
> + If you say yes here you get support for Texas Instruments ADS8684 and
> + and ADS8688 ADC chips
> +
> + This driver can also be built as a module. If so, the module will be
> + called ti-ads868x.
> +
> config TI_AM335X_ADC
> tristate "TI's AM335X ADC driver"
> depends on MFD_TI_AM335X_TSCADC
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index fa5723a..75170d2 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
> obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> +obj-$(CONFIG_TI_ADS868X) += ti-ads868x.o
> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
> obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
> diff --git a/drivers/iio/adc/ti-ads868x.c b/drivers/iio/adc/ti-ads868x.c
> new file mode 100644
> index 0000000..66d9b64
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads868x.c
> @@ -0,0 +1,456 @@
> +/*
> + * Copyright (C) 2015 Prevas A/S
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define ADS868X_CMD_REG(x) (x << 8)
> +#define ADS868X_CMD_REG_NOOP 0x00
> +#define ADS868X_CMD_REG_RST 0x85
> +#define ADS868X_CMD_REG_MAN_CH(chan) (0xC0 | (4 * chan))
> +#define ADS868X_CMD_DONT_CARE_BITS 16
> +
> +#define ADS868X_PROG_REG(x) (x << 9)
> +#define ADS868X_PROG_REG_RANGE_CH(chan) (0x05 + chan)
> +#define ADS868X_PROG_WR_BIT BIT(8)
> +#define ADS868X_PROG_DONT_CARE_BITS 8
> +
> +#define ADS868X_VREF_MV 4096
> +#define ADS868X_REALBITS 16
> +
> +struct ads868x_chip_info {
> + unsigned int id;
> + const struct iio_chan_spec *channels;
> + unsigned int num_channels;
> + unsigned int flags;
flags isn't used that I can see.
> + const struct iio_info *iio_info;
Why bother? Right now you only have one iio_info structure for both
supported parts. Just use it directly and drop it form this structure.
> +};
> +
> +struct ads868x_state {
> + const struct ads868x_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regulator *reg;
> + unsigned int vref_mv;
prefer u8 type to a char as it clearly isn't actually a character.
See below for more detail, but I'd suggest having a contiguous enum to
reference into the below ranges structure then store that in your
device instance specific structure rather than these values.
It avoids a fair bit of searching! That would also change the type
of this to be an array of enums rather than u8/chars.
> + char range[8];
> + union {
> + __be32 d32;
> + u8 d8[4];
> + } data[2] ____cacheline_aligned;
> +};
> +
> +enum ads868x_id {
> + ID_ADS8684,
> + ID_ADS8688,
> +};
> +
> +enum ads868x_range {
> + ADS868X_PLUSMINUS25VREF = 0x00,
> + ADS868X_PLUSMINUS125VREF = 0x01,
> + ADS868X_PLUSMINUS0625VREF = 0x02,
> + ADS868X_PLUS25VREF = 0x05,
> + ADS868X_PLUS125VREF = 0x06,
> +};
> +
> +struct ads868x_ranges {
> + enum ads868x_range range;
> + unsigned int scale;
> + int offset;
> +};
> +
const
> +static struct ads868x_ranges ads868x_range_def[5] = {
> + {
> + .range = ADS868X_PLUSMINUS25VREF,
> + .scale = 76295,
> + .offset = -(1 << (ADS868X_REALBITS - 1)),
> + }, {
> + .range = ADS868X_PLUSMINUS125VREF,
> + .scale = 38148,
> + .offset = -(1 << (ADS868X_REALBITS - 1)),
> + }, {
> + .range = ADS868X_PLUSMINUS0625VREF,
> + .scale = 19074,
> + .offset = -(1 << (ADS868X_REALBITS - 1)),
> + }, {
> + .range = ADS868X_PLUS25VREF,
> + .scale = 38148,
> + .offset = 0,
> + }, {
> + .range = ADS868X_PLUS125VREF,
> + .scale = 19074,
> + .offset = 0,
> + }
> +};
> +
> +static ssize_t ads868x_show_scales(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ads868x_state *st = iio_priv(dev_to_iio_dev(dev));
> +
> + return sprintf(buf, "0.%09u 0.%09u 0.%09u\n",
> + ads868x_range_def[0].scale * st->vref_mv,
> + ads868x_range_def[1].scale * st->vref_mv,
> + ads868x_range_def[2].scale * st->vref_mv);
> +}
> +
> +static ssize_t ads868x_show_offsets(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%d %d\n", ads868x_range_def[0].offset,
> + ads868x_range_def[3].offset);
> +}
> +
> +static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO,
> + ads868x_show_scales, NULL, 0);
> +static IIO_DEVICE_ATTR(in_voltage_offset_available, S_IRUGO,
> + ads868x_show_offsets, NULL, 0);
> +
> +static struct attribute *ads868x_attributes[] = {
> + &iio_dev_attr_in_voltage_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage_offset_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group ads868x_attribute_group = {
> + .attrs = ads868x_attributes,
> +};
> +
> +#define ADS868X_CHAN(index) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = index, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \
> + | BIT(IIO_CHAN_INFO_SCALE) \
> + | BIT(IIO_CHAN_INFO_OFFSET), \
> +}
> +
> +static const struct iio_chan_spec ads8684_channels[] = {
> + ADS868X_CHAN(0),
> + ADS868X_CHAN(1),
> + ADS868X_CHAN(2),
> + ADS868X_CHAN(3),
> +};
> +
> +static const struct iio_chan_spec ads8688_channels[] = {
> + ADS868X_CHAN(0),
> + ADS868X_CHAN(1),
> + ADS868X_CHAN(2),
> + ADS868X_CHAN(3),
> + ADS868X_CHAN(4),
> + ADS868X_CHAN(5),
> + ADS868X_CHAN(6),
> + ADS868X_CHAN(7),
> +};
> +
> +static int ads868x_prog_write(struct iio_dev *indio_dev, unsigned int addr,
> + unsigned int val)
> +{
> + struct ads868x_state *st = iio_priv(indio_dev);
> + unsigned int tmp;
> +
> + tmp = ADS868X_PROG_REG(addr) | ADS868X_PROG_WR_BIT | val;
> + tmp <<= ADS868X_PROG_DONT_CARE_BITS;
> + st->data[0].d32 = cpu_to_be32(tmp);
> +
> + return spi_write(st->spi, &st->data[0].d8[1], 3);
> +}
> +
> +static int ads868x_reset(struct iio_dev *indio_dev)
> +{
> + struct ads868x_state *st = iio_priv(indio_dev);
> + unsigned int tmp;
> +
> + tmp = ADS868X_CMD_REG(ADS868X_CMD_REG_RST);
> + tmp <<= ADS868X_CMD_DONT_CARE_BITS;
> + st->data[0].d32 = cpu_to_be32(tmp);
> +
> + return spi_write(st->spi, &st->data[0].d8[0], 4);
> +}
> +
> +static int ads868x_read(struct iio_dev *indio_dev, unsigned int chan)
> +{
> + struct ads868x_state *st = iio_priv(indio_dev);
> + int ret;
> + unsigned int tmp;
> + struct spi_transfer t[] = {
> + {
> + .tx_buf = &st->data[0].d8[0],
> + .len = 4,
> + .cs_change = 1,
> + }, {
> + .tx_buf = &st->data[1].d8[0],
> + .rx_buf = &st->data[1].d8[0],
> + .len = 4,
> + },
> + };
> +
> + tmp = ADS868X_CMD_REG(ADS868X_CMD_REG_MAN_CH(chan));
> + tmp <<= ADS868X_CMD_DONT_CARE_BITS;
> + st->data[0].d32 = cpu_to_be32(tmp);
> +
> + tmp = ADS868X_CMD_REG(ADS868X_CMD_REG_NOOP);
> + tmp <<= ADS868X_CMD_DONT_CARE_BITS;
> + st->data[1].d32 = cpu_to_be32(tmp);
> +
> + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> + if (ret < 0)
> + return ret;
> +
> + return be32_to_cpu(st->data[1].d32) & 0xffff;
> +}
> +
> +static int ads868x_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long m)
> +{
> + int ret, offset, i;
> + unsigned long scale_mv;
> +
> + struct ads868x_state *st = iio_priv(indio_dev);
> +
> + switch (m) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&indio_dev->mlock);
> + ret = ads868x_read(indio_dev, chan->channel);
> + mutex_unlock(&indio_dev->mlock);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + scale_mv = st->vref_mv;
> + for (i = 0; i < ARRAY_SIZE(ads868x_range_def); i++) {
Having this lookup in several places seems overly complex.
If there weren't gaps in the ads868x_range, I'd suggest just using
that as an index, but clearly that's awkward here.
Perhaps you just need to define a new enum which doesn't correspond
directly to the register value and having a reg_value field in your
indexed structure alongside range etc.
That way your stored channel range enum entries will allow a direct
lookup rather than searching on each read for the right entry.
> + if (st->range[chan->channel] == ads868x_range_def[i].range)
> + scale_mv *= ads868x_range_def[i].scale;
> + }
> + *val = 0;
> + *val2 = scale_mv;
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_OFFSET:
> + for (i = 0; i < ARRAY_SIZE(ads868x_range_def); i++) {
> + if (st->range[chan->channel] == ads868x_range_def[i].range)
> + offset = ads868x_range_def[i].offset;
> + }
> + *val = offset;
> + return IIO_VAL_INT;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ads868x_write_reg_range(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + enum ads868x_range range)
> +{
> + unsigned int tmp;
> + int ret;
> +
> + tmp = ADS868X_PROG_REG_RANGE_CH(chan->channel);
Technically this lock is really meant to be device state changes (moving
in and out of buffered mode for example) rather than use in drivers to
protect bus accesses which is a much lower level. It probably doesn't actually
matter, but I'd prefer a locally defined lock for this.
> + mutex_lock(&indio_dev->mlock);
> + ret = ads868x_prog_write(indio_dev, tmp, range);
> + mutex_unlock(&indio_dev->mlock);
> +
> + return ret;
> +}
> +
> +static int ads868x_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ads868x_state *st = iio_priv(indio_dev);
> + unsigned int scale = 0;
> + int ret = -EINVAL, i, offset = 0;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + for (i = 0; i < ARRAY_SIZE(ads868x_range_def); i++)
> + if (st->range[chan->channel] ==
> + ads868x_range_def[i].range) {
> + offset = ads868x_range_def[i].offset;
> + if (offset == 0 &&
> + val2 == ads868x_range_def[0].scale *
> + st->vref_mv / 1000)
Is this a nasty trick of mess to avoid having iio_val_int_plus nano
on the write? Just provide the callback write_raw_get_fmt and keep
all your units the same across _avail, _read_raw and _write_raw.
> + return ret;
> + break;
> + }
> + for (i = 0; i < ARRAY_SIZE(ads868x_range_def); i++)
> + if (val2 ==
> + ads868x_range_def[i].scale * st->vref_mv / 1000 &&
> + offset == ads868x_range_def[i].offset) {
> + ret = ads868x_write_reg_range(indio_dev, chan,
> + ads868x_range_def[i].range);
> + }
> + break;
> + case IIO_CHAN_INFO_OFFSET:
The depth of nesting here is making this next block rather hard to read.
I'd be tempted to try breaking it out to a utility function thus dropping
at least one level of indentation.
A comment here to explain why only the two values are of interest.
(clearly these are the only choiced, but it's not obvious without searching
around for where they are defined).
> + if (!(ads868x_range_def[0].offset == val ||
> + ads868x_range_def[3].offset == val))
> + return ret;
return -EINVAL to make it obvious that we have an error here rather than
an uninteresting good return path.
> + if (0 == val &&
> + st->range[chan->channel] == ADS868X_PLUSMINUS25VREF)
> + return ret;
same here.
I'd also like a comment or two in here to help me understand what is happening.
First check is about establishing if we have a valid range and picking the
scale from that, the second about finding the right one to get the offset
as well? I can't see why these are separate or for that matter why you
don't stop looking once a good answer has been found.
Basically I'm confused :(
> + for (i = 0; i < ARRAY_SIZE(ads868x_range_def); i++)
> + if (st->range[chan->channel] ==
> + ads868x_range_def[i].range)
> + scale = ads868x_range_def[i].scale;
Found a scale for the current range?
> + for (i = 0; i < ARRAY_SIZE(ads868x_range_def); i++)
> + if (val == ads868x_range_def[i].offset &&
> + scale == ads868x_range_def[i].scale) {
Found an offset compatible with the current scale and hence range?
I'm clearly missing something here!
> + ret = ads868x_write_reg_range(indio_dev, chan,
> + ads868x_range_def[i].range);
> + }
> + break;
> + default:
> + ret = -EINVAL;
return -EINVAL then you don't need the if (!ret) below.
> + }
> +
> + if (!ret)
> + st->range[chan->channel] = ads868x_range_def[i].range;
> +
> + return ret;
> +}
> +
> +static const struct iio_info ads868x_info = {
> + .read_raw = &ads868x_read_raw,
> + .write_raw = &ads868x_write_raw,
> + .attrs = &ads868x_attribute_group,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static const struct ads868x_chip_info ads868x_chip_info_tbl[] = {
> + [ID_ADS8684] = {
> + .channels = ads8684_channels,
> + .num_channels = ARRAY_SIZE(ads8684_channels),
> + .iio_info = &ads868x_info,
> + },
> + [ID_ADS8688] = {
> + .channels = ads8688_channels,
> + .num_channels = ARRAY_SIZE(ads8688_channels),
> + .iio_info = &ads868x_info,
> + },
> +};
> +
> +static int ads868x_probe(struct spi_device *spi)
> +{
> + struct ads868x_state *st;
> + struct iio_dev *indio_dev;
> + bool ext_ref;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (indio_dev == NULL)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + if (spi->dev.of_node)
> + ext_ref = of_property_read_bool(spi->dev.of_node,
> + "vref-supply");
> + else
> + ext_ref = false;
> +
Could do this as
if (spi->dev.of_node && of_property_read_bool(spi->dev.of_node,
"vref-supply"))
I'm not entirely sure it's a good idea even if it saves introducing
a local variable. Up to you.
> + if (ext_ref) {
> + st->reg = devm_regulator_get(&spi->dev, "vref");
> + if (!IS_ERR(st->reg)) {
> + ret = regulator_enable(st->reg);
> + if (ret)
> + return ret;
> +
> + ret = regulator_get_voltage(st->reg);
> + if (ret < 0)
> + goto error_out;
> + st->vref_mv = ret / 1000;
> + }
> + } else {
> + /* Use internal reference */
> + st->vref_mv = ADS868X_VREF_MV;
> + }
> +
> + st->chip_info = &ads868x_chip_info_tbl[spi_get_device_id(spi)->driver_data];
> +
> + spi->mode = SPI_MODE_1;
> +
> + spi_set_drvdata(spi, indio_dev);
> +
> + st->spi = spi;
> +
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = st->chip_info->channels;
> + indio_dev->num_channels = st->chip_info->num_channels;
> + indio_dev->info = st->chip_info->iio_info;
> +
> + /* Reset ADS868x */
> + mutex_lock(&indio_dev->mlock);
> + ads868x_reset(indio_dev);
> + mutex_unlock(&indio_dev->mlock);
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto error_out;
> +
> + return 0;
> +
> +error_out:
> + if (!IS_ERR_OR_NULL(st->reg))
> + regulator_disable(st->reg);
> +
> + return ret;
> +}
> +
> +static int ads868x_remove(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> + struct ads868x_state *st = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> +
> + if (!IS_ERR_OR_NULL(st->reg))
> + regulator_disable(st->reg);
> +
> + return 0;
> +}
> +
> +static const struct spi_device_id ads868x_id[] = {
> + {"ads8684", ID_ADS8684},
> + {"ads8688", ID_ADS8688},
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, ads868x_id);
> +
> +static const struct of_device_id ads868x_of_match[] = {
> + { .compatible = "ti,ads8684" },
> + { .compatible = "ti,ads8688" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ads868x_of_match);
> +
> +static struct spi_driver ads868x_driver = {
> + .driver = {
> + .name = "ads868x",
> + .owner = THIS_MODULE,
> + },
> + .probe = ads868x_probe,
> + .remove = ads868x_remove,
> + .id_table = ads868x_id,
> +};
> +module_spi_driver(ads868x_driver);
> +
> +MODULE_AUTHOR("Sean Nyekjaer <sean.nyekjaer@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Texas Instruments ADS868x driver");
> +MODULE_LICENSE("GPL v2");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
