RE: [PATCH v11 3/4] drm: renesas: Add RZ/G2L DU Support

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>,
	Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Cc: David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Magnus Damm <magnus.damm@gmail.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	Fabrizio Castro <fabrizio.castro.jz@renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: RE: [PATCH v11 3/4] drm: renesas: Add RZ/G2L DU Support
Date: Thu, 12 Oct 2023 09:44:01 +0000	[thread overview]
Message-ID: <TYCPR01MB112698FFB7FBFF2131BF4B94A86D3A@TYCPR01MB11269.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdWC5Nd3nezR1K_XD5CJTM76XUbv3=Pf9fHvYK43n4iqmQ@mail.gmail.com>

Hi Geert Uytterhoeven,

Thanks for the feedback.

> Subject: Re: [PATCH v11 3/4] drm: renesas: Add RZ/G2L DU Support
> 
> Hi Biju,
> 
> On Mon, Oct 2, 2023 at 2:28 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > Video Signal Processor (VSPD), and Display Unit (DU).
> >
> > It has DPI/DSI interfaces and supports a maximum resolution of 1080p
> > along with 2 RPFs to support the blending of two picture layers and
> > raster operations (ROPs).
> >
> > The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L
> > alike SoCs.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > v9->v10:
> 
> >  * Added rzg2l_du_write() wrapper function.
> 
> > --- /dev/null
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> 
> > +static inline void rzg2l_du_write(struct rzg2l_du_device *rcdu, u32
> > +reg, u32 data) {
> > +       writel(data, rcdu->mmio + reg); }
> 
> What is the added value of this wrapper?

I think, for debugging we can add some prints here and check reg values. Other than I don't see
any benefit here. Laurent/ Jacopo please confirm.

> The order of the reg/data parameters is the inverse of the standard
> writel() operation...

OK.

> 
> > +       rzg2l_du_write(rcdu, DU_DITR0, ditr0);
> 
> ... and using it actually needs one more keystroke than open-coding it:
> 
> -       writel(ditr0, rcdu->mmio + DU_DITR0);
> 
> Sorry for missing this before.

I have changed this based on review comment from Laurent and Jacopo. If the wrapper
is not adding value, I am happy to use writel instead.

Please confirm.

Cheers,
Biju