Re: [PATCH] platform/x86: int3472: handle GPIO type 0x08 as a power regulator
Jakob Berg Jespersen <[email protected]>
| Newsgroups | org.kernel.vger.platform-driver-x86,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <wWCJgdk1-0MrfeN6bRValpATGnQ4BcEzShHI5qI7Nywi6LWcYiq9BDKaaaH5Y0SmHJl2IlGNepOmbjVkwr0APh4dA1fzX7M9VCWZM6XR_-w=@berg.pm> |
On Monday, July 20th, 2026 at 12:14, Hans de Goede <[email protected]> wrote: > Hi, > > On 20-Jul-26 00:00, Sakari Ailus wrote: > > Hi Jakob, > > > > Thanks for the patch. > > > > On Sun, Jul 19, 2026 at 08:15:12PM +0200, Jakob Berg Jespersen wrote: > >> On the Microsoft Surface Pro 7+, the rear camera's INT3472 device > >> (INT3472:01, sensor INT347A / ov8865) enumerates a GPIO of type 0x08 > >> in its _DSM which the driver does not recognise, so it is left > >> unmapped: > >> > >> int3472-discrete INT3472:01: GPIO type 0x08 unknown; the sensor may > >> not work > >> > >> The DSDT shows this pin (0x0100AF08, pin 175, active-high) sitting > >> alongside a normal type-0x0b power-enable GPIO: it gates a second > >> sensor power rail. Because the type is unknown the rail is never > >> enabled, the ov8865's "dvdd" supply resolves to a dummy regulator, and > >> the sensor never probes. > >> > >> Map type 0x08 for INT347A to a POWER_ENABLE regulator with con_id > >> "dvdd", the supply the in-tree ov8865 driver already requests, so no > >> sensor-driver change is needed. ("dvdd" rather than "avdd" because the > >> type-0x0b power-enable pin on this INT3472 is already registered as > >> "avdd".) > >> > >> Type 0x08 is not in the driver's documented function list and is not > >> otherwise handled by mainline; based on the sensor's supply set and the > >> con_id that brings it up, it appears to be the DVDD (digital core > >> voltage) rail, the counterpart to the existing DOVDD (0x10) type. > >> > >> Related out-of-tree work approached the same rail by adding a new "pwr1" > >> supply instead of the sensor's existing "dvdd" (linux-surface PR #1867 > >> for the Surface Pro 9, PR #2201 for the Pro 7+ ov8865); neither was sent > >> upstream. Mapping to "dvdd" keeps the change contained to int3472. > >> > >> With this change the SP7+ rear camera probes and streams reliably, > >> validated by over a month of use. > >> > >> Link: https://github.com/linux-surface/linux-surface/pull/1867 > >> Link: https://github.com/linux-surface/linux-surface/pull/2201 > >> Cc: Tooraj Taraz <[email protected]> > >> Cc: Joseph V. Lavigne <[email protected]> > >> Signed-off-by: Jakob Berg Jespersen <[email protected]> > >> --- > >> 0x08 is left as a bare literal rather than a named constant here; if > >> its canonical meaning is known I'm happy to add e.g. > >> INT3472_GPIO_TYPE_DVDD and/or handle it generically like DOVDD in v2. > >> > >> This is my first kernel contribution, so please bear with me on any > >> process mistakes. > >> --- > >> drivers/platform/x86/intel/int3472/discrete.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > >> index 115bb37577a1..98c7b9bc393f 100644 > >> --- a/drivers/platform/x86/intel/int3472/discrete.c > >> +++ b/drivers/platform/x86/intel/int3472/discrete.c > >> @@ -164,6 +164,19 @@ static const struct int3472_gpio_map int3472_gpio_map[] = { > >> .con_id = "dvdd", > >> .enable_time_us = 45 * USEC_PER_MSEC, > >> }, > >> + { /* > >> + * Surface Pro 7+ ov8865 (rear camera): type 0x08 is an > >> + * undocumented sensor power rail; left undriven the sensor > >> + * stays in a failed power state. "dvdd" rather than "avdd" > >> + * because this INT3472 also has a type 0x0b (power enable) > >> + * pin already registered as "avdd". > >> + */ > > > > GPIO type 8 is in fact "POWER1" (type 7 being "POWER0"). This hasn't been > > seen anywhere yet so support for it hasn't been added either. > > > > I understand the POWER_ENABLE GPIO is for executing the entire power > > sequence and isn't expected to be found with the other regulators, hence an > > apparent conflict here wouldn't be an issue. > > > > I think for now we could map type POWER1 (8) to "dvdd", however not using a > > quirk but doing it for all devices, like mapping POWER_ENABLE to "avdd". > > That'd leave mapping POWER0 to "avdd" for later. The GPIO definition for > > POWER0 could be added in any case. > > I was wondering what the internal Intel headers said for type 8, so thank > you for clarifying that that is called POWER1. Adding a generic mapping > of POWER1 to "dvdd" instead of using a quirk sounds good to me. > > Regards, > > Hans > > Thanks both, and thanks for the POWER0/POWER1 naming. That approach makes sense to me. For v2 I'll: * add INT3472_GPIO_TYPE_POWER0 (7) and INT3472_GPIO_TYPE_POWER1 (8) definitions, * map POWER1 to "dvdd" generically in int3472_get_con_id_and_polarity(), dropping the INT347A quirk, * leave POWER0 defined but unmapped for now. I'll retest on the Surface Pro 7+ and send v2, with a Suggested-by: trailer for you, Sakari, for the generic approach. Thanks for the quick review. Jakob