* [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r
2024-05-10 14:18 [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants Angelo Dureghello
@ 2024-05-10 14:18 ` Angelo Dureghello
2024-05-10 15:42 ` David Lechner
2024-05-10 14:18 ` [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names Angelo Dureghello
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Angelo Dureghello @ 2024-05-10 14:18 UTC (permalink / raw)
To: jic23, robh, krzk+dt, conor+dt
Cc: nuno.sa, lars, Michael.Hennerich, linux-iio, devicetree,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r
2024-05-10 14:18 ` [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r Angelo Dureghello
@ 2024-05-10 15:42 ` David Lechner
2024-05-16 13:17 ` Angelo Dureghello
0 siblings, 1 reply; 12+ messages in thread
From: David Lechner @ 2024-05-10 15:42 UTC (permalink / raw)
To: Angelo Dureghello
Cc: jic23, robh, krzk+dt, conor+dt, nuno.sa, lars, Michael.Hennerich,
linux-iio, devicetree, linux-kernel
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)
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r
2024-05-10 15:42 ` David Lechner
@ 2024-05-16 13:17 ` Angelo Dureghello
0 siblings, 0 replies; 12+ messages in thread
From: Angelo Dureghello @ 2024-05-16 13:17 UTC (permalink / raw)
To: David Lechner
Cc: jic23, robh, krzk+dt, conor+dt, nuno.sa, lars, Michael.Hennerich,
linux-iio, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names
2024-05-10 14:18 [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants Angelo Dureghello
2024-05-10 14:18 ` [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r Angelo Dureghello
@ 2024-05-10 14:18 ` Angelo Dureghello
2024-05-10 15:43 ` David Lechner
2024-05-10 15:39 ` [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants David Lechner
2024-05-11 16:04 ` Jonathan Cameron
3 siblings, 1 reply; 12+ messages in thread
From: Angelo Dureghello @ 2024-05-10 14:18 UTC (permalink / raw)
To: jic23, robh, krzk+dt, conor+dt
Cc: nuno.sa, lars, Michael.Hennerich, linux-iio, devicetree,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names
2024-05-10 14:18 ` [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names Angelo Dureghello
@ 2024-05-10 15:43 ` David Lechner
2024-05-13 18:52 ` Rob Herring
0 siblings, 1 reply; 12+ messages in thread
From: David Lechner @ 2024-05-10 15:43 UTC (permalink / raw)
To: Angelo Dureghello
Cc: jic23, robh, krzk+dt, conor+dt, nuno.sa, lars, Michael.Hennerich,
linux-iio, devicetree, linux-kernel
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)
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names
2024-05-10 15:43 ` David Lechner
@ 2024-05-13 18:52 ` Rob Herring
2024-05-13 19:03 ` David Lechner
0 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2024-05-13 18:52 UTC (permalink / raw)
To: David Lechner
Cc: Angelo Dureghello, jic23, krzk+dt, conor+dt, nuno.sa, lars,
Michael.Hennerich, linux-iio, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names
2024-05-13 18:52 ` Rob Herring
@ 2024-05-13 19:03 ` David Lechner
0 siblings, 0 replies; 12+ messages in thread
From: David Lechner @ 2024-05-13 19:03 UTC (permalink / raw)
To: Rob Herring
Cc: Angelo Dureghello, jic23, krzk+dt, conor+dt, nuno.sa, lars,
Michael.Hennerich, linux-iio, devicetree, linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants
2024-05-10 14:18 [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants Angelo Dureghello
2024-05-10 14:18 ` [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r Angelo Dureghello
2024-05-10 14:18 ` [PATCH 3/3] dt-bindings: iio: dac: fix ad3552r gain parameter names Angelo Dureghello
@ 2024-05-10 15:39 ` David Lechner
2024-05-11 16:05 ` Jonathan Cameron
2024-05-11 16:04 ` Jonathan Cameron
3 siblings, 1 reply; 12+ messages in thread
From: David Lechner @ 2024-05-10 15:39 UTC (permalink / raw)
To: Angelo Dureghello
Cc: jic23, robh, krzk+dt, conor+dt, nuno.sa, lars, Michael.Hennerich,
linux-iio, devicetree, linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants
2024-05-10 15:39 ` [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants David Lechner
@ 2024-05-11 16:05 ` Jonathan Cameron
2024-05-16 13:20 ` Angelo Dureghello
0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2024-05-11 16:05 UTC (permalink / raw)
To: David Lechner
Cc: Angelo Dureghello, robh, krzk+dt, conor+dt, nuno.sa, lars,
Michael.Hennerich, linux-iio, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants
2024-05-11 16:05 ` Jonathan Cameron
@ 2024-05-16 13:20 ` Angelo Dureghello
0 siblings, 0 replies; 12+ messages in thread
From: Angelo Dureghello @ 2024-05-16 13:20 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner
Cc: robh, krzk+dt, conor+dt, nuno.sa, lars, Michael.Hennerich,
linux-iio, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants
2024-05-10 14:18 [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants Angelo Dureghello
` (2 preceding siblings ...)
2024-05-10 15:39 ` [PATCH 1/3] dt-bindings: iio: dac: add ad35xxr single output variants David Lechner
@ 2024-05-11 16:04 ` Jonathan Cameron
3 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2024-05-11 16:04 UTC (permalink / raw)
To: Angelo Dureghello
Cc: robh, krzk+dt, conor+dt, nuno.sa, lars, Michael.Hennerich,
linux-iio, devicetree, linux-kernel
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])$":
^ permalink raw reply [flat|nested] 12+ messages in thread