On 06/10/15 09:12, Sean Nyekjaer wrote:
> This patch adds support for the Texas Intruments ADS8688 ADC.
>
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@xxxxxxxxx>
> Reviewed-by: Martin Hundebøll <martin.hundeboll@xxxxxxxxx>
only one issue that I can see. You do need locking to protect
your local buffers. A local lock in the same structure would be
appropriate for this.
Neither sysfs nor IIO provide any serialization guarantees
so multiple concurrent reads are more than possible and may
cause you data corruption as it stands.
Other than that, it's looking very nice.
Thanks,
Jonathan
> ---
> Changes since v3:
> - fixed multiline comments
> - write_raw_get_fmt _SCALE is nano, _OFFSET is int
>
> Changes since v2:
> - Removed unused variables
> - Removed unnecessary mutex lock
> - Range array is a enum
> - Made the read and write functions less complex and easier to read
> - Added inline comments
> - Added callback to write_raw_get_fmt
>
> 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-ads8688.c | 472 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 483 insertions(+)
> create mode 100644 drivers/iio/adc/ti-ads8688.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7868c74..4135d17 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -333,6 +333,16 @@ config TI_ADC128S052
> This driver can also be built as a module. If so, the module will be
> called ti-adc128s052.
>
> +config TI_ADS8688
> + tristate "Texas Instruments ADS8688"
> + 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-ads8688.
> +
> 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 99b37a9..04ab2c8 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -32,6 +32,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_ADS8688) += ti-ads8688.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-ads8688.c b/drivers/iio/adc/ti-ads8688.c
> new file mode 100644
> index 0000000..537bb0a
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads8688.c
> @@ -0,0 +1,472 @@
> +/*
> + * 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 ADS8688_CMD_REG(x) (x << 8)
> +#define ADS8688_CMD_REG_NOOP 0x00
> +#define ADS8688_CMD_REG_RST 0x85
> +#define ADS8688_CMD_REG_MAN_CH(chan) (0xC0 | (4 * chan))
> +#define ADS8688_CMD_DONT_CARE_BITS 16
> +
> +#define ADS8688_PROG_REG(x) (x << 9)
> +#define ADS8688_PROG_REG_RANGE_CH(chan) (0x05 + chan)
> +#define ADS8688_PROG_WR_BIT BIT(8)
> +#define ADS8688_PROG_DONT_CARE_BITS 8
> +
> +#define ADS8688_REG_PLUSMINUS25VREF 0
> +#define ADS8688_REG_PLUSMINUS125VREF 1
> +#define ADS8688_REG_PLUSMINUS0625VREF 2
> +#define ADS8688_REG_PLUS25VREF 5
> +#define ADS8688_REG_PLUS125VREF 6
> +
> +#define ADS8688_VREF_MV 4096
> +#define ADS8688_REALBITS 16
> +
> +/*
> + * enum ads8688_range - ADS8688 reference voltage range
> + * @ADS8688_PLUSMINUS25VREF: Device is configured for input range ±2.5 * VREF
> + * @ADS8688_PLUSMINUS125VREF: Device is configured for input range ±1.25 * VREF
> + * @ADS8688_PLUSMINUS0625VREF: Device is configured for input range ±0.625 * VREF
> + * @ADS8688_PLUS25VREF: Device is configured for input range 0 - 2.5 * VREF
> + * @ADS8688_PLUS125VREF: Device is configured for input range 0 - 1.25 * VREF
> + */
> +enum ads8688_range {
> + ADS8688_PLUSMINUS25VREF,
> + ADS8688_PLUSMINUS125VREF,
> + ADS8688_PLUSMINUS0625VREF,
> + ADS8688_PLUS25VREF,
> + ADS8688_PLUS125VREF,
> +};
> +
> +struct ads8688_chip_info {
> + const struct iio_chan_spec *channels;
> + unsigned int num_channels;
> +};
> +
> +struct ads8688_state {
> + const struct ads8688_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regulator *reg;
> + unsigned int vref_mv;
> + enum ads8688_range range[8];
Oops, missed this before. You need to have locking around
your databuffer. There is nothing in sysfs or the IIO
core to prevent multiple concurrent reads..
I note you mention removing an unecessary lock. I think that
was a missunderstanding. Locking is necessary, just not
using the IIO mutex that exists to protect against state
change issues (enabling buffers for example).
So there should be a local lock to protect this next element.
> + union {
> + __be32 d32;
> + u8 d8[4];
> + } data[2] ____cacheline_aligned;
> +};
> +
> +enum ads8688_id {
> + ID_ADS8684,
> + ID_ADS8688,
> +};
> +
> +struct ads8688_ranges {
> + enum ads8688_range range;
> + unsigned int scale;
> + int offset;
> + u8 reg;
> +};
> +
> +static const struct ads8688_ranges ads8688_range_def[5] = {
> + {
> + .range = ADS8688_PLUSMINUS25VREF,
> + .scale = 76295,
> + .offset = -(1 << (ADS8688_REALBITS - 1)),
> + .reg = ADS8688_REG_PLUSMINUS25VREF,
> + }, {
> + .range = ADS8688_PLUSMINUS125VREF,
> + .scale = 38148,
> + .offset = -(1 << (ADS8688_REALBITS - 1)),
> + .reg = ADS8688_REG_PLUSMINUS125VREF,
> + }, {
> + .range = ADS8688_PLUSMINUS0625VREF,
> + .scale = 19074,
> + .offset = -(1 << (ADS8688_REALBITS - 1)),
> + .reg = ADS8688_REG_PLUSMINUS0625VREF,
> + }, {
> + .range = ADS8688_PLUS25VREF,
> + .scale = 38148,
> + .offset = 0,
> + .reg = ADS8688_REG_PLUS25VREF,
> + }, {
> + .range = ADS8688_PLUS125VREF,
> + .scale = 19074,
> + .offset = 0,
> + .reg = ADS8688_REG_PLUS125VREF,
> + }
> +};
> +
> +static ssize_t ads8688_show_scales(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ads8688_state *st = iio_priv(dev_to_iio_dev(dev));
> +
> + return sprintf(buf, "0.%09u 0.%09u 0.%09u\n",
> + ads8688_range_def[0].scale * st->vref_mv,
> + ads8688_range_def[1].scale * st->vref_mv,
> + ads8688_range_def[2].scale * st->vref_mv);
> +}
> +
> +static ssize_t ads8688_show_offsets(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "%d %d\n", ads8688_range_def[0].offset,
> + ads8688_range_def[3].offset);
> +}
> +
> +static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO,
> + ads8688_show_scales, NULL, 0);
> +static IIO_DEVICE_ATTR(in_voltage_offset_available, S_IRUGO,
> + ads8688_show_offsets, NULL, 0);
> +
> +static struct attribute *ads8688_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 ads8688_attribute_group = {
> + .attrs = ads8688_attributes,
> +};
> +
> +#define ADS8688_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[] = {
> + ADS8688_CHAN(0),
> + ADS8688_CHAN(1),
> + ADS8688_CHAN(2),
> + ADS8688_CHAN(3),
> +};
> +
> +static const struct iio_chan_spec ads8688_channels[] = {
> + ADS8688_CHAN(0),
> + ADS8688_CHAN(1),
> + ADS8688_CHAN(2),
> + ADS8688_CHAN(3),
> + ADS8688_CHAN(4),
> + ADS8688_CHAN(5),
> + ADS8688_CHAN(6),
> + ADS8688_CHAN(7),
> +};
> +
> +static int ads8688_prog_write(struct iio_dev *indio_dev, unsigned int addr,
> + unsigned int val)
> +{
> + struct ads8688_state *st = iio_priv(indio_dev);
> + u32 tmp;
> +
> + tmp = ADS8688_PROG_REG(addr) | ADS8688_PROG_WR_BIT | val;
> + tmp <<= ADS8688_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 ads8688_reset(struct iio_dev *indio_dev)
> +{
> + struct ads8688_state *st = iio_priv(indio_dev);
> + u32 tmp;
> +
> + tmp = ADS8688_CMD_REG(ADS8688_CMD_REG_RST);
> + tmp <<= ADS8688_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 ads8688_read(struct iio_dev *indio_dev, unsigned int chan)
> +{
> + struct ads8688_state *st = iio_priv(indio_dev);
> + int ret;
> + u32 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 = ADS8688_CMD_REG(ADS8688_CMD_REG_MAN_CH(chan));
> + tmp <<= ADS8688_CMD_DONT_CARE_BITS;
> + st->data[0].d32 = cpu_to_be32(tmp);
> +
> + tmp = ADS8688_CMD_REG(ADS8688_CMD_REG_NOOP);
> + tmp <<= ADS8688_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 ads8688_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long m)
> +{
> + int ret, offset;
> + unsigned long scale_mv;
> +
> + struct ads8688_state *st = iio_priv(indio_dev);
> +
> + switch (m) {
> + case IIO_CHAN_INFO_RAW:
> + ret = ads8688_read(indio_dev, chan->channel);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + scale_mv = st->vref_mv;
> + scale_mv *= ads8688_range_def[st->range[chan->channel]].scale;
> + *val = 0;
> + *val2 = scale_mv;
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_OFFSET:
> + offset = ads8688_range_def[st->range[chan->channel]].offset;
> + *val = offset;
> + return IIO_VAL_INT;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ads8688_write_reg_range(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + enum ads8688_range range)
> +{
> + unsigned int tmp;
> + int ret;
> +
> + tmp = ADS8688_PROG_REG_RANGE_CH(chan->channel);
> + ret = ads8688_prog_write(indio_dev, tmp, range);
> +
> + return ret;
> +}
> +
> +static int ads8688_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ads8688_state *st = iio_priv(indio_dev);
> + unsigned int scale = 0;
> + int ret = -EINVAL, i, offset = 0;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + /* If the offset is 0 the ±2.5 * VREF mode is not available */
> + offset = ads8688_range_def[st->range[chan->channel]].offset;
> + if (offset == 0 && val2 == ads8688_range_def[0].scale * st->vref_mv)
> + return -EINVAL;
> +
> + /* Lookup new mode */
> + for (i = 0; i < ARRAY_SIZE(ads8688_range_def); i++)
> + if (val2 == ads8688_range_def[i].scale * st->vref_mv &&
> + offset == ads8688_range_def[i].offset) {
> + ret = ads8688_write_reg_range(indio_dev, chan,
> + ads8688_range_def[i].reg);
> + break;
> + }
> + break;
> + case IIO_CHAN_INFO_OFFSET:
> + /*
> + * There are only two available offsets:
> + * 0 and -(1 << (ADS8688_REALBITS - 1))
> + */
> + if (!(ads8688_range_def[0].offset == val ||
> + ads8688_range_def[3].offset == val))
> + return -EINVAL;
> +
> + /*
> + * If the device are in ±2.5 * VREF mode, it's not allowed to
> + * switch to a mode where the offset is 0
> + */
> + if (0 == val &&
> + st->range[chan->channel] == ADS8688_PLUSMINUS25VREF)
> + return -EINVAL;
> +
> + scale = ads8688_range_def[st->range[chan->channel]].scale;
> +
> + /* Lookup new mode */
> + for (i = 0; i < ARRAY_SIZE(ads8688_range_def); i++)
> + if (val == ads8688_range_def[i].offset &&
> + scale == ads8688_range_def[i].scale) {
> + ret = ads8688_write_reg_range(indio_dev, chan,
> + ads8688_range_def[i].reg);
> + break;
> + }
> + break;
> + }
> +
> + if (!ret)
> + st->range[chan->channel] = ads8688_range_def[i].range;
> +
> + return ret;
> +}
> +
> +static int ads8688_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_OFFSET:
> + return IIO_VAL_INT_PLUS_NANO;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct iio_info ads8688_info = {
> + .read_raw = &ads8688_read_raw,
> + .write_raw = &ads8688_write_raw,
> + .write_raw_get_fmt = &ads8688_write_raw_get_fmt,
> + .attrs = &ads8688_attribute_group,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static const struct ads8688_chip_info ads8688_chip_info_tbl[] = {
> + [ID_ADS8684] = {
> + .channels = ads8684_channels,
> + .num_channels = ARRAY_SIZE(ads8684_channels),
> + },
> + [ID_ADS8688] = {
> + .channels = ads8688_channels,
> + .num_channels = ARRAY_SIZE(ads8688_channels),
> + },
> +};
> +
> +static int ads8688_probe(struct spi_device *spi)
> +{
> + struct ads8688_state *st;
> + struct iio_dev *indio_dev;
> + 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 && of_property_read_bool(spi->dev.of_node,
> + "vref-supply")) {
> + 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 = ADS8688_VREF_MV;
> + }
> +
> + st->chip_info = &ads8688_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 = &ads8688_info;
> +
> + ads8688_reset(indio_dev);
> +
> + 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 ads8688_remove(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> + struct ads8688_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 ads8688_id[] = {
> + {"ads8684", ID_ADS8684},
> + {"ads8688", ID_ADS8688},
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, ads8688_id);
> +
> +static const struct of_device_id ads8688_of_match[] = {
> + { .compatible = "ti,ads8684" },
> + { .compatible = "ti,ads8688" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ads8688_of_match);
> +
> +static struct spi_driver ads8688_driver = {
> + .driver = {
> + .name = "ads8688",
> + .owner = THIS_MODULE,
> + },
> + .probe = ads8688_probe,
> + .remove = ads8688_remove,
> + .id_table = ads8688_id,
> +};
> +module_spi_driver(ads8688_driver);
> +
> +MODULE_AUTHOR("Sean Nyekjaer <sean.nyekjaer@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Texas Instruments ADS8688 driver");
> +MODULE_LICENSE("GPL v2");
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
