From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753963AbdK1PzJ (ORCPT ); Tue, 28 Nov 2017 10:55:09 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:38376 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753238AbdK1PzH (ORCPT ); Tue, 28 Nov 2017 10:55:07 -0500 Date: Tue, 28 Nov 2017 16:54:42 +0100 From: Maxime Ripard To: Jernej Skrabec Cc: wens@csie.org, airlied@linux.ie, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, icenowy@aosc.io, linux-sunxi@googlegroups.com Subject: Re: [PATCH 01/17] drm/sun4i: Refactor DE2 code Message-ID: <20171128155442.fpmmf4xfb3rbf67p@flea.home> References: <20171127205750.19277-1-jernej.skrabec@siol.net> <20171127205750.19277-2-jernej.skrabec@siol.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="flxkqbb5q46lcspl" Content-Disposition: inline In-Reply-To: <20171127205750.19277-2-jernej.skrabec@siol.net> User-Agent: NeoMutt/20171027 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --flxkqbb5q46lcspl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Mon, Nov 27, 2017 at 09:57:34PM +0100, Jernej Skrabec wrote: > Since the time initial DE2 driver was written, some knowledge was gained > what setting are really necessary and what most of the magic values > mean. >=20 > This commit renames some of the macro names to better fit the real > meaning, replace default values with self explaining macros where > possible and removes settings which are not really needed. >=20 > Signed-off-by: Jernej Skrabec While those changes are quite welcome, it should be split in a number of patches... > --- > drivers/gpu/drm/sun4i/sun8i_mixer.c | 65 +++++++++++++++++++------------= ------ > drivers/gpu/drm/sun4i/sun8i_mixer.h | 31 ++++++++---------- > 2 files changed, 47 insertions(+), 49 deletions(-) >=20 > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/= sun8i_mixer.c > index cb193c5f1686..015943c0ed5a 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > @@ -44,7 +44,8 @@ void sun8i_mixer_layer_enable(struct sun8i_mixer *mixer, > /* Currently the first UI channel is used */ > int chan =3D mixer->cfg->vi_num; > =20 > - DRM_DEBUG_DRIVER("Enabling layer %d in channel %d\n", layer, chan); > + DRM_DEBUG_DRIVER("%sabling layer %d in channel %d\n", > + enable ? "En" : "Dis", layer, chan); > =20 > if (enable) > val =3D SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN; > @@ -55,15 +56,14 @@ void sun8i_mixer_layer_enable(struct sun8i_mixer *mix= er, > SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer), > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val); > =20 > - /* Set the alpha configuration */ > - regmap_update_bits(mixer->engine.regs, > - SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer), > - SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK, > - SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_DEF); > + if (enable) > + val =3D SUN8I_MIXER_BLEND_PIPE_CTL_EN(chan); > + else > + val =3D 0; > + > regmap_update_bits(mixer->engine.regs, > - SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer), > - SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK, > - SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_DEF); > + SUN8I_MIXER_BLEND_PIPE_CTL, > + SUN8I_MIXER_BLEND_PIPE_CTL_EN(chan), val); =2E.. For example, this part right here will remove the alpha setup part, without any justification in the commit log ... > } > =20 > static int sun8i_mixer_drm_format_to_layer(struct drm_plane *plane, > @@ -71,15 +71,15 @@ static int sun8i_mixer_drm_format_to_layer(struct drm= _plane *plane, > { > switch (format) { > case DRM_FORMAT_ARGB8888: > - *mode =3D SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_ARGB8888; > + *mode =3D SUN8I_MIXER_FBFMT_ARGB8888; > break; > =20 > case DRM_FORMAT_XRGB8888: > - *mode =3D SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_XRGB8888; > + *mode =3D SUN8I_MIXER_FBFMT_XRGB8888; > break; > =20 > case DRM_FORMAT_RGB888: > - *mode =3D SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_RGB888; > + *mode =3D SUN8I_MIXER_FBFMT_RGB888; > break; > =20 > default: > @@ -107,7 +107,7 @@ int sun8i_mixer_update_layer_coord(struct sun8i_mixer= *mixer, > state->crtc_h)); > DRM_DEBUG_DRIVER("Updating blender size\n"); > regmap_write(mixer->engine.regs, > - SUN8I_MIXER_BLEND_ATTR_INSIZE(0), > + SUN8I_MIXER_BLEND_ATTR_INSIZE(chan), I guess that one is fixing a bug too. > SUN8I_MIXER_SIZE(state->crtc_w, > state->crtc_h)); > regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_OUTSIZE, > @@ -173,6 +173,7 @@ int sun8i_mixer_update_layer_formats(struct sun8i_mix= er *mixer, > return ret; > } > =20 > + val <<=3D SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_OFFSET; > regmap_update_bits(mixer->engine.regs, > SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer), > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_MASK, val); > @@ -247,6 +248,7 @@ static int sun8i_mixer_bind(struct device *dev, struc= t device *master, > struct sun8i_mixer *mixer; > struct resource *res; > void __iomem *regs; > + int plane_cnt; > int i, ret; > =20 > /* > @@ -325,27 +327,26 @@ static int sun8i_mixer_bind(struct device *dev, str= uct device *master, > regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL, > SUN8I_MIXER_GLOBAL_CTL_RT_EN); > =20 > - /* Initialize blender */ > - regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_FCOLOR_CTL, > - SUN8I_MIXER_BLEND_FCOLOR_CTL_DEF); > - regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PREMULTIPLY, > - SUN8I_MIXER_BLEND_PREMULTIPLY_DEF); > + /* Set background color to black */ > regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_BKCOLOR, > - SUN8I_MIXER_BLEND_BKCOLOR_DEF); > - regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_MODE(0), > - SUN8I_MIXER_BLEND_MODE_DEF); > - regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_CK_CTL, > - SUN8I_MIXER_BLEND_CK_CTL_DEF); > + SUN8I_MIXER_BLEND_COLOR_BLACK); > =20 > - regmap_write(mixer->engine.regs, > - SUN8I_MIXER_BLEND_ATTR_FCOLOR(0), > - SUN8I_MIXER_BLEND_ATTR_FCOLOR_DEF); > - > - /* Select the first UI channel */ > - DRM_DEBUG_DRIVER("Selecting channel %d (first UI channel)\n", > - mixer->cfg->vi_num); > - regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ROUTE, > - mixer->cfg->vi_num); > + /* > + * Set fill color of bottom plane to black. Generally not needed > + * except when VI plane is at bottom (zpos =3D 0) and enabled. > + */ > + regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_PIPE_CTL, > + SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(0)); > + regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(0), > + SUN8I_MIXER_BLEND_COLOR_BLACK); > + > + /* Fixed zpos for now */ > + regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ROUTE, 0x43210); > + > + plane_cnt =3D mixer->cfg->vi_num + mixer->cfg->ui_num; > + for (i =3D 0; i < plane_cnt; i++) > + regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_MODE(i), > + SUN8I_MIXER_BLEND_MODE_DEF); You're reworking a significant part here as well, with some part moving (or being removed) and no clear justifications as to why you need to do that. This is not only an issue when you want to review this code, but also if you happen to introduce a bug, then someone bisects and finds that commit. It's quite difficult in that case what part is actually breaking stuff. Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --flxkqbb5q46lcspl Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEE0VqZU19dR2zEVaqr0rTAlCFNr3QFAlodhr8ACgkQ0rTAlCFN r3QpZBAAiHPIm0qpsdtBWhpgWpgrwvQb9hFCS/u/UpTyMwW1m75YydGnO+cMDeM4 cfbalP5ULiyGKj3IsEwr3tgOvmvtyYa2ry2JzZxMvrnK0vw5JKS9OzlBexdbI4Q7 vAzYOQ7uEC/+2XGdQYe6dkacA91vUja3gBz98Thd6zp6XGDh7gZq2PwkBfLjV/BV 2Ipo/9iYkTYh5s6yKU4f5JQFw6jpUHR1HRNo4plplFFOd2lpZfVtBk183JvcTFCv PSYRvgfEpVum7p7hBbmStgfkBWnazkfIL3RC1paq3bv1uB76KNFh7egc9rQVE2e3 qY6OY0HnbO9LShdx3ivgNpl3L30jEbfrShLF1BQCbgoextmx43WHapHE0DbR6jGO dJIet+mogyV2W7NOFl7FZLJpeZM+XNoWMi5GEYW4+Y5EFyF3L5jWO0BhQ5Zpm2rl vCAHnRvcVicCgoC8XnBeEA+LWYQWnMy/TmFQC5SAEuamtrgZy0KfZkTYTNeTA47e 0o30GDVvJXy1kiZGdt+bUAbVBq41halu8/8WfNw+DYNLiHMa0Fkn39QUBqur9ZOx G9JWiu6H833Y61qXrSPmL91UBMPLL+yD2yPsQnPVy60MtmmN9aJvD0nGtER+Q8s+ je4K2p243LoRI+dBcmux/VO4jdxEGFYtkq+6Yng6EMLZYgStfyM= =dtwh -----END PGP SIGNATURE----- --flxkqbb5q46lcspl--