| Message ID | 20230311223019.14123-1-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | platform/x86: int3472: Remap reset GPIO for INT347E | expand |
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@ideasonboard.com> 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 ? 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;
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@ideasonboard.com> > > 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@redhat.com/ 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; >
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;
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@ideasonboard.com> --- drivers/platform/x86/intel/int3472/discrete.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)