On 3 May 2017 10:13:04 BST, "Mårten Lindahl" <marten.lindahl@xxxxxxxx> wrote: >On Sun, 2017-04-30 at 16:51 +0100, Jonathan Cameron wrote: >> On 30/04/17 13:53, Mårten Lindahl wrote: >> > From: Mårten Lindahl <martenli@xxxxxxxx> >> > >> > This adds support for the Texas Instruments ADC084S021 ADC chip. >> > >> > Signed-off-by: Mårten Lindahl <martenli@xxxxxxxx> >> A few more bits inline. Mostly stuff that has come up >> in the V2 changes (and the inevitable bits I missed the >> first time!) >> >> Jonathan > >Hi Jonathan! Please see my comments. I will send v3 today. >Thanks, >Mårten > >> > --- >> > Changes in v2: >> > - Removed most #defines in favor of inlines >> > - Corrected channel macro >> > - Removed configuration array with only one item >> > - Updated func adc084s021_adc_conversion to use be16_to_cpu >> > - Added IIO_CHAN_INFO_SCALE to func adc084s021_read_raw >> > - Use iio_device_claim_direct_mode in func adc084s021_read_raw >> > - Removed documentation for standard driver functions >> > - Changed retval to ret everywhere >> > - Removed dynamic alloc for data buffer in trigger handler >> > - Keeping mutex for all iterations in trigger handler >> > - Removed usage of events in this driver >> > - Removed info log in probe >> > - Use spi_message_init_with_transfers for spi message structs >> > - Use preenable and postdisable functions for regulator >> > - Inserted blank line before last return in all functions >> > >> > drivers/iio/adc/Kconfig | 12 ++ >> > drivers/iio/adc/Makefile | 1 + >> > drivers/iio/adc/ti-adc084s021.c | 294 >++++++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 307 insertions(+) >> > create mode 100644 drivers/iio/adc/ti-adc084s021.c >> > >> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> > index dedae7a..13141e5 100644 >> > --- a/drivers/iio/adc/Kconfig >> > +++ b/drivers/iio/adc/Kconfig >> > @@ -560,6 +560,18 @@ config TI_ADC0832 >> > This driver can also be built as a module. If so, the module >will be >> > called ti-adc0832. >> > >> > +config TI_ADC084S021 >> > + tristate "Texas Instruments ADC084S021" >> > + depends on SPI >> > + select IIO_BUFFER >> > + select IIO_TRIGGERED_BUFFER >> > + help >> > + If you say yes here you get support for Texas Instruments >ADC084S021 >> > + chips. >> > + >> > + This driver can also be built as a module. If so, the module >will be >> > + called ti-adc084s021. >> > + >> > config TI_ADC12138 >> > tristate "Texas Instruments ADC12130/ADC12132/ADC12138" >> > depends on SPI >> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >> > index d001262..b1a6158 100644 >> > --- a/drivers/iio/adc/Makefile >> > +++ b/drivers/iio/adc/Makefile >> > @@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o >> > obj-$(CONFIG_STM32_ADC) += stm32-adc.o >> > obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o >> > obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o >> > +obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o >> > obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o >> > obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o >> > obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o >> > diff --git a/drivers/iio/adc/ti-adc084s021.c >b/drivers/iio/adc/ti-adc084s021.c >> > new file mode 100644 >> > index 0000000..2dce257 >> > --- /dev/null >> > +++ b/drivers/iio/adc/ti-adc084s021.c >> > @@ -0,0 +1,294 @@ >> > +/** >> > + * Copyright (C) 2017 Axis Communications AB >> > + * >> > + * Driver for Texas Instruments' ADC084S021 ADC chip. >> > + * Datasheets can be found here: >> > + * http://www.ti.com/lit/ds/symlink/adc084s021.pdf >> > + * >> > + * 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/err.h> >> > +#include <linux/spi/spi.h> >> > +#include <linux/module.h> >> > +#include <linux/interrupt.h> >> > +#include <linux/iio/iio.h> >> > +#include <linux/iio/buffer.h> >> > +#include <linux/iio/triggered_buffer.h> >> > +#include <linux/iio/trigger_consumer.h> >> > +#include <linux/regulator/consumer.h> >> > + >> > +#define ADC084S021_DRIVER_NAME "adc084s021" >> > + >> > +struct adc084s021 { >> > + struct spi_device *spi; >> > + struct spi_message message; >> > + struct spi_transfer spi_trans[2]; >> > + struct regulator *reg; >> > + struct mutex lock; >> > + /* >> > + * DMA (thus cache coherency maintenance) requires the >> > + * transfer buffers to live in their own cache lines. >> > + */ >> > + union { >> > + u16 tx_buf; >> > + __be16 rx_buf; >> > + } ____cacheline_aligned; >> > +}; >> > + >> > +/** >> > + * Channel specification >> > + */ >> Comment doesn't add much so I'd drop it. >Fixed in v3. >> > +#define ADC084S021_VOLTAGE_CHANNEL(num) \ >> > + { \ >> > + .type = IIO_VOLTAGE, \ >> > + .channel = (num), \ >> > + .address = (num) << 3, \ >> This does feel a little pointless as you can just do the shift inline >and >> use the channel value instead. Doesn't really matter though. >Ok, fixed in v3. >> > + .indexed = 1, \ >> > + .scan_index = (num), \ >> > + .scan_type = { \ >> > + .sign = 'u', \ >> > + .realbits = 8, \ >> > + .storagebits = 16, \ >> > + .shift = 4, \ >> > + .endianness = IIO_BE, \ >> > + }, \ >> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\ >> > + } >> > + >> > +static const struct iio_chan_spec adc084s021_channels[] = { >> > + ADC084S021_VOLTAGE_CHANNEL(0), >> > + ADC084S021_VOLTAGE_CHANNEL(1), >> > + ADC084S021_VOLTAGE_CHANNEL(2), >> > + ADC084S021_VOLTAGE_CHANNEL(3), >> > + IIO_CHAN_SOFT_TIMESTAMP(4), >> > +}; >> > + >> > +/** >> > + * Read an ADC channel and return its value. >> > + * >> > + * @adc: The ADC SPI data. >> > + * @channel: The IIO channel data structure. >> > + */ >> > +static int adc084s021_adc_conversion(struct adc084s021 *adc, >> > + struct iio_chan_spec const *channel) >> > +{ >> > + u8 value; >> > + int ret; >> > + >> > + adc->tx_buf = channel->address; >> > + >> > + /* Do the transfer */ >> > + ret = spi_sync(adc->spi, &adc->message); >> > + if (ret < 0) >> > + return ret; >> > + >> > + value = (be16_to_cpu(adc->rx_buf) >> channel->scan_type.shift) & >0xff;You want to do this for the read_raw sysfs calls, but not the >buffered ones >> (where this shift and mask is made userspace's problem). >Fixed in v3. >> > + >> > + dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n", >> > + value, channel->channel); >> > + >> > + return value; >> > +} >> > + >> > +static int adc084s021_read_raw(struct iio_dev *indio_dev, >> > + struct iio_chan_spec const *channel, int *val, >> > + int *val2, long mask) >> > +{ >> > + struct adc084s021 *adc = iio_priv(indio_dev); >> > + int ret; >> > + >> > + switch (mask) { >> > + case IIO_CHAN_INFO_RAW: >> > + ret = iio_device_claim_direct_mode(indio_dev); >> > + if (ret < 0) >> > + return ret; >> > + >> Unless I'm dozing off this morning, you don't have any power.. >> So you'll need to enable the regulator first in this path. >Fixed in v3. >> >> Note that the runtime power management autosuspend stuff can >> be good for this as it stops the power cycling if a short burst >> of readings is taken. >I don't have much experience of the pm_runtime system, so I looked >around to the other drivers but I see very few users of it. I can >absolutely look into it if you think it should be applied to this >driver. I never insist on people implementing features they personally don't use as long as they don't prevent them being added later! Hence DT bindings need to be easy to extend or complete... So here it is entirely up to you. If someone else needs it down the line they can add it. Nice to have though. If you want to do it I would suggest a follow up patch so it doesn't delay the bit that matters to you. In the grand scheme of things runtime pm is relatively new (a few years) and not necessary for a driver to work. >> > + ret = adc084s021_adc_conversion(adc, channel); >> > + iio_device_release_direct_mode(indio_dev); >> > + if (ret < 0) >> > + return ret; >> > + >> > + *val = ret; >> > + >> > + return IIO_VAL_INT; >> > + case IIO_CHAN_INFO_SCALE: >> > + ret = regulator_get_voltage(adc->reg); >> Given the regulator is currently off as far as this device is >concerned >> it's possible the answer will be 0... >Fixed in v3. >> > + if (ret < 0) >> > + return ret; >> > + >> > + *val = ret / 1000; >> > + >> > + return IIO_VAL_INT; >> > + default: >> > + return -EINVAL; >> > + } >> > +} >> > + >> > +/** >> > + * Read enabled ADC channels and push data to the buffer. >> > + * >> > + * @irq: The interrupt number (not used). >> > + * @pollfunc: Pointer to the poll func. >> > + */ >> > +static irqreturn_t adc084s021_buffer_trigger_handler(int irq, void >*pollfunc) >> > +{ >> > + struct iio_poll_func *pf = pollfunc; >> > + struct iio_dev *indio_dev = pf->indio_dev; >> > + struct adc084s021 *adc = iio_priv(indio_dev); >> > + __be16 data[8] = {0}; /* 4 * 16-bit words of data + 8 bytes >timestamp */ >> > + int scan_index; >> > + int i = 0; >> > + >> > + mutex_lock(&adc->lock); >> > + >> > + for_each_set_bit(scan_index, indio_dev->active_scan_mask, >> > + indio_dev->masklength) { >> > + const struct iio_chan_spec *channel = >> > + &indio_dev->channels[scan_index]; >> > + data[i++] = adc084s021_adc_conversion(adc, channel); >> This is clearly a rather simplistic way of handling the read outs. >> Perfectly good for an initial version (which may never get improved >> on!) but you could do the spi transfers for a set of channels much >> more efficiently. >> >> Figure 1 on the datasheet shows how the control register for the next >> read can be written in parallel with the previous read. That would >> mean each scan took N + 1 2 byte transfers rather than 2N as >currently. >> >> I'm not particularly suggesting you do this, but thought I'd just >> comment on the possibility as it is a common situation with spi >> ADCs (sometimes you get a greater lag between setup and read out >> making this even fiddlier!) >> >> If you do want to do this, the trick is to do your transfer setup >> and creation stuff in preenable so that it can be customised for >> whatever channels are enabled. >Ok, Fixed in v3. >I have looked into this and I think I improved the design according to >your suggestion. Please note: According to the datasheet (section 8.3) >after each power up the ADC will read out last scanned/first channel, >which will result in a very first read of 16 bits that is ignored. >Also, >the readings are setup by only one transfer segment now. Cool >> > + } >> > + >> > + iio_push_to_buffers_with_timestamp(indio_dev, data, >> > + iio_get_time_ns(indio_dev)); >> > + mutex_unlock(&adc->lock); >> > + iio_trigger_notify_done(indio_dev->trig); >> > + >> > + return IRQ_HANDLED; >> > +} >> > + >> > +static int adc084s021_buffer_preenable(struct iio_dev *indio_dev) >> > +{ >> > + struct adc084s021 *adc = iio_priv(indio_dev); >> > + >> > + return regulator_enable(adc->reg); >> > +} >> > + >> > +static int adc084s021_buffer_postdisable(struct iio_dev >*indio_dev) >> > +{ >> > + struct adc084s021 *adc = iio_priv(indio_dev); >> > + >> > + return regulator_disable(adc->reg); >> > +} >> > + >> > +static const struct iio_info adc084s021_info = { >> > + .read_raw = adc084s021_read_raw, >> > + .driver_module = THIS_MODULE, >> > +}; >> > + >> > +static const struct iio_buffer_setup_ops >adc084s021_buffer_setup_ops = { >> > + .preenable = adc084s021_buffer_preenable, >> > + .postenable = iio_triggered_buffer_postenable, >> > + .predisable = iio_triggered_buffer_predisable, >> > + .postdisable = adc084s021_buffer_postdisable, >> > +}; >> > + >> > +static int adc084s021_probe(struct spi_device *spi) >> > +{ >> > + struct iio_dev *indio_dev; >> > + struct adc084s021 *adc; >> > + int ret; >> > + >> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); >> > + if (!indio_dev) { >> > + dev_err(&spi->dev, "Failed to allocate IIO device\n"); >> > + return -ENOMEM; >> > + } >> > + >> > + adc = iio_priv(indio_dev); >> > + adc->spi = spi; >> > + spi->bits_per_word = 8; >> > + >> > + /* Update the SPI device with config and connect the iio dev */ >> > + ret = spi_setup(spi); >> > + if (ret) { >> > + dev_err(&spi->dev, "Failed to update SPI device\n"); >> > + return ret; >> > + } >> > + spi_set_drvdata(spi, indio_dev); >> > + >> > + /* Initiate the Industrial I/O device */ >> > + indio_dev->dev.parent = &spi->dev; >> > + indio_dev->dev.of_node = spi->dev.of_node; >> > + indio_dev->name = spi_get_device_id(spi)->name; >> > + indio_dev->modes = INDIO_DIRECT_MODE; >> > + indio_dev->info = &adc084s021_info; >> > + indio_dev->channels = adc084s021_channels; >> > + indio_dev->num_channels = ARRAY_SIZE(adc084s021_channels); >> > + >> > + /* Create SPI transfer for channel reads */ >> > + adc->spi_trans[0].tx_buf = &adc->tx_buf; >> > + adc->spi_trans[0].len = 2; >> > + adc->spi_trans[0].speed_hz = spi->max_speed_hz; >> You don't need to set these for individual transfers unless they >> are different from how the spi bus has been set up... >> >> It's handled in __spi_validate in drivers/spi/spi.c >> >> list_for_each_entry(xfer, &message->transfers, transfer_list) { >> message->frame_length += xfer->len; >> if (!xfer->bits_per_word) >> xfer->bits_per_word = spi->bits_per_word; >> >> if (!xfer->speed_hz) >> xfer->speed_hz = spi->max_speed_hz; >> ... >> >> So it will set them spi->... in the core if you haven't overridden. >Fixed in v3. >> >> > + adc->spi_trans[0].bits_per_word = spi->bits_per_word; >> > + adc->spi_trans[1].rx_buf = &adc->rx_buf; >> > + adc->spi_trans[1].len = 2; >> > + adc->spi_trans[1].speed_hz = spi->max_speed_hz; >> > + adc->spi_trans[1].bits_per_word = spi->bits_per_word; >> > + >> > + spi_message_init_with_transfers(&adc->message, adc->spi_trans, >2); >> > + >> > + adc->reg = devm_regulator_get(&spi->dev, "vref"); >> > + if (IS_ERR(adc->reg)) >> > + return PTR_ERR(adc->reg); >> > + >> > + mutex_init(&adc->lock); >> > + >> > + /* Setup triggered buffer with pollfunction */ >> > + ret = iio_triggered_buffer_setup(indio_dev, NULL, >> > + adc084s021_buffer_trigger_handler, >> > + &adc084s021_buffer_setup_ops); >> > + if (ret) { >> > + dev_err(&spi->dev, "Failed to setup triggered buffer\n"); >> > + return ret; >> > + } >> > + >> > + ret = iio_device_register(indio_dev); >> > + if (ret) { >> > + dev_err(&spi->dev, "Failed to register IIO device\n"); >> > + iio_triggered_buffer_cleanup(indio_dev); >> With devm usage this won't be needed any more. >Fixed in v3. >> >> Hmm. We should improve the error message from the core as it'll allow >us >> to remove a lot of boiler plate in drivers. Ah well, a project for >> another day. >I removed this message in v3. >> > + } >> > + >> > + return ret; >> > +} >> > + >> > +static int adc084s021_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); >> Now you have moved to dynamically enabling the regulator, it makes >> sense to use the devm_ versions of iio_device_register and >> iio_triggered_buffer setup. >> >> With those, you'll be able to get rid of the remove callback entirely >as >> there will be nothing to do. >Fixed in v3. >> > + >> > + return 0; >> > +} >> > + >> > +static const struct of_device_id adc084s021_of_match[] = { >> > + { .compatible = "ti,adc084s021", }, >> > + {}, >> > +}; >> > +MODULE_DEVICE_TABLE(of, adc084s021_of_match); >> > + >> > +static const struct spi_device_id adc084s021_id[] = { >> > + { ADC084S021_DRIVER_NAME, 0}, >> > + {} >> > +}; >> > +MODULE_DEVICE_TABLE(spi, adc084s021_id); >> > + >> > +static struct spi_driver adc084s021_driver = { >> > + .driver = { >> > + .name = ADC084S021_DRIVER_NAME, >> > + .of_match_table = of_match_ptr(adc084s021_of_match), >> > + }, >> > + .probe = adc084s021_probe, >> > + .remove = adc084s021_remove, >> > + .id_table = adc084s021_id, >> > +}; >> > +module_spi_driver(adc084s021_driver); >> > + >> > +MODULE_AUTHOR("Mårten Lindahl <martenli@xxxxxxxx>"); >> > +MODULE_DESCRIPTION("Texas Instruments ADC084S021"); >> > +MODULE_LICENSE("GPL v2"); >> > +MODULE_VERSION("1.0"); >> > >> > > >-- >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 -- Sent from my Android device with K-9 Mail. Please excuse my brevity. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
