| Message ID | 20250411082357.392713-1-dongcheng.yan@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] platform/x86: int3472: add hpd pin support | expand |
Hi, On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: > Typically HDMI to MIPI CSI-2 bridges have a pin to signal image data is > being received. On the host side this is wired to a GPIO for polling or > interrupts. This includes the Lontium HDMI to MIPI CSI-2 bridges > lt6911uxe and lt6911uxc. > > The GPIO "hpd" is used already by other HDMI to CSI-2 bridges, use it > here as well. > > Signed-off-by: Dongcheng Yan <dongcheng.yan@intel.com> > --- > drivers/platform/x86/intel/int3472/common.h | 1 + > drivers/platform/x86/intel/int3472/discrete.c | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h > index 145dec66df64..db4cd3720e24 100644 > --- a/drivers/platform/x86/intel/int3472/common.h > +++ b/drivers/platform/x86/intel/int3472/common.h > @@ -22,6 +22,7 @@ > #define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b > #define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c > #define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d > +#define INT3472_GPIO_TYPE_HOTPLUG_DETECT 0x13 > > #define INT3472_PDEV_MAX_NAME_LEN 23 > #define INT3472_MAX_SENSOR_GPIOS 3 > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index 30ff8f3ea1f5..28d41b1b3809 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -186,6 +186,10 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, > *con_id = "privacy-led"; > *gpio_flags = GPIO_ACTIVE_HIGH; > break; > + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: > + *con_id = "hpd"; > + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; This looks wrong, we really need to clearly provide a polarity here since the ACPI GPIO resources do not provide one. Regards, Hans > + break; > case INT3472_GPIO_TYPE_POWER_ENABLE: > *con_id = "power-enable"; > *gpio_flags = GPIO_ACTIVE_HIGH; > @@ -212,6 +216,7 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, > * 0x0b Power enable > * 0x0c Clock enable > * 0x0d Privacy LED > + * 0x13 Hotplug detect > * > * There are some known platform specific quirks where that does not quite > * hold up; for example where a pin with type 0x01 (Power down) is mapped to > @@ -281,6 +286,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > switch (type) { > case INT3472_GPIO_TYPE_RESET: > case INT3472_GPIO_TYPE_POWERDOWN: > + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: > ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags); > if (ret) > err_msg = "Failed to map GPIO pin to sensor\n"; > > base-commit: 01c6df60d5d4ae00cd5c1648818744838bba7763
Hi hans, On 4/11/2025 4:33 PM, Hans de Goede wrote: > Hi, > > On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: >> Typically HDMI to MIPI CSI-2 bridges have a pin to signal image data is >> being received. On the host side this is wired to a GPIO for polling or >> interrupts. This includes the Lontium HDMI to MIPI CSI-2 bridges >> lt6911uxe and lt6911uxc. >> >> The GPIO "hpd" is used already by other HDMI to CSI-2 bridges, use it >> here as well. >> >> Signed-off-by: Dongcheng Yan <dongcheng.yan@intel.com> >> --- >> drivers/platform/x86/intel/int3472/common.h | 1 + >> drivers/platform/x86/intel/int3472/discrete.c | 6 ++++++ >> 2 files changed, 7 insertions(+) >> >> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h >> index 145dec66df64..db4cd3720e24 100644 >> --- a/drivers/platform/x86/intel/int3472/common.h >> +++ b/drivers/platform/x86/intel/int3472/common.h >> @@ -22,6 +22,7 @@ >> #define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b >> #define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c >> #define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d >> +#define INT3472_GPIO_TYPE_HOTPLUG_DETECT 0x13 >> >> #define INT3472_PDEV_MAX_NAME_LEN 23 >> #define INT3472_MAX_SENSOR_GPIOS 3 >> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >> index 30ff8f3ea1f5..28d41b1b3809 100644 >> --- a/drivers/platform/x86/intel/int3472/discrete.c >> +++ b/drivers/platform/x86/intel/int3472/discrete.c >> @@ -186,6 +186,10 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, >> *con_id = "privacy-led"; >> *gpio_flags = GPIO_ACTIVE_HIGH; >> break; >> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: >> + *con_id = "hpd"; >> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; > > This looks wrong, we really need to clearly provide a polarity > here since the ACPI GPIO resources do not provide one. > I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe driver can pass the test and work normally. Is this the rule of int3472 driver? In addition, GPIO_LOOKUP_FLAGS_DEFAULT = GPIO_ACTIVE_HIGH | GPIO_PERSISTENT as defined, maybe it provides a polarity also. I can change to GPIO_ACTIVE_LOW, but I want to understand the reason. Best Regard, Dongcheng> Regards, > > Hans > > > >> + break; >> case INT3472_GPIO_TYPE_POWER_ENABLE: >> *con_id = "power-enable"; >> *gpio_flags = GPIO_ACTIVE_HIGH; >> @@ -212,6 +216,7 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, >> * 0x0b Power enable >> * 0x0c Clock enable >> * 0x0d Privacy LED >> + * 0x13 Hotplug detect >> * >> * There are some known platform specific quirks where that does not quite >> * hold up; for example where a pin with type 0x01 (Power down) is mapped to >> @@ -281,6 +286,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >> switch (type) { >> case INT3472_GPIO_TYPE_RESET: >> case INT3472_GPIO_TYPE_POWERDOWN: >> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: >> ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags); >> if (ret) >> err_msg = "Failed to map GPIO pin to sensor\n"; >> >> base-commit: 01c6df60d5d4ae00cd5c1648818744838bba7763 > >
On Mon, Apr 14, 2025 at 03:52:50PM +0800, Yan, Dongcheng wrote: > On 4/11/2025 4:33 PM, Hans de Goede wrote: > > On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: ... > >> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: > >> + *con_id = "hpd"; > >> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; > > > > This looks wrong, we really need to clearly provide a polarity > > here since the ACPI GPIO resources do not provide one. > > > I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe > driver can pass the test and work normally. I doubt you tested that correctly. It's impossible to have level triggered event to work with either polarity. It might be also a bug in the code lurking somewhere, but it would be unlikely (taking into account amount of systems relying on this). Is it edge triggered event?
Hi Andy, On 4/14/2025 4:11 PM, Andy Shevchenko wrote: > On Mon, Apr 14, 2025 at 03:52:50PM +0800, Yan, Dongcheng wrote: >> On 4/11/2025 4:33 PM, Hans de Goede wrote: >>> On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: > > ... > >>>> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: >>>> + *con_id = "hpd"; >>>> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; >>> >>> This looks wrong, we really need to clearly provide a polarity >>> here since the ACPI GPIO resources do not provide one. >>> >> I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe >> driver can pass the test and work normally. > > I doubt you tested that correctly. It's impossible to have level triggered > event to work with either polarity. It might be also a bug in the code lurking > somewhere, but it would be unlikely (taking into account amount of systems > relying on this). > > Is it edge triggered event? > It is an edge triggered event in lt6911uxe. In order to better adapt to other uses, "hpd" is meaningful to specify a polarity here. In lt6911uxe, GPIO "hpd" is used as irq, and set irq-flag to IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT. So no matter rising or falling, driver can work normally. " ret = request_threaded_irq(gpiod_to_irq(lt6911uxe->irq_gpio), NULL, lt6911uxe_threaded_irq_fn, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, NULL, lt6911uxe); " Thanks, Dongcheng
On Mon, Apr 14, 2025 at 04:40:26PM +0800, Yan, Dongcheng wrote: > On 4/14/2025 4:11 PM, Andy Shevchenko wrote: > > On Mon, Apr 14, 2025 at 03:52:50PM +0800, Yan, Dongcheng wrote: > >> On 4/11/2025 4:33 PM, Hans de Goede wrote: > >>> On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: ... > >>>> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: > >>>> + *con_id = "hpd"; > >>>> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; > >>> > >>> This looks wrong, we really need to clearly provide a polarity > >>> here since the ACPI GPIO resources do not provide one. > >>> > >> I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe > >> driver can pass the test and work normally. > > > > I doubt you tested that correctly. It's impossible to have level triggered > > event to work with either polarity. It might be also a bug in the code lurking > > somewhere, but it would be unlikely (taking into account amount of systems > > relying on this). > > > > Is it edge triggered event? > > > > It is an edge triggered event in lt6911uxe. In order to better adapt to > other uses, "hpd" is meaningful to specify a polarity here. > > In lt6911uxe, GPIO "hpd" is used as irq, and set irq-flag to > IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT. So no matter > rising or falling, driver can work normally. > " > ret = request_threaded_irq(gpiod_to_irq(lt6911uxe->irq_gpio), NULL, > lt6911uxe_threaded_irq_fn, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | > IRQF_ONESHOT, NULL, lt6911uxe); > " So, the driver must not override the firmware, if there is no bugs. So, why do you even use those flags there? It seems like a bad code in the driver that doesn't look correct to me.
Hi Dongcheng, On Mon, Apr 14, 2025 at 04:40:26PM +0800, Yan, Dongcheng wrote: > Hi Andy, > > On 4/14/2025 4:11 PM, Andy Shevchenko wrote: > > On Mon, Apr 14, 2025 at 03:52:50PM +0800, Yan, Dongcheng wrote: > >> On 4/11/2025 4:33 PM, Hans de Goede wrote: > >>> On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: > > > > ... > > > >>>> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: > >>>> + *con_id = "hpd"; > >>>> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; > >>> > >>> This looks wrong, we really need to clearly provide a polarity > >>> here since the ACPI GPIO resources do not provide one. > >>> > >> I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe > >> driver can pass the test and work normally. > > > > I doubt you tested that correctly. It's impossible to have level triggered > > event to work with either polarity. It might be also a bug in the code lurking > > somewhere, but it would be unlikely (taking into account amount of systems > > relying on this). > > > > Is it edge triggered event? > > > > It is an edge triggered event in lt6911uxe. In order to better adapt to > other uses, "hpd" is meaningful to specify a polarity here. > > In lt6911uxe, GPIO "hpd" is used as irq, and set irq-flag to > IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT. So no matter > rising or falling, driver can work normally. > " > ret = request_threaded_irq(gpiod_to_irq(lt6911uxe->irq_gpio), NULL, > lt6911uxe_threaded_irq_fn, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | > IRQF_ONESHOT, NULL, lt6911uxe); > " include/linux/gpio/machine.h: GPIO_PERSISTENT = (0 << 3), ... GPIO_LOOKUP_FLAGS_DEFAULT = GPIO_ACTIVE_HIGH | GPIO_PERSISTENT, I.e. you effectively have GPIO_ACTIVE_HIGH there. Btw. no need to cc this set to stable@vger.kernel.org, I've dropped it from the cc list. See Documentation/process/submitting-patches.rst .
Hi Andy and Hans, I found the description of lt6911uxe's GPIO in the spec: GPIO5 is used as the interrupt signal (50ms low level) to inform SOC start reading registers from 6911UXE; So setting the polarity as GPIO_ACTIVE_LOW is acceptable? We used RISING and FALLING in irq(not GPIO) to ensure that HDMI events will not be lost to the greatest extent possible. Thanks, Dongcheng On 4/14/2025 4:49 PM, Andy Shevchenko wrote: > On Mon, Apr 14, 2025 at 04:40:26PM +0800, Yan, Dongcheng wrote: >> On 4/14/2025 4:11 PM, Andy Shevchenko wrote: >>> On Mon, Apr 14, 2025 at 03:52:50PM +0800, Yan, Dongcheng wrote: >>>> On 4/11/2025 4:33 PM, Hans de Goede wrote: >>>>> On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: > > ... > >>>>>> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: >>>>>> + *con_id = "hpd"; >>>>>> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; >>>>> >>>>> This looks wrong, we really need to clearly provide a polarity >>>>> here since the ACPI GPIO resources do not provide one. >>>>> >>>> I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe >>>> driver can pass the test and work normally. >>> >>> I doubt you tested that correctly. It's impossible to have level triggered >>> event to work with either polarity. It might be also a bug in the code lurking >>> somewhere, but it would be unlikely (taking into account amount of systems >>> relying on this). >>> >>> Is it edge triggered event? >>> >> >> It is an edge triggered event in lt6911uxe. In order to better adapt to >> other uses, "hpd" is meaningful to specify a polarity here. >> >> In lt6911uxe, GPIO "hpd" is used as irq, and set irq-flag to >> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT. So no matter >> rising or falling, driver can work normally. >> " >> ret = request_threaded_irq(gpiod_to_irq(lt6911uxe->irq_gpio), NULL, >> lt6911uxe_threaded_irq_fn, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | >> IRQF_ONESHOT, NULL, lt6911uxe); >> " > > So, the driver must not override the firmware, if there is no bugs. > So, why do you even use those flags there? It seems like a bad code > in the driver that doesn't look correct to me. >
Hi Sakari, On 4/14/2025 4:52 PM, Sakari Ailus wrote: > Hi Dongcheng, > > On Mon, Apr 14, 2025 at 04:40:26PM +0800, Yan, Dongcheng wrote: >> Hi Andy, >> >> On 4/14/2025 4:11 PM, Andy Shevchenko wrote: >>> On Mon, Apr 14, 2025 at 03:52:50PM +0800, Yan, Dongcheng wrote: >>>> On 4/11/2025 4:33 PM, Hans de Goede wrote: >>>>> On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: >>> >>> ... >>> >>>>>> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: >>>>>> + *con_id = "hpd"; >>>>>> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; >>>>> >>>>> This looks wrong, we really need to clearly provide a polarity >>>>> here since the ACPI GPIO resources do not provide one. >>>>> >>>> I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe >>>> driver can pass the test and work normally. >>> >>> I doubt you tested that correctly. It's impossible to have level triggered >>> event to work with either polarity. It might be also a bug in the code lurking >>> somewhere, but it would be unlikely (taking into account amount of systems >>> relying on this). >>> >>> Is it edge triggered event? >>> >> >> It is an edge triggered event in lt6911uxe. In order to better adapt to >> other uses, "hpd" is meaningful to specify a polarity here. >> >> In lt6911uxe, GPIO "hpd" is used as irq, and set irq-flag to >> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT. So no matter >> rising or falling, driver can work normally. >> " >> ret = request_threaded_irq(gpiod_to_irq(lt6911uxe->irq_gpio), NULL, >> lt6911uxe_threaded_irq_fn, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | >> IRQF_ONESHOT, NULL, lt6911uxe); >> " > > include/linux/gpio/machine.h: > > GPIO_PERSISTENT = (0 << 3), > ... > GPIO_LOOKUP_FLAGS_DEFAULT = GPIO_ACTIVE_HIGH | GPIO_PERSISTENT, > > I.e. you effectively have GPIO_ACTIVE_HIGH there. > > Btw. no need to cc this set to stable@vger.kernel.org, I've dropped it from > the cc list. See Documentation/process/submitting-patches.rst . > Thanks, I will remove it in a subsequent email. Thanks, Dongcheng
Hi, On 14-Apr-25 11:59, Yan, Dongcheng wrote: > Hi Andy and Hans, > > I found the description of lt6911uxe's GPIO in the spec: > GPIO5 is used as the interrupt signal (50ms low level) to inform SOC > start reading registers from 6911UXE; > > So setting the polarity as GPIO_ACTIVE_LOW is acceptable? Yes that is acceptable, thank you for looking this up. Regards, Hans > We used RISING and FALLING in irq(not GPIO) to ensure that HDMI events > will not be lost to the greatest extent possible. > > Thanks, > Dongcheng > > On 4/14/2025 4:49 PM, Andy Shevchenko wrote: >> On Mon, Apr 14, 2025 at 04:40:26PM +0800, Yan, Dongcheng wrote: >>> On 4/14/2025 4:11 PM, Andy Shevchenko wrote: >>>> On Mon, Apr 14, 2025 at 03:52:50PM +0800, Yan, Dongcheng wrote: >>>>> On 4/11/2025 4:33 PM, Hans de Goede wrote: >>>>>> On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: >> >> ... >> >>>>>>> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: >>>>>>> + *con_id = "hpd"; >>>>>>> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; >>>>>> >>>>>> This looks wrong, we really need to clearly provide a polarity >>>>>> here since the ACPI GPIO resources do not provide one. >>>>>> >>>>> I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe >>>>> driver can pass the test and work normally. >>>> >>>> I doubt you tested that correctly. It's impossible to have level triggered >>>> event to work with either polarity. It might be also a bug in the code lurking >>>> somewhere, but it would be unlikely (taking into account amount of systems >>>> relying on this). >>>> >>>> Is it edge triggered event? >>>> >>> >>> It is an edge triggered event in lt6911uxe. In order to better adapt to >>> other uses, "hpd" is meaningful to specify a polarity here. >>> >>> In lt6911uxe, GPIO "hpd" is used as irq, and set irq-flag to >>> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT. So no matter >>> rising or falling, driver can work normally. >>> " >>> ret = request_threaded_irq(gpiod_to_irq(lt6911uxe->irq_gpio), NULL, >>> lt6911uxe_threaded_irq_fn, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | >>> IRQF_ONESHOT, NULL, lt6911uxe); >>> " >> >> So, the driver must not override the firmware, if there is no bugs. >> So, why do you even use those flags there? It seems like a bad code >> in the driver that doesn't look correct to me. >> >
Hi, On 14-Apr-25 13:04, Hans de Goede wrote: > Hi, > > On 14-Apr-25 11:59, Yan, Dongcheng wrote: >> Hi Andy and Hans, >> >> I found the description of lt6911uxe's GPIO in the spec: >> GPIO5 is used as the interrupt signal (50ms low level) to inform SOC >> start reading registers from 6911UXE; >> >> So setting the polarity as GPIO_ACTIVE_LOW is acceptable? > > Yes that is acceptable, thank you for looking this up. p.s. Note that setting GPIO_ACTIVE_LOW will invert the values returned by gpiod_get_value(), so if the driver uses that you will need to fix this in the driver. Hmm, thinking more about this, I just realized that this is an input pin to the CPU, not an output pin like all other pins described by the INT3472 device. I missed that as first. In that case using GPIO_LOOKUP_FLAGS_DEFAULT as before probably makes the most sense. Please add a comment that this is an input pin to the INT3472 patch and keep GPIO_LOOKUP_FLAGS_DEFAULT for the next version. Regards, Hans > > Regards, > > Hans > > > >> We used RISING and FALLING in irq(not GPIO) to ensure that HDMI events >> will not be lost to the greatest extent possible. >> >> Thanks, >> Dongcheng >> >> On 4/14/2025 4:49 PM, Andy Shevchenko wrote: >>> On Mon, Apr 14, 2025 at 04:40:26PM +0800, Yan, Dongcheng wrote: >>>> On 4/14/2025 4:11 PM, Andy Shevchenko wrote: >>>>> On Mon, Apr 14, 2025 at 03:52:50PM +0800, Yan, Dongcheng wrote: >>>>>> On 4/11/2025 4:33 PM, Hans de Goede wrote: >>>>>>> On 11-Apr-25 10:23 AM, Dongcheng Yan wrote: >>> >>> ... >>> >>>>>>>> + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: >>>>>>>> + *con_id = "hpd"; >>>>>>>> + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; >>>>>>> >>>>>>> This looks wrong, we really need to clearly provide a polarity >>>>>>> here since the ACPI GPIO resources do not provide one. >>>>>>> >>>>>> I tested gpio_flags=GPIO_LOOKUP_FLAGS_DEFAULT/HIGH/LOW, the lt6911uxe >>>>>> driver can pass the test and work normally. >>>>> >>>>> I doubt you tested that correctly. It's impossible to have level triggered >>>>> event to work with either polarity. It might be also a bug in the code lurking >>>>> somewhere, but it would be unlikely (taking into account amount of systems >>>>> relying on this). >>>>> >>>>> Is it edge triggered event? >>>>> >>>> >>>> It is an edge triggered event in lt6911uxe. In order to better adapt to >>>> other uses, "hpd" is meaningful to specify a polarity here. >>>> >>>> In lt6911uxe, GPIO "hpd" is used as irq, and set irq-flag to >>>> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT. So no matter >>>> rising or falling, driver can work normally. >>>> " >>>> ret = request_threaded_irq(gpiod_to_irq(lt6911uxe->irq_gpio), NULL, >>>> lt6911uxe_threaded_irq_fn, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | >>>> IRQF_ONESHOT, NULL, lt6911uxe); >>>> " >>> >>> So, the driver must not override the firmware, if there is no bugs. >>> So, why do you even use those flags there? It seems like a bad code >>> in the driver that doesn't look correct to me. >>> >> >
Hans, Dongcheng, On Mon, Apr 14, 2025 at 01:09:47PM +0200, Hans de Goede wrote: > Hi, > > On 14-Apr-25 13:04, Hans de Goede wrote: > > Hi, > > > > On 14-Apr-25 11:59, Yan, Dongcheng wrote: > >> Hi Andy and Hans, > >> > >> I found the description of lt6911uxe's GPIO in the spec: > >> GPIO5 is used as the interrupt signal (50ms low level) to inform SOC > >> start reading registers from 6911UXE; > >> > >> So setting the polarity as GPIO_ACTIVE_LOW is acceptable? > > > > Yes that is acceptable, thank you for looking this up. > > p.s. > > Note that setting GPIO_ACTIVE_LOW will invert the values returned > by gpiod_get_value(), so if the driver uses that you will need > to fix this in the driver. > > Hmm, thinking more about this, I just realized that this is an > input pin to the CPU, not an output pin like all other pins > described by the INT3472 device. I missed that as first. > > In that case using GPIO_LOOKUP_FLAGS_DEFAULT as before probably > makes the most sense. Please add a comment that this is an input > pin to the INT3472 patch and keep GPIO_LOOKUP_FLAGS_DEFAULT for > the next version. The GPIO_LOOKUP_FLAGS_DEFAULT is the "Linux default", not the hardware or firmware default so I don't think it's relevant in this context. There's a single non-GPIO bank driver using it, probably mistakenly. I'd also use GPIO_ACTIVE_LOW, for the reason Dongcheng pointed to above.
Hi Sakari, On 14-Apr-25 13:43, Sakari Ailus wrote: > Hans, Dongcheng, > > On Mon, Apr 14, 2025 at 01:09:47PM +0200, Hans de Goede wrote: >> Hi, >> >> On 14-Apr-25 13:04, Hans de Goede wrote: >>> Hi, >>> >>> On 14-Apr-25 11:59, Yan, Dongcheng wrote: >>>> Hi Andy and Hans, >>>> >>>> I found the description of lt6911uxe's GPIO in the spec: >>>> GPIO5 is used as the interrupt signal (50ms low level) to inform SOC >>>> start reading registers from 6911UXE; >>>> >>>> So setting the polarity as GPIO_ACTIVE_LOW is acceptable? >>> >>> Yes that is acceptable, thank you for looking this up. >> >> p.s. >> >> Note that setting GPIO_ACTIVE_LOW will invert the values returned >> by gpiod_get_value(), so if the driver uses that you will need >> to fix this in the driver. >> >> Hmm, thinking more about this, I just realized that this is an >> input pin to the CPU, not an output pin like all other pins >> described by the INT3472 device. I missed that as first. >> >> In that case using GPIO_LOOKUP_FLAGS_DEFAULT as before probably >> makes the most sense. Please add a comment that this is an input >> pin to the INT3472 patch and keep GPIO_LOOKUP_FLAGS_DEFAULT for >> the next version. > > The GPIO_LOOKUP_FLAGS_DEFAULT is the "Linux default", not the hardware or > firmware default so I don't think it's relevant in this context. There's a > single non-GPIO bank driver using it, probably mistakenly. > > I'd also use GPIO_ACTIVE_LOW, for the reason Dongcheng pointed to above. The GPIO being interpreted as active-low is a thing specific to the chip used though. Where as in the future the HPD pin type in the INT3472 device might be used with other chips... Anyways either way is fine with me bu, as mentioned, using GPIO_ACTIVE_LOW will invert the values returned by gpiod_get_value(), for which the driver likely needs to be adjusted. Regards, Hans
Hi Hans, On Mon, Apr 14, 2025 at 02:21:56PM +0200, Hans de Goede wrote: > Hi Sakari, > > On 14-Apr-25 13:43, Sakari Ailus wrote: > > Hans, Dongcheng, > > > > On Mon, Apr 14, 2025 at 01:09:47PM +0200, Hans de Goede wrote: > >> Hi, > >> > >> On 14-Apr-25 13:04, Hans de Goede wrote: > >>> Hi, > >>> > >>> On 14-Apr-25 11:59, Yan, Dongcheng wrote: > >>>> Hi Andy and Hans, > >>>> > >>>> I found the description of lt6911uxe's GPIO in the spec: > >>>> GPIO5 is used as the interrupt signal (50ms low level) to inform SOC > >>>> start reading registers from 6911UXE; > >>>> > >>>> So setting the polarity as GPIO_ACTIVE_LOW is acceptable? > >>> > >>> Yes that is acceptable, thank you for looking this up. > >> > >> p.s. > >> > >> Note that setting GPIO_ACTIVE_LOW will invert the values returned > >> by gpiod_get_value(), so if the driver uses that you will need > >> to fix this in the driver. > >> > >> Hmm, thinking more about this, I just realized that this is an > >> input pin to the CPU, not an output pin like all other pins > >> described by the INT3472 device. I missed that as first. > >> > >> In that case using GPIO_LOOKUP_FLAGS_DEFAULT as before probably > >> makes the most sense. Please add a comment that this is an input > >> pin to the INT3472 patch and keep GPIO_LOOKUP_FLAGS_DEFAULT for > >> the next version. > > > > The GPIO_LOOKUP_FLAGS_DEFAULT is the "Linux default", not the hardware or > > firmware default so I don't think it's relevant in this context. There's a > > single non-GPIO bank driver using it, probably mistakenly. > > > > I'd also use GPIO_ACTIVE_LOW, for the reason Dongcheng pointed to above. > > The GPIO being interpreted as active-low is a thing specific to > the chip used though. Where as in the future the HPD pin type > in the INT3472 device might be used with other chips... > > Anyways either way is fine with me bu, as mentioned, using GPIO_ACTIVE_LOW > will invert the values returned by gpiod_get_value(), for which the driver > likely needs to be adjusted. The driver appears to ask for both IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and it only uses the GPIO for an ISR so it doesn't seem to require driver changes IMO. Although this also seems to make the polarit irrelevant, at least for this driver.
Hi, On 14-Apr-25 14:32, Sakari Ailus wrote: > Hi Hans, > > On Mon, Apr 14, 2025 at 02:21:56PM +0200, Hans de Goede wrote: >> Hi Sakari, >> >> On 14-Apr-25 13:43, Sakari Ailus wrote: >>> Hans, Dongcheng, >>> >>> On Mon, Apr 14, 2025 at 01:09:47PM +0200, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 14-Apr-25 13:04, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 14-Apr-25 11:59, Yan, Dongcheng wrote: >>>>>> Hi Andy and Hans, >>>>>> >>>>>> I found the description of lt6911uxe's GPIO in the spec: >>>>>> GPIO5 is used as the interrupt signal (50ms low level) to inform SOC >>>>>> start reading registers from 6911UXE; >>>>>> >>>>>> So setting the polarity as GPIO_ACTIVE_LOW is acceptable? >>>>> >>>>> Yes that is acceptable, thank you for looking this up. >>>> >>>> p.s. >>>> >>>> Note that setting GPIO_ACTIVE_LOW will invert the values returned >>>> by gpiod_get_value(), so if the driver uses that you will need >>>> to fix this in the driver. >>>> >>>> Hmm, thinking more about this, I just realized that this is an >>>> input pin to the CPU, not an output pin like all other pins >>>> described by the INT3472 device. I missed that as first. >>>> >>>> In that case using GPIO_LOOKUP_FLAGS_DEFAULT as before probably >>>> makes the most sense. Please add a comment that this is an input >>>> pin to the INT3472 patch and keep GPIO_LOOKUP_FLAGS_DEFAULT for >>>> the next version. >>> >>> The GPIO_LOOKUP_FLAGS_DEFAULT is the "Linux default", not the hardware or >>> firmware default so I don't think it's relevant in this context. There's a >>> single non-GPIO bank driver using it, probably mistakenly. >>> >>> I'd also use GPIO_ACTIVE_LOW, for the reason Dongcheng pointed to above. >> >> The GPIO being interpreted as active-low is a thing specific to >> the chip used though. Where as in the future the HPD pin type >> in the INT3472 device might be used with other chips... >> >> Anyways either way is fine with me bu, as mentioned, using GPIO_ACTIVE_LOW >> will invert the values returned by gpiod_get_value(), for which the driver >> likely needs to be adjusted. > > The driver appears to ask for both IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING > and it only uses the GPIO for an ISR so it doesn't seem to require driver > changes IMO. Although this also seems to make the polarit irrelevant, at > least for this driver. If the driver does not care about this I would prefer for the INT3472 code to use GPIO_ACTIVE_HIGH to avoid the inverting behavior of GPIO_ACTIVE_LOW making things harder for other future drivers using the hpd pin through the INT3472 glue code. Regards, Hans
Hi Hans, On Mon, Apr 14, 2025 at 02:37:36PM +0200, Hans de Goede wrote: > Hi, > > On 14-Apr-25 14:32, Sakari Ailus wrote: > > Hi Hans, > > > > On Mon, Apr 14, 2025 at 02:21:56PM +0200, Hans de Goede wrote: > >> Hi Sakari, > >> > >> On 14-Apr-25 13:43, Sakari Ailus wrote: > >>> Hans, Dongcheng, > >>> > >>> On Mon, Apr 14, 2025 at 01:09:47PM +0200, Hans de Goede wrote: > >>>> Hi, > >>>> > >>>> On 14-Apr-25 13:04, Hans de Goede wrote: > >>>>> Hi, > >>>>> > >>>>> On 14-Apr-25 11:59, Yan, Dongcheng wrote: > >>>>>> Hi Andy and Hans, > >>>>>> > >>>>>> I found the description of lt6911uxe's GPIO in the spec: > >>>>>> GPIO5 is used as the interrupt signal (50ms low level) to inform SOC > >>>>>> start reading registers from 6911UXE; > >>>>>> > >>>>>> So setting the polarity as GPIO_ACTIVE_LOW is acceptable? > >>>>> > >>>>> Yes that is acceptable, thank you for looking this up. > >>>> > >>>> p.s. > >>>> > >>>> Note that setting GPIO_ACTIVE_LOW will invert the values returned > >>>> by gpiod_get_value(), so if the driver uses that you will need > >>>> to fix this in the driver. > >>>> > >>>> Hmm, thinking more about this, I just realized that this is an > >>>> input pin to the CPU, not an output pin like all other pins > >>>> described by the INT3472 device. I missed that as first. > >>>> > >>>> In that case using GPIO_LOOKUP_FLAGS_DEFAULT as before probably > >>>> makes the most sense. Please add a comment that this is an input > >>>> pin to the INT3472 patch and keep GPIO_LOOKUP_FLAGS_DEFAULT for > >>>> the next version. > >>> > >>> The GPIO_LOOKUP_FLAGS_DEFAULT is the "Linux default", not the hardware or > >>> firmware default so I don't think it's relevant in this context. There's a > >>> single non-GPIO bank driver using it, probably mistakenly. > >>> > >>> I'd also use GPIO_ACTIVE_LOW, for the reason Dongcheng pointed to above. > >> > >> The GPIO being interpreted as active-low is a thing specific to > >> the chip used though. Where as in the future the HPD pin type > >> in the INT3472 device might be used with other chips... > >> > >> Anyways either way is fine with me bu, as mentioned, using GPIO_ACTIVE_LOW > >> will invert the values returned by gpiod_get_value(), for which the driver > >> likely needs to be adjusted. > > > > The driver appears to ask for both IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING > > and it only uses the GPIO for an ISR so it doesn't seem to require driver > > changes IMO. Although this also seems to make the polarit irrelevant, at > > least for this driver. > > If the driver does not care about this I would prefer for the INT3472 code to > use GPIO_ACTIVE_HIGH to avoid the inverting behavior of GPIO_ACTIVE_LOW making > things harder for other future drivers using the hpd pin through the INT3472 > glue code. I'm fine with that, too. (My main point was indeed GPIO_LOOKUP_FLAGS_DEFAULT doesn't seem to be a good fit here.)
Hi, On 14-Apr-25 14:54, Sakari Ailus wrote: > Hi Hans, > > On Mon, Apr 14, 2025 at 02:37:36PM +0200, Hans de Goede wrote: >> Hi, >> >> On 14-Apr-25 14:32, Sakari Ailus wrote: >>> Hi Hans, >>> >>> On Mon, Apr 14, 2025 at 02:21:56PM +0200, Hans de Goede wrote: >>>> Hi Sakari, >>>> >>>> On 14-Apr-25 13:43, Sakari Ailus wrote: >>>>> Hans, Dongcheng, >>>>> >>>>> On Mon, Apr 14, 2025 at 01:09:47PM +0200, Hans de Goede wrote: >>>>>> Hi, >>>>>> >>>>>> On 14-Apr-25 13:04, Hans de Goede wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 14-Apr-25 11:59, Yan, Dongcheng wrote: >>>>>>>> Hi Andy and Hans, >>>>>>>> >>>>>>>> I found the description of lt6911uxe's GPIO in the spec: >>>>>>>> GPIO5 is used as the interrupt signal (50ms low level) to inform SOC >>>>>>>> start reading registers from 6911UXE; >>>>>>>> >>>>>>>> So setting the polarity as GPIO_ACTIVE_LOW is acceptable? >>>>>>> >>>>>>> Yes that is acceptable, thank you for looking this up. >>>>>> >>>>>> p.s. >>>>>> >>>>>> Note that setting GPIO_ACTIVE_LOW will invert the values returned >>>>>> by gpiod_get_value(), so if the driver uses that you will need >>>>>> to fix this in the driver. >>>>>> >>>>>> Hmm, thinking more about this, I just realized that this is an >>>>>> input pin to the CPU, not an output pin like all other pins >>>>>> described by the INT3472 device. I missed that as first. >>>>>> >>>>>> In that case using GPIO_LOOKUP_FLAGS_DEFAULT as before probably >>>>>> makes the most sense. Please add a comment that this is an input >>>>>> pin to the INT3472 patch and keep GPIO_LOOKUP_FLAGS_DEFAULT for >>>>>> the next version. >>>>> >>>>> The GPIO_LOOKUP_FLAGS_DEFAULT is the "Linux default", not the hardware or >>>>> firmware default so I don't think it's relevant in this context. There's a >>>>> single non-GPIO bank driver using it, probably mistakenly. >>>>> >>>>> I'd also use GPIO_ACTIVE_LOW, for the reason Dongcheng pointed to above. >>>> >>>> The GPIO being interpreted as active-low is a thing specific to >>>> the chip used though. Where as in the future the HPD pin type >>>> in the INT3472 device might be used with other chips... >>>> >>>> Anyways either way is fine with me bu, as mentioned, using GPIO_ACTIVE_LOW >>>> will invert the values returned by gpiod_get_value(), for which the driver >>>> likely needs to be adjusted. >>> >>> The driver appears to ask for both IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING >>> and it only uses the GPIO for an ISR so it doesn't seem to require driver >>> changes IMO. Although this also seems to make the polarit irrelevant, at >>> least for this driver. >> >> If the driver does not care about this I would prefer for the INT3472 code to >> use GPIO_ACTIVE_HIGH to avoid the inverting behavior of GPIO_ACTIVE_LOW making >> things harder for other future drivers using the hpd pin through the INT3472 >> glue code. > > I'm fine with that, too. (My main point was indeed > GPIO_LOOKUP_FLAGS_DEFAULT doesn't seem to be a good fit here.) Ok lets go with GPIO_ACTIVE_HIGH for the int3472/discrete.c changes then. Regards, Hans
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h index 145dec66df64..db4cd3720e24 100644 --- a/drivers/platform/x86/intel/int3472/common.h +++ b/drivers/platform/x86/intel/int3472/common.h @@ -22,6 +22,7 @@ #define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b #define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c #define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d +#define INT3472_GPIO_TYPE_HOTPLUG_DETECT 0x13 #define INT3472_PDEV_MAX_NAME_LEN 23 #define INT3472_MAX_SENSOR_GPIOS 3 diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index 30ff8f3ea1f5..28d41b1b3809 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -186,6 +186,10 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, *con_id = "privacy-led"; *gpio_flags = GPIO_ACTIVE_HIGH; break; + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: + *con_id = "hpd"; + *gpio_flags = GPIO_LOOKUP_FLAGS_DEFAULT; + break; case INT3472_GPIO_TYPE_POWER_ENABLE: *con_id = "power-enable"; *gpio_flags = GPIO_ACTIVE_HIGH; @@ -212,6 +216,7 @@ static void int3472_get_con_id_and_polarity(struct acpi_device *adev, u8 *type, * 0x0b Power enable * 0x0c Clock enable * 0x0d Privacy LED + * 0x13 Hotplug detect * * There are some known platform specific quirks where that does not quite * hold up; for example where a pin with type 0x01 (Power down) is mapped to @@ -281,6 +286,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, switch (type) { case INT3472_GPIO_TYPE_RESET: case INT3472_GPIO_TYPE_POWERDOWN: + case INT3472_GPIO_TYPE_HOTPLUG_DETECT: ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, con_id, gpio_flags); if (ret) err_msg = "Failed to map GPIO pin to sensor\n";
Typically HDMI to MIPI CSI-2 bridges have a pin to signal image data is being received. On the host side this is wired to a GPIO for polling or interrupts. This includes the Lontium HDMI to MIPI CSI-2 bridges lt6911uxe and lt6911uxc. The GPIO "hpd" is used already by other HDMI to CSI-2 bridges, use it here as well. Signed-off-by: Dongcheng Yan <dongcheng.yan@intel.com> --- drivers/platform/x86/intel/int3472/common.h | 1 + drivers/platform/x86/intel/int3472/discrete.c | 6 ++++++ 2 files changed, 7 insertions(+) base-commit: 01c6df60d5d4ae00cd5c1648818744838bba7763