Message ID | 20230710042723.46084-2-kimseer.paller@analog.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | [v9,1/2] dt-bindings: iio: adc: add max14001 | expand |
On Mon, Jul 10, 2023 at 7:27 AM Kim Seer Paller <kimseer.paller@analog.com> wrote: > > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range > binary inputs. ... > V8 -> V9: Changed SPI buffer data types to __le16, Why? ... > + __le16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN); > + __le16 spi_rx_buffer; ... > + /* > + * Prepare SPI transmit buffer 16 bit-value to big-endian format and > + * reverses bit order to align with the LSB-first input on SDI port. reverse > + */ > + st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK, > + reg_addr))); ... > + /* > + * Convert received 16-bit value from big-endian to little-endian format > + * and reverses bit order. reverse > + */ > + *data = bitrev16(be16_to_cpu(st->spi_rx_buffer)); ... > + /* > + * Prepare SPI transmit buffer 16 bit-value to big-endian format and > + * reverses bit order to align with the LSB-first input on SDI port. reverse > + */ > + st->spi_tx_buffer = bitrev16(cpu_to_be16( > + FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) | > + FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) | > + FIELD_PREP(MAX14001_DATA_MASK, data))); Obviously it's incorrect now even more than before. The types are defined as __le, while ops are against __be.
On Mon, Jul 10, 2023 at 10:36 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jul 10, 2023 at 7:27 AM Kim Seer Paller > <kimseer.paller@analog.com> wrote: ... > > V8 -> V9: Changed SPI buffer data types to __le16, > > Why? > > ... > > > + __le16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN); > > + __le16 spi_rx_buffer; > > ... > > > + /* > > + * Prepare SPI transmit buffer 16 bit-value to big-endian format and > > + * reverses bit order to align with the LSB-first input on SDI port. > > reverse > > > + */ > > + st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK, > > + reg_addr))); > > ... > > > + /* > > + * Convert received 16-bit value from big-endian to little-endian format > > + * and reverses bit order. > > reverse > > > + */ > > + *data = bitrev16(be16_to_cpu(st->spi_rx_buffer)); On top of that, this left unfixed. ... > > + /* > > + * Prepare SPI transmit buffer 16 bit-value to big-endian format and > > + * reverses bit order to align with the LSB-first input on SDI port. > > reverse > > > + */ > > + st->spi_tx_buffer = bitrev16(cpu_to_be16( > > + FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) | > > + FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) | > > + FIELD_PREP(MAX14001_DATA_MASK, data))); > > Obviously it's incorrect now even more than before. > The types are defined as __le, while ops are against __be.
Return to the public space of the discussion. On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer <KimSeer.Paller@analog.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Monday, July 10, 2023 3:37 PM > > On Mon, Jul 10, 2023 at 7:27 AM Kim Seer Paller <kimseer.paller@analog.com> > > wrote: ... > > > V8 -> V9: Changed SPI buffer data types to __le16, > > > > Why? > > Based on the previous comments, I have taken the __le16 data type > into account. The device seems to function the same as the __be data type. > I have not yet sure but technically speaking, do I have to retain the data > types as __be16 based on the overall operation? If the type is __be, the *be*() APIs should be used, otherwise __le and *le*(). ... > > Obviously it's incorrect now even more than before. > > The types are defined as __le, while ops are against __be. > > Would it be right to implement this by reverting the types back to __be? > What other considerations could there be? First of all, you need to document what you are doing with these bit twiddlers. Based on the clear understanding by everyone we can suggest what data type(s) suits the best. Hence instead of v10, reply with a draft of the comment in the code (I have asked before) that explains these bit twiddlers.
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Monday, July 10, 2023 6:50 PM > To: Paller, Kim Seer <KimSeer.Paller@analog.com>; Hennerich, Michael > <Michael.Hennerich@analog.com> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de> > Subject: Re: [PATCH v9 2/2] iio: adc: max14001: New driver > > [External] > > Return to the public space of the discussion. Oh late to notice, I had mistakenly set my email client to "quick reply" instead of 'reply to all'. > On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer > <KimSeer.Paller@analog.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Sent: Monday, July 10, 2023 3:37 PM > > > On Mon, Jul 10, 2023 at 7:27 AM Kim Seer Paller > <kimseer.paller@analog.com> > > > wrote: > > ... > > > > > V8 -> V9: Changed SPI buffer data types to __le16, > > > > > > Why? > > > > Based on the previous comments, I have taken the __le16 data type > > into account. The device seems to function the same as the __be data type. > > I have not yet sure but technically speaking, do I have to retain the data > > types as __be16 based on the overall operation? > > If the type is __be, the *be*() APIs should be used, otherwise __le and *le*(). I became a little confused with the previous discussions, will make the necessary changes accordingly. > > > Obviously it's incorrect now even more than before. > > > The types are defined as __le, while ops are against __be. > > > > Would it be right to implement this by reverting the types back to __be? > > What other considerations could there be? > > First of all, you need to document what you are doing with these bit > twiddlers. Based on the clear understanding by everyone we can suggest > what data type(s) suits the best. > > Hence instead of v10, reply with a draft of the comment in the code (I > have asked before) that explains these bit twiddlers. In patch v9, regarding with my bit arrangement comments, is it somewhat correct or do I need to totally replace it? I am not yet familiar with the terminologies, so I hope you can provide some suggestions and I'll definitely send the draft first. Thanks, Kim
On Tue, Jul 11, 2023 at 9:55 AM Paller, Kim Seer <KimSeer.Paller@analog.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer > > <KimSeer.Paller@analog.com> wrote: ... > > Hence instead of v10, reply with a draft of the comment in the code (I > > have asked before) that explains these bit twiddlers. > > In patch v9, regarding with my bit arrangement comments, is it somewhat correct > or do I need to totally replace it? > > I am not yet familiar with the terminologies, so I hope you can provide some > suggestions and I'll definitely send the draft first. I'm not sure I understand what comments you are referring to. The v9 does not explain the algorithm clearly. What you need is to cite or retell what the datasheet explains about bit ordering along with the proposed algo (in AN as far as I understood). Because I haven't got, why do you need to use be16 + bitrev if your data is le16 (and that's my understanding of the datasheet). Is it because of the answer from the device? I don't remember if it keep the bit order the same (i.e. D0...D9) as on the wire. For the terminology, use what the datasheet and AN provide you. Also good to put those URLs to the code and datasheet as Datasheet: tag in the commit message.
On Tue, 11 Jul 2023 12:08:04 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jul 11, 2023 at 9:55 AM Paller, Kim Seer > <KimSeer.Paller@analog.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer > > > <KimSeer.Paller@analog.com> wrote: > > ... > > > > Hence instead of v10, reply with a draft of the comment in the code (I > > > have asked before) that explains these bit twiddlers. > > > > In patch v9, regarding with my bit arrangement comments, is it somewhat correct > > or do I need to totally replace it? > > > > I am not yet familiar with the terminologies, so I hope you can provide some > > suggestions and I'll definitely send the draft first. > > I'm not sure I understand what comments you are referring to. > The v9 does not explain the algorithm clearly. > > What you need is to cite or retell what the datasheet explains about > bit ordering along with the proposed algo (in AN as far as I > understood). Because I haven't got, why do you need to use be16 + > bitrev if your data is le16 (and that's my understanding of the > datasheet). Is it because of the answer from the device? I don't > remember if it keep the bit order the same (i.e. D0...D9) as on the > wire. > > For the terminology, use what the datasheet and AN provide you. Also > good to put those URLs to the code and datasheet as Datasheet: tag in > the commit message. > Absolutely agree. The data format is weird enough we need the info to be available for anyone who tries to work out what is going on in the future. This is a case where I'd rather see too much detail in the comments than too little! Jonathan
On Sun, 16 Jul 2023 14:42:23 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Tue, 11 Jul 2023 12:08:04 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jul 11, 2023 at 9:55 AM Paller, Kim Seer > > <KimSeer.Paller@analog.com> wrote: > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > On Mon, Jul 10, 2023 at 11:17 AM Paller, Kim Seer > > > > <KimSeer.Paller@analog.com> wrote: > > > > ... > > > > > > Hence instead of v10, reply with a draft of the comment in the code (I > > > > have asked before) that explains these bit twiddlers. > > > > > > In patch v9, regarding with my bit arrangement comments, is it somewhat correct > > > or do I need to totally replace it? > > > > > > I am not yet familiar with the terminologies, so I hope you can provide some > > > suggestions and I'll definitely send the draft first. > > > > I'm not sure I understand what comments you are referring to. > > The v9 does not explain the algorithm clearly. > > > > What you need is to cite or retell what the datasheet explains about > > bit ordering along with the proposed algo (in AN as far as I > > understood). Because I haven't got, why do you need to use be16 + > > bitrev if your data is le16 (and that's my understanding of the > > datasheet). Is it because of the answer from the device? I don't > > remember if it keep the bit order the same (i.e. D0...D9) as on the > > wire. > > > > For the terminology, use what the datasheet and AN provide you. Also > > good to put those URLs to the code and datasheet as Datasheet: tag in > > the commit message. > > > > Absolutely agree. The data format is weird enough we need the > info to be available for anyone who tries to work out what is going > on in the future. This is a case where I'd rather see too much detail > in the comments than too little! > > Jonathan Note that I had applied (and forgotten I'd done so) this driver. Given the outstanding questions and a build issue with clang, I'm dropping it for now. Hopefully that doesn't cause anyone too many problems (those based on my togreg branch which rarely rebases) Jonathan
diff --git a/MAINTAINERS b/MAINTAINERS index 0253058c345b..bd4bbe4f1749 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12797,6 +12797,7 @@ L: linux-iio@vger.kernel.org S: Supported W: https://ez.analog.com/linux-software-drivers F: Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml +F: drivers/iio/adc/max14001.c MAXBOTIX ULTRASONIC RANGER IIO DRIVER M: Andreas Klinger <ak@it-klinger.de> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index dc14bde31ac1..bdd5033b0733 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -706,6 +706,16 @@ config MAX11410 To compile this driver as a module, choose M here: the module will be called max11410. +config MAX14001 + tristate "Analog Devices MAX14001 ADC driver" + depends on SPI + help + Say yes here to build support for Analog Devices MAX14001 Configurable, + Isolated 10-bit ADCs for Multi-Range Binary Inputs. + + To compile this driver as a module, choose M here: the module will be + called max14001. + config MAX1241 tristate "Maxim max1241 ADC driver" depends on SPI_MASTER diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index eb6e891790fb..5a825cf9066f 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -65,6 +65,7 @@ obj-$(CONFIG_MAX11100) += max11100.o obj-$(CONFIG_MAX1118) += max1118.o obj-$(CONFIG_MAX11205) += max11205.o obj-$(CONFIG_MAX11410) += max11410.o +obj-$(CONFIG_MAX14001) += max14001.o obj-$(CONFIG_MAX1241) += max1241.o obj-$(CONFIG_MAX1363) += max1363.o obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c new file mode 100644 index 000000000000..09686a9faa2e --- /dev/null +++ b/drivers/iio/adc/max14001.c @@ -0,0 +1,325 @@ +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) +/* + * Analog Devices MAX14001 ADC driver + * + * Copyright 2023 Analog Devices Inc. + */ + +#include <linux/bitfield.h> +#include <linux/bitrev.h> +#include <linux/device.h> +#include <linux/iio/iio.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> +#include <linux/types.h> + +/* MAX14001 Registers Address */ +#define MAX14001_ADC 0x00 +#define MAX14001_FADC 0x01 +#define MAX14001_FLAGS 0x02 +#define MAX14001_FLTEN 0x03 +#define MAX14001_THL 0x04 +#define MAX14001_THU 0x05 +#define MAX14001_INRR 0x06 +#define MAX14001_INRT 0x07 +#define MAX14001_INRP 0x08 +#define MAX14001_CFG 0x09 +#define MAX14001_ENBL 0x0A +#define MAX14001_ACT 0x0B +#define MAX14001_WEN 0x0C + +#define MAX14001_VERIFICATION_REG(x) ((x) + 0x10) + +#define MAX14001_CFG_EXRF BIT(5) + +#define MAX14001_ADDR_MASK GENMASK(15, 11) +#define MAX14001_DATA_MASK GENMASK(9, 0) +#define MAX14001_FILTER_MASK GENMASK(3, 2) + +#define MAX14001_SET_WRITE_BIT BIT(10) +#define MAX14001_WRITE_WEN 0x294 + +struct max14001_state { + struct spi_device *spi; + /* + * lock protect against multiple concurrent accesses, RMW sequence, and + * SPI transfer + */ + struct mutex lock; + int vref_mv; + /* + * DMA (thus cache coherency maintenance) requires the + * transfer buffers to live in their own cache lines. + */ + __le16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN); + __le16 spi_rx_buffer; +}; + +static int max14001_read(void *context, unsigned int reg_addr, unsigned int *data) +{ + struct max14001_state *st = context; + int ret; + + struct spi_transfer xfers[] = { + { + .tx_buf = &st->spi_tx_buffer, + .len = sizeof(st->spi_tx_buffer), + .cs_change = 1, + }, { + .rx_buf = &st->spi_rx_buffer, + .len = sizeof(st->spi_rx_buffer), + }, + }; + + /* + * Prepare SPI transmit buffer 16 bit-value to big-endian format and + * reverses bit order to align with the LSB-first input on SDI port. + */ + st->spi_tx_buffer = bitrev16(cpu_to_be16(FIELD_PREP(MAX14001_ADDR_MASK, + reg_addr))); + + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); + if (ret) + return ret; + + /* + * Convert received 16-bit value from big-endian to little-endian format + * and reverses bit order. + */ + *data = bitrev16(be16_to_cpu(st->spi_rx_buffer)); + + return 0; +} + +static int max14001_write(void *context, unsigned int reg_addr, unsigned int data) +{ + struct max14001_state *st = context; + + /* + * Prepare SPI transmit buffer 16 bit-value to big-endian format and + * reverses bit order to align with the LSB-first input on SDI port. + */ + st->spi_tx_buffer = bitrev16(cpu_to_be16( + FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) | + FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) | + FIELD_PREP(MAX14001_DATA_MASK, data))); + + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer)); +} + +static int max14001_write_verification_reg(struct max14001_state *st, + unsigned int reg_addr) +{ + unsigned int reg_data; + int ret; + + ret = max14001_read(st, reg_addr, ®_data); + if (ret) + return ret; + + return max14001_write(st, MAX14001_VERIFICATION_REG(reg_addr), reg_data); +} + +static int max14001_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct max14001_state *st = iio_priv(indio_dev); + unsigned int data; + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + mutex_lock(&st->lock); + ret = max14001_read(st, MAX14001_ADC, &data); + mutex_unlock(&st->lock); + if (ret < 0) + return ret; + + *val = data; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + *val = st->vref_mv; + *val2 = 10; + + return IIO_VAL_FRACTIONAL_LOG2; + default: + return -EINVAL; + } +} + +static const struct iio_info max14001_info = { + .read_raw = max14001_read_raw, +}; + +static const struct iio_chan_spec max14001_channels[] = { + { + .type = IIO_VOLTAGE, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + } +}; + +static void max14001_regulator_disable(void *data) +{ + struct regulator *reg = data; + + regulator_disable(reg); +} + +static int max14001_init(struct max14001_state *st) +{ + int ret; + + /* Enable SPI Registers Write */ + ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN); + if (ret) + return ret; + + /* + * Reads all registers and writes the values back to their appropriate + * verification registers to clear the Memory Validation fault. + */ + ret = max14001_write_verification_reg(st, MAX14001_FLTEN); + if (ret) + return ret; + + ret = max14001_write_verification_reg(st, MAX14001_THL); + if (ret) + return ret; + + ret = max14001_write_verification_reg(st, MAX14001_THU); + if (ret) + return ret; + + ret = max14001_write_verification_reg(st, MAX14001_INRR); + if (ret) + return ret; + + ret = max14001_write_verification_reg(st, MAX14001_INRT); + if (ret) + return ret; + + ret = max14001_write_verification_reg(st, MAX14001_INRP); + if (ret) + return ret; + + ret = max14001_write_verification_reg(st, MAX14001_CFG); + if (ret) + return ret; + + ret = max14001_write_verification_reg(st, MAX14001_ENBL); + if (ret) + return ret; + + /* Disable SPI Registers Write */ + return max14001_write(st, MAX14001_WEN, 0); +} + +static int max14001_probe(struct spi_device *spi) +{ + struct iio_dev *indio_dev; + struct max14001_state *st; + struct regulator *vref; + struct device *dev = &spi->dev; + unsigned int reg_data; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); + if (!indio_dev) + return -ENOMEM; + + st = iio_priv(indio_dev); + st->spi = spi; + + indio_dev->name = "max14001"; + indio_dev->info = &max14001_info; + indio_dev->channels = max14001_channels; + indio_dev->num_channels = ARRAY_SIZE(max14001_channels); + indio_dev->modes = INDIO_DIRECT_MODE; + + ret = max14001_init(st); + if (ret) + return ret; + + vref = devm_regulator_get_optional(dev, "vref"); + if (IS_ERR(vref)) { + if (PTR_ERR(vref) != -ENODEV) + return dev_err_probe(dev, PTR_ERR(vref), + "Failed to get vref regulator"); + + /* internal reference */ + st->vref_mv = 1250; + } else { + ret = regulator_enable(vref); + if (ret) + return dev_err_probe(dev, ret, + "Failed to enable vref regulators\n"); + + ret = devm_add_action_or_reset(dev, max14001_regulator_disable, + vref); + if (ret) + return ret; + + ret = regulator_get_voltage(vref); + if (ret < 0) + return dev_err_probe(dev, ret, + "Failed to get vref\n"); + + st->vref_mv = ret / 1000; + + /* + * Enable SPI registers write and select external voltage + * reference source for the ADC. + */ + ret = max14001_write(st, MAX14001_WEN, MAX14001_WRITE_WEN); + if (ret) + return ret; + + ret = max14001_read(st, MAX14001_CFG, ®_data); + if (ret) + return ret; + + reg_data |= FIELD_PREP(MAX14001_CFG_EXRF, 1); + + ret = max14001_write(st, MAX14001_CFG, reg_data); + if (ret) + return ret; + + /* Write Verification Register */ + ret = max14001_write_verification_reg(st, MAX14001_CFG); + if (ret) + return ret; + + /* Disable SPI Registers Write */ + ret = max14001_write(st, MAX14001_WEN, 0); + if (ret) + return ret; + } + + mutex_init(&st->lock); + + return devm_iio_device_register(dev, indio_dev); +} + +static const struct of_device_id max14001_of_match[] = { + { .compatible = "adi,max14001" }, + { } +}; +MODULE_DEVICE_TABLE(of, max14001_of_match); + +static struct spi_driver max14001_driver = { + .driver = { + .name = "max14001", + .of_match_table = max14001_of_match, + }, + .probe = max14001_probe, +}; +module_spi_driver(max14001_driver); + +MODULE_AUTHOR("Kim Seer Paller"); +MODULE_DESCRIPTION("MAX14001 ADC driver"); +MODULE_LICENSE("GPL");
The MAX14001 is configurable, isolated 10-bit ADCs for multi-range binary inputs. Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com> --- V8 -> V9: Changed SPI buffer data types to __le16, removed force cast and modified spi buffers bit sequence. Removed max14001_reg_update function and directly handled its functionality within the caller. Removed unused headers. V7 -> V8: Added a bitwise OR operation to combine the bitfield element with the reg_data value. V6 -> V7: Swapped ADC external vref source and regulator_get_voltage calls. Added forced cast and comment to addressed endian warning. V5 -> V6: Replaced fixed length assignment with dynamic size assignment in xfers struct initialization. Removed .channel assignment in max14001_channels definition. V4 -> V5: No changes. V3 -> V4: Removed regmap setup, structures, and include. MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 10 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/max14001.c | 325 +++++++++++++++++++++++++++++++++++++ 4 files changed, 337 insertions(+) create mode 100644 drivers/iio/adc/max14001.c