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

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

Hi

On Fri, 26 Sept 2025 at 08:34, Loic Poulain
<loic.poulain@oss.qualcomm.com> 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.
>
> This breaks backward compatibility for any downstream dts using the
> wrong polarity.

Ouch. That'll be a nasty surprise to some if this lands, and
particularly with a Fixes: tag so it gets backported to stable
kernels.

There are a number of sensor drivers that have been in the tree for
multiple years that do indeed have the reset logic inverted as it
wasn't something being routinely picked up in code review. ov9282,
imx219, imx334, and imx274 for a start. Krzysztof sent [1] recently to
flag that they are wrong and shouldn't be copied, but changing the
behaviour feels unfriendly.

I'll defer to Sakari as to whether this change is acceptable.

Checking all my use cases, they use regulators instead of reset-gpio,
so it doesn't actually have an impact.

  Dave

[1] https://lore.kernel.org/linux-media/20250818150025.247209-2-krzysztof.kozlowski@linaro.org/

> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
>  drivers/media/i2c/ov9282.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index c882a021cf18..fb6fcba503c8 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -1127,7 +1127,7 @@ 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));
> @@ -1238,7 +1238,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, 0);
>
>         ret = clk_prepare_enable(ov9282->inclk);
>         if (ret) {
> @@ -1261,7 +1261,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, 1);
>
>         regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
>
> @@ -1279,7 +1279,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, 1);
>
>         clk_disable_unprepare(ov9282->inclk);
>
> --
> 2.34.1
>