Re: [RFC PATCH] drm/bridge: ti-sn65dsi86: Enable HPD functionality

["Kumar, Udit" <u-kumar1@ti.com>]

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-choudhary@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)


>     vccio-supply:
>       description: A 1.8V supply that powers the digital IOs.
>   
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f72675766e01..4081cc957c6c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -191,6 +191,7 @@ struct ti_sn65dsi86 {
>   	u8				ln_polrs;
>   	bool				comms_enabled;
>   	struct mutex			comms_mutex;
> +	bool				keep_hpd;
>   
>   #if defined(CONFIG_OF_GPIO)
>   	struct gpio_chip		gchip;
> @@ -348,12 +349,15 @@ static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata,
>   	 * 200 ms.  We'll assume that the panel driver will have the hardcoded
>   	 * delay in its prepare and always disable HPD.
>   	 *
> -	 * If HPD somehow makes sense on some future panel we'll have to
> -	 * change this to be conditional on someone specifying that HPD should
> -	 * be used.
> +	 * If needed, use 'keep-hpd' property in the hardware description in
> +	 * board file as a conditional specifying that HPD should be used.
>   	 */
> -	regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> -			   HPD_DISABLE);
> +
> +	pdata->keep_hpd = of_property_read_bool(pdata->dev->of_node, "keep-hpd");
> +
> +	if (!pdata->keep_hpd)
> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> +				   HPD_DISABLE);

you may want to extend interrupt support in this driver if HPD is enabled.


>   
>   	pdata->comms_enabled = true;
>