[PATCH v2 0/4] iio: adc: ad7192: Add support for AD7194

[PATCH v2 0/4] iio: adc: ad7192: Add support for AD7194

[Alisa-Dariana Roman]

Dear maintainers,

Thank you very much for your feedback!

I am submitting the upgraded series of patches for the ad7192 driver.

Please consider applying the last 3 patches in order.

Thank you!

v1 -> v2
	- new commit with missing documentation for properties
	- add constraint for channels in binding
	- correct pattern for channels
	- correct commit message by adding "()" to functions
	- use in_range
	- use preferred structure in Kconfig

Kind regards,

Alisa-Dariana Roman (4):
  dt-bindings: iio: adc: ad7192: Add properties
  iio: adc: ad7192: Use device api
  dt-bindings: iio: adc: ad7192: Add AD7194 support
  iio: adc: ad7192: Add AD7194 support

 .../bindings/iio/adc/adi,ad7192.yaml          |  84 +++++++++
 drivers/iio/adc/Kconfig                       |  11 +-
 drivers/iio/adc/ad7192.c                      | 176 ++++++++++++++++--
 3 files changed, 249 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] dt-bindings: iio: adc: ad7192: Add properties

[Alisa-Dariana Roman]

Document properties used for clock configuration.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7192.yaml        | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 16def2985ab4..9b59d6eea368 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -80,6 +80,16 @@ properties:
       and when chop is disabled.
     type: boolean
 
+  adi,clock-xtal:
+    description: |
+      External crystal connected from MCLK1 to MCLK2.
+    type: boolean
+
+  adi,int-clock-output-enable:
+    description: |
+      Internal 4.92 MHz clock available on MCLK2 pin.
+    type: boolean
+
   bipolar:
     description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
     type: boolean
-- 
2.34.1

Re: [PATCH v2 1/4] dt-bindings: iio: adc: ad7192: Add properties

[Krzysztof Kozlowski]

On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
> Document properties used for clock configuration.

Some background here is missing - otherwise it looks like you are adding
new properties...

> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7192.yaml        | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index 16def2985ab4..9b59d6eea368 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -80,6 +80,16 @@ properties:
>        and when chop is disabled.
>      type: boolean
>  
> +  adi,clock-xtal:
> +    description: |
> +      External crystal connected from MCLK1 to MCLK2.

And this should be input clock.

> +    type: boolean
> +
> +  adi,int-clock-output-enable:
> +    description: |
> +      Internal 4.92 MHz clock available on MCLK2 pin.
> +    type: boolean

This should be clock-cells and clock provider.

Unless you are just documenting already used interface which you do not
want to break...

> +
>    bipolar:
>      description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
>      type: boolean

Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: iio: adc: ad7192: Add properties

[Jonathan Cameron]

On Tue, 14 Nov 2023 21:29:58 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
> > Document properties used for clock configuration.  
> 
> Some background here is missing - otherwise it looks like you are adding
> new properties...
> 
> > 
> > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> > ---
> >  .../devicetree/bindings/iio/adc/adi,ad7192.yaml        | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > index 16def2985ab4..9b59d6eea368 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> > @@ -80,6 +80,16 @@ properties:
> >        and when chop is disabled.
> >      type: boolean
> >  
> > +  adi,clock-xtal:
> > +    description: |
> > +      External crystal connected from MCLK1 to MCLK2.  
> 
> And this should be input clock.

Fair enough. We've been doing this wrong for a while then, but good not
to do it in new bindings.

It's a bit of an oddity as arguably it's just putting an analog component
in as part of an internally generated clock signal.
A clock binding provides info that the crystal is present though I guess,
even though if you actually connected a conventional clock there it
wouldn't work.

> 
> > +    type: boolean
> > +
> > +  adi,int-clock-output-enable:
> > +    description: |
> > +      Internal 4.92 MHz clock available on MCLK2 pin.
> > +    type: boolean  
> 
> This should be clock-cells and clock provider.
> 
> Unless you are just documenting already used interface which you do not
> want to break...
> 
> > +
> >    bipolar:
> >      description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
> >      type: boolean  
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/4] dt-bindings: iio: adc: ad7192: Add properties

[Alisa-Dariana Roman]

On 14.11.2023 22:29, Krzysztof Kozlowski wrote:
> On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
>> Document properties used for clock configuration.
> 
> Some background here is missing - otherwise it looks like you are adding
> new properties...
> 
>>
>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
>> ---
>>   .../devicetree/bindings/iio/adc/adi,ad7192.yaml        | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> index 16def2985ab4..9b59d6eea368 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>> @@ -80,6 +80,16 @@ properties:
>>         and when chop is disabled.
>>       type: boolean
>>   
>> +  adi,clock-xtal:
>> +    description: |
>> +      External crystal connected from MCLK1 to MCLK2.
> 
> And this should be input clock.
> 
>> +    type: boolean
>> +
>> +  adi,int-clock-output-enable:
>> +    description: |
>> +      Internal 4.92 MHz clock available on MCLK2 pin.
>> +    type: boolean
> 
> This should be clock-cells and clock provider.
> 
> Unless you are just documenting already used interface which you do not
> want to break...
> 
>> +
>>     bipolar:
>>       description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
>>       type: boolean
> 
> Best regards,
> Krzysztof
> 

Thank you very much for the feedback!

If I understand correctly, there is already an input clock in the bindings:
```
   clocks:
     maxItems: 1
     description: phandle to the master clock (mclk)

   clock-names:
     items:
       - const: mclk
```

What I wanted to accomplish with this patch is to document these boolean 
properties (from the ad7192 driver code):
```
	/* use internal clock */
	if (!st->mclk) {
		if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
			clock_sel = AD7192_CLK_INT_CO;
	} else {
		if (device_property_read_bool(dev, "adi,clock-xtal"))
			clock_sel = AD7192_CLK_EXT_MCLK1_2;
		else
			clock_sel = AD7192_CLK_EXT_MCLK2;
	}
```

Please let me know how to proceed further!

Kind regards,
Alisa-Dariana Roman

Re: [PATCH v2 1/4] dt-bindings: iio: adc: ad7192: Add properties

[Krzysztof Kozlowski]

On 02/02/2024 15:14, Alisa-Dariana Roman wrote:
> On 14.11.2023 22:29, Krzysztof Kozlowski wrote:
>> On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
>>> Document properties used for clock configuration.
>>
>> Some background here is missing - otherwise it looks like you are adding
>> new properties...
>>
>>>
>>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
>>> ---
>>>   .../devicetree/bindings/iio/adc/adi,ad7192.yaml        | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> index 16def2985ab4..9b59d6eea368 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> @@ -80,6 +80,16 @@ properties:
>>>         and when chop is disabled.
>>>       type: boolean
>>>   
>>> +  adi,clock-xtal:
>>> +    description: |
>>> +      External crystal connected from MCLK1 to MCLK2.
>>
>> And this should be input clock.
>>
>>> +    type: boolean
>>> +
>>> +  adi,int-clock-output-enable:
>>> +    description: |
>>> +      Internal 4.92 MHz clock available on MCLK2 pin.
>>> +    type: boolean
>>
>> This should be clock-cells and clock provider.
>>
>> Unless you are just documenting already used interface which you do not
>> want to break...
>>
>>> +
>>>     bipolar:
>>>       description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
>>>       type: boolean
>>
>> Best regards,
>> Krzysztof
>>
> 
> Thank you very much for the feedback!
> 
> If I understand correctly, there is already an input clock in the bindings:

You tell me...

> 
> What I wanted to accomplish with this patch is to document these boolean 
> properties (from the ad7192 driver code):

Please explain with clear words: do you mean that existing upstream
Linux driver has it? I don't care about some out of tree drivers...


> ```
> 	/* use internal clock */
> 	if (!st->mclk) {
> 		if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
> 			clock_sel = AD7192_CLK_INT_CO;
> 	} else {
> 		if (device_property_read_bool(dev, "adi,clock-xtal"))
> 			clock_sel = AD7192_CLK_EXT_MCLK1_2;
> 		else
> 			clock_sel = AD7192_CLK_EXT_MCLK2;
> 	}
> ```
> 
> Please let me know how to proceed further!

Please open the datasheet of your product and add properties matching
the hardware, not driver.

I don't know what to say more except the same - you want to enable clock
provided from this device on some pin to some other devices? If yes,
then this is clock-provider, so you clock-provider bindings I mentioned
in my first reply.

Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: iio: adc: ad7192: Add properties

[David Lechner]

On Fri, Feb 2, 2024 at 8:14 AM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> On 14.11.2023 22:29, Krzysztof Kozlowski wrote:
> > On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
> >> Document properties used for clock configuration.
> >
> > Some background here is missing - otherwise it looks like you are adding
> > new properties...
> >
> >>
> >> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> >> ---
> >>   .../devicetree/bindings/iio/adc/adi,ad7192.yaml        | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> >> index 16def2985ab4..9b59d6eea368 100644
> >> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> >> @@ -80,6 +80,16 @@ properties:
> >>         and when chop is disabled.
> >>       type: boolean
> >>
> >> +  adi,clock-xtal:
> >> +    description: |
> >> +      External crystal connected from MCLK1 to MCLK2.

A better description could be:

When this flag is present, it indicates that the clock from the clocks
property is a crystal oscillator connected to MCLK1 and MCLK2. When
omitted the clock is a CMOS-compatible clock connected to MCLK2.

> >
> > And this should be input clock.
> >
> >> +    type: boolean
> >> +
> >> +  adi,int-clock-output-enable:
> >> +    description: |
> >> +      Internal 4.92 MHz clock available on MCLK2 pin.
> >> +    type: boolean
> >
> > This should be clock-cells and clock provider.
> >
> > Unless you are just documenting already used interface which you do not
> > want to break...

This property is already used in the mainline Linux driver, so sounds
like the "don't want to break it" case. But it would make sense to
deprecate this property and use standard clock provider bindings
instead.

> >
> >> +
> >>     bipolar:
> >>       description: see Documentation/devicetree/bindings/iio/adc/adc.yaml
> >>       type: boolean
> >
> > Best regards,
> > Krzysztof
> >
>
> Thank you very much for the feedback!
>
> If I understand correctly, there is already an input clock in the bindings:
> ```
>    clocks:
>      maxItems: 1
>      description: phandle to the master clock (mclk)
>
>    clock-names:
>      items:
>        - const: mclk
> ```
>
> What I wanted to accomplish with this patch is to document these boolean
> properties (from the ad7192 driver code):
> ```
>         /* use internal clock */
>         if (!st->mclk) {
>                 if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
>                         clock_sel = AD7192_CLK_INT_CO;
>         } else {
>                 if (device_property_read_bool(dev, "adi,clock-xtal"))
>                         clock_sel = AD7192_CLK_EXT_MCLK1_2;
>                 else
>                         clock_sel = AD7192_CLK_EXT_MCLK2;
>         }
> ```
>
> Please let me know how to proceed further!
>
> Kind regards,
> Alisa-Dariana Roman
>

There was another recent discussion about this exact same clock
input/output on another chip recently [1]. So it would be nice if we
could end up with the same bindings in both cases (cc Ceclan). In the
other thread, it was proposed to have the clocks property to be an
array of two phandles, one for the crystal oscillator and one for the
external clock rather than a single clock and the adi,clock-xtal
property. But that would be a breaking change to these bindings.

[1]: https://lore.kernel.org/linux-iio/20240122-bloating-dyslexic-cbc0258c898a@spud/t/#m4e375aa36dae6da0c319518137f03e2f63e72af9

Re: [PATCH v2 1/4] dt-bindings: iio: adc: ad7192: Add properties

[Krzysztof Kozlowski]

On 02/02/2024 23:20, David Lechner wrote:
>>>
>>> And this should be input clock.
>>>
>>>> +    type: boolean
>>>> +
>>>> +  adi,int-clock-output-enable:
>>>> +    description: |
>>>> +      Internal 4.92 MHz clock available on MCLK2 pin.
>>>> +    type: boolean
>>>
>>> This should be clock-cells and clock provider.
>>>
>>> Unless you are just documenting already used interface which you do not
>>> want to break...
> 
> This property is already used in the mainline Linux driver, so sounds
> like the "don't want to break it" case. But it would make sense to
> deprecate this property and use standard clock provider bindings
> instead.

One could think like that, but what type of process would it create?
Send driver changes WITHOUT binding, then document whatever crap you
have saying "Linux already supports it!".

Whenever such argument is used, I am repeating the same.

Let's be more clear: NAK if this is clock provider and the only argument
is that someone sneaked undocumented interface bypassing review.



Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: iio: adc: ad7192: Add properties

[Nuno Sá]

On Mon, 2024-02-05 at 10:28 +0100, Krzysztof Kozlowski wrote:
> On 02/02/2024 23:20, David Lechner wrote:
> > > > 
> > > > And this should be input clock.
> > > > 
> > > > > +    type: boolean
> > > > > +
> > > > > +  adi,int-clock-output-enable:
> > > > > +    description: |
> > > > > +      Internal 4.92 MHz clock available on MCLK2 pin.
> > > > > +    type: boolean
> > > > 
> > > > This should be clock-cells and clock provider.
> > > > 
> > > > Unless you are just documenting already used interface which you do not
> > > > want to break...
> > 
> > This property is already used in the mainline Linux driver, so sounds
> > like the "don't want to break it" case. But it would make sense to
> > deprecate this property and use standard clock provider bindings
> > instead.
> 
> One could think like that, but what type of process would it create?
> Send driver changes WITHOUT binding, then document whatever crap you
> have saying "Linux already supports it!".
> 
> Whenever such argument is used, I am repeating the same.
> 
> Let's be more clear: NAK if this is clock provider and the only argument
> is that someone sneaked undocumented interface bypassing review.
> 

Fair enough... 

Alisa,

I guess you have two alternatives then:

1) Drop this patch;
2) Add proper clock provider properties.

I would obviously go with 2). You can then take care of backward compatibility in the
driver. Like, if clock-cells is present, ignore the legacy properties and properly
export the clocks we have in the device.

- Nuno Sá

[PATCH v2 2/4] iio: adc: ad7192: Use device api

[Alisa-Dariana Roman]

Replace of.h and corresponding functions with preferred device specific
functions.

Also replace of_device_get_match_data() with
spi_get_device_match_data().

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/adc/ad7192.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index adc3cbe92d6e..48e0357564af 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -17,7 +17,6 @@
 #include <linux/err.h>
 #include <linux/sched.h>
 #include <linux/delay.h>
-#include <linux/of.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -364,19 +363,19 @@ static inline bool ad7192_valid_external_frequency(u32 freq)
 		freq <= AD7192_EXT_FREQ_MHZ_MAX);
 }
 
-static int ad7192_of_clock_select(struct ad7192_state *st)
+static int ad7192_device_clock_select(struct ad7192_state *st)
 {
-	struct device_node *np = st->sd.spi->dev.of_node;
+	struct device *dev = &st->sd.spi->dev;
 	unsigned int clock_sel;
 
 	clock_sel = AD7192_CLK_INT;
 
 	/* use internal clock */
 	if (!st->mclk) {
-		if (of_property_read_bool(np, "adi,int-clock-output-enable"))
+		if (device_property_read_bool(dev, "adi,int-clock-output-enable"))
 			clock_sel = AD7192_CLK_INT_CO;
 	} else {
-		if (of_property_read_bool(np, "adi,clock-xtal"))
+		if (device_property_read_bool(dev, "adi,clock-xtal"))
 			clock_sel = AD7192_CLK_EXT_MCLK1_2;
 		else
 			clock_sel = AD7192_CLK_EXT_MCLK2;
@@ -385,9 +384,10 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
 	return clock_sel;
 }
 
-static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
+static int ad7192_setup(struct iio_dev *indio_dev)
 {
 	struct ad7192_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->sd.spi->dev;
 	bool rej60_en, refin2_en;
 	bool buf_en, bipolar, burnout_curr_en;
 	unsigned long long scale_uv;
@@ -416,26 +416,26 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
 
 	st->conf = FIELD_PREP(AD7192_CONF_GAIN_MASK, 0);
 
-	rej60_en = of_property_read_bool(np, "adi,rejection-60-Hz-enable");
+	rej60_en = device_property_read_bool(dev, "adi,rejection-60-Hz-enable");
 	if (rej60_en)
 		st->mode |= AD7192_MODE_REJ60;
 
-	refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
+	refin2_en = device_property_read_bool(dev, "adi,refin2-pins-enable");
 	if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
 		st->conf |= AD7192_CONF_REFSEL;
 
 	st->conf &= ~AD7192_CONF_CHOP;
 
-	buf_en = of_property_read_bool(np, "adi,buffer-enable");
+	buf_en = device_property_read_bool(dev, "adi,buffer-enable");
 	if (buf_en)
 		st->conf |= AD7192_CONF_BUF;
 
-	bipolar = of_property_read_bool(np, "bipolar");
+	bipolar = device_property_read_bool(dev, "bipolar");
 	if (!bipolar)
 		st->conf |= AD7192_CONF_UNIPOLAR;
 
-	burnout_curr_en = of_property_read_bool(np,
-						"adi,burnout-currents-enable");
+	burnout_curr_en =
+		device_property_read_bool(dev, "adi,burnout-currents-enable");
 	if (burnout_curr_en && buf_en) {
 		st->conf |= AD7192_CONF_BURN;
 	} else if (burnout_curr_en) {
@@ -1117,9 +1117,7 @@ static int ad7192_probe(struct spi_device *spi)
 	}
 	st->int_vref_mv = ret / 1000;
 
-	st->chip_info = of_device_get_match_data(&spi->dev);
-	if (!st->chip_info)
-		st->chip_info = (void *)spi_get_device_id(spi)->driver_data;
+	st->chip_info = spi_get_device_match_data(spi);
 	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = st->chip_info->channels;
@@ -1140,7 +1138,7 @@ static int ad7192_probe(struct spi_device *spi)
 	if (IS_ERR(st->mclk))
 		return PTR_ERR(st->mclk);
 
-	st->clock_sel = ad7192_of_clock_select(st);
+	st->clock_sel = ad7192_device_clock_select(st);
 
 	if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
 	    st->clock_sel == AD7192_CLK_EXT_MCLK2) {
@@ -1152,7 +1150,7 @@ static int ad7192_probe(struct spi_device *spi)
 		}
 	}
 
-	ret = ad7192_setup(indio_dev, spi->dev.of_node);
+	ret = ad7192_setup(indio_dev);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v2 2/4] iio: adc: ad7192: Use device api

[Nuno Sá]

On Tue, 2023-11-14 at 22:05 +0200, Alisa-Dariana Roman wrote:
> Replace of.h and corresponding functions with preferred device specific
> functions.
> 
> Also replace of_device_get_match_data() with
> spi_get_device_match_data().
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

[PATCH v2 3/4] dt-bindings: iio: adc: ad7192: Add AD7194 support

[Alisa-Dariana Roman]

Unlike the other AD719Xs, AD7194 has configurable differential
channels. The default configuration for these channels can be changed
from the devicetree.

Also add an example for AD7194 devicetree.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 .../bindings/iio/adc/adi,ad7192.yaml          | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
index 9b59d6eea368..800b396f5993 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
@@ -21,6 +21,7 @@ properties:
       - adi,ad7190
       - adi,ad7192
       - adi,ad7193
+      - adi,ad7194
       - adi,ad7195
 
   reg:
@@ -108,6 +109,42 @@ required:
 
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      properties:
+        compatible:
+          const: adi,ad7194
+    then:
+      properties:
+        '#address-cells':
+          const: 1
+
+        '#size-cells':
+          const: 0
+
+      patternProperties:
+        "^channel@([0-7a-f])$":
+          type: object
+          $ref: adc.yaml
+          unevaluatedProperties: false
+
+          properties:
+            reg:
+              description: The channel index.
+              minimum: 0
+              maximum: 7
+
+            diff-channels:
+              description: |
+                The differential channel pair for Ad7194 configurable channels. The
+                first channel is the positive input, the second channel is the
+                negative input.
+              items:
+                minimum: 1
+                maximum: 16
+
+          required:
+            - reg
+            - diff-channels
 
 unevaluatedProperties: false
 
@@ -137,3 +174,40 @@ examples:
             adi,burnout-currents-enable;
         };
     };
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "adi,ad7194";
+            reg = <0>;
+            spi-max-frequency = <1000000>;
+            spi-cpol;
+            spi-cpha;
+            clocks = <&ad7192_mclk>;
+            clock-names = "mclk";
+            interrupts = <25 0x2>;
+            interrupt-parent = <&gpio>;
+            dvdd-supply = <&dvdd>;
+            avdd-supply = <&avdd>;
+            vref-supply = <&vref>;
+
+            adi,refin2-pins-enable;
+            adi,rejection-60-Hz-enable;
+            adi,buffer-enable;
+            adi,burnout-currents-enable;
+
+            channel@0 {
+                reg = <0>;
+                diff-channels = <1 6>;
+            };
+
+            channel@1 {
+                reg = <1>;
+                diff-channels = <2 3>;
+            };
+        };
+    };
-- 
2.34.1

Re: [PATCH v2 3/4] dt-bindings: iio: adc: ad7192: Add AD7194 support

[Krzysztof Kozlowski]

On 14/11/2023 21:05, Alisa-Dariana Roman wrote:
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The default configuration for these channels can be changed
> from the devicetree.
> 
> Also add an example for AD7194 devicetree.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---

Where is the changelog?

>  .../bindings/iio/adc/adi,ad7192.yaml          | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index 9b59d6eea368..800b396f5993 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -21,6 +21,7 @@ properties:
>        - adi,ad7190
>        - adi,ad7192
>        - adi,ad7193
> +      - adi,ad7194
>        - adi,ad7195
>  
>    reg:
> @@ -108,6 +109,42 @@ required:
>  
>  allOf:
>    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          const: adi,ad7194
> +    then:
> +      properties:
> +        '#address-cells':

Use consistent quotes through the file, either ' or ".

> +          const: 1
> +
> +        '#size-cells':
> +          const: 0
> +
> +      patternProperties:
> +        "^channel@([0-7a-f])$":

Properties always go to top-level. In allOf you only constrain them or
disallow (: false).

Best regards,
Krzysztof

[PATCH v2 4/4] iio: adc: ad7192: Add AD7194 support

[Alisa-Dariana Roman]

Unlike the other AD719Xs, AD7194 has configurable differential
channels. The default configuration for these channels can be changed
from the devicetree.

The default configuration is hardcoded in order to have a stable number
of channels.

Also modify config AD7192 description for better scaling.

Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
---
 drivers/iio/adc/Kconfig  |  11 ++-
 drivers/iio/adc/ad7192.c | 144 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 150 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1e2b7a2c67c6..05344054b88e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -55,12 +55,17 @@ config AD7124
 	  called ad7124.
 
 config AD7192
-	tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
+	tristate "Analog Devices AD7192 and similar ADC driver"
 	depends on SPI
 	select AD_SIGMA_DELTA
 	help
-	  Say yes here to build support for Analog Devices AD7190,
-	  AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
+	  Say yes here to build support for Analog Devices SPI analog to digital
+	  converters (ADC):
+	  - AD7190
+	  - AD7192
+	  - AD7193
+	  - AD7194
+	  - AD7195
 	  If unsure, say N (but it's safe to say "Y").
 
 	  To compile this driver as a module, choose M here: the
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 48e0357564af..0532678ad665 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
+ * AD7190 AD7192 AD7193 AD7194 AD7195 SPI ADC driver
  *
  * Copyright 2011-2015 Analog Devices Inc.
  */
@@ -125,10 +125,39 @@
 #define AD7193_CH_AIN8		0x480 /* AIN7 - AINCOM */
 #define AD7193_CH_AINCOM	0x600 /* AINCOM - AINCOM */
 
+#define AD7194_CH_TEMP		0x100 /* Temp sensor */
+#define AD7194_CH_AIN1		0x400 /* AIN1 - AINCOM */
+#define AD7194_CH_AIN2		0x410 /* AIN2 - AINCOM */
+#define AD7194_CH_AIN3		0x420 /* AIN3 - AINCOM */
+#define AD7194_CH_AIN4		0x430 /* AIN4 - AINCOM */
+#define AD7194_CH_AIN5		0x440 /* AIN5 - AINCOM */
+#define AD7194_CH_AIN6		0x450 /* AIN6 - AINCOM */
+#define AD7194_CH_AIN7		0x460 /* AIN7 - AINCOM */
+#define AD7194_CH_AIN8		0x470 /* AIN8 - AINCOM */
+#define AD7194_CH_AIN9		0x480 /* AIN9 - AINCOM */
+#define AD7194_CH_AIN10		0x490 /* AIN10 - AINCOM */
+#define AD7194_CH_AIN11		0x4A0 /* AIN11 - AINCOM */
+#define AD7194_CH_AIN12		0x4B0 /* AIN12 - AINCOM */
+#define AD7194_CH_AIN13		0x4C0 /* AIN13 - AINCOM */
+#define AD7194_CH_AIN14		0x4D0 /* AIN14 - AINCOM */
+#define AD7194_CH_AIN15		0x4E0 /* AIN15 - AINCOM */
+#define AD7194_CH_AIN16		0x4F0 /* AIN16 - AINCOM */
+#define AD7194_CH_POS_MASK	GENMASK(7, 4)
+#define AD7194_CH_POS(x)	FIELD_PREP(AD7194_CH_POS_MASK, (x))
+#define AD7194_CH_NEG_MASK	GENMASK(3, 0)
+#define AD7194_CH_NEG(x)	FIELD_PREP(AD7194_CH_NEG_MASK, (x))
+#define AD7194_CH_DIFF(pos, neg) \
+		(AD7194_CH_POS(pos) | AD7194_CH_NEG(neg))
+#define AD7194_CH_DIFF_START	0
+#define AD7194_CH_DIFF_NR	8
+#define AD7194_CH_AIN_START	1
+#define AD7194_CH_AIN_NR	16
+
 /* ID Register Bit Designations (AD7192_REG_ID) */
 #define CHIPID_AD7190		0x4
 #define CHIPID_AD7192		0x0
 #define CHIPID_AD7193		0x2
+#define CHIPID_AD7194		0x3
 #define CHIPID_AD7195		0x6
 #define AD7192_ID_MASK		GENMASK(3, 0)
 
@@ -166,6 +195,7 @@ enum {
 	ID_AD7190,
 	ID_AD7192,
 	ID_AD7193,
+	ID_AD7194,
 	ID_AD7195,
 };
 
@@ -644,6 +674,15 @@ static const struct attribute_group ad7192_attribute_group = {
 	.attrs = ad7192_attributes,
 };
 
+static struct attribute *ad7194_attributes[] = {
+	&iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ad7194_attribute_group = {
+	.attrs = ad7194_attributes,
+};
+
 static struct attribute *ad7195_attributes[] = {
 	&iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
 	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
@@ -928,6 +967,16 @@ static const struct iio_info ad7192_info = {
 	.update_scan_mode = ad7192_update_scan_mode,
 };
 
+static const struct iio_info ad7194_info = {
+	.read_raw = ad7192_read_raw,
+	.write_raw = ad7192_write_raw,
+	.write_raw_get_fmt = ad7192_write_raw_get_fmt,
+	.read_avail = ad7192_read_avail,
+	.attrs = &ad7194_attribute_group,
+	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7192_update_scan_mode,
+};
+
 static const struct iio_info ad7195_info = {
 	.read_raw = ad7192_read_raw,
 	.write_raw = ad7192_write_raw,
@@ -1017,6 +1066,35 @@ static const struct iio_chan_spec ad7193_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(14),
 };
 
+static struct iio_chan_spec ad7194_channels[] = {
+	AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
+	AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
+	AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
+	AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
+	AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
+	AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
+	AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
+	AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
+	AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
+	AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
+	AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
+	AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
+	AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
+	AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
+	AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
+	AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
+	AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
+	AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
+	AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
+	AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
+	AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
+	AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
+	AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
+	AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
+	AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
+	IIO_CHAN_SOFT_TIMESTAMP(25),
+};
+
 static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 	[ID_AD7190] = {
 		.chip_id = CHIPID_AD7190,
@@ -1039,6 +1117,13 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 		.num_channels = ARRAY_SIZE(ad7193_channels),
 		.info = &ad7192_info,
 	},
+	[ID_AD7194] = {
+		.chip_id = CHIPID_AD7194,
+		.name = "ad7194",
+		.channels = ad7194_channels,
+		.num_channels = ARRAY_SIZE(ad7194_channels),
+		.info = &ad7194_info,
+	},
 	[ID_AD7195] = {
 		.chip_id = CHIPID_AD7195,
 		.name = "ad7195",
@@ -1053,6 +1138,53 @@ static void ad7192_reg_disable(void *reg)
 	regulator_disable(reg);
 }
 
+static int ad7192_parse_channel(struct iio_dev *indio_dev,
+				struct fwnode_handle *child)
+{
+	u32 reg, ain[2];
+	int ret;
+
+	ret = fwnode_property_read_u32(child, "reg", &reg);
+	if (ret)
+		return ret;
+
+	if (!in_range(reg, AD7194_CH_DIFF_START, AD7194_CH_DIFF_NR))
+		return -EINVAL;
+
+	ret = fwnode_property_read_u32_array(child, "diff-channels", ain,
+					     ARRAY_SIZE(ain));
+	if (ret)
+		return ret;
+
+	if (!in_range(ain[0], AD7194_CH_AIN_START, AD7194_CH_AIN_NR) ||
+	    !in_range(ain[1], AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
+		return -EINVAL;
+
+	ad7194_channels[reg].channel = ain[0];
+	ad7194_channels[reg].channel2 = ain[1];
+	ad7194_channels[reg].address = AD7194_CH_DIFF(ain[0], ain[1]);
+
+	return 0;
+}
+
+static int ad7192_parse_channels(struct iio_dev *indio_dev)
+{
+	struct ad7192_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->sd.spi->dev;
+	struct fwnode_handle *child;
+	int ret;
+
+	device_for_each_child_node(dev, child) {
+		ret = ad7192_parse_channel(indio_dev, child);
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int ad7192_probe(struct spi_device *spi)
 {
 	struct ad7192_state *st;
@@ -1150,6 +1282,12 @@ static int ad7192_probe(struct spi_device *spi)
 		}
 	}
 
+	if (st->chip_info->chip_id == CHIPID_AD7194) {
+		ret = ad7192_parse_channels(indio_dev);
+		if (ret)
+			return ret;
+	}
+
 	ret = ad7192_setup(indio_dev);
 	if (ret)
 		return ret;
@@ -1161,6 +1299,7 @@ static const struct of_device_id ad7192_of_match[] = {
 	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
 	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
 	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
+	{ .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
 	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
 	{}
 };
@@ -1170,6 +1309,7 @@ static const struct spi_device_id ad7192_ids[] = {
 	{ "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
 	{ "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
 	{ "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
+	{ "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
 	{ "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
 	{}
 };
@@ -1186,6 +1326,6 @@ static struct spi_driver ad7192_driver = {
 module_spi_driver(ad7192_driver);
 
 MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
+MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7194, AD7195 ADC");
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
-- 
2.34.1

Re: [PATCH v2 4/4] iio: adc: ad7192: Add AD7194 support

[Nuno Sá]

On Tue, 2023-11-14 at 22:05 +0200, Alisa-Dariana Roman wrote:
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The default configuration for these channels can be changed
> from the devicetree.
> 
> The default configuration is hardcoded in order to have a stable number
> of channels.
> 
> Also modify config AD7192 description for better scaling.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---

Well, just some minor stuff from my side. Even if you don't address the, feel
still free to:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/Kconfig  |  11 ++-
>  drivers/iio/adc/ad7192.c | 144 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 150 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 1e2b7a2c67c6..05344054b88e 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -55,12 +55,17 @@ config AD7124
>           called ad7124.
>  
>  config AD7192
> -       tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
> +       tristate "Analog Devices AD7192 and similar ADC driver"
>         depends on SPI
>         select AD_SIGMA_DELTA
>         help
> -         Say yes here to build support for Analog Devices AD7190,
> -         AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
> +         Say yes here to build support for Analog Devices SPI analog to digital
> +         converters (ADC):
> +         - AD7190
> +         - AD7192
> +         - AD7193
> +         - AD7194
> +         - AD7195
>           If unsure, say N (but it's safe to say "Y").
>  
>           To compile this driver as a module, choose M here: the
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 48e0357564af..0532678ad665 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
> + * AD7190 AD7192 AD7193 AD7194 AD7195 SPI ADC driver

Maybe this should also be "Analog Devices AD7192 and similar ADC driver"

>   *
>   * Copyright 2011-2015 Analog Devices Inc.
>   */
> @@ -125,10 +125,39 @@
>  #define AD7193_CH_AIN8         0x480 /* AIN7 - AINCOM */
>  #define AD7193_CH_AINCOM       0x600 /* AINCOM - AINCOM */
>  
> +#define AD7194_CH_TEMP         0x100 /* Temp sensor */
> +#define AD7194_CH_AIN1         0x400 /* AIN1 - AINCOM */
> +#define AD7194_CH_AIN2         0x410 /* AIN2 - AINCOM */
> +#define AD7194_CH_AIN3         0x420 /* AIN3 - AINCOM */
> +#define AD7194_CH_AIN4         0x430 /* AIN4 - AINCOM */
> +#define AD7194_CH_AIN5         0x440 /* AIN5 - AINCOM */
> +#define AD7194_CH_AIN6         0x450 /* AIN6 - AINCOM */
> +#define AD7194_CH_AIN7         0x460 /* AIN7 - AINCOM */
> +#define AD7194_CH_AIN8         0x470 /* AIN8 - AINCOM */
> +#define AD7194_CH_AIN9         0x480 /* AIN9 - AINCOM */
> +#define AD7194_CH_AIN10                0x490 /* AIN10 - AINCOM */
> +#define AD7194_CH_AIN11                0x4A0 /* AIN11 - AINCOM */
> +#define AD7194_CH_AIN12                0x4B0 /* AIN12 - AINCOM */
> +#define AD7194_CH_AIN13                0x4C0 /* AIN13 - AINCOM */
> +#define AD7194_CH_AIN14                0x4D0 /* AIN14 - AINCOM */
> +#define AD7194_CH_AIN15                0x4E0 /* AIN15 - AINCOM */
> +#define AD7194_CH_AIN16                0x4F0 /* AIN16 - AINCOM */
> +#define AD7194_CH_POS_MASK     GENMASK(7, 4)
> +#define AD7194_CH_POS(x)       FIELD_PREP(AD7194_CH_POS_MASK, (x))
> +#define AD7194_CH_NEG_MASK     GENMASK(3, 0)
> +#define AD7194_CH_NEG(x)       FIELD_PREP(AD7194_CH_NEG_MASK, (x))
> +#define AD7194_CH_DIFF(pos, neg) \
> +               (AD7194_CH_POS(pos) | AD7194_CH_NEG(neg))
> +#define AD7194_CH_DIFF_START   0
> +#define AD7194_CH_DIFF_NR      8
> +#define AD7194_CH_AIN_START    1
> +#define AD7194_CH_AIN_NR       16
> +
>  /* ID Register Bit Designations (AD7192_REG_ID) */
>  #define CHIPID_AD7190          0x4
>  #define CHIPID_AD7192          0x0
>  #define CHIPID_AD7193          0x2
> +#define CHIPID_AD7194          0x3
>  #define CHIPID_AD7195          0x6
>  #define AD7192_ID_MASK         GENMASK(3, 0)
>  
> @@ -166,6 +195,7 @@ enum {
>         ID_AD7190,
>         ID_AD7192,
>         ID_AD7193,
> +       ID_AD7194,
>         ID_AD7195,
>  };
>  
> @@ -644,6 +674,15 @@ static const struct attribute_group ad7192_attribute_group = {
>         .attrs = ad7192_attributes,
>  };
>  
> +static struct attribute *ad7194_attributes[] = {
> +       &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
> +       NULL
> +};
> +
> +static const struct attribute_group ad7194_attribute_group = {
> +       .attrs = ad7194_attributes,
> +};
> +
>  static struct attribute *ad7195_attributes[] = {
>         &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
>         &iio_dev_attr_bridge_switch_en.dev_attr.attr,
> @@ -928,6 +967,16 @@ static const struct iio_info ad7192_info = {
>         .update_scan_mode = ad7192_update_scan_mode,
>  };
>  
> +static const struct iio_info ad7194_info = {
> +       .read_raw = ad7192_read_raw,
> +       .write_raw = ad7192_write_raw,
> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> +       .read_avail = ad7192_read_avail,
> +       .attrs = &ad7194_attribute_group,
> +       .validate_trigger = ad_sd_validate_trigger,
> +       .update_scan_mode = ad7192_update_scan_mode,
> +};
> +
>  static const struct iio_info ad7195_info = {
>         .read_raw = ad7192_read_raw,
>         .write_raw = ad7192_write_raw,
> @@ -1017,6 +1066,35 @@ static const struct iio_chan_spec ad7193_channels[] = {
>         IIO_CHAN_SOFT_TIMESTAMP(14),
>  };
>  
> +static struct iio_chan_spec ad7194_channels[] = {
> +       AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
> +       AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
> +       AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
> +       AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
> +       AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
> +       AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
> +       AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
> +       AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
> +       AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
> +       AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
> +       AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
> +       AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
> +       AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
> +       AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
> +       AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
> +       AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
> +       AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
> +       AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
> +       AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
> +       AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
> +       AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
> +       AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
> +       AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
> +       AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
> +       AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> +};
> +
>  static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
>         [ID_AD7190] = {
>                 .chip_id = CHIPID_AD7190,
> @@ -1039,6 +1117,13 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[]
> = {
>                 .num_channels = ARRAY_SIZE(ad7193_channels),
>                 .info = &ad7192_info,
>         },
> +       [ID_AD7194] = {
> +               .chip_id = CHIPID_AD7194,
> +               .name = "ad7194",
> +               .channels = ad7194_channels,
> +               .num_channels = ARRAY_SIZE(ad7194_channels),
> +               .info = &ad7194_info,
> +       },
>         [ID_AD7195] = {
>                 .chip_id = CHIPID_AD7195,
>                 .name = "ad7195",
> @@ -1053,6 +1138,53 @@ static void ad7192_reg_disable(void *reg)
>         regulator_disable(reg);
>  }
>  
> +static int ad7192_parse_channel(struct iio_dev *indio_dev,
> +                               struct fwnode_handle *child)
> +{
> +       u32 reg, ain[2];
> +       int ret;
> +
> +       ret = fwnode_property_read_u32(child, "reg", &reg);
> +       if (ret)
> +               return ret;
> +
> +       if (!in_range(reg, AD7194_CH_DIFF_START, AD7194_CH_DIFF_NR))
> +               return -EINVAL;
> +
> +       ret = fwnode_property_read_u32_array(child, "diff-channels", ain,
> +                                            ARRAY_SIZE(ain));
> +       if (ret)
> +               return ret;
> +
> +       if (!in_range(ain[0], AD7194_CH_AIN_START, AD7194_CH_AIN_NR) ||
> +           !in_range(ain[1], AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> +               return -EINVAL;
> +
> +       ad7194_channels[reg].channel = ain[0];
> +       ad7194_channels[reg].channel2 = ain[1];
> +       ad7194_channels[reg].address = AD7194_CH_DIFF(ain[0], ain[1]);
> +
> +       return 0;
> +}
> +
> +static int ad7192_parse_channels(struct iio_dev *indio_dev)
> +{
> +       struct ad7192_state *st = iio_priv(indio_dev);

Don't really like this much... We should just pass the 'ad7192_state' but I see
you're being consistent with the rest of the probe(). If you need to re-spin the
series, I would like to see a precursor patch changing this and the other relevant
places.

> +       struct device *dev = &st->sd.spi->dev;
> +       struct fwnode_handle *child;
> +       int ret;
> +
> +       device_for_each_child_node(dev, child) {
> +               ret = ad7192_parse_channel(indio_dev, child);
> +               if (ret) {
> +                       fwnode_handle_put(child);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int ad7192_probe(struct spi_device *spi)
>  {
>         struct ad7192_state *st;
> @@ -1150,6 +1282,12 @@ static int ad7192_probe(struct spi_device *spi)
>                 }
>         }
>  
> +       if (st->chip_info->chip_id == CHIPID_AD7194) {
> +               ret = ad7192_parse_channels(indio_dev);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         ret = ad7192_setup(indio_dev);
>         if (ret)
>                 return ret;
> @@ -1161,6 +1299,7 @@ static const struct of_device_id ad7192_of_match[] = {
>         { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
>         { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
>         { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> +       { .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
>         { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
>         {}
>  };
> @@ -1170,6 +1309,7 @@ static const struct spi_device_id ad7192_ids[] = {
>         { "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
>         { "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
>         { "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
> +       { "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
>         { "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
>         {}
>  };
> @@ -1186,6 +1326,6 @@ static struct spi_driver ad7192_driver = {
>  module_spi_driver(ad7192_driver);
>  
>  MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> -MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
> +MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7194, AD7195 ADC");

Ditto...

- Nuno Sá

Re: [PATCH v2 4/4] iio: adc: ad7192: Add AD7194 support

[Jonathan Cameron]

On Tue, 14 Nov 2023 22:05:33 +0200
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The default configuration for these channels can be changed
> from the devicetree.
> 
> The default configuration is hardcoded in order to have a stable number
> of channels.
> 
> Also modify config AD7192 description for better scaling.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>

Not directly related to this patch (which looks fine to me)
but any idea why 3db_frequency_available is not using read_avail?

Seems sensible to convert it over given all the other cases are using that
and it will allow dropping at least some of the attributes infrastructure
for some devices.

Random aside on the fact that we should be able to do cleanup.h magic
to deal with the fwnode_handle_put() in error paths. +CC Peter Z, Andy S and Rafael,
mostly so they can shout if someone has already done it. That would avoid one
of our more common bugs with property handling and drop a bunch of lines in
every driver looping over fwnodes.
(my lore search foo isn't finding anything).

I have a bunch of other cleanup.h stuff to send out, but if no one points
out a nasty flaw, I'll circle back to this one in a week or two.

Jonathan

> ---
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 48e0357564af..0532678ad665 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
...

> +static int ad7192_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->sd.spi->dev;
> +	struct fwnode_handle *child;

Not specific to this driver, but someone (maybe me if I ever get
time) should look at a cleanup.h class for fwnode_handle.  Should be easy to do
and would get rid of all the manual fwnode_handle_put calls once and for all!

Would look something like (completely untested)
	DEFINE_FREE(fwnode_handle_put, struct fwnode_handle, if (_T) fwnode_handle_put(_T));

	struct fwnode_handle __free(fwnode_handle_put) *child = NULL;

minor benefit in this driver, but much larger in others that are parsing lots
of attributes under the fwnode.

> +	int ret;
> +
> +	device_for_each_child_node(dev, child) {
> +		ret = ad7192_parse_channel(indio_dev, child);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int ad7192_probe(struct spi_device *spi)
>  {
>  	struct ad7192_state *st;
> @@ -1150,6 +1282,12 @@ static int ad7192_probe(struct spi_device *spi)
>  		}
>  	}
>  
> +	if (st->chip_info->chip_id == CHIPID_AD7194) {

You match on 7194, then call a function named ad7192_xxx
which with that name we'd expect to apply to either the
ad7192 or to all parts in this driver.
So that function is not well named- it's find to rename it
to ad7194_parse_channels().  However...

> +		ret = ad7192_parse_channels(indio_dev);
I'd prefer to see this done via a callback in chip_info

	if (st->chip_info->channel_parse) {
		ret = st->chip_info->channel_parse(indio_dev);
		...

> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = ad7192_setup(indio_dev);
>  	if (ret)
>  		return ret;
> @@ -1161,6 +1299,7 @@ static const struct of_device_id ad7192_of_match[] = {
>  	{ .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
>  	{ .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
>  	{ .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> +	{ .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
>  	{ .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
>  	{}
>  };
> @@ -1170,6 +1309,7 @@ static const struct spi_device_id ad7192_ids[] = {
>  	{ "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
>  	{ "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
>  	{ "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
> +	{ "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
>  	{ "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
>  	{}
>  };
> @@ -1186,6 +1326,6 @@ static struct spi_driver ad7192_driver = {
>  module_spi_driver(ad7192_driver);
>  
>  MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> -MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
> +MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7194, AD7195 ADC");

Maybe time to switch to 'and similar' here.

Thanks,

Jonathan

>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);

Re: [PATCH v2 4/4] iio: adc: ad7192: Add AD7194 support

[Alisa-Dariana Roman]

On 26.11.2023 18:34, Jonathan Cameron wrote:
> On Tue, 14 Nov 2023 22:05:33 +0200
> Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> 
>> Unlike the other AD719Xs, AD7194 has configurable differential
>> channels. The default configuration for these channels can be changed
>> from the devicetree.
>>
>> The default configuration is hardcoded in order to have a stable number
>> of channels.
>>
>> Also modify config AD7192 description for better scaling.
>>
>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> 
> Not directly related to this patch (which looks fine to me)
> but any idea why 3db_frequency_available is not using read_avail?
> 
> Seems sensible to convert it over given all the other cases are using that
> and it will allow dropping at least some of the attributes infrastructure
> for some devices.

Thank you very much for the feedback!

I actually tried then to use read_avail for the 3db frequencies, but it 
required a greater rework. If I remember correctly, the four possible 
frequency choices need to be stored in the ad7192_state for it to work. 
Should I add a patch with these changes?

Kind regards,
Alisa-Dariana Roman

Re: [PATCH v2 4/4] iio: adc: ad7192: Add AD7194 support

[Jonathan Cameron]

On Fri, 2 Feb 2024 16:32:14 +0200
Alisa-Dariana Roman <alisadariana@gmail.com> wrote:

> On 26.11.2023 18:34, Jonathan Cameron wrote:
> > On Tue, 14 Nov 2023 22:05:33 +0200
> > Alisa-Dariana Roman <alisadariana@gmail.com> wrote:
> >   
> >> Unlike the other AD719Xs, AD7194 has configurable differential
> >> channels. The default configuration for these channels can be changed
> >> from the devicetree.
> >>
> >> The default configuration is hardcoded in order to have a stable number
> >> of channels.
> >>
> >> Also modify config AD7192 description for better scaling.
> >>
> >> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>  
> > 
> > Not directly related to this patch (which looks fine to me)
> > but any idea why 3db_frequency_available is not using read_avail?
> > 
> > Seems sensible to convert it over given all the other cases are using that
> > and it will allow dropping at least some of the attributes infrastructure
> > for some devices.  
> 
> Thank you very much for the feedback!
> 
> I actually tried then to use read_avail for the 3db frequencies, but it 
> required a greater rework. If I remember correctly, the four possible 
> frequency choices need to be stored in the ad7192_state for it to work. 
> Should I add a patch with these changes?
> 
If will end up a tiny bit more complex than currently because of the need
to stash them in an array rather than simply print the strings directly.
But not a lot and should allow you to get rid of a small amount of other code.

Not urgent, but I think this would be a nice change in the longer term

Thanks,

Jonathan