Re: [PATCH 17/18] drm: rockchip: Add VOP2 driver

From: Sascha Hauer <s.hauer@pengutronix.de>
To: Johan Jonker <jbx6244@gmail.com>
Cc: dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org,
	kernel@pengutronix.de, "Andy Yan" <andy.yan@rock-chips.com>,
	"Benjamin Gaignard" <benjamin.gaignard@collabora.com>,
	"Michael Riesch" <michael.riesch@wolfvision.net>,
	"Sandy Huang" <hjc@rock-chips.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Peter Geis" <pgwipeout@gmail.com>
Subject: Re: [PATCH 17/18] drm: rockchip: Add VOP2 driver
Date: Fri, 10 Dec 2021 09:55:36 +0100	[thread overview]
Message-ID: <20211210085536.GC25513@pengutronix.de> (raw)
In-Reply-To: <bd576949-a42b-9750-fe97-b9b567c7ea75@gmail.com>

Hi Johan,

On Wed, Dec 08, 2021 at 05:59:16PM +0100, Johan Jonker wrote:
> Hi,
> 
> On 12/8/21 4:12 PM, Sascha Hauer wrote:
> > From: Andy Yan <andy.yan@rock-chips.com>
> > 
> > The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568.
> > It replaces the VOP unit found in the older Rockchip SoCs.
> > 
> > This driver has been derived from the downstream Rockchip Kernel and
> > heavily modified:
> > 
> > - All nonstandard DRM properties have been removed
> > - dropped struct vop2_plane_state and pass around less data between
> >   functions
> > - Dropped all DRM_FORMAT_* not known on upstream
> > - rework register access to get rid of excessively used macros
> > - Drop all waiting for framesyncs
> > 
> > The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB
> > board. Overlay support is tested with the modetest utility. AFBC support
> > on the cluster windows is tested with weston-simple-dmabuf-egl on
> > weston using the (yet to be upstreamed) panfrost driver support.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> 
> [..]
> 
> > +
> > +static const struct of_device_id vop2_dt_match[] = {
> > +	{
> > +		.compatible = "rockchip,rk3568-vop",
> > +		.data = &rk3568_vop
> > +	}, {
> 
> > +		.compatible = "rockchip,rk3568-vop",
> 
> Maybe use:
> .compatible = "rockchip,rk3566-vop",

Copy/Paste bug. Will fix.

> 
> > +		.data = &rk3566_vop
> > +	}, {
> > +	},
> 
> Maybe sort this list alphabetical based on compatible in case later more
> SoCs are added.
> 
> rk3566
> rk3568

Ok.

> 
> ===
> 
> The structure layout size above could be reduced for if we get more
> compatible strings additions.
> 
> Example vop1:
> 
> static const struct of_device_id vop_driver_dt_match[] = {
> 	{ .compatible = "rockchip,rk3036-vop",
> 	  .data = &rk3036_vop },
> 	{ .compatible = "rockchip,rk3126-vop",
> 	  .data = &rk3126_vop },
> 	{ .compatible = "rockchip,px30-vop-big",
> 	  .data = &px30_vop_big },
> 	{ .compatible = "rockchip,px30-vop-lit",
> 	  .data = &px30_vop_lit },
> 	{ .compatible = "rockchip,rk3066-vop",
> 	  .data = &rk3066_vop },
> 	{ .compatible = "rockchip,rk3188-vop",
> 	  .data = &rk3188_vop },
> 	{ .compatible = "rockchip,rk3288-vop",
> 	  .data = &rk3288_vop },
> 	{ .compatible = "rockchip,rk3368-vop",
> 	  .data = &rk3368_vop },
> 	{ .compatible = "rockchip,rk3366-vop",
> 	  .data = &rk3366_vop },
> 	{ .compatible = "rockchip,rk3399-vop-big",
> 	  .data = &rk3399_vop_big },
> 	{ .compatible = "rockchip,rk3399-vop-lit",
> 	  .data = &rk3399_vop_lit },
> 	{ .compatible = "rockchip,rk3228-vop",
> 	  .data = &rk3228_vop },
> 	{ .compatible = "rockchip,rk3328-vop",
> 	  .data = &rk3328_vop },
> 	{},

It's shorter, but ugly ;)
That's only my personal taste though, I don't care much.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel