On Tue, Jun 02, 2020 at 02:55:52PM +0100, Emil Velikov wrote:
> On Mon, 1 Jun 2020 at 07:29, <sandor.yu@xxxxxxx> wrote:
> >
> > From: Sandor Yu <Sandor.yu@xxxxxxx>
> >
> > - 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.
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
