Add support for AD7091R-2/-4/-8

PATCH Superseded Marcelo Schmitt 7 participants 34 messages Dec 7, 2023 – Dec 10, 2023

Subsystems (5)

Device tree bindings IIO Analog Devices IIO drivers Checkpatch +1 more subsystems

lore

Share & tools

lore

Marcelo Schmitt

author maintainer Applied

2023-12-07 18:36 UTC #0 lore raw

v3 cover Add support for AD7091R-2/-4/-8

From: Marcelo Schmitt <[email protected]>

----------------- Updates -----------------

Applied all changes suggested to the previous series.

I tried to better explain the changes but, since there is a fair amount of rework in ad7091-base and ad7091r5, it may be hard to get the reasoning for the early patches before looking at the patch for ad7091r8.

Change log v2 -> v3:
- Split alert fix patch into 2 fix patches and one alignment cleanup patch
- Corrected Fixes tag format
- Moved MAINTAINERS update to the end of the series
- Reworded some commit messages to provide context and make their goal clearer
- Removed erroneous gmail sign off
- Created container struct to store chip_info, regmap_config, and callbacks specific to each ADC design
- Created callbacks for chip specific tasks such as setting device operation mode
- Dropped the chip type enum struct
- Applied suggestions related to device tree documentation
- Added __aligned to list the of checkpatch attribute notes
- Other code style tidy ups.

I see regmap's interface for reading device registers under /sys/kernel/debug/regmap/. I can read all registers but can't write to any of them unless I force define REGMAP_ALLOW_WRITE_DEBUGFS.

When testing events for this driver I often write to device registers to set different rising/falling thresholds. I do something like this:

# echo 0x17 0x100 > /sys/kernel/debug/iio/iio:device0/direct_reg_access

I tried read/writing to files under iio:device events directory but always get segmentation fault. I must be forgetting to implement something. What am I missing?

Thanks
Marcelo

----------------- Context -----------------

This series adds support for AD7091R-2/-4/-8 ADCs which can do single shot or sequenced readings. Threshold events are also supported.
Overall, AD7091R-2/-4/-8 are very similar to AD7091R-5 except they use SPI interface.

Changes have been tested with raspberrypi and eval board on raspberrypi kernel 6.7-rc3 from raspberrypi fork.

Link https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad7091r8

Marcelo Schmitt (13):
scripts: checkpatch: Add __aligned to the list of attribute notes
iio: adc: ad7091r: Populate device driver data field
iio: adc: ad7091r: Set alert bit in config register
iio: adc: ad7091r: Align arguments to function call parenthesis
iio: adc: ad7091r: Move generic AD7091R code to base driver and header
file
iio: adc: ad7091r: Move chip init data to container struct
iio: adc: ad7091r: Set device mode through chip_info callback
iio: adc: ad7091r: Enable internal vref if external vref is not
supplied
iio: adc: ad7091r: Add chip_info callback to get conversion result
channel
dt-bindings: iio: Add AD7091R-8
iio: adc: Split AD7091R-5 config symbol
iio: adc: Add support for AD7091R-8
MAINTAINERS: Add MAINTAINERS entry for AD7091R

 .../bindings/iio/adc/adi,ad7091r8.yaml        |  99 +++++++
 MAINTAINERS                                   |  12 +
 drivers/iio/adc/Kconfig                       |  16 ++
 drivers/iio/adc/Makefile                      |   4 +-
 drivers/iio/adc/ad7091r-base.c                | 141 ++++------
 drivers/iio/adc/ad7091r-base.h                |  78 +++++-
 drivers/iio/adc/ad7091r5.c                    | 119 ++++----
 drivers/iio/adc/ad7091r8.c                    | 261 ++++++++++++++++++
 scripts/checkpatch.pl                         |   1 +
 9 files changed, 597 insertions(+), 134 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
 create mode 100644 drivers/iio/adc/ad7091r8.c

Signature · 1 line

2.42.0

Marcelo Schmitt

author maintainer

2023-12-07 18:37 UTC #1 lore raw

PATCH v3 1/13 scripts: checkpatch: Add __aligned to the list of attribute notes

Checkpatch presumes attributes marked with __aligned(alignment) are part of a function declaration and throws a warning stating that those compiler attributes should have an identifier name which is not correct. Add __aligned compiler attributes to the list of attribute notes so they don't cause warnings anymore.

Signed-off-by Marcelo Schmitt <[email protected]>

---
The patch that would trigger the mentioned checkpatch warning in this series is patch number 12 (iio: adc: Add support for AD7091R-8).

 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

scripts/checkpatch.pl +1 -0

index 25fdb7fda112..e6773ae0ad08 100755
@@ -512,6 +512,7 @@ our $Attribute	= qr{
 			__ro_after_init|
 			__kprobes|
 			$InitAttribute|
+			__aligned|
 			____cacheline_aligned|
 			____cacheline_aligned_in_smp|
 			____cacheline_internodealigned_in_smp|

Signature · 1 line

2.42.0

Joe Perches

maintainer

2023-12-07 18:56 UTC #2 lore raw

Re: [PATCH v3 01/13] scripts: checkpatch: Add __aligned to the list of attribute notes

Quoted 19 lines · Marcelo Schmitt

Checkpatch presumes attributes marked with __aligned(alignment) are part of a function declaration and throws a warning stating that those compiler attributes should have an identifier name which is not correct. Add __aligned compiler attributes to the list of attribute notes so they don't cause warnings anymore.
Signed-off-by Marcelo Schmitt <[email protected]>
---
The patch that would trigger the mentioned checkpatch warning in this series is patch number 12 (iio: adc: Add support for AD7091R-8).
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)
scripts/checkpatch.pl +0 -0
index 25fdb7fda112..e6773ae0ad08 100755

scripts/checkpatch.pl our $Attribute = qr{ · line 515

Quoted 4 lines

scripts/checkpatch.pl +1 -0

@@ -512,6 +512,7 @@ our $Attribute	= qr{
 			__ro_after_init|
 			__kprobes|
 			$InitAttribute|
+			__aligned|

__aligned takes an argument so I think there needs to have something like the use of __alloc_size below this addition

	__alloc_size\s*\(\s*\d+\s*(?:,\s*\d+\s*)?\)

maybe

			__aligned\s*\([^\)]*\)

though even that would work well with most uses it would not work with things like

drivers/crypto/inside-secure/safexcel_hash.c: u8 cache[HASH_CACHE_SIZE] __aligned(sizeof(u32));

Marcelo Schmitt

2023-12-08 12:21 UTC #3 lore raw

Re: [PATCH v3 01/13] scripts: checkpatch: Add __aligned to the list of attribute notes

Quoted 30 lines · Joe Perches

Quoted 19 lines · Marcelo Schmitt
Checkpatch presumes attributes marked with __aligned(alignment) are part of a function declaration and throws a warning stating that those compiler attributes should have an identifier name which is not correct. Add __aligned compiler attributes to the list of attribute notes so they don't cause warnings anymore.
Signed-off-by Marcelo Schmitt <[email protected]>
---
The patch that would trigger the mentioned checkpatch warning in this series is patch number 12 (iio: adc: Add support for AD7091R-8).
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)
scripts/checkpatch.pl +0 -0
index 25fdb7fda112..e6773ae0ad08 100755
scripts/checkpatch.pl our $Attribute = qr{ · line 515
Quoted 4 lines
scripts/checkpatch.pl +1 -0
@@ -512,6 +512,7 @@ our $Attribute	= qr{
 			__ro_after_init|
 			__kprobes|
 			$InitAttribute|
+			__aligned|
__aligned takes an argument so I think there needs to have something like the use of __alloc_size below this addition
	__alloc_size\s*$\s*\d+\s*(?:,\s*\d+\s*)?$

Quoted 9 lines

maybe
			__aligned\s*$[^$]*\)
though even that would work well with most uses it would not work with things like
drivers/crypto/inside-secure/safexcel_hash.c: u8 cache[HASH_CACHE_SIZE] __aligned(sizeof(u32));

Will think of something that may work for all cases and include in v4.

Thanks

Marcelo Schmitt

author maintainer

2023-12-07 18:38 UTC #4 lore raw

PATCH v3 2/13 iio: adc: ad7091r: Populate device driver data field

In reply to Marcelo Schmitt

Set device driver data so it can be retrieved when handling alert events, avoiding null pointer dereference.

Fixes ca69300173b6 ("iio: adc: Add support for AD7091R5 ADC")

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iio/adc/ad7091r-base.c +1 -0

index 8e252cde735b..0f192fbecbd4 100644
@@ -232,6 +232,7 @@ int ad7091r_probe(struct device *dev, const char *name,
 	iio_dev->channels = chip_info->channels;
 
 	if (irq) {
+		dev_set_drvdata(st->dev, iio_dev);
 		ret = devm_request_threaded_irq(dev, irq, NULL,
 				ad7091r_event_handler,
 				IRQF_TRIGGER_FALLING | IRQF_ONESHOT, name, st);

Signature · 1 line

2.42.0

David Lechner

reviewer

2023-12-07 23:18 UTC #5 lore raw

Re: [PATCH v3 02/13] iio: adc: ad7091r: Populate device driver data field

Quoted 23 lines · Marcelo Schmitt

Set device driver data so it can be retrieved when handling alert events, avoiding null pointer dereference.

Fixes ca69300173b6 ("iio: adc: Add support for AD7091R5 ADC")

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iio/adc/ad7091r-base.c +1 -1

index 8e252cde735b..0f192fbecbd4 100644
@@ -232,6 +232,7 @@ int ad7091r_probe(struct device *dev, const char *name,
        iio_dev->channels = chip_info->channels;
 
        if (irq) {
+               dev_set_drvdata(st->dev, iio_dev);
                ret = devm_request_threaded_irq(dev, irq, NULL,
                                ad7091r_event_handler,
                                IRQF_TRIGGER_FALLING | IRQF_ONESHOT, name, st);
--

Quoted 3 lines

2.42.0

Instead of introducing a new relationship between iio_dev and st, why not pass iio_dev to devm_request_threaded_irq() instead of st and then use iio_priv() to get st in ad7091r_event_handler?

drivers/iio/adc/ad7091r-base.c +3 -3

index 8e252cde735b..0e5d3d2e9c98 100644
@@ -174,8 +174,8 @@ static const struct iio_info ad7091r_info = {
 
 static irqreturn_t ad7091r_event_handler(int irq, void *private)
 {
-    struct ad7091r_state *st = (struct ad7091r_state *) private;
-    struct iio_dev *iio_dev = dev_get_drvdata(st->dev);
+    struct iio_dev *iio_dev = private;
+    struct ad7091r_state *st = iio_priv(iio_dev);
     unsigned int i, read_val;
     int ret;
     s64 timestamp = iio_get_time_ns(iio_dev);

@@ -234,7 +234,7 @@ int ad7091r_probe(struct device *dev, const char *name,
     if (irq) {
         ret = devm_request_threaded_irq(dev, irq, NULL,
                 ad7091r_event_handler,
-                IRQF_TRIGGER_FALLING | IRQF_ONESHOT, name, st);
+                IRQF_TRIGGER_FALLING | IRQF_ONESHOT, name, iio_dev);
         if (ret)
             return ret;
     }

Marcelo Schmitt

2023-12-08 12:16 UTC #6 lore raw

Re: [PATCH v3 02/13] iio: adc: ad7091r: Populate device driver data field

Quoted 6 lines · David Lechner

Quoted 4 lines · Marcelo Schmitt
Set device driver data so it can be retrieved when handling alert events, avoiding null pointer dereference.

[...]

Quoted 21 lines

drivers/iio/adc/ad7091r-base.c +2 -2

index 8e252cde735b..0e5d3d2e9c98 100644
@@ -174,8 +174,8 @@ static const struct iio_info ad7091r_info = {
 
 static irqreturn_t ad7091r_event_handler(int irq, void *private)
 {
-    struct ad7091r_state *st = (struct ad7091r_state *) private;
-    struct iio_dev *iio_dev = dev_get_drvdata(st->dev);
+    struct iio_dev *iio_dev = private;
+    struct ad7091r_state *st = iio_priv(iio_dev);
     unsigned int i, read_val;
     int ret;
     s64 timestamp = iio_get_time_ns(iio_dev);

drivers/iio/adc/ad7091r-base.c int ad7091r_probe(struct device *dev, const char *name, · line 240

Quoted 8 lines

drivers/iio/adc/ad7091r-base.c +1 -1

@@ -234,7 +234,7 @@ int ad7091r_probe(struct device *dev, const char *name,
     if (irq) {
         ret = devm_request_threaded_irq(dev, irq, NULL,
                 ad7091r_event_handler,
-                IRQF_TRIGGER_FALLING | IRQF_ONESHOT, name, st);
+                IRQF_TRIGGER_FALLING | IRQF_ONESHOT, name, iio_dev);
         if (ret)
             return ret;
     }

Looks good, will do for v4.

Thanks

Marcelo Schmitt

author maintainer

2023-12-07 18:39 UTC #7 lore raw

PATCH v3 3/13 iio: adc: ad7091r: Set alert bit in config register

In reply to Marcelo Schmitt

The ad7091r-base driver sets up an interrupt handler for firing events when inputs are either above or below a certain threshold. However, for the interrupt signal to come from the device it must be configured to enable the ALERT/BUSY/GPO pin to be used as ALERT, which was not being done until now.
Enable interrupt signals on the ALERT/BUSY/GPO pin by setting the proper bit in the configuration register.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/iio/adc/ad7091r-base.c +6 -0

index 0f192fbecbd4..6056a66d756c 100644
@@ -28,6 +28,7 @@
 #define AD7091R_REG_RESULT_CONV_RESULT(x)   ((x) & 0xfff)
 
 /* AD7091R_REG_CONF */
+#define AD7091R_REG_CONF_ALERT_EN   BIT(4)
 #define AD7091R_REG_CONF_AUTO   BIT(8)
 #define AD7091R_REG_CONF_CMD    BIT(10)

@@ -232,6 +233,11 @@ int ad7091r_probe(struct device *dev, const char *name,
 	iio_dev->channels = chip_info->channels;
 
 	if (irq) {
+		ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
+					 AD7091R_REG_CONF_ALERT_EN, BIT(4));
+		if (ret)
+			return ret;
+
 		dev_set_drvdata(st->dev, iio_dev);
 		ret = devm_request_threaded_irq(dev, irq, NULL,
 				ad7091r_event_handler,

Signature · 1 line

2.42.0

Jonathan Cameron

maintainer

2023-12-10 12:14 UTC #8 lore raw

Re: [PATCH v3 03/13] iio: adc: ad7091r: Set alert bit in config register

Quoted 9 lines · On Thu, 7 Dec 2023 15:38:53 -0300 Marcelo Schmitt <[email protected]>…

The ad7091r-base driver sets up an interrupt handler for firing events when inputs are either above or below a certain threshold. However, for the interrupt signal to come from the device it must be configured to enable the ALERT/BUSY/GPO pin to be used as ALERT, which was not being done until now.
Enable interrupt signals on the ALERT/BUSY/GPO pin by setting the proper bit in the configuration register.
Signed-off-by Marcelo Schmitt <[email protected]>

Also a fix? Feels like people expect this to work but I guess we could in theory call it a 'feature' given it never did :)

Jonathan

Quoted 28 lines

---
 drivers/iio/adc/ad7091r-base.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/iio/adc/ad7091r-base.c +6 -0

index 0f192fbecbd4..6056a66d756c 100644
@@ -28,6 +28,7 @@
 #define AD7091R_REG_RESULT_CONV_RESULT(x)   ((x) & 0xfff)
 
 /* AD7091R_REG_CONF */
+#define AD7091R_REG_CONF_ALERT_EN   BIT(4)
 #define AD7091R_REG_CONF_AUTO   BIT(8)
 #define AD7091R_REG_CONF_CMD    BIT(10)

@@ -232,6 +233,11 @@ int ad7091r_probe(struct device *dev, const char *name,
 	iio_dev->channels = chip_info->channels;
 
 	if (irq) {
+		ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
+					 AD7091R_REG_CONF_ALERT_EN, BIT(4));
+		if (ret)
+			return ret;
+
 		dev_set_drvdata(st->dev, iio_dev);
 		ret = devm_request_threaded_irq(dev, irq, NULL,
 				ad7091r_event_handler,

Marcelo Schmitt

author maintainer

2023-12-07 18:40 UTC #9 lore raw

PATCH v3 4/13 iio: adc: ad7091r: Align arguments to function call parenthesis

In reply to Marcelo Schmitt

Align arguments to function call open parenthesis to comply with the Linux kernel coding style.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/iio/adc/ad7091r-base.c +3 -2

index 6056a66d756c..3ecac3164446 100644
@@ -240,8 +240,9 @@ int ad7091r_probe(struct device *dev, const char *name,
 
 		dev_set_drvdata(st->dev, iio_dev);
 		ret = devm_request_threaded_irq(dev, irq, NULL,
-				ad7091r_event_handler,
-				IRQF_TRIGGER_FALLING | IRQF_ONESHOT, name, st);
+						ad7091r_event_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT, name, st);
 		if (ret)
 			return ret;
 	}

Signature · 1 line

2.42.0

Marcelo Schmitt

author maintainer

2023-12-07 18:40 UTC #10 lore raw

PATCH v3 5/13 iio: adc: ad7091r: Move generic AD7091R code to base driver and header file

In reply to Marcelo Schmitt

Some code generic to AD7091R devices such as channel definitions and event spec structs were in the AD7091R-5 driver.
There was also some generic register definitions declared in the base driver which would make more sense to be in the header file. The device state struct will be needed for the ad7091r8 driver in a follow up patch so that ought to be moved to the header file as well. Lastly, a couple of regmap callback functions are also capable of abstracting characteristics of different AD7091R devices and those are now being exported to IIO_AD7091R name space.

Move AD7091R generic code either to the base driver or to the header file so both the ad7091r5 and the ad7091r8 driver can use those declaration in follow up patches.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 46 +++++++++++++++++-----------------
 drivers/iio/adc/ad7091r-base.h | 42 ++++++++++++++++++++++++++++++-
 drivers/iio/adc/ad7091r5.c     | 39 +++-------------------------
 3 files changed, 68 insertions(+), 59 deletions(-)

drivers/iio/adc/ad7091r-base.c +23 ~~-23~~

index 3ecac3164446..4d0a1eeebb8a 100644
@@ -15,14 +15,6 @@
 
 #include "ad7091r-base.h"
 
-#define AD7091R_REG_RESULT  0
-#define AD7091R_REG_CHANNEL 1
-#define AD7091R_REG_CONF    2
-#define AD7091R_REG_ALERT   3
-#define AD7091R_REG_CH_LOW_LIMIT(ch) ((ch) * 3 + 4)
-#define AD7091R_REG_CH_HIGH_LIMIT(ch) ((ch) * 3 + 5)
-#define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6)
-
 /* AD7091R_REG_RESULT */
 #define AD7091R_REG_RESULT_CH_ID(x)	    (((x) >> 13) & 0x3)
 #define AD7091R_REG_RESULT_CONV_RESULT(x)   ((x) & 0xfff)

@@ -35,20 +27,26 @@
 #define AD7091R_REG_CONF_MODE_MASK  \
 	(AD7091R_REG_CONF_AUTO | AD7091R_REG_CONF_CMD)
 
-enum ad7091r_mode {
-	AD7091R_MODE_SAMPLE,
-	AD7091R_MODE_COMMAND,
-	AD7091R_MODE_AUTOCYCLE,
-};
-
-struct ad7091r_state {
-	struct device *dev;
-	struct regmap *map;
-	struct regulator *vref;
-	const struct ad7091r_chip_info *chip_info;
-	enum ad7091r_mode mode;
-	struct mutex lock; /*lock to prevent concurent reads */
+const struct iio_event_spec ad7091r_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_HYSTERESIS),
+	},
 };
+EXPORT_SYMBOL_NS_GPL(ad7091r_events, IIO_AD7091R);
 
 static int ad7091r_set_mode(struct ad7091r_state *st, enum ad7091r_mode mode)
 {

@@ -270,7 +268,7 @@ int ad7091r_probe(struct device *dev, const char *name,
 }
 EXPORT_SYMBOL_NS_GPL(ad7091r_probe, IIO_AD7091R);
 
-static bool ad7091r_writeable_reg(struct device *dev, unsigned int reg)
+bool ad7091r_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case AD7091R_REG_RESULT:

@@ -280,8 +278,9 @@ static bool ad7091r_writeable_reg(struct device *dev, unsigned int reg)
 		return true;
 	}
 }
+EXPORT_SYMBOL_NS_GPL(ad7091r_writeable_reg, IIO_AD7091R);
 
-static bool ad7091r_volatile_reg(struct device *dev, unsigned int reg)
+bool ad7091r_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case AD7091R_REG_RESULT:

@@ -291,6 +290,7 @@ static bool ad7091r_volatile_reg(struct device *dev, unsigned int reg)
 		return false;
 	}
 }
+EXPORT_SYMBOL_NS_GPL(ad7091r_volatile_reg, IIO_AD7091R);
 
 const struct regmap_config ad7091r_regmap_config = {
 	.reg_bits = 8,

drivers/iio/adc/ad7091r-base.h +41 -1

59 lines not on this page · raw message

drivers/iio/adc/ad7091r5.c +4 ~~-35~~

index 2f048527b7b7..9d3ccfca94ec 100644
@@ -12,42 +12,11 @@
 
 #include "ad7091r-base.h"
 
-static const struct iio_event_spec ad7091r5_events[] = {
-	{
-		.type = IIO_EV_TYPE_THRESH,
-		.dir = IIO_EV_DIR_RISING,
-		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-				 BIT(IIO_EV_INFO_ENABLE),
-	},
-	{
-		.type = IIO_EV_TYPE_THRESH,
-		.dir = IIO_EV_DIR_FALLING,
-		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
-				 BIT(IIO_EV_INFO_ENABLE),
-	},
-	{
-		.type = IIO_EV_TYPE_THRESH,
-		.dir = IIO_EV_DIR_EITHER,
-		.mask_separate = BIT(IIO_EV_INFO_HYSTERESIS),
-	},
-};
-
-#define AD7091R_CHANNEL(idx, bits, ev, num_ev) { \
-	.type = IIO_VOLTAGE, \
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
-	.indexed = 1, \
-	.channel = idx, \
-	.event_spec = ev, \
-	.num_event_specs = num_ev, \
-	.scan_type.storagebits = 16, \
-	.scan_type.realbits = bits, \
-}
 static const struct iio_chan_spec ad7091r5_channels_irq[] = {
-	AD7091R_CHANNEL(0, 12, ad7091r5_events, ARRAY_SIZE(ad7091r5_events)),
-	AD7091R_CHANNEL(1, 12, ad7091r5_events, ARRAY_SIZE(ad7091r5_events)),
-	AD7091R_CHANNEL(2, 12, ad7091r5_events, ARRAY_SIZE(ad7091r5_events)),
-	AD7091R_CHANNEL(3, 12, ad7091r5_events, ARRAY_SIZE(ad7091r5_events)),
+	AD7091R_CHANNEL(0, 12, ad7091r_events, ARRAY_SIZE(ad7091r_events)),
+	AD7091R_CHANNEL(1, 12, ad7091r_events, ARRAY_SIZE(ad7091r_events)),
+	AD7091R_CHANNEL(2, 12, ad7091r_events, ARRAY_SIZE(ad7091r_events)),
+	AD7091R_CHANNEL(3, 12, ad7091r_events, ARRAY_SIZE(ad7091r_events)),
 };
 
 static const struct iio_chan_spec ad7091r5_channels_noirq[] = {

Signature · 1 line

2.42.0

Marcelo Schmitt

author maintainer

2023-12-07 18:41 UTC #11 lore raw

PATCH v3 6/13 iio: adc: ad7091r: Move chip init data to container struct

In reply to Marcelo Schmitt

AD7091R designs may differ on their communication protocol and resources required for proper setup. Extract what is design specific into a init_info struct so the base driver can use data and callback functions from that struct rather than checking which specific chip is connected during device initialization.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 27 ++++++++++-----------
 drivers/iio/adc/ad7091r-base.h | 14 ++++++++---
 drivers/iio/adc/ad7091r5.c     | 43 ++++++++++++++++++++++++----------
 3 files changed, 54 insertions(+), 30 deletions(-)

drivers/iio/adc/ad7091r-base.c +12 ~~-15~~

index 4d0a1eeebb8a..90470b4a98c5 100644
@@ -207,7 +207,7 @@ static void ad7091r_remove(void *data)
 }
 
 int ad7091r_probe(struct device *dev, const char *name,
-		const struct ad7091r_chip_info *chip_info,
+		const struct ad7091r_init_info *init_info,
 		struct regmap *map, int irq)
 {
 	struct iio_dev *iio_dev;

@@ -220,17 +220,16 @@ int ad7091r_probe(struct device *dev, const char *name,
 
 	st = iio_priv(iio_dev);
 	st->dev = dev;
-	st->chip_info = chip_info;
-	st->map = map;
+	init_info->ad7091r_regmap_init(st, init_info->regmap_config);
+	if (IS_ERR(st->map))
+		return dev_err_probe(st->dev, PTR_ERR(st->map),
+				     "Error initializing regmap\n");
 
-	iio_dev->name = name;
 	iio_dev->info = &ad7091r_info;
 	iio_dev->modes = INDIO_DIRECT_MODE;
 
-	iio_dev->num_channels = chip_info->num_channels;
-	iio_dev->channels = chip_info->channels;
-
 	if (irq) {
+		st->chip_info = &init_info->irq_info;
 		ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
 					 AD7091R_REG_CONF_ALERT_EN, BIT(4));
 		if (ret)

@@ -243,8 +242,14 @@ int ad7091r_probe(struct device *dev, const char *name,
 						IRQF_ONESHOT, name, st);
 		if (ret)
 			return ret;
+	} else {
+		st->chip_info = &init_info->info_no_irq;
 	}
 
+	iio_dev->name = st->chip_info->name;
+	iio_dev->num_channels = st->chip_info->num_channels;
+	iio_dev->channels = st->chip_info->channels;
+
 	st->vref = devm_regulator_get_optional(dev, "vref");
 	if (IS_ERR(st->vref)) {
 		if (PTR_ERR(st->vref) == -EPROBE_DEFER)

@@ -292,14 +297,6 @@ bool ad7091r_volatile_reg(struct device *dev, unsigned int reg)
 }
 EXPORT_SYMBOL_NS_GPL(ad7091r_volatile_reg, IIO_AD7091R);
 
-const struct regmap_config ad7091r_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 16,
-	.writeable_reg = ad7091r_writeable_reg,
-	.volatile_reg = ad7091r_volatile_reg,
-};
-EXPORT_SYMBOL_NS_GPL(ad7091r_regmap_config, IIO_AD7091R);
-
 MODULE_AUTHOR("Beniamin Bia <[email protected]>");
 MODULE_DESCRIPTION("Analog Devices AD7091Rx multi-channel converters");
 MODULE_LICENSE("GPL v2");

drivers/iio/adc/ad7091r-base.h +11 -3

index 1d30eeb46bcc..d58e2b08015a 100644
@@ -8,6 +8,8 @@
 #ifndef __DRIVERS_IIO_ADC_AD7091R_BASE_H__
 #define __DRIVERS_IIO_ADC_AD7091R_BASE_H__
 
+#include <linux/regmap.h>
+
 #define AD7091R_REG_RESULT  0
 #define AD7091R_REG_CHANNEL 1
 #define AD7091R_REG_CONF    2

@@ -52,12 +54,18 @@ struct ad7091r_chip_info {
 	unsigned int vref_mV;
 };
 
-extern const struct iio_event_spec ad7091r_events[3];
+struct ad7091r_init_info {
+	struct ad7091r_chip_info irq_info;
+	struct ad7091r_chip_info info_no_irq;
+	const struct regmap_config *regmap_config;
+	void (*ad7091r_regmap_init)(struct ad7091r_state *st,
+				    const struct regmap_config *regmap_conf);
+};
 
-extern const struct regmap_config ad7091r_regmap_config;
+extern const struct iio_event_spec ad7091r_events[3];
 
 int ad7091r_probe(struct device *dev, const char *name,
-		const struct ad7091r_chip_info *chip_info,
+		const struct ad7091r_init_info *init_info,
 		struct regmap *map, int irq);
 
 bool ad7091r_volatile_reg(struct device *dev, unsigned int reg);

drivers/iio/adc/ad7091r5.c +31 ~~-12~~

73 lines not on this page · raw message

Signature · 1 line

2.42.0

kernel test robot

build bot

2023-12-08 12:28 UTC #12 lore raw

Re: [PATCH v3 6/13] iio: adc: ad7091r: Move chip init data to container struct

kernel test robot noticed the following build errors:

Hi Marcelo,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.7-rc4 next-20231208] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marcelo-Schmitt/scripts-checkpatch-Add-__aligned-to-the-list-of-attribute-notes/20231208-063850
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/f45d5dfde5fc2082ac1fcac18a4a3e9b4b941402.1701971344.git.marcelo.schmitt1%40gmail.com
patch subject: [PATCH v3 6/13] iio: adc: ad7091r: Move chip init data to container struct
config: arm-randconfig-002-20231208 (https://download.01.org/0day-ci/archive/20231208/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231208/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

drivers/iio/adc/ad7091r5.c:30:3: error: field designator 'name' does not refer to any field in type 'const struct ad7091r_chip_info'

           .name = "ad7091r-5",
            ^
   drivers/iio/adc/ad7091r5.c:37:3: error: field designator 'name' does not refer to any field in type 'const struct ad7091r_chip_info'
           .name = "ad7091r-5",
            ^
>> drivers/iio/adc/ad7091r5.c:59:14: error: initializer element is not a compile-time constant
           .irq_info = ad7091r5_chip_info_irq,
                       ^~~~~~~~~~~~~~~~~~~~~~

3 errors generated.

vim +59 drivers/iio/adc/ad7091r5.c

    57	
    58	static struct ad7091r_init_info ad7091r5_init_info = {
  > 59		.irq_info = ad7091r5_chip_info_irq,
    60		.info_no_irq = ad7091r5_chip_info_noirq,
    61		.regmap_config = &ad7091r_regmap_config,
    62		.ad7091r_regmap_init = &ad7091r5_regmap_init
    63	};
    64

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Marcelo Schmitt

author maintainer

2023-12-07 18:41 UTC #13 lore raw

PATCH v3 7/13 iio: adc: ad7091r: Set device mode through chip_info callback

In reply to Marcelo Schmitt

AD7091R-5 devices have a few modes of operation (sample, command, autocycle) which are set by writing to configuration register fields. Follow up patches will add support for AD7091R-2/-4/-8 which don't have those operation modes nor the register fields for setting them. Make ad7091r_set_mode() a callback function of AD7091R chip_info struct so the base driver can appropriately handle each design without having to check which actual chip is connected.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 38 +---------------------------------
 drivers/iio/adc/ad7091r-base.h |  9 ++++++++
 drivers/iio/adc/ad7091r5.c     | 30 +++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 37 deletions(-)

drivers/iio/adc/ad7091r-base.c +1 ~~-37~~

index 90470b4a98c5..f2cb638b8d77 100644
@@ -19,14 +19,6 @@
 #define AD7091R_REG_RESULT_CH_ID(x)	    (((x) >> 13) & 0x3)
 #define AD7091R_REG_RESULT_CONV_RESULT(x)   ((x) & 0xfff)
 
-/* AD7091R_REG_CONF */
-#define AD7091R_REG_CONF_ALERT_EN   BIT(4)
-#define AD7091R_REG_CONF_AUTO   BIT(8)
-#define AD7091R_REG_CONF_CMD    BIT(10)
-
-#define AD7091R_REG_CONF_MODE_MASK  \
-	(AD7091R_REG_CONF_AUTO | AD7091R_REG_CONF_CMD)
-
 const struct iio_event_spec ad7091r_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,

@@ -48,34 +40,6 @@ const struct iio_event_spec ad7091r_events[] = {
 };
 EXPORT_SYMBOL_NS_GPL(ad7091r_events, IIO_AD7091R);
 
-static int ad7091r_set_mode(struct ad7091r_state *st, enum ad7091r_mode mode)
-{
-	int ret, conf;
-
-	switch (mode) {
-	case AD7091R_MODE_SAMPLE:
-		conf = 0;
-		break;
-	case AD7091R_MODE_COMMAND:
-		conf = AD7091R_REG_CONF_CMD;
-		break;
-	case AD7091R_MODE_AUTOCYCLE:
-		conf = AD7091R_REG_CONF_AUTO;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
-				 AD7091R_REG_CONF_MODE_MASK, conf);
-	if (ret)
-		return ret;
-
-	st->mode = mode;
-
-	return 0;
-}
-
 static int ad7091r_set_channel(struct ad7091r_state *st, unsigned int channel)
 {
 	unsigned int dummy;

@@ -265,7 +229,7 @@ int ad7091r_probe(struct device *dev, const char *name,
 	}
 
 	/* Use command mode by default to convert only desired channels*/
-	ret = ad7091r_set_mode(st, AD7091R_MODE_COMMAND);
+	ret = st->chip_info->ad7091r_set_mode(st, AD7091R_MODE_COMMAND);
 	if (ret)
 		return ret;

drivers/iio/adc/ad7091r-base.h +9 -0

index d58e2b08015a..9546d0bf1da7 100644
@@ -19,6 +19,14 @@
 #define AD7091R_REG_CH_HIGH_LIMIT(ch) ((ch) * 3 + 5)
 #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6)
 
+/* AD7091R_REG_CONF */
+#define AD7091R_REG_CONF_ALERT_EN	BIT(4)
+#define AD7091R_REG_CONF_AUTO		BIT(8)
+#define AD7091R_REG_CONF_CMD		BIT(10)
+
+#define AD7091R_REG_CONF_MODE_MASK  \
+	(AD7091R_REG_CONF_AUTO | AD7091R_REG_CONF_CMD)
+
 #define AD7091R_CHANNEL(idx, bits, ev, num_ev) {			\
 	.type = IIO_VOLTAGE,						\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\

@@ -52,6 +60,7 @@ struct ad7091r_chip_info {
 	unsigned int num_channels;
 	const struct iio_chan_spec *channels;
 	unsigned int vref_mV;
+	int (*ad7091r_set_mode)(struct ad7091r_state *st, enum ad7091r_mode mode);
 };
 
 struct ad7091r_init_info {

drivers/iio/adc/ad7091r5.c +30 -0

index 51aad8df7f3a..ed5e00cc82e2 100644
@@ -26,11 +26,40 @@ static const struct iio_chan_spec ad7091r5_channels_noirq[] = {
 	AD7091R_CHANNEL(3, 12, NULL, 0),
 };
 
+static int ad7091r_set_mode(struct ad7091r_state *st, enum ad7091r_mode mode)
+{
+	int ret, conf;
+
+	switch (mode) {
+	case AD7091R_MODE_SAMPLE:
+		conf = 0;
+		break;
+	case AD7091R_MODE_COMMAND:
+		conf = AD7091R_REG_CONF_CMD;
+		break;
+	case AD7091R_MODE_AUTOCYCLE:
+		conf = AD7091R_REG_CONF_AUTO;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
+				 AD7091R_REG_CONF_MODE_MASK, conf);
+	if (ret)
+		return ret;
+
+	st->mode = mode;
+
+	return 0;
+}
+
 static const struct ad7091r_chip_info ad7091r5_chip_info_irq = {
 	.name = "ad7091r-5",
 	.channels = ad7091r5_channels_irq,
 	.num_channels = ARRAY_SIZE(ad7091r5_channels_irq),
 	.vref_mV = 2500,
+	.ad7091r_set_mode = &ad7091r_set_mode,
 };
 
 static const struct ad7091r_chip_info ad7091r5_chip_info_noirq = {

@@ -38,6 +67,7 @@ static const struct ad7091r_chip_info ad7091r5_chip_info_noirq = {
 	.channels = ad7091r5_channels_noirq,
 	.num_channels = ARRAY_SIZE(ad7091r5_channels_noirq),
 	.vref_mV = 2500,
+	.ad7091r_set_mode = &ad7091r_set_mode,
 };
 
 static const struct regmap_config ad7091r_regmap_config = {

Signature · 1 line

2.42.0

Jonathan Cameron

maintainer

2023-12-10 12:35 UTC #14 lore raw

Re: [PATCH v3 07/13] iio: adc: ad7091r: Set device mode through chip_info callback

Quoted 9 lines · On Thu, 7 Dec 2023 15:41:03 -0300 Marcelo Schmitt <[email protected]>…

AD7091R-5 devices have a few modes of operation (sample, command, autocycle) which are set by writing to configuration register fields. Follow up patches will add support for AD7091R-2/-4/-8 which don't have those operation modes nor the register fields for setting them. Make ad7091r_set_mode() a callback function of AD7091R chip_info struct so the base driver can appropriately handle each design without having to check which actual chip is connected.
Signed-off-by Marcelo Schmitt <[email protected]>

Hi Marcelo,

drivers/iio/adc/ad7091r-base.h struct ad7091r_chip_info { · line 63

Quoted 9 lines

drivers/iio/adc/ad7091r-base.h +2 -0

+
 #define AD7091R_CHANNEL(idx, bits, ev, num_ev) {			\
 	.type = IIO_VOLTAGE,						\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\

@@ -52,6 +60,7 @@ struct ad7091r_chip_info {
 	unsigned int num_channels;
 	const struct iio_chan_spec *channels;
 	unsigned int vref_mV;
+	int (*ad7091r_set_mode)(struct ad7091r_state *st, enum ad7091r_mode mode);

Given it's embedded in a driver specific structure, I'm not seeing a clear reason to prefix the callback with ad7091r. set_mode is probably enough. Same for the ones introduced in later patches.

Marcelo Schmitt

author maintainer

2023-12-07 18:42 UTC #15 lore raw

PATCH v3 8/13 iio: adc: ad7091r: Enable internal vref if external vref is not supplied

In reply to Marcelo Schmitt

The ADC needs a voltage reference to work correctly. Enable AD7091R internal voltage reference if no external vref is supplied.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 9 ++++++---
 drivers/iio/adc/ad7091r-base.h | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)

drivers/iio/adc/ad7091r-base.c +6 -3

index f2cb638b8d77..59a7ec44955d 100644
@@ -215,10 +215,13 @@ int ad7091r_probe(struct device *dev, const char *name,
 	iio_dev->channels = st->chip_info->channels;
 
 	st->vref = devm_regulator_get_optional(dev, "vref");
-	if (IS_ERR(st->vref)) {
-		if (PTR_ERR(st->vref) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+	if (IS_ERR_OR_NULL(st->vref)) {
+		/* Enable internal vref */
 		st->vref = NULL;
+		ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
+					 AD7091R_REG_CONF_INT_VREF, BIT(0));
+		if (ret)
+			return ret;
 	} else {
 		ret = regulator_enable(st->vref);
 		if (ret)

drivers/iio/adc/ad7091r-base.h +1 -0

index 9546d0bf1da7..e153c2d7deb5 100644
@@ -20,6 +20,7 @@
 #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6)
 
 /* AD7091R_REG_CONF */
+#define AD7091R_REG_CONF_INT_VREF	BIT(0)
 #define AD7091R_REG_CONF_ALERT_EN	BIT(4)
 #define AD7091R_REG_CONF_AUTO		BIT(8)
 #define AD7091R_REG_CONF_CMD		BIT(10)

Signature · 1 line

2.42.0

Jonathan Cameron

maintainer

2023-12-10 12:22 UTC #16 lore raw

Re: [PATCH v3 08/13] iio: adc: ad7091r: Enable internal vref if external vref is not supplied

Quoted 15 lines · On Thu, 7 Dec 2023 15:41:25 -0300 Marcelo Schmitt <[email protected]>…

The ADC needs a voltage reference to work correctly. Enable AD7091R internal voltage reference if no external vref is supplied.
Signed-off-by Marcelo Schmitt <[email protected]>
---
 drivers/iio/adc/ad7091r-base.c | 9 ++++++---
 drivers/iio/adc/ad7091r-base.h | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)
drivers/iio/adc/ad7091r-base.c +0 -0
index f2cb638b8d77..59a7ec44955d 100644

drivers/iio/adc/ad7091r-base.c int ad7091r_probe(struct device *dev, const char *name, · line 217

Quoted 3 lines

drivers/iio/adc/ad7091r-base.c +0 -0

@@ -215,10 +215,13 @@ int ad7091r_probe(struct device *dev, const char *name,
 	iio_dev->channels = st->chip_info->channels;
 
 	st->vref = devm_regulator_get_optional(dev, "vref");

This does not return NULL, only a valid regulator or an error code.

drivers/iio/adc/ad7091r-base.c int ad7091r_probe(struct device *dev, const char *name, · line 218

Quoted 4 lines

drivers/iio/adc/ad7091r-base.c +1 -3

-	if (IS_ERR(st->vref)) {
-		if (PTR_ERR(st->vref) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+	if (IS_ERR_OR_NULL(st->vref)) {

You still need to explicitly handle deferal here.
There might be a perfectly good regulator that just isn't ready yet and if that happens we want to try probing this driver again later rather than papering over it.

Quoted 21 lines

drivers/iio/adc/ad7091r-base.c +5 -0

+		/* Enable internal vref */
 		st->vref = NULL;
+		ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
+					 AD7091R_REG_CONF_INT_VREF, BIT(0));
+		if (ret)
+			return ret;
 	} else {
 		ret = regulator_enable(st->vref);
 		if (ret)

drivers/iio/adc/ad7091r-base.h +1 -0

index 9546d0bf1da7..e153c2d7deb5 100644
@@ -20,6 +20,7 @@
 #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6)
 
 /* AD7091R_REG_CONF */
+#define AD7091R_REG_CONF_INT_VREF	BIT(0)
 #define AD7091R_REG_CONF_ALERT_EN	BIT(4)
 #define AD7091R_REG_CONF_AUTO		BIT(8)
 #define AD7091R_REG_CONF_CMD		BIT(10)

Marcelo Schmitt

author maintainer

2023-12-07 18:42 UTC #17 lore raw

PATCH v3 9/13 iio: adc: ad7091r: Add chip_info callback to get conversion result channel

In reply to Marcelo Schmitt

AD7091R-5 and AD7091R-2/-4/-8 have slightly different register field layout and due to that require different masks for getting the index of the channel associated with each read.
Add a callback function so the base driver can get correct channel ID for each chip variant.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/ad7091r-base.c | 6 +-----
 drivers/iio/adc/ad7091r-base.h | 6 ++++++
 drivers/iio/adc/ad7091r5.c     | 7 +++++++
 3 files changed, 14 insertions(+), 5 deletions(-)

drivers/iio/adc/ad7091r-base.c +1 -5

index 59a7ec44955d..5a7046f6f0ce 100644
@@ -15,10 +15,6 @@
 
 #include "ad7091r-base.h"
 
-/* AD7091R_REG_RESULT */
-#define AD7091R_REG_RESULT_CH_ID(x)	    (((x) >> 13) & 0x3)
-#define AD7091R_REG_RESULT_CONV_RESULT(x)   ((x) & 0xfff)
-
 const struct iio_event_spec ad7091r_events[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,

@@ -73,7 +69,7 @@ static int ad7091r_read_one(struct iio_dev *iio_dev,
 	if (ret)
 		return ret;
 
-	if (AD7091R_REG_RESULT_CH_ID(val) != channel)
+	if (st->chip_info->ad7091r_reg_result_chan_id(val) != channel)
 		return -EIO;
 
 	*read_val = AD7091R_REG_RESULT_CONV_RESULT(val);

drivers/iio/adc/ad7091r-base.h +6 -0

index e153c2d7deb5..c554f04e7448 100644
@@ -19,6 +19,11 @@
 #define AD7091R_REG_CH_HIGH_LIMIT(ch) ((ch) * 3 + 5)
 #define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6)
 
+/* AD7091R_REG_RESULT */
+#define AD7091R5_REG_RESULT_CH_ID(x)	    (((x) >> 13) & 0x3)
+#define AD7091R8_REG_RESULT_CH_ID(x)	    (((x) >> 13) & 0x7)
+#define AD7091R_REG_RESULT_CONV_RESULT(x)   ((x) & 0xfff)
+
 /* AD7091R_REG_CONF */
 #define AD7091R_REG_CONF_INT_VREF	BIT(0)
 #define AD7091R_REG_CONF_ALERT_EN	BIT(4)

@@ -61,6 +66,7 @@ struct ad7091r_chip_info {
 	unsigned int num_channels;
 	const struct iio_chan_spec *channels;
 	unsigned int vref_mV;
+	unsigned int (*ad7091r_reg_result_chan_id)(unsigned int val);
 	int (*ad7091r_set_mode)(struct ad7091r_state *st, enum ad7091r_mode mode);
 };

drivers/iio/adc/ad7091r5.c +7 -0

index ed5e00cc82e2..53a3f74f6452 100644
@@ -54,11 +54,17 @@ static int ad7091r_set_mode(struct ad7091r_state *st, enum ad7091r_mode mode)
 	return 0;
 }
 
+static unsigned int ad7091r5_reg_result_chan_id(unsigned int val)
+{
+	return AD7091R5_REG_RESULT_CH_ID(val);
+}
+
 static const struct ad7091r_chip_info ad7091r5_chip_info_irq = {
 	.name = "ad7091r-5",
 	.channels = ad7091r5_channels_irq,
 	.num_channels = ARRAY_SIZE(ad7091r5_channels_irq),
 	.vref_mV = 2500,
+	.ad7091r_reg_result_chan_id = &ad7091r5_reg_result_chan_id,
 	.ad7091r_set_mode = &ad7091r_set_mode,
 };

@@ -67,6 +73,7 @@ static const struct ad7091r_chip_info ad7091r5_chip_info_noirq = {
 	.channels = ad7091r5_channels_noirq,
 	.num_channels = ARRAY_SIZE(ad7091r5_channels_noirq),
 	.vref_mV = 2500,
+	.ad7091r_reg_result_chan_id = &ad7091r5_reg_result_chan_id,
 	.ad7091r_set_mode = &ad7091r_set_mode,
 };

Signature · 1 line

2.42.0

Marcelo Schmitt

author maintainer

2023-12-07 18:42 UTC #18 lore raw

PATCH v3 10/13 dt-bindings: iio: Add AD7091R-8

In reply to Marcelo Schmitt

Add device tree documentation for AD7091R-8.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +99 -0

new file mode 100644
index 000000000000..02320778f225
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7091R8 8-Channel 12-Bit ADC
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD7091R-8 8-Channel 12-Bit ADC
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7091r2
+      - adi,ad7091r4
+      - adi,ad7091r8
+
+  reg:
+    maxItems: 1
+
+  vref-supply: true
+
+  adi,conversion-start-gpios:
+    description:
+      GPIO connected to the CONVST pin.
+      This logic input is used to initiate conversions on the analog
+      input channels.
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+patternProperties:
+  "^channel@[0-7]$":
+    $ref: adc.yaml
+    type: object
+    description: Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 7
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+  - adi,conversion-start-gpios
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+  # AD7091R-2 does not have ALERT/BUSY/GPO pin
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7091r4
+              - adi,ad7091r8
+    then:
+      properties:
+        interrupts: true
+    else:
+      properties:
+        interrupts: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+                compatible = "adi,ad7091r8";
+                reg = <0x0>;
+                spi-max-frequency = <45454545>;
+                vref-supply = <&adc_vref>;
+                adi,conversion-start-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
+                reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
+                interrupts = <22 IRQ_TYPE_EDGE_FALLING>;
+                interrupt-parent = <&gpio>;
+        };
+    };
+...

Signature · 1 line

2.42.0

David Lechner

reviewer

2023-12-07 23:57 UTC #19 lore raw

Re: [PATCH v3 10/13] dt-bindings: iio: Add AD7091R-8

Quoted 30 lines · Marcelo Schmitt

Add device tree documentation for AD7091R-8.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +15 -0

new file mode 100644
index 000000000000..02320778f225
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7091R8 8-Channel 12-Bit ADC
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD7091R-8 8-Channel 12-Bit ADC
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf
+

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 25

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +10 -0
+properties:
+  compatible:
+    enum:
+      - adi,ad7091r2
+      - adi,ad7091r4
+      - adi,ad7091r8
+
+  reg:
+    maxItems: 1
+

Missing other supplies? Like vdd-supply and vdrive-supply?

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 26

Quoted 1 line

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +1 -0
+  vref-supply: true

refin-supply might be a better name to match the datasheet pin name.

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 28

Quoted 2 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +2 -0
+
+  adi,conversion-start-gpios:

gpios usually don't get a vendor prefix do they?

convst-gpios could be a better name to match the pin name on the datasheet.

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 39

Quoted 11 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +11 -0

+    description:
+      GPIO connected to the CONVST pin.
+      This logic input is used to initiate conversions on the analog
+      input channels.
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1

A description of what the interrupt is attached to (ALERT/BUSY/GPO0 pin) would be helpful.

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 50

Quoted 11 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +11 -0

+
+patternProperties:
+  "^channel@[0-7]$":
+    $ref: adc.yaml
+    type: object
+    description: Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 7

Shouldn't this be:

items:
- minimum: 0
maximum: 7

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 53

Quoted 3 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +3 -0
+
+    required:
+      - reg

Missing unevaluatedProperties: false for channels?

Bigger picture: since no other properties besides reg are included here, do we actually need channel nodes?

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +10 -0

+
+required:
+  - compatible
+  - reg
+  - adi,conversion-start-gpios
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+  # AD7091R-2 does not have ALERT/BUSY/GPO pin

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 73

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +10 -0

+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7091r4
+              - adi,ad7091r8
+    then:
+      properties:
+        interrupts: true

Interrupts is already true. Maybe better to only match chips without interrupts and set false?

Quoted 30 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +26 -1

+    else:
+      properties:
+        interrupts: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+                compatible = "adi,ad7091r8";
+                reg = <0x0>;
+                spi-max-frequency = <45454545>;
+                vref-supply = <&adc_vref>;
+                adi,conversion-start-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
+                reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
+                interrupts = <22 IRQ_TYPE_EDGE_FALLING>;
+                interrupt-parent = <&gpio>;
+        };
+    };
+...
--

2.42.0

Marcelo Schmitt

2023-12-08 13:28 UTC #20 lore raw

Re: [PATCH v3 10/13] dt-bindings: iio: Add AD7091R-8

Hi David, thank you for your suggestions.
Comments inline.

Quoted 36 lines · David Lechner

Quoted 34 lines · Marcelo Schmitt

Add device tree documentation for AD7091R-8.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +19 -0

new file mode 100644
index 000000000000..02320778f225
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7091R8 8-Channel 12-Bit ADC
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD7091R-8 8-Channel 12-Bit ADC
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7091r2

Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 25
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +6 -0
+      - adi,ad7091r4
+      - adi,ad7091r8
+
+  reg:
+    maxItems: 1
+
Missing other supplies? Like vdd-supply and vdrive-supply?

I used the name that would work with ad7091r-base.c.
If I'm not misinterpreting the datasheet, vdd-supply and vdrive-supply are for powering the ADC and setting SPI lanes logic level, respectively. They don't have any impact on ADC readings.
By the way, should maybe I extend ad7091r5 dt doc instead of creating this new one?

Quoted 4 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 26
Quoted 1 line
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +1 -0
+  vref-supply: true
refin-supply might be a better name to match the datasheet pin name.

Agree, though I guess changing the name now would break users of ad7091r5 if they happen to update the driver without updating their device tree.

Quoted 6 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 28
Quoted 2 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +2 -0
+
+  adi,conversion-start-gpios:
gpios usually don't get a vendor prefix do they?
convst-gpios could be a better name to match the pin name on the datasheet.

Ack, will do for v4.

Quoted 16 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 39
Quoted 12 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +11 -0
+    description:
+      GPIO connected to the CONVST pin.
+      This logic input is used to initiate conversions on the analog
+      input channels.
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
A description of what the interrupt is attached to (ALERT/BUSY/GPO0 pin) would be helpful.

Ack, will do for v4.

Quoted 14 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 50
Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +11 -0
+
+patternProperties:
+  "^channel@[0-7]$":
+    $ref: adc.yaml
+    type: object
+    description: Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 7
Shouldn't this be:

Quoted 4 lines

items:
- minimum: 0
maximum: 7

Ack

Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 53
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +3 -0
+
+    required:
+      - reg
Missing unevaluatedProperties: false for channels?
Bigger picture: since no other properties besides reg are included here, do we actually need channel nodes?

The channel nodes are not used by the drivers so we can remove them if we want. I thought they would be required as documentation even if they were not used in drivers.
Looks like they're not required so will remove them in v4.

Quoted 15 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +15 -0

+
+required:
+  - compatible
+  - reg
+  - adi,conversion-start-gpios
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+  # AD7091R-2 does not have ALERT/BUSY/GPO pin
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:

Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 73
Quoted 5 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +5 -0
+              - adi,ad7091r4
+              - adi,ad7091r8
+    then:
+      properties:
+        interrupts: true
Interrupts is already true. Maybe better to only match chips without interrupts and set false?

Agree, that should simplify the constrain logic. Will do for v4.

Quoted 30 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +26 -1

+    else:
+      properties:
+        interrupts: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+                compatible = "adi,ad7091r8";
+                reg = <0x0>;
+                spi-max-frequency = <45454545>;
+                vref-supply = <&adc_vref>;
+                adi,conversion-start-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
+                reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>;
+                interrupts = <22 IRQ_TYPE_EDGE_FALLING>;
+                interrupt-parent = <&gpio>;
+        };
+    };
+...
--

2.42.0

David Lechner

reviewer

2023-12-08 14:50 UTC #21 lore raw

Re: [PATCH v3 10/13] dt-bindings: iio: Add AD7091R-8

Quoted 51 lines · Marcelo Schmitt

Hi David, thank you for your suggestions.
Comments inline.
Quoted 36 lines · David Lechner
Quoted 34 lines · Marcelo Schmitt
Add device tree documentation for AD7091R-8.
Signed-off-by Marcelo Schmitt <[email protected]>
---
 .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +19 -0
new file mode 100644
index 000000000000..02320778f225
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7091r8.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7091R8 8-Channel 12-Bit ADC
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD7091R-8 8-Channel 12-Bit ADC
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7091R-2_7091R-4_7091R-8.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7091r2
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 25
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +6 -0
+      - adi,ad7091r4
+      - adi,ad7091r8
+
+  reg:
+    maxItems: 1
+
Missing other supplies? Like vdd-supply and vdrive-supply?

Quoted 4 lines

I used the name that would work with ad7091r-base.c.
If I'm not misinterpreting the datasheet, vdd-supply and vdrive-supply are for powering the ADC and setting SPI lanes logic level, respectively. They don't have any impact on ADC readings.

The guidelines [1] say that bindings should be complete even if the feature is not used. In the most recent bindings I have submitted, Jonathan specifically called out making sure all supplies were included in the bindings. So I would assume the same applies here.

[1]: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html

Quoted 2 lines

By the way, should maybe I extend ad7091r5 dt doc instead of creating this new one?

If it is pin-compatible or 90% the same, then perhaps.

Jonathan Cameron

maintainer

2023-12-10 12:26 UTC #22 lore raw

Re: [PATCH v3 10/13] dt-bindings: iio: Add AD7091R-8

In reply to Marcelo Schmitt

On Fri, 8 Dec 2023 10:28:25 -0300

Quoted 6 lines

Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 53
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +3 -0
+
+    required:
+      - reg  
Missing unevaluatedProperties: false for channels?

Quoted 8 lines

Quoted 3 lines
Bigger picture: since no other properties besides reg are included here, do we actually need channel nodes?
The channel nodes are not used by the drivers so we can remove them if we want. I thought they would be required as documentation even if they were not used in drivers.
Looks like they're not required so will remove them in v4.

A lot of drivers assume that if you paid for a device with N channels you probably want N channels. Of course there are always boards that wire a subset but it's optional whether a driver cares about that.

We have drivers where not channel nodes being supplied means they are all on so this is extensible if we later decide that fine grained information about what is routed where is needed or need to add per channel controls.

So fine to drop this.

Jonathan

Krzysztof Kozlowski

2023-12-08 08:05 UTC #23 lore raw

Re: [PATCH v3 10/13] dt-bindings: iio: Add AD7091R-8

In reply to Marcelo Schmitt

Quoted 3 lines · Marcelo Schmitt

Add device tree documentation for AD7091R-8.
Signed-off-by Marcelo Schmitt <[email protected]>

Except what David said also:

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 89

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +10 -0

+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+                compatible = "adi,ad7091r8";

Use 4 spaces for example indentation.

Best regards,
Krzysztof

Marcelo Schmitt

2023-12-08 13:29 UTC #24 lore raw

Re: [PATCH v3 10/13] dt-bindings: iio: Add AD7091R-8

Quoted 7 lines · Krzysztof Kozlowski

Quoted 3 lines · Marcelo Schmitt
Add device tree documentation for AD7091R-8.
Signed-off-by Marcelo Schmitt <[email protected]>
Except what David said also:

Quoted 12 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml line 89
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml new file +10 -0
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+                compatible = "adi,ad7091r8";
Use 4 spaces for example indentation.

Ack, will do for v4.

Thanks

Quoted 4 lines

Best regards,
Krzysztof

Marcelo Schmitt

author maintainer

2023-12-07 18:43 UTC #25 lore raw

PATCH v3 11/13 iio: adc: Split AD7091R-5 config symbol

In reply to Marcelo Schmitt

Split AD7091R-5 kconfig symbol into one symbol for the base AD7091R driver and another one for the I2C interface AD7091R-5 driver.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/Kconfig  | 4 ++++
 drivers/iio/adc/Makefile | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

drivers/iio/adc/Kconfig +4 -0

index 1e2b7a2c67c6..dcf45d7478e1 100644
@@ -36,9 +36,13 @@ config AD4130
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4130.
 
+config AD7091R
+	tristate
+
 config AD7091R5
 	tristate "Analog Devices AD7091R5 ADC Driver"
 	depends on I2C
+	select AD7091R
 	select REGMAP_I2C
 	help
 	  Say yes here to build support for Analog Devices AD7091R-5 ADC.

drivers/iio/adc/Makefile +2 -1

index c0803383a7cc..1e289d674d4d 100644
@@ -7,7 +7,8 @@
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4130) += ad4130.o
-obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o
+obj-$(CONFIG_AD7091R) += ad7091r-base.o
+obj-$(CONFIG_AD7091R5) += ad7091r5.o
 obj-$(CONFIG_AD7124) += ad7124.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7266) += ad7266.o

Signature · 1 line

2.42.0

Marcelo Schmitt

author maintainer

2023-12-07 18:43 UTC #26 lore raw

PATCH v3 12/13 iio: adc: Add support for AD7091R-8

In reply to Marcelo Schmitt

Add support for Analog Devices AD7091R-2, AD7091R-4, and AD7091R-8 low power 12-Bit SAR ADCs with SPI interface.
Extend ad7091r-base driver so it can be used by the AD7091R-8 driver.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/iio/adc/Kconfig        |  12 ++
 drivers/iio/adc/Makefile       |   1 +
 drivers/iio/adc/ad7091r-base.c |   7 +
 drivers/iio/adc/ad7091r-base.h |   8 +
 drivers/iio/adc/ad7091r8.c     | 261 +++++++++++++++++++++++++++++++++
 5 files changed, 289 insertions(+)
 create mode 100644 drivers/iio/adc/ad7091r8.c

drivers/iio/adc/Kconfig +12 -0

index dcf45d7478e1..284d898790a2 100644
@@ -47,6 +47,18 @@ config AD7091R5
 	help
 	  Say yes here to build support for Analog Devices AD7091R-5 ADC.
 
+config AD7091R8
+	tristate "Analog Devices AD7091R8 ADC Driver"
+	depends on SPI
+	select AD7091R
+	select REGMAP_SPI
+	help
+	  Say yes here to build support for Analog Devices AD7091R-2, AD7091R-4,
+	  and AD7091R-8 ADC.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad7091r8.
+
 config AD7124
 	tristate "Analog Devices AD7124 and similar sigma-delta ADCs driver"
 	depends on SPI_MASTER

drivers/iio/adc/Makefile +1 -0

index 1e289d674d4d..d2fda54a3259 100644
@@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD7091R) += ad7091r-base.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o
+obj-$(CONFIG_AD7091R8) += ad7091r8.o
 obj-$(CONFIG_AD7124) += ad7124.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7266) += ad7266.o

drivers/iio/adc/ad7091r-base.c +7 -0

index 5a7046f6f0ce..0add6e144d63 100644
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/interrupt.h>

@@ -188,6 +189,12 @@ int ad7091r_probe(struct device *dev, const char *name,
 	iio_dev->info = &ad7091r_info;
 	iio_dev->modes = INDIO_DIRECT_MODE;
 
+	if (init_info->ad7091r_setup) {
+		ret = init_info->ad7091r_setup(st);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (irq) {
 		st->chip_info = &init_info->irq_info;
 		ret = regmap_update_bits(st->map, AD7091R_REG_CONF,

drivers/iio/adc/ad7091r-base.h +8 -0

index c554f04e7448..7140d5fd0ac2 100644
@@ -45,6 +45,8 @@
 	.scan_type.realbits = bits,					\
 }
 
+#include <linux/gpio/consumer.h>
+
 struct device;
 
 enum ad7091r_mode {

@@ -56,13 +58,18 @@ enum ad7091r_mode {
 struct ad7091r_state {
 	struct device *dev;
 	struct regmap *map;
+	struct gpio_desc *convst_gpio;
+	struct gpio_desc *reset_gpio;
 	struct regulator *vref;
 	const struct ad7091r_chip_info *chip_info;
 	enum ad7091r_mode mode;
 	struct mutex lock; /*lock to prevent concurent reads */
+	__be16 tx_buf __aligned(IIO_DMA_MINALIGN);
+	__be16 rx_buf;
 };
 
 struct ad7091r_chip_info {
+	const char *name;
 	unsigned int num_channels;
 	const struct iio_chan_spec *channels;
 	unsigned int vref_mV;

@@ -74,6 +81,7 @@ struct ad7091r_init_info {
 	struct ad7091r_chip_info irq_info;
 	struct ad7091r_chip_info info_no_irq;
 	const struct regmap_config *regmap_config;
+	int (*ad7091r_setup)(struct ad7091r_state *st);
 	void (*ad7091r_regmap_init)(struct ad7091r_state *st,
 				    const struct regmap_config *regmap_conf);
 };

drivers/iio/adc/ad7091r8.c new file +261 -0

261 lines not on this page · raw message

Signature · 1 line

2.42.0

Jonathan Cameron

maintainer

2023-12-10 12:33 UTC #27 lore raw

Re: [PATCH v3 12/13] iio: adc: Add support for AD7091R-8

Quoted 5 lines · On Thu, 7 Dec 2023 15:42:56 -0300 Marcelo Schmitt <[email protected]>…

Add support for Analog Devices AD7091R-2, AD7091R-4, and AD7091R-8 low power 12-Bit SAR ADCs with SPI interface.
Extend ad7091r-base driver so it can be used by the AD7091R-8 driver.
Signed-off-by Marcelo Schmitt <[email protected]>

A few trivial things inline.
Otherwise looks pretty good to me.

Jonathan

Quoted 19 lines

drivers/iio/adc/ad7091r8.c new file +13 -0

new file mode 100644
index 000000000000..8dc0f784913b
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Analog Devices AD7091R8 12-bit SAR ADC driver
+ *
+ * Copyright 2023 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
+#include <linux/spi/spi.h>

drivers/iio/adc/ad7091r8.c line 23

Quoted 10 lines

drivers/iio/adc/ad7091r8.c new file +10 -0

+
+#include "ad7091r-base.h"
+
+#define AD7091R8_REG_ADDR_MSK				GENMASK(15, 11)
+#define AD7091R8_RD_WR_FLAG_MSK				BIT(10)
+#define AD7091R8_REG_DATA_MSK				GENMASK(9, 0)
+
+#define AD7091R2_DEV_NAME				"ad7091r-2"
+#define AD7091R4_DEV_NAME				"ad7091r-4"
+#define AD7091R8_DEV_NAME				"ad7091r-8"

Not seeing any advantage in these macros. It will be more readable to just have the strings inline where the macros are currently used.

drivers/iio/adc/ad7091r8.c line 175

Quoted 12 lines

drivers/iio/adc/ad7091r8.c new file +12 -0

+static int ad7091r8_gpio_setup(struct ad7091r_state *st)
+{
+	st->convst_gpio = devm_gpiod_get(st->dev, "adi,conversion-start",
+					 GPIOD_OUT_LOW);
+	if (IS_ERR(st->convst_gpio))
+		return dev_err_probe(st->dev, PTR_ERR(st->convst_gpio),
+				     "Error getting convst GPIO\n");
+
+	st->reset_gpio =  devm_gpiod_get_optional(st->dev, "reset",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(st->reset_gpio))
+		return PTR_ERR(st->reset_gpio);

Maybe a dev_err_probe() here as well both for consistency and for the debug info that gets stashed if it's EPROBE_DEFER

Quoted 33 lines

drivers/iio/adc/ad7091r8.c new file +32 -0

+
+	if (st->reset_gpio) {
+		fsleep(20);
+		gpiod_set_value_cansleep(st->reset_gpio, 0);
+	}
+
+	return 0;
+}
 
+
+static struct ad7091r_init_info ad7091r2_init_info = {
+	.info_no_irq = AD7091R_SPI_CHIP_INFO(2),
+	.regmap_config = &ad7091r2_reg_conf,
+	.ad7091r_regmap_init = &ad7091r8_regmap_init,
+	.ad7091r_setup = &ad7091r8_gpio_setup
+};
+
+static struct ad7091r_init_info ad7091r4_init_info = {
+	.irq_info = AD7091R_SPI_CHIP_INFO_IRQ(4),
+	.info_no_irq = AD7091R_SPI_CHIP_INFO(4),
+	.regmap_config = &ad7091r4_reg_conf,
+	.ad7091r_regmap_init = &ad7091r8_regmap_init,
+	.ad7091r_setup = &ad7091r8_gpio_setup
+};
+
+static struct ad7091r_init_info ad7091r8_init_info = {
+	.irq_info = AD7091R_SPI_CHIP_INFO_IRQ(8),
+	.info_no_irq = AD7091R_SPI_CHIP_INFO(8),
+	.regmap_config = &ad7091r8_reg_conf,
+	.ad7091r_regmap_init = &ad7091r8_regmap_init,
+	.ad7091r_setup = &ad7091r8_gpio_setup
+};
+

drivers/iio/adc/ad7091r8.c line 229

Quoted 10 lines

drivers/iio/adc/ad7091r8.c new file +10 -0

+static int ad7091r8_spi_probe(struct spi_device *spi)
+{
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	const struct ad7091r_init_info *init_info;
+
+	init_info = spi_get_device_match_data(spi);
+	if (!init_info)
+		return -EINVAL;
+
+	return ad7091r_probe(&spi->dev, id->name, init_info, NULL, spi->irq);

id->name isn't generally a good idea because we end up with lots of odd corner cases if the of_device_id and spi_device_id tables get out of sync - which can happen if fallback compatibles get used.

Normal way round this is just put the naming of the device in the info structure. Costs a little storage, but makes the code simpler and less probe to odd corner cases. Also, I think you already have it in there!

Quoted 5 lines

drivers/iio/adc/ad7091r8.c new file +5 -0

+}
+
+static const struct of_device_id ad7091r8_of_match[] = {
+	{ .compatible = "adi,ad7091r2", .data = &ad7091r2_init_info },
+	{ .compatible = "adi,ad7091r4", .data = &ad7091r4_init_info },

drivers/iio/adc/ad7091r8.c line 244

Quoted 10 lines

drivers/iio/adc/ad7091r8.c new file +10 -0

+	{ .compatible = "adi,ad7091r8", .data = &ad7091r8_init_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7091r8_of_match);
+
+static const struct spi_device_id ad7091r8_spi_id[] = {
+	{ "ad7091r2", (kernel_ulong_t)&ad7091r2_init_info },
+	{ "ad7091r4", (kernel_ulong_t)&ad7091r4_init_info },
+	{ "ad7091r8", (kernel_ulong_t)&ad7091r8_init_info },
+	{}

Trivial but be consistent on spacing for these terminators. I like a space, so { } but I don't mind if an author prefers {} as long as they are consistent!

Quoted 2 lines

drivers/iio/adc/ad7091r8.c new file +2 -0
+};
+MODULE_DEVICE_TABLE(spi, ad7091r8_spi_id);

Marcelo Schmitt

2023-12-10 19:54 UTC #28 lore raw

Re: [PATCH v3 12/13] iio: adc: Add support for AD7091R-8

Hi Jonathan,

Thank you for all comments to this set.
Will do the changes and send v4.

Thanks,
Marcelo

Quoted 132 lines · Jonathan Cameron

Quoted 5 lines · On Thu, 7 Dec 2023 15:42:56 -0300 Marcelo Schmitt <[email protected]>…
Add support for Analog Devices AD7091R-2, AD7091R-4, and AD7091R-8 low power 12-Bit SAR ADCs with SPI interface.
Extend ad7091r-base driver so it can be used by the AD7091R-8 driver.
Signed-off-by Marcelo Schmitt <[email protected]>
A few trivial things inline.
Otherwise looks pretty good to me.
Jonathan
Quoted 19 lines
drivers/iio/adc/ad7091r8.c new file +13 -0
new file mode 100644
index 000000000000..8dc0f784913b
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Analog Devices AD7091R8 12-bit SAR ADC driver
+ *
+ * Copyright 2023 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
+#include <linux/spi/spi.h>
drivers/iio/adc/ad7091r8.c line 23
Quoted 10 lines
drivers/iio/adc/ad7091r8.c new file +10 -0
+
+#include "ad7091r-base.h"
+
+#define AD7091R8_REG_ADDR_MSK				GENMASK(15, 11)
+#define AD7091R8_RD_WR_FLAG_MSK				BIT(10)
+#define AD7091R8_REG_DATA_MSK				GENMASK(9, 0)
+
+#define AD7091R2_DEV_NAME				"ad7091r-2"
+#define AD7091R4_DEV_NAME				"ad7091r-4"
+#define AD7091R8_DEV_NAME				"ad7091r-8"
Not seeing any advantage in these macros. It will be more readable to just have the strings inline where the macros are currently used.
drivers/iio/adc/ad7091r8.c line 175
Quoted 12 lines
drivers/iio/adc/ad7091r8.c new file +12 -0
+static int ad7091r8_gpio_setup(struct ad7091r_state *st)
+{
+	st->convst_gpio = devm_gpiod_get(st->dev, "adi,conversion-start",
+					 GPIOD_OUT_LOW);
+	if (IS_ERR(st->convst_gpio))
+		return dev_err_probe(st->dev, PTR_ERR(st->convst_gpio),
+				     "Error getting convst GPIO\n");
+
+	st->reset_gpio =  devm_gpiod_get_optional(st->dev, "reset",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(st->reset_gpio))
+		return PTR_ERR(st->reset_gpio);
Maybe a dev_err_probe() here as well both for consistency and for the debug info that gets stashed if it's EPROBE_DEFER
Quoted 33 lines
drivers/iio/adc/ad7091r8.c new file +32 -0
+
+	if (st->reset_gpio) {
+		fsleep(20);
+		gpiod_set_value_cansleep(st->reset_gpio, 0);
+	}
+
+	return 0;
+}
 
+
+static struct ad7091r_init_info ad7091r2_init_info = {
+	.info_no_irq = AD7091R_SPI_CHIP_INFO(2),
+	.regmap_config = &ad7091r2_reg_conf,
+	.ad7091r_regmap_init = &ad7091r8_regmap_init,
+	.ad7091r_setup = &ad7091r8_gpio_setup
+};
+
+static struct ad7091r_init_info ad7091r4_init_info = {
+	.irq_info = AD7091R_SPI_CHIP_INFO_IRQ(4),
+	.info_no_irq = AD7091R_SPI_CHIP_INFO(4),
+	.regmap_config = &ad7091r4_reg_conf,
+	.ad7091r_regmap_init = &ad7091r8_regmap_init,
+	.ad7091r_setup = &ad7091r8_gpio_setup
+};
+
+static struct ad7091r_init_info ad7091r8_init_info = {
+	.irq_info = AD7091R_SPI_CHIP_INFO_IRQ(8),
+	.info_no_irq = AD7091R_SPI_CHIP_INFO(8),
+	.regmap_config = &ad7091r8_reg_conf,
+	.ad7091r_regmap_init = &ad7091r8_regmap_init,
+	.ad7091r_setup = &ad7091r8_gpio_setup
+};
+
drivers/iio/adc/ad7091r8.c line 229
Quoted 10 lines
drivers/iio/adc/ad7091r8.c new file +10 -0
+static int ad7091r8_spi_probe(struct spi_device *spi)
+{
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	const struct ad7091r_init_info *init_info;
+
+	init_info = spi_get_device_match_data(spi);
+	if (!init_info)
+		return -EINVAL;
+
+	return ad7091r_probe(&spi->dev, id->name, init_info, NULL, spi->irq);
id->name isn't generally a good idea because we end up with lots of odd corner cases if the of_device_id and spi_device_id tables get out of sync - which can happen if fallback compatibles get used.
Normal way round this is just put the naming of the device in the info structure. Costs a little storage, but makes the code simpler and less probe to odd corner cases. Also, I think you already have it in there!
Quoted 5 lines
drivers/iio/adc/ad7091r8.c new file +5 -0
+}
+
+static const struct of_device_id ad7091r8_of_match[] = {
+	{ .compatible = "adi,ad7091r2", .data = &ad7091r2_init_info },
+	{ .compatible = "adi,ad7091r4", .data = &ad7091r4_init_info },
drivers/iio/adc/ad7091r8.c line 244
Quoted 10 lines
drivers/iio/adc/ad7091r8.c new file +10 -0
+	{ .compatible = "adi,ad7091r8", .data = &ad7091r8_init_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7091r8_of_match);
+
+static const struct spi_device_id ad7091r8_spi_id[] = {
+	{ "ad7091r2", (kernel_ulong_t)&ad7091r2_init_info },
+	{ "ad7091r4", (kernel_ulong_t)&ad7091r4_init_info },
+	{ "ad7091r8", (kernel_ulong_t)&ad7091r8_init_info },
+	{}
Trivial but be consistent on spacing for these terminators. I like a space, so { } but I don't mind if an author prefers {} as long as they are consistent!
Quoted 2 lines
drivers/iio/adc/ad7091r8.c new file +2 -0
+};
+MODULE_DEVICE_TABLE(spi, ad7091r8_spi_id);

Marcelo Schmitt

author maintainer

2023-12-07 18:43 UTC #29 lore raw

PATCH v3 13/13 MAINTAINERS: Add MAINTAINERS entry for AD7091R

In reply to Marcelo Schmitt

The driver for AD7091R was added in
ca693001: iio: adc: Add support for AD7091R5 ADC but no MAINTAINERS file entry was added for it since then. Add a proper MAINTAINERS file entry for the AD7091R driver.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 MAINTAINERS | 12 ++++++++++++
 1 file changed, 12 insertions(+)

MAINTAINERS +12 -0

index 1338e1176ea5..a6957678f546 100644
@@ -1126,6 +1126,18 @@ F:	Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
 F:	drivers/iio/adc/ad4130.c
 
+ANALOG DEVICES INC AD7091R DRIVER
+M:	Marcelo Schmitt <[email protected]>
+L:	[email protected]
+S:	Supported
+W:	http://ez.analog.com/community/linux-device-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r5.yaml
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r-base.c
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r-base.h
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r5.c
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r8.c
+
 ANALOG DEVICES INC AD7192 DRIVER
 M:	Alexandru Tachici <[email protected]>
 L:	[email protected]

Signature · 1 line

2.42.0

Joe Perches

maintainer

2023-12-07 18:58 UTC #30 lore raw

Re: [PATCH v3 13/13] MAINTAINERS: Add MAINTAINERS entry for AD7091R

Quoted 4 lines · Marcelo Schmitt

The driver for AD7091R was added in
ca693001: iio: adc: Add support for AD7091R5 ADC but no MAINTAINERS file entry was added for it since then. Add a proper MAINTAINERS file entry for the AD7091R driver.

[]

MAINTAINERS

Quoted 1 line

MAINTAINERS +0 -0

[]

Quoted 5 lines

MAINTAINERS +1 -0

@@ -1126,6 +1126,18 @@ F:	Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
 F:	drivers/iio/adc/ad4130.c
 
+ANALOG DEVICES INC AD7091R DRIVER

MAINTAINERS F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 · line 1139

Quoted 10 lines

MAINTAINERS +10 -0

+M:	Marcelo Schmitt <[email protected]>
+L:	[email protected]
+S:	Supported
+W:	http://ez.analog.com/community/linux-device-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r5.yaml
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r-base.c
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r-base.h
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r5.c
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r8.c

Maybe use wildcards?

F: Documentation/devicetree/bindings/iio/adc/adi,ad7091r*
F: drivers/iio/adc/drivers/iio/adc/ad7091r*

Marcelo Schmitt

2023-12-08 12:12 UTC #31 lore raw

Re: [PATCH v3 13/13] MAINTAINERS: Add MAINTAINERS entry for AD7091R

Quoted 26 lines · Joe Perches

Quoted 4 lines · Marcelo Schmitt
The driver for AD7091R was added in
ca693001: iio: adc: Add support for AD7091R5 ADC but no MAINTAINERS file entry was added for it since then. Add a proper MAINTAINERS file entry for the AD7091R driver.
[]
MAINTAINERS
Quoted 1 line
MAINTAINERS +0 -0
[]
Quoted 5 lines
MAINTAINERS +1 -0
@@ -1126,6 +1126,18 @@ F:	Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
 F:	drivers/iio/adc/ad4130.c
 
+ANALOG DEVICES INC AD7091R DRIVER
MAINTAINERS F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 · line 1139
Quoted 10 lines
MAINTAINERS +10 -0
+M:	Marcelo Schmitt <[email protected]>
+L:	[email protected]
+S:	Supported
+W:	http://ez.analog.com/community/linux-device-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r5.yaml
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r-base.c
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r-base.h
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r5.c
+F:	drivers/iio/adc/drivers/iio/adc/ad7091r8.c
Maybe use wildcards?

Quoted 3 lines

F: Documentation/devicetree/bindings/iio/adc/adi,ad7091r*
F: drivers/iio/adc/drivers/iio/adc/ad7091r*

Good idea, will do for v4.

Thanks

David Lechner

reviewer

2023-12-07 23:26 UTC #32 lore raw

In reply to Marcelo Schmitt

Quoted 29 lines · Marcelo Schmitt

From: Marcelo Schmitt <[email protected]>
----------------- Updates -----------------
Applied all changes suggested to the previous series.
I tried to better explain the changes but, since there is a fair amount of rework in ad7091-base and ad7091r5, it may be hard to get the reasoning for the early patches before looking at the patch for ad7091r8.
Change log v2 -> v3:
- Split alert fix patch into 2 fix patches and one alignment cleanup patch
- Corrected Fixes tag format
- Moved MAINTAINERS update to the end of the series
- Reworded some commit messages to provide context and make their goal clearer
- Removed erroneous gmail sign off
- Created container struct to store chip_info, regmap_config, and callbacks specific to each ADC design
- Created callbacks for chip specific tasks such as setting device operation mode
- Dropped the chip type enum struct
- Applied suggestions related to device tree documentation
- Added __aligned to list the of checkpatch attribute notes
- Other code style tidy ups.
I see regmap's interface for reading device registers under /sys/kernel/debug/regmap/. I can read all registers but can't write to any of them unless I force define REGMAP_ALLOW_WRITE_DEBUGFS.

Quoted 8 lines

When testing events for this driver I often write to device registers to set different rising/falling thresholds. I do something like this:
# echo 0x17 0x100 > /sys/kernel/debug/iio/iio:device0/direct_reg_access
I tried read/writing to files under iio:device events directory but always get segmentation fault. I must be forgetting to implement something. What am I missing?

It looks like event callbacks (.read_event_value and friends) are missing from static const struct iio_info ad7091r_info = { ... }. These callbacks aren't checked for NULL, e.g. in iio_ev_value_show(), so that is likely where the segfault is happening.

Marcelo Schmitt

2023-12-08 13:50 UTC #33 lore raw

Quoted 5 lines · David Lechner

Quoted 3 lines · Marcelo Schmitt
From: Marcelo Schmitt <[email protected]>

[...]

Quoted 14 lines

Quoted 5 lines
I see regmap's interface for reading device registers under /sys/kernel/debug/regmap/. I can read all registers but can't write to any of them unless I force define REGMAP_ALLOW_WRITE_DEBUGFS.
Quoted 8 lines
When testing events for this driver I often write to device registers to set different rising/falling thresholds. I do something like this:
# echo 0x17 0x100 > /sys/kernel/debug/iio/iio:device0/direct_reg_access
I tried read/writing to files under iio:device events directory but always get segmentation fault. I must be forgetting to implement something. What am I missing?

Quoted 4 lines

It looks like event callbacks (.read_event_value and friends) are missing from static const struct iio_info ad7091r_info = { ... }. These callbacks aren't checked for NULL, e.g. in iio_ev_value_show(), so that is likely where the segfault is happening.

Hi David, thank you for pointing that out.
Will implement those calls in v4.