On Wed, Sep 10, 2025 at 06:34:40PM +0200, Luca Ceresoli wrote: > Hi Maxime, > > On Wed, 10 Sep 2025 09:52:21 +0200 > Maxime Ripard <[email protected]> wrote: > > > 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. > > So you're OK with option 2. I just implemented it, works well and the > resulting code is a bit cleaner. Queued for v2.
Kind of, but you shouldn't remove but release it, and it doesn't have to be a single action / function. Maxime
signature.asc
Description: PGP signature
