* [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
@ 2024-09-19 13:04 Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros Alexandru Ardelean
` (7 more replies)
0 siblings, 8 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean
The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
The main difference between AD7606C-16 & AD7606C-18 is the precision in
bits (16 vs 18).
Because of that, some scales need to be defined for the 18-bit variants, as
they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
Because the AD7606C-16,18 also supports bipolar & differential channels,
for SW-mode, the default range of 10 V or ±10V should be set at probe.
On reset, the default range (in the registers) is set to value 0x3 which
corresponds to '±10 V single-ended range', regardless of bipolar or
differential configuration.
Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
This changeset, does a bit of rework to the existing ad7606 driver and then
adds support for the AD7606C-16 & AD7606C-18 parts.
Datasheet links:
https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
Changelog v6 -> v7:
- v6: https://lore.kernel.org/linux-iio/20240913135744.152669-1-aardelean@baylibre.com/
- Patch 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
- Properly implemented HW scales for AD7606C-{16,18} in 'ad7606c_18bit_chan_scale_setup'
& 'ad7606c_16bit_chan_scale_setup()' ; I omitted them by accident in v6 as
I was focusing too much on SW scales.
Changelog v5 -> v6:
- v5: https://lore.kernel.org/linux-iio/20240907065043.771364-1-aardelean@baylibre.com/
- Dropped patch 'iio: adc: ad7606: remove frstdata check for serial mode'
- It's part of iio/testing now
- For patch 'iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()'
- Added early return 'if (ret) goto error_ret'
- Reworked patch 'iio: adc: ad7606: split a 'ad7606_sw_mode_setup()' from probe'
to 'iio: adc: ad7606: move scale_setup as function pointer on chip-info'
- Splitting 'ad7606_sw_mode_setup()' & 'ad7606_chan_scales_setup()' from probe
- Added scale_setup_cb hook to chip_info table
- For patch 'iio: adc: ad7606: add 'bits' parameter to channels macros'
- Adjusted paragraph 'Up until now, all chips were 16 bit.'
- Patch 'iio: adc: ad7606: wrap channel ranges & scales into struct' re-applied
on new context
- Patch 'iio: adc: ad7606: rework available attributes for SW channels'
re-applied on new context
- Patch 'dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts'
- Simplified 'bipolar' property ; it is now mandatory if channels node exists
- If no channel node exists it's unipolar single-ended
- Update descriptions to reflect this
- For patch 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
- In 'ad7606_spi_read_block18to32' change 'sizeof(uint32_t)' -> 'sizeof(u32)'
- In ad7606.h changed d16/d32 -> buf16/buf32; and types to u16/u32
- Reworked channel setup logic
Changelog v4 -> v5:
- v4: https://lore.kernel.org/linux-iio/20240905082404.119022-1-aardelean@baylibre.com/
- Not all topics from v4 have been fully resolved; but I created a v5 in
case it helps to spot other (new) topics
- Added patch 'iio: adc: ad7606: remove frstdata check for serial mode'
- This is from the 'fixes-togreg' branch
- It should be ignored
- Should help with a bit of context for this series
- For patch 'iio: adc: ad7606: add 'bits' parameter to channels macros'
- Added '.storagebits = (bits) > 16 ? 32 : 16'
- Reduces the final patch a bit
- i.e. 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
- For patch 'iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct() '
- Added 'Reviewed-by: David Lechner <dlechner@baylibre.com>'
- For patch 'iio: adc: ad7606: rework available attributes for SW channels'
- Added '.storagebits = (bits) > 16 ? 32 : 16'
- Reduces the final patch a bit
- i.e. 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
- For patch 'dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts'
- Added '"^channel@[1-8]$": false' if not 'adi,sw-mode'
- Added 'Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>'
- Given the change, I would need confirmation that the Krzysztof's
Reviewed-by tag is still valid
- There is still an open topic about using
'oneOf:required:{diff-channels,bipolar}' vs
'if:required: [diff-channels]then:required: [bipolar]'
- I'm leaning towards 'oneOf:required:{diff-channels,bipolar}'
- Let's see what a discussio will yield
- For patch 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
- In 'ad7606_spi_read_block18to32()' fixed
'.len = count * sizeof(uint32_t)' in 'struct spi_transfer xfer'
- In 'ad7606_read_samples()' changed 'u16 *data = st->data;' to
'void *data = st->data.d16;' ; both would compile though ;
converting 'data' to 'void *' may show that it's not just 16 bits
- In ad7606c_18_chan_setup() & ad7606c_16_chan_setup()
- Added explicit 'cs->reg_offset = 0;'
- In 'ad7606c_sw_mode_setup_channels()' :
- If pins are specified incorrectly, an error is triggered (vs
ignoring it before)
- Updated comment about why 'st->bops->sw_mode_config()' is called
first
Changelog v3 -> v4:
- v3: https://lore.kernel.org/linux-iio/20240904072718.1143440-1-aardelean@baylibre.com/
- For patch 'dt-bindings: iio: adc: document diff-channels corner case
for some ADCs'
- Added 'Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>'
- Renamed patch 'dt-bindings: iio: adc: add adi,ad7606c-{16,18} compatible strings'
to 'dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts'
- Updated based on notes from Krzysztof Kozlowski (from v3)
- Dropped ()
- Re-ordered the patternProperties:oneOf:required specification
- Unified match-pattern to '^channel@[1-8]$'
Changelog v2 -> v3:
- v2: https://lore.kernel.org/linux-iio/20240902103638.686039-1-aardelean@baylibre.com/
- Applied checkpatch.pl changes
- Managed to setup and run 'make dt_binding_check DT_SCHEMA_FILES=adi,ad7606.yaml'
- Found the winning combination for this setup
- David Lechner also helped
- For patch 'iio: adc: ad7606: rework available attributes for SW channels'
- Removed an extra space that checkpatch found
- For patch 'dt-bindings: iio: adc: document diff-channels corner case
for some ADCs'
- Removed 'the the' stutter (that I did in writing)
- For patch 'dt-bindings: iio: adc: add adi,ad7606c-{16,18} compatible strings'
- Updated binding with some description for 'diff-channels' & 'bipolar'
properties
- Channel definitions are counted from 1 to 8 to match datasheet
- Added more bindings rules for 'diff-channels' & 'bipolar' for AD7606C
- Adapted some ideas from adi,ad7192.yaml
- For patch 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
- Updated 'diff-channels' property with channel numbers (from 1 to 8)
handling
Changelog v1 -> v2:
- v1: https://lore.kernel.org/linux-iio/20240819064721.91494-1-aardelean@baylibre.com/
- Fixed description in 'iio: adc: ad7606: add 'bits' parameter to channels macros'
- Added patch 'dt-bindings: iio: adc: document diff-channels corner case
for some ADCs'
- diff-channels = <reg reg> can be used to define differential channels
with dedicated positive + negative pins
- Re-worked patch 'dt-bindings: iio: adc: add adi,ad7606c-{16,18} compatible strings'
- Using standard 'diff-channels' & 'bipolar' properties from adc.yaml
- Re-worked patch 'iio: adc: ad7606: add support for AD7606C-{16,18} parts'
- Reading 18-bit samples now relies on SPI controllers being able to
pad 18-bits to 32-bits.
- Implemented 'diff-channels = <reg reg>' setting
- Removed some bad/left-over channel configuration code which I forgot
during development and rebasing.
Alexandru Ardelean (8):
iio: adc: ad7606: add 'bits' parameter to channels macros
iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()
iio: adc: ad7606: move scale_setup as function pointer on chip-info
iio: adc: ad7606: wrap channel ranges & scales into struct
iio: adc: ad7606: rework available attributes for SW channels
dt-bindings: iio: adc: document diff-channels corner case for some
ADCs
dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts
iio: adc: ad7606: add support for AD7606C-{16,18} parts
.../devicetree/bindings/iio/adc/adc.yaml | 4 +
.../bindings/iio/adc/adi,ad7606.yaml | 120 +++++
drivers/iio/adc/ad7606.c | 474 +++++++++++++++---
drivers/iio/adc/ad7606.h | 86 +++-
drivers/iio/adc/ad7606_spi. | 0
drivers/iio/adc/ad7606_spi.c | 71 ++-
6 files changed, 663 insertions(+), 92 deletions(-)
create mode 100644 drivers/iio/adc/ad7606_spi.
--
2.46.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
@ 2024-09-19 13:04 ` Alexandru Ardelean
2024-09-23 14:50 ` David Lechner
2024-09-19 13:04 ` [PATCH v7 2/8] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct() Alexandru Ardelean
` (6 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean
There are some newer additions to the AD7606 family, which support 18 bit
precision. Up until now, all chips were 16 bit.
This change adds a 'bits' parameter to the AD760X_CHANNEL macro and renames
'ad7606_channels' -> 'ad7606_channels_16bit' for the current devices.
The AD7606_SW_CHANNEL() macro is also introduced, as a short-hand for IIO
channels in SW mode.
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
drivers/iio/adc/ad7606.c | 58 ++++++++++++++++++------------------
drivers/iio/adc/ad7606.h | 18 ++++++-----
drivers/iio/adc/ad7606_spi.c | 16 +++++-----
3 files changed, 47 insertions(+), 45 deletions(-)
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 9b457472d49c..8ebfe8abc3f4 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -309,16 +309,16 @@ static const struct iio_chan_spec ad7605_channels[] = {
AD7605_CHANNEL(3),
};
-static const struct iio_chan_spec ad7606_channels[] = {
+static const struct iio_chan_spec ad7606_channels_16bit[] = {
IIO_CHAN_SOFT_TIMESTAMP(8),
- AD7606_CHANNEL(0),
- AD7606_CHANNEL(1),
- AD7606_CHANNEL(2),
- AD7606_CHANNEL(3),
- AD7606_CHANNEL(4),
- AD7606_CHANNEL(5),
- AD7606_CHANNEL(6),
- AD7606_CHANNEL(7),
+ AD7606_CHANNEL(0, 16),
+ AD7606_CHANNEL(1, 16),
+ AD7606_CHANNEL(2, 16),
+ AD7606_CHANNEL(3, 16),
+ AD7606_CHANNEL(4, 16),
+ AD7606_CHANNEL(5, 16),
+ AD7606_CHANNEL(6, 16),
+ AD7606_CHANNEL(7, 16),
};
/*
@@ -333,22 +333,22 @@ static const struct iio_chan_spec ad7606_channels[] = {
*/
static const struct iio_chan_spec ad7616_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(16),
- AD7606_CHANNEL(0),
- AD7606_CHANNEL(1),
- AD7606_CHANNEL(2),
- AD7606_CHANNEL(3),
- AD7606_CHANNEL(4),
- AD7606_CHANNEL(5),
- AD7606_CHANNEL(6),
- AD7606_CHANNEL(7),
- AD7606_CHANNEL(8),
- AD7606_CHANNEL(9),
- AD7606_CHANNEL(10),
- AD7606_CHANNEL(11),
- AD7606_CHANNEL(12),
- AD7606_CHANNEL(13),
- AD7606_CHANNEL(14),
- AD7606_CHANNEL(15),
+ AD7606_CHANNEL(0, 16),
+ AD7606_CHANNEL(1, 16),
+ AD7606_CHANNEL(2, 16),
+ AD7606_CHANNEL(3, 16),
+ AD7606_CHANNEL(4, 16),
+ AD7606_CHANNEL(5, 16),
+ AD7606_CHANNEL(6, 16),
+ AD7606_CHANNEL(7, 16),
+ AD7606_CHANNEL(8, 16),
+ AD7606_CHANNEL(9, 16),
+ AD7606_CHANNEL(10, 16),
+ AD7606_CHANNEL(11, 16),
+ AD7606_CHANNEL(12, 16),
+ AD7606_CHANNEL(13, 16),
+ AD7606_CHANNEL(14, 16),
+ AD7606_CHANNEL(15, 16),
};
static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
@@ -358,25 +358,25 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
.num_channels = 5,
},
[ID_AD7606_8] = {
- .channels = ad7606_channels,
+ .channels = ad7606_channels_16bit,
.num_channels = 9,
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
},
[ID_AD7606_6] = {
- .channels = ad7606_channels,
+ .channels = ad7606_channels_16bit,
.num_channels = 7,
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
},
[ID_AD7606_4] = {
- .channels = ad7606_channels,
+ .channels = ad7606_channels_16bit,
.num_channels = 5,
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
},
[ID_AD7606B] = {
- .channels = ad7606_channels,
+ .channels = ad7606_channels_16bit,
.num_channels = 9,
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 6649e84d25de..204a343067e5 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -8,7 +8,7 @@
#ifndef IIO_ADC_AD7606_H_
#define IIO_ADC_AD7606_H_
-#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all) { \
+#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) { \
.type = IIO_VOLTAGE, \
.indexed = 1, \
.channel = num, \
@@ -19,24 +19,26 @@
.scan_index = num, \
.scan_type = { \
.sign = 's', \
- .realbits = 16, \
- .storagebits = 16, \
+ .realbits = (bits), \
+ .storagebits = (bits) > 16 ? 32 : 16, \
.endianness = IIO_CPU, \
}, \
}
#define AD7605_CHANNEL(num) \
AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
- BIT(IIO_CHAN_INFO_SCALE), 0)
+ BIT(IIO_CHAN_INFO_SCALE), 0, 16)
-#define AD7606_CHANNEL(num) \
+#define AD7606_CHANNEL(num, bits) \
AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
BIT(IIO_CHAN_INFO_SCALE), \
- BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
-#define AD7616_CHANNEL(num) \
+#define AD7606_SW_CHANNEL(num, bits) \
AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
- 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
+ 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
+
+#define AD7616_CHANNEL(num) AD7606_SW_CHANNEL(num, 16)
/**
* struct ad7606_chip_info - chip specific information
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index 62ec12195307..e00f58a6a0e9 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -67,14 +67,14 @@ static const struct iio_chan_spec ad7616_sw_channels[] = {
static const struct iio_chan_spec ad7606b_sw_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(8),
- AD7616_CHANNEL(0),
- AD7616_CHANNEL(1),
- AD7616_CHANNEL(2),
- AD7616_CHANNEL(3),
- AD7616_CHANNEL(4),
- AD7616_CHANNEL(5),
- AD7616_CHANNEL(6),
- AD7616_CHANNEL(7),
+ AD7606_SW_CHANNEL(0, 16),
+ AD7606_SW_CHANNEL(1, 16),
+ AD7606_SW_CHANNEL(2, 16),
+ AD7606_SW_CHANNEL(3, 16),
+ AD7606_SW_CHANNEL(4, 16),
+ AD7606_SW_CHANNEL(5, 16),
+ AD7606_SW_CHANNEL(6, 16),
+ AD7606_SW_CHANNEL(7, 16),
};
static const unsigned int ad7606B_oversampling_avail[9] = {
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v7 2/8] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct()
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros Alexandru Ardelean
@ 2024-09-19 13:04 ` Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 3/8] iio: adc: ad7606: move scale_setup as function pointer on chip-info Alexandru Ardelean
` (5 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean
The ad7606_scan_direct() function returns 'int', which is fine for 16-bit
samples.
But when going to 18-bit samples, these need to be implemented as 32-bit
(or int) type.
In that case when getting samples (which can be negative), we'd get random
error codes.
So, the easiest thing is to just move the 'val' pointer to
'ad7606_scan_direct()'. This doesn't qualify as a fix, it's just a
preparation for 18-bit ADCs (of the AD7606 family).
Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
drivers/iio/adc/ad7606.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 8ebfe8abc3f4..032a8135c912 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -114,7 +114,8 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
return IRQ_HANDLED;
}
-static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
+static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
+ int *val)
{
struct ad7606_state *st = iio_priv(indio_dev);
int ret;
@@ -128,8 +129,10 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
}
ret = ad7606_read_samples(st);
- if (ret == 0)
- ret = st->data[ch];
+ if (ret)
+ goto error_ret;
+
+ *val = sign_extend32(st->data[ch], 15);
error_ret:
gpiod_set_value(st->gpio_convst, 0);
@@ -149,10 +152,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
switch (m) {
case IIO_CHAN_INFO_RAW:
iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = ad7606_scan_direct(indio_dev, chan->address);
+ ret = ad7606_scan_direct(indio_dev, chan->address, val);
if (ret < 0)
return ret;
- *val = (short) ret;
return IIO_VAL_INT;
}
unreachable();
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v7 3/8] iio: adc: ad7606: move scale_setup as function pointer on chip-info
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 2/8] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct() Alexandru Ardelean
@ 2024-09-19 13:04 ` Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 4/8] iio: adc: ad7606: wrap channel ranges & scales into struct Alexandru Ardelean
` (4 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean
Up until now, all ADCs were 16-bit precision.
With the addition of the AD7606C some things will change. For one thing,
we'll need to setup available-scales for each channel. Also for the 18-bit
precision variants, the scales will be different.
This change adds a function-pointer to the chip-info struct to be able to
set this up (differently) for the new parts. For the current parts, the
scales are the same (for all parts) between HW and SW modes.
Also creating a 'ad7606_sw_mode_setup()' function that must be called
before the scale_setup callback. This is needed in case SW mode is enabled
for some ADCs.
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
drivers/iio/adc/ad7606.c | 84 +++++++++++++++++++++++++++++-----------
drivers/iio/adc/ad7606.h | 6 +++
2 files changed, 68 insertions(+), 22 deletions(-)
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 032a8135c912..7dc299aeee15 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -32,12 +32,12 @@
* Scales are computed as 5000/32768 and 10000/32768 respectively,
* so that when applied to the raw values they provide mV values
*/
-static const unsigned int ad7606_scale_avail[2] = {
+static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
152588, 305176
};
-static const unsigned int ad7616_sw_scale_avail[3] = {
+static const unsigned int ad7606_16bit_sw_scale_avail[3] = {
76293, 152588, 305176
};
@@ -62,6 +62,25 @@ int ad7606_reset(struct ad7606_state *st)
}
EXPORT_SYMBOL_NS_GPL(ad7606_reset, IIO_AD7606);
+static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st, int ch)
+{
+ if (!st->sw_mode_en) {
+ /* tied to logic low, analog input range is +/- 5V */
+ st->range[ch] = 0;
+ st->scale_avail = ad7606_16bit_hw_scale_avail;
+ st->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
+ return 0;
+ }
+
+ /* Scale of 0.076293 is only available in sw mode */
+ /* After reset, in software mode, ±10 V is set by default */
+ st->range[ch] = 2;
+ st->scale_avail = ad7606_16bit_sw_scale_avail;
+ st->num_scales = ARRAY_SIZE(ad7606_16bit_sw_scale_avail);
+
+ return 0;
+}
+
static int ad7606_reg_access(struct iio_dev *indio_dev,
unsigned int reg,
unsigned int writeval,
@@ -358,34 +377,40 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
[ID_AD7605_4] = {
.channels = ad7605_channels,
.num_channels = 5,
+ .scale_setup_cb = ad7606_16bit_chan_scale_setup,
},
[ID_AD7606_8] = {
.channels = ad7606_channels_16bit,
.num_channels = 9,
+ .scale_setup_cb = ad7606_16bit_chan_scale_setup,
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
},
[ID_AD7606_6] = {
.channels = ad7606_channels_16bit,
.num_channels = 7,
+ .scale_setup_cb = ad7606_16bit_chan_scale_setup,
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
},
[ID_AD7606_4] = {
.channels = ad7606_channels_16bit,
.num_channels = 5,
+ .scale_setup_cb = ad7606_16bit_chan_scale_setup,
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
},
[ID_AD7606B] = {
.channels = ad7606_channels_16bit,
.num_channels = 9,
+ .scale_setup_cb = ad7606_16bit_chan_scale_setup,
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
},
[ID_AD7616] = {
.channels = ad7616_channels,
.num_channels = 17,
+ .scale_setup_cb = ad7606_16bit_chan_scale_setup,
.oversampling_avail = ad7616_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7616_oversampling_avail),
.os_req_reset = true,
@@ -523,6 +548,35 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
.validate_device = iio_trigger_validate_own_device,
};
+static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
+{
+ struct ad7606_state *st = iio_priv(indio_dev);
+
+ st->sw_mode_en = st->bops->sw_mode_config &&
+ device_property_present(st->dev, "adi,sw-mode");
+ if (!st->sw_mode_en)
+ return 0;
+
+ indio_dev->info = &ad7606_info_os_range_and_debug;
+
+ return st->bops->sw_mode_config(indio_dev);
+}
+
+static int ad7606_chan_scales_setup(struct iio_dev *indio_dev)
+{
+ unsigned int num_channels = indio_dev->num_channels - 1;
+ struct ad7606_state *st = iio_priv(indio_dev);
+ int ch, ret;
+
+ for (ch = 0; ch < num_channels; ch++) {
+ ret = st->chip_info->scale_setup_cb(st, ch);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
const char *name, unsigned int id,
const struct ad7606_bus_ops *bops)
@@ -542,11 +596,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
mutex_init(&st->lock);
st->bops = bops;
st->base_address = base_address;
- /* tied to logic low, analog input range is +/- 5V */
- st->range[0] = 0;
st->oversampling = 1;
- st->scale_avail = ad7606_scale_avail;
- st->num_scales = ARRAY_SIZE(ad7606_scale_avail);
ret = devm_regulator_get_enable(dev, "avcc");
if (ret)
@@ -595,23 +645,13 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
st->write_scale = ad7606_write_scale_hw;
st->write_os = ad7606_write_os_hw;
- if (st->bops->sw_mode_config)
- st->sw_mode_en = device_property_present(st->dev,
- "adi,sw-mode");
-
- if (st->sw_mode_en) {
- /* Scale of 0.076293 is only available in sw mode */
- st->scale_avail = ad7616_sw_scale_avail;
- st->num_scales = ARRAY_SIZE(ad7616_sw_scale_avail);
-
- /* After reset, in software mode, ±10 V is set by default */
- memset32(st->range, 2, ARRAY_SIZE(st->range));
- indio_dev->info = &ad7606_info_os_range_and_debug;
+ ret = ad7606_sw_mode_setup(indio_dev);
+ if (ret)
+ return ret;
- ret = st->bops->sw_mode_config(indio_dev);
- if (ret < 0)
- return ret;
- }
+ ret = ad7606_chan_scales_setup(indio_dev);
+ if (ret)
+ return ret;
st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
indio_dev->name,
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 204a343067e5..95f3b3cb0be3 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -40,10 +40,15 @@
#define AD7616_CHANNEL(num) AD7606_SW_CHANNEL(num, 16)
+struct ad7606_state;
+
+typedef int (*ad7606_scale_setup_cb_t)(struct ad7606_state *st, int ch);
+
/**
* struct ad7606_chip_info - chip specific information
* @channels: channel specification
* @num_channels: number of channels
+ * @scale_setup_cb: callback to setup the scales for each channel
* @oversampling_avail pointer to the array which stores the available
* oversampling ratios.
* @oversampling_num number of elements stored in oversampling_avail array
@@ -54,6 +59,7 @@
struct ad7606_chip_info {
const struct iio_chan_spec *channels;
unsigned int num_channels;
+ ad7606_scale_setup_cb_t scale_setup_cb;
const unsigned int *oversampling_avail;
unsigned int oversampling_num;
bool os_req_reset;
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v7 4/8] iio: adc: ad7606: wrap channel ranges & scales into struct
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
` (2 preceding siblings ...)
2024-09-19 13:04 ` [PATCH v7 3/8] iio: adc: ad7606: move scale_setup as function pointer on chip-info Alexandru Ardelean
@ 2024-09-19 13:04 ` Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 5/8] iio: adc: ad7606: rework available attributes for SW channels Alexandru Ardelean
` (3 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean
With the addition of AD7606C-16,18 which have differential & bipolar
channels (and ranges), which can vary from channel to channel, we'll need
to keep more information about each channel range.
To do that, we'll add a 'struct ad7606_chan_scale' type to hold just
configuration for each channel.
This includes the scales per channel (which can be different with
AD7606C-16,18), as well as the range for each channel.
This driver was already keeping the range value for each channel before,
and since this is couple with the scales, it also makes sense to put them
in the same struct.
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
drivers/iio/adc/ad7606.c | 29 ++++++++++++++++++-----------
drivers/iio/adc/ad7606.h | 22 ++++++++++++++++------
2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 7dc299aeee15..94a254c0725e 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -64,19 +64,21 @@ EXPORT_SYMBOL_NS_GPL(ad7606_reset, IIO_AD7606);
static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st, int ch)
{
+ struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+
if (!st->sw_mode_en) {
/* tied to logic low, analog input range is +/- 5V */
- st->range[ch] = 0;
- st->scale_avail = ad7606_16bit_hw_scale_avail;
- st->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
+ cs->range = 0;
+ cs->scale_avail = ad7606_16bit_hw_scale_avail;
+ cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
return 0;
}
/* Scale of 0.076293 is only available in sw mode */
/* After reset, in software mode, ±10 V is set by default */
- st->range[ch] = 2;
- st->scale_avail = ad7606_16bit_sw_scale_avail;
- st->num_scales = ARRAY_SIZE(ad7606_16bit_sw_scale_avail);
+ cs->range = 2;
+ cs->scale_avail = ad7606_16bit_sw_scale_avail;
+ cs->num_scales = ARRAY_SIZE(ad7606_16bit_sw_scale_avail);
return 0;
}
@@ -167,6 +169,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
{
int ret, ch = 0;
struct ad7606_state *st = iio_priv(indio_dev);
+ struct ad7606_chan_scale *cs;
switch (m) {
case IIO_CHAN_INFO_RAW:
@@ -180,8 +183,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_SCALE:
if (st->sw_mode_en)
ch = chan->address;
+ cs = &st->chan_scales[ch];
*val = 0;
- *val2 = st->scale_avail[st->range[ch]];
+ *val2 = cs->scale_avail[cs->range];
return IIO_VAL_INT_PLUS_MICRO;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
*val = st->oversampling;
@@ -211,8 +215,9 @@ static ssize_t in_voltage_scale_available_show(struct device *dev,
{
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct ad7606_state *st = iio_priv(indio_dev);
+ struct ad7606_chan_scale *cs = &st->chan_scales[0];
- return ad7606_show_avail(buf, st->scale_avail, st->num_scales, true);
+ return ad7606_show_avail(buf, cs->scale_avail, cs->num_scales, true);
}
static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
@@ -250,19 +255,21 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
long mask)
{
struct ad7606_state *st = iio_priv(indio_dev);
+ struct ad7606_chan_scale *cs;
int i, ret, ch = 0;
guard(mutex)(&st->lock);
switch (mask) {
case IIO_CHAN_INFO_SCALE:
- i = find_closest(val2, st->scale_avail, st->num_scales);
if (st->sw_mode_en)
ch = chan->address;
+ cs = &st->chan_scales[ch];
+ i = find_closest(val2, cs->scale_avail, cs->num_scales);
ret = st->write_scale(indio_dev, ch, i);
if (ret < 0)
return ret;
- st->range[ch] = i;
+ cs->range = i;
return 0;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -707,7 +714,7 @@ static int ad7606_resume(struct device *dev)
struct ad7606_state *st = iio_priv(indio_dev);
if (st->gpio_standby) {
- gpiod_set_value(st->gpio_range, st->range[0]);
+ gpiod_set_value(st->gpio_range, st->chan_scales[0].range);
gpiod_set_value(st->gpio_standby, 1);
ad7606_reset(st);
}
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 95f3b3cb0be3..2b90f52affba 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -8,6 +8,8 @@
#ifndef IIO_ADC_AD7606_H_
#define IIO_ADC_AD7606_H_
+#define AD760X_MAX_CHANNELS 16
+
#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) { \
.type = IIO_VOLTAGE, \
.indexed = 1, \
@@ -66,17 +68,27 @@ struct ad7606_chip_info {
unsigned long init_delay_ms;
};
+/**
+ * struct ad7606_chan_scale - channel scale configuration
+ * @scale_avail pointer to the array which stores the available scales
+ * @num_scales number of elements stored in the scale_avail array
+ * @range voltage range selection, selects which scale to apply
+ */
+struct ad7606_chan_scale {
+ const unsigned int *scale_avail;
+ unsigned int num_scales;
+ unsigned int range;
+};
+
/**
* struct ad7606_state - driver instance specific data
* @dev pointer to kernel device
* @chip_info entry in the table of chips that describes this device
* @bops bus operations (SPI or parallel)
- * @range voltage range selection, selects which scale to apply
+ * @chan_scales scale configuration for channels
* @oversampling oversampling selection
* @base_address address from where to read data in parallel operation
* @sw_mode_en software mode enabled
- * @scale_avail pointer to the array which stores the available scales
- * @num_scales number of elements stored in the scale_avail array
* @oversampling_avail pointer to the array which stores the available
* oversampling ratios.
* @num_os_ratios number of elements stored in oversampling_avail array
@@ -100,12 +112,10 @@ struct ad7606_state {
struct device *dev;
const struct ad7606_chip_info *chip_info;
const struct ad7606_bus_ops *bops;
- unsigned int range[16];
+ struct ad7606_chan_scale chan_scales[AD760X_MAX_CHANNELS];
unsigned int oversampling;
void __iomem *base_address;
bool sw_mode_en;
- const unsigned int *scale_avail;
- unsigned int num_scales;
const unsigned int *oversampling_avail;
unsigned int num_os_ratios;
int (*write_scale)(struct iio_dev *indio_dev, int ch, int val);
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v7 5/8] iio: adc: ad7606: rework available attributes for SW channels
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
` (3 preceding siblings ...)
2024-09-19 13:04 ` [PATCH v7 4/8] iio: adc: ad7606: wrap channel ranges & scales into struct Alexandru Ardelean
@ 2024-09-19 13:04 ` Alexandru Ardelean
2024-10-17 17:50 ` Jonathan Cameron
2024-09-19 13:04 ` [PATCH v7 6/8] dt-bindings: iio: adc: document diff-channels corner case for some ADCs Alexandru Ardelean
` (2 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean
For SW mode, the oversampling and scales attributes are always present.
So, they can be implemented via a 'read_avail' hook in iio_info.
For HW mode, it's a bit tricky, as these attributes get assigned based on
GPIO definitions.
So, for SW mode, we define a separate AD7606_SW_CHANNEL() macro, and use
that for the SW channels.
And 'ad7606_info_os_range_and_debug' can be renamed to
'ad7606_info_sw_mode' as it is only used for SW mode.
For the 'read_avail' hook, we'll need to allocate the SW scales, so that
they are just returned userspace without any extra processing.
The allocation will happen when then ad7606_state struct is allocated.
The oversampling available parameters don't need any extra processing; they
can just be passed back to userspace (as they are).
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
drivers/iio/adc/ad7606.c | 52 ++++++++++++++++++++++++++++++++++---
drivers/iio/adc/ad7606.h | 32 ++++++++++++++++++++---
drivers/iio/adc/ad7606_spi. | 0
3 files changed, 77 insertions(+), 7 deletions(-)
create mode 100644 drivers/iio/adc/ad7606_spi.
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 94a254c0725e..b909ee14fd81 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -512,6 +512,37 @@ static int ad7606_buffer_predisable(struct iio_dev *indio_dev)
return 0;
}
+static int ad7606_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long info)
+{
+ struct ad7606_state *st = iio_priv(indio_dev);
+ struct ad7606_chan_scale *cs;
+ unsigned int ch = 0;
+
+ switch (info) {
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ *vals = st->oversampling_avail;
+ *length = st->num_os_ratios;
+ *type = IIO_VAL_INT;
+
+ return IIO_AVAIL_LIST;
+
+ case IIO_CHAN_INFO_SCALE:
+ if (st->sw_mode_en)
+ ch = chan->address;
+
+ cs = &st->chan_scales[ch];
+ *vals = cs->scale_avail_show;
+ *length = cs->num_scales * 2;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+
+ return IIO_AVAIL_LIST;
+ }
+ return -EINVAL;
+}
+
static const struct iio_buffer_setup_ops ad7606_buffer_ops = {
.postenable = &ad7606_buffer_postenable,
.predisable = &ad7606_buffer_predisable,
@@ -529,11 +560,11 @@ static const struct iio_info ad7606_info_os_and_range = {
.validate_trigger = &ad7606_validate_trigger,
};
-static const struct iio_info ad7606_info_os_range_and_debug = {
+static const struct iio_info ad7606_info_sw_mode = {
.read_raw = &ad7606_read_raw,
.write_raw = &ad7606_write_raw,
+ .read_avail = &ad7606_read_avail,
.debugfs_reg_access = &ad7606_reg_access,
- .attrs = &ad7606_attribute_group_os_and_range,
.validate_trigger = &ad7606_validate_trigger,
};
@@ -564,7 +595,7 @@ static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
if (!st->sw_mode_en)
return 0;
- indio_dev->info = &ad7606_info_os_range_and_debug;
+ indio_dev->info = &ad7606_info_sw_mode;
return st->bops->sw_mode_config(indio_dev);
}
@@ -576,9 +607,24 @@ static int ad7606_chan_scales_setup(struct iio_dev *indio_dev)
int ch, ret;
for (ch = 0; ch < num_channels; ch++) {
+ struct ad7606_chan_scale *cs;
+ int i;
+
ret = st->chip_info->scale_setup_cb(st, ch);
if (ret)
return ret;
+
+ cs = &st->chan_scales[ch];
+
+ if (cs->num_scales * 2 > AD760X_MAX_SCALE_SHOW)
+ return dev_err_probe(st->dev, -ERANGE,
+ "Driver error: scale range too big");
+
+ /* Generate a scale_avail list for showing to userspace */
+ for (i = 0; i < cs->num_scales; i++) {
+ cs->scale_avail_show[i * 2] = 0;
+ cs->scale_avail_show[i * 2 + 1] = cs->scale_avail[i];
+ }
}
return 0;
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 2b90f52affba..25e84efd15c3 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -27,6 +27,29 @@
}, \
}
+#define AD7606_SW_CHANNEL(num, bits) { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = num, \
+ .address = num, \
+ .info_mask_separate = \
+ BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_separate_available = \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_all = \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .info_mask_shared_by_all_available = \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .scan_index = num, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = (bits), \
+ .storagebits = (bits) > 16 ? 32 : 16, \
+ .endianness = IIO_CPU, \
+ }, \
+}
+
#define AD7605_CHANNEL(num) \
AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
BIT(IIO_CHAN_INFO_SCALE), 0, 16)
@@ -36,10 +59,6 @@
BIT(IIO_CHAN_INFO_SCALE), \
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
-#define AD7606_SW_CHANNEL(num, bits) \
- AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
- 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
-
#define AD7616_CHANNEL(num) AD7606_SW_CHANNEL(num, 16)
struct ad7606_state;
@@ -71,11 +90,16 @@ struct ad7606_chip_info {
/**
* struct ad7606_chan_scale - channel scale configuration
* @scale_avail pointer to the array which stores the available scales
+ * @scale_avail_show a duplicate of 'scale_avail' which is readily formatted
+ * such that it can be read via the 'read_avail' hook
* @num_scales number of elements stored in the scale_avail array
* @range voltage range selection, selects which scale to apply
*/
struct ad7606_chan_scale {
+#define AD760X_MAX_SCALES 16
+#define AD760X_MAX_SCALE_SHOW (AD760X_MAX_SCALES * 2)
const unsigned int *scale_avail;
+ int scale_avail_show[AD760X_MAX_SCALE_SHOW];
unsigned int num_scales;
unsigned int range;
};
diff --git a/drivers/iio/adc/ad7606_spi. b/drivers/iio/adc/ad7606_spi.
new file mode 100644
index 000000000000..e69de29bb2d1
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v7 6/8] dt-bindings: iio: adc: document diff-channels corner case for some ADCs
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
` (4 preceding siblings ...)
2024-09-19 13:04 ` [PATCH v7 5/8] iio: adc: ad7606: rework available attributes for SW channels Alexandru Ardelean
@ 2024-09-19 13:04 ` Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 8/8] iio: adc: ad7606: add support " Alexandru Ardelean
7 siblings, 0 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean, Krzysztof Kozlowski
Some ADCs have channels with negative and positive inputs, which can be
used to measure differential voltage levels. These inputs/pins are
dedicated (to the given channel) and cannot be muxed as with other ADCs.
For those types of setups, the 'diff-channels' property can be specified to
be used with the channel number (or reg property) for both negative and
positive inputs/pins.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
Documentation/devicetree/bindings/iio/adc/adc.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/adc.yaml b/Documentation/devicetree/bindings/iio/adc/adc.yaml
index 8e7835cf36fd..b9bc02b5b07a 100644
--- a/Documentation/devicetree/bindings/iio/adc/adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adc.yaml
@@ -37,6 +37,10 @@ properties:
to both the positive and negative inputs of a differential ADC.
The first value specifies the positive input pin, the second
specifies the negative input pin.
+ There are also some ADCs, where the differential channel has dedicated
+ positive and negative inputs which can be used to measure differential
+ voltage levels. For those setups, this property can be configured with
+ the 'reg' property for both inputs (i.e. diff-channels = <reg reg>).
single-channel:
$ref: /schemas/types.yaml#/definitions/uint32
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v7 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
` (5 preceding siblings ...)
2024-09-19 13:04 ` [PATCH v7 6/8] dt-bindings: iio: adc: document diff-channels corner case for some ADCs Alexandru Ardelean
@ 2024-09-19 13:04 ` Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 8/8] iio: adc: ad7606: add support " Alexandru Ardelean
7 siblings, 0 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean, Krzysztof Kozlowski
The driver will support the AD7606C-16 and AD7606C-18.
This change adds the compatible strings for these devices.
The AD7606C-16,18 channels also support these (individually configurable)
types of channels:
- bipolar single-ended
- unipolar single-ended
- bipolar differential
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
.../bindings/iio/adc/adi,ad7606.yaml | 120 ++++++++++++++++++
1 file changed, 120 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 69408cae3db9..bec7cfba52a7 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -14,6 +14,8 @@ description: |
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf
properties:
@@ -24,11 +26,19 @@ properties:
- adi,ad7606-6
- adi,ad7606-8 # Referred to as AD7606 (without -8) in the datasheet
- adi,ad7606b
+ - adi,ad7606c-16
+ - adi,ad7606c-18
- adi,ad7616
reg:
maxItems: 1
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
spi-cpha: true
spi-cpol: true
@@ -114,6 +124,47 @@ properties:
assumed that the pins are hardwired to VDD.
type: boolean
+patternProperties:
+ "^channel@[1-8]$":
+ type: object
+ $ref: adc.yaml
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description:
+ The channel number, as specified in the datasheet (from 1 to 8).
+ minimum: 1
+ maximum: 8
+
+ diff-channels:
+ description:
+ Each channel can be configured as a bipolar differential channel.
+ The ADC uses the same positive and negative inputs for this.
+ This property must be specified as 'reg' (or the channel number) for
+ both positive and negative inputs (i.e. diff-channels = <reg reg>).
+ Since the configuration is bipolar differential, the 'bipolar'
+ property is required.
+ items:
+ minimum: 1
+ maximum: 8
+
+ bipolar:
+ description:
+ The ADC channels can be configured as
+ * Bipolar single-ended
+ * Unipolar single-ended
+ * Bipolar differential
+ Therefore in the DT, if no channel node is specified, it is considered
+ 'unipolar single-ended'. So for the other configurations the 'bipolar'
+ property must be specified. If 'diff-channels' is specified, it is
+ considered a bipolar differential channel. Otherwise it is bipolar
+ single-ended.
+
+ required:
+ - reg
+ - bipolar
+
required:
- compatible
- reg
@@ -170,6 +221,25 @@ allOf:
adi,conversion-start-gpios:
maxItems: 1
+ - if:
+ not:
+ required:
+ - adi,sw-mode
+ then:
+ patternProperties:
+ "^channel@[1-8]$": false
+
+ - if:
+ not:
+ properties:
+ compatible:
+ enum:
+ - adi,ad7606c-16
+ - adi,ad7606c-18
+ then:
+ patternProperties:
+ "^channel@[1-8]$": false
+
unevaluatedProperties: false
examples:
@@ -202,4 +272,54 @@ examples:
standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
};
};
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@0 {
+ compatible = "adi,ad7606c-18";
+ reg = <0>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ spi-max-frequency = <1000000>;
+ spi-cpol;
+ spi-cpha;
+
+ avcc-supply = <&adc_vref>;
+ vdrive-supply = <&vdd_supply>;
+
+ interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-parent = <&gpio>;
+
+ adi,conversion-start-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>;
+ reset-gpios = <&gpio 27 GPIO_ACTIVE_HIGH>;
+ adi,first-data-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>;
+ standby-gpios = <&gpio 24 GPIO_ACTIVE_LOW>;
+
+ adi,sw-mode;
+
+ channel@1 {
+ reg = <1>;
+ diff-channels = <1 1>;
+ bipolar;
+ };
+
+ channel@3 {
+ reg = <3>;
+ bipolar;
+ };
+
+ channel@8 {
+ reg = <8>;
+ diff-channels = <8 8>;
+ bipolar;
+ };
+
+ };
+ };
...
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
` (6 preceding siblings ...)
2024-09-19 13:04 ` [PATCH v7 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts Alexandru Ardelean
@ 2024-09-19 13:04 ` Alexandru Ardelean
2024-10-01 8:26 ` Alexandru Ardelean
7 siblings, 1 reply; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-19 13:04 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner,
Alexandru Ardelean
The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
The main difference between AD7606C-16 & AD7606C-18 is the precision in
bits (16 vs 18).
Because of that, some scales need to be defined for the 18-bit variants, as
they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
Because the AD7606C-16,18 also supports bipolar & differential channels,
for SW-mode, the default range of 10 V or ±10V should be set at probe.
On reset, the default range (in the registers) is set to value 0x3 which
corresponds to '±10 V single-ended range', regardless of bipolar or
differential configuration.
Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
The AD7606C-18 variant offers 18-bit precision. Because of this, the
requirement to use this chip is that the SPI controller supports padding
of 18-bit sequences to 32-bit arrays.
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
drivers/iio/adc/ad7606.c | 263 +++++++++++++++++++++++++++++++++--
drivers/iio/adc/ad7606.h | 16 ++-
drivers/iio/adc/ad7606_spi.c | 55 ++++++++
3 files changed, 322 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index b909ee14fd81..f04e5660d2f8 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -36,6 +36,33 @@ static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
152588, 305176
};
+static const unsigned int ad7606_18bit_hw_scale_avail[2] = {
+ 38147, 76294
+};
+
+static const unsigned int ad7606c_16bit_single_ended_unipolar_scale_avail[3] = {
+ 76294, 152588, 190735,
+};
+
+static const unsigned int ad7606c_16bit_single_ended_bipolar_scale_avail[5] = {
+ 76294, 152588, 190735, 305176, 381470
+};
+
+static const unsigned int ad7606c_16bit_differential_bipolar_scale_avail[4] = {
+ 152588, 305176, 381470, 610352
+};
+
+static const unsigned int ad7606c_18bit_single_ended_unipolar_scale_avail[3] = {
+ 19073, 38147, 47684
+};
+
+static const unsigned int ad7606c_18bit_single_ended_bipolar_scale_avail[5] = {
+ 19073, 38147, 47684, 76294, 95367
+};
+
+static const unsigned int ad7606c_18bit_differential_bipolar_scale_avail[4] = {
+ 38147, 76294, 95367, 152588
+};
static const unsigned int ad7606_16bit_sw_scale_avail[3] = {
76293, 152588, 305176
@@ -62,7 +89,8 @@ int ad7606_reset(struct ad7606_state *st)
}
EXPORT_SYMBOL_NS_GPL(ad7606_reset, IIO_AD7606);
-static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st, int ch)
+static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st,
+ struct iio_chan_spec *chan, int ch)
{
struct ad7606_chan_scale *cs = &st->chan_scales[ch];
@@ -83,6 +111,173 @@ static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st, int ch)
return 0;
}
+static int ad7606_get_chan_config(struct ad7606_state *st, int ch,
+ bool *bipolar, bool *differential)
+{
+ unsigned int num_channels = st->chip_info->num_channels - 1;
+ struct device *dev = st->dev;
+ int ret;
+
+ *bipolar = false;
+ *differential = false;
+
+ device_for_each_child_node_scoped(dev, child) {
+ u32 pins[2];
+ int reg;
+
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret)
+ continue;
+
+ /* channel number (here) is from 1 to num_channels */
+ if (reg == 0 || reg > num_channels) {
+ dev_warn(dev,
+ "Invalid channel number (ignoring): %d\n", reg);
+ continue;
+ }
+
+ if (reg != (ch + 1))
+ continue;
+
+ *bipolar = fwnode_property_read_bool(child, "bipolar");
+
+ ret = fwnode_property_read_u32_array(child, "diff-channels",
+ pins, ARRAY_SIZE(pins));
+ /* Channel is differential, if pins are the same as 'reg' */
+ if (ret == 0 && (pins[0] != reg || pins[1] != reg)) {
+ dev_err(dev,
+ "Differential pins must be the same as 'reg'");
+ return -EINVAL;
+ }
+
+ *differential = (ret == 0);
+
+ if (*differential && !*bipolar) {
+ dev_err(dev,
+ "'bipolar' must be added for diff channel %d\n",
+ reg);
+ return -EINVAL;
+ }
+
+ return 0;
+ }
+
+ return 0;
+}
+
+static int ad7606c_18bit_chan_scale_setup(struct ad7606_state *st,
+ struct iio_chan_spec *chan, int ch)
+{
+ struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+ bool bipolar, differential;
+ int ret;
+
+ if (!st->sw_mode_en) {
+ cs->range = 0;
+ cs->scale_avail = ad7606_18bit_hw_scale_avail;
+ cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
+ return 0;
+ }
+
+ ret = ad7606_get_chan_config(st, ch, &bipolar, &differential);
+ if (ret)
+ return ret;
+
+ if (differential) {
+ cs->scale_avail = ad7606c_18bit_differential_bipolar_scale_avail;
+ cs->num_scales =
+ ARRAY_SIZE(ad7606c_18bit_differential_bipolar_scale_avail);
+ /* Bipolar differential ranges start at 8 (b1000) */
+ cs->reg_offset = 8;
+ cs->range = 1;
+ chan->differential = 1;
+ chan->channel2 = chan->channel;
+
+ return 0;
+ }
+
+ chan->differential = 0;
+
+ if (bipolar) {
+ cs->scale_avail = ad7606c_18bit_single_ended_bipolar_scale_avail;
+ cs->num_scales =
+ ARRAY_SIZE(ad7606c_18bit_single_ended_bipolar_scale_avail);
+ /* Bipolar single-ended ranges start at 0 (b0000) */
+ cs->reg_offset = 0;
+ cs->range = 3;
+ chan->scan_type.sign = 's';
+
+ return 0;
+ }
+
+ cs->scale_avail = ad7606c_18bit_single_ended_unipolar_scale_avail;
+ cs->num_scales =
+ ARRAY_SIZE(ad7606c_18bit_single_ended_unipolar_scale_avail);
+ /* Unipolar single-ended ranges start at 5 (b0101) */
+ cs->reg_offset = 5;
+ cs->range = 1;
+ chan->scan_type.sign = 'u';
+
+ return 0;
+}
+
+static int ad7606c_16bit_chan_scale_setup(struct ad7606_state *st,
+ struct iio_chan_spec *chan, int ch)
+{
+ struct ad7606_chan_scale *cs = &st->chan_scales[ch];
+ bool bipolar, differential;
+ int ret;
+
+ if (!st->sw_mode_en) {
+ cs->range = 0;
+ cs->scale_avail = ad7606_16bit_hw_scale_avail;
+ cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
+ return 0;
+ }
+
+ ret = ad7606_get_chan_config(st, ch, &bipolar, &differential);
+ if (ret)
+ return ret;
+
+ if (differential) {
+ cs->scale_avail = ad7606c_16bit_differential_bipolar_scale_avail;
+ cs->num_scales =
+ ARRAY_SIZE(ad7606c_16bit_differential_bipolar_scale_avail);
+ /* Bipolar differential ranges start at 8 (b1000) */
+ cs->reg_offset = 8;
+ cs->range = 1;
+ chan->differential = 1;
+ chan->channel2 = chan->channel;
+ chan->scan_type.sign = 's';
+
+ return 0;
+ }
+
+ chan->differential = 0;
+
+ if (bipolar) {
+ cs->scale_avail = ad7606c_16bit_single_ended_bipolar_scale_avail;
+ cs->num_scales =
+ ARRAY_SIZE(ad7606c_16bit_single_ended_bipolar_scale_avail);
+ /* Bipolar single-ended ranges start at 0 (b0000) */
+ cs->reg_offset = 0;
+ cs->range = 3;
+ chan->scan_type.sign = 's';
+
+ return 0;
+ }
+
+ cs->scale_avail = ad7606c_16bit_single_ended_unipolar_scale_avail;
+ cs->num_scales =
+ ARRAY_SIZE(ad7606c_16bit_single_ended_unipolar_scale_avail);
+ /* Unipolar single-ended ranges start at 5 (b0101) */
+ cs->reg_offset = 5;
+ cs->range = 1;
+ chan->scan_type.sign = 'u';
+
+ return 0;
+}
+
static int ad7606_reg_access(struct iio_dev *indio_dev,
unsigned int reg,
unsigned int writeval,
@@ -107,9 +302,8 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
static int ad7606_read_samples(struct ad7606_state *st)
{
unsigned int num = st->chip_info->num_channels - 1;
- u16 *data = st->data;
- return st->bops->read_block(st->dev, num, data);
+ return st->bops->read_block(st->dev, num, &st->data);
}
static irqreturn_t ad7606_trigger_handler(int irq, void *p)
@@ -125,7 +319,7 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
if (ret)
goto error_ret;
- iio_push_to_buffers_with_timestamp(indio_dev, st->data,
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->data,
iio_get_time_ns(indio_dev));
error_ret:
iio_trigger_notify_done(indio_dev->trig);
@@ -139,6 +333,8 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
int *val)
{
struct ad7606_state *st = iio_priv(indio_dev);
+ unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
+ const struct iio_chan_spec *chan;
int ret;
gpiod_set_value(st->gpio_convst, 1);
@@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
if (ret)
goto error_ret;
- *val = sign_extend32(st->data[ch], 15);
+ chan = &indio_dev->channels[ch + 1];
+ if (chan->scan_type.sign == 'u') {
+ if (storagebits > 16)
+ *val = st->data.buf32[ch];
+ else
+ *val = st->data.buf16[ch];
+ return 0;
+ }
+
+ if (storagebits > 16)
+ *val = sign_extend32(st->data.buf32[ch], 17);
+ else
+ *val = sign_extend32(st->data.buf16[ch], 15);
error_ret:
gpiod_set_value(st->gpio_convst, 0);
@@ -266,7 +474,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
ch = chan->address;
cs = &st->chan_scales[ch];
i = find_closest(val2, cs->scale_avail, cs->num_scales);
- ret = st->write_scale(indio_dev, ch, i);
+ ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
if (ret < 0)
return ret;
cs->range = i;
@@ -349,6 +557,18 @@ static const struct iio_chan_spec ad7606_channels_16bit[] = {
AD7606_CHANNEL(7, 16),
};
+static const struct iio_chan_spec ad7606_channels_18bit[] = {
+ IIO_CHAN_SOFT_TIMESTAMP(8),
+ AD7606_CHANNEL(0, 18),
+ AD7606_CHANNEL(1, 18),
+ AD7606_CHANNEL(2, 18),
+ AD7606_CHANNEL(3, 18),
+ AD7606_CHANNEL(4, 18),
+ AD7606_CHANNEL(5, 18),
+ AD7606_CHANNEL(6, 18),
+ AD7606_CHANNEL(7, 18),
+};
+
/*
* The current assumption that this driver makes for AD7616, is that it's
* working in Hardware Mode with Serial, Burst and Sequencer modes activated.
@@ -414,6 +634,20 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
.oversampling_avail = ad7606_oversampling_avail,
.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
},
+ [ID_AD7606C_16] = {
+ .channels = ad7606_channels_16bit,
+ .num_channels = 9,
+ .scale_setup_cb = ad7606c_16bit_chan_scale_setup,
+ .oversampling_avail = ad7606_oversampling_avail,
+ .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+ },
+ [ID_AD7606C_18] = {
+ .channels = ad7606_channels_18bit,
+ .num_channels = 9,
+ .scale_setup_cb = ad7606c_18bit_chan_scale_setup,
+ .oversampling_avail = ad7606_oversampling_avail,
+ .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+ },
[ID_AD7616] = {
.channels = ad7616_channels,
.num_channels = 17,
@@ -586,7 +820,7 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
.validate_device = iio_trigger_validate_own_device,
};
-static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
+static int ad7606_sw_mode_setup(struct iio_dev *indio_dev, unsigned int id)
{
struct ad7606_state *st = iio_priv(indio_dev);
@@ -604,13 +838,24 @@ static int ad7606_chan_scales_setup(struct iio_dev *indio_dev)
{
unsigned int num_channels = indio_dev->num_channels - 1;
struct ad7606_state *st = iio_priv(indio_dev);
+ struct iio_chan_spec *chans;
+ size_t size;
int ch, ret;
+ /* Clone IIO channels, since some may be differential */
+ size = indio_dev->num_channels * sizeof(*indio_dev->channels);
+ chans = devm_kzalloc(st->dev, size, GFP_KERNEL);
+ if (!chans)
+ return -ENOMEM;
+
+ memcpy(chans, indio_dev->channels, size);
+ indio_dev->channels = chans;
+
for (ch = 0; ch < num_channels; ch++) {
struct ad7606_chan_scale *cs;
int i;
- ret = st->chip_info->scale_setup_cb(st, ch);
+ ret = st->chip_info->scale_setup_cb(st, &chans[ch + 1], ch);
if (ret)
return ret;
@@ -698,7 +943,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
st->write_scale = ad7606_write_scale_hw;
st->write_os = ad7606_write_os_hw;
- ret = ad7606_sw_mode_setup(indio_dev);
+ ret = ad7606_sw_mode_setup(indio_dev, id);
if (ret)
return ret;
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 25e84efd15c3..14ee75aa225b 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -63,7 +63,8 @@
struct ad7606_state;
-typedef int (*ad7606_scale_setup_cb_t)(struct ad7606_state *st, int ch);
+typedef int (*ad7606_scale_setup_cb_t)(struct ad7606_state *st,
+ struct iio_chan_spec *chan, int ch);
/**
* struct ad7606_chip_info - chip specific information
@@ -94,6 +95,8 @@ struct ad7606_chip_info {
* such that it can be read via the 'read_avail' hook
* @num_scales number of elements stored in the scale_avail array
* @range voltage range selection, selects which scale to apply
+ * @reg_offset offset for the register value, to be applied when
+ * writing the value of 'range' to the register value
*/
struct ad7606_chan_scale {
#define AD760X_MAX_SCALES 16
@@ -102,6 +105,7 @@ struct ad7606_chan_scale {
int scale_avail_show[AD760X_MAX_SCALE_SHOW];
unsigned int num_scales;
unsigned int range;
+ unsigned int reg_offset;
};
/**
@@ -158,9 +162,13 @@ struct ad7606_state {
/*
* DMA (thus cache coherency maintenance) may require the
* transfer buffers to live in their own cache lines.
- * 16 * 16-bit samples + 64-bit timestamp
+ * 16 * 16-bit samples + 64-bit timestamp - for AD7616
+ * 8 * 32-bit samples + 64-bit timestamp - for AD7616C-18 (and similar)
*/
- unsigned short data[20] __aligned(IIO_DMA_MINALIGN);
+ union {
+ u16 buf16[20];
+ u32 buf32[10];
+ } data __aligned(IIO_DMA_MINALIGN);
__be16 d16[2];
};
@@ -201,6 +209,8 @@ enum ad7606_supported_device_ids {
ID_AD7606_6,
ID_AD7606_4,
ID_AD7606B,
+ ID_AD7606C_16,
+ ID_AD7606C_18,
ID_AD7616,
};
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index e00f58a6a0e9..143440e73aab 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -77,6 +77,18 @@ static const struct iio_chan_spec ad7606b_sw_channels[] = {
AD7606_SW_CHANNEL(7, 16),
};
+static const struct iio_chan_spec ad7606c_18_sw_channels[] = {
+ IIO_CHAN_SOFT_TIMESTAMP(8),
+ AD7606_SW_CHANNEL(0, 18),
+ AD7606_SW_CHANNEL(1, 18),
+ AD7606_SW_CHANNEL(2, 18),
+ AD7606_SW_CHANNEL(3, 18),
+ AD7606_SW_CHANNEL(4, 18),
+ AD7606_SW_CHANNEL(5, 18),
+ AD7606_SW_CHANNEL(6, 18),
+ AD7606_SW_CHANNEL(7, 18),
+};
+
static const unsigned int ad7606B_oversampling_avail[9] = {
1, 2, 4, 8, 16, 32, 64, 128, 256
};
@@ -120,6 +132,19 @@ static int ad7606_spi_read_block(struct device *dev,
return 0;
}
+static int ad7606_spi_read_block18to32(struct device *dev,
+ int count, void *buf)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct spi_transfer xfer = {
+ .bits_per_word = 18,
+ .len = count * sizeof(u32),
+ .rx_buf = buf,
+ };
+
+ return spi_sync_transfer(spi, &xfer, 1);
+}
+
static int ad7606_spi_reg_read(struct ad7606_state *st, unsigned int addr)
{
struct spi_device *spi = to_spi_device(st->dev);
@@ -283,6 +308,19 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
return 0;
}
+static int ad7606c_18_sw_mode_config(struct iio_dev *indio_dev)
+{
+ int ret;
+
+ ret = ad7606B_sw_mode_config(indio_dev);
+ if (ret)
+ return ret;
+
+ indio_dev->channels = ad7606c_18_sw_channels;
+
+ return 0;
+}
+
static const struct ad7606_bus_ops ad7606_spi_bops = {
.read_block = ad7606_spi_read_block,
};
@@ -305,6 +343,15 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
.sw_mode_config = ad7606B_sw_mode_config,
};
+static const struct ad7606_bus_ops ad7606c_18_spi_bops = {
+ .read_block = ad7606_spi_read_block18to32,
+ .reg_read = ad7606_spi_reg_read,
+ .reg_write = ad7606_spi_reg_write,
+ .write_mask = ad7606_spi_write_mask,
+ .rd_wr_cmd = ad7606B_spi_rd_wr_cmd,
+ .sw_mode_config = ad7606c_18_sw_mode_config,
+};
+
static int ad7606_spi_probe(struct spi_device *spi)
{
const struct spi_device_id *id = spi_get_device_id(spi);
@@ -315,8 +362,12 @@ static int ad7606_spi_probe(struct spi_device *spi)
bops = &ad7616_spi_bops;
break;
case ID_AD7606B:
+ case ID_AD7606C_16:
bops = &ad7606B_spi_bops;
break;
+ case ID_AD7606C_18:
+ bops = &ad7606c_18_spi_bops;
+ break;
default:
bops = &ad7606_spi_bops;
break;
@@ -333,6 +384,8 @@ static const struct spi_device_id ad7606_id_table[] = {
{ "ad7606-6", ID_AD7606_6 },
{ "ad7606-8", ID_AD7606_8 },
{ "ad7606b", ID_AD7606B },
+ { "ad7606c-16", ID_AD7606C_16 },
+ { "ad7606c-18", ID_AD7606C_18 },
{ "ad7616", ID_AD7616 },
{ }
};
@@ -344,6 +397,8 @@ static const struct of_device_id ad7606_of_match[] = {
{ .compatible = "adi,ad7606-6" },
{ .compatible = "adi,ad7606-8" },
{ .compatible = "adi,ad7606b" },
+ { .compatible = "adi,ad7606c-16" },
+ { .compatible = "adi,ad7606c-18" },
{ .compatible = "adi,ad7616" },
{ }
};
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros
2024-09-19 13:04 ` [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros Alexandru Ardelean
@ 2024-09-23 14:50 ` David Lechner
2024-09-24 8:11 ` Alexandru Ardelean
0 siblings, 1 reply; 22+ messages in thread
From: David Lechner @ 2024-09-23 14:50 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-iio, linux-kernel, devicetree, jic23, krzk+dt, robh, lars,
michael.hennerich, gstols
On Thu, Sep 19, 2024 at 3:04 PM Alexandru Ardelean
<aardelean@baylibre.com> wrote:
>
> There are some newer additions to the AD7606 family, which support 18 bit
> precision. Up until now, all chips were 16 bit.
>
> This change adds a 'bits' parameter to the AD760X_CHANNEL macro and renames
> 'ad7606_channels' -> 'ad7606_channels_16bit' for the current devices.
>
> The AD7606_SW_CHANNEL() macro is also introduced, as a short-hand for IIO
> channels in SW mode.
>
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
...
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index 6649e84d25de..204a343067e5 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -8,7 +8,7 @@
> #ifndef IIO_ADC_AD7606_H_
> #define IIO_ADC_AD7606_H_
>
> -#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all) { \
> +#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) { \
> .type = IIO_VOLTAGE, \
> .indexed = 1, \
> .channel = num, \
> @@ -19,24 +19,26 @@
> .scan_index = num, \
> .scan_type = { \
> .sign = 's', \
> - .realbits = 16, \
> - .storagebits = 16, \
> + .realbits = (bits), \
> + .storagebits = (bits) > 16 ? 32 : 16, \
> .endianness = IIO_CPU, \
> }, \
> }
>
> #define AD7605_CHANNEL(num) \
> AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
> - BIT(IIO_CHAN_INFO_SCALE), 0)
> + BIT(IIO_CHAN_INFO_SCALE), 0, 16)
>
> -#define AD7606_CHANNEL(num) \
> +#define AD7606_CHANNEL(num, bits) \
> AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
> BIT(IIO_CHAN_INFO_SCALE), \
> - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
>
> -#define AD7616_CHANNEL(num) \
> +#define AD7606_SW_CHANNEL(num, bits) \
> AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
> - 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> + 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
> +
> +#define AD7616_CHANNEL(num) AD7606_SW_CHANNEL(num, 16)
It looks like the AD7616_CHANNEL macro is no longer used, so can be
dropped. Or alternately, don't change the lines below to use
AD7606_SW_CHANNEL.
With either of those changes:
Reviewed-by: David Lechner <dlechner@baylibre.com>
>
> /**
> * struct ad7606_chip_info - chip specific information
> diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> index 62ec12195307..e00f58a6a0e9 100644
> --- a/drivers/iio/adc/ad7606_spi.c
> +++ b/drivers/iio/adc/ad7606_spi.c
> @@ -67,14 +67,14 @@ static const struct iio_chan_spec ad7616_sw_channels[] = {
>
> static const struct iio_chan_spec ad7606b_sw_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(8),
> - AD7616_CHANNEL(0),
> - AD7616_CHANNEL(1),
> - AD7616_CHANNEL(2),
> - AD7616_CHANNEL(3),
> - AD7616_CHANNEL(4),
> - AD7616_CHANNEL(5),
> - AD7616_CHANNEL(6),
> - AD7616_CHANNEL(7),
> + AD7606_SW_CHANNEL(0, 16),
> + AD7606_SW_CHANNEL(1, 16),
> + AD7606_SW_CHANNEL(2, 16),
> + AD7606_SW_CHANNEL(3, 16),
> + AD7606_SW_CHANNEL(4, 16),
> + AD7606_SW_CHANNEL(5, 16),
> + AD7606_SW_CHANNEL(6, 16),
> + AD7606_SW_CHANNEL(7, 16),
> };
>
> static const unsigned int ad7606B_oversampling_avail[9] = {
> --
> 2.46.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros
2024-09-23 14:50 ` David Lechner
@ 2024-09-24 8:11 ` Alexandru Ardelean
2024-09-28 14:43 ` Jonathan Cameron
0 siblings, 1 reply; 22+ messages in thread
From: Alexandru Ardelean @ 2024-09-24 8:11 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-kernel, devicetree, jic23, krzk+dt, robh, lars,
michael.hennerich, gstols
On Mon, Sep 23, 2024 at 4:51 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On Thu, Sep 19, 2024 at 3:04 PM Alexandru Ardelean
> <aardelean@baylibre.com> wrote:
> >
> > There are some newer additions to the AD7606 family, which support 18 bit
> > precision. Up until now, all chips were 16 bit.
> >
> > This change adds a 'bits' parameter to the AD760X_CHANNEL macro and renames
> > 'ad7606_channels' -> 'ad7606_channels_16bit' for the current devices.
> >
> > The AD7606_SW_CHANNEL() macro is also introduced, as a short-hand for IIO
> > channels in SW mode.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
>
> ...
>
> > diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> > index 6649e84d25de..204a343067e5 100644
> > --- a/drivers/iio/adc/ad7606.h
> > +++ b/drivers/iio/adc/ad7606.h
> > @@ -8,7 +8,7 @@
> > #ifndef IIO_ADC_AD7606_H_
> > #define IIO_ADC_AD7606_H_
> >
> > -#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all) { \
> > +#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) { \
> > .type = IIO_VOLTAGE, \
> > .indexed = 1, \
> > .channel = num, \
> > @@ -19,24 +19,26 @@
> > .scan_index = num, \
> > .scan_type = { \
> > .sign = 's', \
> > - .realbits = 16, \
> > - .storagebits = 16, \
> > + .realbits = (bits), \
> > + .storagebits = (bits) > 16 ? 32 : 16, \
> > .endianness = IIO_CPU, \
> > }, \
> > }
> >
> > #define AD7605_CHANNEL(num) \
> > AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
> > - BIT(IIO_CHAN_INFO_SCALE), 0)
> > + BIT(IIO_CHAN_INFO_SCALE), 0, 16)
> >
> > -#define AD7606_CHANNEL(num) \
> > +#define AD7606_CHANNEL(num, bits) \
> > AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
> > BIT(IIO_CHAN_INFO_SCALE), \
> > - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
> >
> > -#define AD7616_CHANNEL(num) \
> > +#define AD7606_SW_CHANNEL(num, bits) \
> > AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
> > - 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> > + 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
> > +
> > +#define AD7616_CHANNEL(num) AD7606_SW_CHANNEL(num, 16)
>
> It looks like the AD7616_CHANNEL macro is no longer used, so can be
> dropped. Or alternately, don't change the lines below to use
> AD7606_SW_CHANNEL.
Well, the AD7616_CHANNEL() macro is still being used for the actual
AD7616 channels.
For the AD7606B software channels, the AD7616_CHANNEL() macro was
being re-used, which seemed like a bit of a lazy/convenient way to do
it.
The patch here, just cleans up that minor quirk, but just for AD7606B.
The AD7616 driver part, still uses the AD7616_CHANNEL() macro.
Though, interestingly, the AD7616_CHANNEL() macro is only used for SW channels.
Maybe in a subsequent patch, the AD7616_CHANNEL() macro can be removed
altogether.
I thought about doing it in this series, but decided against it, to
keep the series small.
>
> With either of those changes:
>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
>
> >
> > /**
> > * struct ad7606_chip_info - chip specific information
> > diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> > index 62ec12195307..e00f58a6a0e9 100644
> > --- a/drivers/iio/adc/ad7606_spi.c
> > +++ b/drivers/iio/adc/ad7606_spi.c
> > @@ -67,14 +67,14 @@ static const struct iio_chan_spec ad7616_sw_channels[] = {
> >
> > static const struct iio_chan_spec ad7606b_sw_channels[] = {
> > IIO_CHAN_SOFT_TIMESTAMP(8),
> > - AD7616_CHANNEL(0),
> > - AD7616_CHANNEL(1),
> > - AD7616_CHANNEL(2),
> > - AD7616_CHANNEL(3),
> > - AD7616_CHANNEL(4),
> > - AD7616_CHANNEL(5),
> > - AD7616_CHANNEL(6),
> > - AD7616_CHANNEL(7),
> > + AD7606_SW_CHANNEL(0, 16),
> > + AD7606_SW_CHANNEL(1, 16),
> > + AD7606_SW_CHANNEL(2, 16),
> > + AD7606_SW_CHANNEL(3, 16),
> > + AD7606_SW_CHANNEL(4, 16),
> > + AD7606_SW_CHANNEL(5, 16),
> > + AD7606_SW_CHANNEL(6, 16),
> > + AD7606_SW_CHANNEL(7, 16),
> > };
> >
> > static const unsigned int ad7606B_oversampling_avail[9] = {
> > --
> > 2.46.0
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros
2024-09-24 8:11 ` Alexandru Ardelean
@ 2024-09-28 14:43 ` Jonathan Cameron
0 siblings, 0 replies; 22+ messages in thread
From: Jonathan Cameron @ 2024-09-28 14:43 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: David Lechner, linux-iio, linux-kernel, devicetree, krzk+dt, robh,
lars, michael.hennerich, gstols
On Tue, 24 Sep 2024 10:11:38 +0200
Alexandru Ardelean <aardelean@baylibre.com> wrote:
> On Mon, Sep 23, 2024 at 4:51 PM David Lechner <dlechner@baylibre.com> wrote:
> >
> > On Thu, Sep 19, 2024 at 3:04 PM Alexandru Ardelean
> > <aardelean@baylibre.com> wrote:
> > >
> > > There are some newer additions to the AD7606 family, which support 18 bit
> > > precision. Up until now, all chips were 16 bit.
> > >
> > > This change adds a 'bits' parameter to the AD760X_CHANNEL macro and renames
> > > 'ad7606_channels' -> 'ad7606_channels_16bit' for the current devices.
> > >
> > > The AD7606_SW_CHANNEL() macro is also introduced, as a short-hand for IIO
> > > channels in SW mode.
> > >
> > > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > > ---
> >
> > ...
> >
> > > diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> > > index 6649e84d25de..204a343067e5 100644
> > > --- a/drivers/iio/adc/ad7606.h
> > > +++ b/drivers/iio/adc/ad7606.h
> > > @@ -8,7 +8,7 @@
> > > #ifndef IIO_ADC_AD7606_H_
> > > #define IIO_ADC_AD7606_H_
> > >
> > > -#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all) { \
> > > +#define AD760X_CHANNEL(num, mask_sep, mask_type, mask_all, bits) { \
> > > .type = IIO_VOLTAGE, \
> > > .indexed = 1, \
> > > .channel = num, \
> > > @@ -19,24 +19,26 @@
> > > .scan_index = num, \
> > > .scan_type = { \
> > > .sign = 's', \
> > > - .realbits = 16, \
> > > - .storagebits = 16, \
> > > + .realbits = (bits), \
> > > + .storagebits = (bits) > 16 ? 32 : 16, \
> > > .endianness = IIO_CPU, \
> > > }, \
> > > }
> > >
> > > #define AD7605_CHANNEL(num) \
> > > AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
> > > - BIT(IIO_CHAN_INFO_SCALE), 0)
> > > + BIT(IIO_CHAN_INFO_SCALE), 0, 16)
> > >
> > > -#define AD7606_CHANNEL(num) \
> > > +#define AD7606_CHANNEL(num, bits) \
> > > AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW), \
> > > BIT(IIO_CHAN_INFO_SCALE), \
> > > - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> > > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
> > >
> > > -#define AD7616_CHANNEL(num) \
> > > +#define AD7606_SW_CHANNEL(num, bits) \
> > > AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
> > > - 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> > > + 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), bits)
> > > +
> > > +#define AD7616_CHANNEL(num) AD7606_SW_CHANNEL(num, 16)
> >
> > It looks like the AD7616_CHANNEL macro is no longer used, so can be
> > dropped. Or alternately, don't change the lines below to use
> > AD7606_SW_CHANNEL.
>
> Well, the AD7616_CHANNEL() macro is still being used for the actual
> AD7616 channels.
> For the AD7606B software channels, the AD7616_CHANNEL() macro was
> being re-used, which seemed like a bit of a lazy/convenient way to do
> it.
>
> The patch here, just cleans up that minor quirk, but just for AD7606B.
> The AD7616 driver part, still uses the AD7616_CHANNEL() macro.
>
> Though, interestingly, the AD7616_CHANNEL() macro is only used for SW channels.
> Maybe in a subsequent patch, the AD7616_CHANNEL() macro can be removed
> altogether.
> I thought about doing it in this series, but decided against it, to
> keep the series small.
It's been a while, and David hasn't said he disagrees with this
argument so I've picked up David's tag whilst applying this.
So David, if you'd rather I didn't then shout!
Series applied to the togreg branch of iio.git and pushed out as testing
for all the normal reasons + I'm waiting for rc1 to rebase on.
Thanks
Jonathan
>
> >
> > With either of those changes:
> >
> > Reviewed-by: David Lechner <dlechner@baylibre.com>
> >
> > >
> > > /**
> > > * struct ad7606_chip_info - chip specific information
> > > diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> > > index 62ec12195307..e00f58a6a0e9 100644
> > > --- a/drivers/iio/adc/ad7606_spi.c
> > > +++ b/drivers/iio/adc/ad7606_spi.c
> > > @@ -67,14 +67,14 @@ static const struct iio_chan_spec ad7616_sw_channels[] = {
> > >
> > > static const struct iio_chan_spec ad7606b_sw_channels[] = {
> > > IIO_CHAN_SOFT_TIMESTAMP(8),
> > > - AD7616_CHANNEL(0),
> > > - AD7616_CHANNEL(1),
> > > - AD7616_CHANNEL(2),
> > > - AD7616_CHANNEL(3),
> > > - AD7616_CHANNEL(4),
> > > - AD7616_CHANNEL(5),
> > > - AD7616_CHANNEL(6),
> > > - AD7616_CHANNEL(7),
> > > + AD7606_SW_CHANNEL(0, 16),
> > > + AD7606_SW_CHANNEL(1, 16),
> > > + AD7606_SW_CHANNEL(2, 16),
> > > + AD7606_SW_CHANNEL(3, 16),
> > > + AD7606_SW_CHANNEL(4, 16),
> > > + AD7606_SW_CHANNEL(5, 16),
> > > + AD7606_SW_CHANNEL(6, 16),
> > > + AD7606_SW_CHANNEL(7, 16),
> > > };
> > >
> > > static const unsigned int ad7606B_oversampling_avail[9] = {
> > > --
> > > 2.46.0
> > >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-09-19 13:04 ` [PATCH v7 8/8] iio: adc: ad7606: add support " Alexandru Ardelean
@ 2024-10-01 8:26 ` Alexandru Ardelean
2024-10-01 9:26 ` Guillaume Stols
2024-10-01 13:42 ` David Lechner
0 siblings, 2 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-10-01 8:26 UTC (permalink / raw)
To: linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols, dlechner
On Thu, Sep 19, 2024 at 4:05 PM Alexandru Ardelean
<aardelean@baylibre.com> wrote:
>
> The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
> The main difference between AD7606C-16 & AD7606C-18 is the precision in
> bits (16 vs 18).
> Because of that, some scales need to be defined for the 18-bit variants, as
> they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
>
> Because the AD7606C-16,18 also supports bipolar & differential channels,
> for SW-mode, the default range of 10 V or ±10V should be set at probe.
> On reset, the default range (in the registers) is set to value 0x3 which
> corresponds to '±10 V single-ended range', regardless of bipolar or
> differential configuration.
>
> Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
>
> The AD7606C-18 variant offers 18-bit precision. Because of this, the
> requirement to use this chip is that the SPI controller supports padding
> of 18-bit sequences to 32-bit arrays.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
>
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
> drivers/iio/adc/ad7606.c | 263 +++++++++++++++++++++++++++++++++--
> drivers/iio/adc/ad7606.h | 16 ++-
> drivers/iio/adc/ad7606_spi.c | 55 ++++++++
> 3 files changed, 322 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index b909ee14fd81..f04e5660d2f8 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -36,6 +36,33 @@ static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
> 152588, 305176
> };
>
> +static const unsigned int ad7606_18bit_hw_scale_avail[2] = {
> + 38147, 76294
> +};
> +
> +static const unsigned int ad7606c_16bit_single_ended_unipolar_scale_avail[3] = {
> + 76294, 152588, 190735,
> +};
> +
> +static const unsigned int ad7606c_16bit_single_ended_bipolar_scale_avail[5] = {
> + 76294, 152588, 190735, 305176, 381470
> +};
> +
> +static const unsigned int ad7606c_16bit_differential_bipolar_scale_avail[4] = {
> + 152588, 305176, 381470, 610352
> +};
> +
> +static const unsigned int ad7606c_18bit_single_ended_unipolar_scale_avail[3] = {
> + 19073, 38147, 47684
> +};
> +
> +static const unsigned int ad7606c_18bit_single_ended_bipolar_scale_avail[5] = {
> + 19073, 38147, 47684, 76294, 95367
> +};
> +
> +static const unsigned int ad7606c_18bit_differential_bipolar_scale_avail[4] = {
> + 38147, 76294, 95367, 152588
> +};
>
> static const unsigned int ad7606_16bit_sw_scale_avail[3] = {
> 76293, 152588, 305176
> @@ -62,7 +89,8 @@ int ad7606_reset(struct ad7606_state *st)
> }
> EXPORT_SYMBOL_NS_GPL(ad7606_reset, IIO_AD7606);
>
> -static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st, int ch)
> +static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st,
> + struct iio_chan_spec *chan, int ch)
> {
> struct ad7606_chan_scale *cs = &st->chan_scales[ch];
>
> @@ -83,6 +111,173 @@ static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st, int ch)
> return 0;
> }
>
> +static int ad7606_get_chan_config(struct ad7606_state *st, int ch,
> + bool *bipolar, bool *differential)
> +{
> + unsigned int num_channels = st->chip_info->num_channels - 1;
> + struct device *dev = st->dev;
> + int ret;
> +
> + *bipolar = false;
> + *differential = false;
> +
> + device_for_each_child_node_scoped(dev, child) {
> + u32 pins[2];
> + int reg;
> +
> + ret = fwnode_property_read_u32(child, "reg", ®);
> + if (ret)
> + continue;
> +
> + /* channel number (here) is from 1 to num_channels */
> + if (reg == 0 || reg > num_channels) {
> + dev_warn(dev,
> + "Invalid channel number (ignoring): %d\n", reg);
> + continue;
> + }
> +
> + if (reg != (ch + 1))
> + continue;
> +
> + *bipolar = fwnode_property_read_bool(child, "bipolar");
> +
> + ret = fwnode_property_read_u32_array(child, "diff-channels",
> + pins, ARRAY_SIZE(pins));
> + /* Channel is differential, if pins are the same as 'reg' */
> + if (ret == 0 && (pins[0] != reg || pins[1] != reg)) {
> + dev_err(dev,
> + "Differential pins must be the same as 'reg'");
> + return -EINVAL;
> + }
> +
> + *differential = (ret == 0);
> +
> + if (*differential && !*bipolar) {
> + dev_err(dev,
> + "'bipolar' must be added for diff channel %d\n",
> + reg);
> + return -EINVAL;
> + }
> +
> + return 0;
> + }
> +
> + return 0;
> +}
> +
> +static int ad7606c_18bit_chan_scale_setup(struct ad7606_state *st,
> + struct iio_chan_spec *chan, int ch)
> +{
> + struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> + bool bipolar, differential;
> + int ret;
> +
> + if (!st->sw_mode_en) {
> + cs->range = 0;
> + cs->scale_avail = ad7606_18bit_hw_scale_avail;
> + cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
> + return 0;
> + }
> +
> + ret = ad7606_get_chan_config(st, ch, &bipolar, &differential);
> + if (ret)
> + return ret;
> +
> + if (differential) {
> + cs->scale_avail = ad7606c_18bit_differential_bipolar_scale_avail;
> + cs->num_scales =
> + ARRAY_SIZE(ad7606c_18bit_differential_bipolar_scale_avail);
> + /* Bipolar differential ranges start at 8 (b1000) */
> + cs->reg_offset = 8;
> + cs->range = 1;
> + chan->differential = 1;
> + chan->channel2 = chan->channel;
> +
> + return 0;
> + }
> +
> + chan->differential = 0;
> +
> + if (bipolar) {
> + cs->scale_avail = ad7606c_18bit_single_ended_bipolar_scale_avail;
> + cs->num_scales =
> + ARRAY_SIZE(ad7606c_18bit_single_ended_bipolar_scale_avail);
> + /* Bipolar single-ended ranges start at 0 (b0000) */
> + cs->reg_offset = 0;
> + cs->range = 3;
> + chan->scan_type.sign = 's';
> +
> + return 0;
> + }
> +
> + cs->scale_avail = ad7606c_18bit_single_ended_unipolar_scale_avail;
> + cs->num_scales =
> + ARRAY_SIZE(ad7606c_18bit_single_ended_unipolar_scale_avail);
> + /* Unipolar single-ended ranges start at 5 (b0101) */
> + cs->reg_offset = 5;
> + cs->range = 1;
> + chan->scan_type.sign = 'u';
> +
> + return 0;
> +}
> +
> +static int ad7606c_16bit_chan_scale_setup(struct ad7606_state *st,
> + struct iio_chan_spec *chan, int ch)
> +{
> + struct ad7606_chan_scale *cs = &st->chan_scales[ch];
> + bool bipolar, differential;
> + int ret;
> +
> + if (!st->sw_mode_en) {
> + cs->range = 0;
> + cs->scale_avail = ad7606_16bit_hw_scale_avail;
> + cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
> + return 0;
> + }
> +
> + ret = ad7606_get_chan_config(st, ch, &bipolar, &differential);
> + if (ret)
> + return ret;
> +
> + if (differential) {
> + cs->scale_avail = ad7606c_16bit_differential_bipolar_scale_avail;
> + cs->num_scales =
> + ARRAY_SIZE(ad7606c_16bit_differential_bipolar_scale_avail);
> + /* Bipolar differential ranges start at 8 (b1000) */
> + cs->reg_offset = 8;
> + cs->range = 1;
> + chan->differential = 1;
> + chan->channel2 = chan->channel;
> + chan->scan_type.sign = 's';
> +
> + return 0;
> + }
> +
> + chan->differential = 0;
> +
> + if (bipolar) {
> + cs->scale_avail = ad7606c_16bit_single_ended_bipolar_scale_avail;
> + cs->num_scales =
> + ARRAY_SIZE(ad7606c_16bit_single_ended_bipolar_scale_avail);
> + /* Bipolar single-ended ranges start at 0 (b0000) */
> + cs->reg_offset = 0;
> + cs->range = 3;
> + chan->scan_type.sign = 's';
> +
> + return 0;
> + }
> +
> + cs->scale_avail = ad7606c_16bit_single_ended_unipolar_scale_avail;
> + cs->num_scales =
> + ARRAY_SIZE(ad7606c_16bit_single_ended_unipolar_scale_avail);
> + /* Unipolar single-ended ranges start at 5 (b0101) */
> + cs->reg_offset = 5;
> + cs->range = 1;
> + chan->scan_type.sign = 'u';
> +
> + return 0;
> +}
> +
> static int ad7606_reg_access(struct iio_dev *indio_dev,
> unsigned int reg,
> unsigned int writeval,
> @@ -107,9 +302,8 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
> static int ad7606_read_samples(struct ad7606_state *st)
> {
> unsigned int num = st->chip_info->num_channels - 1;
> - u16 *data = st->data;
>
> - return st->bops->read_block(st->dev, num, data);
> + return st->bops->read_block(st->dev, num, &st->data);
> }
>
> static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> @@ -125,7 +319,7 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> if (ret)
> goto error_ret;
>
> - iio_push_to_buffers_with_timestamp(indio_dev, st->data,
> + iio_push_to_buffers_with_timestamp(indio_dev, &st->data,
> iio_get_time_ns(indio_dev));
> error_ret:
> iio_trigger_notify_done(indio_dev->trig);
> @@ -139,6 +333,8 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> int *val)
> {
> struct ad7606_state *st = iio_priv(indio_dev);
> + unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> + const struct iio_chan_spec *chan;
> int ret;
>
> gpiod_set_value(st->gpio_convst, 1);
> @@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> if (ret)
> goto error_ret;
>
> - *val = sign_extend32(st->data[ch], 15);
> + chan = &indio_dev->channels[ch + 1];
> + if (chan->scan_type.sign == 'u') {
> + if (storagebits > 16)
> + *val = st->data.buf32[ch];
> + else
> + *val = st->data.buf16[ch];
> + return 0;
Arrggh...
I messed up here.
Guillaume found a bug here, where this should be "goto error_ret" or
do an "if () {} else {}"
How should we do it here?
Do we send a fix-patch or send a new series?
> + }
> +
> + if (storagebits > 16)
> + *val = sign_extend32(st->data.buf32[ch], 17);
> + else
> + *val = sign_extend32(st->data.buf16[ch], 15);
>
> error_ret:
> gpiod_set_value(st->gpio_convst, 0);
> @@ -266,7 +474,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> ch = chan->address;
> cs = &st->chan_scales[ch];
> i = find_closest(val2, cs->scale_avail, cs->num_scales);
> - ret = st->write_scale(indio_dev, ch, i);
> + ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
> if (ret < 0)
> return ret;
> cs->range = i;
> @@ -349,6 +557,18 @@ static const struct iio_chan_spec ad7606_channels_16bit[] = {
> AD7606_CHANNEL(7, 16),
> };
>
> +static const struct iio_chan_spec ad7606_channels_18bit[] = {
> + IIO_CHAN_SOFT_TIMESTAMP(8),
> + AD7606_CHANNEL(0, 18),
> + AD7606_CHANNEL(1, 18),
> + AD7606_CHANNEL(2, 18),
> + AD7606_CHANNEL(3, 18),
> + AD7606_CHANNEL(4, 18),
> + AD7606_CHANNEL(5, 18),
> + AD7606_CHANNEL(6, 18),
> + AD7606_CHANNEL(7, 18),
> +};
> +
> /*
> * The current assumption that this driver makes for AD7616, is that it's
> * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
> @@ -414,6 +634,20 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
> .oversampling_avail = ad7606_oversampling_avail,
> .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> },
> + [ID_AD7606C_16] = {
> + .channels = ad7606_channels_16bit,
> + .num_channels = 9,
> + .scale_setup_cb = ad7606c_16bit_chan_scale_setup,
> + .oversampling_avail = ad7606_oversampling_avail,
> + .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> + },
> + [ID_AD7606C_18] = {
> + .channels = ad7606_channels_18bit,
> + .num_channels = 9,
> + .scale_setup_cb = ad7606c_18bit_chan_scale_setup,
> + .oversampling_avail = ad7606_oversampling_avail,
> + .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
> + },
> [ID_AD7616] = {
> .channels = ad7616_channels,
> .num_channels = 17,
> @@ -586,7 +820,7 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
> .validate_device = iio_trigger_validate_own_device,
> };
>
> -static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
> +static int ad7606_sw_mode_setup(struct iio_dev *indio_dev, unsigned int id)
> {
> struct ad7606_state *st = iio_priv(indio_dev);
>
> @@ -604,13 +838,24 @@ static int ad7606_chan_scales_setup(struct iio_dev *indio_dev)
> {
> unsigned int num_channels = indio_dev->num_channels - 1;
> struct ad7606_state *st = iio_priv(indio_dev);
> + struct iio_chan_spec *chans;
> + size_t size;
> int ch, ret;
>
> + /* Clone IIO channels, since some may be differential */
> + size = indio_dev->num_channels * sizeof(*indio_dev->channels);
> + chans = devm_kzalloc(st->dev, size, GFP_KERNEL);
> + if (!chans)
> + return -ENOMEM;
> +
> + memcpy(chans, indio_dev->channels, size);
> + indio_dev->channels = chans;
> +
> for (ch = 0; ch < num_channels; ch++) {
> struct ad7606_chan_scale *cs;
> int i;
>
> - ret = st->chip_info->scale_setup_cb(st, ch);
> + ret = st->chip_info->scale_setup_cb(st, &chans[ch + 1], ch);
> if (ret)
> return ret;
>
> @@ -698,7 +943,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> st->write_scale = ad7606_write_scale_hw;
> st->write_os = ad7606_write_os_hw;
>
> - ret = ad7606_sw_mode_setup(indio_dev);
> + ret = ad7606_sw_mode_setup(indio_dev, id);
> if (ret)
> return ret;
>
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index 25e84efd15c3..14ee75aa225b 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -63,7 +63,8 @@
>
> struct ad7606_state;
>
> -typedef int (*ad7606_scale_setup_cb_t)(struct ad7606_state *st, int ch);
> +typedef int (*ad7606_scale_setup_cb_t)(struct ad7606_state *st,
> + struct iio_chan_spec *chan, int ch);
>
> /**
> * struct ad7606_chip_info - chip specific information
> @@ -94,6 +95,8 @@ struct ad7606_chip_info {
> * such that it can be read via the 'read_avail' hook
> * @num_scales number of elements stored in the scale_avail array
> * @range voltage range selection, selects which scale to apply
> + * @reg_offset offset for the register value, to be applied when
> + * writing the value of 'range' to the register value
> */
> struct ad7606_chan_scale {
> #define AD760X_MAX_SCALES 16
> @@ -102,6 +105,7 @@ struct ad7606_chan_scale {
> int scale_avail_show[AD760X_MAX_SCALE_SHOW];
> unsigned int num_scales;
> unsigned int range;
> + unsigned int reg_offset;
> };
>
> /**
> @@ -158,9 +162,13 @@ struct ad7606_state {
> /*
> * DMA (thus cache coherency maintenance) may require the
> * transfer buffers to live in their own cache lines.
> - * 16 * 16-bit samples + 64-bit timestamp
> + * 16 * 16-bit samples + 64-bit timestamp - for AD7616
> + * 8 * 32-bit samples + 64-bit timestamp - for AD7616C-18 (and similar)
> */
> - unsigned short data[20] __aligned(IIO_DMA_MINALIGN);
> + union {
> + u16 buf16[20];
> + u32 buf32[10];
> + } data __aligned(IIO_DMA_MINALIGN);
> __be16 d16[2];
> };
>
> @@ -201,6 +209,8 @@ enum ad7606_supported_device_ids {
> ID_AD7606_6,
> ID_AD7606_4,
> ID_AD7606B,
> + ID_AD7606C_16,
> + ID_AD7606C_18,
> ID_AD7616,
> };
>
> diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
> index e00f58a6a0e9..143440e73aab 100644
> --- a/drivers/iio/adc/ad7606_spi.c
> +++ b/drivers/iio/adc/ad7606_spi.c
> @@ -77,6 +77,18 @@ static const struct iio_chan_spec ad7606b_sw_channels[] = {
> AD7606_SW_CHANNEL(7, 16),
> };
>
> +static const struct iio_chan_spec ad7606c_18_sw_channels[] = {
> + IIO_CHAN_SOFT_TIMESTAMP(8),
> + AD7606_SW_CHANNEL(0, 18),
> + AD7606_SW_CHANNEL(1, 18),
> + AD7606_SW_CHANNEL(2, 18),
> + AD7606_SW_CHANNEL(3, 18),
> + AD7606_SW_CHANNEL(4, 18),
> + AD7606_SW_CHANNEL(5, 18),
> + AD7606_SW_CHANNEL(6, 18),
> + AD7606_SW_CHANNEL(7, 18),
> +};
> +
> static const unsigned int ad7606B_oversampling_avail[9] = {
> 1, 2, 4, 8, 16, 32, 64, 128, 256
> };
> @@ -120,6 +132,19 @@ static int ad7606_spi_read_block(struct device *dev,
> return 0;
> }
>
> +static int ad7606_spi_read_block18to32(struct device *dev,
> + int count, void *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct spi_transfer xfer = {
> + .bits_per_word = 18,
> + .len = count * sizeof(u32),
> + .rx_buf = buf,
> + };
> +
> + return spi_sync_transfer(spi, &xfer, 1);
> +}
> +
> static int ad7606_spi_reg_read(struct ad7606_state *st, unsigned int addr)
> {
> struct spi_device *spi = to_spi_device(st->dev);
> @@ -283,6 +308,19 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
> return 0;
> }
>
> +static int ad7606c_18_sw_mode_config(struct iio_dev *indio_dev)
> +{
> + int ret;
> +
> + ret = ad7606B_sw_mode_config(indio_dev);
> + if (ret)
> + return ret;
> +
> + indio_dev->channels = ad7606c_18_sw_channels;
> +
> + return 0;
> +}
> +
> static const struct ad7606_bus_ops ad7606_spi_bops = {
> .read_block = ad7606_spi_read_block,
> };
> @@ -305,6 +343,15 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
> .sw_mode_config = ad7606B_sw_mode_config,
> };
>
> +static const struct ad7606_bus_ops ad7606c_18_spi_bops = {
> + .read_block = ad7606_spi_read_block18to32,
> + .reg_read = ad7606_spi_reg_read,
> + .reg_write = ad7606_spi_reg_write,
> + .write_mask = ad7606_spi_write_mask,
> + .rd_wr_cmd = ad7606B_spi_rd_wr_cmd,
> + .sw_mode_config = ad7606c_18_sw_mode_config,
> +};
> +
> static int ad7606_spi_probe(struct spi_device *spi)
> {
> const struct spi_device_id *id = spi_get_device_id(spi);
> @@ -315,8 +362,12 @@ static int ad7606_spi_probe(struct spi_device *spi)
> bops = &ad7616_spi_bops;
> break;
> case ID_AD7606B:
> + case ID_AD7606C_16:
> bops = &ad7606B_spi_bops;
> break;
> + case ID_AD7606C_18:
> + bops = &ad7606c_18_spi_bops;
> + break;
> default:
> bops = &ad7606_spi_bops;
> break;
> @@ -333,6 +384,8 @@ static const struct spi_device_id ad7606_id_table[] = {
> { "ad7606-6", ID_AD7606_6 },
> { "ad7606-8", ID_AD7606_8 },
> { "ad7606b", ID_AD7606B },
> + { "ad7606c-16", ID_AD7606C_16 },
> + { "ad7606c-18", ID_AD7606C_18 },
> { "ad7616", ID_AD7616 },
> { }
> };
> @@ -344,6 +397,8 @@ static const struct of_device_id ad7606_of_match[] = {
> { .compatible = "adi,ad7606-6" },
> { .compatible = "adi,ad7606-8" },
> { .compatible = "adi,ad7606b" },
> + { .compatible = "adi,ad7606c-16" },
> + { .compatible = "adi,ad7606c-18" },
> { .compatible = "adi,ad7616" },
> { }
> };
> --
> 2.46.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-10-01 8:26 ` Alexandru Ardelean
@ 2024-10-01 9:26 ` Guillaume Stols
2024-10-01 13:42 ` David Lechner
1 sibling, 0 replies; 22+ messages in thread
From: Guillaume Stols @ 2024-10-01 9:26 UTC (permalink / raw)
To: Alexandru Ardelean, linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, dlechner
On 10/1/24 10:26, Alexandru Ardelean wrote:
> On Thu, Sep 19, 2024 at 4:05 PM Alexandru Ardelean
> <aardelean@baylibre.com> wrote:
>> The AD7606C-16 and AD7606C-18 are pretty similar with the AD7606B.
>> The main difference between AD7606C-16 & AD7606C-18 is the precision in
>> bits (16 vs 18).
>> Because of that, some scales need to be defined for the 18-bit variants, as
>> they need to be computed against 2**18 (vs 2**16 for the 16 bit-variants).
>>
>> Because the AD7606C-16,18 also supports bipolar & differential channels,
>> for SW-mode, the default range of 10 V or ±10V should be set at probe.
>> On reset, the default range (in the registers) is set to value 0x3 which
>> corresponds to '±10 V single-ended range', regardless of bipolar or
>> differential configuration.
>>
>> Aside from the scale/ranges, the AD7606C-16 is similar to the AD7606B.
>>
>> The AD7606C-18 variant offers 18-bit precision. Because of this, the
>> requirement to use this chip is that the SPI controller supports padding
>> of 18-bit sequences to 32-bit arrays.
>>
>> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
>> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
>>
>> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
>> ---
>> drivers/iio/adc/ad7606.c | 263 +++++++++++++++++++++++++++++++++--
>> drivers/iio/adc/ad7606.h | 16 ++-
>> drivers/iio/adc/ad7606_spi.c | 55 ++++++++
>> 3 files changed, 322 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
>> index b909ee14fd81..f04e5660d2f8 100644
>> --- a/drivers/iio/adc/ad7606.c
>> +++ b/drivers/iio/adc/ad7606.c
>> @@ -36,6 +36,33 @@ static const unsigned int ad7606_16bit_hw_scale_avail[2] = {
>> 152588, 305176
>> };
>>
>> +static const unsigned int ad7606_18bit_hw_scale_avail[2] = {
>> + 38147, 76294
>> +};
>> +
>> +static const unsigned int ad7606c_16bit_single_ended_unipolar_scale_avail[3] = {
>> + 76294, 152588, 190735,
>> +};
>> +
>> +static const unsigned int ad7606c_16bit_single_ended_bipolar_scale_avail[5] = {
>> + 76294, 152588, 190735, 305176, 381470
>> +};
>> +
>> +static const unsigned int ad7606c_16bit_differential_bipolar_scale_avail[4] = {
>> + 152588, 305176, 381470, 610352
>> +};
>> +
>> +static const unsigned int ad7606c_18bit_single_ended_unipolar_scale_avail[3] = {
>> + 19073, 38147, 47684
>> +};
>> +
>> +static const unsigned int ad7606c_18bit_single_ended_bipolar_scale_avail[5] = {
>> + 19073, 38147, 47684, 76294, 95367
>> +};
>> +
>> +static const unsigned int ad7606c_18bit_differential_bipolar_scale_avail[4] = {
>> + 38147, 76294, 95367, 152588
>> +};
>>
>> static const unsigned int ad7606_16bit_sw_scale_avail[3] = {
>> 76293, 152588, 305176
>> @@ -62,7 +89,8 @@ int ad7606_reset(struct ad7606_state *st)
>> }
>> EXPORT_SYMBOL_NS_GPL(ad7606_reset, IIO_AD7606);
>>
>> -static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st, int ch)
>> +static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st,
>> + struct iio_chan_spec *chan, int ch)
>> {
>> struct ad7606_chan_scale *cs = &st->chan_scales[ch];
>>
>> @@ -83,6 +111,173 @@ static int ad7606_16bit_chan_scale_setup(struct ad7606_state *st, int ch)
>> return 0;
>> }
>>
>> +static int ad7606_get_chan_config(struct ad7606_state *st, int ch,
>> + bool *bipolar, bool *differential)
>> +{
>> + unsigned int num_channels = st->chip_info->num_channels - 1;
>> + struct device *dev = st->dev;
>> + int ret;
>> +
>> + *bipolar = false;
>> + *differential = false;
>> +
>> + device_for_each_child_node_scoped(dev, child) {
>> + u32 pins[2];
>> + int reg;
>> +
>> + ret = fwnode_property_read_u32(child, "reg", ®);
>> + if (ret)
>> + continue;
>> +
>> + /* channel number (here) is from 1 to num_channels */
>> + if (reg == 0 || reg > num_channels) {
>> + dev_warn(dev,
>> + "Invalid channel number (ignoring): %d\n", reg);
>> + continue;
>> + }
>> +
>> + if (reg != (ch + 1))
>> + continue;
>> +
>> + *bipolar = fwnode_property_read_bool(child, "bipolar");
>> +
>> + ret = fwnode_property_read_u32_array(child, "diff-channels",
>> + pins, ARRAY_SIZE(pins));
>> + /* Channel is differential, if pins are the same as 'reg' */
>> + if (ret == 0 && (pins[0] != reg || pins[1] != reg)) {
>> + dev_err(dev,
>> + "Differential pins must be the same as 'reg'");
>> + return -EINVAL;
>> + }
>> +
>> + *differential = (ret == 0);
>> +
>> + if (*differential && !*bipolar) {
>> + dev_err(dev,
>> + "'bipolar' must be added for diff channel %d\n",
>> + reg);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ad7606c_18bit_chan_scale_setup(struct ad7606_state *st,
>> + struct iio_chan_spec *chan, int ch)
>> +{
>> + struct ad7606_chan_scale *cs = &st->chan_scales[ch];
>> + bool bipolar, differential;
>> + int ret;
>> +
>> + if (!st->sw_mode_en) {
>> + cs->range = 0;
>> + cs->scale_avail = ad7606_18bit_hw_scale_avail;
>> + cs->num_scales = ARRAY_SIZE(ad7606_18bit_hw_scale_avail);
>> + return 0;
>> + }
>> +
>> + ret = ad7606_get_chan_config(st, ch, &bipolar, &differential);
>> + if (ret)
>> + return ret;
>> +
>> + if (differential) {
>> + cs->scale_avail = ad7606c_18bit_differential_bipolar_scale_avail;
>> + cs->num_scales =
>> + ARRAY_SIZE(ad7606c_18bit_differential_bipolar_scale_avail);
>> + /* Bipolar differential ranges start at 8 (b1000) */
>> + cs->reg_offset = 8;
>> + cs->range = 1;
>> + chan->differential = 1;
>> + chan->channel2 = chan->channel;
>> +
>> + return 0;
>> + }
>> +
>> + chan->differential = 0;
>> +
>> + if (bipolar) {
>> + cs->scale_avail = ad7606c_18bit_single_ended_bipolar_scale_avail;
>> + cs->num_scales =
>> + ARRAY_SIZE(ad7606c_18bit_single_ended_bipolar_scale_avail);
>> + /* Bipolar single-ended ranges start at 0 (b0000) */
>> + cs->reg_offset = 0;
>> + cs->range = 3;
>> + chan->scan_type.sign = 's';
>> +
>> + return 0;
>> + }
>> +
>> + cs->scale_avail = ad7606c_18bit_single_ended_unipolar_scale_avail;
>> + cs->num_scales =
>> + ARRAY_SIZE(ad7606c_18bit_single_ended_unipolar_scale_avail);
>> + /* Unipolar single-ended ranges start at 5 (b0101) */
>> + cs->reg_offset = 5;
>> + cs->range = 1;
>> + chan->scan_type.sign = 'u';
>> +
>> + return 0;
>> +}
>> +
>> +static int ad7606c_16bit_chan_scale_setup(struct ad7606_state *st,
>> + struct iio_chan_spec *chan, int ch)
>> +{
>> + struct ad7606_chan_scale *cs = &st->chan_scales[ch];
>> + bool bipolar, differential;
>> + int ret;
>> +
>> + if (!st->sw_mode_en) {
>> + cs->range = 0;
>> + cs->scale_avail = ad7606_16bit_hw_scale_avail;
>> + cs->num_scales = ARRAY_SIZE(ad7606_16bit_hw_scale_avail);
>> + return 0;
>> + }
>> +
>> + ret = ad7606_get_chan_config(st, ch, &bipolar, &differential);
>> + if (ret)
>> + return ret;
>> +
>> + if (differential) {
>> + cs->scale_avail = ad7606c_16bit_differential_bipolar_scale_avail;
>> + cs->num_scales =
>> + ARRAY_SIZE(ad7606c_16bit_differential_bipolar_scale_avail);
>> + /* Bipolar differential ranges start at 8 (b1000) */
>> + cs->reg_offset = 8;
>> + cs->range = 1;
>> + chan->differential = 1;
>> + chan->channel2 = chan->channel;
>> + chan->scan_type.sign = 's';
>> +
>> + return 0;
>> + }
>> +
>> + chan->differential = 0;
>> +
>> + if (bipolar) {
>> + cs->scale_avail = ad7606c_16bit_single_ended_bipolar_scale_avail;
>> + cs->num_scales =
>> + ARRAY_SIZE(ad7606c_16bit_single_ended_bipolar_scale_avail);
>> + /* Bipolar single-ended ranges start at 0 (b0000) */
>> + cs->reg_offset = 0;
>> + cs->range = 3;
>> + chan->scan_type.sign = 's';
>> +
>> + return 0;
>> + }
>> +
>> + cs->scale_avail = ad7606c_16bit_single_ended_unipolar_scale_avail;
>> + cs->num_scales =
>> + ARRAY_SIZE(ad7606c_16bit_single_ended_unipolar_scale_avail);
>> + /* Unipolar single-ended ranges start at 5 (b0101) */
>> + cs->reg_offset = 5;
>> + cs->range = 1;
>> + chan->scan_type.sign = 'u';
>> +
>> + return 0;
>> +}
>> +
>> static int ad7606_reg_access(struct iio_dev *indio_dev,
>> unsigned int reg,
>> unsigned int writeval,
>> @@ -107,9 +302,8 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
>> static int ad7606_read_samples(struct ad7606_state *st)
>> {
>> unsigned int num = st->chip_info->num_channels - 1;
>> - u16 *data = st->data;
>>
>> - return st->bops->read_block(st->dev, num, data);
>> + return st->bops->read_block(st->dev, num, &st->data);
>> }
>>
>> static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>> @@ -125,7 +319,7 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>> if (ret)
>> goto error_ret;
>>
>> - iio_push_to_buffers_with_timestamp(indio_dev, st->data,
>> + iio_push_to_buffers_with_timestamp(indio_dev, &st->data,
>> iio_get_time_ns(indio_dev));
>> error_ret:
>> iio_trigger_notify_done(indio_dev->trig);
>> @@ -139,6 +333,8 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>> int *val)
>> {
>> struct ad7606_state *st = iio_priv(indio_dev);
>> + unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
>> + const struct iio_chan_spec *chan;
>> int ret;
>>
>> gpiod_set_value(st->gpio_convst, 1);
>> @@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>> if (ret)
>> goto error_ret;
>>
>> - *val = sign_extend32(st->data[ch], 15);
>> + chan = &indio_dev->channels[ch + 1];
>> + if (chan->scan_type.sign == 'u') {
>> + if (storagebits > 16)
>> + *val = st->data.buf32[ch];
>> + else
>> + *val = st->data.buf16[ch];
>> + return 0;
> Arrggh...
> I messed up here.
> Guillaume found a bug here, where this should be "goto error_ret" or
> do an "if () {} else {}"
> How should we do it here?
>
> Do we send a fix-patch or send a new series?
I can also send a fix in my next version of the "Add iio backend
support" series which depends on this one.
>
>
>> + }
>> +
>> + if (storagebits > 16)
>> + *val = sign_extend32(st->data.buf32[ch], 17);
>> + else
>> + *val = sign_extend32(st->data.buf16[ch], 15);
>>
>> error_ret:
>> gpiod_set_value(st->gpio_convst, 0);
>> @@ -266,7 +474,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>> ch = chan->address;
>> cs = &st->chan_scales[ch];
>> i = find_closest(val2, cs->scale_avail, cs->num_scales);
>> - ret = st->write_scale(indio_dev, ch, i);
>> + ret = st->write_scale(indio_dev, ch, i + cs->reg_offset);
>> if (ret < 0)
>> return ret;
>> cs->range = i;
>> @@ -349,6 +557,18 @@ static const struct iio_chan_spec ad7606_channels_16bit[] = {
>> AD7606_CHANNEL(7, 16),
>> };
>>
>> +static const struct iio_chan_spec ad7606_channels_18bit[] = {
>> + IIO_CHAN_SOFT_TIMESTAMP(8),
>> + AD7606_CHANNEL(0, 18),
>> + AD7606_CHANNEL(1, 18),
>> + AD7606_CHANNEL(2, 18),
>> + AD7606_CHANNEL(3, 18),
>> + AD7606_CHANNEL(4, 18),
>> + AD7606_CHANNEL(5, 18),
>> + AD7606_CHANNEL(6, 18),
>> + AD7606_CHANNEL(7, 18),
>> +};
>> +
>> /*
>> * The current assumption that this driver makes for AD7616, is that it's
>> * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
>> @@ -414,6 +634,20 @@ static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
>> .oversampling_avail = ad7606_oversampling_avail,
>> .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
>> },
>> + [ID_AD7606C_16] = {
>> + .channels = ad7606_channels_16bit,
>> + .num_channels = 9,
>> + .scale_setup_cb = ad7606c_16bit_chan_scale_setup,
>> + .oversampling_avail = ad7606_oversampling_avail,
>> + .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
>> + },
>> + [ID_AD7606C_18] = {
>> + .channels = ad7606_channels_18bit,
>> + .num_channels = 9,
>> + .scale_setup_cb = ad7606c_18bit_chan_scale_setup,
>> + .oversampling_avail = ad7606_oversampling_avail,
>> + .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
>> + },
>> [ID_AD7616] = {
>> .channels = ad7616_channels,
>> .num_channels = 17,
>> @@ -586,7 +820,7 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
>> .validate_device = iio_trigger_validate_own_device,
>> };
>>
>> -static int ad7606_sw_mode_setup(struct iio_dev *indio_dev)
>> +static int ad7606_sw_mode_setup(struct iio_dev *indio_dev, unsigned int id)
>> {
>> struct ad7606_state *st = iio_priv(indio_dev);
>>
>> @@ -604,13 +838,24 @@ static int ad7606_chan_scales_setup(struct iio_dev *indio_dev)
>> {
>> unsigned int num_channels = indio_dev->num_channels - 1;
>> struct ad7606_state *st = iio_priv(indio_dev);
>> + struct iio_chan_spec *chans;
>> + size_t size;
>> int ch, ret;
>>
>> + /* Clone IIO channels, since some may be differential */
>> + size = indio_dev->num_channels * sizeof(*indio_dev->channels);
>> + chans = devm_kzalloc(st->dev, size, GFP_KERNEL);
>> + if (!chans)
>> + return -ENOMEM;
>> +
>> + memcpy(chans, indio_dev->channels, size);
>> + indio_dev->channels = chans;
>> +
>> for (ch = 0; ch < num_channels; ch++) {
>> struct ad7606_chan_scale *cs;
>> int i;
>>
>> - ret = st->chip_info->scale_setup_cb(st, ch);
>> + ret = st->chip_info->scale_setup_cb(st, &chans[ch + 1], ch);
>> if (ret)
>> return ret;
>>
>> @@ -698,7 +943,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>> st->write_scale = ad7606_write_scale_hw;
>> st->write_os = ad7606_write_os_hw;
>>
>> - ret = ad7606_sw_mode_setup(indio_dev);
>> + ret = ad7606_sw_mode_setup(indio_dev, id);
>> if (ret)
>> return ret;
>>
>> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
>> index 25e84efd15c3..14ee75aa225b 100644
>> --- a/drivers/iio/adc/ad7606.h
>> +++ b/drivers/iio/adc/ad7606.h
>> @@ -63,7 +63,8 @@
>>
>> struct ad7606_state;
>>
>> -typedef int (*ad7606_scale_setup_cb_t)(struct ad7606_state *st, int ch);
>> +typedef int (*ad7606_scale_setup_cb_t)(struct ad7606_state *st,
>> + struct iio_chan_spec *chan, int ch);
>>
>> /**
>> * struct ad7606_chip_info - chip specific information
>> @@ -94,6 +95,8 @@ struct ad7606_chip_info {
>> * such that it can be read via the 'read_avail' hook
>> * @num_scales number of elements stored in the scale_avail array
>> * @range voltage range selection, selects which scale to apply
>> + * @reg_offset offset for the register value, to be applied when
>> + * writing the value of 'range' to the register value
>> */
>> struct ad7606_chan_scale {
>> #define AD760X_MAX_SCALES 16
>> @@ -102,6 +105,7 @@ struct ad7606_chan_scale {
>> int scale_avail_show[AD760X_MAX_SCALE_SHOW];
>> unsigned int num_scales;
>> unsigned int range;
>> + unsigned int reg_offset;
>> };
>>
>> /**
>> @@ -158,9 +162,13 @@ struct ad7606_state {
>> /*
>> * DMA (thus cache coherency maintenance) may require the
>> * transfer buffers to live in their own cache lines.
>> - * 16 * 16-bit samples + 64-bit timestamp
>> + * 16 * 16-bit samples + 64-bit timestamp - for AD7616
>> + * 8 * 32-bit samples + 64-bit timestamp - for AD7616C-18 (and similar)
>> */
>> - unsigned short data[20] __aligned(IIO_DMA_MINALIGN);
>> + union {
>> + u16 buf16[20];
>> + u32 buf32[10];
>> + } data __aligned(IIO_DMA_MINALIGN);
>> __be16 d16[2];
>> };
>>
>> @@ -201,6 +209,8 @@ enum ad7606_supported_device_ids {
>> ID_AD7606_6,
>> ID_AD7606_4,
>> ID_AD7606B,
>> + ID_AD7606C_16,
>> + ID_AD7606C_18,
>> ID_AD7616,
>> };
>>
>> diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
>> index e00f58a6a0e9..143440e73aab 100644
>> --- a/drivers/iio/adc/ad7606_spi.c
>> +++ b/drivers/iio/adc/ad7606_spi.c
>> @@ -77,6 +77,18 @@ static const struct iio_chan_spec ad7606b_sw_channels[] = {
>> AD7606_SW_CHANNEL(7, 16),
>> };
>>
>> +static const struct iio_chan_spec ad7606c_18_sw_channels[] = {
>> + IIO_CHAN_SOFT_TIMESTAMP(8),
>> + AD7606_SW_CHANNEL(0, 18),
>> + AD7606_SW_CHANNEL(1, 18),
>> + AD7606_SW_CHANNEL(2, 18),
>> + AD7606_SW_CHANNEL(3, 18),
>> + AD7606_SW_CHANNEL(4, 18),
>> + AD7606_SW_CHANNEL(5, 18),
>> + AD7606_SW_CHANNEL(6, 18),
>> + AD7606_SW_CHANNEL(7, 18),
>> +};
>> +
>> static const unsigned int ad7606B_oversampling_avail[9] = {
>> 1, 2, 4, 8, 16, 32, 64, 128, 256
>> };
>> @@ -120,6 +132,19 @@ static int ad7606_spi_read_block(struct device *dev,
>> return 0;
>> }
>>
>> +static int ad7606_spi_read_block18to32(struct device *dev,
>> + int count, void *buf)
>> +{
>> + struct spi_device *spi = to_spi_device(dev);
>> + struct spi_transfer xfer = {
>> + .bits_per_word = 18,
>> + .len = count * sizeof(u32),
>> + .rx_buf = buf,
>> + };
>> +
>> + return spi_sync_transfer(spi, &xfer, 1);
>> +}
>> +
>> static int ad7606_spi_reg_read(struct ad7606_state *st, unsigned int addr)
>> {
>> struct spi_device *spi = to_spi_device(st->dev);
>> @@ -283,6 +308,19 @@ static int ad7606B_sw_mode_config(struct iio_dev *indio_dev)
>> return 0;
>> }
>>
>> +static int ad7606c_18_sw_mode_config(struct iio_dev *indio_dev)
>> +{
>> + int ret;
>> +
>> + ret = ad7606B_sw_mode_config(indio_dev);
>> + if (ret)
>> + return ret;
>> +
>> + indio_dev->channels = ad7606c_18_sw_channels;
>> +
>> + return 0;
>> +}
>> +
>> static const struct ad7606_bus_ops ad7606_spi_bops = {
>> .read_block = ad7606_spi_read_block,
>> };
>> @@ -305,6 +343,15 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
>> .sw_mode_config = ad7606B_sw_mode_config,
>> };
>>
>> +static const struct ad7606_bus_ops ad7606c_18_spi_bops = {
>> + .read_block = ad7606_spi_read_block18to32,
>> + .reg_read = ad7606_spi_reg_read,
>> + .reg_write = ad7606_spi_reg_write,
>> + .write_mask = ad7606_spi_write_mask,
>> + .rd_wr_cmd = ad7606B_spi_rd_wr_cmd,
>> + .sw_mode_config = ad7606c_18_sw_mode_config,
>> +};
>> +
>> static int ad7606_spi_probe(struct spi_device *spi)
>> {
>> const struct spi_device_id *id = spi_get_device_id(spi);
>> @@ -315,8 +362,12 @@ static int ad7606_spi_probe(struct spi_device *spi)
>> bops = &ad7616_spi_bops;
>> break;
>> case ID_AD7606B:
>> + case ID_AD7606C_16:
>> bops = &ad7606B_spi_bops;
>> break;
>> + case ID_AD7606C_18:
>> + bops = &ad7606c_18_spi_bops;
>> + break;
>> default:
>> bops = &ad7606_spi_bops;
>> break;
>> @@ -333,6 +384,8 @@ static const struct spi_device_id ad7606_id_table[] = {
>> { "ad7606-6", ID_AD7606_6 },
>> { "ad7606-8", ID_AD7606_8 },
>> { "ad7606b", ID_AD7606B },
>> + { "ad7606c-16", ID_AD7606C_16 },
>> + { "ad7606c-18", ID_AD7606C_18 },
>> { "ad7616", ID_AD7616 },
>> { }
>> };
>> @@ -344,6 +397,8 @@ static const struct of_device_id ad7606_of_match[] = {
>> { .compatible = "adi,ad7606-6" },
>> { .compatible = "adi,ad7606-8" },
>> { .compatible = "adi,ad7606b" },
>> + { .compatible = "adi,ad7606c-16" },
>> + { .compatible = "adi,ad7606c-18" },
>> { .compatible = "adi,ad7616" },
>> { }
>> };
>> --
>> 2.46.0
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-10-01 8:26 ` Alexandru Ardelean
2024-10-01 9:26 ` Guillaume Stols
@ 2024-10-01 13:42 ` David Lechner
2024-10-01 18:41 ` Jonathan Cameron
1 sibling, 1 reply; 22+ messages in thread
From: David Lechner @ 2024-10-01 13:42 UTC (permalink / raw)
To: Alexandru Ardelean, linux-iio, linux-kernel, devicetree
Cc: jic23, krzk+dt, robh, lars, michael.hennerich, gstols
On 10/1/24 3:26 AM, Alexandru Ardelean wrote:
> On Thu, Sep 19, 2024 at 4:05 PM Alexandru Ardelean
> <aardelean@baylibre.com> wrote:
>>
...
>> @@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>> if (ret)
>> goto error_ret;
>>
>> - *val = sign_extend32(st->data[ch], 15);
>> + chan = &indio_dev->channels[ch + 1];
>> + if (chan->scan_type.sign == 'u') {
>> + if (storagebits > 16)
>> + *val = st->data.buf32[ch];
>> + else
>> + *val = st->data.buf16[ch];
>> + return 0;
>
> Arrggh...
> I messed up here.
> Guillaume found a bug here, where this should be "goto error_ret" or
> do an "if () {} else {}"
> How should we do it here?
>
> Do we send a fix-patch or send a new series?
>
Since this patch is already applied, just follow up with another
patch with a Fixes: tag.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-10-01 13:42 ` David Lechner
@ 2024-10-01 18:41 ` Jonathan Cameron
2024-10-02 6:12 ` Alexandru Ardelean
0 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2024-10-01 18:41 UTC (permalink / raw)
To: David Lechner
Cc: Alexandru Ardelean, linux-iio, linux-kernel, devicetree, krzk+dt,
robh, lars, michael.hennerich, gstols
On Tue, 1 Oct 2024 08:42:23 -0500
David Lechner <dlechner@baylibre.com> wrote:
> On 10/1/24 3:26 AM, Alexandru Ardelean wrote:
> > On Thu, Sep 19, 2024 at 4:05 PM Alexandru Ardelean
> > <aardelean@baylibre.com> wrote:
> >>
>
> ...
>
> >> @@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> >> if (ret)
> >> goto error_ret;
> >>
> >> - *val = sign_extend32(st->data[ch], 15);
> >> + chan = &indio_dev->channels[ch + 1];
> >> + if (chan->scan_type.sign == 'u') {
> >> + if (storagebits > 16)
> >> + *val = st->data.buf32[ch];
> >> + else
> >> + *val = st->data.buf16[ch];
> >> + return 0;
> >
> > Arrggh...
> > I messed up here.
> > Guillaume found a bug here, where this should be "goto error_ret" or
> > do an "if () {} else {}"
> > How should we do it here?
if / else. Goto an error label when it's not an error would be horrible!
> >
> > Do we send a fix-patch or send a new series?
> >
>
> Since this patch is already applied, just follow up with another
> patch with a Fixes: tag.
I sometimes tweak these sort of things if I haven't pushed out
as togreg yet (or they are really bad!) but in this case I'm not
100% sure what the comment is, so I'll just apply a fix on top.
So David is entirely correct in general but by luck of timing
this time I'll tweak it.
Please check the result in iio.git/testing
I'll hold off pushing that out as togreg until at least end of
tomorrow. One more day o
Jonathan
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-10-01 18:41 ` Jonathan Cameron
@ 2024-10-02 6:12 ` Alexandru Ardelean
2024-10-04 13:54 ` Jonathan Cameron
0 siblings, 1 reply; 22+ messages in thread
From: Alexandru Ardelean @ 2024-10-02 6:12 UTC (permalink / raw)
To: Jonathan Cameron
Cc: David Lechner, linux-iio, linux-kernel, devicetree, krzk+dt, robh,
lars, michael.hennerich, gstols
On Tue, Oct 1, 2024 at 9:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 1 Oct 2024 08:42:23 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On 10/1/24 3:26 AM, Alexandru Ardelean wrote:
> > > On Thu, Sep 19, 2024 at 4:05 PM Alexandru Ardelean
> > > <aardelean@baylibre.com> wrote:
> > >>
> >
> > ...
> >
> > >> @@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> > >> if (ret)
> > >> goto error_ret;
> > >>
> > >> - *val = sign_extend32(st->data[ch], 15);
> > >> + chan = &indio_dev->channels[ch + 1];
> > >> + if (chan->scan_type.sign == 'u') {
> > >> + if (storagebits > 16)
> > >> + *val = st->data.buf32[ch];
> > >> + else
> > >> + *val = st->data.buf16[ch];
> > >> + return 0;
> > >
> > > Arrggh...
> > > I messed up here.
> > > Guillaume found a bug here, where this should be "goto error_ret" or
> > > do an "if () {} else {}"
> > > How should we do it here?
> if / else. Goto an error label when it's not an error would be horrible!
> > >
> > > Do we send a fix-patch or send a new series?
> > >
> >
> > Since this patch is already applied, just follow up with another
> > patch with a Fixes: tag.
>
> I sometimes tweak these sort of things if I haven't pushed out
> as togreg yet (or they are really bad!) but in this case I'm not
> 100% sure what the comment is, so I'll just apply a fix on top.
>
> So David is entirely correct in general but by luck of timing
> this time I'll tweak it.
>
> Please check the result in iio.git/testing
> I'll hold off pushing that out as togreg until at least end of
> tomorrow. One more day o
The "return 0" needs to be removed in the driver.
if (chan->scan_type.sign == 'u') {
if (storagebits > 16)
*val = st->data.buf32[ch];
else
*val = st->data.buf16[ch];
- return 0;
} else {
if (storagebits > 16)
*val = sign_extend32(st->data.buf32[ch], 17);
else
*val = sign_extend32(st->data.buf16[ch], 15);
}
>
> Jonathan
>
>
> >
> >
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-10-02 6:12 ` Alexandru Ardelean
@ 2024-10-04 13:54 ` Jonathan Cameron
2024-10-06 10:56 ` Jonathan Cameron
0 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2024-10-04 13:54 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: Jonathan Cameron, David Lechner, linux-iio, linux-kernel,
devicetree, krzk+dt, robh, lars, michael.hennerich, gstols
On Wed, 2 Oct 2024 09:12:09 +0300
Alexandru Ardelean <aardelean@baylibre.com> wrote:
> On Tue, Oct 1, 2024 at 9:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 1 Oct 2024 08:42:23 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >
> > > On 10/1/24 3:26 AM, Alexandru Ardelean wrote:
> > > > On Thu, Sep 19, 2024 at 4:05 PM Alexandru Ardelean
> > > > <aardelean@baylibre.com> wrote:
> > > >>
> > >
> > > ...
> > >
> > > >> @@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> > > >> if (ret)
> > > >> goto error_ret;
> > > >>
> > > >> - *val = sign_extend32(st->data[ch], 15);
> > > >> + chan = &indio_dev->channels[ch + 1];
> > > >> + if (chan->scan_type.sign == 'u') {
> > > >> + if (storagebits > 16)
> > > >> + *val = st->data.buf32[ch];
> > > >> + else
> > > >> + *val = st->data.buf16[ch];
> > > >> + return 0;
> > > >
> > > > Arrggh...
> > > > I messed up here.
> > > > Guillaume found a bug here, where this should be "goto error_ret" or
> > > > do an "if () {} else {}"
> > > > How should we do it here?
> > if / else. Goto an error label when it's not an error would be horrible!
> > > >
> > > > Do we send a fix-patch or send a new series?
> > > >
> > >
> > > Since this patch is already applied, just follow up with another
> > > patch with a Fixes: tag.
> >
> > I sometimes tweak these sort of things if I haven't pushed out
> > as togreg yet (or they are really bad!) but in this case I'm not
> > 100% sure what the comment is, so I'll just apply a fix on top.
> >
> > So David is entirely correct in general but by luck of timing
> > this time I'll tweak it.
> >
> > Please check the result in iio.git/testing
> > I'll hold off pushing that out as togreg until at least end of
> > tomorrow. One more day o
>
> The "return 0" needs to be removed in the driver.
>
> if (chan->scan_type.sign == 'u') {
> if (storagebits > 16)
> *val = st->data.buf32[ch];
> else
> *val = st->data.buf16[ch];
> - return 0;
Doh!. Just goes to show why I shouldn't just edit these things.
Stupid mistake. I'll fix when on right machine.
Jonathan
> } else {
> if (storagebits > 16)
> *val = sign_extend32(st->data.buf32[ch], 17);
> else
> *val = sign_extend32(st->data.buf16[ch], 15);
> }
>
>
>
> >
> > Jonathan
> >
> >
> > >
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-10-04 13:54 ` Jonathan Cameron
@ 2024-10-06 10:56 ` Jonathan Cameron
2024-10-07 7:00 ` Alexandru Ardelean
0 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2024-10-06 10:56 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Alexandru Ardelean, David Lechner, linux-iio, linux-kernel,
devicetree, krzk+dt, robh, lars, michael.hennerich, gstols
On Fri, 4 Oct 2024 14:54:30 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> On Wed, 2 Oct 2024 09:12:09 +0300
> Alexandru Ardelean <aardelean@baylibre.com> wrote:
>
> > On Tue, Oct 1, 2024 at 9:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Tue, 1 Oct 2024 08:42:23 -0500
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >
> > > > On 10/1/24 3:26 AM, Alexandru Ardelean wrote:
> > > > > On Thu, Sep 19, 2024 at 4:05 PM Alexandru Ardelean
> > > > > <aardelean@baylibre.com> wrote:
> > > > >>
> > > >
> > > > ...
> > > >
> > > > >> @@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> > > > >> if (ret)
> > > > >> goto error_ret;
> > > > >>
> > > > >> - *val = sign_extend32(st->data[ch], 15);
> > > > >> + chan = &indio_dev->channels[ch + 1];
> > > > >> + if (chan->scan_type.sign == 'u') {
> > > > >> + if (storagebits > 16)
> > > > >> + *val = st->data.buf32[ch];
> > > > >> + else
> > > > >> + *val = st->data.buf16[ch];
> > > > >> + return 0;
> > > > >
> > > > > Arrggh...
> > > > > I messed up here.
> > > > > Guillaume found a bug here, where this should be "goto error_ret" or
> > > > > do an "if () {} else {}"
> > > > > How should we do it here?
> > > if / else. Goto an error label when it's not an error would be horrible!
> > > > >
> > > > > Do we send a fix-patch or send a new series?
> > > > >
> > > >
> > > > Since this patch is already applied, just follow up with another
> > > > patch with a Fixes: tag.
> > >
> > > I sometimes tweak these sort of things if I haven't pushed out
> > > as togreg yet (or they are really bad!) but in this case I'm not
> > > 100% sure what the comment is, so I'll just apply a fix on top.
> > >
> > > So David is entirely correct in general but by luck of timing
> > > this time I'll tweak it.
> > >
> > > Please check the result in iio.git/testing
> > > I'll hold off pushing that out as togreg until at least end of
> > > tomorrow. One more day o
> >
> > The "return 0" needs to be removed in the driver.
> >
> > if (chan->scan_type.sign == 'u') {
> > if (storagebits > 16)
> > *val = st->data.buf32[ch];
> > else
> > *val = st->data.buf16[ch];
> > - return 0;
> Doh!. Just goes to show why I shouldn't just edit these things.
> Stupid mistake. I'll fix when on right machine.
hopefully now done
J
>
> Jonathan
>
> > } else {
> > if (storagebits > 16)
> > *val = sign_extend32(st->data.buf32[ch], 17);
> > else
> > *val = sign_extend32(st->data.buf16[ch], 15);
> > }
> >
> >
> >
> > >
> > > Jonathan
> > >
> > >
> > > >
> > > >
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 8/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts
2024-10-06 10:56 ` Jonathan Cameron
@ 2024-10-07 7:00 ` Alexandru Ardelean
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-10-07 7:00 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, David Lechner, linux-iio, linux-kernel,
devicetree, krzk+dt, robh, lars, michael.hennerich, gstols
On Sun, Oct 6, 2024 at 1:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 4 Oct 2024 14:54:30 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>
> > On Wed, 2 Oct 2024 09:12:09 +0300
> > Alexandru Ardelean <aardelean@baylibre.com> wrote:
> >
> > > On Tue, Oct 1, 2024 at 9:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > >
> > > > On Tue, 1 Oct 2024 08:42:23 -0500
> > > > David Lechner <dlechner@baylibre.com> wrote:
> > > >
> > > > > On 10/1/24 3:26 AM, Alexandru Ardelean wrote:
> > > > > > On Thu, Sep 19, 2024 at 4:05 PM Alexandru Ardelean
> > > > > > <aardelean@baylibre.com> wrote:
> > > > > >>
> > > > >
> > > > > ...
> > > > >
> > > > > >> @@ -153,7 +349,19 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> > > > > >> if (ret)
> > > > > >> goto error_ret;
> > > > > >>
> > > > > >> - *val = sign_extend32(st->data[ch], 15);
> > > > > >> + chan = &indio_dev->channels[ch + 1];
> > > > > >> + if (chan->scan_type.sign == 'u') {
> > > > > >> + if (storagebits > 16)
> > > > > >> + *val = st->data.buf32[ch];
> > > > > >> + else
> > > > > >> + *val = st->data.buf16[ch];
> > > > > >> + return 0;
> > > > > >
> > > > > > Arrggh...
> > > > > > I messed up here.
> > > > > > Guillaume found a bug here, where this should be "goto error_ret" or
> > > > > > do an "if () {} else {}"
> > > > > > How should we do it here?
> > > > if / else. Goto an error label when it's not an error would be horrible!
> > > > > >
> > > > > > Do we send a fix-patch or send a new series?
> > > > > >
> > > > >
> > > > > Since this patch is already applied, just follow up with another
> > > > > patch with a Fixes: tag.
> > > >
> > > > I sometimes tweak these sort of things if I haven't pushed out
> > > > as togreg yet (or they are really bad!) but in this case I'm not
> > > > 100% sure what the comment is, so I'll just apply a fix on top.
> > > >
> > > > So David is entirely correct in general but by luck of timing
> > > > this time I'll tweak it.
> > > >
> > > > Please check the result in iio.git/testing
> > > > I'll hold off pushing that out as togreg until at least end of
> > > > tomorrow. One more day o
> > >
> > > The "return 0" needs to be removed in the driver.
> > >
> > > if (chan->scan_type.sign == 'u') {
> > > if (storagebits > 16)
> > > *val = st->data.buf32[ch];
> > > else
> > > *val = st->data.buf16[ch];
> > > - return 0;
> > Doh!. Just goes to show why I shouldn't just edit these things.
> > Stupid mistake. I'll fix when on right machine.
> hopefully now done
Looks good now.
Apologies for the slow reply.
Thanks
Alex
>
> J
> >
> > Jonathan
> >
> > > } else {
> > > if (storagebits > 16)
> > > *val = sign_extend32(st->data.buf32[ch], 17);
> > > else
> > > *val = sign_extend32(st->data.buf16[ch], 15);
> > > }
> > >
> > >
> > >
> > > >
> > > > Jonathan
> > > >
> > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 5/8] iio: adc: ad7606: rework available attributes for SW channels
2024-09-19 13:04 ` [PATCH v7 5/8] iio: adc: ad7606: rework available attributes for SW channels Alexandru Ardelean
@ 2024-10-17 17:50 ` Jonathan Cameron
2024-10-19 16:15 ` Alexandru Ardelean
0 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2024-10-17 17:50 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-iio, linux-kernel, devicetree, krzk+dt, robh, lars,
michael.hennerich, gstols, dlechner
On Thu, 19 Sep 2024 16:04:40 +0300
Alexandru Ardelean <aardelean@baylibre.com> wrote:
> For SW mode, the oversampling and scales attributes are always present.
> So, they can be implemented via a 'read_avail' hook in iio_info.
>
> For HW mode, it's a bit tricky, as these attributes get assigned based on
> GPIO definitions.
>
> So, for SW mode, we define a separate AD7606_SW_CHANNEL() macro, and use
> that for the SW channels.
> And 'ad7606_info_os_range_and_debug' can be renamed to
> 'ad7606_info_sw_mode' as it is only used for SW mode.
>
> For the 'read_avail' hook, we'll need to allocate the SW scales, so that
> they are just returned userspace without any extra processing.
> The allocation will happen when then ad7606_state struct is allocated.
> The oversampling available parameters don't need any extra processing; they
> can just be passed back to userspace (as they are).
>
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
> drivers/iio/adc/ad7606.c | 52 ++++++++++++++++++++++++++++++++++---
> drivers/iio/adc/ad7606.h | 32 ++++++++++++++++++++---
> drivers/iio/adc/ad7606_spi. | 0
Magic empty file :(
Sending a patch shortly and already queued up.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 5/8] iio: adc: ad7606: rework available attributes for SW channels
2024-10-17 17:50 ` Jonathan Cameron
@ 2024-10-19 16:15 ` Alexandru Ardelean
0 siblings, 0 replies; 22+ messages in thread
From: Alexandru Ardelean @ 2024-10-19 16:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, linux-kernel, devicetree, krzk+dt, robh, lars,
michael.hennerich, gstols, dlechner
On Thu, Oct 17, 2024 at 8:51 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 19 Sep 2024 16:04:40 +0300
> Alexandru Ardelean <aardelean@baylibre.com> wrote:
>
> > For SW mode, the oversampling and scales attributes are always present.
> > So, they can be implemented via a 'read_avail' hook in iio_info.
> >
> > For HW mode, it's a bit tricky, as these attributes get assigned based on
> > GPIO definitions.
> >
> > So, for SW mode, we define a separate AD7606_SW_CHANNEL() macro, and use
> > that for the SW channels.
> > And 'ad7606_info_os_range_and_debug' can be renamed to
> > 'ad7606_info_sw_mode' as it is only used for SW mode.
> >
> > For the 'read_avail' hook, we'll need to allocate the SW scales, so that
> > they are just returned userspace without any extra processing.
> > The allocation will happen when then ad7606_state struct is allocated.
> > The oversampling available parameters don't need any extra processing; they
> > can just be passed back to userspace (as they are).
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
> > drivers/iio/adc/ad7606.c | 52 ++++++++++++++++++++++++++++++++++---
> > drivers/iio/adc/ad7606.h | 32 ++++++++++++++++++++---
> > drivers/iio/adc/ad7606_spi. | 0
>
> Magic empty file :(
oops
apologies for that;
this looks like it could be a job for a CI check (or checkpatch?)
>
> Sending a patch shortly and already queued up.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2024-10-19 16:15 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-19 13:04 [PATCH v7 0/8] iio: adc: ad7606: add support for AD7606C-{16,18} parts Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 1/8] iio: adc: ad7606: add 'bits' parameter to channels macros Alexandru Ardelean
2024-09-23 14:50 ` David Lechner
2024-09-24 8:11 ` Alexandru Ardelean
2024-09-28 14:43 ` Jonathan Cameron
2024-09-19 13:04 ` [PATCH v7 2/8] iio: adc: ad7606: move 'val' pointer to ad7606_scan_direct() Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 3/8] iio: adc: ad7606: move scale_setup as function pointer on chip-info Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 4/8] iio: adc: ad7606: wrap channel ranges & scales into struct Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 5/8] iio: adc: ad7606: rework available attributes for SW channels Alexandru Ardelean
2024-10-17 17:50 ` Jonathan Cameron
2024-10-19 16:15 ` Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 6/8] dt-bindings: iio: adc: document diff-channels corner case for some ADCs Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 7/8] dt-bindings: iio: adc: add docs for AD7606C-{16,18} parts Alexandru Ardelean
2024-09-19 13:04 ` [PATCH v7 8/8] iio: adc: ad7606: add support " Alexandru Ardelean
2024-10-01 8:26 ` Alexandru Ardelean
2024-10-01 9:26 ` Guillaume Stols
2024-10-01 13:42 ` David Lechner
2024-10-01 18:41 ` Jonathan Cameron
2024-10-02 6:12 ` Alexandru Ardelean
2024-10-04 13:54 ` Jonathan Cameron
2024-10-06 10:56 ` Jonathan Cameron
2024-10-07 7:00 ` Alexandru Ardelean
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).