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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Doug,

On Tue, Mar 23, 2021 at 03:55:05PM -0700, Doug Anderson wrote:
> On Tue, Mar 23, 2021 at 2:42 PM Laurent Pinchart wrote:
> > 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@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 ?
> 
> Sure, it can be a separate patch. I'd kinda prefer it be a patch
> _before_ ${SUBJECT} patch, though. Specifically it's harder for me to
> reason about whether your new function call is in the right place and
> won't cause any problems with the order being all jumbled. If we fix
> the order first then it's easy to reason about your patch.
> 
> > You can
> > even submit a patch if you want :-)
> 
> Happy to post it up if it won't cause more confusion w/ you posting
> your next version and trying to figure out what to base it on (since
> it will definitely conflict with your series).

I'll need quite a bit of time before v2, as I'd like to test it, and
that requires finishing support for the DSI bridge and the display
controller :-) Please feel free to post a patch if you have time, I
think it could get merged in drm-misc quite quickly.

-- 
Regards,

Laurent Pinchart