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

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 3463 bytes --]

Hi Lucas,

Le Tue 31 Mar 26, 11:09, Lucas Stach a écrit :
> 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.

Well I guess the cost of a few writes vs a single one is rather
negligible. I'm rather worried that there might be an undocumented
reason why these registers exist in the first place and why they are
used in the BSP.

But yes this is only speculation and I could not witness any actual
issue. My setup (lcdif3 with hdmi) uses all positive polarities which is
the default state, so not a good way to check.

It would be great if somebody from NXP could confirm whether this is
needed or not. In the meantime I guess we can drop the patch. It'll stay
on the list in case someone has polarity issues later :)

All the best,

Paul

> Regards,
> Lucas
> 
> > Signed-off-by: Paul Kocialkowski <paulk@sys-base.io>
> > ---
> >  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),
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]