[PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Dongcheng Yan]

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
-- 
2.34.1

[PATCH v1 2/2] media: i2c: change lt6911uxe irq_gpio name to "hpd"

[Dongcheng Yan]

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)
-- 
2.34.1

Re: [PATCH v1 2/2] media: i2c: change lt6911uxe irq_gpio name to "hpd"

[Hans de Goede]

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)

Re: [PATCH v1 2/2] media: i2c: change lt6911uxe irq_gpio name to "hpd"

[Yan, Dongcheng]

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)
> 

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Hans de Goede]

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

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Yan, Dongcheng]

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
> 
> 

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Yan, Dongcheng]

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

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Yan, Dongcheng]

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.
> 

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Hans de Goede]

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.
>>
> 

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Hans de Goede]

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.
>>>
>>
> 

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Hans de Goede]

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

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Hans de Goede]

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

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Sakari Ailus]

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.)

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Hans de Goede]

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

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Sakari Ailus]

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 .

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

[Yan, Dongcheng]

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