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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4me3tw572feft3x4dn3ritpr6avss6ebupixrg7qrlsy5z6kny@mqeoqhr7uh2x>
Date: Mon, 29 Jul 2024 10:12:43 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Umang Jain <umang.jain@...asonboard.com>
Cc: linux-media@...r.kernel.org, 
	Kieran Bingham <kieran.bingham@...asonboard.com>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	open list <linux-kernel@...r.kernel.org>, Tommaso Merciai <tomm.merciai@...il.com>
Subject: Re: [PATCH] media: imx335: Fix reset-gpio handling

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@...asonboard.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
>
>