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

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

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