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

[Hans de Goede <hdegoede@redhat.com>]

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