Re: [PATCH v2 4/8] iio: adc: rzt2h: Add optional reset support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iio: adc: rzt2h: Shared reset control leaked if unbound while active
- [Medium] iio: adc: rzt2h: hardware settling delay bypassed when calibration is unsupported
--

commit 1e05a1801dd44e25bef73244db6d8173cc097ea8
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

iio: adc: rzt2h: Add optional reset support
    
Prepare the driver for RZ/V2H(P), where the ADC has a module reset line
that is not present on RZ/T2H. Assert the optional reset when the ADC is
runtime-suspended and deassert it on runtime resume before performing
calibration. Use a shared reset control to support SoCs such as RZ/V2N where
the reset line is shared between multiple ADC instances.

> diff --git a/drivers/iio/adc/rzt2h_adc.c b/drivers/iio/adc/rzt2h_adc.c
> index 58e33fd0249c9..62120d2185152 100644
> --- a/drivers/iio/adc/rzt2h_adc.c
> +++ b/drivers/iio/adc/rzt2h_adc.c

[ ... ]

> @@ -710,6 +712,10 @@ static int rzt2h_adc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, adc);
>  
> +	adc->rstc = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(adc->rstc))
> +		return PTR_ERR(adc->rstc);
> +

[Severity: High]
Because this driver acquires a shared reset control and enables runtime PM via
devm_pm_runtime_enable() without providing a custom unbind action or .remove
callback to synchronously suspend the device, could this leak the shared reset
control's deassert count?

If the driver unbinds while the device is in RPM_ACTIVE (for instance, due to
a concurrent sysfs read or a user setting power/control to "on"), the devres
cleanup calls pm_runtime_disable() which leaves the device active. 

This would bypass rzt2h_adc_pm_runtime_suspend() entirely, meaning
reset_control_assert() is never called, and the deassert count is permanently
leaked. Other driver instances sharing this reset line would then be unable
to assert it, potentially breaking their power management transitions.

>  	ret = rzt2h_adc_parse_properties(adc);

[ ... ]

> @@ -771,10 +777,22 @@ static const struct of_device_id rzt2h_adc_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, rzt2h_adc_match);
>  
> +static int rzt2h_adc_pm_runtime_suspend(struct device *dev)
> +{
> +	struct rzt2h_adc *adc = dev_get_drvdata(dev);
> +
> +	return reset_control_assert(adc->rstc);
> +}
> +
>  static int rzt2h_adc_pm_runtime_resume(struct device *dev)
>  {
>  	struct rzt2h_adc *adc = dev_get_drvdata(dev);
>  	const struct rzt2h_adc_chip_info *chip_info = adc->chip_info;
> +	int ret;
> +
> +	ret = reset_control_deassert(adc->rstc);
> +	if (ret)
> +		return ret;
>  
>  	if (!(chip_info->features & RZT2H_ADC_FEATURE_CALIBRATION))
>  		return 0;
> 
>  	/*
>  	 * Datasheet Page 2810, Section 41.5.6:
>  	 * After release from the module-stop state, wait for at least
>  	 * 0.5 µs before starting A/D conversion.
>  	 */
>  	fsleep(1);

[Severity: Medium]
Does this early return bypass the required hardware settling delay for
devices without the calibration feature? 

When rzt2h_adc_pm_runtime_resume() is called for hardware lacking
RZT2H_ADC_FEATURE_CALIBRATION, returning 0 immediately skips the fsleep(1)
that follows. This means the IIO core can immediately start A/D conversions on
the newly-resumed hardware before the datasheet-mandated 0.5 us module-stop
release delay has elapsed, potentially resulting in unreliable initial ADC
conversions.

>  
> -	return rzt2h_adc_calibrate(adc);
> +	ret = rzt2h_adc_calibrate(adc);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009184122.1378797-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=4