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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB8PR04MB682817AED57AF25816BF9147F4890@DB8PR04MB6828.eurprd04.prod.outlook.com>
Date:   Thu, 4 Jun 2020 09:28:18 +0000
From:   Sandor Yu <sandor.yu@....com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Emil Velikov <emil.l.velikov@...il.com>
CC:     Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Heiko Stübner <heiko@...ech.de>,
        Sandy Huang <hjc@...k-chips.com>,
        "dkos@...ence.com" <dkos@...ence.com>,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-rockchip <linux-rockchip@...ts.infradead.org>,
        "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        LAKML <linux-arm-kernel@...ts.infradead.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [EXT] Re: [PATCH 1/7] drm/rockchip: prepare common code for cdns
 and rk dpi/dp driver



> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Sent: Wednesday, June 3, 2020 7:29 AM
> To: Emil Velikov <emil.l.velikov@...il.com>
> Cc: Sandor Yu <sandor.yu@....com>; Andrzej Hajda <a.hajda@...sung.com>;
> Neil Armstrong <narmstrong@...libre.com>; Jonas Karlman
> <jonas@...boo.se>; Jernej Skrabec <jernej.skrabec@...l.net>; Heiko Stübner
> <heiko@...ech.de>; Sandy Huang <hjc@...k-chips.com>;
> dkos@...ence.com; ML dri-devel <dri-devel@...ts.freedesktop.org>;
> linux-rockchip <linux-rockchip@...ts.infradead.org>; Linux-Kernel@...r.
> Kernel. Org <linux-kernel@...r.kernel.org>; LAKML
> <linux-arm-kernel@...ts.infradead.org>; dl-linux-imx <linux-imx@....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@....com> wrote:
> > >
> > > From: Sandor Yu <Sandor.yu@....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:
> >  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