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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0e2306d7-3a07-45ad-958f-1039fb10a8cf@redhat.com>
Date: Mon, 14 Apr 2025 14:37:36 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: "Yan, Dongcheng" <dongcheng.yan@...el.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 "Yan, Dongcheng" <dongcheng.yan@...ux.intel.com>,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 hverkuil@...all.nl, u.kleine-koenig@...libre.com, ricardo.ribalda@...il.com,
 bingbu.cao@...ux.intel.com, hao.yao@...el.com
Subject: Re: [PATCH v1 1/2] platform/x86: int3472: add hpd pin support

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