On Mon, Sep 08, 2025 at 03:49:01PM +0200, Luca Ceresoli wrote: > 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?
Would using devm_add_action in atomic_pre_enable, and devm_release_action in atomic_post_disable work? That way, if you have a typical enable / disable cycle, the action will get registered and executed properly, and if you only have an enable but no matching disable, it will be collected after remove. Maxime
signature.asc
Description: PGP signature
