> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Wednesday, June 3, 2020 7:29 AM
> To: Emil Velikov <[email protected]>
> Cc: Sandor Yu <[email protected]>; Andrzej Hajda <[email protected]>;
> Neil Armstrong <[email protected]>; Jonas Karlman
> <[email protected]>; Jernej Skrabec <[email protected]>; Heiko Stübner
> <[email protected]>; Sandy Huang <[email protected]>;
> [email protected]; ML dri-devel <[email protected]>;
> linux-rockchip <[email protected]>; Linux-Kernel@Vger.
> Kernel. Org <[email protected]>; LAKML
> <[email protected]>; dl-linux-imx <[email protected]>
> 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, <[email protected]> wrote:
> > >
> > > From: Sandor Yu <[email protected]>
> > >
> > > - 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:
> > 1) use put_unalligned*
> > 2) '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; }
> >
> > 5)
> > -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
