Hi Doug,
On Thu, Mar 25, 2021 at 05:43:22PM -0700, Doug Anderson wrote:
> On Tue, Mar 23, 2021 at 4:03 PM Laurent Pinchart wrote:
> > 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@xxxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > 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.
>
> I haven't forgotten about this and I've got it written, but I'm trying
> to put it together with the work I'm doing to fix EDID reading and
> that's still going to take me a while longer. I'm out tomorrow but
> _hoping_ that I'll be able to at least get a new patch series (at
> least RFC quality) next week...
No worries at all, it will take a few weeks at least before I get the
display controller and DSI working on my board, so you're not blocking
me :-)
--
Regards,
Laurent Pinchart
