[PATCH v3 00/13] Add support for AD7091R-2/-4/-8

[PATCH v3 00/13] Add support for AD7091R-2/-4/-8

[Marcelo Schmitt]

From: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

----------------- 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

-- 
2.42.0

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

[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 <marcelo.schmitt@analog.com>
---
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(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda112..e6773ae0ad08 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -512,6 +512,7 @@ our $Attribute	= qr{
 			__ro_after_init|
 			__kprobes|
 			$InitAttribute|
+			__aligned|
 			____cacheline_aligned|
 			____cacheline_aligned_in_smp|
 			____cacheline_internodealigned_in_smp|
-- 
2.42.0

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

[Joe Perches]

On Thu, 2023-12-07 at 15:37 -0300, Marcelo Schmitt wrote:
> 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 <marcelo.schmitt@analog.com>
> ---
> 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(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 25fdb7fda112..e6773ae0ad08 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -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));

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

[Marcelo Schmitt]

On 12/07, Joe Perches wrote:
> On Thu, 2023-12-07 at 15:37 -0300, Marcelo Schmitt wrote:
> > 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 <marcelo.schmitt@analog.com>
> > ---
> > 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(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 25fdb7fda112..e6773ae0ad08 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -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));
> 

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

Thanks

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

[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 <marcelo.schmitt@analog.com>
---
 drivers/iio/adc/ad7091r-base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 8e252cde735b..0f192fbecbd4 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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);
-- 
2.42.0

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

[David Lechner]

On Thu, Dec 7, 2023 at 12:38 PM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> 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 <marcelo.schmitt@analog.com>
> ---
>  drivers/iio/adc/ad7091r-base.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> index 8e252cde735b..0f192fbecbd4 100644
> --- a/drivers/iio/adc/ad7091r-base.c
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -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);
> --
> 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?

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 8e252cde735b..0e5d3d2e9c98 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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;
     }

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

[Marcelo Schmitt]

On 12/07, David Lechner wrote:
> On Thu, Dec 7, 2023 at 12:38 PM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> >
> > Set device driver data so it can be retrieved when handling alert
> > events, avoiding null pointer dereference.
> >

[...]

> 
> 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?
> 
> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> index 8e252cde735b..0e5d3d2e9c98 100644
> --- a/drivers/iio/adc/ad7091r-base.c
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -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;
>      }

Looks good, will do for v4.

Thanks

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

[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 <marcelo.schmitt@analog.com>
---
 drivers/iio/adc/ad7091r-base.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 0f192fbecbd4..6056a66d756c 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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,
-- 
2.42.0

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

[Jonathan Cameron]

On Thu, 7 Dec 2023 15:38:53 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> 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 <marcelo.schmitt@analog.com>
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

> ---
>  drivers/iio/adc/ad7091r-base.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> index 0f192fbecbd4..6056a66d756c 100644
> --- a/drivers/iio/adc/ad7091r-base.c
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -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,

[PATCH v3 04/13] iio: adc: ad7091r: Align arguments to function call parenthesis

[Marcelo Schmitt]

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

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 drivers/iio/adc/ad7091r-base.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 6056a66d756c..3ecac3164446 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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;
 	}
-- 
2.42.0

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

[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 <marcelo.schmitt@analog.com>
---
 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(-)

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 3ecac3164446..4d0a1eeebb8a 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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,
diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
index 509748aef9b1..1d30eeb46bcc 100644
--- a/drivers/iio/adc/ad7091r-base.h
+++ b/drivers/iio/adc/ad7091r-base.h
@@ -8,8 +8,43 @@
 #ifndef __DRIVERS_IIO_ADC_AD7091R_BASE_H__
 #define __DRIVERS_IIO_ADC_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)
+
+#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,					\
+}
+
 struct device;
-struct ad7091r_state;
+
+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 */
+};
 
 struct ad7091r_chip_info {
 	unsigned int num_channels;
@@ -17,10 +52,15 @@ struct ad7091r_chip_info {
 	unsigned int vref_mV;
 };
 
+extern const struct iio_event_spec ad7091r_events[3];
+
 extern const struct regmap_config ad7091r_regmap_config;
 
 int ad7091r_probe(struct device *dev, const char *name,
 		const struct ad7091r_chip_info *chip_info,
 		struct regmap *map, int irq);
 
+bool ad7091r_volatile_reg(struct device *dev, unsigned int reg);
+bool ad7091r_writeable_reg(struct device *dev, unsigned int reg);
+
 #endif /* __DRIVERS_IIO_ADC_AD7091R_BASE_H__ */
diff --git a/drivers/iio/adc/ad7091r5.c b/drivers/iio/adc/ad7091r5.c
index 2f048527b7b7..9d3ccfca94ec 100644
--- a/drivers/iio/adc/ad7091r5.c
+++ b/drivers/iio/adc/ad7091r5.c
@@ -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[] = {
-- 
2.42.0

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

[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 <marcelo.schmitt@analog.com>
---
 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(-)

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 4d0a1eeebb8a..90470b4a98c5 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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 <beniamin.bia@analog.com>");
 MODULE_DESCRIPTION("Analog Devices AD7091Rx multi-channel converters");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
index 1d30eeb46bcc..d58e2b08015a 100644
--- a/drivers/iio/adc/ad7091r-base.h
+++ b/drivers/iio/adc/ad7091r-base.h
@@ -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);
diff --git a/drivers/iio/adc/ad7091r5.c b/drivers/iio/adc/ad7091r5.c
index 9d3ccfca94ec..51aad8df7f3a 100644
--- a/drivers/iio/adc/ad7091r5.c
+++ b/drivers/iio/adc/ad7091r5.c
@@ -27,42 +27,61 @@ static const struct iio_chan_spec ad7091r5_channels_noirq[] = {
 };
 
 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,
 };
 
 static const struct ad7091r_chip_info ad7091r5_chip_info_noirq = {
+	.name = "ad7091r-5",
 	.channels = ad7091r5_channels_noirq,
 	.num_channels = ARRAY_SIZE(ad7091r5_channels_noirq),
 	.vref_mV = 2500,
 };
 
+static const struct regmap_config ad7091r_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.writeable_reg = ad7091r_writeable_reg,
+	.volatile_reg = ad7091r_volatile_reg,
+};
+
+static void ad7091r5_regmap_init(struct ad7091r_state *st,
+				 const struct regmap_config *regmap_conf)
+{
+	struct i2c_client *i2c = container_of(st->dev, struct i2c_client, dev);
+
+	st->map = devm_regmap_init_i2c(i2c, regmap_conf);
+}
+
+static struct ad7091r_init_info ad7091r5_init_info = {
+	.irq_info = ad7091r5_chip_info_irq,
+	.info_no_irq = ad7091r5_chip_info_noirq,
+	.regmap_config = &ad7091r_regmap_config,
+	.ad7091r_regmap_init = &ad7091r5_regmap_init
+};
+
 static int ad7091r5_i2c_probe(struct i2c_client *i2c)
 {
 	const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
-	const struct ad7091r_chip_info *chip_info;
-	struct regmap *map = devm_regmap_init_i2c(i2c, &ad7091r_regmap_config);
-
-	if (IS_ERR(map))
-		return PTR_ERR(map);
+	const struct ad7091r_init_info *init_info;
 
-	if (i2c->irq)
-		chip_info = &ad7091r5_chip_info_irq;
-	else
-		chip_info = &ad7091r5_chip_info_noirq;
+	init_info = i2c_get_match_data(i2c);
+	if (!init_info)
+		return -EINVAL;
 
-	return ad7091r_probe(&i2c->dev, id->name, chip_info, map, i2c->irq);
+	return ad7091r_probe(&i2c->dev, id->name, init_info, NULL, i2c->irq);
 }
 
 static const struct of_device_id ad7091r5_dt_ids[] = {
-	{ .compatible = "adi,ad7091r5" },
+	{ .compatible = "adi,ad7091r5", .data = &ad7091r5_init_info },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ad7091r5_dt_ids);
 
 static const struct i2c_device_id ad7091r5_i2c_ids[] = {
-	{"ad7091r5", 0},
+	{"ad7091r5", (kernel_ulong_t)&ad7091r5_init_info },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, ad7091r5_i2c_ids);
-- 
2.42.0

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

[kernel test robot]

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/202312082022.IOtnLeck-lkp@intel.com/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/202312082022.IOtnLeck-lkp@intel.com/reproduce)

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

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	

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

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

[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 <marcelo.schmitt@analog.com>
---
 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(-)

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 90470b4a98c5..f2cb638b8d77 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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;
 
diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
index d58e2b08015a..9546d0bf1da7 100644
--- a/drivers/iio/adc/ad7091r-base.h
+++ b/drivers/iio/adc/ad7091r-base.h
@@ -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 {
diff --git a/drivers/iio/adc/ad7091r5.c b/drivers/iio/adc/ad7091r5.c
index 51aad8df7f3a..ed5e00cc82e2 100644
--- a/drivers/iio/adc/ad7091r5.c
+++ b/drivers/iio/adc/ad7091r5.c
@@ -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 = {
-- 
2.42.0

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

[Jonathan Cameron]

On Thu, 7 Dec 2023 15:41:03 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> 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 <marcelo.schmitt@analog.com>

Hi Marcelo,
> +
>  #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.

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

[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 <marcelo.schmitt@analog.com>
---
 drivers/iio/adc/ad7091r-base.c | 9 ++++++---
 drivers/iio/adc/ad7091r-base.h | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index f2cb638b8d77..59a7ec44955d 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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)
diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
index 9546d0bf1da7..e153c2d7deb5 100644
--- a/drivers/iio/adc/ad7091r-base.h
+++ b/drivers/iio/adc/ad7091r-base.h
@@ -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)
-- 
2.42.0

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

[Jonathan Cameron]

On Thu, 7 Dec 2023 15:41:25 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> 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 <marcelo.schmitt@analog.com>
> ---
>  drivers/iio/adc/ad7091r-base.c | 9 ++++++---
>  drivers/iio/adc/ad7091r-base.h | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
> index f2cb638b8d77..59a7ec44955d 100644
> --- a/drivers/iio/adc/ad7091r-base.c
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -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.

> -	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.


> +		/* 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)
> diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
> index 9546d0bf1da7..e153c2d7deb5 100644
> --- a/drivers/iio/adc/ad7091r-base.h
> +++ b/drivers/iio/adc/ad7091r-base.h
> @@ -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)

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

[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 <marcelo.schmitt@analog.com>
---
 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(-)

diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 59a7ec44955d..5a7046f6f0ce 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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);
diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
index e153c2d7deb5..c554f04e7448 100644
--- a/drivers/iio/adc/ad7091r-base.h
+++ b/drivers/iio/adc/ad7091r-base.h
@@ -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);
 };
 
diff --git a/drivers/iio/adc/ad7091r5.c b/drivers/iio/adc/ad7091r5.c
index ed5e00cc82e2..53a3f74f6452 100644
--- a/drivers/iio/adc/ad7091r5.c
+++ b/drivers/iio/adc/ad7091r5.c
@@ -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,
 };
 
-- 
2.42.0

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

[Marcelo Schmitt]

Add device tree documentation for AD7091R-8.

Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
 .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
new file mode 100644
index 000000000000..02320778f225
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
@@ -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 <marcelo.schmitt@analog.com>
+
+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>;
+        };
+    };
+...
-- 
2.42.0

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

[David Lechner]

On Thu, Dec 7, 2023 at 12:42 PM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> Add device tree documentation for AD7091R-8.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> new file mode 100644
> index 000000000000..02320778f225
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> @@ -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 <marcelo.schmitt@analog.com>
> +
> +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
> +

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

> +  vref-supply: true

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

> +
> +  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.

> +    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.

> +
> +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

> +
> +    required:
> +      - reg

Missing `unevaluatedProperties: false` for channels?

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

> +
> +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

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

> +    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
>
>

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

[Marcelo Schmitt]

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

On 12/07, David Lechner wrote:
> On Thu, Dec 7, 2023 at 12:42 PM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> >
> > Add device tree documentation for AD7091R-8.
> >
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> >  .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
> >  1 file changed, 99 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> > new file mode 100644
> > index 000000000000..02320778f225
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> > @@ -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 <marcelo.schmitt@analog.com>
> > +
> > +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
> > +
> 
> 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?

> > +  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.

> > +
> > +  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.

> 
> > +    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.

> > +
> > +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
> 

Ack

> > +
> > +    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.

> > +
> > +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
> 
> 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.

> > +    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
> >
> >

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

[David Lechner]

On Fri, Dec 8, 2023 at 7:28 AM Marcelo Schmitt
<marcelo.schmitt1@gmail.com> wrote:
>
> Hi David, thank you for your suggestions.
> Comments inline.
>
> On 12/07, David Lechner wrote:
> > On Thu, Dec 7, 2023 at 12:42 PM Marcelo Schmitt
> > <marcelo.schmitt@analog.com> wrote:
> > >
> > > Add device tree documentation for AD7091R-8.
> > >
> > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > > ---
> > >  .../bindings/iio/adc/adi,ad7091r8.yaml        | 99 +++++++++++++++++++
> > >  1 file changed, 99 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> > > new file mode 100644
> > > index 000000000000..02320778f225
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7091r8.yaml
> > > @@ -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 <marcelo.schmitt@analog.com>
> > > +
> > > +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
> > > +
> >
> > 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.

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

> 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.

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

[Jonathan Cameron]

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


> > > +
> > > +    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.

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

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

[Krzysztof Kozlowski]

On 07/12/2023 19:42, Marcelo Schmitt wrote:
> Add device tree documentation for AD7091R-8.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Except what David said also:

> +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

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

[Marcelo Schmitt]

On 12/08, Krzysztof Kozlowski wrote:
> On 07/12/2023 19:42, Marcelo Schmitt wrote:
> > Add device tree documentation for AD7091R-8.
> > 
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> 
> Except what David said also:
> 
> > +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

> 
> Best regards,
> Krzysztof
> 

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

[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 <marcelo.schmitt@analog.com>
---
 drivers/iio/adc/Kconfig  | 4 ++++
 drivers/iio/adc/Makefile | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1e2b7a2c67c6..dcf45d7478e1 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -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.
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index c0803383a7cc..1e289d674d4d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -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
-- 
2.42.0

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

[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 <marcelo.schmitt@analog.com>
---
 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

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dcf45d7478e1..284d898790a2 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -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
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 1e289d674d4d..d2fda54a3259 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -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
diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-base.c
index 5a7046f6f0ce..0add6e144d63 100644
--- a/drivers/iio/adc/ad7091r-base.c
+++ b/drivers/iio/adc/ad7091r-base.c
@@ -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,
diff --git a/drivers/iio/adc/ad7091r-base.h b/drivers/iio/adc/ad7091r-base.h
index c554f04e7448..7140d5fd0ac2 100644
--- a/drivers/iio/adc/ad7091r-base.h
+++ b/drivers/iio/adc/ad7091r-base.h
@@ -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);
 };
diff --git a/drivers/iio/adc/ad7091r8.c b/drivers/iio/adc/ad7091r8.c
new file mode 100644
index 000000000000..8dc0f784913b
--- /dev/null
+++ b/drivers/iio/adc/ad7091r8.c
@@ -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>
+
+#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"
+
+#define AD7091R_SPI_REGMAP_CONFIG(n) {					\
+	.reg_bits = 8,							\
+	.val_bits = 16,							\
+	.volatile_reg = ad7091r_volatile_reg,				\
+	.writeable_reg = ad7091r_writeable_reg,				\
+	.max_register = AD7091R_REG_CH_HYSTERESIS(n),			\
+}
+
+#define AD7091R_SPI_CHIP_INFO(n) {					\
+	.name = AD7091R##n##_DEV_NAME,					\
+	.channels = ad7091r##n##_channels,				\
+	.num_channels = ARRAY_SIZE(ad7091r##n##_channels),		\
+	.vref_mV = 2500,						\
+	.ad7091r_reg_result_chan_id = &ad7091r8_reg_result_chan_id,	\
+	.ad7091r_set_mode = &ad7091r8_set_mode,				\
+}
+
+#define AD7091R_SPI_CHIP_INFO_IRQ(n) {					\
+	.name = AD7091R##n##_DEV_NAME,					\
+	.channels = ad7091r##n##_channels_irq,				\
+	.num_channels = ARRAY_SIZE(ad7091r##n##_channels_irq),		\
+	.vref_mV = 2500,						\
+	.ad7091r_reg_result_chan_id = &ad7091r8_reg_result_chan_id,	\
+	.ad7091r_set_mode = &ad7091r8_set_mode,				\
+}
+
+static const struct iio_chan_spec ad7091r2_channels[] = {
+	AD7091R_CHANNEL(0, 12, NULL, 0),
+	AD7091R_CHANNEL(1, 12, NULL, 0),
+};
+
+static const struct iio_chan_spec ad7091r4_channels[] = {
+	AD7091R_CHANNEL(0, 12, NULL, 0),
+	AD7091R_CHANNEL(1, 12, NULL, 0),
+	AD7091R_CHANNEL(2, 12, NULL, 0),
+	AD7091R_CHANNEL(3, 12, NULL, 0),
+};
+
+static const struct iio_chan_spec ad7091r4_channels_irq[] = {
+	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 ad7091r8_channels[] = {
+	AD7091R_CHANNEL(0, 12, NULL, 0),
+	AD7091R_CHANNEL(1, 12, NULL, 0),
+	AD7091R_CHANNEL(2, 12, NULL, 0),
+	AD7091R_CHANNEL(3, 12, NULL, 0),
+	AD7091R_CHANNEL(4, 12, NULL, 0),
+	AD7091R_CHANNEL(5, 12, NULL, 0),
+	AD7091R_CHANNEL(6, 12, NULL, 0),
+	AD7091R_CHANNEL(7, 12, NULL, 0),
+};
+
+static const struct iio_chan_spec ad7091r8_channels_irq[] = {
+	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)),
+	AD7091R_CHANNEL(4, 12, ad7091r_events, ARRAY_SIZE(ad7091r_events)),
+	AD7091R_CHANNEL(5, 12, ad7091r_events, ARRAY_SIZE(ad7091r_events)),
+	AD7091R_CHANNEL(6, 12, ad7091r_events, ARRAY_SIZE(ad7091r_events)),
+	AD7091R_CHANNEL(7, 12, ad7091r_events, ARRAY_SIZE(ad7091r_events)),
+};
+
+static void ad7091r_pulse_convst(struct ad7091r_state *st)
+{
+	gpiod_set_value_cansleep(st->convst_gpio, 1);
+	gpiod_set_value_cansleep(st->convst_gpio, 0);
+}
+
+static int ad7091r_regmap_bus_reg_read(void *context, unsigned int reg,
+				       unsigned int *val)
+{
+	struct ad7091r_state *st = context;
+	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
+	int ret;
+
+	struct spi_transfer t[] = {
+		{
+			.tx_buf = &st->tx_buf,
+			.len = 2,
+			.cs_change = 1,
+		}, {
+			.rx_buf = &st->rx_buf,
+			.len = 2,
+		}
+	};
+
+	if (reg == AD7091R_REG_RESULT)
+		ad7091r_pulse_convst(st);
+
+	st->tx_buf = cpu_to_be16(reg << 11);
+
+	ret = spi_sync_transfer(spi, t, ARRAY_SIZE(t));
+	if (ret < 0)
+		return ret;
+
+	*val = be16_to_cpu(st->rx_buf);
+	return 0;
+}
+
+static int ad7091r_regmap_bus_reg_write(void *context, unsigned int reg,
+					unsigned int val)
+{
+	struct ad7091r_state *st = context;
+	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
+
+	/*
+	 * AD7091R-2/-4/-8 protocol (datasheet page 31) is to do a single SPI
+	 * transfer with reg address set in bits B15:B11 and value set in B9:B0.
+	 */
+	st->tx_buf = cpu_to_be16(FIELD_PREP(AD7091R8_REG_DATA_MSK, val) |
+				 FIELD_PREP(AD7091R8_RD_WR_FLAG_MSK, 1) |
+				 FIELD_PREP(AD7091R8_REG_ADDR_MSK, reg));
+
+	return spi_write(spi, &st->tx_buf, 2);
+}
+
+static struct regmap_bus ad7091r8_regmap_bus = {
+	.reg_read = ad7091r_regmap_bus_reg_read,
+	.reg_write = ad7091r_regmap_bus_reg_write,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG,
+};
+
+static const struct regmap_config ad7091r2_reg_conf = AD7091R_SPI_REGMAP_CONFIG(2);
+static const struct regmap_config ad7091r4_reg_conf = AD7091R_SPI_REGMAP_CONFIG(4);
+static const struct regmap_config ad7091r8_reg_conf = AD7091R_SPI_REGMAP_CONFIG(8);
+
+static void ad7091r8_regmap_init(struct ad7091r_state *st,
+				 const struct regmap_config *regmap_conf)
+{
+	st->map = devm_regmap_init(st->dev, &ad7091r8_regmap_bus, st,
+				   regmap_conf);
+}
+
+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);
+
+	if (st->reset_gpio) {
+		fsleep(20);
+		gpiod_set_value_cansleep(st->reset_gpio, 0);
+	}
+
+	return 0;
+}
+
+static int ad7091r8_set_mode(struct ad7091r_state *st, enum ad7091r_mode mode)
+{
+	/* AD7091R-2/-4/-8 don't set sample/command/autocycle mode in conf reg */
+	st->mode = mode;
+	return 0;
+}
+
+static unsigned int ad7091r8_reg_result_chan_id(unsigned int val)
+{
+	return AD7091R8_REG_RESULT_CH_ID(val);
+}
+
+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
+};
+
+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);
+}
+
+static const struct of_device_id ad7091r8_of_match[] = {
+	{ .compatible = "adi,ad7091r2", .data = &ad7091r2_init_info },
+	{ .compatible = "adi,ad7091r4", .data = &ad7091r4_init_info },
+	{ .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 },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad7091r8_spi_id);
+
+static struct spi_driver ad7091r8_driver = {
+	.driver = {
+		.name = "ad7091r8",
+		.of_match_table = ad7091r8_of_match,
+	},
+	.probe = ad7091r8_spi_probe,
+	.id_table = ad7091r8_spi_id,
+};
+module_spi_driver(ad7091r8_driver);
+
+MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7091R8 ADC driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_AD7091R);
-- 
2.42.0

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

[Jonathan Cameron]

On Thu, 7 Dec 2023 15:42:56 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> 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 <marcelo.schmitt@analog.com>

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

Jonathan
> diff --git a/drivers/iio/adc/ad7091r8.c b/drivers/iio/adc/ad7091r8.c
> new file mode 100644
> index 000000000000..8dc0f784913b
> --- /dev/null
> +++ b/drivers/iio/adc/ad7091r8.c
> @@ -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>
> +
> +#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.

> +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
> +
> +	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
> +};
> +
> +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!

> +}
> +
> +static const struct of_device_id ad7091r8_of_match[] = {
> +	{ .compatible = "adi,ad7091r2", .data = &ad7091r2_init_info },
> +	{ .compatible = "adi,ad7091r4", .data = &ad7091r4_init_info },
> +	{ .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!

> +};
> +MODULE_DEVICE_TABLE(spi, ad7091r8_spi_id);

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

[Marcelo Schmitt]

Hi Jonathan,

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

Thanks,
Marcelo

On 12/10, Jonathan Cameron wrote:
> On Thu, 7 Dec 2023 15:42:56 -0300
> Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:
> 
> > 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 <marcelo.schmitt@analog.com>
> 
> A few trivial things inline.
> Otherwise looks pretty good to me.
> 
> Jonathan
> > diff --git a/drivers/iio/adc/ad7091r8.c b/drivers/iio/adc/ad7091r8.c
> > new file mode 100644
> > index 000000000000..8dc0f784913b
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad7091r8.c
> > @@ -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>
> > +
> > +#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.
> 
> > +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
> > +
> > +	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
> > +};
> > +
> > +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!
> 
> > +}
> > +
> > +static const struct of_device_id ad7091r8_of_match[] = {
> > +	{ .compatible = "adi,ad7091r2", .data = &ad7091r2_init_info },
> > +	{ .compatible = "adi,ad7091r4", .data = &ad7091r4_init_info },
> > +	{ .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!
> 
> > +};
> > +MODULE_DEVICE_TABLE(spi, ad7091r8_spi_id);
> 

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

[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 <marcelo.schmitt@analog.com>
---
 MAINTAINERS | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1338e1176ea5..a6957678f546 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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 <marcelo.schmitt@analog.com>
+L:	linux-iio@vger.kernel.org
+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 <alexandru.tachici@analog.com>
 L:	linux-iio@vger.kernel.org
-- 
2.42.0

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

[Joe Perches]

On Thu, 2023-12-07 at 15:43 -0300, Marcelo Schmitt wrote:
> 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.
[]
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -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 <marcelo.schmitt@analog.com>
> +L:	linux-iio@vger.kernel.org
> +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*

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

[Marcelo Schmitt]

On 12/07, Joe Perches wrote:
> On Thu, 2023-12-07 at 15:43 -0300, Marcelo Schmitt wrote:
> > 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.
> []
> > diff --git a/MAINTAINERS b/MAINTAINERS
> []
> > @@ -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 <marcelo.schmitt@analog.com>
> > +L:	linux-iio@vger.kernel.org
> > +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*
> 

Good idea, will do for v4.

Thanks

Re: [PATCH v3 00/13] Add support for AD7091R-2/-4/-8

[David Lechner]

On Thu, Dec 7, 2023 at 12:36 PM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> From: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
>
> ----------------- 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?
>

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.

Re: [PATCH v3 00/13] Add support for AD7091R-2/-4/-8

[Marcelo Schmitt]

On 12/07, David Lechner wrote:
> On Thu, Dec 7, 2023 at 12:36 PM Marcelo Schmitt
> <marcelo.schmitt@analog.com> wrote:
> >
> > From: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> >
[...]
> >
> > 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?
> >
> 
> 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.