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
