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

[Philipp Zabel <p.zabel@pengutronix.de>]

On Fr, 2026-06-19 at 09:18 -0500, Frank Li wrote:
> 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.

Please keep the gpiod fallback, the reset-gpio driver may not be
available on all platforms using ov5640.

> > +
> > +       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.

See above.


regards
Philipp