iio: dac: ad5504: bindings, cleanups, locking, and scale fixes
Subsystems (3)
v6 cover iio: dac: ad5504: bindings, cleanups, locking, and scale fixes
Jonathan Cameron applied patches 2 (sort headers alphabetically) and 4 (introduce local dev pointer) from v3 directly to iio/testing. This series rebases on top of that state and does not re-send those patches.
This v6 series addresses one remaining item from the v5 review by Andy Shevchenko:
* In ad5504_write_dac_powerdown(), move guard(mutex) to after kstrtobool() since input parsing does not touch shared state and does not need to be under the lock (Andy)
Testing:
No functional change from v5. This only narrows the critical section in one sysfs write path. Re-verified on ARM64 Raspberry Pi 5 (BCM2712):
powerdown sysfs read/write and powerdown mode switching (20kohm_to_gnd, three_state) verified on all four channels without errors.
Previous versions:
v5: https://lore.kernel.org/linux-iio/[email protected]/
v4: https://lore.kernel.org/linux-iio/[email protected]/
v3: https://lore.kernel.org/linux-iio/[email protected]/
v2: https://lore.kernel.org/linux-iio/[email protected]/
v1: https://lore.kernel.org/linux-iio/[email protected]/
Thank you to Jonathan Cameron, David Lechner, Andy Shevchenko, Nuno Sá, Krzysztof Kozlowski, Conor Dooley, Uwe Kleine-König, and Joshua Crofts for the continued reviews.
Taha Ed-Dafili (6):
dt-bindings: iio: dac: ad5504: add output-range and missing gpios
iio: dac: ad5504: Align headers with IWYU principle
iio: dac: ad5504: separate ACPI and DT paths
iio: dac: ad5504: remove legacy platform data support
iio: dac: ad5504: introduce local lock to protect state and spi
transfers
iio: dac: ad5504: support scale via output-range-microvolt property
.../bindings/iio/dac/adi,ad5504.yaml | 39 ++++++++- drivers/iio/dac/ad5504.c | 82 +++++++++++++++---- include/linux/iio/dac/ad5504.h | 15 ---- 3 files changed, 103 insertions(+), 33 deletions(-) delete mode 100644 include/linux/iio/dac/ad5504.h
Signature · 1 line
2.55.0
PATCH v6 1/6 dt-bindings: iio: dac: ad5504: add output-range and missing gpios
The AD5504 output range (0-30V or 0-60V) is determined by the R_SEL pin.
Use standard output-range-microvolt and range-sel-gpios properties to describe the hardware configuration of the R_SEL pin. Ensure mutual exclusivity using the not/required logic. Additionally, add missing vlogic-supply, clr-gpios, ldac-gpios and datasheet links, and provide a complete usage example.
--- .../bindings/iio/dac/adi,ad5504.yaml | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +38 -1
@@ -10,8 +10,10 @@ maintainers: - Lars-Peter Clausen <[email protected]> - Jonathan Cameron <[email protected]> -description: +description: | High voltage (up to 60V) DACs with temperature sensor alarm function + https://www.analog.com/media/en/technical-documentation/data-sheets/ad5504.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad5501.pdf properties: compatible:
@@ -27,6 +29,29 @@ properties: maxItems: 1 vcc-supply: true + vlogic-supply: true + + output-range-microvolt: + description: | + Specify the channel output full scale range. The R_SEL pin + determines if the range is 0-30V or 0-60V. + items: + - const: 0 + - enum: [30000000, 60000000] + default: [0, 60000000] + + range-sel-gpios: + description: + GPIO connected to the R_SEL pin to select the output voltage range. + maxItems: 1 + + clr-gpios: + description: GPIO that controls the /CLR pin (active low). + maxItems: 1 + + ldac-gpios: + description: GPIO that controls the /LDAC pin (active low). + maxItems: 1 additionalProperties: false
@@ -34,9 +59,17 @@ required: - compatible - reg +allOf: + - not: + required: + - range-sel-gpios + - output-range-microvolt + examples: - | #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> + spi { #address-cells = <1>; #size-cells = <0>;
@@ -45,6 +78,10 @@ examples: compatible = "adi,ad5504"; vcc-supply = <&dac_vcc>; interrupts = <55 IRQ_TYPE_EDGE_FALLING>; + + output-range-microvolt = <0 60000000>; + clr-gpios = <&gpio 4 GPIO_ACTIVE_LOW>; + ldac-gpios = <&gpio 5 GPIO_ACTIVE_LOW>; }; }; ...
Signature · 1 line
2.55.0
David Lechner
reviewerRe: [PATCH v6 1/6] dt-bindings: iio: dac: ad5504: add output-range and missing gpios
Quoted 41 lines · Taha Ed-Dafili
The AD5504 output range (0-30V or 0-60V) is determined by the
R_SELpin.Use standard output-range-microvolt and range-sel-gpios properties to describe the hardware configuration of the
R_SELpin. Ensure mutual exclusivity using the not/required logic. Additionally, add missing vlogic-supply, clr-gpios, ldac-gpios and datasheet links, and provide a complete usage example.Signed-off-by Taha Ed-Dafili <[email protected]>Acked-by Conor Dooley <[email protected]>--- .../bindings/iio/dac/adi,ad5504.yaml | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +10
-1@@ -10,8 +10,10 @@ maintainers: - Lars-Peter Clausen <[email protected]> - Jonathan Cameron <[email protected]> -description: +description: | High voltage (up to 60V) DACs with temperature sensor alarm function + https://www.analog.com/media/en/technical-documentation/data-sheets/ad5504.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad5501.pdf properties: compatible:@@ -27,6 +29,29 @@ properties: maxItems: 1 vcc-supply: true + vlogic-supply: true + + output-range-microvolt: + description: | + Specify the channel output full scale range. The R_SEL pin + determines if the range is 0-30V or 0-60V. + items:
Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml properties: · line 48
Quoted 10 lines
Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +10
-0+ - const: 0 + - enum: [30000000, 60000000] + default: [0, 60000000] + + range-sel-gpios: + description: + GPIO connected to the R_SEL pin to select the output voltage range. + maxItems: 1 + + clr-gpios:
This is one that is starting to emerge as a common name, so I would call it clear-gpios instead to match the same name on chips with a similar function.
Quoted 38 lines
Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +18
-0+ description: GPIO that controls the /CLR pin (active low). + maxItems: 1 + + ldac-gpios: + description: GPIO that controls the /LDAC pin (active low). + maxItems: 1 additionalProperties: false@@ -34,9 +59,17 @@ required: - compatible - reg +allOf: + - not: + required: + - range-sel-gpios + - output-range-microvolt + examples: - | #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> + spi { #address-cells = <1>; #size-cells = <0>;@@ -45,6 +78,10 @@ examples: compatible = "adi,ad5504"; vcc-supply = <&dac_vcc>; interrupts = <55 IRQ_TYPE_EDGE_FALLING>; + + output-range-microvolt = <0 60000000>; + clr-gpios = <&gpio 4 GPIO_ACTIVE_LOW>; + ldac-gpios = <&gpio 5 GPIO_ACTIVE_LOW>; }; };
Re: [PATCH v6 1/6] dt-bindings: iio: dac: ad5504: add output-range and missing gpios
Quoted 3 lines
This is one that is starting to emerging as a common name, so I would call it clear-gpios instead to match the same name on chips with a similar function.
Worth noting Nuno asked for the opposite in v2 review, specifically to match the datasheet's /CLR pin name:
https://lore.kernel.org/linux-iio/[email protected]/
Let me know which name you'd prefer given that context, and I'll update it in v7.
Best Regards,
Taha Ed-Dafili
David Lechner
reviewerRe: [PATCH v6 1/6] dt-bindings: iio: dac: ad5504: add output-range and missing gpios
Quoted 6 lines · Taha Ed-Dafili
Quoted 3 lines
This is one that is starting to emerging as a common name, so I would call it clear-gpios instead to match the same name on chips with a similar function.
Worth noting Nuno asked for the opposite in v2 review, specifically to match the datasheet's /CLR pin name:
Normally I advocate for matching the pin name too. But there are some common things like reset-gpios where we try to always use the same name. This clear input seems to be one of those common things.
Quoted 8 lines
https://lore.kernel.org/linux-iio/[email protected]/
Let me know which name you'd prefer given that context, and I'll update it in v7.
Best Regards,
Taha Ed-Dafili
There is only one binding currently with clr-gpios (adi,ltc2688).
And only one with clear-gpios (adi,ad5791). Plus at least two more coming soon [1] [2].
In [3], there is a /CLR pin, but we called it reset-gpios because it also resets the control registers.
I didn't re-check the datasheets on all of these, but hopefully we are being consistent.
[1]: https://lore.kernel.org/linux-iio/[email protected]/
[2]: https://lore.kernel.org/linux-iio/[email protected]/
[3]: https://lore.kernel.org/linux-iio/[email protected]/
Jonathan Cameron
maintainerRe: [PATCH v6 1/6] dt-bindings: iio: dac: ad5504: add output-range and missing gpios
Quoted 15 lines · On Sat, 22 Aug 2026 17:37:22 -0500 David Lechner <[email protected]> wrote:
Quoted 6 lines · Taha Ed-Dafili
Quoted 3 lines
This is one that is starting to emerging as a common name, so I would call it clear-gpios instead to match the same name on chips with a similar function.
Worth noting Nuno asked for the opposite in v2 review, specifically to match the datasheet's /CLR pin name:
Normally I advocate for matching the pin name too. But there are some common things like reset-gpios where we try to always use the same name. This clear input seems to be one of those common things.
Quoted 3 lines
Quoted 8 lines
Quoted 5 lines
Let me know which name you'd prefer given that context, and I'll update it in v7.
Best Regards,
Taha Ed-DafiliThere is only one binding currently with clr-gpios (adi,ltc2688).
That one is a reset I think. Includes "The control registers are cleared to default values". It sort of clears the DAC as the default is 0V So probably like 3 below.
Quoted 3 lines
And only one with clear-gpios (adi,ad5791). Plus at least two more coming soon [1] [2].
ad5701 clr sets it to a user defined value.. If that's left as default it sets to 0. That does overwrite the dac register (table 9)
ad5529r actually calls it clear on the datasheet. And just sets DAC outputs to 0 (including the registers)
dac8163 is 0 scale, or mid scale depending on part. Registers written.
Quoted 6 lines
In [3], there is a /CLR pin, but we called it reset-gpios because it also resets the control registers.
I didn't re-check the datasheets on all of these, but hopefully we are being consistent.
Other than the one that maybe should have been a reset and isn't :(
They aren't quite the same across parts but close enough perhaps to generalize the binding with some flexibility in the description.
Jonathan
PATCH v6 2/6 iio: dac: ad5504: Align headers with IWYU principle
In reply to Taha Ed-DafiliUpdate the header inclusions to follow the IWYU principle and ensure they are sorted alphabetically:
- Remove <linux/fs.h>, <linux/slab.h>, and <linux/kernel.h> as they are unused. The driver relies on devm_ managed allocations, so slab is not required.
- Replace <linux/bitops.h> with <linux/bits.h> as only the BIT() and GENMASK() macros are used.
- Add <linux/errno.h> and <linux/types.h> for error codes and data types.
- Add <asm/byteorder.h> for cpu_to_be16().
- Add <linux/array_size.h> for ARRAY_SIZE().
- Add <linux/kstrtox.h> for string to integer conversions.
- Replaced <linux/device.h> with <linux/dev_printk.h>
--- drivers/iio/dac/ad5504.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
drivers/iio/dac/ad5504.c +8 -5
@@ -5,22 +5,25 @@* Copyright 2011 Analog Devices Inc. */ -#include <linux/bitops.h> -#include <linux/device.h> -#include <linux/fs.h> +#include <linux/array_size.h> +#include <linux/bits.h> +#include <linux/dev_printk.h> +#include <linux/errno.h> #include <linux/interrupt.h> -#include <linux/kernel.h> +#include <linux/kstrtox.h> #include <linux/module.h> #include <linux/regulator/consumer.h> -#include <linux/slab.h> #include <linux/spi/spi.h> #include <linux/sysfs.h> +#include <linux/types.h> #include <linux/iio/dac/ad5504.h> #include <linux/iio/events.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> +#include <asm/byteorder.h> +#define AD5504_RES_MASK GENMASK(11, 0)#define AD5504_CMD_READ BIT(15)#define AD5504_CMD_WRITE 0
Signature · 1 line
2.55.0
PATCH v6 3/6 iio: dac: ad5504: separate ACPI and DT paths
In reply to Taha Ed-DafiliThe driver relied on a fragile -ENODEV check from the regulator framework to bypass voltage configuration on ACPI platforms, sharing the same fallback path as legacy pdata.
Fork probe on is_acpi_device_node() to explicitly handle each path. On ACPI, skip the regulator and use a fixed 60V default via AD5504_VREF_ACPI_DEFAULT_mV. The pdata fallback remains for non-ACPI systems in this commit; it is removed in the next.
--- drivers/iio/dac/ad5504.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)
drivers/iio/dac/ad5504.c +20 -9
@@ -5,6 +5,7 @@* Copyright 2011 Analog Devices Inc. */ +#include <linux/acpi.h> #include <linux/array_size.h> #include <linux/bits.h> #include <linux/dev_printk.h>
@@ -12,10 +13,12 @@#include <linux/interrupt.h> #include <linux/kstrtox.h> #include <linux/module.h> +#include <linux/property.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> #include <linux/sysfs.h> #include <linux/types.h> +#include <linux/units.h> #include <linux/iio/dac/ad5504.h> #include <linux/iio/events.h>
@@ -24,6 +27,10 @@#include <asm/byteorder.h> +/* +* In case of ACPI, we use the 60 V as default voltage reference. +*/ +#define AD5504_VREF_ACPI_DEFAULT_mV (60 * MILLI)#define AD5504_RES_MASK GENMASK(11, 0)#define AD5504_CMD_READ BIT(15)#define AD5504_CMD_WRITE 0
@@ -285,16 +292,20 @@ static int ad5504_probe(struct spi_device *spi) st =iio_priv (indio_dev); - ret =devm_regulator_get_enable_read_voltage (dev,"vcc" ); -if (ret <0 && ret != -ENODEV ) -return ret; -if (ret == -ENODEV ) { -if (pdata->vref_mv) - st->vref_mv = pdata->vref_mv; -else -dev_warn (dev,"reference voltage unspecified\n" ); +if (is_acpi_device_node (dev_fwnode (dev))) { + st->vref_mv = AD5504_VREF_ACPI_DEFAULT_mV; }else { - st->vref_mv = ret /1000 ; + ret =devm_regulator_get_enable_read_voltage (dev,"vcc" ); +if (ret <0 && ret != -ENODEV ) +return ret; +if (ret == -ENODEV ) { +if (pdata->vref_mv) + st->vref_mv = pdata->vref_mv; +else +dev_warn (dev,"reference voltage unspecified\n" ); + }else { + st->vref_mv = ret /1000 ; + } } st->spi = spi;
Signature · 1 line
2.55.0
PATCH v6 4/6 iio: dac: ad5504: remove legacy platform data support
In reply to Taha Ed-DafiliThe AD5504 driver has no in-tree users of its legacy platform data support. Now that ACPI has its own explicit path, non-ACPI systems can safely treat the vcc regulator as mandatory.
Drop the ad5504_platform_data structure, remove the fallback logic from probe, and delete include/linux/iio/dac/ad5504.h.
--- drivers/iio/dac/ad5504.c | 17 +++++------------ include/linux/iio/dac/ad5504.h | 15 --------------- 2 files changed, 5 insertions(+), 27 deletions(-) delete mode 100644 include/linux/iio/dac/ad5504.h
drivers/iio/dac/ad5504.c +5 -12
@@ -20,7 +20,6 @@#include <linux/types.h> #include <linux/units.h> -#include <linux/iio/dac/ad5504.h> #include <linux/iio/events.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h>
@@ -281,7 +280,6 @@ static const struct iio_chan_spec ad5504_channels[] = {static int ad5504_probe (struct spi_device *spi) {struct device *dev = &spi->dev; -const struct ad5504_platform_data *pdata =dev_get_platdata (dev);struct iio_dev *indio_dev;struct ad5504_state *st;int ret;
@@ -296,16 +294,11 @@ static int ad5504_probe(struct spi_device *spi) st->vref_mv = AD5504_VREF_ACPI_DEFAULT_mV; }else { ret =devm_regulator_get_enable_read_voltage (dev,"vcc" ); -if (ret <0 && ret != -ENODEV ) -return ret; -if (ret == -ENODEV ) { -if (pdata->vref_mv) - st->vref_mv = pdata->vref_mv; -else -dev_warn (dev,"reference voltage unspecified\n" ); - }else { - st->vref_mv = ret /1000 ; - } +if (ret <0 ) +return dev_err_probe (dev, ret, +"Failed to get vcc regulator\n" ); + + st->vref_mv = ret /1000 ; } st->spi = spi;
include/linux/iio/dac/ad5504.h deleted +0 -15
@@ -1,15 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* -* AD5504 SPI DAC driver -* -* Copyright 2011 Analog Devices Inc. -*/ - -#ifndef SPI_AD5504_H_ -#define SPI_AD5504_H_ - -struct ad5504_platform_data { -u16 vref_mv; -}; - -#endif /* SPI_AD5504_H_ */
Signature · 1 line
2.55.0
PATCH v6 5/6 iio: dac: ad5504: introduce local lock to protect state and spi transfers
In reply to Taha Ed-DafiliThe driver lacks locking, exposing concurrent access to the shared DMA SPI buffers, the non-atomic CTRL+NOOP write sequence in powerdown, and the pwr_down_mask/pwr_down_mode state variables.
Introduce a mutex via devm_mutex_init() and guard(mutex) at the IIO callbacks to serialize access. In read_raw() and write_raw(), scope the lock to IIO_CHAN_INFO_RAW only since IIO_CHAN_INFO_SCALE reads vref_mv which is immutable after probe.
--- drivers/iio/dac/ad5504.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)
drivers/iio/dac/ad5504.c +24 -2
@@ -8,11 +8,13 @@#include <linux/acpi.h> #include <linux/array_size.h> #include <linux/bits.h> +#include <linux/cleanup.h> #include <linux/dev_printk.h> #include <linux/errno.h> #include <linux/interrupt.h> #include <linux/kstrtox.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/property.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h>
@@ -55,10 +57,12 @@* @pwr_down_mask: power down mask * @pwr_down_mode: current power down mode * @data: transfer buffer +* @lock: lock to protect state and spi transfers */ struct ad5504_state {struct spi_device *spi;struct regulator *reg; +struct mutex lock;unsigned short vref_mv;unsigned pwr_down_mask;unsigned pwr_down_mode;
@@ -109,7 +113,9 @@ static int ad5504_read_raw(struct iio_dev *indio_dev,int ret;switch (m) { -case IIO_CHAN_INFO_RAW : +case IIO_CHAN_INFO_RAW : { +guard (mutex)(&st->lock); + ret =ad5504_spi_read (st, chan->address);if (ret <0 )return ret;
@@ -117,6 +123,7 @@ static int ad5504_read_raw(struct iio_dev *indio_dev, *val = ret;return IIO_VAL_INT ; + }case IIO_CHAN_INFO_SCALE : *val = st->vref_mv; *val2 = chan->scan_type.realbits;
@@ -134,11 +141,14 @@ static int ad5504_write_raw(struct iio_dev *indio_dev,struct ad5504_state *st =iio_priv (indio_dev);switch (mask) { -case IIO_CHAN_INFO_RAW : +case IIO_CHAN_INFO_RAW : { +guard (mutex)(&st->lock); +if (val >= (1 << chan->scan_type.realbits) || val <0 )return -EINVAL ;return ad5504_spi_write (st, chan->address, val); + }default :return -EINVAL ; }
@@ -154,6 +164,8 @@ static int ad5504_get_powerdown_mode(struct iio_dev *indio_dev, {struct ad5504_state *st =iio_priv (indio_dev); +guard (mutex)(&st->lock); +return st->pwr_down_mode; }
@@ -162,6 +174,8 @@ static int ad5504_set_powerdown_mode(struct iio_dev *indio_dev, {struct ad5504_state *st =iio_priv (indio_dev); +guard (mutex)(&st->lock); + st->pwr_down_mode = mode;return 0 ;
@@ -179,6 +193,8 @@ static ssize_t ad5504_read_dac_powerdown(struct iio_dev *indio_dev, {struct ad5504_state *st =iio_priv (indio_dev); +guard (mutex)(&st->lock); +return sysfs_emit (buf,"%d\n" , !(st->pwr_down_mask & (1 << chan->channel))); }
@@ -195,6 +211,8 @@ static ssize_t ad5504_write_dac_powerdown(struct iio_dev *indio_dev,if (ret)return ret; +guard (mutex)(&st->lock); +if (pwr_down) st->pwr_down_mask &= ~(1 << chan->channel);else
@@ -290,6 +308,10 @@ static int ad5504_probe(struct spi_device *spi) st =iio_priv (indio_dev); + ret =devm_mutex_init (dev, &st->lock); +if (ret) +return ret; +if (is_acpi_device_node (dev_fwnode (dev))) { st->vref_mv = AD5504_VREF_ACPI_DEFAULT_mV; }else {
Signature · 1 line
2.55.0
PATCH v6 6/6 iio: dac: ad5504: support scale via output-range-microvolt property
In reply to Taha Ed-DafiliThe AD5504 output range is set by the R_SEL pin, not the VCC supply. Add support for 'output-range-microvolt' to set vref_mv directly from firmware. Use device_property_present() to distinguish absent from malformed property rather than relying on the ambiguous -EINVAL from device_property_read_u32_array(). Fall back to the vcc regulator voltage for old DTs that predate this property.
--- drivers/iio/dac/ad5504.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)
drivers/iio/dac/ad5504.c +25 -6
@@ -300,6 +300,7 @@ static int ad5504_probe(struct spi_device *spi)struct device *dev = &spi->dev;struct iio_dev *indio_dev;struct ad5504_state *st; +u32 range[2 ];int ret; indio_dev =devm_iio_device_alloc (dev,sizeof (*st));
@@ -315,12 +316,30 @@ static int ad5504_probe(struct spi_device *spi)if (is_acpi_device_node (dev_fwnode (dev))) { st->vref_mv = AD5504_VREF_ACPI_DEFAULT_mV; }else { - ret =devm_regulator_get_enable_read_voltage (dev,"vcc" ); -if (ret <0 ) -return dev_err_probe (dev, ret, -"Failed to get vcc regulator\n" ); - - st->vref_mv = ret /1000 ; +if (device_property_present (dev,"output-range-microvolt" )) { + ret =device_property_read_u32_array (dev,"output-range-microvolt" , + range,ARRAY_SIZE (range)); +if (ret) +return dev_err_probe (dev, ret, +"Error parsing output-range-microvolt\n" ); + +if (range[0 ] !=0 || (range[1 ] !=30 *MICRO && range[1 ] !=60 *MICRO )) +return dev_err_probe (dev, -EINVAL , +"Invalid output-range-microvolt\n" ); + + st->vref_mv = range[1 ] / (MICRO /MILLI ); + + ret =devm_regulator_get_enable (dev,"vcc" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to enable vcc regulator\n" ); + }else { +/* Backward compat: old DTs without output-range-microvolt */ + ret =devm_regulator_get_enable_read_voltage (dev,"vcc" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get vcc regulator\n" ); + + st->vref_mv = ret / (MICRO /MILLI ); + } } st->spi = spi;
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v6 6/6] iio: dac: ad5504: support scale via output-range-microvolt property
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The driver ignores the unified 'output-range-microvolt' device property for ACPI nodes. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver ignores the unified 'output-range-microvolt' device property for ACPI nodes. --
--- Patch [6]: [PATCH v6 6/6] iio: dac: ad5504: support scale via output-range-microvolt property --- commit b4531e8367c77df3a5fc929eab7e4b61b58ad05a
Author: Taha Ed-Dafili <[email protected]>
iio: dac: ad5504: support scale via output-range-microvolt property
This commit introduces the output-range-microvolt device property to configure the vref_mv scale directly from firmware. It provides a fallback to the vcc regulator voltage for older device trees that do not have this property.
drivers/iio/dac/ad5504.c static int ad5504_probe(struct spi_device *spi) · line 319
Quoted 9 lines
drivers/iio/dac/ad5504.c +1
-0@@ -315,12 +316,30 @@ static int ad5504_probe(struct spi_device *spi)if (is_acpi_device_node (dev_fwnode (dev))) { st->vref_mv = AD5504_VREF_ACPI_DEFAULT_mV; }else { +if (device_property_present (dev,"output-range-microvolt" )) {
[Severity: Medium]
Does this structure prevent ACPI devices from utilizing the newly added output-range-microvolt property?
Since the device_property_present() check in ad5504_probe() is located inside the else block of the is_acpi_device_node() condition, an ACPI platform specifying this property via _DSD will have it silently ignored. The driver will instead always use the hardcoded AD5504_VREF_ACPI_DEFAULT_mV value.
Should the firmware-agnostic device property check occur before falling back to the legacy ACPI-specific default?
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
David Lechner
reviewerRe: [PATCH v6 6/6] iio: dac: ad5504: support scale via output-range-microvolt property
In reply to Taha Ed-DafiliQuoted 40 lines · Taha Ed-Dafili
The AD5504 output range is set by the
R_SELpin, not the VCC supply. Add support for 'output-range-microvolt' to setvref_mvdirectly from firmware. Usedevice_property_present()to distinguish absent from malformed property rather than relying on the ambiguous-EINVALfromdevice_property_read_u32_array(). Fall back to the vcc regulator voltage for old DTs that predate this property.Signed-off-by Taha Ed-Dafili <[email protected]>--- drivers/iio/dac/ad5504.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)drivers/iio/dac/ad5504.c +7
-6@@ -300,6 +300,7 @@ static int ad5504_probe(struct spi_device *spi)struct device *dev = &spi->dev;struct iio_dev *indio_dev;struct ad5504_state *st; +u32 range[2 ];int ret; indio_dev =devm_iio_device_alloc (dev,sizeof (*st));@@ -315,12 +316,30 @@ static int ad5504_probe(struct spi_device *spi)if (is_acpi_device_node (dev_fwnode (dev))) { st->vref_mv = AD5504_VREF_ACPI_DEFAULT_mV; }else { - ret =devm_regulator_get_enable_read_voltage (dev,"vcc" ); -if (ret <0 ) -return dev_err_probe (dev, ret, -"Failed to get vcc regulator\n" ); - - st->vref_mv = ret /1000 ; +if (device_property_present (dev,"output-range-microvolt" )) { + ret =device_property_read_u32_array (dev,"output-range-microvolt" , + range,ARRAY_SIZE (range)); +if (ret) +return dev_err_probe (dev, ret, +"Error parsing output-range-microvolt\n" );
drivers/iio/dac/ad5504.c static int ad5504_probe(struct spi_device *spi) · line 334
Quoted 10 lines
drivers/iio/dac/ad5504.c +10
-0+ +if (range[0 ] !=0 || (range[1 ] !=30 *MICRO && range[1 ] !=60 *MICRO )) +return dev_err_probe (dev, -EINVAL , +"Invalid output-range-microvolt\n" ); + + st->vref_mv = range[1 ] / (MICRO /MILLI ); + + ret =devm_regulator_get_enable (dev,"vcc" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to enable vcc regulator\n" );
We should probably handle range-sel-gpios here. Otherwise, if a new DT tries to use it, it will end up falling back to this old wrong behavior.
} else if (device_property_present("range-sel-gpios")) {Either return an error here that it is not supported. Or implement it by making IIO_CHAN_INFO_SCALE writable.
Quoted 11 lines
drivers/iio/dac/ad5504.c +8
-0+ }else { +/* Backward compat: old DTs without output-range-microvolt */ + ret =devm_regulator_get_enable_read_voltage (dev,"vcc" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get vcc regulator\n" ); + + st->vref_mv = ret / (MICRO /MILLI ); + } } st->spi = spi;
Re: [PATCH v6 6/6] iio: dac: ad5504: support scale via output-range-microvolt property
Quoted 6 lines
We should probably handle range-sel-gpios here. Otherwise, if a new DT tries to use it, it will end up falling back to this old wrong behavior.
Either return an error here that it is not supported. Or implement it by making
IIO_CHAN_INFO_SCALEwritable.
Thank you for catching that. For now I'll return an error in that case, we can always expand it later if needed.
Best Regards,
Taha Ed-Dafili