Jonathan Thanks for the review
On 12/08/2018 05:56 AM, Jonathan Cameron wrote: > On Tue, 4 Dec 2018 11:59:55 -0600 > Dan Murphy <dmur...@ti.com> wrote: > >> Introduce the TI ADS124S08 and the ADS124S06 ADC >> devices from TI. The ADS124S08 is the 12 channel ADC >> and the ADS124S06 is the 6 channel ADC device >> >> These devices share a common datasheet: >> http://www.ti.com/lit/gpn/ads124s08 >> >> Signed-off-by: Dan Murphy <dmur...@ti.com> > Various minor things inline. > No worries. I believe I used the ADS8688 driver as a reference so that driver may need to be updated as well > Thanks, > > Jonathan > >> --- >> drivers/iio/adc/Kconfig | 10 + >> drivers/iio/adc/Makefile | 1 + >> drivers/iio/adc/ti-ads124s08.c | 437 +++++++++++++++++++++++++++++++++ >> 3 files changed, 448 insertions(+) >> create mode 100644 drivers/iio/adc/ti-ads124s08.c >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index a52fea8749a9..e8c5686e6716 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -887,6 +887,16 @@ config TI_ADS8688 >> This driver can also be built as a module. If so, the module will be >> called ti-ads8688. >> >> +config TI_ADS124S08 >> + tristate "Texas Instruments ADS124S08" >> + depends on SPI && OF >> + help >> + If you say yes here you get support for Texas Instruments ADS124S08 >> + and ADS124S06 ADC chips >> + >> + This driver can also be built as a module. If so, the module will be >> + called ti-ads124s08. >> + >> config TI_AM335X_ADC >> tristate "TI's AM335X ADC driver" >> depends on MFD_TI_AM335X_TSCADC && HAS_DMA >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >> index a6e6a0b659e2..d70293abfdba 100644 >> --- a/drivers/iio/adc/Makefile >> +++ b/drivers/iio/adc/Makefile >> @@ -79,6 +79,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o >> obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o >> obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o >> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o >> +obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o >> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o >> obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o >> obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o >> diff --git a/drivers/iio/adc/ti-ads124s08.c b/drivers/iio/adc/ti-ads124s08.c >> new file mode 100644 >> index 000000000000..b6fc93f37355 >> --- /dev/null >> +++ b/drivers/iio/adc/ti-ads124s08.c >> @@ -0,0 +1,437 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// TI ADS124S0X chip family driver >> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > An oddity of current kernel style is that typically only the spdx > line is // > > The rest are a normal kernel multiline comment. Ack. Finding it is up to the maintainer here on the preference so I will change it. > >> + >> +#include <linux/err.h> >> +#include <linux/delay.h> >> +#include <linux/device.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_gpio.h> >> +#include <linux/slab.h> >> +#include <linux/sysfs.h> >> + >> +#include <linux/gpio/consumer.h> >> +#include <linux/regulator/consumer.h> >> +#include <linux/spi/spi.h> >> + >> +#include <linux/iio/iio.h> >> +#include <linux/iio/buffer.h> >> +#include <linux/iio/trigger_consumer.h> >> +#include <linux/iio/triggered_buffer.h> >> +#include <linux/iio/sysfs.h> >> + >> +#define ADS124S08_ID 0x00 >> +#define ADS124S06_ID 0x01 > > Use an enum for these as the actual values don't matter, only that > they are unique. > Ack. Is there a preference on using the sentinnal or not? >> + >> +/* Commands */ >> +#define ADS124S_CMD_NOP 0x00 > Why this shorter prefix? Are all ADS124S parts > compatible with this list? > Ack. Yes the 124S are all compatible since there are only 2 ATM. >> +#define ADS124S_CMD_WAKEUP 0x02 >> +#define ADS124S_CMD_PWRDWN 0x04 >> +#define ADS124S_CMD_RESET 0x06 >> +#define ADS124S_CMD_START 0x08 >> +#define ADS124S_CMD_STOP 0x0a >> +#define ADS124S_CMD_SYOCAL 0x16 >> +#define ADS124S_CMD_SYGCAL 0x17 >> +#define ADS124S_CMD_SFOCAL 0x19 >> +#define ADS124S_CMD_RDATA 0x12 >> +#define ADS124S_CMD_RREG 0x20 >> +#define ADS124S_CMD_WREG 0x40 >> + >> +/* Registers */ >> +#define ADS124S0X_ID 0x00 > Avoid wild cards in naming. It always goes wrong when > along comes another part that is similar but not quite the > same yet fits in your naming scheme. Just pick a part > and name after that. The issue is that there are some registers below that are S08 only and do not apply to the S06. This is why I choose the wild card. I can use the 08 since that has a master set. Unless I can keep the 0X > >> +#define ADS124S0X_STATUS 0x01 >> +#define ADS124S0X_INPUT_MUX 0x02 >> +#define ADS124S0X_PGA 0x03 >> +#define ADS124S0X_DATA_RATE 0x04 >> +#define ADS124S0X_REF 0x05 >> +#define ADS124S0X_IDACMAG 0x06 >> +#define ADS124S0X_IDACMUX 0x07 >> +#define ADS124S0X_VBIAS 0x08 >> +#define ADS124S0X_SYS 0x09 >> +#define ADS124S0X_OFCAL0 0x0a >> +#define ADS124S0X_OFCAL1 0x0b >> +#define ADS124S0X_OFCAL2 0x0c >> +#define ADS124S0X_FSCAL0 0x0d >> +#define ADS124S0X_FSCAL1 0x0e >> +#define ADS124S0X_FSCAL2 0x0f >> +#define ADS124S0X_GPIODAT 0x10 >> +#define ADS124S0X_GPIOCON 0x11 >> + >> +/* ADS124S0x common channels */ >> +#define ADS124S0X_AIN0 0x00 >> +#define ADS124S0X_AIN1 0x01 >> +#define ADS124S0X_AIN2 0x02 >> +#define ADS124S0X_AIN3 0x03 >> +#define ADS124S0X_AIN4 0x04 >> +#define ADS124S0X_AIN5 0x05 >> +#define ADS124S08_AINCOM 0x0c >> +/* ADS124S08 only channels */ >> +#define ADS124S08_AIN6 0x06 >> +#define ADS124S08_AIN7 0x07 >> +#define ADS124S08_AIN8 0x08 >> +#define ADS124S08_AIN9 0x09 >> +#define ADS124S08_AIN10 0x0a >> +#define ADS124S08_AIN11 0x0b >> +#define ADS124S0X_MAX_CHANNELS 12 >> + >> +#define ADS124S0X_POS_MUX_SHIFT 0x04 >> +#define ADS124S_INT_REF 0x09 >> + >> +#define ADS124S_START_REG_MASK 0x1f >> +#define ADS124S_NUM_BYTES_MASK 0x1f >> + >> +#define ADS124S_START_CONV 0x01 >> +#define ADS124S_STOP_CONV 0x00 >> + >> +struct ads124s_chip_info { >> + const struct iio_chan_spec *channels; >> + unsigned int num_channels; >> +}; >> + >> +struct ads124s_private { >> + const struct ads124s_chip_info *chip_info; >> + struct gpio_desc *reset_gpio; >> + struct spi_device *spi; >> + struct regulator *reg; >> + struct mutex lock; >> + unsigned int vref_mv; >> + u8 data[ADS124S0X_MAX_CHANNELS]; > This will break in horrible ways on spi controllers using DMA. > The data buffer needs to be in it's own cacheline to avoid > possibly overwriting data in the rest of this structure. > > ___cache_line_aligned is your friend. Wolfram did a very > good talk on this issue at elce. > https://events.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf > > There's a video on youtube as well. > > He was addressing i2c, but the arguments still hold and unlike > i2c, spi has always had dma controllers who assume they have > dma safe buffers. > ACK. I will look into this. Probably will do something similar to the 8688 buffer >> +}; >> + >> +#define ADS124S_CHAN(index) \ >> +{ \ >> + .type = IIO_VOLTAGE, \ >> + .indexed = 1, \ >> + .channel = index, \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > It looks like you started adding scale but didn't finish doing so. > It's definitely a good thing to have so please see if that can be > added for V2. Yes I did but could not figure out how to apply it to this part. I will investigate this in v2 > >> + .scan_index = index, \ >> + .scan_type = { \ >> + .sign = 'u', \ >> + .realbits = 24, \ >> + .storagebits = 24, \ >> + .endianness = IIO_BE, \ >> + }, \ >> +} >> + >> +static const struct iio_chan_spec ads124s06_channels[] = { >> + ADS124S_CHAN(0), >> + ADS124S_CHAN(1), >> + ADS124S_CHAN(2), >> + ADS124S_CHAN(3), >> + ADS124S_CHAN(4), >> + ADS124S_CHAN(5), >> +}; >> + >> +static const struct iio_chan_spec ads124s08_channels[] = { >> + ADS124S_CHAN(0), >> + ADS124S_CHAN(1), >> + ADS124S_CHAN(2), >> + ADS124S_CHAN(3), >> + ADS124S_CHAN(4), >> + ADS124S_CHAN(5), >> + ADS124S_CHAN(6), >> + ADS124S_CHAN(7), >> + ADS124S_CHAN(8), >> + ADS124S_CHAN(9), >> + ADS124S_CHAN(10), >> + ADS124S_CHAN(11), >> +}; >> + >> +static const struct ads124s_chip_info ads124s_chip_info_tbl[] = { >> + [ADS124S08_ID] = { >> + .channels = ads124s08_channels, >> + .num_channels = ARRAY_SIZE(ads124s08_channels), >> + }, >> + [ADS124S06_ID] = { >> + .channels = ads124s06_channels, >> + .num_channels = ARRAY_SIZE(ads124s06_channels), >> + }, >> +}; >> + >> +static void ads124s_fill_nop(u8 *data, int start, int count) >> +{ >> + int i; >> + >> + for (i = start; i <= count; i++) >> + data[i] = ADS124S_CMD_NOP; > > memset > ACK >> + >> +}; >> + >> +static int ads124s_write_cmd(struct iio_dev *indio_dev, u8 command) >> +{ >> + struct ads124s_private *priv = iio_priv(indio_dev); >> + struct spi_transfer t[] = { >> + { >> + .tx_buf = &priv->data[0], >> + .len = 1, >> + } >> + }; >> + >> + priv->data[0] = command; >> + >> + return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t)); > > spi_write? ACK > >> +} >> + >> +static int ads124s_write_reg(struct iio_dev *indio_dev, u8 reg, u8 data) >> +{ >> + struct ads124s_private *priv = iio_priv(indio_dev); >> + struct spi_transfer t[] = { >> + { >> + .tx_buf = &priv->data[0], >> + .len = 3, >> + } >> + }; >> + >> + priv->data[0] = ADS124S_CMD_WREG | reg; >> + priv->data[1] = 0x0; >> + priv->data[2] = data; >> + >> + return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t)); > > spi_write? > ACK >> +} >> + >> +static int ads124s_reset(struct iio_dev *indio_dev) >> +{ >> + struct ads124s_private *priv = iio_priv(indio_dev); >> + >> + if (priv->reset_gpio) { >> + gpiod_direction_output(priv->reset_gpio, 0); > I'd expect that gpio to always be in the output > direction, so just being controlled here. > > So gpiod_set_value ACK > >> + udelay(200); >> + gpiod_direction_output(priv->reset_gpio, 1); >> + } else { >> + return ads124s_write_cmd(indio_dev, ADS124S_CMD_RESET); >> + } >> + >> + return 0; >> +}; >> + >> +static int ads124s_read(struct iio_dev *indio_dev, unsigned int chan) >> +{ >> + struct ads124s_private *priv = iio_priv(indio_dev); >> + int ret; >> + u32 tmp; >> + struct spi_transfer t[] = { >> + { >> + .tx_buf = &priv->data[0], >> + .len = 4, >> + .cs_change = 1, >> + }, { >> + .tx_buf = &priv->data[1], >> + .rx_buf = &priv->data[1], >> + .len = 4, >> + }, >> + }; > > Pity we don't have a spi_write_the_read_with_cs or > something like that. I wonder how common this structure is? This is common with this part and the ads8688 and another spi part I am working on for CAN. I just don't know if there are any parts that hold CS for more then one data xfer. > >> + >> + priv->data[0] = ADS124S_CMD_RDATA; >> + ads124s_fill_nop(priv->data, 1, 3); >> + >> + ret = spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t)); >> + if (ret < 0) >> + return ret; >> + >> + tmp = priv->data[2] << 16 | priv->data[3] << 8 | priv->data[4]; >> + >> + return tmp; >> +} >> + >> +static int ads124s_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int *val, int *val2, long m) >> +{ >> + struct ads124s_private *priv = iio_priv(indio_dev); >> + int ret; >> + >> + mutex_lock(&priv->lock); >> + switch (m) { >> + case IIO_CHAN_INFO_RAW: >> + ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX, >> + chan->channel); >> + if (ret) { >> + dev_err(&priv->spi->dev, "Set ADC CH failed\n"); > > You are eating a bus error in favour of -EINVAL. Please don't! ACK. I was debating on setting ret here so I will set ret and return that instead. Rinse and repeat for the ones below too. > >> + goto out; >> + } >> + >> + ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV); >> + if (ret) { >> + dev_err(&priv->spi->dev, "Start ADC converions >> failed\n"); >> + goto out; >> + } >> + >> + ret = ads124s_read(indio_dev, chan->channel); >> + if (ret < 0) { >> + dev_err(&priv->spi->dev, "Read ADC failed\n"); >> + goto out; >> + } else >> + *val = ret; > > This is the same code block as found in the buffered version below. Factor > it out to a utility function and use it both paths. It is not exactly the same but I will see where I can consolidate the code. The buffered version loops through and reads every channel this reads just one. > >> + >> + ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV); >> + if (ret) { >> + dev_err(&priv->spi->dev, "Stop ADC converions >> failed\n"); >> + goto out; >> + } >> + >> + mutex_unlock(&priv->lock); >> + if (ret < 0) >> + return ret; > Given you need to unlock in both this path and the error path, cleaner > to just set ret here and break. Then return ret below having set > it appropriately in the error paths. ACK. Similar to the above. >> + >> + return IIO_VAL_INT; >> + default: >> + break; >> + } >> +out: >> + mutex_unlock(&priv->lock); >> + return -EINVAL; >> +} >> + >> +static const struct iio_info ads124s_info = { >> + .read_raw = &ads124s_read_raw, >> +}; >> + >> +static irqreturn_t ads124s_trigger_handler(int irq, void *p) >> +{ >> + struct iio_poll_func *pf = p; >> + struct iio_dev *indio_dev = pf->indio_dev; >> + struct ads124s_private *priv = iio_priv(indio_dev); >> + u16 buffer[ADS124S0X_MAX_CHANNELS]; > > There is an unfortunate oddity in how push_to_buffers_with_timestamp > works in that it builds the data for the kfifo inline in the buffer > provided. Thus that buffer has to have room for the channels and > the 64 bit timestamp. Hmmm. This is like the 8688. I am starting to think the 8688 needs to be updated. > >> + int i, j = 0; >> + int ret; >> + >> + for (i = 0; i < indio_dev->masklength; i++) { >> + if (!test_bit(i, indio_dev->active_scan_mask)) > > for_each_bit_set Same as above. But I can change it. > >> + continue; >> + >> + ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX, i); > > Does this need to be rewritten if the channel is already correct? This is an iterative loop so the channel should be different each time 0 - 12 > >> + if (ret) >> + dev_err(&priv->spi->dev, "Set ADC CH failed\n"); >> + >> + ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV); >> + if (ret) >> + dev_err(&priv->spi->dev, "Start ADC converions >> failed\n"); >> + >> + buffer[j] = ads124s_read(indio_dev, i); >> + ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV); >> + if (ret) >> + dev_err(&priv->spi->dev, "Stop ADC converions >> failed\n"); >> + >> + j++; >> + } >> + >> + iio_push_to_buffers_with_timestamp(indio_dev, buffer, >> + pf->timestamp); >> + >> + iio_trigger_notify_done(indio_dev->trig); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int ads124s_probe(struct spi_device *spi) >> +{ >> + struct ads124s_private *ads124s_priv; >> + struct iio_dev *indio_dev; >> + int ret; >> + >> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*ads124s_priv)); >> + if (indio_dev == NULL) >> + return -ENOMEM; >> + >> + ads124s_priv = iio_priv(indio_dev); >> + >> + ads124s_priv->reg = devm_regulator_get_optional(&spi->dev, "vref"); >> + if (!IS_ERR(ads124s_priv->reg)) { >> + ret = regulator_enable(ads124s_priv->reg); >> + if (ret) >> + return ret; > > As mentioned below, use devm_add_action_or_reset to provide > automatic disabling of this regulator letting you used managed > cleanup throughout. ACK. will look into these APIs > >> + >> + ads124s_priv->vref_mv = >> regulator_get_voltage(ads124s_priv->reg); > > This doesn't actually seem to be used... As a general rule the only time you > want to read this is to provide a scale value. As that isn't a fast path > it's better to read it where needed in case the value is changing (not unusual > during boot up if the voltage is being used for several things). I will look into this if I add the scale otherwise I will remove it. > >> + if (ads124s_priv->vref_mv <= 0) >> + goto err_regulator_disable; >> + } else { >> + /* Use internal reference */ >> + ads124s_priv->vref_mv = 0; >> + } >> + >> + ads124s_priv->reset_gpio = devm_gpiod_get_optional(&spi->dev, >> + "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(ads124s_priv->reset_gpio)) >> + dev_info(&spi->dev, "Reset GPIO not defined\n"); >> + >> + ads124s_priv->chip_info = >> &ads124s_chip_info_tbl[spi_get_device_id(spi)->driver_data]; >> + >> + spi_set_drvdata(spi, indio_dev); >> + >> + ads124s_priv->spi = spi; >> + >> + indio_dev->name = spi_get_device_id(spi)->name; >> + indio_dev->dev.parent = &spi->dev; >> + indio_dev->dev.of_node = spi->dev.of_node; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->channels = ads124s_priv->chip_info->channels; >> + indio_dev->num_channels = ads124s_priv->chip_info->num_channels; >> + indio_dev->info = &ads124s_info; >> + >> + mutex_init(&ads124s_priv->lock); >> + >> + ret = iio_triggered_buffer_setup(indio_dev, NULL, >> + ads124s_trigger_handler, NULL); >> + if (ret < 0) { >> + dev_err(&spi->dev, "iio triggered buffer setup failed\n"); >> + goto err_regulator_disable; >> + } >> + >> + ads124s_reset(indio_dev); >> + >> + ret = iio_device_register(indio_dev); >> + if (ret) >> + goto err_buffer_cleanup; >> + >> + return 0; >> + >> +err_buffer_cleanup: >> + iio_triggered_buffer_cleanup(indio_dev); >> + >> +err_regulator_disable: >> + if (!IS_ERR(ads124s_priv->reg)) >> + regulator_disable(ads124s_priv->reg); >> + >> + return ret; >> +} >> + >> +static int ads124s_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); > Both of these functions can be automatically cleaned > up if you use the devm_ variants of the registration functions > which makes me suspicious.. Ah. The regulator disable > is missing. Ack. Regulator is based on above > > You can move to fully managed if you use devm_add_action_or_reset > to turn the regulator off. > >> + >> + return 0; >> +} >> + >> +static const struct spi_device_id ads124s_id[] = { >> + { "ads124s08", ADS124S08_ID }, >> + { "ads124s06", ADS124S06_ID }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(spi, ads124s_id); >> + >> +static const struct of_device_id ads124s_of_table[] = { >> + { .compatible = "ti,ads124s08" }, >> + { .compatible = "ti,ads124s06" }, > > It's trivial but numerical order preferred as it makes it > obvious where to add new devices later. Actually I based these off the ID read from the device. The S08 ID is 0x0 and the S06 is 0x1. Bit I can reorder this since it just the compatible. Dan > >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, ads124s_of_table); >> + >> +static struct spi_driver ads124s_driver = { >> + .driver = { >> + .name = "ads124s", >> + .of_match_table = ads124s_of_table, >> + }, >> + .probe = ads124s_probe, >> + .remove = ads124s_remove, >> + .id_table = ads124s_id, >> +}; >> +module_spi_driver(ads124s_driver); >> + >> +MODULE_AUTHOR("Dan Murphy <dmup...@ti.com>"); >> +MODULE_DESCRIPTION("TI TI_ADS12S0X ADC"); >> +MODULE_LICENSE("GPL v2"); > -- ------------------ Dan Murphy
