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@xxxxxxxxxxxxxxx/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
