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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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