| Message ID | 20240729060535.3227-1-umang.jain@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: imx335: Fix reset-gpio handling | expand |
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>
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 > >
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 >> >>
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>
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`).
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 > > > > > > >
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
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);
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(-)