On 07/09/15 10:34, Sean Nyekjaer wrote:
A comment here describing the device and the functionality supported by
the driver would be good.
Also, there are device tree bindings in here. They should have
accompanying documentation (cc'd to the device tree list).
Various comments inline. Nothing particularly major, though be
careful to obey the ABI and not use magic values in attributes.
Jonathan
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@xxxxxxxxx>
> Reviewed-by: Martin Hundebøll <martin.hundeboll@xxxxxxxxx>
> ---
> drivers/iio/adc/Kconfig | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads868x.c | 398 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 409 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..368e22c
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads868x.c
> @@ -0,0 +1,398 @@
> +/*
> + * 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/delay.h>
Not used that I can immediately spot.
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
not used
> +#include <linux/of.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
Not currently used.
> +
> +#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 (1 << 8)
BIT Macro preferred.
> +#define ADS868X_PROG_DONT_CARE_BITS 8
> +
> +#define ADS868X_VREF_MV 4096
> +
> +struct ads868x_chip_info {
> + unsigned int id;
> + const struct iio_chan_spec *channels;
> + unsigned int num_channels;
> + unsigned int flags;
> + const struct iio_info *iio_info;
> +};
> +
> +struct ads868x_state {
> + const struct ads868x_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regulator *reg;
> + unsigned long vref_mv;
> + 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,
> +};
> +
> +static const char ads868x_range_def[] = {
> + ADS868X_PLUSMINUS25VREF,
> + ADS868X_PLUSMINUS125VREF,
> + ADS868X_PLUSMINUS0625VREF,
> + ADS868X_PLUS25VREF,
> + ADS868X_PLUS125VREF,
> +};
> +
> +static ssize_t ads868x_show_scales(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int i, len = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(ads868x_range_def); i++)
> + len += sprintf(buf + len, "%d ", ads868x_range_def[i]);
> +
> + len += sprintf(buf + len, "\n");
These values don't look like scales in appropriate units? They
look to be perhaps register values.
Scale should be such that (raw_value + offset)*scale = real value
as per the ABI in Documentation/ABI/testing/sysfs-bus-iio
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(in_voltage_scale_available, S_IRUGO,
> + ads868x_show_scales, NULL, 0);
> +
> +static struct attribute *ads868x_attributes[] = {
> + &iio_dev_attr_in_voltage_scale_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), \
If address == channel then no point in setting it, use channel instead.
> + .address = index, \
No buffer support as yet so no need for everything below here.
> + .scan_index = index, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_BE, \
> + }, \
> +}
> +
> +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));
Fiddly! Ah well.
> + 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;
> + 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;
> + switch (st->range[chan->channel]) {
> + case ADS868X_PLUSMINUS25VREF:
> + scale_mv *= 5;
> + break;
> + case ADS868X_PLUSMINUS125VREF:
> + case ADS868X_PLUS25VREF:
> + scale_mv = scale_mv * 25 / 10;
> + break;
> + case ADS868X_PLUSMINUS0625VREF:
> + case ADS868X_PLUS125VREF:
> + scale_mv = scale_mv * 125 / 100;
> + break;
> + default:
> + return -EINVAL;
> + }
> + *val = scale_mv;
> + *val2 = chan->scan_type.realbits;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + }
> +
> + return -EINVAL;
> +}
> +
> +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 tmp;
> + int ret, i;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + ret = -EINVAL;
> + for (i = 0; i < ARRAY_SIZE(ads868x_range_def); i++)
> + if (val != ads868x_range_def[i])
> + continue;
This logic is a bit flipped around to the more conventional loop
and break with the error set by checking if we reached the end
of the search range or not. I think I'd slightly prefer the more
conventional option. Personally I prefer to see error cases
as the 'exception' rather than set on the more natural flow through
the code.
> +
> + ret = 0;
> + st->range[chan->channel] = val;
> + mutex_lock(&indio_dev->mlock);
> + tmp = ADS868X_PROG_REG_RANGE_CH(chan->channel);
> + ads868x_prog_write(indio_dev, tmp, val);
check for write errors. Also the cached value should be set after
the write has succeeded not before.
> + mutex_unlock(&indio_dev->mlock);
mlock is really intended as a lock on the device operating mode, but
I suppose using it for other things in a driver that doesn't
handle device state changes (no buffer support) is fine if not
as obvious as a local lock might be.
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + 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;
> + enum ads868x_id drv_id;
I'm unconvinced this local drv_id adds anything in terms of readability.
If it's just here to avoid overly long lines, note the checkpatch warning
for that should be taken as a recomendation only!
> + 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;
> +
> + if (ext_ref) {
> + st->reg = devm_regulator_get(&spi->dev, "vref");
> + if (!IS_ERR_OR_NULL(st->reg)) {
As far as I can see from a quick look at get_regulator docs
(driver/regulator/core.c) which is called from the devm call
the null condition cannot occur. Hence IS_ERR is probably
more appropriate (and more commonly used).
> + 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;
> + }
> +
> + drv_id = spi_get_device_id(spi)->driver_data;
> + st->chip_info = &ads868x_chip_info_tbl[drv_id];
> +
> + 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 linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
