Re: [PATCH v3 2/2] iio: dac: Add AD5529R DAC driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Incorrect error handling for the optional 'hvss' regulator causes probe failure when it is absent.
- [High] A signedness error during the parsing of the 'reg' property allows an out-of-bounds array write using a negative channel index.
- [High] Incomplete hardware reset sequence fails to guarantee a known device state.
- [Medium] The driver allows an external voltage reference to be provided but fails to read its voltage to dynamically scale the output ranges.
- [Medium] Reading raw DAC values from the write-only input register instead of the dedicated readback registers.
- [Low] Passing incompatible pointer types to `fwnode_property_read_u32` functions causes compiler warnings/errors.
- [Low] The `scan_type` struct inside `struct iio_chan_spec` is initialized with a non-existent member `.format`, causing a build failure.
--

commit c4b761222e21c0ab2352b5405104edc721f22ee1
Author: Janani Sunil <janani.sunil@analog.com>
Subject: iio: dac: Add AD5529R DAC driver support

Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 0000000000000..9bb63030db952
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c
> @@ -0,0 +1,527 @@
[ ... ]
> +#define AD5529R_DAC_CHANNEL(chan, bits) {			\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = (chan),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE) |	\
> +			      BIT(IIO_CHAN_INFO_OFFSET),	\
> +	.scan_type = {						\
> +		.format = 'u',					\

[Severity: Low]
Is this member name correct? struct iio_scan_type (defined in 
include/linux/iio/types.h) does not have a member named format. It appears
the correct member name is sign.

Will attempting to compile this code result in a compiler error about
the struct having no member named format?

> +		.realbits = (bits),				\
> +		.storagebits = 16,				\
> +	},							\
> +}
[ ... ]
> +static int ad5529r_reset(struct ad5529r_state *st)
> +{
> +	struct reset_control *rst;
> +	int ret;
> +
> +	rst = devm_reset_control_get_optional_exclusive(&st->spi->dev, NULL);
> +	if (IS_ERR(rst))
> +		return PTR_ERR(rst);
> +
> +	if (rst) {
> +		ret = reset_control_deassert(rst);

[Severity: High]
Does this sequence guarantee a hardware reset?

Since the driver calls reset_control_deassert(rst) without first asserting
the reset line, if the device was already out of reset (e.g., left running
by the bootloader or after a warm reboot), deasserting an already deasserted
line might do nothing.

Would using reset_control_reset(rst) ensure the device receives a reset edge?

> +		if (ret)
> +			return ret;
> +	} else {
[ ... ]
> +static int ad5529r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ad5529r_state *st = iio_priv(indio_dev);
> +	unsigned int reg_addr, reg_val_h;
> +	int ret, range_idx, span_mv;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		reg_addr = AD5529R_REG_DAC_INPUT_A(chan->channel);

[Severity: Medium]
Is it intentional to read back from the input register AD5529R_REG_DAC_INPUT_A?

The driver explicitly defines a dedicated range of registers for reading back
DAC data starting at AD5529R_REG_DAC_DATA_READBACK_BASE, which are marked as
read-only in the regmap configuration.

Could reading from the input register instead of the readback register return
zeroes or stale data depending on the hardware latching mechanism?

> +		ret = regmap_read(st->regmap_16bit, reg_addr, &reg_val_h);
> +		if (ret)
> +			return ret;
[ ... ]
> +static int ad5529r_parse_channel_ranges(struct device *dev,
> +					struct ad5529r_state *st)
> +{
> +	int ret, ch, range_idx;
> +	s32 vals[2];
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		range_idx = AD5529R_RANGE_0V_5V;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);

[Severity: Low]
Will passing the address of an int to fwnode_property_read_u32() cause
incompatible pointer type warnings? fwnode_property_read_u32() expects
a u32 *.

[Severity: High]
Could passing the address of the signed int ch to fwnode_property_read_u32()
allow an out-of-bounds array write?

If the device tree provides a reg value with the most significant bit set
(e.g., 0x80000000), it would be written as an unsigned 32-bit integer but
interpreted as a negative value by the signed integer ch.

The subsequent bounds check:

    if (ch >= 16)

would evaluate to false for negative values, allowing the check to be bypassed.
The driver would then write to st->output_range_idx[ch] using a negative index.

> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing reg property in channel node\n");
> +
> +		if (ch >= 16)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid channel number: %d\n", ch);
> +
> +		if (!fwnode_property_read_u32_array(child,
> +						    "adi,output-range-microvolt",
> +						    vals, 2)) {

[Severity: Low]
Will passing an array of s32 vals to fwnode_property_read_u32_array() cause
incompatible pointer type warnings? It expects a u32 *.

> +			range_idx = ad5529r_find_output_range(vals);
> +			if (range_idx < 0)
> +				return dev_err_probe(dev, range_idx,
> +						     "Invalid range [%d %d] for ch %d\n",
> +						     vals[0], vals[1], ch);
> +		}
> +
> +		st->output_range_idx[ch] = range_idx;
[ ... ]
> +static int ad5529r_probe(struct spi_device *spi)
> +{
[ ... ]
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad5529r_supply_names),
> +					     ad5529r_supply_names);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable regulators\n");
> +
> +	ret = devm_regulator_get_enable_optional(dev, "hvss");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable hvss regulator\n");

[Severity: High]
Does this error handling prevent the driver from probing when the optional
hvss regulator is absent?

devm_regulator_get_enable_optional() returns -ENODEV if the optional regulator
is not provided in the device tree. Because the code checks if (ret) without
ignoring -ENODEV, this causes dev_err_probe() to return -ENODEV and fail
the probe entirely, effectively making the optional regulator mandatory.

> +
> +	st->vref_regulator = devm_regulator_get_optional(dev, "vref");
> +	if (IS_ERR(st->vref_regulator)) {
> +		if (PTR_ERR(st->vref_regulator) != -ENODEV)
> +			return dev_err_probe(dev, PTR_ERR(st->vref_regulator),
> +					     "Failed to get vref regulator\n");
> +		st->vref_regulator = NULL;
> +	}
> +
> +	if (st->vref_regulator) {
> +		ret = regulator_enable(st->vref_regulator);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to enable vref regulator\n");
> +
> +		ret = devm_add_action_or_reset(dev, ad5529r_disable_regulator,
> +					       st->vref_regulator);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to add vref regulator cleanup\n");
> +	}

[Severity: Medium]
If an external voltage reference is provided, should the driver query its
voltage to dynamically scale the output ranges?

The driver requests the optional vref regulator and configures the hardware
to use it instead of the internal 4.096V reference, but it doesn't seem to
read the actual voltage of the external reference.

The IIO output scale calculations in ad5529r_read_raw() appear to hardcode
the spans based on standard ranges that assume the nominal internal 4.096V
reference. If an external reference has a different voltage, will the reported
IIO scale be mathematically incorrect?

> +
> +	st->regmap_8bit = devm_regmap_init_spi(spi, &ad5529r_regmap_8bit_config);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-ad5529r-driver-v3-0-267c0731aa68@analog.com?part=2