[RFC PATCH 03/11] drm/bridge: ti-sn65dsi86: Unregister AUX adapter in remove()

[Laurent Pinchart]

Hi Doug,

On Tue, Mar 23, 2021 at 02:08:42PM -0700, Doug Anderson wrote:
> On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote:
> >
> > The AUX adapter registered in probe() need to be unregistered in
> > remove(). Do so.
> >
> > Fixes: b814ec6d4535 ("drm/bridge: ti-sn65dsi86: Implement AUX channel")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index da78a12e58b5..c45420a50e73 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -1307,6 +1307,9 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
> >                 return -EINVAL;
> >
> >         kfree(pdata->edid);
> > +
> > +       drm_dp_aux_unregister(&pdata->aux);
> > +
> >         ti_sn_debugfs_remove(pdata);
> >
> >         of_node_put(pdata->host_node);
> 
> Good catch. One question, though. I know DRM sometimes has different
> conventions than the rest of the kernel, but I always look for the
> "remove" to be backwards of probe. That means that your code (and
> probably most of the remove function) should come _after_ the
> drm_bridge_remove(), right?  ...since drm_bridge_add() was the last
> thing in probe then drm_bridge_remove() should be the first thing in
> remove?

I agree in theory, yes. However, in practice, if you remove a bridge
that is currently in use, all hell will break lose. And if the bridge
isn't being used, it makes no difference. Still, it's worth changing the
order of operations to move drm_bridge_remove() first, as it won't hurt
in any case and is logically better. It's not an issue introduced by
this series though, so how how about it on top, or in parallel ? You can
even submit a patch if you want :-)

-- 
Regards,

Laurent Pinchart