Re: [PATCH v5 06/12] iio: dac: ad5686: consume optional reset signal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The return values of `reset_control_assert()` and `reset_control_deassert()` are ignored.
- [Medium] Missing reset recovery delay after hardware reset deassertion.

Pre-existing issues:
- [High] The driver unconditionally requires the `vdd` and `vlogic` regulators, breaking probe on older device trees on platforms with full regulator constraints.
--

commit a475ccccddaa1769cffb9d807bfd0a081ce1514a
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: dac: ad5686: consume optional reset signal

Add RESET pin GPIO support through an optional reset control, which is
local to the probe function. A reset pulse is manually generated after
the device is powered up.

> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index e2ebabca68873..8ad8931a3d7f8 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c

[ ... ]

> @@ -486,6 +488,11 @@ int ad5686_probe(struct device *dev,
>  	st->ops = ops;
>  	st->chip_info = chip_info;
>  
> +	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc),
> +				     "Failed to get reset control\n");
> +
>  	ret = devm_regulator_get_enable(dev, "vdd");
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to enable vdd supply\n");

[Severity: High]
This is a pre-existing issue, but does unconditionally requesting the vdd
supply here break backwards compatibility for older device trees?

If a platform boots with an older device tree that lacks these supplies and
full regulator constraints are applied, the regulator framework might return
-ENODEV and cause ad5686_probe() to abort, breaking probe on older systems.

[ ... ]

> @@ -509,6 +516,11 @@ int ad5686_probe(struct device *dev,
>  	/* 4.5us power-up time: Datasheet Table 4: Timing Characteristics */
>  	fsleep(5);
>  
> +	/* 1us >> 30ns reset pulse activation time: Datasheet Table 4 */
> +	reset_control_assert(rstc);
> +	fsleep(1);
> +	reset_control_deassert(rstc);

[Severity: High]
Can the return values of reset_control_assert() and reset_control_deassert()
be ignored here?

If the underlying reset controller (e.g., an I2C/SPI GPIO expander) encounters
a communication error, it could return a negative error code like -EIO. The
device might then remain stuck in a physical reset state, causing subsequent
communication to silently fail rather than failing early during probe with a
clear error message.

[Severity: Medium]
Is a reset recovery delay needed after deasserting the reset control before
moving on?

Since there is no explicit delay after deassertion, fast CPUs might begin
sending the first SPI/I2C commands before the DAC's internal state machine
has finished initializing from the hard reset.

> +
>  	/* Initialize masks to all ones */
>  	st->pwr_down_mask = ~0;
>  	st->pwr_down_mode = ~0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-ad5686-new-features-v5-0-577caadbf7c0@analog.com?part=6