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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing return value checks for `reset_control_assert()` and `reset_control_deassert()`.
- [High] The reset pin is left in a deasserted (driven) state on probe failure or device unbind, leading to potential back-powering of the DAC.
- [Medium] Acquiring the reset control after enabling regulators causes unnecessary power toggling on `-EPROBE_DEFER` and leaves the reset pin floating during power-up.
- [Medium] Missing recovery delay after deasserting the hardware reset may cause subsequent device communications to fail on fast CPUs.
--

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

iio: dac: ad5686: consume optional reset signal
    
This commit adds GPIO support for the RESET pin using an optional
reset control local to the probe function. It manually generates
a reset pulse after the device is powered up.

> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index fc3863274b291..a48acedff51c1 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
[ ... ]
> @@ -508,6 +510,15 @@ int ad5686_probe(struct device *dev,
>  
>  	fsleep(5); /* power-up time */
>  
> +	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);

[Severity: Medium]
Is it intentional to fetch the reset control after the regulators are
enabled and the 5µs power-up delay has occurred?

If devm_reset_control_get_optional_exclusive() returns -EPROBE_DEFER,
the driver will disable the regulators and have to retry the sequence
later. Fetching the reset control before enabling regulators might also
prevent the reset pin from floating during power-up.

> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc),
> +				     "Failed to get reset control\n");
> +
> +	reset_control_assert(rstc);

[Severity: High]
Do we need to check the return value of reset_control_assert() and
reset_control_deassert()?

These functions can fail and return error codes, particularly if the
reset controller is behind a slow bus. Proceeding without checking
could cause the driver to communicate with a device that is still held
in reset.

> +	fsleep(1); /* reset pulse: comfortably bigger than the spec */
> +	reset_control_deassert(rstc);

[Severity: Medium]
Does the device need a guaranteed recovery delay after the reset is
deasserted?

Exiting the hardware reset initiates an internal power-on reset
sequence. On faster CPUs, the execution time before the first SPI/I2C
command might be shorter than the device's required reset recovery time.

[Severity: High]
Should there be a registered devm action to assert the reset pin on
driver removal or probe failure?

Currently, the reset pin is deasserted here but never asserted again on
cleanup. Since devm_regulator_get_enable() automatically disables the
VDD and VLOGIC regulators during cleanup, leaving this pin driven HIGH
while the device is unpowered could forward-bias ESD protection diodes
and back-power or damage the DAC.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-ad5686-new-features-v3-0-f829fb7e9262@analog.com?part=6