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