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

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

[Umang Jain]

The imx335 reset-gpio is initialised with GPIO_OUT_LOW during probe.
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.

Rectify the logical value of reset-gpio so that it is set to
0 during power-on and to 1 during power-off.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 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

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

[Sakari Ailus]

Hi Umang,

Thanks for the patch.

On Mon, Jul 29, 2024 at 11:35:35AM +0530, Umang Jain wrote:
> The imx335 reset-gpio is initialised with GPIO_OUT_LOW during probe.

Should it be initialised to high instead, to enable reset?

I think you should also add a Fixes: tag to this and Cc: stable.

> 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.
> 
> Rectify the logical value of reset-gpio so that it is set to
> 0 during power-on and to 1 during power-off.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

-- 
Kind regards,

Sakari Ailus

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

[Umang Jain]

Hi Sakari

On 29/07/24 1:11 pm, Sakari Ailus wrote:
> Hi Umang,
>
> Thanks for the patch.
>
> On Mon, Jul 29, 2024 at 11:35:35AM +0530, Umang Jain wrote:
>> The imx335 reset-gpio is initialised with GPIO_OUT_LOW during probe.
> Should it be initialised to high instead, to enable reset?

This initialization matches the physical line status, which is low in 
this case.

> I think you should also add a Fixes: tag to this and Cc: stable.

ack
>> 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.
>>
>> Rectify the logical value of reset-gpio so that it is set to
>> 0 during power-on and to 1 during power-off.
>>
>> Signed-off-by: Umang Jain<umang.jain@ideasonboard.com>

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

[Sakari Ailus]

Hi Umang,

On Mon, Jul 29, 2024 at 02:19:32PM +0530, Umang Jain wrote:
> Hi Sakari
> 
> On 29/07/24 1:11 pm, Sakari Ailus wrote:
> > Hi Umang,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Jul 29, 2024 at 11:35:35AM +0530, Umang Jain wrote:
> > > The imx335 reset-gpio is initialised with GPIO_OUT_LOW during probe.
> > Should it be initialised to high instead, to enable reset?
> 
> This initialization matches the physical line status, which is low in this
> case.

Documentation/driver-api/gpio/consumer.rst:

	* GPIOD_OUT_LOW to initialize the GPIO as output with a value of 0.

...

	Note that the initial value is *logical* and the physical line
	level depends on whether the line is configured active high or
	active low (see :ref:`active_low_semantics`).

-- 
Sakari Ailus

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

[Kieran Bingham]

Quoting Sakari Ailus (2024-07-29 09:54:01)
> Hi Umang,
> 
> On Mon, Jul 29, 2024 at 02:19:32PM +0530, Umang Jain wrote:
> > Hi Sakari
> > 
> > On 29/07/24 1:11 pm, Sakari Ailus wrote:
> > > Hi Umang,
> > > 
> > > Thanks for the patch.
> > > 
> > > On Mon, Jul 29, 2024 at 11:35:35AM +0530, Umang Jain wrote:
> > > > The imx335 reset-gpio is initialised with GPIO_OUT_LOW during probe.
> > > Should it be initialised to high instead, to enable reset?
> > 
> > This initialization matches the physical line status, which is low in this
> > case.
> 
> Documentation/driver-api/gpio/consumer.rst:
> 
>         * GPIOD_OUT_LOW to initialize the GPIO as output with a value of 0.
> 
> ...
> 
>         Note that the initial value is *logical* and the physical line
>         level depends on whether the line is configured active high or
>         active low (see :ref:`active_low_semantics`).
> 

Yes, I think this patch should also update/fix the call in
imx335_parse_hw_config()

	/* Request optional reset pin */
	imx335->reset_gpio = devm_gpiod_get_optional(imx335->dev, "reset",
-						     GPIOD_OUT_LOW);
+						     GPIOD_OUT_HIGH);

To make sure it starts off in reset until it's set accordingly in
imx335_power_{on,off}()

--
Kieran


> -- 
> Sakari Ailus

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

[Jacopo Mondi]

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 ?

> 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]()
>
> 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 ?

>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  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
>
>

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

[Umang Jain]

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

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

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

>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   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
>>
>>

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@ideasonboard.com>
> > > ---
> > >   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
> > >
> > >
>