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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/12/2025 11:00, Loic Poulain wrote:
> Hi Laurent,
> 
> On Mon, Nov 17, 2025 at 6:30 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
>>
>> 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.
> 
> Do you have any feedback on this change?
> 
>>
>>>
>>> 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.

This obviously is not true - driver will fail to work with some of old DTS.

I am surprised that it is second approach last weeks duplicating the
same problem and claiming the same - the change is backwards compatible,
while it is not - instead of taking my old explanation from WSA drivers
and admit the actual case of broken DTS.

With proper description I could agree, but with incorrect claims - no,
it's wrong.

Best regards,
Krzysztof