Re: [PATCH 2/3] drm: lcdif: Use dedicated set/clr registers for polarity/edge

[Lucas Stach]

Hi Paul,

Am Dienstag, dem 31.03.2026 um 00:46 +0200 schrieb Paul Kocialkowski:
> The lcdif v3 hardware comes with dedicated registers to set and clear
> polarity bits in the CTRL register. It is unclear if there is a
> difference with writing to the CTRL register directly.
> 
> Follow the NXP BSP reference by using these registers, in case there is
> a subtle difference caused by using them.
> 
I don't really like that patch, as it blows up what is currently a
single register access to three separate ones. If there is no clear
benefit (as in it has been shown to fix any issue), I would prefer this
code to stay as-is.

Regards,
Lucas

> Signed-off-by: Paul Kocialkowski <paulk@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_kms.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index a00c4f6d63f4..1aac354041c7 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -296,18 +296,27 @@ static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
>  static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
>  {
>  	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
> -	u32 ctrl = 0;
> +	u32 ctrl;
>  
>  	if (m->flags & DRM_MODE_FLAG_NHSYNC)
> -		ctrl |= CTRL_INV_HS;
> +		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_SET);
> +	else
> +		writel(CTRL_INV_HS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
> +
>  	if (m->flags & DRM_MODE_FLAG_NVSYNC)
> -		ctrl |= CTRL_INV_VS;
> +		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_SET);
> +	else
> +		writel(CTRL_INV_VS, lcdif->base + LCDC_V8_CTRL + REG_CLR);
> +
>  	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
> -		ctrl |= CTRL_INV_DE;
> -	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> -		ctrl |= CTRL_INV_PXCK;
> +		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_SET);
> +	else
> +		writel(CTRL_INV_DE, lcdif->base + LCDC_V8_CTRL + REG_CLR);
>  
> -	writel(ctrl, lcdif->base + LCDC_V8_CTRL);
> +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> +		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_SET);
> +	else
> +		writel(CTRL_INV_PXCK, lcdif->base + LCDC_V8_CTRL + REG_CLR);
>  
>  	writel(DISP_SIZE_DELTA_Y(m->vdisplay) |
>  	       DISP_SIZE_DELTA_X(m->hdisplay),