Re: [PATCH] media: imx335: Fix reset-gpio handling

[Jacopo Mondi]

Hi Umang

On Mon, Jul 29, 2024 at 02:18:20PM GMT, Umang Jain wrote:
> Hi Jacopo
>
> On 29/07/24 1:42 pm, Jacopo Mondi wrote:
> > Hi Umang
> >
> > On Mon, Jul 29, 2024 at 11:35:35AM GMT, Umang Jain wrote:
> > > The imx335 reset-gpio is initialised with GPIO_OUT_LOW during probe.
> > How is this related to this change ? The value to which the GPIO is
> > initialized to in probe is the physical level.
> >
> > What matters is the gpio line active level, which should be described
> > in the sensor's datasheet. What's the active level of the reset gpio
> > line ?
>
> The XCLR active level is "Low" at the init time. It is set to "high" during
> power-on / normal operation
>

Sorry for not being clear, but the physical active level is a property
of the chip, and doesn't depend on the setting at init time made by
the driver.

According to the imx335 datasheet, the XCLR pin is said to be:
High: normal
Low: clear

Which I presume means a physical low level puts the chip in "reset"
state.

> >
> > > However, the reset-gpio logical value is set to 1 in during power-on
> > > and to 0 on power-off. This is incorrect as the reset line
> > > cannot be high during power-on and low during power-off.
> > If the line is physically high or low only depends on how the active
> > level is specified in DTS, not by the logical value provided to
> > gpiod_set_value[_cansleep]()
>
> True.
>
> AS far as I can see, the DT binding schema specifies 'reset-gpios:' -
> without the active level
>
> The active level is I suppose, intentionally left to the DT implementation ?

Not really a decision of the DT implementation, but rather a property
of the chip, so I guess the line should be described as active low in
bindings and initialized accordingly in DTS with the GPIO_ACTIVE_LOW
flag.

>
> > > Rectify the logical value of reset-gpio so that it is set to
> > > 0 during power-on and to 1 during power-off.
> > This is correct, the reset line should be set to logical 0 (inactive)
> > during power on and to logical 1 (active) when powering off. However
> > the GPIO active state should have been specified in bindings and as
> > this driver has been mainline quite some time, this change will break
> > .dtbo already used succesfully with previous kernel releases.
> >
> > Is this an issue ?
>
> Yes, if the patch is accepted, the Device-tree implementation for IMX335
> will need to be adjusted accordingly. This can be an issue definitely - but
> on the other hand, this attempts to rectify a mistake, no?
>

Indeed it does rectify a mistake, but I presume existing dtbos have the
gpio line described with GPIO_ACTIVE_HIGH, otherwise they wouldn't
work with the existing driver version. Now, you change (or rather,
fix) the driver, and existing dtbos in the wild (iow not in the
mainline code base (*)) won't work anymore. The
expectation is that we don't break working dtbos with new kernel
releases, however I'm not sure how much this is actually enforced.
I'll defer this call to maintainers.

In case it is fine to break existing dtbos, I think a patch to the
bindings to specify the gpio line active level would be required too ?

Thanks
  j

(*) as far as I can tell no dts in mainline uses imx335.

> >
> > > Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/media/i2c/imx335.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > > index cd150606a8a9..878d88b5f476 100644
> > > --- a/drivers/media/i2c/imx335.c
> > > +++ b/drivers/media/i2c/imx335.c
> > > @@ -1171,7 +1171,7 @@ static int imx335_power_on(struct device *dev)
> > >   	usleep_range(500, 550); /* Tlow */
> > >
> > >   	/* Set XCLR */
> > > -	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
> > > +	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> > >
> > >   	ret = clk_prepare_enable(imx335->inclk);
> > >   	if (ret) {
> > > @@ -1184,7 +1184,7 @@ static int imx335_power_on(struct device *dev)
> > >   	return 0;
> > >
> > >   error_reset:
> > > -	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> > > +	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
> > >   	regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
> > >
> > >   	return ret;
> > > @@ -1201,7 +1201,7 @@ static int imx335_power_off(struct device *dev)
> > >   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > >   	struct imx335 *imx335 = to_imx335(sd);
> > >
> > > -	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> > > +	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
> > >   	clk_disable_unprepare(imx335->inclk);
> > >   	regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
> > >
> > > --
> > > 2.45.0
> > >
> > >
>