* [PATCH 1/2] Add AD7949 ADC driver family @ 2018-10-06 20:30 Charles-Antoine Couret 2018-10-06 20:30 ` [PATCH 2/2] Add ad7949 device tree bindings in documentation Charles-Antoine Couret 2018-10-07 16:25 ` [PATCH 1/2] Add AD7949 ADC driver family Jonathan Cameron 0 siblings, 2 replies; 8+ messages in thread From: Charles-Antoine Couret @ 2018-10-06 20:30 UTC (permalink / raw) To: linux-iio; +Cc: Charles-Antoine Couret Compatible with AD7682 and AD7689 chips. It is a Analog Devices ADC driver 14/16 bits 4/8 channels with SPI protocol Datasheet of the device: http://www.analog.com/media/en/technical-documentation/data-sheets/AD7949.pdf Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> --- drivers/iio/adc/Kconfig | 10 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad7949.c | 329 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 340 insertions(+) create mode 100644 drivers/iio/adc/ad7949.c diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 4a754921fb6f..42e66efff6c0 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -116,6 +116,16 @@ config AD7923 To compile this driver as a module, choose M here: the module will be called ad7923. +config AD7949 + tristate "Analog Devices AD7949 and similar ADCs driver" + depends on SPI + help + Say yes here to build support for Analog Devices + AD7949, AD7682, AD7689 8 Channel ADCs. + + To compile this driver as a module, choose M here: the + module will be called ad7949. + config AD799X tristate "Analog Devices AD799x ADC driver" depends on I2C diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 03db7b578f9c..88804c867aa9 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7766) += ad7766.o obj-$(CONFIG_AD7791) += ad7791.o obj-$(CONFIG_AD7793) += ad7793.o obj-$(CONFIG_AD7887) += ad7887.o +obj-$(CONFIG_AD7949) += ad7949.o obj-$(CONFIG_AD799X) += ad799x.o obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o obj-$(CONFIG_AT91_ADC) += at91_adc.o diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c new file mode 100644 index 000000000000..667636b476f8 --- /dev/null +++ b/drivers/iio/adc/ad7949.c @@ -0,0 +1,329 @@ +// SPDX-License-Identifier: GPL-2.0 +/* ad7949.c - Analog Devices ADC driver 14/16 bits 4/8 channels + * + * Copyright (C) 2018 CMC NV + * + * http://www.analog.com/media/en/technical-documentation/data-sheets/AD7949.pdf + */ + +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> +#include <linux/delay.h> + +#define AD7949_MASK_CFG 0x3C7F +#define AD7949_MASK_CHANNEL_SEL 0x380 +#define AD7949_MASK_TOTAL 0x3FFF +#define AD7949_OFFSET_CHANNEL_SEL 7 +#define AD7949_CFG_READ_BACK 0x1 +#define AD7949_CFG_REG_SIZE_BITS 14 + +enum { + HEIGHT_14BITS = 0, + QUAD_16BITS, + HEIGHT_16BITS, +}; + +struct ad7949_adc_spec { + u8 num_channels; + u8 resolution; +}; + +static const struct ad7949_adc_spec ad7949_adc_spec[] = { + [HEIGHT_14BITS] = { .num_channels = 8, .resolution = 14 }, + [QUAD_16BITS] = { .num_channels = 4, .resolution = 16 }, + [HEIGHT_16BITS] = { .num_channels = 8, .resolution = 16 }, +}; + +/** + * struct ad7949_adc_chip - AD ADC chip + * @lock: protects write sequences + * @vref: regulator generating Vref + * @iio_dev: reference to iio structure + * @resolution: resolution of the chip + */ +struct ad7949_adc_chip { + struct mutex lock; + struct regulator *vref; + struct iio_dev *indio_dev; + struct spi_device *spi; + u8 resolution; + u16 cfg; + unsigned int current_channel; +}; + +static u16 ad7949_spi_read_cfg(struct ad7949_adc_chip *ad7949_adc) +{ + return ad7949_adc->cfg; +} + +static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc) +{ + u16 cfg = ad7949_spi_read_cfg(ad7949_adc) & AD7949_MASK_TOTAL; + bool is_cfg_read_back = cfg & AD7949_CFG_READ_BACK ? false : true; + int ret = ad7949_adc->resolution; + + if (is_cfg_read_back) + ret += AD7949_CFG_REG_SIZE_BITS; + + return ret; +} + +static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, + u16 mask) +{ + int ret; + u16 cfg = ad7949_spi_read_cfg(ad7949_adc) & AD7949_MASK_TOTAL; + int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc); + int shift = (bits_per_word - AD7949_CFG_REG_SIZE_BITS); + u32 buf_value = ((val & mask) | (cfg & ~mask)) << shift; + struct spi_message msg; + struct spi_transfer tx[] = { + { + .tx_buf = &buf_value, + .len = 4, + .bits_per_word = bits_per_word, + }, + }; + + ad7949_adc->cfg = buf_value >> shift; + spi_message_init(&msg); + spi_message_add_tail(&tx[0], &msg); + ret = spi_sync(ad7949_adc->spi, &msg); + udelay(2); + + return ret; +} + +static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, + unsigned int channel) +{ + int ret; + int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc); + int shift = (bits_per_word - AD7949_CFG_REG_SIZE_BITS); + u32 buf_value = 0; + struct spi_message msg; + struct spi_transfer tx[] = { + { + .rx_buf = &buf_value, + .len = 4, + .bits_per_word = bits_per_word, + }, + }; + + if (!val) + return -EINVAL; + + ad7949_spi_write_cfg(ad7949_adc, channel << AD7949_OFFSET_CHANNEL_SEL, + AD7949_MASK_CHANNEL_SEL); + spi_message_init(&msg); + spi_message_add_tail(&tx[0], &msg); + ret = spi_sync(ad7949_adc->spi, &msg); + udelay(2); + + ad7949_adc->current_channel = channel; + *val = (buf_value >> shift); + return ret; +} + +#define AD7949_ADC_CHANNEL(chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .scan_index = (chan), \ + .channel = (chan), \ + .address = (chan), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ +} + +static const struct iio_chan_spec ad7949_adc_channels[] = { + AD7949_ADC_CHANNEL(0), + AD7949_ADC_CHANNEL(1), + AD7949_ADC_CHANNEL(2), + AD7949_ADC_CHANNEL(3), + AD7949_ADC_CHANNEL(4), + AD7949_ADC_CHANNEL(5), + AD7949_ADC_CHANNEL(6), + AD7949_ADC_CHANNEL(7), +}; + +static int ad7949_spi_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + mutex_lock(&ad7949_adc->lock); + ret = ad7949_spi_read_channel(ad7949_adc, val, chan->channel); + mutex_unlock(&ad7949_adc->lock); + + if (ret < 0) + return ret; + + ret = IIO_VAL_INT; + break; + + case IIO_CHAN_INFO_SCALE: + ret = regulator_get_voltage(ad7949_adc->vref); + if (ret < 0) + return ret; + + *val = ret / 5000; + *val2 = 0; + ret = IIO_VAL_INT; + break; + + default: + ret = -EINVAL; + } + + return ret; +} + +static int ad7949_spi_reg_access(struct iio_dev *indio_dev, + unsigned int reg, unsigned int writeval, + unsigned int *readval) +{ + struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev); + int ret = 0; + + if (readval) + *readval = ad7949_spi_read_cfg(ad7949_adc); + else + ret = ad7949_spi_write_cfg(ad7949_adc, + writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL); + + return ret; +} + +static const struct iio_info ad7949_spi_info = { + .read_raw = ad7949_spi_read_raw, + .debugfs_reg_access = ad7949_spi_reg_access, +}; + +static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc) +{ + /* Sequencer disabled, CFG readback disabled, IN0 as default channel */ + ad7949_adc->current_channel = 0; + return ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL); +} + +static int ad7949_spi_probe(struct spi_device *spi) +{ + struct device *dev = &spi->dev; + const struct ad7949_adc_spec *spec; + struct ad7949_adc_chip *ad7949_adc; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc)); + if (!indio_dev) { + dev_err(dev, "can not allocate iio device\n"); + return -ENOMEM; + } + + spi->max_speed_hz = 33000000; + spi->mode = SPI_MODE_0; + spi->irq = -1; + spi->bits_per_word = 8; + spi_setup(spi); + + indio_dev->dev.parent = dev; + indio_dev->dev.of_node = spi->dev.of_node; + indio_dev->info = &ad7949_spi_info; + indio_dev->name = spi_get_device_id(spi)->name; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = ad7949_adc_channels; + spi_set_drvdata(spi, indio_dev); + + ad7949_adc = iio_priv(indio_dev); + ad7949_adc->indio_dev = indio_dev; + ad7949_adc->spi = spi; + + spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data]; + indio_dev->num_channels = spec->num_channels; + ad7949_adc->resolution = spec->resolution; + + ad7949_adc->vref = devm_regulator_get(dev, "vref"); + if (IS_ERR(ad7949_adc->vref)) { + dev_err(dev, "fail to request regulator\n"); + return PTR_ERR(ad7949_adc->vref); + } + + ret = regulator_enable(ad7949_adc->vref); + if (ret < 0) { + dev_err(dev, "fail to enable regulator\n"); + goto err_regulator; + } + + mutex_init(&ad7949_adc->lock); + + ret = iio_device_register(indio_dev); + if (ret) { + dev_err(dev, "fail to register iio device: %d\n", ret); + goto err; + } + + ret = ad7949_spi_init(ad7949_adc); + if (ret) { + dev_err(dev, "enable to init this device: %d\n", ret); + goto err; + } + + return 0; + +err: + mutex_destroy(&ad7949_adc->lock); +err_regulator: + regulator_disable(ad7949_adc->vref); + return ret; +} + +static int ad7949_spi_remove(struct spi_device *spi) +{ + struct iio_dev *indio_dev = spi_get_drvdata(spi); + struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev); + + iio_device_unregister(indio_dev); + mutex_destroy(&ad7949_adc->lock); + regulator_disable(ad7949_adc->vref); + + return 0; +} + +#ifdef CONFIG_OF +static const struct of_device_id ad7949_spi_of_id[] = { + { .compatible = "ad7949" }, + { .compatible = "ad7682" }, + { .compatible = "ad7689" }, + { } +}; +MODULE_DEVICE_TABLE(of, ad7949_spi_of_id); +#endif + +static const struct spi_device_id ad7949_spi_id[] = { + { "ad7949", HEIGHT_14BITS }, + { "ad7682", QUAD_16BITS }, + { "ad7689", HEIGHT_16BITS }, + { } +}; +MODULE_DEVICE_TABLE(spi, ad7949_spi_id); + +static struct spi_driver ad7949_spi_driver = { + .driver = { + .name = "ad7949", + .of_match_table = of_match_ptr(ad7949_spi_of_id), + }, + .probe = ad7949_spi_probe, + .remove = ad7949_spi_remove, + .id_table = ad7949_spi_id, +}; +module_spi_driver(ad7949_spi_driver); + +MODULE_AUTHOR("Charles-Antoine Couret <charles-antoine.couret@essensium.com>"); +MODULE_DESCRIPTION("Analog Devices 14/16-bit 8-channel ADC driver"); +MODULE_LICENSE("GPL v2"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] Add ad7949 device tree bindings in documentation 2018-10-06 20:30 [PATCH 1/2] Add AD7949 ADC driver family Charles-Antoine Couret @ 2018-10-06 20:30 ` Charles-Antoine Couret 2018-10-07 16:00 ` Jonathan Cameron 2018-10-07 16:25 ` [PATCH 1/2] Add AD7949 ADC driver family Jonathan Cameron 1 sibling, 1 reply; 8+ messages in thread From: Charles-Antoine Couret @ 2018-10-06 20:30 UTC (permalink / raw) To: linux-iio; +Cc: Charles-Antoine Couret Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> --- .../devicetree/bindings/iio/adc/ad7949.txt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/ad7949.txt diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt new file mode 100644 index 000000000000..4dc921961df7 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt @@ -0,0 +1,18 @@ +* Analog Devices AD7949/AD7682/AD7689 + +Required properties: + - compatible: Should be one of + * "ad7949" + * "ad7682" + * "ad7689" + - reg: spi chip select number for the device + - vref-supply: The regulator supply for ADC reference voltage + - spi-max-frequency: Max SPI frequency to use (< 33000000) + +Example: +adc@0 { + compatible = "ti,adc0832"; + reg = <0>; + vref-supply = <&vdd_supply>; + spi-max-frequency = <200000>; +}; -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Add ad7949 device tree bindings in documentation 2018-10-06 20:30 ` [PATCH 2/2] Add ad7949 device tree bindings in documentation Charles-Antoine Couret @ 2018-10-07 16:00 ` Jonathan Cameron 0 siblings, 0 replies; 8+ messages in thread From: Jonathan Cameron @ 2018-10-07 16:00 UTC (permalink / raw) To: Charles-Antoine Couret; +Cc: linux-iio, Rob Herring, Mark Rutland, devicetree On Sat, 6 Oct 2018 22:30:36 +0200 Charles-Antoine Couret <charles-antoine.couret@essensium.com> wrote: > Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> Device tree bindings should be cc'd to the devicetree maintainers and mailing list for review. As has been shown far too many times, I'm not very good and spotting the detail issues in these! So over to Rob and Mark's eagle eyes. Jonathan > --- > .../devicetree/bindings/iio/adc/ad7949.txt | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/ad7949.txt > > diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt > new file mode 100644 > index 000000000000..4dc921961df7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt > @@ -0,0 +1,18 @@ > +* Analog Devices AD7949/AD7682/AD7689 > + > +Required properties: > + - compatible: Should be one of > + * "ad7949" > + * "ad7682" > + * "ad7689" > + - reg: spi chip select number for the device > + - vref-supply: The regulator supply for ADC reference voltage > + - spi-max-frequency: Max SPI frequency to use (< 33000000) > + > +Example: > +adc@0 { > + compatible = "ti,adc0832"; > + reg = <0>; > + vref-supply = <&vdd_supply>; > + spi-max-frequency = <200000>; > +}; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Add AD7949 ADC driver family 2018-10-06 20:30 [PATCH 1/2] Add AD7949 ADC driver family Charles-Antoine Couret 2018-10-06 20:30 ` [PATCH 2/2] Add ad7949 device tree bindings in documentation Charles-Antoine Couret @ 2018-10-07 16:25 ` Jonathan Cameron 2018-10-08 21:18 ` Couret Charles-Antoine 1 sibling, 1 reply; 8+ messages in thread From: Jonathan Cameron @ 2018-10-07 16:25 UTC (permalink / raw) To: Charles-Antoine Couret; +Cc: linux-iio On Sat, 6 Oct 2018 22:30:35 +0200 Charles-Antoine Couret <charles-antoine.couret@essensium.com> wrote: > Compatible with AD7682 and AD7689 chips. > It is a Analog Devices ADC driver 14/16 bits 4/8 channels > with SPI protocol > > Datasheet of the device: > http://www.analog.com/media/en/technical-documentation/data-sheets/AD7949.pdf > > Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> Hi Charles-Antoine, Welcome to IIO. Always good to see a new submitter, particularly when they come with several new drivers. There are a few issues highlighted inline. Thanks and looking forward to v2, Jonathan > --- > drivers/iio/adc/Kconfig | 10 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ad7949.c | 329 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 340 insertions(+) > create mode 100644 drivers/iio/adc/ad7949.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 4a754921fb6f..42e66efff6c0 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -116,6 +116,16 @@ config AD7923 > To compile this driver as a module, choose M here: the > module will be called ad7923. > > +config AD7949 > + tristate "Analog Devices AD7949 and similar ADCs driver" > + depends on SPI > + help > + Say yes here to build support for Analog Devices > + AD7949, AD7682, AD7689 8 Channel ADCs. >From below, looks like one of them is 4 channel? > + > + To compile this driver as a module, choose M here: the > + module will be called ad7949. > + > config AD799X > tristate "Analog Devices AD799x ADC driver" > depends on I2C > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 03db7b578f9c..88804c867aa9 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7766) += ad7766.o > obj-$(CONFIG_AD7791) += ad7791.o > obj-$(CONFIG_AD7793) += ad7793.o > obj-$(CONFIG_AD7887) += ad7887.o > +obj-$(CONFIG_AD7949) += ad7949.o > obj-$(CONFIG_AD799X) += ad799x.o > obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o > obj-$(CONFIG_AT91_ADC) += at91_adc.o > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > new file mode 100644 > index 000000000000..667636b476f8 > --- /dev/null > +++ b/drivers/iio/adc/ad7949.c > @@ -0,0 +1,329 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* ad7949.c - Analog Devices ADC driver 14/16 bits 4/8 channels > + * > + * Copyright (C) 2018 CMC NV > + * > + * http://www.analog.com/media/en/technical-documentation/data-sheets/AD7949.pdf > + */ > + > +#include <linux/iio/iio.h> > +#include <linux/module.h> > +#include <linux/regulator/consumer.h> > +#include <linux/spi/spi.h> > +#include <linux/delay.h> Kernel style is to normally put headers in alphabetical order if the order isn't conveying some other information. > + > +#define AD7949_MASK_CFG 0x3C7F > +#define AD7949_MASK_CHANNEL_SEL 0x380 > +#define AD7949_MASK_TOTAL 0x3FFF GENMASK for masks please. > +#define AD7949_OFFSET_CHANNEL_SEL 7 > +#define AD7949_CFG_READ_BACK 0x1 > +#define AD7949_CFG_REG_SIZE_BITS 14 > + > +enum { > + HEIGHT_14BITS = 0, > + QUAD_16BITS, > + HEIGHT_16BITS, Height? I guess EIGHT was the intent. I would just use the part numbers for this rather than a descriptive phrase. > +}; > + > +struct ad7949_adc_spec { > + u8 num_channels; > + u8 resolution; > +}; > + > +static const struct ad7949_adc_spec ad7949_adc_spec[] = { > + [HEIGHT_14BITS] = { .num_channels = 8, .resolution = 14 }, > + [QUAD_16BITS] = { .num_channels = 4, .resolution = 16 }, > + [HEIGHT_16BITS] = { .num_channels = 8, .resolution = 16 }, > +}; > + > +/** > + * struct ad7949_adc_chip - AD ADC chip > + * @lock: protects write sequences > + * @vref: regulator generating Vref > + * @iio_dev: reference to iio structure > + * @resolution: resolution of the chip Good to see kernel-doc style commenting. Please make sure you document all elements and sanity check it by running the kernel-doc scripts over the file. > + */ > +struct ad7949_adc_chip { > + struct mutex lock; > + struct regulator *vref; > + struct iio_dev *indio_dev; > + struct spi_device *spi; > + u8 resolution; > + u16 cfg; > + unsigned int current_channel; > +}; > + > +static u16 ad7949_spi_read_cfg(struct ad7949_adc_chip *ad7949_adc) > +{ > + return ad7949_adc->cfg; This seems unnecessary abstraction. > +} > + > +static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc) > +{ > + u16 cfg = ad7949_spi_read_cfg(ad7949_adc) & AD7949_MASK_TOTAL; > + bool is_cfg_read_back = cfg & AD7949_CFG_READ_BACK ? false : true; Given this is only used in once place, more readable to just put it in that if statement and not have the ternary operator. > + int ret = ad7949_adc->resolution; > + > + if (is_cfg_read_back) > + ret += AD7949_CFG_REG_SIZE_BITS; > + > + return ret; > +} > + > +static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val, > + u16 mask) > +{ > + int ret; > + u16 cfg = ad7949_spi_read_cfg(ad7949_adc) & AD7949_MASK_TOTAL; > + int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc); > + int shift = (bits_per_word - AD7949_CFG_REG_SIZE_BITS); > + u32 buf_value = ((val & mask) | (cfg & ~mask)) << shift; Same problem with this buffer being passed to spi_sync as the one below. (I tend to review backwards as drivers make more sense starting from probe and review!) > + struct spi_message msg; > + struct spi_transfer tx[] = { > + { > + .tx_buf = &buf_value, > + .len = 4, > + .bits_per_word = bits_per_word, > + }, > + }; > + > + ad7949_adc->cfg = buf_value >> shift; > + spi_message_init(&msg); > + spi_message_add_tail(&tx[0], &msg); > + ret = spi_sync(ad7949_adc->spi, &msg); > + udelay(2); These delays need explaining as they are non obvious and there may be cleaner ways to handle them. > + > + return ret; > +} > + > +static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, > + unsigned int channel) > +{ > + int ret; > + int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc); > + int shift = (bits_per_word - AD7949_CFG_REG_SIZE_BITS); > + u32 buf_value = 0; Look very carefully at the requirements for a buffer being passed to spi_sync. It needs to be DMA safe. This one is not. The usual way to do that easily is to put a cacheline aligned buffer in your ad7949_adc_chip structure. Lots of examples to copy, but it's also worth making sure you understand why this is necessary. > + struct spi_message msg; > + struct spi_transfer tx[] = { > + { > + .rx_buf = &buf_value, > + .len = 4, > + .bits_per_word = bits_per_word, > + }, > + }; I 'think' this is the same in every call of this function, so why not set it up in the probe function and have it ready all the time rather than spinning up a new copy here each time. > + > + if (!val) > + return -EINVAL; > + > + ad7949_spi_write_cfg(ad7949_adc, channel << AD7949_OFFSET_CHANNEL_SEL, > + AD7949_MASK_CHANNEL_SEL); > + spi_message_init(&msg); > + spi_message_add_tail(&tx[0], &msg); > + ret = spi_sync(ad7949_adc->spi, &msg); Why? A delay after the spi sequence has occurred? > + udelay(2); > + > + ad7949_adc->current_channel = channel; > + *val = (buf_value >> shift); Brackets not adding anything so please remove. A blank line is always nice before a return statement like this. Makes the code slightly more predictable from a readability point of view. > + return ret; > +} > + > +#define AD7949_ADC_CHANNEL(chan) { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .scan_index = (chan), \ > + .channel = (chan), \ > + .address = (chan), \ Why set all 3 of these to the same thing? Scan index won't be used by the core unless you support buffered read modes some time in the future. Channel is used in the naming, but if we have the address as the same value, we can read channel just as well so I would just set that. > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > +} > + > +static const struct iio_chan_spec ad7949_adc_channels[] = { > + AD7949_ADC_CHANNEL(0), > + AD7949_ADC_CHANNEL(1), > + AD7949_ADC_CHANNEL(2), > + AD7949_ADC_CHANNEL(3), > + AD7949_ADC_CHANNEL(4), > + AD7949_ADC_CHANNEL(5), > + AD7949_ADC_CHANNEL(6), > + AD7949_ADC_CHANNEL(7), > +}; > + > +static int ad7949_spi_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + mutex_lock(&ad7949_adc->lock); > + ret = ad7949_spi_read_channel(ad7949_adc, val, chan->channel); > + mutex_unlock(&ad7949_adc->lock); > + > + if (ret < 0) > + return ret; > + > + ret = IIO_VAL_INT; There isn't anything to be done outside the switch statement (no locks held etc) so just use a direct return here. return IIO_VAL_INT; Same in all the other cases. > + break; > + > + case IIO_CHAN_INFO_SCALE: > + ret = regulator_get_voltage(ad7949_adc->vref); > + if (ret < 0) > + return ret; > + > + *val = ret / 5000; > + *val2 = 0; No need to set val2. The upper layers won't read it anyway. > + ret = IIO_VAL_INT; > + break; > + > + default: > + ret = -EINVAL; > + } > + > + return ret; > +} > + > +static int ad7949_spi_reg_access(struct iio_dev *indio_dev, > + unsigned int reg, unsigned int writeval, > + unsigned int *readval) > +{ > + struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev); > + int ret = 0; > + > + if (readval) > + *readval = ad7949_spi_read_cfg(ad7949_adc); > + else > + ret = ad7949_spi_write_cfg(ad7949_adc, > + writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL); > + > + return ret; > +} > + > +static const struct iio_info ad7949_spi_info = { > + .read_raw = ad7949_spi_read_raw, > + .debugfs_reg_access = ad7949_spi_reg_access, > +}; > + > +static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc) > +{ > + /* Sequencer disabled, CFG readback disabled, IN0 as default channel */ > + ad7949_adc->current_channel = 0; > + return ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL); > +} > + > +static int ad7949_spi_probe(struct spi_device *spi) > +{ > + struct device *dev = &spi->dev; > + const struct ad7949_adc_spec *spec; > + struct ad7949_adc_chip *ad7949_adc; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc)); > + if (!indio_dev) { > + dev_err(dev, "can not allocate iio device\n"); > + return -ENOMEM; > + } > + > + spi->max_speed_hz = 33000000; > + spi->mode = SPI_MODE_0; > + spi->irq = -1; That is rather surprising to see. What is the intent of setting this to -1? > + spi->bits_per_word = 8; > + spi_setup(spi); > + > + indio_dev->dev.parent = dev; > + indio_dev->dev.of_node = spi->dev.of_node; You have a local variable for spi->dev, might as well use it here. > + indio_dev->info = &ad7949_spi_info; > + indio_dev->name = spi_get_device_id(spi)->name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = ad7949_adc_channels; > + spi_set_drvdata(spi, indio_dev); > + > + ad7949_adc = iio_priv(indio_dev); > + ad7949_adc->indio_dev = indio_dev; > + ad7949_adc->spi = spi; > + > + spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data]; > + indio_dev->num_channels = spec->num_channels; > + ad7949_adc->resolution = spec->resolution; > + > + ad7949_adc->vref = devm_regulator_get(dev, "vref"); > + if (IS_ERR(ad7949_adc->vref)) { > + dev_err(dev, "fail to request regulator\n"); > + return PTR_ERR(ad7949_adc->vref); > + } > + > + ret = regulator_enable(ad7949_adc->vref); > + if (ret < 0) { > + dev_err(dev, "fail to enable regulator\n"); A rule of thumb in the kernel is that if a given function fails internally it should always exit as if it never happened at all. The result here is that you don't want to call regulator_disable if this function failed, only if a later function fails after this one succeeds. > + goto err_regulator; > + } > + > + mutex_init(&ad7949_adc->lock); > + > + ret = iio_device_register(indio_dev); > + if (ret) { > + dev_err(dev, "fail to register iio device: %d\n", ret); > + goto err; > + } > + > + ret = ad7949_spi_init(ad7949_adc); This looks immediately like a race condition to me. We should not have to do anything to the device after we have called iio_device_register. That function has enabled all userspace and in kernel interfaces so we can be talking to it before we get to this spi_init call. Also, note you you don't call iio_device_unregister in this error path. > + if (ret) { > + dev_err(dev, "enable to init this device: %d\n", ret); > + goto err; > + } > + > + return 0; > + > +err: > + mutex_destroy(&ad7949_adc->lock); > +err_regulator: > + regulator_disable(ad7949_adc->vref); > + return ret; > +} > + > +static int ad7949_spi_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev); > + > + iio_device_unregister(indio_dev); > + mutex_destroy(&ad7949_adc->lock); > + regulator_disable(ad7949_adc->vref); > + > + return 0; > +} > + > +#ifdef CONFIG_OF > +static const struct of_device_id ad7949_spi_of_id[] = { > + { .compatible = "ad7949" }, > + { .compatible = "ad7682" }, > + { .compatible = "ad7689" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ad7949_spi_of_id); > +#endif > + > +static const struct spi_device_id ad7949_spi_id[] = { > + { "ad7949", HEIGHT_14BITS }, > + { "ad7682", QUAD_16BITS }, > + { "ad7689", HEIGHT_16BITS }, > + { } > +}; > +MODULE_DEVICE_TABLE(spi, ad7949_spi_id); > + > +static struct spi_driver ad7949_spi_driver = { > + .driver = { > + .name = "ad7949", > + .of_match_table = of_match_ptr(ad7949_spi_of_id), > + }, > + .probe = ad7949_spi_probe, > + .remove = ad7949_spi_remove, > + .id_table = ad7949_spi_id, > +}; > +module_spi_driver(ad7949_spi_driver); > + > +MODULE_AUTHOR("Charles-Antoine Couret <charles-antoine.couret@essensium.com>"); > +MODULE_DESCRIPTION("Analog Devices 14/16-bit 8-channel ADC driver"); > +MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Add AD7949 ADC driver family 2018-10-07 16:25 ` [PATCH 1/2] Add AD7949 ADC driver family Jonathan Cameron @ 2018-10-08 21:18 ` Couret Charles-Antoine 2018-10-09 6:25 ` Ardelean, Alexandru 0 siblings, 1 reply; 8+ messages in thread From: Couret Charles-Antoine @ 2018-10-08 21:18 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-iio Le 07/10/2018 à 18:25, Jonathan Cameron a écrit : > Hi Charles-Antoine, Hello, Thank you for your code review, I'm trying to take everything into account but I have some questions about it before making the V2. >> +#define AD7949_OFFSET_CHANNEL_SEL 7 >> +#define AD7949_CFG_READ_BACK 0x1 >> +#define AD7949_CFG_REG_SIZE_BITS 14 >> + >> +enum { >> + HEIGHT_14BITS = 0, >> + QUAD_16BITS, >> + HEIGHT_16BITS, > Height? I guess EIGHT was the intent. > I would just use the part numbers for this rather than a > descriptive phrase. Thank you for the typo. But I don't understand your remark. What do you mean by "part numbers" here? >> + struct spi_message msg; >> + struct spi_transfer tx[] = { >> + { >> + .tx_buf = &buf_value, >> + .len = 4, >> + .bits_per_word = bits_per_word, >> + }, >> + }; >> + >> + ad7949_adc->cfg = buf_value >> shift; >> + spi_message_init(&msg); >> + spi_message_add_tail(&tx[0], &msg); >> + ret = spi_sync(ad7949_adc->spi, &msg); >> + udelay(2); > These delays need explaining as they are non obvious and there > may be cleaner ways to handle them. I want to add this comment: /* This delay is to avoid a new request before the required time to * send a new command to the device */ It is clear and relevant enough? > >> + struct spi_message msg; >> + struct spi_transfer tx[] = { >> + { >> + .rx_buf = &buf_value, >> + .len = 4, >> + .bits_per_word = bits_per_word, >> + }, >> + }; > I 'think' this is the same in every call of this function, so why not > set it up in the probe function and have it ready all the time rather than > spinning up a new copy here each time. bits_per_word depends on if the configuration readback is enabled or not. It could be changed in runtime. So I considered we can not optimize in that way. Regards, Charles-Antoine Couret ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Add AD7949 ADC driver family 2018-10-08 21:18 ` Couret Charles-Antoine @ 2018-10-09 6:25 ` Ardelean, Alexandru 2018-10-09 7:12 ` Couret Charles-Antoine 0 siblings, 1 reply; 8+ messages in thread From: Ardelean, Alexandru @ 2018-10-09 6:25 UTC (permalink / raw) To: charles-antoine.couret, jic23; +Cc: linux-iio T24gTW9uLCAyMDE4LTEwLTA4IGF0IDIzOjE4ICswMjAwLCBDb3VyZXQgQ2hhcmxlcy1BbnRvaW5l IHdyb3RlOg0KPiBMZSAwNy8xMC8yMDE4IMOgIDE4OjI1LCBKb25hdGhhbiBDYW1lcm9uIGEgw6lj cml0IDoNCj4gPiBIaSBDaGFybGVzLUFudG9pbmUsIA0KPiANCj4gSGVsbG8sDQo+IA0KPiBUaGFu ayB5b3UgZm9yIHlvdXIgY29kZSByZXZpZXcsIEknbSB0cnlpbmcgdG8gdGFrZSBldmVyeXRoaW5n IGludG8gDQo+IGFjY291bnQgYnV0IEkgaGF2ZSBzb21lIHF1ZXN0aW9ucyBhYm91dCBpdCBiZWZv cmUgbWFraW5nIHRoZSBWMi4NCj4gDQo+ID4gPiArI2RlZmluZSBBRDc5NDlfT0ZGU0VUX0NIQU5O RUxfU0VMCTcNCj4gPiA+ICsjZGVmaW5lIEFENzk0OV9DRkdfUkVBRF9CQUNLCQkweDENCj4gPiA+ ICsjZGVmaW5lIEFENzk0OV9DRkdfUkVHX1NJWkVfQklUUwkxNA0KPiA+ID4gKw0KPiA+ID4gK2Vu dW0gew0KPiA+ID4gKwlIRUlHSFRfMTRCSVRTID0gMCwNCj4gPiA+ICsJUVVBRF8xNkJJVFMsDQo+ ID4gPiArCUhFSUdIVF8xNkJJVFMsDQo+ID4gDQo+ID4gSGVpZ2h0PyBJIGd1ZXNzIEVJR0hUIHdh cyB0aGUgaW50ZW50Lg0KPiA+IEkgd291bGQganVzdCB1c2UgdGhlIHBhcnQgbnVtYmVycyBmb3Ig dGhpcyByYXRoZXIgdGhhbiBhDQo+ID4gZGVzY3JpcHRpdmUgcGhyYXNlLg0KPiANCj4gVGhhbmsg eW91IGZvciB0aGUgdHlwby4NCj4gDQo+IEJ1dCBJIGRvbid0IHVuZGVyc3RhbmQgeW91ciByZW1h cmsuIFdoYXQgZG8geW91IG1lYW4gYnkgInBhcnQgbnVtYmVycyIgDQo+IGhlcmU/DQoNCkEgbG90 IG9mIGRyaXZlcnMgZGVmaW5lIHNvbWV0aGluZyBsaWtlOg0KZW51bSB7DQogICBJRF9BRDc5NDks DQogICBJRF9BRDc2ODIsDQogICBJRF9BRDc2ODksDQp9DQp3aGljaCBjYW4gYmUgcmVmZXJlZCB0 byBhcyAicGFydCBudW1iZXIiLCBhbmQgdGhlbiB5b3UgY2FuIHVzZSB0aGVzZSBlbnVtDQp2YWx1 ZXMgdG8gaWRlbnRpZnkgYmVoYXZpb3IgYW5kIGNvbmZpZ3VyYXRpb24gZm9yIGVhY2ggZGV2aWNl IHRoZSBkcml2ZXINCnN1cHBvcnRzLg0KDQpUaGlzIG1ldGhvZCBpcyBwcmVmZXJyZWQsIGJlY2F1 c2Ugd2hlbi9pZiBhIG5ldyBjaGlwIGNvbWVzIGFsb25nIHRoYXQgZml0cw0KaW50byB0aGlzIGRy aXZlciAobGV0J3Mgc2F5IElEX0FEWFhZWiksIGFuZCBtYXkgaGF2ZSBRVUFEXzE2QklUUyBhbmQN CmRpZmZlcnMgaW4gc29tZSBvdGhlciBtaW5vciBhc3BlY3QsIGl0IGNhbiBiZSBlYXNpZXIgdG8g aWRlbnRpZnkgdmlhIHRoZQ0KcGFydC1udW1iZXIuIE9yLCBpbiBzb21lIGNhc2VzLCBzb21lIGNo aXBzIGdldCBhIG5ld2VyIHJldmlzaW9uIChleGFtcGxlOg0KSURfQUQ3OTQ5QikgdGhhdCBtYXkg ZGlmZmVyIHNsaWdodGx5IChmcm9tIElEX0FENzk0OSkuDQoNCg0KPiANCj4gPiA+ICsJc3RydWN0 IHNwaV9tZXNzYWdlIG1zZzsNCj4gPiA+ICsJc3RydWN0IHNwaV90cmFuc2ZlciB0eFtdID0gew0K PiA+ID4gKwkJew0KPiA+ID4gKwkJCS50eF9idWYgPSAmYnVmX3ZhbHVlLA0KPiA+ID4gKwkJCS5s ZW4gPSA0LA0KPiA+ID4gKwkJCS5iaXRzX3Blcl93b3JkID0gYml0c19wZXJfd29yZCwNCj4gPiA+ ICsJCX0sDQo+ID4gPiArCX07DQo+ID4gPiArDQo+ID4gPiArCWFkNzk0OV9hZGMtPmNmZyA9IGJ1 Zl92YWx1ZSA+PiBzaGlmdDsNCj4gPiA+ICsJc3BpX21lc3NhZ2VfaW5pdCgmbXNnKTsNCj4gPiA+ ICsJc3BpX21lc3NhZ2VfYWRkX3RhaWwoJnR4WzBdLCAmbXNnKTsNCj4gPiA+ICsJcmV0ID0gc3Bp X3N5bmMoYWQ3OTQ5X2FkYy0+c3BpLCAmbXNnKTsNCj4gPiA+ICsJdWRlbGF5KDIpOw0KPiA+IA0K PiA+IFRoZXNlIGRlbGF5cyBuZWVkIGV4cGxhaW5pbmcgYXMgdGhleSBhcmUgbm9uIG9idmlvdXMg YW5kIHRoZXJlDQo+ID4gbWF5IGJlIGNsZWFuZXIgd2F5cyB0byBoYW5kbGUgdGhlbS4NCj4gDQo+ IEkgd2FudCB0byBhZGQgdGhpcyBjb21tZW50Og0KPiANCj4gICAgICAvKiBUaGlzIGRlbGF5IGlz IHRvIGF2b2lkIGEgbmV3IHJlcXVlc3QgYmVmb3JlIHRoZSByZXF1aXJlZCB0aW1lIHRvDQo+ICAg ICAgICogc2VuZCBhIG5ldyBjb21tYW5kIHRvIHRoZSBkZXZpY2UNCj4gICAgICAgKi8NCj4gDQo+ IEl0IGlzIGNsZWFyIGFuZCByZWxldmFudCBlbm91Z2g/DQoNCkkgdGhpbmsgaW4gc3VjaCBhIGNh c2UsIGEgbG9jay9tdXRleCB3b3VsZCBiZSBuZWVkZWQuDQpBcyBmYXIgYXMgSSByZW1lbWJlciwg a2VybmVsIFNQSSBjYWxscyBzaG91bGQgaGF2ZSB0aGVpciBvd24gbG9ja3MgZm9yDQpzaW5nbGUg U1BJIHRyYW5zYWN0aW9ucywgc28gbWF5YmUgc29tZSBsb2NrcyBmb3IgYWNjZXNzaW5nIHRoZSBj aGlwIGR1cmluZw0KYSBzZXQgb2YgU1BJIHRyYW5zYWN0aW9ucyB3b3VsZCBiZSBuZWF0ZXIuDQoN Cj4gDQo+ID4gDQo+ID4gPiArCXN0cnVjdCBzcGlfbWVzc2FnZSBtc2c7DQo+ID4gPiArCXN0cnVj dCBzcGlfdHJhbnNmZXIgdHhbXSA9IHsNCj4gPiA+ICsJCXsNCj4gPiA+ICsJCQkucnhfYnVmID0g JmJ1Zl92YWx1ZSwNCj4gPiA+ICsJCQkubGVuID0gNCwNCj4gPiA+ICsJCQkuYml0c19wZXJfd29y ZCA9IGJpdHNfcGVyX3dvcmQsDQo+ID4gPiArCQl9LA0KPiA+ID4gKwl9Ow0KPiA+IA0KPiA+IEkg J3RoaW5rJyB0aGlzIGlzIHRoZSBzYW1lIGluIGV2ZXJ5IGNhbGwgb2YgdGhpcyBmdW5jdGlvbiwg c28gd2h5IG5vdA0KPiA+IHNldCBpdCB1cCBpbiB0aGUgcHJvYmUgZnVuY3Rpb24gYW5kIGhhdmUg aXQgcmVhZHkgYWxsIHRoZSB0aW1lIHJhdGhlcg0KPiA+IHRoYW4NCj4gPiBzcGlubmluZyB1cCBh IG5ldyBjb3B5IGhlcmUgZWFjaCB0aW1lLg0KPiANCj4gYml0c19wZXJfd29yZCBkZXBlbmRzIG9u IGlmIHRoZSBjb25maWd1cmF0aW9uIHJlYWRiYWNrIGlzIGVuYWJsZWQgb3IgDQo+IG5vdC4gSXQg Y291bGQgYmUgY2hhbmdlZCBpbiBydW50aW1lLiBTbyBJIGNvbnNpZGVyZWQgd2UgY2FuIG5vdCBv cHRpbWl6ZSANCj4gaW4gdGhhdCB3YXkuDQo+IA0KDQpBbGV4DQoNCj4gUmVnYXJkcywNCj4gQ2hh cmxlcy1BbnRvaW5lIENvdXJldA0KPiANCg== ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Add AD7949 ADC driver family 2018-10-09 6:25 ` Ardelean, Alexandru @ 2018-10-09 7:12 ` Couret Charles-Antoine 2018-10-13 12:25 ` Jonathan Cameron 0 siblings, 1 reply; 8+ messages in thread From: Couret Charles-Antoine @ 2018-10-09 7:12 UTC (permalink / raw) To: Ardelean, Alexandru, jic23; +Cc: linux-iio Le 09/10/2018 à 08:25, Ardelean, Alexandru a écrit : > >>>> +#define AD7949_OFFSET_CHANNEL_SEL 7 >>>> +#define AD7949_CFG_READ_BACK 0x1 >>>> +#define AD7949_CFG_REG_SIZE_BITS 14 >>>> + >>>> +enum { >>>> + HEIGHT_14BITS = 0, >>>> + QUAD_16BITS, >>>> + HEIGHT_16BITS, >>> Height? I guess EIGHT was the intent. >>> I would just use the part numbers for this rather than a >>> descriptive phrase. >> Thank you for the typo. >> >> But I don't understand your remark. What do you mean by "part numbers" >> here? > A lot of drivers define something like: > enum { > ID_AD7949, > ID_AD7682, > ID_AD7689, > } > which can be refered to as "part number", and then you can use these enum > values to identify behavior and configuration for each device the driver > supports. > > This method is preferred, because when/if a new chip comes along that fits > into this driver (let's say ID_ADXXYZ), and may have QUAD_16BITS and > differs in some other minor aspect, it can be easier to identify via the > part-number. Or, in some cases, some chips get a newer revision (example: > ID_AD7949B) that may differ slightly (from ID_AD7949). Ok, I understand, thank you for the explanation. >>>> + struct spi_message msg; >>>> + struct spi_transfer tx[] = { >>>> + { >>>> + .tx_buf = &buf_value, >>>> + .len = 4, >>>> + .bits_per_word = bits_per_word, >>>> + }, >>>> + }; >>>> + >>>> + ad7949_adc->cfg = buf_value >> shift; >>>> + spi_message_init(&msg); >>>> + spi_message_add_tail(&tx[0], &msg); >>>> + ret = spi_sync(ad7949_adc->spi, &msg); >>>> + udelay(2); >>> These delays need explaining as they are non obvious and there >>> may be cleaner ways to handle them. >> I want to add this comment: >> >> /* This delay is to avoid a new request before the required time to >> * send a new command to the device >> */ >> >> It is clear and relevant enough? > I think in such a case, a lock/mutex would be needed. > As far as I remember, kernel SPI calls should have their own locks for > single SPI transactions, so maybe some locks for accessing the chip during > a set of SPI transactions would be neater. The mutex is used in parent level (the functions which make the link between userspace and this function). It seems enough for me. In that case the purpose of the delay is only to avoid a new request just after this one which will fail because too early for the device. It is just a timing protection, it is not uncommon from my point of view. Regards, Charles-Antoine Couret ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Add AD7949 ADC driver family 2018-10-09 7:12 ` Couret Charles-Antoine @ 2018-10-13 12:25 ` Jonathan Cameron 0 siblings, 0 replies; 8+ messages in thread From: Jonathan Cameron @ 2018-10-13 12:25 UTC (permalink / raw) To: Couret Charles-Antoine; +Cc: Ardelean, Alexandru, linux-iio On Tue, 9 Oct 2018 09:12:35 +0200 Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote: > Le 09/10/2018 à 08:25, Ardelean, Alexandru a écrit : > > > >>>> +#define AD7949_OFFSET_CHANNEL_SEL 7 > >>>> +#define AD7949_CFG_READ_BACK 0x1 > >>>> +#define AD7949_CFG_REG_SIZE_BITS 14 > >>>> + > >>>> +enum { > >>>> + HEIGHT_14BITS = 0, > >>>> + QUAD_16BITS, > >>>> + HEIGHT_16BITS, > >>> Height? I guess EIGHT was the intent. > >>> I would just use the part numbers for this rather than a > >>> descriptive phrase. > >> Thank you for the typo. > >> > >> But I don't understand your remark. What do you mean by "part numbers" > >> here? > > A lot of drivers define something like: > > enum { > > ID_AD7949, > > ID_AD7682, > > ID_AD7689, > > } > > which can be refered to as "part number", and then you can use these enum > > values to identify behavior and configuration for each device the driver > > supports. > > > > This method is preferred, because when/if a new chip comes along that fits > > into this driver (let's say ID_ADXXYZ), and may have QUAD_16BITS and > > differs in some other minor aspect, it can be easier to identify via the > > part-number. Or, in some cases, some chips get a newer revision (example: > > ID_AD7949B) that may differ slightly (from ID_AD7949). > Ok, I understand, thank you for the explanation. > >>>> + struct spi_message msg; > >>>> + struct spi_transfer tx[] = { > >>>> + { > >>>> + .tx_buf = &buf_value, > >>>> + .len = 4, > >>>> + .bits_per_word = bits_per_word, > >>>> + }, > >>>> + }; > >>>> + > >>>> + ad7949_adc->cfg = buf_value >> shift; > >>>> + spi_message_init(&msg); > >>>> + spi_message_add_tail(&tx[0], &msg); > >>>> + ret = spi_sync(ad7949_adc->spi, &msg); > >>>> + udelay(2); > >>> These delays need explaining as they are non obvious and there > >>> may be cleaner ways to handle them. > >> I want to add this comment: > >> > >> /* This delay is to avoid a new request before the required time to > >> * send a new command to the device > >> */ > >> > >> It is clear and relevant enough? > > I think in such a case, a lock/mutex would be needed. > > As far as I remember, kernel SPI calls should have their own locks for > > single SPI transactions, so maybe some locks for accessing the chip during > > a set of SPI transactions would be neater. > > The mutex is used in parent level (the functions which make the link > between userspace and this function). It seems enough for me. > > In that case the purpose of the delay is only to avoid a new request > just after this one which will fail because too early for the device. It > is just a timing protection, it is not uncommon from my point of view. This is fine (with the comment). There has always been a comment in spi.h suggesting that we could potentially move such timing constraints into the protocol handling rather than individual drivers. It is a very short delay so it is probably not a problem to insert it before reporting the requested value. If it had been longer we would have wanted to store a timestamp here and only force a sleep on the following command if necessary, rather than always inserting a delay here. Thanks, Jonathan > > > Regards, > > Charles-Antoine Couret > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-13 20:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-06 20:30 [PATCH 1/2] Add AD7949 ADC driver family Charles-Antoine Couret 2018-10-06 20:30 ` [PATCH 2/2] Add ad7949 device tree bindings in documentation Charles-Antoine Couret 2018-10-07 16:00 ` Jonathan Cameron 2018-10-07 16:25 ` [PATCH 1/2] Add AD7949 ADC driver family Jonathan Cameron 2018-10-08 21:18 ` Couret Charles-Antoine 2018-10-09 6:25 ` Ardelean, Alexandru 2018-10-09 7:12 ` Couret Charles-Antoine 2018-10-13 12:25 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.