Hello Maxime,
On Wed, 20 Aug 2025 13:13:02 +0200
Luca Ceresoli <[email protected]> wrote:
> > > + /*
> > > + * 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.
I'm catching up with this series after being busy a few weeks...
I looked at this, but contrary my initial impression I think it would
not be an improvement.
The reason is at least one of these cleanup actions (namely the
regulator_disable()) must be done only if there is a matching enable,
which is in atomic_pre_enable. This is why I introduced a flag in the
first place.
I'm not sure which usage of devres you had in mind, but I see two
options.
Option 1: in probe, add a devres action to call a function like:
sn65dsi83_cleanups()
{
if (ctx->disable_resources_needed) {
/* the same cleanups */
}
}
But that is just a more indirect way of doing the same thing, and
relies on the same flag.
Option 2: have a function to unconditionally do the cleanups:
sn65dsi83_cleanups()
{
/* the same cleanups (no if) */
}
And then:
* in atomic_pre_enable, instead of setting the flag
add a devres action to call sn65dsi83_cleanups()
* in atomic_disable, instead of clearing the flag
remove the devres action
Even this option looks like more complicated and less readable code
to do the same thing.
Do you have in mind a better option that I haven't figured out?
If you don't, I think this part of the patch should stay as is.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
