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; + 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
lt6911uxe is used in IPU6 / x86 platform, worked with an out-of-tree int3472 patch and upstream intel/ipu6 before. The upstream int3472 driver uses "hpd" instead of "readystat" now. this patch updates the irq_gpio name to "hpd" accordingly, so that mere users can now use the upstream version directly without relying on out-of-tree int3472 pin support.
The new name "hpd" (Hotplug Detect) aligns with common naming conventions used in other drivers(like adv7604) and documentation.
Fixes: e49563c3be09d4 ("media: i2c: add lt6911uxe hdmi bridge driver") Cc: stable@vger.kernel.org Signed-off-by: Dongcheng Yan dongcheng.yan@intel.com --- drivers/media/i2c/lt6911uxe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/lt6911uxe.c b/drivers/media/i2c/lt6911uxe.c index c5b40bb58a37..24857d683fcf 100644 --- a/drivers/media/i2c/lt6911uxe.c +++ b/drivers/media/i2c/lt6911uxe.c @@ -605,10 +605,10 @@ static int lt6911uxe_probe(struct i2c_client *client) return dev_err_probe(dev, PTR_ERR(lt6911uxe->reset_gpio), "failed to get reset gpio\n");
- lt6911uxe->irq_gpio = devm_gpiod_get(dev, "readystat", GPIOD_IN); + lt6911uxe->irq_gpio = devm_gpiod_get(dev, "hpd", GPIOD_IN); if (IS_ERR(lt6911uxe->irq_gpio)) return dev_err_probe(dev, PTR_ERR(lt6911uxe->irq_gpio), - "failed to get ready_stat gpio\n"); + "failed to get hpd gpio\n");
ret = lt6911uxe_fwnode_parse(lt6911uxe, dev); if (ret)
Hi,
On 11-Apr-25 10:23 AM, Dongcheng Yan wrote:
lt6911uxe is used in IPU6 / x86 platform, worked with an out-of-tree int3472 patch and upstream intel/ipu6 before. The upstream int3472 driver uses "hpd" instead of "readystat" now. this patch updates the irq_gpio name to "hpd" accordingly, so that mere users can now use the upstream version directly without relying on out-of-tree int3472 pin support.
The new name "hpd" (Hotplug Detect) aligns with common naming conventions used in other drivers(like adv7604) and documentation.
Fixes: e49563c3be09d4 ("media: i2c: add lt6911uxe hdmi bridge driver") Cc: stable@vger.kernel.org Signed-off-by: Dongcheng Yan dongcheng.yan@intel.com
The commit message needs an extra sentence to explain that this change is ok to make since at the moment this driver is only used on ACPI platforms which will use the new "hpd" name and there are no devicetree bindings for this driver.
Regards,
Hans
drivers/media/i2c/lt6911uxe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/lt6911uxe.c b/drivers/media/i2c/lt6911uxe.c index c5b40bb58a37..24857d683fcf 100644 --- a/drivers/media/i2c/lt6911uxe.c +++ b/drivers/media/i2c/lt6911uxe.c @@ -605,10 +605,10 @@ static int lt6911uxe_probe(struct i2c_client *client) return dev_err_probe(dev, PTR_ERR(lt6911uxe->reset_gpio), "failed to get reset gpio\n");
- lt6911uxe->irq_gpio = devm_gpiod_get(dev, "readystat", GPIOD_IN);
- lt6911uxe->irq_gpio = devm_gpiod_get(dev, "hpd", GPIOD_IN); if (IS_ERR(lt6911uxe->irq_gpio)) return dev_err_probe(dev, PTR_ERR(lt6911uxe->irq_gpio),
"failed to get ready_stat gpio\n");
"failed to get hpd gpio\n");ret = lt6911uxe_fwnode_parse(lt6911uxe, dev); if (ret)
Hi hans,
On 4/11/2025 4:34 PM, Hans de Goede wrote:
Hi,
On 11-Apr-25 10:23 AM, Dongcheng Yan wrote:
lt6911uxe is used in IPU6 / x86 platform, worked with an out-of-tree int3472 patch and upstream intel/ipu6 before. The upstream int3472 driver uses "hpd" instead of "readystat" now. this patch updates the irq_gpio name to "hpd" accordingly, so that mere users can now use the upstream version directly without relying on out-of-tree int3472 pin support.
The new name "hpd" (Hotplug Detect) aligns with common naming conventions used in other drivers(like adv7604) and documentation.
Fixes: e49563c3be09d4 ("media: i2c: add lt6911uxe hdmi bridge driver") Cc: stable@vger.kernel.org Signed-off-by: Dongcheng Yan dongcheng.yan@intel.com
The commit message needs an extra sentence to explain that this change is ok to make since at the moment this driver is only used on ACPI platforms which will use the new "hpd" name and there are no devicetree bindings for this driver.
Thanks for your great advice. I'll update commit message next patch.
Best Regard, Dongcheng> Regards,
Hans
drivers/media/i2c/lt6911uxe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/lt6911uxe.c b/drivers/media/i2c/lt6911uxe.c index c5b40bb58a37..24857d683fcf 100644 --- a/drivers/media/i2c/lt6911uxe.c +++ b/drivers/media/i2c/lt6911uxe.c @@ -605,10 +605,10 @@ static int lt6911uxe_probe(struct i2c_client *client) return dev_err_probe(dev, PTR_ERR(lt6911uxe->reset_gpio), "failed to get reset gpio\n");
- lt6911uxe->irq_gpio = devm_gpiod_get(dev, "readystat", GPIOD_IN);
- lt6911uxe->irq_gpio = devm_gpiod_get(dev, "hpd", GPIOD_IN); if (IS_ERR(lt6911uxe->irq_gpio)) return dev_err_probe(dev, PTR_ERR(lt6911uxe->irq_gpio),
"failed to get ready_stat gpio\n");
"failed to get hpd gpio\n");ret = lt6911uxe_fwnode_parse(lt6911uxe, dev); if (ret)
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;@@ -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;@@ -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 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,
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.
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support Link: https://lore.kernel.org/stable/20250411082357.392713-1-dongcheng.yan%40intel...
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Dongcheng Yan -
Hans de Goede -
kernel test robot -
Yan, Dongcheng
-
Yan, Dongcheng