Re: [PATCH v3 1/4] media: i2c: ov9282: Fix reset-gpio logical state

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Loic,

On Fri, Nov 14, 2025 at 02:38:19PM +0100, Loic Poulain wrote:
> Ensure reset state is low in the power-on state and high in the
> power-off state (assert reset). Note that the polarity is abstracted
> by the GPIO subsystem, so the logic level reflects the intended reset
> behavior.

That's an interesting approach to fix DTS gone systematically wrong.

I was thinking of the drivers that have this issue, too, but I would have
introduced a new GPIO under a different name (many sensors use "enable",
too). Any thoughts?

Cc Laurent.

> 
> To maintain backward compatibility with DTS files that use an incorrect
> flag, we implement a mechanism similar to:
>   commit 738455858a2d ("ASoC: codecs: wsa881x: Use proper shutdown GPIO polarity")
> 
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
>  drivers/media/i2c/ov9282.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index a9f6176e9729..e79b326cdd94 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -160,6 +160,7 @@ struct ov9282_mode {
>   * @sd: V4L2 sub-device
>   * @pad: Media pad. Only one pad supported
>   * @reset_gpio: Sensor reset gpio
> + * @reset_gpio_val: Logical value to reset the sensor
>   * @inclk: Sensor input clock
>   * @supplies: Regulator supplies for the sensor
>   * @ctrl_handler: V4L2 control handler
> @@ -180,6 +181,7 @@ struct ov9282 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  	struct gpio_desc *reset_gpio;
> +	unsigned int reset_gpio_val;
>  	struct clk *inclk;
>  	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
>  	struct v4l2_ctrl_handler ctrl_handler;
> @@ -1127,13 +1129,29 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
>  
>  	/* Request optional reset pin */
>  	ov9282->reset_gpio = devm_gpiod_get_optional(ov9282->dev, "reset",
> -						     GPIOD_OUT_LOW);
> +						     GPIOD_OUT_HIGH);
>  	if (IS_ERR(ov9282->reset_gpio)) {
>  		dev_err(ov9282->dev, "failed to get reset gpio %ld",
>  			PTR_ERR(ov9282->reset_gpio));
>  		return PTR_ERR(ov9282->reset_gpio);
>  	}
>  
> +	/*
> +	 * Backwards compatibility work-around.
> +	 *
> +	 * The reset GPIO is active-low, but the driver has always used the
> +	 * gpiod API with inverted logic. As a result, the DTS had to
> +	 * incorrectly mark the GPIO as active-high to compensate for this
> +	 * behavior. Changing the flag in the driver now would break backward
> +	 * compatibility with existing DTS configurations. To address this,
> +	 * we add a simple value inversion so the driver works with both old
> +	 * and new DTS.
> +	 */
> +	ov9282->reset_gpio_val = gpiod_is_active_low(ov9282->reset_gpio);
> +	if (!ov9282->reset_gpio_val)
> +		dev_warn(ov9282->dev, "Using ACTIVE_HIGH for reset GPIO. Your DTB might be outdated\n");
> +	gpiod_set_value_cansleep(ov9282->reset_gpio, ov9282->reset_gpio_val);
> +
>  	/* Get sensor input clock */
>  	ov9282->inclk = devm_v4l2_sensor_clk_get(ov9282->dev, NULL);
>  	if (IS_ERR(ov9282->inclk))
> @@ -1237,7 +1255,7 @@ static int ov9282_power_on(struct device *dev)
>  
>  	usleep_range(400, 600);
>  
> -	gpiod_set_value_cansleep(ov9282->reset_gpio, 1);
> +	gpiod_set_value_cansleep(ov9282->reset_gpio, !ov9282->reset_gpio_val);
>  
>  	ret = clk_prepare_enable(ov9282->inclk);
>  	if (ret) {
> @@ -1260,7 +1278,7 @@ static int ov9282_power_on(struct device *dev)
>  error_clk:
>  	clk_disable_unprepare(ov9282->inclk);
>  error_reset:
> -	gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
> +	gpiod_set_value_cansleep(ov9282->reset_gpio, ov9282->reset_gpio_val);
>  
>  	regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
>  
> @@ -1278,7 +1296,7 @@ static int ov9282_power_off(struct device *dev)
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct ov9282 *ov9282 = to_ov9282(sd);
>  
> -	gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
> +	gpiod_set_value_cansleep(ov9282->reset_gpio, ov9282->reset_gpio_val);
>  
>  	clk_disable_unprepare(ov9282->inclk);
>  

-- 
Kind regards,

Sakari Ailus