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

[Frank Li]

On Mon, Jun 22, 2026 at 11:05:34AM +0200, Philipp Zabel wrote:
> On Fr, 2026-06-19 at 09:18 -0500, Frank Li wrote:
> > On Fri, Jun 19, 2026 at 06:05:32PM +0800, robby.cai@xxxxxxxxxxx wrote:
> > > [You don't often get email from robby.cai@xxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > >
> > > From: Robby Cai <robby.cai@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > > ---
> > >  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.

Maybe try promote reset-gpio is default build in / as module. Leave such
fallback every where make code complex.

Krysztof:
	Any suggestion?

Frank

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