On Mon, Nov 11, 2024 at 10:46:32AM +0100, Hans de Goede wrote: > On 11-Nov-24 8:55 AM, Sakari Ailus wrote: > > On Fri, Nov 08, 2024 at 07:19:05PM +0100, Hans de Goede wrote: > >> On 8-Nov-24 5:42 PM, Sakari Ailus wrote: > >>> 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. Sakari, what's the status on this, please? We have non-working camera just because of this small patch is absent. -- With Best Regards, Andy Shevchenko
