On 10/09/15 08:22, Sean Nyekjær wrote:
> Hi Jonathan
>
> Regarding ads868x_show_scales.
> Yes it is register values, the challenge here is the [±1.25*vref, +2.5*vref] and [±0.625*VREF, +1.25*VREF] results in the same scaling values. How can we control this?
Compute the actual scale and offsets and export them via *_available.
Then a write to either will change to relevant register setting.
So as per the ABI thought obviously it can be a bit fiddly to compute
the values!
Jonathan
>
> Sean
>
> On 2015-09-09 21:33, Jonathan Cameron wrote:
>> 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
--
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
