On Mon, Sep 08, 2025 at 03:49:06PM +0200, Luca Ceresoli wrote:
> Hi Maxime,
>
> On Wed, 27 Aug 2025 09:46:03 +0200
> Maxime Ripard <mrip...@kernel.org> wrote:
>
> > On Wed, Aug 20, 2025 at 01:13:02PM +0200, Luca Ceresoli wrote:
> > > Hello Maxime,
> > >
> > > On Tue, 19 Aug 2025 14:29:32 +0200
> > > Maxime Ripard <mrip...@kernel.org> wrote:
> > >
> > > > > @@ -1005,7 +1041,24 @@ static void sn65dsi83_remove(struct i2c_client
> > > > > *client)
> > > > > {
> > > > > struct sn65dsi83 *ctx = i2c_get_clientdata(client);
> > > > >
> > > > > + drm_bridge_unplug(&ctx->bridge);
> > > > > drm_bridge_remove(&ctx->bridge);
> > > >
> > > > Shouldn't we merge drm_bridge_unplug with the release part of
> > > > devm_drm_bridge_alloc?
> > >
> > > I'm not sure I got what you are suggesting here, sorry.
> > >
> > > Do you mean that __devm_drm_bridge_alloc() should add a devres action
> > > to call drm_bridge_unplug(), so the unplug is called implicitly and
> > > does not need to be called explicitly by all drivers?
> >
> > Yes
> >
> > > If that's what you mean, I don't think that would work. Unless I'm
> > > missing something, devres actions are always invoked just after the
> > > driver .remove callback.
> >
> > Yes, they are called in reverse order of registration, after remove.
> >
> > > But we need to call drm_bridge_unplug() at the beginning (or just
> > > before) .remove, at least for drivers that need to do something in
> > > .remove that cannot be done by devm.
> > >
> > > In pseudocode:
> > >
> > > mybridge_remove()
> > > {
> > > drm_bridge_unplug(); <-- explicit call as in my patch
> > > xyz_disable();
> > > drm_bridge_unplug(); <-- implicitly done by devres
> > > }
> > >
> > > We want xyz_disable() to be done after drm_bridge_unplug(), so other
> > > code paths using drm_bridge_enter/exit() won't mess with xyz.
> >
> > It's not clear to me why doing it before xyz_disable() is important
> > here? If anything, it would prevent from disabling the hardware for
> > example, even though you still have your memory mapping, clocks, power
> > domains, regulators, etc. to properly disable it.
> >
> > You're still correct that it's a bad idea though because we want to do
> > it before we start freeing all those, so it needs to execute as the
> > before the devm actions ...
> >
> > > devres actions cannot be added to be executed _before_ .remove, AFAIK.
> >
> > ... and we can't do that either.
>
> I understand your words as "the drm_bridge_unplug() is OK where it is,
> your patch is OK in this respect". Correct?
>
> So if this is correct, and my reply on the devres cleanups is also
> correct (other reply in this thread), that means the whole patch is OK.I'm still confused why it's so important than in your example xyz_disable must be called after drm_bridge_unplug. Maxime
signature.asc
Description: PGP signature
