On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
