Re: [PATCH 1/2] drm/tegra: dpaux: Populate AUX bus

[Doug Anderson <dianders@chromium.org>]

Hi,

On Mon, Dec 20, 2021 at 2:49 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> The DPAUX hardware block exposes an DP AUX interface that provides
> access to an AUX bus and the devices on that bus. Use the DP AUX bus
> infrastructure that was recently introduced to probe devices on this
> bus from DT.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/Kconfig | 1 +
>  drivers/gpu/drm/tegra/dpaux.c | 7 +++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> index 8cf5aeb9db6c..201f5175ecfe 100644
> --- a/drivers/gpu/drm/tegra/Kconfig
> +++ b/drivers/gpu/drm/tegra/Kconfig
> @@ -5,6 +5,7 @@ config DRM_TEGRA
>         depends on COMMON_CLK
>         depends on DRM
>         depends on OF
> +       select DRM_DP_AUX_BUS
>         select DRM_KMS_HELPER
>         select DRM_MIPI_DSI
>         select DRM_PANEL
> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> index 1f96e416fa08..9da1edcdc835 100644
> --- a/drivers/gpu/drm/tegra/dpaux.c
> +++ b/drivers/gpu/drm/tegra/dpaux.c
> @@ -18,6 +18,7 @@
>  #include <linux/reset.h>
>  #include <linux/workqueue.h>
>
> +#include <drm/drm_dp_aux_bus.h>
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_panel.h>
>
> @@ -570,6 +571,12 @@ static int tegra_dpaux_probe(struct platform_device *pdev)
>         list_add_tail(&dpaux->list, &dpaux_list);
>         mutex_unlock(&dpaux_lock);
>
> +       err = devm_of_dp_aux_populate_ep_devices(&dpaux->aux);
> +       if (err < 0) {
> +               dev_err(dpaux->dev, "failed to populate AUX bus: %d\n", err);
> +               return err;

nit:

if (err < 0)
  return dev_err_probe(dpaux->dev, err, "failed to populate AUX bus\n");

That's more concise and gives the pretty formatting for the error code
(not that devm_of_dp_aux_populate_ep_devices() currently returns
anything other than 0).

> +       }

I think you need to use of_dp_aux_populate_ep_devices() and then
depopulate in your remove, right? Otherwise the ordering won't be
right. Either that or fully transition your probe to devm (and get rid
of your remove) by adding some devm_add_action_or_reset() helpers...

Hrm, I guess looking closer it's also a little concerning that if
devm_of_dp_aux_populate_ep_devices() returns an error then you'll
totally skip all the things that tegra_dpaux_remove() is supposed to
handle. I guess that's a pre-existing bug (earlier error cases in the
probe don't, for instance, undo the pm_runtime state) but this makes
it worse.


-Doug