On 04/07/16 04:05, Matt Ranostay wrote:
> On Sun, Jul 3, 2016 at 6:04 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>> 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).
>
> There doesn't seem to be a trivial-devices.txt for spi device bindings.
Hmm. Must have imagined it!
>
>>> @@ -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
>
--
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
