Re: [PATCH v1 1/1] media: ov7251: Remap "reset" to "enable" for OV7251

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11-Nov-24 8:55 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 8-Nov-24 5:42 PM, Sakari Ailus wrote:
>>> Hi Andy,
>>>
>>> On Fri, Nov 08, 2024 at 06:28:05PM +0200, Andy Shevchenko wrote:
>>>> On Fri, Nov 08, 2024 at 04:06:39PM +0000, Sakari Ailus wrote:
>>>>> On Fri, Nov 08, 2024 at 04:50:24PM +0200, Andy Shevchenko wrote:
>>>>>> The driver of OmniVision OV7251 expects "enable" pin instead of "reset".
>>>>>> Remap "reset" to "enable" and update polarity.
>>>>>>
>>>>>> In particular, the Linux kernel can't load the camera sensor
>>>>>> driver on Microsoft Surface Book without this change:
>>>>>>
>>>>>>  ov7251 i2c-INT347E:00: supply vdddo not found, using dummy regulator
>>>>>>  ov7251 i2c-INT347E:00: supply vddd not found, using dummy regulator
>>>>>>  ov7251 i2c-INT347E:00: supply vdda not found, using dummy regulator
>>>>>>  ov7251 i2c-INT347E:00: cannot get enable gpio
>>>>>>  ov7251 i2c-INT347E:00: probe with driver ov7251 failed with error -2
>>>>
>>>> ...
>>>>
>>>>> Should this be cc'd to stable? I guess it's not exactly a fix in the driver
>>>>> but a BIOS bug, but it can be worked around in the driver. :-)
>>>>
>>>> It's everything, but a BIOS bug, it's DT bug and whoever first introduced that
>>>> GPIO in the driver. Even in the DT present in kernel the pin was referred as
>>>
>>> How is that a DT (binding?) bug?
>>
>> Since it is not following the datasheet name for the pin,
>> it arguably is a DT binding bug
>>
>> But whatever, the whole discussion about if it is a bug and whose
>> bug it is is not useful. Since we cannot go back in time and change
>> the DT binding DT and ACPI are simply going to disagree on the name
>> and we will need something like this patch.
>>
>>>> CAM_RST_N, which is exactly how this patch names it.
>>>>
>>>> OTOH it's a fix to the driver that never worked for ACPI case, so there never
>>>> was a regression to fix.
>>>
>>> It probably worked just fine, just not with that Surface Book.
>>>
>>> The polarity of the enable gpio appears to be set wrong in devm_gpiod_get()
>>> call. I can post a patch but cannot test it.
>>
>> That is on purpose, at least the polarity if the devm_gpiod_get(..., "reset",
>> ...) is inverted from the existing one for "enable" because reset needs
>> to be inactive/disabled to enable the sensor.
>>
>>> Similarly, you should actually set the flags to GPIOD_OUT_HIGH as reset
>>> should be enabled here -- it's disabled only in power_on() as part of the
>>> power-on sequence.
>>
>> This seems to be a pre-existing bug in this driver, which currently
>> starts driving enable high, enabling the sensor at gpiod_get() time.
>>
>> Note that fixing this is tricky-ish, if the pin was already high at
>> gpiod_get() time then changing the gpiod_get() to drive it low
>> will result in it only being driven low for a very short time since
>> ov7251_set_power_on() will get called almost immediately after this
>> and it will drive the pin high again without any delays.
> 
> The question here is not about how long the hard reset is applied, but
> whether or not the sensor's power-on sequence is followed. Currently it is
> not.

Right / agreed. The 2 points which I am trying to make are:

1. This is a pre-existing problem unrelated to this patch.

So this should be fixed in a separate patch.

2. That separate patch should put a delay after requesting the GPIO
to enforce that it is (logically) low (for "enable") for a minimum
amount of time.

Regards,

Hans