Re: [PATCH 3/9] drm/rcar-du: dsi: Clean up VCLKSET register macros

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Marek,

Thank you for the patch.

On Mon, Sep 22, 2025 at 08:54:59PM +0200, Marek Vasut wrote:
> Introduce VCLKSET_BPP_MASK macro and use FIELD_PREP() to generate
> appropriate bitfield from mask and value without bitshift. Remove
> VCLKSET_COLOR_RGB which is never used, replace it with code comment.
> 
> Do not convert bits and bitfields to BIT() and GENMASK() yet, to be
> consisten with the current style. Conversion to BIT() and GENMASK()
> macros is done at the very end of this series in the last two patches.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: David Airlie <airlied@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> NOTE: No functional change expected, this is a preparatory patch which
> partly removes macros which evaluate to zeroes from rcar_mipi_dsi_regs.h .
> The other patches in this series proceed with that job, piece by piece,
> to make it all reviewable.
> ---
>  drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c      |  9 +++++----
>  drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h | 12 ++++++------
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> index a550bda6debbe..2374cbe3768f2 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> @@ -621,18 +621,19 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>  	vclkset = VCLKSET_CKEN;
>  	rcar_mipi_dsi_write(dsi, VCLKSET, vclkset);
>  
> +	/* Output is always RGB, never YCbCr */
>  	if (dsi_format == 24)
> -		vclkset |= VCLKSET_BPP_24;
> +		vclkset |= FIELD_PREP(VCLKSET_BPP_MASK, VCLKSET_BPP_24);
>  	else if (dsi_format == 18)
> -		vclkset |= VCLKSET_BPP_18;
> +		vclkset |= FIELD_PREP(VCLKSET_BPP_MASK, VCLKSET_BPP_18);
>  	else if (dsi_format == 16)
> -		vclkset |= VCLKSET_BPP_16;
> +		vclkset |= FIELD_PREP(VCLKSET_BPP_MASK, VCLKSET_BPP_16);

I personally find this less readable.

>  	else {
>  		dev_warn(dsi->dev, "unsupported format");
>  		return -EINVAL;
>  	}
>  
> -	vclkset |= VCLKSET_COLOR_RGB | VCLKSET_LANE(dsi->lanes - 1);
> +	vclkset |= VCLKSET_LANE(dsi->lanes - 1);
>  
>  	switch (dsi->info->model) {
>  	case RCAR_DSI_V3U:
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> index dd871e17dcf53..c2cb06ef144ed 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
> @@ -245,14 +245,14 @@
>  
>  #define VCLKSET				0x100c
>  #define VCLKSET_CKEN			(1 << 16)
> -#define VCLKSET_COLOR_RGB		(0 << 8)
> -#define VCLKSET_COLOR_YCC		(1 << 8)
> +#define VCLKSET_COLOR_YCC		(1 << 8) /* 0:RGB 1:YCbCr */

This I like.

>  #define VCLKSET_DIV_V3U(x)		(((x) & 0x3) << 4)
>  #define VCLKSET_DIV_V4H(x)		(((x) & 0x7) << 4)
> -#define VCLKSET_BPP_16			(0 << 2)
> -#define VCLKSET_BPP_18			(1 << 2)
> -#define VCLKSET_BPP_18L			(2 << 2)
> -#define VCLKSET_BPP_24			(3 << 2)
> +#define VCLKSET_BPP_MASK		(3 << 2)
> +#define VCLKSET_BPP_16			0
> +#define VCLKSET_BPP_18			1
> +#define VCLKSET_BPP_18L			2
> +#define VCLKSET_BPP_24			3

If you'd rather get rid of the shifts, why not

#define VCLKSET_BPP_16			FIELD_PREP(VCLKSET_BPP_MASK, 0)
#define VCLKSET_BPP_18			FIELD_PREP(VCLKSET_BPP_MASK, 1)
#define VCLKSET_BPP_18L			FIELD_PREP(VCLKSET_BPP_MASK, 2)
#define VCLKSET_BPP_24			FIELD_PREP(VCLKSET_BPP_MASK, 3)

without touching the users ? Personally I'd keep the shifts though.

>  #define VCLKSET_LANE(x)			(((x) & 0x3) << 0)
>  
>  #define VCLKEN				0x1010

-- 
Regards,

Laurent Pinchart