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

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

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