Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Ensure that a runtime PM reference is acquired each time the DPAUX
> registers are accessed. Otherwise the code may end up running without
> the controller being powered, out-of-reset or clocked in some corner
> cases, resulting in a crash.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

On this one here I'm very confused.

- Why do you drop the runtime pm between enable and disable? Is that just
  how the hw works, i.e. the pad config stays, just the registers go away?

- I'm not seeing any locking between the different users (dp aux and
  pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
  make this easier (but I'm not super fond of that pattern from i2c).

- Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
  callback, otherwise the various userspace interface (dp aux, but also
  i2c on top of that) won't work. Some pre/post_transfer functions like
  i2c has might be useful for stuff like this.

Cheers, Daniel

> ---
>  drivers/gpu/drm/tegra/dpaux.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> index 622cdf1ad246..4b2b86aed1a5 100644
> --- a/drivers/gpu/drm/tegra/dpaux.c
> +++ b/drivers/gpu/drm/tegra/dpaux.c
> @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev *pinctrl,
>  			       unsigned int function, unsigned int group)
>  {
>  	struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> +	int err;
> +
> +	pm_runtime_get_sync(dpaux->dev);
> +	err = tegra_dpaux_pad_config(dpaux, function);
> +	pm_runtime_put(dpaux->dev);
>  
> -	return tegra_dpaux_pad_config(dpaux, function);
> +	return err;
>  }
>  
>  static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct drm_dp_aux *aux)
>  int drm_dp_aux_enable(struct drm_dp_aux *aux)
>  {
>  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> +	int err;
> +
> +	pm_runtime_get_sync(dpaux->dev);
> +	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> +	pm_runtime_put(dpaux->dev);
>  
> -	return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> +	return err;
>  }
>  
>  int drm_dp_aux_disable(struct drm_dp_aux *aux)
>  {
>  	struct tegra_dpaux *dpaux = to_dpaux(aux);
>  
> +	pm_runtime_get_sync(dpaux->dev);
>  	tegra_dpaux_pad_power_down(dpaux);
> +	pm_runtime_put(dpaux->dev);
>  
>  	return 0;
>  }
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel