Hi Maxime,
On Thu, 11 Sep 2025 08:44:34 +0200
Maxime Ripard <[email protected]> wrote:
> > > > 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.
Released instead of removed: yes.
Doesn't have to be a single function: I currently implemented a single
function with the 3 actions that are currently done in atomic_disable.
I think you propose to add 3 different devres actions, one for each,
but it would be more code and a little more resources used and I don't
see the advantage.
I think it makes sense that I send my current version and we can
continue discussion based on the code. I'm waiting a bit before
sending it, in case you have feedback about the other branch of this
discussion (placement of drm_bridge_unplug()).
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
