On 02/07/16 23:05, Matt Ranostay wrote:
> Add support for Texas Instruments ADC141S626, and ADC161S626 chips.
>
> Signed-off-by: Matt Ranostay <mranostay@xxxxxxxxx>
Few little points inline and: No wild cards in driver names!
(though I'm occasionally half asleep and let one in).
Storage bits should always be a power of 2. (I let some
of those slip in as well in the past... oops).
Otherwise a few tiny corners where things could, I think,
be a little more readable at the cost of slightly more code.
Basically a nice little driver.
Jonathan
> ---
> .../devicetree/bindings/iio/adc/ti-adc1x1s.txt | 16 ++
> drivers/iio/adc/Kconfig | 12 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-adc1x1s.c | 233 +++++++++++++++++++++
> 4 files changed, 262 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
> create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
> new file mode 100644
> index 000000000000..9cf7db434aee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
Arguably could go in spi trivial devices binding file (unless you are going
to suplement this soon anyway in which case it may be less churn to keep it
here).
> @@ -0,0 +1,16 @@
> +* Texas Instruments' ADC141S626, and ADC161S626 chip
> +
> +Required properties:
> + - compatible: Should be "ti,adc141s626" or "ti,ac161s626"
> + - reg: spi chip select number for the device
> +
> +Recommended properties:
> + - spi-max-frequency: Definition as per
> + Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Example:
> +adc@0 {
> + compatible = "ti,adc161s626";
> + reg = <0>;
> + spi-max-frequency = <4300000>;
> +};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5882e2..cb5e4ae3279d 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -414,6 +414,18 @@ config TI_ADC128S052
> This driver can also be built as a module. If so, the module will be
> called ti-adc128s052.
>
> +config TI_ADC1X1S
> + tristate "Texas Instruments ADC1X1S 1-channel differential ADCs"
> + depends on SPI
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + If you say yes here you get support for Texas Instruments ADC141S626,
> + and ADC161S626 chips.
> +
> + This driver can also be build as a module. If so, the module will be
> + called ti-adc1x1s.
> +
> config TI_ADS1015
> tristate "Texas Instruments ADS1015 ADC"
> depends on I2C && !SENSORS_ADS1015
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d46f972..855ff407fd0d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
> obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> +obj-$(CONFIG_TI_ADC1X1S) += ti-adc1x1s.o
> obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> diff --git a/drivers/iio/adc/ti-adc1x1s.c b/drivers/iio/adc/ti-adc1x1s.c
> new file mode 100644
> index 000000000000..889399968694
> --- /dev/null
> +++ b/drivers/iio/adc/ti-adc1x1s.c
> @@ -0,0 +1,233 @@
> +/*
> + * ti-adc1x1s.c - Support for the Texas Instruments 1-channel differential ADCs
> + *
> + * ADC Devices Supported:
> + * adc141s626 - 14-bit ADC
> + * adc161s626 - 16-bit ADC
> + *
> + * Copyright (C) 2016 Matt Ranostay <mranostay@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define TI_ADC_DRV_NAME "adc1x1s"
I really dislike wild cards.... Just pick a part and name it after that.
> +
> +enum {
> + TI_ADC141S626,
> + TI_ADC161S626,
> +};
> +
> +static const struct iio_chan_spec ti_adc141s626_channels[] = {
> + {
> + .type = IIO_VOLTAGE,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 14,
> + .storagebits = 16,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static const struct iio_chan_spec ti_adc161s626_channels[] = {
> + {
> + .type = IIO_VOLTAGE,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 24,
Storage bits should be a power of 2.
> + .shift = 6,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +struct ti_adc_data {
> + struct iio_dev *indio_dev;
> + struct spi_device *spi;
> + int read_size;
> +
enforce cacheline alignment or this is going to cause possible fun on
dma using spi devices.
> + u8 buffer[16]; /* 3 byte data + 5 byte pad + 8 byte timestamp */
> +};
> +
> +static irqreturn_t ti_adc_trigger_handler(int irq, void *private)
> +{
> + struct iio_poll_func *pf = private;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct ti_adc_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = spi_read(data->spi, &data->buffer, data->read_size);
> + if (!ret)
> + iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> + iio_get_time_ns());
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int ti_adc_read_measurement(struct ti_adc_data *data,
> + struct iio_chan_spec const *chan, int *val)
> +{
> + __be32 buf;
> + int ret;
> +
> + ret = spi_read(data->spi, (void *) &buf, data->read_size);
> + if (ret)
> + return ret;
> +
> + switch (data->read_size) {
> + case 2:
> + *val = be16_to_cpu(buf);
Inelegant as buf is clearly marked as 32 bit by it's type. I'd move the
read into the switch statement then read into the appropriate sized
local variable. More code but easier to read (slightly)
> + break;
> + case 3:
> + *val = be32_to_cpu(buf) >> 8;
> + break;
> + }
> +
> + *val = sign_extend32(*val >> chan->scan_type.shift,
> + chan->scan_type.realbits - 1);
> +
> + return 0;
> +}
> +
> +static int ti_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ti_adc_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + if (iio_device_claim_direct_mode(indio_dev))
> + return -EBUSY;
Should technically store and return the result of iio_device_claim_direct_mode.
It could in future return something else..
> +
> + ret = ti_adc_read_measurement(data, chan, val);
> + if (!ret)
> + ret = IIO_VAL_INT;
I'd make ti_adc_read_measurement return IIO_VAL_INT if successful. Then
the slightly odd good path handling goes away.
> +
> + iio_device_release_direct_mode(indio_dev);
> +
> + return ret;
> +}
> +
> +static const struct iio_info ti_adc_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = ti_adc_read_raw,
> +};
> +
> +static int ti_adc_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ti_adc_data *data;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + indio_dev->info = &ti_adc_info;
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->dev.of_node = spi->dev.of_node;
> + indio_dev->name = TI_ADC_DRV_NAME;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + spi_set_drvdata(spi, indio_dev);
> +
> + data = iio_priv(indio_dev);
> + data->spi = spi;
> +
> + switch (spi_get_device_id(spi)->driver_data) {
> + case TI_ADC141S626:
> + indio_dev->channels = ti_adc141s626_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ti_adc141s626_channels);
> + data->read_size = 2;
> + break;
> + case TI_ADC161S626:
> + indio_dev->channels = ti_adc161s626_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ti_adc161s626_channels);
> + data->read_size = 3;
> + break;
> + }
> +
> + ret = iio_triggered_buffer_setup(indio_dev, NULL,
> + ti_adc_trigger_handler, NULL);
> + if (ret)
> + return ret;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto error_unreg_buffer;
> +
> + return 0;
> +
> +error_unreg_buffer:
> + iio_triggered_buffer_cleanup(indio_dev);
> +
> + return ret;
> +}
> +
> +static int ti_adc_remove(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +
> + iio_device_unregister(indio_dev);
> + iio_triggered_buffer_cleanup(indio_dev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ti_adc_dt_ids[] = {
> + { .compatible = "ti,adc141s626", },
> + { .compatible = "ti,adc161s626", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ti_adc_dt_ids);
> +
> +static const struct spi_device_id ti_adc_id[] = {
> + {"adc141s626", TI_ADC141S626},
> + {"adc161s626", TI_ADC161S626},
> + {},
> +};
> +MODULE_DEVICE_TABLE(spi, ti_adc_id);
> +
> +static struct spi_driver ti_adc_driver = {
> + .driver = {
> + .name = TI_ADC_DRV_NAME,
> + .of_match_table = of_match_ptr(ti_adc_dt_ids),
> + },
> + .probe = ti_adc_probe,
> + .remove = ti_adc_remove,
> + .id_table = ti_adc_id,
> +};
> +module_spi_driver(ti_adc_driver);
> +
> +MODULE_AUTHOR("Matt Ranostay <mranostay@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Texas Instruments ADC1x1S 1-channel differential ADC");
> +MODULE_LICENSE("GPL");
>
--
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
