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?
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. 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.
devres actions cannot be added to be executed _before_ .remove, AFAIK.
> > + /*
> > + * sn65dsi83_atomic_disable() should release some resources, but it
> > + * cannot if we call drm_bridge_unplug() before it can
> > + * drm_bridge_enter(). If that happens, let's release those
> > + * resources now.
> > + */
> > + if (ctx->disable_resources_needed) {
> > + if (!ctx->irq)
> > + sn65dsi83_monitor_stop(ctx);
> > +
> > + gpiod_set_value_cansleep(ctx->enable_gpio, 0);
> > + usleep_range(10000, 11000);
> > +
> > + regulator_disable(ctx->vcc);
> > + }
>
> I'm not sure you need this. Wouldn't registering a devm action do the
> same thing?
Good idea, thanks. I'll give it a try.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
