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

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

On Mon, Jan 20, 2025 at 06:05:05PM +0200, Laurent Pinchart wrote:
> On Mon, Jan 20, 2025 at 02:30:46PM +0100, Hans de Goede wrote:
> > On 20-Jan-25 10:39 AM, Sakari Ailus wrote:
> > > On Thu, Jan 16, 2025 at 04:48:12PM +0200, Andy Shevchenko wrote:
> > >> 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.
> > > 
> > > Thanks for the ping.
> > > 
> > > I took a closer look at the problem, indeed the GPIO name comes from the
> > > int3472 driver and it's much better to work around this there than to ram
> > > Windows ACPI table specifics to sensor drivers. I'll post a patch for that
> > > shortly. Testing would be appreciated as the Surface Go 2 doesn't have a
> > > GPIO connected to this sensor.
> > 
> > Actually Andy wrote this patch after first writing a similar quirk patch
> > as yours from:
> > 
> > https://lore.kernel.org/platform-driver-x86/1cf93f61-9285-f2fe-fb92-8fb8a3f44201@linux.intel.com/T/#t
> > 
> > because I asked him to fix this in the sensor driver instead.
> > 
> > The problem is that we cannot fix this in the INT3472 driver without
> > that becoming one big mess of driver specific quirks.
> > 
> > The Windows code seems to have separate power-ctrl/sequence drivers
> > for the INT3472 device vs the rest of the sensor driver and this
> > power-sequence driver happily consumes whatever GPIOs the INT3472
> > device provides independent of the sensor.
> > 
> > So e.g. on some designs we are going to see a reset pin and on others
> > a powerdown pin.
> > 
> > But in this specific case things are actually more simple,
> > the datasheet describes the pin as:
> > 
> > XSHUTDOWN input reset (active low with internal pull down resistor)
> > 
> > So the devicetree binding calling it "enable" is just wrong and in
> > this case it is actually the DT binding which is buggy (there is no
> > "enable" pin anywhere in the datasheet) and not the ACPI tables.
> 
> There has never been a requirement for the GPIO in DT to match the pin
> name from the datasheet. It's actually quite the contrary, DT
> maintainers request GPIO names to be standardized. Using
> "xshutdown-gpios" wouldn't be appreciated, instead DT bindings are
> encouraged to use standard names such as "reset", "enable" and
> "powerdown". As the pin is called "XSHUTDOWN", "enable" is not
> necessarily a bad match. "reset" could have been picked too, but that
> wasn't the case and we can't change that.

This is very strange. I always thought that DT is about describing _hardware_
and not some abstractions like Linux ones. The HW is pretty much clear that
the pin is "reset" by semantics (if you want standardization), and not
"enable".

> The core of the issue here is that we have two firmware representations,
> DT and ACPI, and no coordination between the two. I don't expect that to
> change, and until Windows OEMs collaborate to standardize ACPI bindings,
> I'll consider this as a Windows-specific hack that I don't want to see
> being pushed to indidividual sensor drivers. This would be better
> handled in my opinion in the INT3472 driver.

Yeah and to me it's clearly the bug in DT schema for this pin. It had to be
"reset" from day 1 in accordance with datasheet.

> > Also since you match on the sensor-type for the remapping this clearly
> > is a per sensor thing and per sensor handling should be in the sensor
> > drivers, which gives us the per sensor mapping for free.
> > 
> > Yes needing to handle this in the sensor driver makes the sensor
> > driver slightly less pretty. But dealing with platform specifics
> > in drivers is done all over the kernel and I don't see why camera
> > sensor drivers are so special that they get to be excluded from
> > sometimes needing to deal with platform specifics.
> 
> The whole point of having standardized firmware descriptions is to avoid
> board-specific code in drivers. INT3472 is meant to handle board
> specificities.

Yes, but world is not ideal and people made mistakes...

-- 
With Best Regards,
Andy Shevchenko