Re: [PATCH v3 2/2] media: i2c: ov5640: Use reset control framework to support shared reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver misuses the shared reset control by attempting to pulse it, which triggers kernel warnings, leaks references, and disrupts other shared devices.
- [High] Switching to the reset control framework breaks backward compatibility for existing DTs using the 'reset-gpios' property unless CONFIG_RESET_GPIO is enabled.
- [Medium] The driver ignores the return values of reset control API calls, which can fail and cause silent hardware initialization errors.
--

commit 2545a9466cdd3cc29f140f86a12d3e3cb61a174f
Author: Robby Cai <robby.cai@nxp.com>

media: i2c: ov5640: Use reset control framework to support shared reset

This commit replaces the direct GPIO-based reset handling in the ov5640
driver with the reset control framework. It uses
devm_reset_control_get_optional_shared() to acquire the reset line, aiming
to support both dedicated and shared reset signals described via reset-gpios
in DT.

> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 92d2d6cd4ba48..427feedede75c 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c

[ ... ]

> @@ -2448,12 +2449,12 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable)
>   *
>   * In such cases, this gpio should be mapped to pwdn_gpio in the driver, and we
>   * should still toggle the pwdn_gpio below with the appropriate delays, while
> - * the calls to reset_gpio will be ignored.
> + * the calls to reset will be ignored.
>   */
>  static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
>  {
>  	if (sensor->pwdn_gpio) {
> -		gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +		reset_control_assert(sensor->reset);

[Severity: High]
Does this code incorrectly pulse a shared reset line?

For shared reset controls, the initial deassert_count is 0. Calling
reset_control_assert() when deassert_count == 0 triggers a WARN_ON in the
reset core and fails with -EINVAL.

Additionally, if multiple devices share the line and another device is
currently active (so deassert_count > 0), calling reset_control_assert()
here will decrement the count.

If the count reaches 0, it physically resets the line while the other device
is actively using it, which could cause that device to crash.

[Severity: Medium]
Should the return value of reset_control_assert() be checked?

Unlike simple memory-mapped GPIOs, reset controllers may reside on slow
buses (like I2C/SPI) and their operations can fail.

If this fails, the driver will silently proceed, which might result in
unexplained I2C transfer errors later.

>  
>  		/* camera power cycle */
>  		ov5640_power(sensor, false);
> @@ -2461,7 +2462,7 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
>  		ov5640_power(sensor, true);
>  		usleep_range(1000, 2000);	/* t3 */
>  
> -		gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> +		reset_control_deassert(sensor->reset);

[Severity: High]
Does this leak the reset control reference?

Because the reset control is never asserted during a power-down sequence,
the deassert_count incremented by this reset_control_deassert() call will
never be decremented, leaking the reference.

[Severity: Medium]
Should the return value of reset_control_deassert() be checked?

If a reset deassertion fails (e.g., due to an I2C bus timeout), the hardware
remains physically in reset. The driver will silently proceed to communicate
with the sensor, resulting in unexplained errors.

>  	} else {
>  		/* software reset */
>  		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,

[ ... ]

> @@ -3914,11 +3915,10 @@ static int ov5640_probe(struct i2c_client *client)
>  	if (IS_ERR(sensor->pwdn_gpio))
>  		return PTR_ERR(sensor->pwdn_gpio);
>  
> -	/* request optional reset pin */
> -	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> -						     GPIOD_OUT_HIGH);
> -	if (IS_ERR(sensor->reset_gpio))
> -		return PTR_ERR(sensor->reset_gpio);
> +	sensor->reset = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(sensor->reset))
> +		return dev_err_probe(dev, PTR_ERR(sensor->reset),
> +				     "Failed to get reset\n");

[Severity: High]
Does switching to the reset control framework break backward compatibility
for existing device trees using the reset-gpios property?

When the resets property is absent, the reset framework has a fallback to
parse reset-gpios. However, this fallback explicitly checks
IS_ENABLED(CONFIG_RESET_GPIO). If this config is disabled, it silently
returns NULL for optional resets.

Because VIDEO_OV5640 does not select RESET_GPIO in Kconfig, any existing
platform using reset-gpios without CONFIG_RESET_GPIO enabled will silently
skip the reset sequence entirely, leading to initialization failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529132334.3333294-1-robby.cai@nxp.com?part=2