RE: [EXT] Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns and rk dpi/dp driver

[Sandor Yu]

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, June 3, 2020 7:29 AM
> To: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> Cc: Sandor Yu <sandor.yu@xxxxxxx>; Andrzej Hajda <a.hajda@xxxxxxxxxxx>;
> Neil Armstrong <narmstrong@xxxxxxxxxxxx>; Jonas Karlman
> <jonas@xxxxxxxxx>; Jernej Skrabec <jernej.skrabec@xxxxxxxx>; Heiko StÃbner
> <heiko@xxxxxxxxx>; Sandy Huang <hjc@xxxxxxxxxxxxxx>;
> dkos@xxxxxxxxxxx; ML dri-devel <dri-devel@xxxxxxxxxxxxxxxxxxxxx>;
> linux-rockchip <linux-rockchip@xxxxxxxxxxxxxxxxxxx>; Linux-Kernel@Vger.
> Kernel. Org <linux-kernel@xxxxxxxxxxxxxxx>; LAKML
> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>
> 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@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.
> 
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