The main modification is moving the DP AUX initialization from function
analogix_dp_bind() to analogix_dp_probe(). In order to get the EDID of
eDP panel during probing, it is also needed to advance PM operations to
ensure that eDP controller and phy are prepared for AUX transmission.

Additionally, add support for &drm_dp_aux.wait_hpd_asserted() to help
confirm the HPD state before doing AUX transfers.

Signed-off-by: Damon Ding <damon.ding@rock-chips.com>

---

Changes in v4:
- Use done_probing() to call drm_of_find_panel_or_bridge() and
  component_add() when getting panel from the DP AUX bus

Changes in v5:
- Advance PM operations to make eDP AUX work well

Changes in v6:
- Use devm_pm_runtime_enable() instead of devm_add_action_or_reset()
- Add a new function analogix_dp_remove() to ensure symmetry for PM
  operations

Changes in v7:
- Fix the misspelling of word 'operations' in commit message
- Remove the check related to CONFIG_PM
- Remove the unnecessary call to pm_runtime_dont_use_autosuspend() if
  devm_pm_runtime_enable() fails
- Remove unnecessary function analogix_dp_remove()
- Add new function analogix_dpaux_wait_hpd_asserted()
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 37 ++++++++++++++-----
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index a57e06d303a1..ff81c37dbe1d 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1548,6 +1548,22 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux,
 	return ret;
 }
 
+static int analogix_dpaux_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us)
+{
+	struct analogix_dp_device *dp = to_dp(aux);
+	int val;
+	int ret;
+
+	pm_runtime_get_sync(dp->dev);
+
+	ret = readx_poll_timeout(analogix_dp_detect_hpd, dp, val, !val, wait_us / 100, wait_us);
+
+	pm_runtime_mark_last_busy(dp->dev);
+	pm_runtime_put_autosuspend(dp->dev);
+
+	return ret;
+}
+
 struct analogix_dp_device *
 analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
 {
@@ -1653,6 +1669,18 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
 		return ERR_PTR(ret);
 	}
 
+	dp->aux.name = "DP-AUX";
+	dp->aux.transfer = analogix_dpaux_transfer;
+	dp->aux.wait_hpd_asserted = analogix_dpaux_wait_hpd_asserted;
+	dp->aux.dev = dp->dev;
+	drm_dp_aux_init(&dp->aux);
+
+	pm_runtime_use_autosuspend(dp->dev);
+	pm_runtime_set_autosuspend_delay(dp->dev, 100);
+	ret = devm_pm_runtime_enable(dp->dev);
+	if (ret)
+		return ERR_PTR(ret);
+
 	return dp;
 }
 EXPORT_SYMBOL_GPL(analogix_dp_probe);
@@ -1699,15 +1727,6 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
 	dp->drm_dev = drm_dev;
 	dp->encoder = dp->plat_data->encoder;
 
-	pm_runtime_use_autosuspend(dp->dev);
-	pm_runtime_set_autosuspend_delay(dp->dev, 100);
-	ret = devm_pm_runtime_enable(dp->dev);
-	if (ret)
-		return ret;
-
-	dp->aux.name = "DP-AUX";
-	dp->aux.transfer = analogix_dpaux_transfer;
-	dp->aux.dev = dp->dev;
 	dp->aux.drm_dev = drm_dev;
 
 	ret = drm_dp_aux_register(&dp->aux);
-- 
2.34.1

Hi,

On Mon, Feb 24, 2025 at 12:14 AM Damon Ding <damon.ding@rock-chips.com> wrote:
>
> The main modification is moving the DP AUX initialization from function
> analogix_dp_bind() to analogix_dp_probe(). In order to get the EDID of
> eDP panel during probing, it is also needed to advance PM operations to
> ensure that eDP controller and phy are prepared for AUX transmission.
>
> Additionally, add support for &drm_dp_aux.wait_hpd_asserted() to help
> confirm the HPD state before doing AUX transfers.

Maybe move the addition of the analogix_dpaux_wait_hpd_asserted() to a
separate patch?


> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>
> ---
>
> Changes in v4:
> - Use done_probing() to call drm_of_find_panel_or_bridge() and
>   component_add() when getting panel from the DP AUX bus
>
> Changes in v5:
> - Advance PM operations to make eDP AUX work well
>
> Changes in v6:
> - Use devm_pm_runtime_enable() instead of devm_add_action_or_reset()
> - Add a new function analogix_dp_remove() to ensure symmetry for PM
>   operations
>
> Changes in v7:
> - Fix the misspelling of word 'operations' in commit message
> - Remove the check related to CONFIG_PM
> - Remove the unnecessary call to pm_runtime_dont_use_autosuspend() if
>   devm_pm_runtime_enable() fails
> - Remove unnecessary function analogix_dp_remove()
> - Add new function analogix_dpaux_wait_hpd_asserted()
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 37 ++++++++++++++-----
>  1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index a57e06d303a1..ff81c37dbe1d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1548,6 +1548,22 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux,
>         return ret;
>  }
>
> +static int analogix_dpaux_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us)
> +{
> +       struct analogix_dp_device *dp = to_dp(aux);
> +       int val;
> +       int ret;
> +
> +       pm_runtime_get_sync(dp->dev);
> +
> +       ret = readx_poll_timeout(analogix_dp_detect_hpd, dp, val, !val, wait_us / 100, wait_us);

More than happy if someone else wants to overrule me, but it seems
weird that you're looping over a function that already has a loop.
Shouldn't you be calling analogix_dp_get_plug_in_status() directly?
...and if "dp->force_hpd" you probably shouldn't be polling at all. If
HPD is not hooked up I think we decided in sn65dsi86 that we should
just sleep for the maximum time (choosing a conservative value if told
to wait forever).


Aside from that and the idea of moving
analogix_dpaux_wait_hpd_asserted() to a separate patch this looks good
to me.


-Doug

Hi Doug,

On 2025/2/25 9:41, Doug Anderson wrote:
> Hi,
> 
> On Mon, Feb 24, 2025 at 12:14 AM Damon Ding <damon.ding@rock-chips.com> wrote:
>>
>> The main modification is moving the DP AUX initialization from function
>> analogix_dp_bind() to analogix_dp_probe(). In order to get the EDID of
>> eDP panel during probing, it is also needed to advance PM operations to
>> ensure that eDP controller and phy are prepared for AUX transmission.
>>
>> Additionally, add support for &drm_dp_aux.wait_hpd_asserted() to help
>> confirm the HPD state before doing AUX transfers.
> 
> Maybe move the addition of the analogix_dpaux_wait_hpd_asserted() to a
> separate patch?
> 

It would be a good idea.

> 
>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>>
>> ---
>>
>> Changes in v4:
>> - Use done_probing() to call drm_of_find_panel_or_bridge() and
>>    component_add() when getting panel from the DP AUX bus
>>
>> Changes in v5:
>> - Advance PM operations to make eDP AUX work well
>>
>> Changes in v6:
>> - Use devm_pm_runtime_enable() instead of devm_add_action_or_reset()
>> - Add a new function analogix_dp_remove() to ensure symmetry for PM
>>    operations
>>
>> Changes in v7:
>> - Fix the misspelling of word 'operations' in commit message
>> - Remove the check related to CONFIG_PM
>> - Remove the unnecessary call to pm_runtime_dont_use_autosuspend() if
>>    devm_pm_runtime_enable() fails
>> - Remove unnecessary function analogix_dp_remove()
>> - Add new function analogix_dpaux_wait_hpd_asserted()
>> ---
>>   .../drm/bridge/analogix/analogix_dp_core.c    | 37 ++++++++++++++-----
>>   1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index a57e06d303a1..ff81c37dbe1d 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -1548,6 +1548,22 @@ static ssize_t analogix_dpaux_transfer(struct drm_dp_aux *aux,
>>          return ret;
>>   }
>>
>> +static int analogix_dpaux_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us)
>> +{
>> +       struct analogix_dp_device *dp = to_dp(aux);
>> +       int val;
>> +       int ret;
>> +
>> +       pm_runtime_get_sync(dp->dev);
>> +
>> +       ret = readx_poll_timeout(analogix_dp_detect_hpd, dp, val, !val, wait_us / 100, wait_us);
> 
> More than happy if someone else wants to overrule me, but it seems
> weird that you're looping over a function that already has a loop.
> Shouldn't you be calling analogix_dp_get_plug_in_status() directly?
> ...and if "dp->force_hpd" you probably shouldn't be polling at all. If
> HPD is not hooked up I think we decided in sn65dsi86 that we should
> just sleep for the maximum time (choosing a conservative value if told
> to wait forever).
> 

Yes, I think there is no need to use analogix_dp_detect_hpd(), and the 
code as you recommended is better:

static int analogix_dpaux_wait_hpd_asserted(struct drm_dp_aux *aux, 
unsigned long wait_us)
{
	struct analogix_dp_device *dp = to_dp(aux);
	int val;
	int ret;

	if (dp->force_hpd)
		return 0;

	pm_runtime_get_sync(dp->dev);

	ret = readx_poll_timeout(analogix_dp_get_plug_in_status, dp, val, !val,
				 wait_us / 100, wait_us);

	pm_runtime_mark_last_busy(dp->dev);
	pm_runtime_put_autosuspend(dp->dev);

	return ret;
}

> 
> Aside from that and the idea of moving
> analogix_dpaux_wait_hpd_asserted() to a separate patch this looks good
> to me.
> 

Best regards
Damon