Hi, On Thu, Apr 24, 2025 at 6:32 PM Kumar, Udit <u-kum...@ti.com> wrote: > > Hello Jayesh, > > On 4/24/2025 4:24 PM, Jayesh Choudhary wrote: > > For TI SoC J784S4, the display pipeline looks like: > > TIDSS -> CDNS-DSI -> SN65DSI86 -> DisplayConnector -> DisplaySink > > This requires HPD to detect connection form the connector. > > By default, the HPD is disabled for eDP. So enable it conditionally > > based on a new flag 'keep-hpd' as mentioned in the comments in the > > driver. > > > > Signed-off-by: Jayesh Choudhary <j-choudh...@ti.com> > > --- > > > > Hello All, > > > > Sending this RFC patch to get some thoughts on hpd for sn65dsi86. > > > > Now that we have a usecase for hpd in sn65dsi86, I wanted to get > > some comments on this approach to "NOT DISABLE" hpd in the bridge. > > As the driver considers the eDP case, it disables hpd by default. > > So I have added another property in the binding for keeping hpd > > functionality (the name used is still debatable) and used it in > > the driver. > > > > Is this approach okay? > > Also should this have a "Fixes" tag? > > > > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 6 ++++++ > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 14 +++++++++----- > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > index c93878b6d718..5948be612849 100644 > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml > > @@ -34,6 +34,12 @@ properties: > > Set if the HPD line on the bridge isn't hooked up to anything or is > > otherwise unusable. > > > > + keep-hpd: > > + type: boolean > > + description: > > + HPD is disabled in the bridge by default. Set it if HPD line makes > > + sense and is used. > > + > > Here are my suggestions > > 1) use interrupt in binding as optional instead of keep-hpd > > 2) use interrupt field (if present to enable of disable HPD functions in > driver)
Officially we've already got a "no-hpd" specified in the device tree. You're supposed to be specifying this if HPD isn't hooked up. It would be best if we could use that property if possible. If we think that using the lack of "no-hpd" will break someone then we should be explicit about that. I'd also note that unless you've figured out a way to turn off the awful debouncing that ti-sn65dsi86 does on HPD that using HPD (at least for initial panel power on) only really makes sense for when we're using ti-sn65dsi86 in "DP" mode. For initial eDP panel poweron it was almost always faster to just wait the maximum delay of the panel than to wait for ti-sn65dsi86 to finally report that HPD was asserted. I could also note that it's possible to use the ti-sn65dsi86's "HPD" detection even if the interrupt isn't hooked up, so I don't totally agree with Udit's suggestion. I guess the summary of my thoughts then: If you want to enable HPD for eDP, please explain why in the commit message. Are you using this to detect "panel interrupt"? Somehow using it for PSR? Using it during panel power on? If using it for panel power on, have you confirmed that this has a benefit compared to using the panel's maximum delay? -Doug
