Hi Dan, On 3/16/23 15:59, Hans de Goede wrote: > Hi Daniel, > > On 3/11/23 23:30, Daniel Scally wrote: >> ACPI _HID INT347E represents the OmniVision 7251 camera sensor. The >> driver for this sensor expects a single pin named "enable", but on >> some Microsoft Surface platforms the sensor is assigned a single >> GPIO who's type flag is INT3472_GPIO_TYPE_RESET. >> >> Remap the GPIO pin's function from "reset" to "enable". This is done >> outside of the existing remap table since it is a more widespread >> discrepancy than that method is designed for. Additionally swap the >> polarity of the pin to match the driver's expectation. >> >> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx> > > I have recently been looking into GPIO mappings on various devices > using the atomisp2 ISP. These use the same _DSM as the IPU3 but > then directly on the sensor ACPI device node. > > What I came up with is this (I still need to submit this upstream): > > https://github.com/jwrdegoede/linux-sunxi/commit/e2287979db43d46fa7d354c1bde92eb6219b613d > > One thing which I learned while working on this is that in the value returned by the _DSM > the byte with mask 0xff00 is the pin-number on the GPIO controller; and (and this is the important bit!) unlike the assumption in the IPU3/INT3472 code the order in which the GPIOs are listed in the "79234640-9e10-4fea-a5c1-b5aa8b19756f" _DSM is *NOT* always the order in which they are listed in the GPIO resources! > > So as you can see in the linked commit I'm finding the GPIO resource to go with the _DSM value by matching the pin-numbers. > > I'm wondering if we need to do the same thing on IPU3 and if this is maybe why we need to do any remapping at all ? > > Can you please check if there is not something like the above going on before we add a remap quirk for this ? So based on our recent related emails: https://lore.kernel.org/linux-media/7fc1a3fb-d300-de09-1e0d-606971560fc1@xxxxxxxxxx/ I have taken another look at this. The datasheet for the OV7251 is a bit ambiguous it names the pin as "XSHUTDOWN" but then describes that pin as: "reset (active low with internal pull down resistor)" so based on the longer description of the pin I believe it would be reasonable to patch the driver to try and get a "reset" con-id GPIO if getting an "enable" con-id pin fails. IMHO this would be preferably to adding more and more GPIO remap hacks to the INT3472 code. Regards, Hans > > Regards, > > Hans > > > > > > >> --- >> drivers/platform/x86/intel/int3472/discrete.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >> index f064da74f50a..2064b3bbe530 100644 >> --- a/drivers/platform/x86/intel/int3472/discrete.c >> +++ b/drivers/platform/x86/intel/int3472/discrete.c >> @@ -98,6 +98,9 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347 >> { >> const struct int3472_sensor_config *sensor_config; >> char *path = agpio->resource_source.string_ptr; >> + const struct acpi_device_id ov7251_ids[] = { >> + { "INT347E" }, >> + }; >> struct gpiod_lookup *table_entry; >> struct acpi_device *adev; >> acpi_handle handle; >> @@ -120,6 +123,17 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347 >> } >> } >> >> + /* >> + * In addition to the function remap table we need to bulk remap the >> + * "reset" GPIO for the OmniVision 7251 sensor, as the driver for that >> + * expects its only GPIO pin to be called "enable" (and to have the >> + * opposite polarity). >> + */ >> + if (!strcmp(func, "reset") && !acpi_match_device_ids(int3472->sensor, ov7251_ids)) { >> + func = "enable"; >> + polarity = GPIO_ACTIVE_HIGH; >> + } >> + >> /* Functions mapped to NULL should not be mapped to the sensor */ >> if (!func) >> return 0; >
