Re: [PATCH v4] drm/bridge: ti-sn65dsi86: Add HPD for DisplayPort connector type

[Jayesh Choudhary <j-choudhary@ti.com>]

Hello Doug,

On 12/06/25 03:08, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 10, 2025 at 10:29 PM Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>
>> @@ -1195,9 +1203,17 @@ static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge)
>>          struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>          int val = 0;
>>
>> -       pm_runtime_get_sync(pdata->dev);
>> +       /*
>> +        * The chip won't report HPD right after being powered on as
>> +        * HPD_DEBOUNCED_STATE reflects correct state only after the
>> +        * debounce time (~100-400 ms).
>> +        * So having pm_runtime_get_sync() and immediately reading
>> +        * the register in detect() won't work, and adding delay()
>> +        * in detect will have performace impact in display.
>> +        * So remove runtime calls here.
> 
> That last sentence makes sense in a commit message, but not long term.
> Someone reading the code later won't understand what "remove" means.
> If you change "remove" to "omit" then it all makes sense, though. You
> could also say that a pm_runtime reference will be grabbed by
> ti_sn_bridge_hpd_enable().

Okay. Will edit this.

> 
> 
>> +        */
>> +
>>          regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val);
>> -       pm_runtime_put_autosuspend(pdata->dev);
>>
>>          return val & HPD_DEBOUNCED_STATE ? connector_status_connected
>>                                           : connector_status_disconnected;
>> @@ -1220,6 +1236,20 @@ static void ti_sn65dsi86_debugfs_init(struct drm_bridge *bridge, struct dentry *
>>          debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
>>   }
>>
>> +static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>> +{
>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +
>> +       pm_runtime_get_sync(pdata->dev);
>> +}
>> +
>> +static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
>> +{
>> +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>> +
>> +       pm_runtime_put_sync(pdata->dev);
>> +}
>> +
>>   static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>>          .attach = ti_sn_bridge_attach,
>>          .detach = ti_sn_bridge_detach,
>> @@ -1234,6 +1264,8 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>>          .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>>          .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>          .debugfs_init = ti_sn65dsi86_debugfs_init,
>> +       .hpd_enable = ti_sn_bridge_hpd_enable,
>> +       .hpd_disable = ti_sn_bridge_hpd_disable,
>>   };
>>
>>   static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
>> @@ -1322,7 +1354,8 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
>>                             ? DRM_MODE_CONNECTOR_DisplayPort : DRM_MODE_CONNECTOR_eDP;
>>
>>          if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort)
>> -               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
>> +               pdata->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT |
>> +                                   DRM_BRIDGE_OP_HPD;
> 
> I think you also need this in the "DRM_MODE_CONNECTOR_DisplayPort" if test:
> 
> /*
>   * If comms were already enabled they would have been enabled
>   * with the wrong value of HPD_DISABLE. Update it now. Comms
>   * could be enabled if anyone is holding a pm_runtime reference
>   * (like if a GPIO is in use). Note that in most cases nobody
>   * is doing AUX channel xfers before the bridge is added so
>   * HPD doesn't _really_ matter then. The only exception is in
>   * the eDP case where the panel wants to read the EDID before
>   * the bridge is added. We always consistently have HPD disabled
>   * for eDP.
>   */
> mutex_lock(&pdata->comms_mutex);
> if (pdata->comms_enabled)
>    regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
>      HPD_DISABLE, 0);
> mutex_unlock(&pdata->comms_mutex);
> 
> Does that sound right?


Here I don't think it is necessary to add this because enable_comms
will be called again after probe either in hpd_enable() (in case
refclk exist) or pre_enable() (in case it doesn't) with correct value.

If this seems okay then I will roll v5 with just the typo fixed in the
comments in detect().

Warm Regards,
Jayesh