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