From: Angelo Dureghello <[email protected]>
Add support for ad3541r and ad3551r single output variants.
Signed-off-by: Angelo Dureghello <[email protected]>
---
.../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
From: Angelo Dureghello <[email protected]>
Add support for single-output dac variants.
Signed-off-by: Angelo Dureghello <[email protected]>
---
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
From: Angelo Dureghello <[email protected]>
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 <[email protected]>
---
.../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
On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
<[email protected]> wrote:
>
> From: Angelo Dureghello <[email protected]>
>
> Add support for ad3541r and ad3551r single output variants.
>
> Signed-off-by: Angelo Dureghello <[email protected]>
> ---
> .../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.
On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
<[email protected]> wrote:
>
> From: Angelo Dureghello <[email protected]>
>
> Add support for single-output dac variants.
>
> Signed-off-by: Angelo Dureghello <[email protected]>
> ---
> 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)
On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
<[email protected]> wrote:
>
> From: Angelo Dureghello <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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)
On Fri, 10 May 2024 16:18:34 +0200
Angelo Dureghello <[email protected]> wrote:
> From: Angelo Dureghello <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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])$":
On Fri, 10 May 2024 10:39:31 -0500
David Lechner <[email protected]> wrote:
> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
> <[email protected]> wrote:
> >
> > From: Angelo Dureghello <[email protected]>
> >
> > Add support for ad3541r and ad3551r single output variants.
> >
> > Signed-off-by: Angelo Dureghello <[email protected]>
> > ---
> > .../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
On Fri, May 10, 2024 at 10:43:18AM -0500, David Lechner wrote:
> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
> <[email protected]> wrote:
> >
> > From: Angelo Dureghello <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > .../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
On Mon, May 13, 2024 at 1:52 PM Rob Herring <[email protected]> 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
> > <[email protected]> wrote:
> > >
> > > From: Angelo Dureghello <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > .../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.
Hi David,
On 10/05/24 5:42 PM, David Lechner wrote:
> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
> <[email protected]> wrote:
>> From: Angelo Dureghello <[email protected]>
>>
>> Add support for single-output dac variants.
>>
>> Signed-off-by: Angelo Dureghello <[email protected]>
>> ---
>> 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
Hi,
On 11/05/24 6:05 PM, Jonathan Cameron wrote:
> On Fri, 10 May 2024 10:39:31 -0500
> David Lechner <[email protected]> wrote:
>
>> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
>> <[email protected]> wrote:
>>> From: Angelo Dureghello <[email protected]>
>>>
>>> Add support for ad3541r and ad3551r single output variants.
>>>
>>> Signed-off-by: Angelo Dureghello <[email protected]>
>>> ---
>>> .../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