Re: [PATCH v4 2/2] media: i2c: ov5640: Add reset controller support with GPIO fallback

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Jun 19, 2026 at 06:05:32PM +0800, robby.cai@oss.nxp.com wrote:
> [You don't often get email from robby.cai@oss.nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> From: Robby Cai <robby.cai@nxp.com>
>
> Add support for the reset controller framework by acquiring the reset
> line using devm_reset_control_get_optional_shared_deasserted(). This
> allows the driver to handle reset lines provided by a reset controller,
> including shared ones, while avoiding unbalanced deassert counts.
>
> Retain support for legacy reset-gpios as a fallback when no reset
> controller is defined. In that case, request the GPIO and keep it in the
> deasserted state as the initial configuration.
>
> This enables the driver to support both reset-controller-backed reset
> lines and older GPIO-based descriptions while preserving the existing
> power-up sequencing behavior.
>
> Signed-off-by: Robby Cai <robby.cai@nxp.com>
> ---
>  drivers/media/i2c/ov5640.c | 80 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 85ecc23b3587..5e6db8aacb11 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <media/v4l2-async.h>
> @@ -442,6 +443,7 @@ struct ov5640_dev {
>         u32 xclk_freq;
>
>         struct regulator_bulk_data supplies[OV5640_NUM_SUPPLIES];
> +       struct reset_control *reset;
>         struct gpio_desc *reset_gpio;
>         struct gpio_desc *pwdn_gpio;
>         bool   upside_down;
> @@ -2431,6 +2433,48 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
>         return ov5640_set_framefmt(sensor, &sensor->fmt);
>  }
>
> +static int ov5640_get_reset(struct device *dev, struct ov5640_dev *sensor)
> +{
> +       /* use deasserted version to avoid unbalanced deassert counts */
> +       sensor->reset =
> +           devm_reset_control_get_optional_shared_deasserted(dev, NULL);
> +       if (IS_ERR(sensor->reset))
> +               return dev_err_probe(dev, PTR_ERR(sensor->reset),
> +                                    "Failed to get reset\n");
> +       else if (sensor->reset)
> +               return 0;
> +
> +       /*
> +        * fallback to legacy reset-gpios
> +        * GPIOD_OUT_HIGH ensures deasserted state for ACTIVE_LOW reset
> +        */
> +       sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +                                                    GPIOD_OUT_HIGH);
> +       if (IS_ERR(sensor->reset_gpio))
> +               return dev_err_probe(dev, PTR_ERR(sensor->reset_gpio),
> +                                    "Failed to get reset gpio");

I think needn't fallback here, NO ABI change, just change to use reset-gpio
driver.

> +
> +       return 0;
> +}
> +
> +static int ov5640_reset_assert(struct ov5640_dev *sensor)
> +{
> +       if (sensor->reset)
> +               return reset_control_assert(sensor->reset);

needn't check sensor->reset, reset_control_assert() is no ops if NULL.

> +
> +       gpiod_set_value_cansleep(sensor->reset_gpio, 1);

Needn't fallback, directly replace.

Frank

> +       return 0;
> +}
> +
> +static int ov5640_reset_deassert(struct ov5640_dev *sensor)
> +{
> +       if (sensor->reset)
> +               return reset_control_deassert(sensor->reset);
> +
> +       gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> +       return 0;
> +}
> +
>  static void ov5640_power(struct ov5640_dev *sensor, bool enable)
>  {
>         gpiod_set_value_cansleep(sensor->pwdn_gpio, enable ? 0 : 1);
> @@ -2448,12 +2492,19 @@ 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.
> + * reset handling (via reset controller or GPIO) will be ignored.
>   */
> -static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
> +static int ov5640_powerup_sequence(struct ov5640_dev *sensor)
>  {
> +       int ret;
> +
>         if (sensor->pwdn_gpio) {
> -               gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> +               ret = ov5640_reset_assert(sensor);
> +               if (ret) {
> +                       dev_err(&sensor->i2c_client->dev,
> +                               "Failed to assert reset: %d\n", ret);
> +                       return ret;
> +               }
>
>                 /* camera power cycle */
>                 ov5640_power(sensor, false);
> @@ -2461,7 +2512,13 @@ 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);
> +               ret = ov5640_reset_deassert(sensor);
> +               if (ret) {
> +                       dev_err(&sensor->i2c_client->dev,
> +                               "Failed to deassert reset: %d\n", ret);
> +                       ov5640_power(sensor, false);
> +                       return ret;
> +               }
>         } else {
>                 /* software reset */
>                 ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> @@ -2475,6 +2532,8 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
>          */
>         ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
>                          OV5640_REG_SYS_CTRL0_SW_PWDN);
> +
> +       return 0;
>  }
>
>  static int ov5640_set_power_on(struct ov5640_dev *sensor)
> @@ -2497,7 +2556,9 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor)
>                 goto xclk_off;
>         }
>
> -       ov5640_powerup_sequence(sensor);
> +       ret = ov5640_powerup_sequence(sensor);
> +       if (ret)
> +               goto regulator_off;
>
>         ret = ov5640_init_slave_id(sensor);
>         if (ret)
> @@ -2507,6 +2568,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor)
>
>  power_off:
>         ov5640_power(sensor, false);
> +regulator_off:
>         regulator_bulk_disable(OV5640_NUM_SUPPLIES, sensor->supplies);
>  xclk_off:
>         clk_disable_unprepare(sensor->xclk);
> @@ -3914,11 +3976,9 @@ 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);
> +       ret = ov5640_get_reset(dev, sensor);
> +       if (ret)
> +               return ret;
>
>         v4l2_i2c_subdev_init(&sensor->sd, client, &ov5640_subdev_ops);
>         sensor->sd.internal_ops = &ov5640_internal_ops;
> --
> 2.50.1
>
>