[PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants

[PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add support for ad3541r and ad3551r single output variants.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 .../devicetree/bindings/iio/dac/adi,ad3552r.yaml       | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
index 8265d709094d..17442cdfbe27 100644
--- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
@@ -19,7 +19,9 @@ description: |
 properties:
   compatible:
     enum:
+      - adi,ad3541r
       - adi,ad3542r
+      - adi,ad3551r
       - adi,ad3552r
 
   reg:
@@ -128,7 +130,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: adi,ad3542r
+            enum:
+              - adi,ad3541r
+              - adi,ad3542r
     then:
       patternProperties:
         "^channel@([0-1])$":
@@ -158,7 +162,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: adi,ad3552r
+            enum:
+              - adi,ad3551r
+              - adi,ad3552r
     then:
       patternProperties:
         "^channel@([0-1])$":
-- 
2.45.0.rc1

[PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add support for single-output dac variants.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/ad3552r.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
index a492e8f2fc0f..0dd6f995c3e2 100644
--- a/drivers/iio/dac/ad3552r.c
+++ b/drivers/iio/dac/ad3552r.c
@@ -140,7 +140,9 @@ enum ad3552r_ch_vref_select {
 };
 
 enum ad3542r_id {
+	AD3541R_ID = 0x400b,
 	AD3542R_ID = 0x4009,
+	AD3551R_ID = 0x400a,
 	AD3552R_ID = 0x4008,
 };
 
@@ -745,7 +747,8 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch)
 	} else {
 		/* Normal range */
 		idx = dac->ch_data[ch].range;
-		if (dac->chip_id == AD3542R_ID) {
+		if (dac->chip_id == AD3541R_ID ||
+		    dac->chip_id == AD3542R_ID) {
 			v_min = ad3542r_ch_ranges[idx][0];
 			v_max = ad3542r_ch_ranges[idx][1];
 		} else {
@@ -780,7 +783,7 @@ static int ad3552r_find_range(u16 id, s32 *vals)
 	int i, len;
 	const s32 (*ranges)[2];
 
-	if (id == AD3542R_ID) {
+	if (id == AD3541R_ID || id == AD3542R_ID) {
 		len = ARRAY_SIZE(ad3542r_ch_ranges);
 		ranges = ad3542r_ch_ranges;
 	} else {
@@ -955,9 +958,10 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
 			dev_err(dev, "mandatory reg property missing\n");
 			goto put_child;
 		}
-		if (ch >= AD3552R_NUM_CH) {
-			dev_err(dev, "reg must be less than %d\n",
-				AD3552R_NUM_CH);
+		if (ch >= AD3552R_NUM_CH ||
+			(dac->chip_id == AD3541R_ID && ch) ||
+			(dac->chip_id == AD3551R_ID && ch)) {
+			dev_err(dev, "channel %d is not supported\n", ch);
 			err = -EINVAL;
 			goto put_child;
 		}
@@ -987,9 +991,10 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
 				goto put_child;
 
 			dac->ch_data[ch].range = val;
-		} else if (dac->chip_id == AD3542R_ID) {
+		} else if (dac->chip_id == AD3541R_ID ||
+			   dac->chip_id == AD3542R_ID) {
 			dev_err(dev,
-				"adi,output-range-microvolt is required for ad3542r\n");
+				"adi,output-range-microvolt is required for ad354xr\n");
 			err = -EINVAL;
 			goto put_child;
 		} else {
@@ -1088,10 +1093,20 @@ static int ad3552r_probe(struct spi_device *spi)
 		return err;
 
 	/* Config triggered buffer device */
-	if (dac->chip_id == AD3552R_ID)
-		indio_dev->name = "ad3552r";
-	else
+	switch (dac->chip_id) {
+	case AD3541R_ID:
+		indio_dev->name = "ad3541r";
+		break;
+	case AD3542R_ID:
 		indio_dev->name = "ad3542r";
+		break;
+	case AD3551R_ID:
+		indio_dev->name = "ad3551r";
+		break;
+	case AD3552R_ID:
+		indio_dev->name = "ad3552r";
+		break;
+	}
 	indio_dev->dev.parent = &spi->dev;
 	indio_dev->info = &ad3552r_iio_info;
 	indio_dev->num_channels = dac->num_ch;
@@ -1110,14 +1125,18 @@ static int ad3552r_probe(struct spi_device *spi)
 }
 
 static const struct spi_device_id ad3552r_id[] = {
+	{ "ad3541r", AD3541R_ID },
 	{ "ad3542r", AD3542R_ID },
+	{ "ad3551r", AD3551R_ID },
 	{ "ad3552r", AD3552R_ID },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ad3552r_id);
 
 static const struct of_device_id ad3552r_of_match[] = {
+	{ .compatible = "adi,ad3541r"},
 	{ .compatible = "adi,ad3542r"},
+	{ .compatible = "adi,ad3551r"},
 	{ .compatible = "adi,ad3552r"},
 	{ }
 };
-- 
2.45.0.rc1

[PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

The adi,gain-scaling-p/n values are an inverted log2,
so initial naiming was set correct, but the driver uses just
adi,gain-scaling-p/n, so uniforming documentation, that seems
a less-risk fix for future rebases, and still conformant to datasheet.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
index 17442cdfbe27..9e3dbf890bfa 100644
--- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
@@ -94,13 +94,13 @@ patternProperties:
             maximum: 511
             minimum: -511
 
-          adi,gain-scaling-p-inv-log2:
-            description: GainP = 1 / ( 2 ^ adi,gain-scaling-p-inv-log2)
+          adi,gain-scaling-p:
+            description: GainP = 1 / ( 2 ^ adi,gain-scaling-p)
             $ref: /schemas/types.yaml#/definitions/uint32
             enum: [0, 1, 2, 3]
 
-          adi,gain-scaling-n-inv-log2:
-            description: GainN = 1 / ( 2 ^ adi,gain-scaling-n-inv-log2)
+          adi,gain-scaling-n:
+            description: GainN = 1 / ( 2 ^ adi,gain-scaling-n)
             $ref: /schemas/types.yaml#/definitions/uint32
             enum: [0, 1, 2, 3]
 
@@ -109,8 +109,8 @@ patternProperties:
 
         required:
           - adi,gain-offset
-          - adi,gain-scaling-p-inv-log2
-          - adi,gain-scaling-n-inv-log2
+          - adi,gain-scaling-p
+          - adi,gain-scaling-n
           - adi,rfb-ohms
 
     required:
@@ -214,8 +214,8 @@ examples:
                 reg = <1>;
                 custom-output-range-config {
                     adi,gain-offset = <5>;
-                    adi,gain-scaling-p-inv-log2 = <1>;
-                    adi,gain-scaling-n-inv-log2 = <2>;
+                    adi,gain-scaling-p = <1>;
+                    adi,gain-scaling-n = <2>;
                     adi,rfb-ohms = <1>;
                 };
             };
-- 
2.45.0.rc1

Re: [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants

[David Lechner]

On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
<adureghello@baylibre.com> wrote:
>
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> Add support for ad3541r and ad3551r single output variants.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml       | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> index 8265d709094d..17442cdfbe27 100644
> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml

It would be nice to also add the datasheet links in the description.

> @@ -19,7 +19,9 @@ description: |
>  properties:
>    compatible:
>      enum:
> +      - adi,ad3541r
>        - adi,ad3542r
> +      - adi,ad3551r
>        - adi,ad3552r
>
>    reg:
> @@ -128,7 +130,9 @@ allOf:
>        properties:
>          compatible:
>            contains:
> -            const: adi,ad3542r
> +            enum:
> +              - adi,ad3541r
> +              - adi,ad3542r
>      then:
>        patternProperties:
>          "^channel@([0-1])$":
> @@ -158,7 +162,9 @@ allOf:
>        properties:
>          compatible:
>            contains:
> -            const: adi,ad3552r
> +            enum:
> +              - adi,ad3551r
> +              - adi,ad3552r
>      then:
>        patternProperties:
>          "^channel@([0-1])$":
> --
> 2.45.0.rc1
>
>

Since these are single channel, it would not hurt to restrict the
`reg` property of of the `channel@` nodes to 1.

Re: [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r

[David Lechner]

On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
<adureghello@baylibre.com> wrote:
>
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> Add support for single-output dac variants.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/dac/ad3552r.c | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
> index a492e8f2fc0f..0dd6f995c3e2 100644
> --- a/drivers/iio/dac/ad3552r.c
> +++ b/drivers/iio/dac/ad3552r.c
> @@ -140,7 +140,9 @@ enum ad3552r_ch_vref_select {
>  };
>
>  enum ad3542r_id {
> +       AD3541R_ID = 0x400b,
>         AD3542R_ID = 0x4009,
> +       AD3551R_ID = 0x400a,
>         AD3552R_ID = 0x4008,
>  };
>
> @@ -745,7 +747,8 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch)
>         } else {
>                 /* Normal range */
>                 idx = dac->ch_data[ch].range;
> -               if (dac->chip_id == AD3542R_ID) {
> +               if (dac->chip_id == AD3541R_ID ||
> +                   dac->chip_id == AD3542R_ID) {
>                         v_min = ad3542r_ch_ranges[idx][0];
>                         v_max = ad3542r_ch_ranges[idx][1];
>                 } else {
> @@ -780,7 +783,7 @@ static int ad3552r_find_range(u16 id, s32 *vals)
>         int i, len;
>         const s32 (*ranges)[2];
>
> -       if (id == AD3542R_ID) {
> +       if (id == AD3541R_ID || id == AD3542R_ID) {
>                 len = ARRAY_SIZE(ad3542r_ch_ranges);
>                 ranges = ad3542r_ch_ranges;
>         } else {
> @@ -955,9 +958,10 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
>                         dev_err(dev, "mandatory reg property missing\n");
>                         goto put_child;
>                 }
> -               if (ch >= AD3552R_NUM_CH) {
> -                       dev_err(dev, "reg must be less than %d\n",
> -                               AD3552R_NUM_CH);
> +               if (ch >= AD3552R_NUM_CH ||
> +                       (dac->chip_id == AD3541R_ID && ch) ||
> +                       (dac->chip_id == AD3551R_ID && ch)) {
> +                       dev_err(dev, "channel %d is not supported\n", ch);
>                         err = -EINVAL;
>                         goto put_child;
>                 }
> @@ -987,9 +991,10 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
>                                 goto put_child;
>
>                         dac->ch_data[ch].range = val;
> -               } else if (dac->chip_id == AD3542R_ID) {
> +               } else if (dac->chip_id == AD3541R_ID ||
> +                          dac->chip_id == AD3542R_ID) {
>                         dev_err(dev,
> -                               "adi,output-range-microvolt is required for ad3542r\n");
> +                               "adi,output-range-microvolt is required for ad354xr\n");
>                         err = -EINVAL;
>                         goto put_child;
>                 } else {
> @@ -1088,10 +1093,20 @@ static int ad3552r_probe(struct spi_device *spi)
>                 return err;
>
>         /* Config triggered buffer device */
> -       if (dac->chip_id == AD3552R_ID)
> -               indio_dev->name = "ad3552r";
> -       else
> +       switch (dac->chip_id) {
> +       case AD3541R_ID:
> +               indio_dev->name = "ad3541r";
> +               break;
> +       case AD3542R_ID:
>                 indio_dev->name = "ad3542r";
> +               break;
> +       case AD3551R_ID:
> +               indio_dev->name = "ad3551r";
> +               break;
> +       case AD3552R_ID:
> +               indio_dev->name = "ad3552r";
> +               break;
> +       }
>         indio_dev->dev.parent = &spi->dev;
>         indio_dev->info = &ad3552r_iio_info;
>         indio_dev->num_channels = dac->num_ch;
> @@ -1110,14 +1125,18 @@ static int ad3552r_probe(struct spi_device *spi)
>  }
>
>  static const struct spi_device_id ad3552r_id[] = {
> +       { "ad3541r", AD3541R_ID },
>         { "ad3542r", AD3542R_ID },
> +       { "ad3551r", AD3551R_ID },
>         { "ad3552r", AD3552R_ID },
>         { }
>  };
>  MODULE_DEVICE_TABLE(spi, ad3552r_id);
>
>  static const struct of_device_id ad3552r_of_match[] = {
> +       { .compatible = "adi,ad3541r"},
>         { .compatible = "adi,ad3542r"},
> +       { .compatible = "adi,ad3551r"},
>         { .compatible = "adi,ad3552r"},
>         { }
>  };
> --
> 2.45.0.rc1
>
>

It looks like it is time for a chip_info struct here instead of the if
and switch statements to get chip-specific data. Most other IIO
drivers have this already and it is the preferred way to look up this
kind of information in the IIO subsystem. I prefer the drivers that
don't put all of the info structs in an array (that way the code is
less verbose). So I would suggest looking at e.g. adc/aspeed_adc,
starting with aspeed_adc_matches, to see what I mean and how to
implement it. (So one patch to add the info structs and a second patch
to add the single channel chips)

Re: [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names

[David Lechner]

On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
<adureghello@baylibre.com> wrote:
>
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> The adi,gain-scaling-p/n values are an inverted log2,
> so initial naiming was set correct, but the driver uses just
> adi,gain-scaling-p/n, so uniforming documentation, that seems
> a less-risk fix for future rebases, and still conformant to datasheet.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> index 17442cdfbe27..9e3dbf890bfa 100644
> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> @@ -94,13 +94,13 @@ patternProperties:
>              maximum: 511
>              minimum: -511
>
> -          adi,gain-scaling-p-inv-log2:
> -            description: GainP = 1 / ( 2 ^ adi,gain-scaling-p-inv-log2)
> +          adi,gain-scaling-p:
> +            description: GainP = 1 / ( 2 ^ adi,gain-scaling-p)
>              $ref: /schemas/types.yaml#/definitions/uint32
>              enum: [0, 1, 2, 3]
>
> -          adi,gain-scaling-n-inv-log2:
> -            description: GainN = 1 / ( 2 ^ adi,gain-scaling-n-inv-log2)
> +          adi,gain-scaling-n:
> +            description: GainN = 1 / ( 2 ^ adi,gain-scaling-n)
>              $ref: /schemas/types.yaml#/definitions/uint32
>              enum: [0, 1, 2, 3]
>
> @@ -109,8 +109,8 @@ patternProperties:
>
>          required:
>            - adi,gain-offset
> -          - adi,gain-scaling-p-inv-log2
> -          - adi,gain-scaling-n-inv-log2
> +          - adi,gain-scaling-p
> +          - adi,gain-scaling-n
>            - adi,rfb-ohms
>
>      required:
> @@ -214,8 +214,8 @@ examples:
>                  reg = <1>;
>                  custom-output-range-config {
>                      adi,gain-offset = <5>;
> -                    adi,gain-scaling-p-inv-log2 = <1>;
> -                    adi,gain-scaling-n-inv-log2 = <2>;
> +                    adi,gain-scaling-p = <1>;
> +                    adi,gain-scaling-n = <2>;
>                      adi,rfb-ohms = <1>;
>                  };
>              };
> --
> 2.45.0.rc1
>
>

The DT bindings are generally considered immutable. So unless we can
prove that no one has ever put adi,gain-scaling-n-inv-log2 in a .dtb
file, we probably need to fix this in the driver rather than in the
bindings. (The driver can still handle adi,gain-scaling-p in the
driver for backwards compatibility but the official binding should be
what was already accepted in the .yaml file)

Re: [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants

[Jonathan Cameron]

On Fri, 10 May 2024 16:18:34 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Add support for ad3541r and ad3551r single output variants.

I'd expect to see constraints on reg for the channel nodes.
Am I missing some reason those don't make sense?

> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml       | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> index 8265d709094d..17442cdfbe27 100644
> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> @@ -19,7 +19,9 @@ description: |
>  properties:
>    compatible:
>      enum:
> +      - adi,ad3541r
>        - adi,ad3542r
> +      - adi,ad3551r
>        - adi,ad3552r
>  
>    reg:
> @@ -128,7 +130,9 @@ allOf:
>        properties:
>          compatible:
>            contains:
> -            const: adi,ad3542r
> +            enum:
> +              - adi,ad3541r
> +              - adi,ad3542r
>      then:
>        patternProperties:
>          "^channel@([0-1])$":
> @@ -158,7 +162,9 @@ allOf:
>        properties:
>          compatible:
>            contains:
> -            const: adi,ad3552r
> +            enum:
> +              - adi,ad3551r
> +              - adi,ad3552r
>      then:
>        patternProperties:
>          "^channel@([0-1])$":

Re: [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants

[Jonathan Cameron]

On Fri, 10 May 2024 10:39:31 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
> <adureghello@baylibre.com> wrote:
> >
> > From: Angelo Dureghello <adureghello@baylibre.com>
> >
> > Add support for ad3541r and ad3551r single output variants.
> >
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> >  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml       | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > index 8265d709094d..17442cdfbe27 100644
> > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml  
> 
> It would be nice to also add the datasheet links in the description.
> 
> > @@ -19,7 +19,9 @@ description: |
> >  properties:
> >    compatible:
> >      enum:
> > +      - adi,ad3541r
> >        - adi,ad3542r
> > +      - adi,ad3551r
> >        - adi,ad3552r
> >
> >    reg:
> > @@ -128,7 +130,9 @@ allOf:
> >        properties:
> >          compatible:
> >            contains:
> > -            const: adi,ad3542r
> > +            enum:
> > +              - adi,ad3541r
> > +              - adi,ad3542r
> >      then:
> >        patternProperties:
> >          "^channel@([0-1])$":
> > @@ -158,7 +162,9 @@ allOf:
> >        properties:
> >          compatible:
> >            contains:
> > -            const: adi,ad3552r
> > +            enum:
> > +              - adi,ad3551r
> > +              - adi,ad3552r
> >      then:
> >        patternProperties:
> >          "^channel@([0-1])$":
> > --
> > 2.45.0.rc1
> >
> >  
> 
> Since these are single channel, it would not hurt to restrict the
> `reg` property of of the `channel@` nodes to 1.
Ah. I missed David's email because threading goes weird without a cover letter
and hence duplicated his comment.
Please add a cover letter with a brief description of the series to
your v2.  Means we get a nice title in patchwork as well!

Thanks,

Jonathan
 

Re: [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names

[Rob Herring]

On Fri, May 10, 2024 at 10:43:18AM -0500, David Lechner wrote:
> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
> <adureghello@baylibre.com> wrote:
> >
> > From: Angelo Dureghello <adureghello@baylibre.com>
> >
> > The adi,gain-scaling-p/n values are an inverted log2,
> > so initial naiming was set correct, but the driver uses just
> > adi,gain-scaling-p/n, so uniforming documentation, that seems
> > a less-risk fix for future rebases, and still conformant to datasheet.
> >
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> >  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > index 17442cdfbe27..9e3dbf890bfa 100644
> > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > @@ -94,13 +94,13 @@ patternProperties:
> >              maximum: 511
> >              minimum: -511
> >
> > -          adi,gain-scaling-p-inv-log2:
> > -            description: GainP = 1 / ( 2 ^ adi,gain-scaling-p-inv-log2)
> > +          adi,gain-scaling-p:
> > +            description: GainP = 1 / ( 2 ^ adi,gain-scaling-p)
> >              $ref: /schemas/types.yaml#/definitions/uint32
> >              enum: [0, 1, 2, 3]
> >
> > -          adi,gain-scaling-n-inv-log2:
> > -            description: GainN = 1 / ( 2 ^ adi,gain-scaling-n-inv-log2)
> > +          adi,gain-scaling-n:
> > +            description: GainN = 1 / ( 2 ^ adi,gain-scaling-n)
> >              $ref: /schemas/types.yaml#/definitions/uint32
> >              enum: [0, 1, 2, 3]
> >
> > @@ -109,8 +109,8 @@ patternProperties:
> >
> >          required:
> >            - adi,gain-offset
> > -          - adi,gain-scaling-p-inv-log2
> > -          - adi,gain-scaling-n-inv-log2
> > +          - adi,gain-scaling-p
> > +          - adi,gain-scaling-n
> >            - adi,rfb-ohms
> >
> >      required:
> > @@ -214,8 +214,8 @@ examples:
> >                  reg = <1>;
> >                  custom-output-range-config {
> >                      adi,gain-offset = <5>;
> > -                    adi,gain-scaling-p-inv-log2 = <1>;
> > -                    adi,gain-scaling-n-inv-log2 = <2>;
> > +                    adi,gain-scaling-p = <1>;
> > +                    adi,gain-scaling-n = <2>;
> >                      adi,rfb-ohms = <1>;
> >                  };
> >              };
> > --
> > 2.45.0.rc1
> >
> >
> 
> The DT bindings are generally considered immutable. So unless we can
> prove that no one has ever put adi,gain-scaling-n-inv-log2 in a .dtb
> file, 

You can't ever prove that. 

> we probably need to fix this in the driver rather than in the
> bindings. (The driver can still handle adi,gain-scaling-p in the
> driver for backwards compatibility but the official binding should be
> what was already accepted in the .yaml file)

If we can reasonable assume that the Linux driver is the only consumer, 
there are no upstream dts users (in kernel or other opensource 
projects), and/or the property is somewhat recent, then that's good 
enough IMO.

Rob

Re: [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names

[David Lechner]

On Mon, May 13, 2024 at 1:52 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, May 10, 2024 at 10:43:18AM -0500, David Lechner wrote:
> > On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
> > <adureghello@baylibre.com> wrote:
> > >
> > > From: Angelo Dureghello <adureghello@baylibre.com>
> > >
> > > The adi,gain-scaling-p/n values are an inverted log2,
> > > so initial naiming was set correct, but the driver uses just
> > > adi,gain-scaling-p/n, so uniforming documentation, that seems
> > > a less-risk fix for future rebases, and still conformant to datasheet.
> > >
> > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > > ---
> > >  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > index 17442cdfbe27..9e3dbf890bfa 100644
> > > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > @@ -94,13 +94,13 @@ patternProperties:
> > >              maximum: 511
> > >              minimum: -511
> > >
> > > -          adi,gain-scaling-p-inv-log2:
> > > -            description: GainP = 1 / ( 2 ^ adi,gain-scaling-p-inv-log2)
> > > +          adi,gain-scaling-p:
> > > +            description: GainP = 1 / ( 2 ^ adi,gain-scaling-p)
> > >              $ref: /schemas/types.yaml#/definitions/uint32
> > >              enum: [0, 1, 2, 3]
> > >
> > > -          adi,gain-scaling-n-inv-log2:
> > > -            description: GainN = 1 / ( 2 ^ adi,gain-scaling-n-inv-log2)
> > > +          adi,gain-scaling-n:
> > > +            description: GainN = 1 / ( 2 ^ adi,gain-scaling-n)
> > >              $ref: /schemas/types.yaml#/definitions/uint32
> > >              enum: [0, 1, 2, 3]
> > >
> > > @@ -109,8 +109,8 @@ patternProperties:
> > >
> > >          required:
> > >            - adi,gain-offset
> > > -          - adi,gain-scaling-p-inv-log2
> > > -          - adi,gain-scaling-n-inv-log2
> > > +          - adi,gain-scaling-p
> > > +          - adi,gain-scaling-n
> > >            - adi,rfb-ohms
> > >
> > >      required:
> > > @@ -214,8 +214,8 @@ examples:
> > >                  reg = <1>;
> > >                  custom-output-range-config {
> > >                      adi,gain-offset = <5>;
> > > -                    adi,gain-scaling-p-inv-log2 = <1>;
> > > -                    adi,gain-scaling-n-inv-log2 = <2>;
> > > +                    adi,gain-scaling-p = <1>;
> > > +                    adi,gain-scaling-n = <2>;
> > >                      adi,rfb-ohms = <1>;
> > >                  };
> > >              };
> > > --
> > > 2.45.0.rc1
> > >
> > >
> >
> > The DT bindings are generally considered immutable. So unless we can
> > prove that no one has ever put adi,gain-scaling-n-inv-log2 in a .dtb
> > file,
>
> You can't ever prove that.
>
> > we probably need to fix this in the driver rather than in the
> > bindings. (The driver can still handle adi,gain-scaling-p in the
> > driver for backwards compatibility but the official binding should be
> > what was already accepted in the .yaml file)
>
> If we can reasonable assume that the Linux driver is the only consumer,
> there are no upstream dts users (in kernel or other opensource
> projects), and/or the property is somewhat recent, then that's good
> enough IMO.
>
> Rob

Ack. I stand corrected then.

Re: [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r

[Angelo Dureghello]

Hi David,

On 10/05/24 5:42 PM, David Lechner wrote:
> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
> <adureghello@baylibre.com> wrote:
>> From: Angelo Dureghello <adureghello@baylibre.com>
>>
>> Add support for single-output dac variants.
>>
>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
>> ---
>>   drivers/iio/dac/ad3552r.c | 39 +++++++++++++++++++++++++++++----------
>>   1 file changed, 29 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
>> index a492e8f2fc0f..0dd6f995c3e2 100644
>> --- a/drivers/iio/dac/ad3552r.c
>> +++ b/drivers/iio/dac/ad3552r.c
>> @@ -140,7 +140,9 @@ enum ad3552r_ch_vref_select {
>>   };
>>
>>   enum ad3542r_id {
>> +       AD3541R_ID = 0x400b,
>>          AD3542R_ID = 0x4009,
>> +       AD3551R_ID = 0x400a,
>>          AD3552R_ID = 0x4008,
>>   };
>>
>> @@ -745,7 +747,8 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch)
>>          } else {
>>                  /* Normal range */
>>                  idx = dac->ch_data[ch].range;
>> -               if (dac->chip_id == AD3542R_ID) {
>> +               if (dac->chip_id == AD3541R_ID ||
>> +                   dac->chip_id == AD3542R_ID) {
>>                          v_min = ad3542r_ch_ranges[idx][0];
>>                          v_max = ad3542r_ch_ranges[idx][1];
>>                  } else {
>> @@ -780,7 +783,7 @@ static int ad3552r_find_range(u16 id, s32 *vals)
>>          int i, len;
>>          const s32 (*ranges)[2];
>>
>> -       if (id == AD3542R_ID) {
>> +       if (id == AD3541R_ID || id == AD3542R_ID) {
>>                  len = ARRAY_SIZE(ad3542r_ch_ranges);
>>                  ranges = ad3542r_ch_ranges;
>>          } else {
>> @@ -955,9 +958,10 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
>>                          dev_err(dev, "mandatory reg property missing\n");
>>                          goto put_child;
>>                  }
>> -               if (ch >= AD3552R_NUM_CH) {
>> -                       dev_err(dev, "reg must be less than %d\n",
>> -                               AD3552R_NUM_CH);
>> +               if (ch >= AD3552R_NUM_CH ||
>> +                       (dac->chip_id == AD3541R_ID && ch) ||
>> +                       (dac->chip_id == AD3551R_ID && ch)) {
>> +                       dev_err(dev, "channel %d is not supported\n", ch);
>>                          err = -EINVAL;
>>                          goto put_child;
>>                  }
>> @@ -987,9 +991,10 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
>>                                  goto put_child;
>>
>>                          dac->ch_data[ch].range = val;
>> -               } else if (dac->chip_id == AD3542R_ID) {
>> +               } else if (dac->chip_id == AD3541R_ID ||
>> +                          dac->chip_id == AD3542R_ID) {
>>                          dev_err(dev,
>> -                               "adi,output-range-microvolt is required for ad3542r\n");
>> +                               "adi,output-range-microvolt is required for ad354xr\n");
>>                          err = -EINVAL;
>>                          goto put_child;
>>                  } else {
>> @@ -1088,10 +1093,20 @@ static int ad3552r_probe(struct spi_device *spi)
>>                  return err;
>>
>>          /* Config triggered buffer device */
>> -       if (dac->chip_id == AD3552R_ID)
>> -               indio_dev->name = "ad3552r";
>> -       else
>> +       switch (dac->chip_id) {
>> +       case AD3541R_ID:
>> +               indio_dev->name = "ad3541r";
>> +               break;
>> +       case AD3542R_ID:
>>                  indio_dev->name = "ad3542r";
>> +               break;
>> +       case AD3551R_ID:
>> +               indio_dev->name = "ad3551r";
>> +               break;
>> +       case AD3552R_ID:
>> +               indio_dev->name = "ad3552r";
>> +               break;
>> +       }
>>          indio_dev->dev.parent = &spi->dev;
>>          indio_dev->info = &ad3552r_iio_info;
>>          indio_dev->num_channels = dac->num_ch;
>> @@ -1110,14 +1125,18 @@ static int ad3552r_probe(struct spi_device *spi)
>>   }
>>
>>   static const struct spi_device_id ad3552r_id[] = {
>> +       { "ad3541r", AD3541R_ID },
>>          { "ad3542r", AD3542R_ID },
>> +       { "ad3551r", AD3551R_ID },
>>          { "ad3552r", AD3552R_ID },
>>          { }
>>   };
>>   MODULE_DEVICE_TABLE(spi, ad3552r_id);
>>
>>   static const struct of_device_id ad3552r_of_match[] = {
>> +       { .compatible = "adi,ad3541r"},
>>          { .compatible = "adi,ad3542r"},
>> +       { .compatible = "adi,ad3551r"},
>>          { .compatible = "adi,ad3552r"},
>>          { }
>>   };
>> --
>> 2.45.0.rc1
>>
>>
> It looks like it is time for a chip_info struct here instead of the if
> and switch statements to get chip-specific data. Most other IIO
> drivers have this already and it is the preferred way to look up this
> kind of information in the IIO subsystem. I prefer the drivers that
> don't put all of the info structs in an array (that way the code is
> less verbose). So I would suggest looking at e.g. adc/aspeed_adc,
> starting with aspeed_adc_matches, to see what I mean and how to
> implement it. (So one patch to add the info structs and a second patch
> to add the single channel chips)

Ack, will change in that way.


Regards,
angelo

Re: [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants

[Angelo Dureghello]

Hi,

On 11/05/24 6:05 PM, Jonathan Cameron wrote:
> On Fri, 10 May 2024 10:39:31 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
>> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
>> <adureghello@baylibre.com> wrote:
>>> From: Angelo Dureghello <adureghello@baylibre.com>
>>>
>>> Add support for ad3541r and ad3551r single output variants.
>>>
>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
>>> ---
>>>   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml       | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>> index 8265d709094d..17442cdfbe27 100644
>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>> It would be nice to also add the datasheet links in the description.

ack,


>>> @@ -19,7 +19,9 @@ description: |
>>>   properties:
>>>     compatible:
>>>       enum:
>>> +      - adi,ad3541r
>>>         - adi,ad3542r
>>> +      - adi,ad3551r
>>>         - adi,ad3552r
>>>
>>>     reg:
>>> @@ -128,7 +130,9 @@ allOf:
>>>         properties:
>>>           compatible:
>>>             contains:
>>> -            const: adi,ad3542r
>>> +            enum:
>>> +              - adi,ad3541r
>>> +              - adi,ad3542r
>>>       then:
>>>         patternProperties:
>>>           "^channel@([0-1])$":
>>> @@ -158,7 +162,9 @@ allOf:
>>>         properties:
>>>           compatible:
>>>             contains:
>>> -            const: adi,ad3552r
>>> +            enum:
>>> +              - adi,ad3551r
>>> +              - adi,ad3552r
>>>       then:
>>>         patternProperties:
>>>           "^channel@([0-1])$":
>>> --
>>> 2.45.0.rc1
>>>
>>>   
>> Since these are single channel, it would not hurt to restrict the
>> `reg` property of of the `channel@` nodes to 1.

ack,


> Ah. I missed David's email because threading goes weird without a cover letter
> and hence duplicated his comment.
> Please add a cover letter with a brief description of the series to
> your v2.  Means we get a nice title in patchwork as well!
>
> Thanks,
>
> Jonathan
>   
>
Thanks for the feedbacks, will fix all in v2.

Regards,
angelo