| Message ID | 20210322030128.2283-4-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Support DisplayPort mode | expand |
Quoting Laurent Pinchart (2021-03-21 20:01:20) > 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> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Hi, On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> 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? -Doug
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@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 :-)
Hi, On Tue, Mar 23, 2021 at 2:42 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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@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). -Doug
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.
Hi, On Tue, Mar 23, 2021 at 4:03 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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. 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... -Doug
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@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. > > 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 :-)
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);
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(+)