diff mbox series

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

Message ID 20240510141836.1624009-1-adureghello@baylibre.org
State Changes Requested
Headers show
Series [1/3] dt-bindings: iio: dac: add ad35xxr single output variants | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Angelo Dureghello May 10, 2024, 2:18 p.m. UTC 
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(-)

Comments

David Lechner May 10, 2024, 3:39 p.m. UTC | #1 
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.
David Lechner May 10, 2024, 3:42 p.m. UTC | #2 
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)
Jonathan Cameron May 11, 2024, 4:04 p.m. UTC | #3 
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])$":
Jonathan Cameron May 11, 2024, 4:05 p.m. UTC | #4 
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
Angelo Dureghello May 16, 2024, 1:17 p.m. UTC | #5 
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
Angelo Dureghello May 16, 2024, 1:20 p.m. UTC | #6 
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
diff mbox series

Patch

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])$":