-----Original Message----- From: Laurent Pinchart laurent.pinchart@ideasonboard.com Sent: Wednesday, June 3, 2020 7:29 AM To: Emil Velikov emil.l.velikov@gmail.com Cc: Sandor Yu sandor.yu@nxp.com; Andrzej Hajda a.hajda@samsung.com; Neil Armstrong narmstrong@baylibre.com; Jonas Karlman jonas@kwiboo.se; Jernej Skrabec jernej.skrabec@siol.net; Heiko Stübner heiko@sntech.de; Sandy Huang hjc@rock-chips.com; dkos@cadence.com; ML dri-devel dri-devel@lists.freedesktop.org; linux-rockchip linux-rockchip@lists.infradead.org; Linux-Kernel@Vger. Kernel. Org linux-kernel@vger.kernel.org; LAKML linux-arm-kernel@lists.infradead.org; dl-linux-imx linux-imx@nxp.com Subject: [EXT] Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver
Caution: EXT Email
On Tue, Jun 02, 2020 at 02:55:52PM +0100, Emil Velikov wrote:
On Mon, 1 Jun 2020 at 07:29, sandor.yu@nxp.com wrote:
From: Sandor Yu Sandor.yu@nxp.com
- Extracted common fields from cdn_dp_device to a new
cdns_mhdp_device
structure which will be used by two separate drivers later on.
- Moved some datatypes (audio_format, audio_info,
vic_pxl_encoding_format,
video_info) from cdn-dp-core.c to cdn-dp-reg.h.
- Changed prefixes from cdn_dp to cdns_mhdp cdn -> cdns to match the other Cadence's drivers dp -> mhdp to distinguish it from a "just a DP" as the IP underneath this registers map can be a HDMI (which is internally different, but the interface for commands, events is pretty much the same).
- Modified cdn-dp-core.c to use the new driver structure and new function names.
- writel and readl are replaced by cdns_mhdp_bus_write and cdns_mhdp_bus_read.
The high-level idea is great - split, refactor and reuse the existing drivers.
Although looking at the patches themselves - they seems to be doing multiple things at once. As indicated by the extensive list in the commit log.
I would suggest splitting those up a bit, roughly in line of the itemisation as per the commit message.
Here is one hand wavy way to chunk this patch:
- use put_unalligned*
- 'use local variable dev' style of changes (as seem in
cdn_dp_clk_enable) 3) add writel/readl wrappers 4) hookup struct cdns_mhdp_device, keep dp->mhdp detail internal. The cdn-dp-reg.h function names/signatures will stay the same. 5) finalize the helpers - use mhdp directly, rename
I second this, otherwise review is very hard.
I will split the patch later, thanks.
Examples: 4) static int cdn_dp_mailbox_read(struct cdn_dp_device *dp) { +" struct cdns_mhdp_device *mhdp = dp->mhdp; int val, ret;
- ret = readx_poll_timeout(readl, dp->regs + MAILBOX_EMPTY_ADDR,
- ret = readx_poll_timeout(readl, mhdp->regs_base +
- MAILBOX_EMPTY_ADDR,
... return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff; }
-static int cdn_dp_mailbox_read(struct cdn_dp_device *dp) +static int mhdp_mailbox_read(struct cdns_mhdp_device *mhdp) {
- struct cdns_mhdp_device *mhdp = dp->mhdp; int val, ret;
...
- return fancy_readl(dp, MAILBOX0_RD_DATA) & 0xff;
- return cdns_mhdp_bus_read(mhdp, MAILBOX0_RD_DATA) & 0xff;
}
-- Regards,
Laurent Pinchart