On Mon, Jan 25, 2021 at 11:52:18AM +0100, Maxime Ripard wrote:
> Hi Ville,
>
> On Fri, Jan 22, 2021 at 02:15:07PM +0200, Ville Syrjälä wrote:
> > On Thu, Jan 21, 2021 at 05:35:33PM +0100, Maxime Ripard wrote:
> > > Some drivers are storing the plane->state pointer in atomic_update and
> > > atomic_disable in a variable simply called state, while the state passed
> > > as an argument is called old_state.
> > >
> > > In order to ease subsequent reworks and to avoid confusing or
> > > inconsistent names, let's rename those variables to new_state.
> > >
> > > This was done using the following coccinelle script, plus some manual
> > > changes for mtk and tegra.
> > >
> > > @ plane_atomic_func @
> > > identifier helpers;
> > > identifier func;
> > > @@
> > >
> > > (
> > > static const struct drm_plane_helper_funcs helpers = {
> > > ...,
> > > .atomic_disable = func,
> > > ...,
> > > };
> > > |
> > > static const struct drm_plane_helper_funcs helpers = {
> > > ...,
> > > .atomic_update = func,
> > > ...,
> > > };
> > > )
> > >
> > > @ moves_new_state_old_state @
> > > identifier plane_atomic_func.func;
> > > identifier plane;
> > > symbol old_state;
> > > symbol state;
> > > @@
> > >
> > > func(struct drm_plane *plane, struct drm_plane_state *old_state)
> > > {
> > > ...
> > > - struct drm_plane_state *state = plane->state;
> > > + struct drm_plane_state *new_state = plane->state;
> > > ...
> > > }
> > >
> > > @ depends on moves_new_state_old_state @
> > > identifier plane_atomic_func.func;
> > > identifier plane;
> > > identifier old_state;
> > > symbol state;
> > > @@
> > >
> > > func(struct drm_plane *plane, struct drm_plane_state *old_state)
> > > {
> > > <...
> > > - state
> > > + new_state
> > > ...>
> >
> > Was going to say that this migh eat something else, but I guess
> > the dependency prevents that?
>
> Yeah, the dependency takes care of this
>
> > Another way to avoid that I suppose would be to declare 'state'
> > as
> > symbol moves_new_state_old_state.state;
> >
> > That would probably make the intent a bit more obvious, even with
> > the dependency. Or does a dependency somehow automagically imply
> > that?
>
> I'm not sure if it does, but it's a symbol here not an identifier or an
> expression, so here moves_new_state_old_state.state would always resolve
> to state (and only state) anyway
Hm. Right. OK, cocci bits look good to me. Variable naming
bikeshed I'll leave to others :)
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
--
Ville Syrjälä
Intel
