Re: [PATCH v2 08/11] drm: Rename plane->state variables in atomic update and disable

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>,
	David Airlie <airlied@linux.ie>,
	Liviu Dudau <liviu.dudau@arm.com>,
	dri-devel@lists.freedesktop.org, linux-mips@vger.kernel.org,
	Paul Cercueil <paul@crapouillou.net>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	linux-stm32@st-md-mailman.stormreply.com,
	Jerome Brunet <jbrunet@baylibre.com>, Marek Vasut <marex@denx.de>,
	linux-samsung-soc@vger.kernel.org,
	Joonyoung Shim <jy0922.shim@samsung.com>,
	linux-rockchip@lists.infradead.org,
	Kevin Hilman <khilman@baylibre.com>,
	Russell King <linux@armlinux.org.uk>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Xinliang Liu <xinliang.liu@linaro.org>,
	Xinwei Kong <kong.kongxinwei@hisilicon.com>,
	Sandy Huang <hjc@rock-chips.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Chen Feng <puck.chen@hisilicon.com>,
	Dave Airlie <airlied@redhat.com>,
	Chun-Kuang Hu <chunkuang.hu@kernel.org>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	linux-arm-msm@vger.kernel.org,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Alison Wang <alison.wang@nxp.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	linux-mediatek@lists.infradead.org,
	Vincent Abriou <vincent.abriou@st.com>,
	Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>,
	linux-tegra@vger.kernel.org, linux-amlogic@lists.infradead.org,
	Sean Paul <sean@poorly.run>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	linux-arm-kernel@lists.infradead.org,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Tomi Valkeinen <tomba@kernel.org>, Jyri Sarha <jyri.sarha@iki.fi>,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	Philippe Cornu <philippe.cornu@st.com>,
	linux-kernel@vger.kernel.org,
	Yannick Fertre <yannick.fertre@st.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Tian Tao <tiantao6@hisilicon.com>,
	freedreno@lists.freedesktop.org
Subject: Re: [PATCH v2 08/11] drm: Rename plane->state variables in atomic update and disable
Date: Mon, 25 Jan 2021 13:52:38 +0200	[thread overview]
Message-ID: <YA6xBuECFjzjY7gG@intel.com> (raw)
In-Reply-To: <20210125105218.kv63vjbxz5b35hdo@gilmour>

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@linux.intel.com>

-- 
Ville Syrjälä
Intel

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic