[21/21] drm/bridge: tc358767: implement naive HPD handling

[Andrey Smirnov]

On Wed, Mar 20, 2019 at 6:03 AM Tomi Valkeinen <tomi.valkeinen at ti.com> wrote:
>
> On 20/03/2019 08:57, Tomi Valkeinen wrote:
> > On 19/03/2019 20:18, Andrey Smirnov wrote:
> >
> >> TC358767 has two GPIO pins that can be used for HPD signal. I think
> >> instead of hardcoding GPIO0 here it would be more flexible to expose
> >> boths gpios as a gpiochip and use gpiolib API to query the value of
> >> HPD as well as use "hpd-gpios" binidng in DT to select which input to
> >> use.
> >>
> >> Another argument in favour of this solution is that Toshiba's FAEs (at
> >> least some) recommend thier customers to connect HPD signal to SoC's
> >> GPIOs and bypass TC358767 entirely. Their reasoning being that
> >> TC358767 implements a generic GPIO contoller and there's no advantage
> >> in going through TC358767 if you could use your "normal" GPIOs.
> >>
> >> Using gpiolib API would allow us to handle both usecase with the same
> >> code.
> >>
> >> Lastly, by treating "hpd-gpios" as an optional property, we can
> >> preserve old driver behaviour regardless if it is connected to DP or
> >> eDP panel. Not saying that this is really worth doing, just pointing
> >> out that this option would be on the table as well.
> >
> > I think that's a good point.
> >
> > There's one thing that TC358767 offers, which may not be available on
> > most generic GPIO controllers, though: it can detect short (programmable
> > length) pulses, thus it's possible to easily implement the DisplayPort
> > IRQ mechanism. I'm not sure if it's possible to implement reliable DP
> > IRQ detection without HW support.
> >
> > Still, I think using standard gpios makes sense.
>
> After implementing the gpiochip (it works), I started to wonder...
>
> If TC358767 is used as a gpio expander, for whatever purpose, outside
> the TC358767 driver, then obviously we need the gpiochip driver. But I
> don't think anyone needs that.
>

Regardless of how it's going to be implemented in the end, there'd
have to be a way to specify which HPD input is being used. Which means
a either a new DT binding or re-using already existing to be agreed on
by DT folks. It just seems to me that there exists a much stronger
case to solve this using existing "language" of GPIO references as
opposed to introducing some vendor specific binding serving just this
single purpose. If DT is supposed to be used to describe the HW, then,
IMHO, it might be the other way around, TC358767 is also a GPIO
expander and has to be modeled/implemented as such, whether anyone
would ever use it in such capacity fully isn't that significant.

> Then we have two cases 1) HPD connected to TC358767, 2) HPD goes
> directly to the SoC, or worded differently, HPD is handled by something
> else than TC358767.
>
> 1) was implemented in this current patch, and there's no real benefit
> with the gpiochip. It's somewhat confusing that the driver provides a
> gpiochip which the same driver then uses, for its internal functionality.
>
> 2) should actually not involve TC358767 driver at all as it's totally
> outside TC358767.
>

There's already precedent for such usage in ti-tfp410.c, analogix/ and
andanalogix-anx78xx.c, so it's not unheard of.

> If the HPD goes from the DP connector to the SoC, we should have the DP
> connector driver handle it. Currently that connector is in the TC358767
> driver, but it should really be separated.
>

Sure, there's definitely more than one way to solve this.

> So... Obviously what's missing from the current patch is that we need to
> be able to say which of the two GPIOs are used for the HPD (if any). But
> I'm debating with myself whether gpiochip here is a sane choice or not.

Yeah, maybe it'd be best to submit a patch to DT mailing list and see
what they have to say?

Thanks,
Andrey Smirnov