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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.

-- 
Regards,

Sakari Ailus