| Message ID | 20240807-iio-adc-ad4695-buffered-read-v1-1-bdafc39b2283@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad4695: implement triggered buffer | expand |
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 7cad163c39cb642ed587d3eeb37a5637ee02740f]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-adc-ad4695-implement-triggered-buffer/20240808-063333
base: 7cad163c39cb642ed587d3eeb37a5637ee02740f
patch link: https://lore.kernel.org/r/20240807-iio-adc-ad4695-buffered-read-v1-1-bdafc39b2283%40baylibre.com
patch subject: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240808/202408081623.ua9EBfoZ-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 408d82d352eb98e2d0a804c66d359cd7a49228fe)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081623.ua9EBfoZ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408081623.ua9EBfoZ-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/iio/adc/ad4695.c:24:
In file included from include/linux/iio/triggered_buffer.h:6:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/iio/adc/ad4695.c:24:
In file included from include/linux/iio/triggered_buffer.h:6:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/iio/adc/ad4695.c:24:
In file included from include/linux/iio/triggered_buffer.h:6:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from drivers/iio/adc/ad4695.c:28:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2228:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/adc/ad4695.c:454:8: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (mask)))' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
454 | val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:115:3: note: expanded from macro 'FIELD_PREP'
115 | __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:72:53: note: expanded from macro '__BF_FIELD_CHECK'
72 | BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
73 | __bf_cast_unsigned(_reg, ~0ull), \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
74 | _pfx "type of reg too small for mask"); \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert'
510 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert'
498 | __compiletime_assert(condition, msg, prefix, suffix)
| ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert'
490 | if (!(condition)) \
| ^~~~~~~~~
8 warnings generated.
vim +454 drivers/iio/adc/ad4695.c
373
374 static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
375 {
376 struct ad4695_state *st = iio_priv(indio_dev);
377 struct spi_transfer *xfer;
378 u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
379 bool temp_chan_en = false;
380 u32 reg, mask, val, bit, num_xfer, num_slots;
381 int ret;
382
383 /*
384 * We are using the advanced sequencer since it is the only way to read
385 * multiple channels that allows individual configuration of each
386 * voltage input channel. Slot 0 in the advanced sequencer is used to
387 * account for the gap between trigger polls - we don't read data from
388 * this slot. Each enabled voltage channel is assigned a slot starting
389 * with slot 1.
390 */
391 num_slots = 1;
392
393 memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
394
395 /* First xfer is only to trigger conversion of slot 1, so no rx. */
396 xfer = &st->buf_read_xfer[0];
397 xfer->cs_change = 1;
398 xfer->delay.value = AD4695_T_CNVL_NS;
399 xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
400 xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
401 xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
402 num_xfer = 1;
403
404 iio_for_each_active_channel(indio_dev, bit) {
405 xfer = &st->buf_read_xfer[num_xfer];
406 xfer->bits_per_word = 16;
407 xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
408 xfer->len = 2;
409 xfer->cs_change = 1;
410 xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
411 xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
412
413 if (bit == temp_chan_bit) {
414 temp_chan_en = true;
415 } else {
416 reg = AD4695_REG_AS_SLOT(num_slots);
417 val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
418
419 ret = regmap_write(st->regmap, reg, val);
420 if (ret)
421 return ret;
422
423 num_slots++;
424 }
425
426 num_xfer++;
427 }
428
429 /*
430 * Don't keep CS asserted after last xfer. Also triggers conversion of
431 * slot 0.
432 */
433 xfer->cs_change = 0;
434
435 /**
436 * The advanced sequencer requires that at least 2 slots are enabled.
437 * Since slot 0 is always used for other purposes, we need only 1
438 * enabled voltage channel to meet this requirement. This error will
439 * only happen if only the temperature channel is enabled.
440 */
441 if (num_slots < 2) {
442 dev_err_ratelimited(&indio_dev->dev,
443 "Buffered read requires at least 1 voltage channel enabled\n");
444 return -EINVAL;
445 }
446
447 /*
448 * Temperature channel isn't included in the sequence, but rather
449 * controlled by setting a bit in the TEMP_CTRL register.
450 */
451
452 reg = AD4695_REG_TEMP_CTRL;
453 mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
> 454 val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
455
456 ret = regmap_update_bits(st->regmap, reg, mask, val);
457 if (ret)
458 return ret;
459
460 spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
461 num_xfer);
462
463 ret = spi_optimize_message(st->spi, &st->buf_read_msg);
464 if (ret)
465 return ret;
466
467 /* This triggers conversion of slot 0. */
468 ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
469 if (ret) {
470 spi_unoptimize_message(&st->buf_read_msg);
471 return ret;
472 }
473
474 return 0;
475 }
476
On Wed, 2024-08-07 at 15:02 -0500, David Lechner wrote: > This implements buffered reads for the ad4695 driver using the typical > triggered buffer implementation, including adding a soft timestamp > channel. > > The chip has 4 different modes for doing conversions. The driver is > using the advanced sequencer mode since that is the only mode that > allows individual configuration of all aspects each channel (e.g. > bipolar config currently and oversampling to be added in the future). > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- Hi David, Just two nit comments... Reviewed-by: Nuno Sa <nuno.sa@analog.com> > drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 230 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c > index 007ecb951bc3..a3bd5be36134 100644 > --- a/drivers/iio/adc/ad4695.c > +++ b/drivers/iio/adc/ad4695.c ... > > > +static int ad4695_buffer_preenable(struct iio_dev *indio_dev) > +{ > + struct ad4695_state *st = iio_priv(indio_dev); > + struct spi_transfer *xfer; > + u8 temp_chan_bit = st->chip_info->num_voltage_inputs; > + bool temp_chan_en = false; > + u32 reg, mask, val, bit, num_xfer, num_slots; > + int ret; > + > + /* > + * We are using the advanced sequencer since it is the only way to read > + * multiple channels that allows individual configuration of each > + * voltage input channel. Slot 0 in the advanced sequencer is used to > + * account for the gap between trigger polls - we don't read data from > + * this slot. Each enabled voltage channel is assigned a slot starting > + * with slot 1. > + */ > + num_slots = 1; > + > + memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer)); > + > + /* First xfer is only to trigger conversion of slot 1, so no rx. */ > + xfer = &st->buf_read_xfer[0]; > + xfer->cs_change = 1; > + xfer->delay.value = AD4695_T_CNVL_NS; > + xfer->delay.unit = SPI_DELAY_UNIT_NSECS; > + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; > + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; > + num_xfer = 1; > + > + iio_for_each_active_channel(indio_dev, bit) { > + xfer = &st->buf_read_xfer[num_xfer]; > + xfer->bits_per_word = 16; > + xfer->rx_buf = &st->buf[(num_xfer - 1) * 2]; > + xfer->len = 2; > + xfer->cs_change = 1; > + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; > + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; > + > + if (bit == temp_chan_bit) { > + temp_chan_en = true; > + } else { > + reg = AD4695_REG_AS_SLOT(num_slots); > + val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit); > + > + ret = regmap_write(st->regmap, reg, val); > + if (ret) > + return ret; > + > + num_slots++; > + } > + > + num_xfer++; > + } > + > + /* > + * Don't keep CS asserted after last xfer. Also triggers conversion of > + * slot 0. > + */ > + xfer->cs_change = 0; > + > + /** > + * The advanced sequencer requires that at least 2 slots are enabled. > + * Since slot 0 is always used for other purposes, we need only 1 > + * enabled voltage channel to meet this requirement. This error will > + * only happen if only the temperature channel is enabled. > + */ > + if (num_slots < 2) { > + dev_err_ratelimited(&indio_dev->dev, > + "Buffered read requires at least 1 voltage channel > enabled\n"); This one is intriguing... Why the ratelimited variant? Normally you'd use that in IRQ routines where the log could be flooded. > + return -EINVAL; > + } > + > + /* > + * Temperature channel isn't included in the sequence, but rather > + * controlled by setting a bit in the TEMP_CTRL register. > + */ > + > + reg = AD4695_REG_TEMP_CTRL; > + mask = AD4695_REG_TEMP_CTRL_TEMP_EN; > + val = FIELD_PREP(mask, temp_chan_en ? 1 : 0); > + > + ret = regmap_update_bits(st->regmap, reg, mask, val); > + if (ret) > + return ret; > + > + spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer, > + num_xfer); > + > + ret = spi_optimize_message(st->spi, &st->buf_read_msg); > + if (ret) > + return ret; > + > + /* This triggers conversion of slot 0. */ > + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); > + if (ret) { > + spi_unoptimize_message(&st->buf_read_msg); > + return ret; > + } Could save one line with (unless ad4695_enter_advanced_sequencer_mode() does not return 0 on success) ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); if (ret) spi_unoptimize_message(&st->buf_read_msg); return ret; - Nuno Sá
On 8/9/24 9:24 AM, Nuno Sá wrote: > On Wed, 2024-08-07 at 15:02 -0500, David Lechner wrote: >> This implements buffered reads for the ad4695 driver using the typical >> triggered buffer implementation, including adding a soft timestamp >> channel. >> >> The chip has 4 different modes for doing conversions. The driver is >> using the advanced sequencer mode since that is the only mode that >> allows individual configuration of all aspects each channel (e.g. >> bipolar config currently and oversampling to be added in the future). >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- > > Hi David, > > Just two nit comments... > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > >> drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 230 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c >> index 007ecb951bc3..a3bd5be36134 100644 >> --- a/drivers/iio/adc/ad4695.c >> +++ b/drivers/iio/adc/ad4695.c > > ... > >> >> >> +static int ad4695_buffer_preenable(struct iio_dev *indio_dev) >> +{ >> + struct ad4695_state *st = iio_priv(indio_dev); >> + struct spi_transfer *xfer; >> + u8 temp_chan_bit = st->chip_info->num_voltage_inputs; >> + bool temp_chan_en = false; >> + u32 reg, mask, val, bit, num_xfer, num_slots; >> + int ret; >> + >> + /* >> + * We are using the advanced sequencer since it is the only way to read >> + * multiple channels that allows individual configuration of each >> + * voltage input channel. Slot 0 in the advanced sequencer is used to >> + * account for the gap between trigger polls - we don't read data from >> + * this slot. Each enabled voltage channel is assigned a slot starting >> + * with slot 1. >> + */ >> + num_slots = 1; >> + >> + memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer)); >> + >> + /* First xfer is only to trigger conversion of slot 1, so no rx. */ >> + xfer = &st->buf_read_xfer[0]; >> + xfer->cs_change = 1; >> + xfer->delay.value = AD4695_T_CNVL_NS; >> + xfer->delay.unit = SPI_DELAY_UNIT_NSECS; >> + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; >> + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; >> + num_xfer = 1; >> + >> + iio_for_each_active_channel(indio_dev, bit) { >> + xfer = &st->buf_read_xfer[num_xfer]; >> + xfer->bits_per_word = 16; >> + xfer->rx_buf = &st->buf[(num_xfer - 1) * 2]; >> + xfer->len = 2; >> + xfer->cs_change = 1; >> + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; >> + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; >> + >> + if (bit == temp_chan_bit) { >> + temp_chan_en = true; >> + } else { >> + reg = AD4695_REG_AS_SLOT(num_slots); >> + val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit); >> + >> + ret = regmap_write(st->regmap, reg, val); >> + if (ret) >> + return ret; >> + >> + num_slots++; >> + } >> + >> + num_xfer++; >> + } >> + >> + /* >> + * Don't keep CS asserted after last xfer. Also triggers conversion of >> + * slot 0. >> + */ >> + xfer->cs_change = 0; >> + >> + /** >> + * The advanced sequencer requires that at least 2 slots are enabled. >> + * Since slot 0 is always used for other purposes, we need only 1 >> + * enabled voltage channel to meet this requirement. This error will >> + * only happen if only the temperature channel is enabled. >> + */ >> + if (num_slots < 2) { >> + dev_err_ratelimited(&indio_dev->dev, >> + "Buffered read requires at least 1 voltage channel >> enabled\n"); > > This one is intriguing... Why the ratelimited variant? Normally you'd use that in IRQ > routines where the log could be flooded. IIO Oscilloscope does a lot of retries of buffered reads very quickly, so was getting a minor flood (10-20 repeats). I'm not sure that ratelimited actually helped in this case though. I suppose we could just drop this and expect people to read the docs if they get an EINVAL when attempting to enable the buffer. Or just make it dev_err() since it isn't 100s of repeats. >> + return -EINVAL; >> + } >> + >> + /* >> + * Temperature channel isn't included in the sequence, but rather >> + * controlled by setting a bit in the TEMP_CTRL register. >> + */ >> + >> + reg = AD4695_REG_TEMP_CTRL; >> + mask = AD4695_REG_TEMP_CTRL_TEMP_EN; >> + val = FIELD_PREP(mask, temp_chan_en ? 1 : 0); >> + >> + ret = regmap_update_bits(st->regmap, reg, mask, val); >> + if (ret) >> + return ret; >> + >> + spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer, >> + num_xfer); >> + >> + ret = spi_optimize_message(st->spi, &st->buf_read_msg); >> + if (ret) >> + return ret; >> + >> + /* This triggers conversion of slot 0. */ >> + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); >> + if (ret) { >> + spi_unoptimize_message(&st->buf_read_msg); >> + return ret; >> + } > > Could save one line with (unless ad4695_enter_advanced_sequencer_mode() does not > return 0 on success) sure > > ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); > if (ret) > spi_unoptimize_message(&st->buf_read_msg); > > return ret; > > - Nuno Sá >
On Thu, 8 Aug 2024 16:52:09 +0800 kernel test robot <lkp@intel.com> wrote: > Hi David, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on 7cad163c39cb642ed587d3eeb37a5637ee02740f] > > url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-adc-ad4695-implement-triggered-buffer/20240808-063333 > base: 7cad163c39cb642ed587d3eeb37a5637ee02740f > patch link: https://lore.kernel.org/r/20240807-iio-adc-ad4695-buffered-read-v1-1-bdafc39b2283%40baylibre.com > patch subject: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer > config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240808/202408081623.ua9EBfoZ-lkp@intel.com/config) > compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 408d82d352eb98e2d0a804c66d359cd7a49228fe) > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081623.ua9EBfoZ-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202408081623.ua9EBfoZ-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > > In file included from drivers/iio/adc/ad4695.c:24: > In file included from include/linux/iio/triggered_buffer.h:6: > In file included from include/linux/interrupt.h:11: > In file included from include/linux/hardirq.h:11: > In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: > In file included from include/asm-generic/hardirq.h:17: > In file included from include/linux/irq.h:20: > In file included from include/linux/io.h:14: > In file included from arch/hexagon/include/asm/io.h:328: > include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] > 548 | val = __raw_readb(PCI_IOBASE + addr); > | ~~~~~~~~~~ ^ > include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] > 561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); > | ~~~~~~~~~~ ^ > include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' > 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) > | ^ > In file included from drivers/iio/adc/ad4695.c:24: > In file included from include/linux/iio/triggered_buffer.h:6: > In file included from include/linux/interrupt.h:11: > In file included from include/linux/hardirq.h:11: > In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: > In file included from include/asm-generic/hardirq.h:17: > In file included from include/linux/irq.h:20: > In file included from include/linux/io.h:14: > In file included from arch/hexagon/include/asm/io.h:328: > include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] > 574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); > | ~~~~~~~~~~ ^ > include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' > 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) > | ^ > In file included from drivers/iio/adc/ad4695.c:24: > In file included from include/linux/iio/triggered_buffer.h:6: > In file included from include/linux/interrupt.h:11: > In file included from include/linux/hardirq.h:11: > In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: > In file included from include/asm-generic/hardirq.h:17: > In file included from include/linux/irq.h:20: > In file included from include/linux/io.h:14: > In file included from arch/hexagon/include/asm/io.h:328: > include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] > 585 | __raw_writeb(value, PCI_IOBASE + addr); > | ~~~~~~~~~~ ^ > include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] > 595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); > | ~~~~~~~~~~ ^ > include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] > 605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); > | ~~~~~~~~~~ ^ > In file included from drivers/iio/adc/ad4695.c:28: > In file included from include/linux/regulator/consumer.h:35: > In file included from include/linux/suspend.h:5: > In file included from include/linux/swap.h:9: > In file included from include/linux/memcontrol.h:21: > In file included from include/linux/mm.h:2228: > include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] > 514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" > | ~~~~~~~~~~~ ^ ~~~ > >> drivers/iio/adc/ad4695.c:454:8: warning: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((mask), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (mask)))' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare] > 454 | val = FIELD_PREP(mask, temp_chan_en ? 1 : 0); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/bitfield.h:115:3: note: expanded from macro 'FIELD_PREP' > 115 | __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/bitfield.h:72:53: note: expanded from macro '__BF_FIELD_CHECK' > 72 | BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \ > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ > 73 | __bf_cast_unsigned(_reg, ~0ull), \ > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 74 | _pfx "type of reg too small for mask"); \ > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG' > 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > | ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ > include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert' > 510 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert' > 498 | __compiletime_assert(condition, msg, prefix, suffix) > | ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert' > 490 | if (!(condition)) \ > | ^~~~~~~~~ > 8 warnings generated. > > > vim +454 drivers/iio/adc/ad4695.c > > 373 > 374 static int ad4695_buffer_preenable(struct iio_dev *indio_dev) > 375 { > 376 struct ad4695_state *st = iio_priv(indio_dev); > 377 struct spi_transfer *xfer; > 378 u8 temp_chan_bit = st->chip_info->num_voltage_inputs; > 379 bool temp_chan_en = false; > 380 u32 reg, mask, val, bit, num_xfer, num_slots; > 381 int ret; > 382 > 383 /* > 384 * We are using the advanced sequencer since it is the only way to read > 385 * multiple channels that allows individual configuration of each > 386 * voltage input channel. Slot 0 in the advanced sequencer is used to > 387 * account for the gap between trigger polls - we don't read data from > 388 * this slot. Each enabled voltage channel is assigned a slot starting > 389 * with slot 1. > 390 */ > 391 num_slots = 1; > 392 > 393 memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer)); > 394 > 395 /* First xfer is only to trigger conversion of slot 1, so no rx. */ > 396 xfer = &st->buf_read_xfer[0]; > 397 xfer->cs_change = 1; > 398 xfer->delay.value = AD4695_T_CNVL_NS; > 399 xfer->delay.unit = SPI_DELAY_UNIT_NSECS; > 400 xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; > 401 xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; > 402 num_xfer = 1; > 403 > 404 iio_for_each_active_channel(indio_dev, bit) { > 405 xfer = &st->buf_read_xfer[num_xfer]; > 406 xfer->bits_per_word = 16; > 407 xfer->rx_buf = &st->buf[(num_xfer - 1) * 2]; > 408 xfer->len = 2; > 409 xfer->cs_change = 1; > 410 xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; > 411 xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; > 412 > 413 if (bit == temp_chan_bit) { > 414 temp_chan_en = true; > 415 } else { > 416 reg = AD4695_REG_AS_SLOT(num_slots); > 417 val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit); > 418 > 419 ret = regmap_write(st->regmap, reg, val); > 420 if (ret) > 421 return ret; > 422 > 423 num_slots++; > 424 } > 425 > 426 num_xfer++; > 427 } > 428 > 429 /* > 430 * Don't keep CS asserted after last xfer. Also triggers conversion of > 431 * slot 0. > 432 */ > 433 xfer->cs_change = 0; > 434 > 435 /** > 436 * The advanced sequencer requires that at least 2 slots are enabled. > 437 * Since slot 0 is always used for other purposes, we need only 1 > 438 * enabled voltage channel to meet this requirement. This error will > 439 * only happen if only the temperature channel is enabled. > 440 */ > 441 if (num_slots < 2) { > 442 dev_err_ratelimited(&indio_dev->dev, > 443 "Buffered read requires at least 1 voltage channel enabled\n"); > 444 return -EINVAL; > 445 } > 446 > 447 /* > 448 * Temperature channel isn't included in the sequence, but rather > 449 * controlled by setting a bit in the TEMP_CTRL register. > 450 */ > 451 > 452 reg = AD4695_REG_TEMP_CTRL; > 453 mask = AD4695_REG_TEMP_CTRL_TEMP_EN; > > 454 val = FIELD_PREP(mask, temp_chan_en ? 1 : 0); Probably a case of the compiler somehow failing to squash the local variable. I'd just go with val = FIELD_PREP(AD4695_REG_TEMP_CTRL_TEMP_EN, temp_chan_en ? 1 : 0); > 455 > 456 ret = regmap_update_bits(st->regmap, reg, mask, val); > 457 if (ret) > 458 return ret; > 459 > 460 spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer, > 461 num_xfer); > 462 > 463 ret = spi_optimize_message(st->spi, &st->buf_read_msg); > 464 if (ret) > 465 return ret; > 466 > 467 /* This triggers conversion of slot 0. */ > 468 ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); > 469 if (ret) { > 470 spi_unoptimize_message(&st->buf_read_msg); > 471 return ret; > 472 } > 473 > 474 return 0; > 475 } > 476 >
On Wed, 7 Aug 2024 15:02:10 -0500 David Lechner <dlechner@baylibre.com> wrote: > This implements buffered reads for the ad4695 driver using the typical > triggered buffer implementation, including adding a soft timestamp > channel. > > The chip has 4 different modes for doing conversions. The driver is > using the advanced sequencer mode since that is the only mode that > allows individual configuration of all aspects each channel (e.g. > bipolar config currently and oversampling to be added in the future). > > Signed-off-by: David Lechner <dlechner@baylibre.com> Main thing in here is I think you can use available_scan_masks to avoid the need for the error path on just the temperature channel being enabled. Jonathan > +/** > + * ad4695_enter_advanced_sequencer_mode - Put the ADC in advanced sequencer mode > + * @st: The driver state > + * @n: The number of slots to use - must be >= 2, <= 128 > + * > + * As per the datasheet, to enable advanced sequencer, we need to set > + * STD_SEQ_EN=0, NUM_SLOTS_AS=n-1 and CYC_CTRL=0 (Table 15). Setting SPI_MODE=1 > + * triggers the first conversion using the channel in AS_SLOT0. > + * > + * Return: 0 on success, a negative error code on failure > + */ > +static int ad4695_enter_advanced_sequencer_mode(struct ad4695_state *st, u32 n) > +{ > + u32 mask, val; > + int ret; > + > + mask = AD4695_REG_SEQ_CTRL_STD_SEQ_EN; > + val = FIELD_PREP(AD4695_REG_SEQ_CTRL_STD_SEQ_EN, 0); > + > + mask |= AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS; > + val |= FIELD_PREP(AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS, n - 1); > + > + ret = regmap_update_bits(st->regmap, AD4695_REG_SEQ_CTRL, mask, val); > + if (ret) > + return ret; > + > + mask = AD4695_REG_SETUP_SPI_MODE; > + val = FIELD_PREP(AD4695_REG_SETUP_SPI_MODE, 1); > + > + mask |= AD4695_REG_SETUP_SPI_CYC_CTRL; > + val |= FIELD_PREP(AD4695_REG_SETUP_SPI_CYC_CTRL, 0); I'd just combine the two parts of mask and val. If it were a long complex list then fair enough to keep them as individual parts, but not needed for 2 items. > + > + return regmap_update_bits(st->regmap, AD4695_REG_SETUP, mask, val); > +} > + > +/** > + * ad4695_exit_conversion_mode - Exit conversion mode > + * @st: The AD4695 state > + * > + * Sends SPI command to exit conversion mode. > + * > + * Return: 0 on success, a negative error code on failure > + */ > +static int ad4695_exit_conversion_mode(struct ad4695_state *st) > +{ > + struct spi_transfer xfer = { }; struct spi_transfer xfer = { .tx_buf = &st->cnv_cmd2, .len = 1, .delay.value ... }; Might as well fill it in from the start. Doesn't matter that the data is filled in just after this even if that doesn't feel quite right, the code is so close I don't think it will confuse readers and it's a common pattern. > + > + st->cnv_cmd2 = AD4695_CMD_EXIT_CNV_MODE << 3; > + xfer.tx_buf = &st->cnv_cmd2; > + xfer.len = 1; > + xfer.delay.value = AD4695_T_REGCONFIG_NS; > + xfer.delay.unit = SPI_DELAY_UNIT_NSECS; > + > + return spi_sync_transfer(st->spi, &xfer, 1); > +} > + > static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv) > { > u8 val; > @@ -296,6 +371,147 @@ static int ad4695_write_chn_cfg(struct ad4695_state *st, > mask, val); > } > > +static int ad4695_buffer_preenable(struct iio_dev *indio_dev) > +{ > + struct ad4695_state *st = iio_priv(indio_dev); > + struct spi_transfer *xfer; > + u8 temp_chan_bit = st->chip_info->num_voltage_inputs; > + bool temp_chan_en = false; > + u32 reg, mask, val, bit, num_xfer, num_slots; > + int ret; > + > + /* > + * We are using the advanced sequencer since it is the only way to read > + * multiple channels that allows individual configuration of each > + * voltage input channel. Slot 0 in the advanced sequencer is used to > + * account for the gap between trigger polls - we don't read data from > + * this slot. Each enabled voltage channel is assigned a slot starting > + * with slot 1. > + */ > + num_slots = 1; > + > + memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer)); > + > + /* First xfer is only to trigger conversion of slot 1, so no rx. */ > + xfer = &st->buf_read_xfer[0]; > + xfer->cs_change = 1; > + xfer->delay.value = AD4695_T_CNVL_NS; > + xfer->delay.unit = SPI_DELAY_UNIT_NSECS; > + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; > + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; > + num_xfer = 1; > + > + iio_for_each_active_channel(indio_dev, bit) { > + xfer = &st->buf_read_xfer[num_xfer]; > + xfer->bits_per_word = 16; > + xfer->rx_buf = &st->buf[(num_xfer - 1) * 2]; > + xfer->len = 2; > + xfer->cs_change = 1; > + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; > + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; > + > + if (bit == temp_chan_bit) { > + temp_chan_en = true; > + } else { > + reg = AD4695_REG_AS_SLOT(num_slots); > + val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit); > + > + ret = regmap_write(st->regmap, reg, val); > + if (ret) > + return ret; > + > + num_slots++; > + } > + > + num_xfer++; > + } > + > + /* > + * Don't keep CS asserted after last xfer. Also triggers conversion of > + * slot 0. > + */ > + xfer->cs_change = 0; > + > + /** > + * The advanced sequencer requires that at least 2 slots are enabled. > + * Since slot 0 is always used for other purposes, we need only 1 > + * enabled voltage channel to meet this requirement. This error will > + * only happen if only the temperature channel is enabled. > + */ > + if (num_slots < 2) { Can you use available_scanmasks to let the IIO core figure out it needs to enable (and then hide) an extra channel? Either that or spin up a channel to meet the requirement, just don't capture it - Given this is an unlikely case, better to leave it to the IIO core buffer demux handling than to bother handling locally. > + dev_err_ratelimited(&indio_dev->dev, > + "Buffered read requires at least 1 voltage channel enabled\n"); > + return -EINVAL; > + } > + > + /* > + * Temperature channel isn't included in the sequence, but rather > + * controlled by setting a bit in the TEMP_CTRL register. > + */ > + > + reg = AD4695_REG_TEMP_CTRL; > + mask = AD4695_REG_TEMP_CTRL_TEMP_EN; > + val = FIELD_PREP(mask, temp_chan_en ? 1 : 0); This is the line the bot didn't like. The local variables reg and mask don't add anything anyway, so get rid of them and use the values inline. > + > + ret = regmap_update_bits(st->regmap, reg, mask, val); > + if (ret) > + return ret; > + > + spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer, > + num_xfer); > + > + ret = spi_optimize_message(st->spi, &st->buf_read_msg); > + if (ret) > + return ret; > + > + /* This triggers conversion of slot 0. */ > + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); > + if (ret) { > + spi_unoptimize_message(&st->buf_read_msg); > + return ret; > + } > + > + return 0; > +}
On 8/10/24 4:35 AM, Jonathan Cameron wrote: > On Wed, 7 Aug 2024 15:02:10 -0500 > David Lechner <dlechner@baylibre.com> wrote: > >> This implements buffered reads for the ad4695 driver using the typical >> triggered buffer implementation, including adding a soft timestamp >> channel. >> >> The chip has 4 different modes for doing conversions. The driver is >> using the advanced sequencer mode since that is the only mode that >> allows individual configuration of all aspects each channel (e.g. >> bipolar config currently and oversampling to be added in the future). >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> > > Main thing in here is I think you can use available_scan_masks > to avoid the need for the error path on just the temperature channel > being enabled. > I had not thought about doing it that way, but now that I am thinking about it, it seems like we would need to have a scan mask in the list for every possible combination of channels. This would be 10s of thousands of possible scan masks for 16 channel chips so that doesn't seem like the best way to go. But adding some special handling to make the temperature channel just work should be easy enough to add.
On Mon, 2024-08-12 at 12:03 -0500, David Lechner wrote: > On 8/10/24 4:35 AM, Jonathan Cameron wrote: > > On Wed, 7 Aug 2024 15:02:10 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > This implements buffered reads for the ad4695 driver using the typical > > > triggered buffer implementation, including adding a soft timestamp > > > channel. > > > > > > The chip has 4 different modes for doing conversions. The driver is > > > using the advanced sequencer mode since that is the only mode that > > > allows individual configuration of all aspects each channel (e.g. > > > bipolar config currently and oversampling to be added in the future). > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > > Main thing in here is I think you can use available_scan_masks > > to avoid the need for the error path on just the temperature channel > > being enabled. > > > I had not thought about doing it that way, but now that I am > thinking about it, it seems like we would need to have a scan > mask in the list for every possible combination of channels. > This would be 10s of thousands of possible scan masks for 16 > channel chips so that doesn't seem like the best way to go. > > But adding some special handling to make the temperature > channel just work should be easy enough to add. > Not sure if the following is meaningful to this usecase but I used to think like you but then realized that iio_scan_mask_match() will do bitmap_subset(). So you only need to enable a subset of the available scan mask for things to work (and with that you should no longer need an insane number of combinations). The core will then take care of demuxing the actual enabled channels. AFAIR, strict scan matching is only used for HW buffering. - Nuno Sá
On Tue, 13 Aug 2024 09:28:06 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Mon, 2024-08-12 at 12:03 -0500, David Lechner wrote: > > On 8/10/24 4:35 AM, Jonathan Cameron wrote: > > > On Wed, 7 Aug 2024 15:02:10 -0500 > > > David Lechner <dlechner@baylibre.com> wrote: > > > > > > > This implements buffered reads for the ad4695 driver using the typical > > > > triggered buffer implementation, including adding a soft timestamp > > > > channel. > > > > > > > > The chip has 4 different modes for doing conversions. The driver is > > > > using the advanced sequencer mode since that is the only mode that > > > > allows individual configuration of all aspects each channel (e.g. > > > > bipolar config currently and oversampling to be added in the future). > > > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > > > > Main thing in here is I think you can use available_scan_masks > > > to avoid the need for the error path on just the temperature channel > > > being enabled. > > > > > I had not thought about doing it that way, but now that I am > > thinking about it, it seems like we would need to have a scan > > mask in the list for every possible combination of channels. > > This would be 10s of thousands of possible scan masks for 16 > > channel chips so that doesn't seem like the best way to go. Indeed not my best suggestion. > > > > But adding some special handling to make the temperature > > channel just work should be easy enough to add. > > > > Not sure if the following is meaningful to this usecase but I used to think like you > but then realized that iio_scan_mask_match() will do bitmap_subset(). So you only > need to enable a subset of the available scan mask for things to work (and with that > you should no longer need an insane number of combinations). The core will then take > care of demuxing the actual enabled channels. AFAIR, strict scan matching is only > used for HW buffering. Hmm. The validate_scan_mask callback also doesn't work as that's really about restricting cases where we are onehot or similar (so restricting number of channels or that sort of thing). So, I think this is a driver problem to hide it. Just have some logic in the driver that enables a dummy channel if only the temperature is enabled and throw it away (probably fine to put it in the data passed to iio_push_to_buffers() and rely on the it either being treated as garbage, or dropped depending on whether it is in a hole, or on the end of the scan. That should give the intuitive interface we expect (no restrictions to bite us that don't have to be there - see onehot case where we have no choice) without adding too much complexity. Jonathan > > - Nuno Sá
diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c index 007ecb951bc3..a3bd5be36134 100644 --- a/drivers/iio/adc/ad4695.c +++ b/drivers/iio/adc/ad4695.c @@ -11,6 +11,7 @@ * Copyright 2024 BayLibre, SAS */ +#include <linux/align.h> #include <linux/bitfield.h> #include <linux/bits.h> #include <linux/compiler.h> @@ -18,7 +19,10 @@ #include <linux/device.h> #include <linux/err.h> #include <linux/gpio/consumer.h> +#include <linux/iio/buffer.h> #include <linux/iio/iio.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/iio/trigger_consumer.h> #include <linux/property.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> @@ -61,6 +65,7 @@ #define AD4695_REG_GPIO_CTRL 0x0026 #define AD4695_REG_GP_MODE 0x0027 #define AD4695_REG_TEMP_CTRL 0x0029 +#define AD4695_REG_TEMP_CTRL_TEMP_EN BIT(0) #define AD4695_REG_CONFIG_IN(n) (0x0030 | (n)) #define AD4695_REG_CONFIG_IN_MODE BIT(6) #define AD4695_REG_CONFIG_IN_PAIR GENMASK(5, 4) @@ -80,13 +85,18 @@ #define AD4695_CMD_VOLTAGE_CHAN(n) (0x10 | (n)) /* timing specs */ +#define AD4695_T_CNVL_NS 80 #define AD4695_T_CONVERT_NS 415 #define AD4695_T_WAKEUP_HW_MS 3 #define AD4695_T_WAKEUP_SW_MS 3 #define AD4695_T_REFBUF_MS 100 +#define AD4695_T_REGCONFIG_NS 20 #define AD4695_REG_ACCESS_SCLK_HZ (10 * MEGA) +/* Max number of voltage input channels. */ #define AD4695_MAX_CHANNELS 16 +/* Max size of 1 raw sample in bytes. */ +#define AD4695_MAX_CHANNEL_SIZE 2 enum ad4695_in_pair { AD4695_IN_PAIR_REFGND, @@ -112,15 +122,21 @@ struct ad4695_state { struct spi_device *spi; struct regmap *regmap; struct gpio_desc *reset_gpio; - struct iio_chan_spec iio_chan[AD4695_MAX_CHANNELS + 1]; + /* voltages channels plus temperature and timestamp */ + struct iio_chan_spec iio_chan[AD4695_MAX_CHANNELS + 2]; struct ad4695_channel_config channels_cfg[AD4695_MAX_CHANNELS]; const struct ad4695_chip_info *chip_info; /* Reference voltage. */ unsigned int vref_mv; /* Common mode input pin voltage. */ unsigned int com_mv; + /* 1 per voltage and temperature chan plus 1 xfer to trigger 1st CNV */ + struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS + 2]; + struct spi_message buf_read_msg; /* Raw conversion data received. */ - u16 raw_data __aligned(IIO_DMA_MINALIGN); + u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE, + sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN); + u16 raw_data; /* Commands to send for single conversion. */ u16 cnv_cmd; u8 cnv_cmd2; @@ -193,6 +209,9 @@ static const struct iio_chan_spec ad4695_temp_channel_template = { }, }; +static const struct iio_chan_spec ad4695_soft_timestamp_channel_template = + IIO_CHAN_SOFT_TIMESTAMP(0); + static const char * const ad4695_power_supplies[] = { "avdd", "vio" }; @@ -254,6 +273,62 @@ static int ad4695_set_single_cycle_mode(struct ad4695_state *st, AD4695_REG_SETUP_SPI_CYC_CTRL); } +/** + * ad4695_enter_advanced_sequencer_mode - Put the ADC in advanced sequencer mode + * @st: The driver state + * @n: The number of slots to use - must be >= 2, <= 128 + * + * As per the datasheet, to enable advanced sequencer, we need to set + * STD_SEQ_EN=0, NUM_SLOTS_AS=n-1 and CYC_CTRL=0 (Table 15). Setting SPI_MODE=1 + * triggers the first conversion using the channel in AS_SLOT0. + * + * Return: 0 on success, a negative error code on failure + */ +static int ad4695_enter_advanced_sequencer_mode(struct ad4695_state *st, u32 n) +{ + u32 mask, val; + int ret; + + mask = AD4695_REG_SEQ_CTRL_STD_SEQ_EN; + val = FIELD_PREP(AD4695_REG_SEQ_CTRL_STD_SEQ_EN, 0); + + mask |= AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS; + val |= FIELD_PREP(AD4695_REG_SEQ_CTRL_NUM_SLOTS_AS, n - 1); + + ret = regmap_update_bits(st->regmap, AD4695_REG_SEQ_CTRL, mask, val); + if (ret) + return ret; + + mask = AD4695_REG_SETUP_SPI_MODE; + val = FIELD_PREP(AD4695_REG_SETUP_SPI_MODE, 1); + + mask |= AD4695_REG_SETUP_SPI_CYC_CTRL; + val |= FIELD_PREP(AD4695_REG_SETUP_SPI_CYC_CTRL, 0); + + return regmap_update_bits(st->regmap, AD4695_REG_SETUP, mask, val); +} + +/** + * ad4695_exit_conversion_mode - Exit conversion mode + * @st: The AD4695 state + * + * Sends SPI command to exit conversion mode. + * + * Return: 0 on success, a negative error code on failure + */ +static int ad4695_exit_conversion_mode(struct ad4695_state *st) +{ + struct spi_transfer xfer = { }; + + st->cnv_cmd2 = AD4695_CMD_EXIT_CNV_MODE << 3; + xfer.tx_buf = &st->cnv_cmd2; + xfer.len = 1; + xfer.delay.value = AD4695_T_REGCONFIG_NS; + xfer.delay.unit = SPI_DELAY_UNIT_NSECS; + + return spi_sync_transfer(st->spi, &xfer, 1); +} + static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv) { u8 val; @@ -296,6 +371,147 @@ static int ad4695_write_chn_cfg(struct ad4695_state *st, mask, val); } +static int ad4695_buffer_preenable(struct iio_dev *indio_dev) +{ + struct ad4695_state *st = iio_priv(indio_dev); + struct spi_transfer *xfer; + u8 temp_chan_bit = st->chip_info->num_voltage_inputs; + bool temp_chan_en = false; + u32 reg, mask, val, bit, num_xfer, num_slots; + int ret; + + /* + * We are using the advanced sequencer since it is the only way to read + * multiple channels that allows individual configuration of each + * voltage input channel. Slot 0 in the advanced sequencer is used to + * account for the gap between trigger polls - we don't read data from + * this slot. Each enabled voltage channel is assigned a slot starting + * with slot 1. + */ + num_slots = 1; + + memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer)); + + /* First xfer is only to trigger conversion of slot 1, so no rx. */ + xfer = &st->buf_read_xfer[0]; + xfer->cs_change = 1; + xfer->delay.value = AD4695_T_CNVL_NS; + xfer->delay.unit = SPI_DELAY_UNIT_NSECS; + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; + num_xfer = 1; + + iio_for_each_active_channel(indio_dev, bit) { + xfer = &st->buf_read_xfer[num_xfer]; + xfer->bits_per_word = 16; + xfer->rx_buf = &st->buf[(num_xfer - 1) * 2]; + xfer->len = 2; + xfer->cs_change = 1; + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS; + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; + + if (bit == temp_chan_bit) { + temp_chan_en = true; + } else { + reg = AD4695_REG_AS_SLOT(num_slots); + val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit); + + ret = regmap_write(st->regmap, reg, val); + if (ret) + return ret; + + num_slots++; + } + + num_xfer++; + } + + /* + * Don't keep CS asserted after last xfer. Also triggers conversion of + * slot 0. + */ + xfer->cs_change = 0; + + /** + * The advanced sequencer requires that at least 2 slots are enabled. + * Since slot 0 is always used for other purposes, we need only 1 + * enabled voltage channel to meet this requirement. This error will + * only happen if only the temperature channel is enabled. + */ + if (num_slots < 2) { + dev_err_ratelimited(&indio_dev->dev, + "Buffered read requires at least 1 voltage channel enabled\n"); + return -EINVAL; + } + + /* + * Temperature channel isn't included in the sequence, but rather + * controlled by setting a bit in the TEMP_CTRL register. + */ + + reg = AD4695_REG_TEMP_CTRL; + mask = AD4695_REG_TEMP_CTRL_TEMP_EN; + val = FIELD_PREP(mask, temp_chan_en ? 1 : 0); + + ret = regmap_update_bits(st->regmap, reg, mask, val); + if (ret) + return ret; + + spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer, + num_xfer); + + ret = spi_optimize_message(st->spi, &st->buf_read_msg); + if (ret) + return ret; + + /* This triggers conversion of slot 0. */ + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots); + if (ret) { + spi_unoptimize_message(&st->buf_read_msg); + return ret; + } + + return 0; +} + +static int ad4695_buffer_postdisable(struct iio_dev *indio_dev) +{ + struct ad4695_state *st = iio_priv(indio_dev); + int ret; + + ret = ad4695_exit_conversion_mode(st); + if (ret) + return ret; + + spi_unoptimize_message(&st->buf_read_msg); + + return 0; +} + +static const struct iio_buffer_setup_ops ad4695_buffer_setup_ops = { + .preenable = ad4695_buffer_preenable, + .postdisable = ad4695_buffer_postdisable, +}; + +static irqreturn_t ad4695_trigger_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct ad4695_state *st = iio_priv(indio_dev); + int ret; + + ret = spi_sync(st->spi, &st->buf_read_msg); + if (ret) + goto out; + + iio_push_to_buffers_with_timestamp(indio_dev, st->buf, pf->timestamp); + +out: + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + /** * ad4695_read_one_sample - Read a single sample using single-cycle mode * @st: The AD4695 state @@ -527,6 +743,10 @@ static int ad4695_parse_channel_cfg(struct ad4695_state *st) /* Temperature channel must be next scan index after voltage channels. */ st->iio_chan[i] = ad4695_temp_channel_template; st->iio_chan[i].scan_index = i; + i++; + + st->iio_chan[i] = ad4695_soft_timestamp_channel_template; + st->iio_chan[i].scan_index = i; return 0; } @@ -695,7 +915,14 @@ static int ad4695_probe(struct spi_device *spi) indio_dev->info = &ad4695_info; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = st->iio_chan; - indio_dev->num_channels = st->chip_info->num_voltage_inputs + 1; + indio_dev->num_channels = st->chip_info->num_voltage_inputs + 2; + + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, + iio_pollfunc_store_time, + ad4695_trigger_handler, + &ad4695_buffer_setup_ops); + if (ret) + return ret; return devm_iio_device_register(dev, indio_dev); }
This implements buffered reads for the ad4695 driver using the typical triggered buffer implementation, including adding a soft timestamp channel. The chip has 4 different modes for doing conversions. The driver is using the advanced sequencer mode since that is the only mode that allows individual configuration of all aspects each channel (e.g. bipolar config currently and oversampling to be added in the future). Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 230 insertions(+), 3 deletions(-)