Re: [PATCH v3 3/4] drm/panel: atna33xc20: Take advantage of wait_hpd_asserted() in struct drm_dp_aux

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 18/04/2022 20:17, Douglas Anderson wrote:
> Let's add support for being able to read the HPD pin even if it's
> hooked directly to the controller. This will let us take away the
> waiting in the AUX transfer functions of the eDP controller drivers.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - Don't check "hpd_asserted" boolean when unset.
> - Handle errors from gpiod_get_value_cansleep() properly.
> 
> Changes in v2:
> - Change is_hpd_asserted() to wait_hpd_asserted()
> 
>   .../gpu/drm/panel/panel-samsung-atna33xc20.c  | 41 +++++++++++++------
>   1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> index 20666b6217e7..5ef1b4032c56 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> @@ -19,6 +19,10 @@
>   #include <drm/drm_edid.h>
>   #include <drm/drm_panel.h>
>   
> +/* T3 VCC to HPD high is max 200 ms */
> +#define HPD_MAX_MS	200
> +#define HPD_MAX_US	(HPD_MAX_MS * 1000)
> +
>   struct atana33xc20_panel {
>   	struct drm_panel base;
>   	bool prepared;
> @@ -30,6 +34,7 @@ struct atana33xc20_panel {
>   
>   	struct regulator *supply;
>   	struct gpio_desc *el_on3_gpio;
> +	struct drm_dp_aux *aux;
>   
>   	struct edid *edid;
>   
> @@ -79,7 +84,7 @@ static int atana33xc20_suspend(struct device *dev)
>   static int atana33xc20_resume(struct device *dev)
>   {
>   	struct atana33xc20_panel *p = dev_get_drvdata(dev);
> -	bool hpd_asserted = false;
> +	int hpd_asserted;
>   	int ret;
>   
>   	/* T12 (Power off time) is min 500 ms */
> @@ -91,20 +96,28 @@ static int atana33xc20_resume(struct device *dev)
>   	p->powered_on_time = ktime_get();
>   
>   	/*
> -	 * Handle HPD. Note: if HPD is hooked up to a dedicated pin on the
> -	 * eDP controller then "no_hpd" will be false _and_ "hpd_gpio" will be
> -	 * NULL. It's up to the controller driver to wait for HPD after
> -	 * preparing the panel in that case.
> +	 * Note that it's possible that no_hpd is false, hpd_gpio is
> +	 * NULL, and wait_hpd_asserted is NULL. This is because
> +	 * wait_hpd_asserted() is optional even if HPD is hooked up to
> +	 * a dedicated pin on the eDP controller. In this case we just
> +	 * assume that the controller driver will wait for HPD at the
> +	 * right times.
>   	 */
>   	if (p->no_hpd) {
> -		/* T3 VCC to HPD high is max 200 ms */
> -		msleep(200);
> -	} else if (p->hpd_gpio) {
> -		ret = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
> -					 hpd_asserted, hpd_asserted,
> -					 1000, 200000);
> -		if (!hpd_asserted)
> -			dev_warn(dev, "Timeout waiting for HPD\n");
> +		msleep(HPD_MAX_MS);
> +	} else {
> +		if (p->hpd_gpio) {
> +			ret = readx_poll_timeout(gpiod_get_value_cansleep,
> +						 p->hpd_gpio, hpd_asserted,
> +						 hpd_asserted, 1000, HPD_MAX_US);
> +			if (hpd_asserted < 0)
> +				ret = hpd_asserted;
> +		} else if (p->aux->wait_hpd_asserted) {
> +			ret = p->aux->wait_hpd_asserted(p->aux, HPD_MAX_US);
> +		}
> +
> +		if (ret)
> +			dev_warn(dev, "Error waiting for HPD: %d\n", ret);

I'd suggest reworking this to:

if (p->no_hpd) {
   msleep();
   return 0;
}

if (p->hpd_gpio) {
  ret = readx_poll_timeout(...)

  if (ret)
    dev_warn()
  return ret;
}

if (p->aux->wait_hpd_asserted) {
   ret = p->aux->wait....
   if (ret)
     dev_warn(...)
   return ret;
}

return 0;


>   	}
>   
>   	return 0;
> @@ -263,6 +276,8 @@ static int atana33xc20_probe(struct dp_aux_ep_device *aux_ep)
>   		return -ENOMEM;
>   	dev_set_drvdata(dev, panel);
>   
> +	panel->aux = aux_ep->aux;
> +
>   	panel->supply = devm_regulator_get(dev, "power");
>   	if (IS_ERR(panel->supply))
>   		return dev_err_probe(dev, PTR_ERR(panel->supply),


-- 
With best wishes
Dmitry