linux-kernel - Re: [PATCH v3 1/7] drm/mediatek: dsi: Use GENMASK() for register mask definitions

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] [day] [month] [year] [list]

Message-ID: <c10b8511f7d539cda9bcd941d5d48b253e8407e6.camel@mediatek.com>
Date: Wed, 7 Feb 2024 08:21:26 +0000
From: CK Hu (胡俊光) <ck.hu@...iatek.com>
To: "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>, "chunkuang.hu@...nel.org"
	<chunkuang.hu@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"kernel@...labora.com" <kernel@...labora.com>, "daniel@...ll.ch"
	<daniel@...ll.ch>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "airlied@...il.com"
	<airlied@...il.com>, "matthias.bgg@...il.com" <matthias.bgg@...il.com>
Subject: Re: [PATCH v3 1/7] drm/mediatek: dsi: Use GENMASK() for register mask
 definitions

Hi, Angelo:

On Tue, 2024-02-06 at 14:27 +0100, AngeloGioacchino Del Regno wrote:
> Il 06/02/24 09:57, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Wed, 2024-01-31 at 12:34 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > Change magic numerical masks with usage of the GENMASK() macro
> > > to improve readability.
> > > 
> > > While at it, also fix the DSI_PS_SEL mask to include all bits
> > > instead
> > > of just a subset of them.
> > > 
> > > This commit brings no functional changes.
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@...labora.com>
> > > ---
> > >   drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++------
> > > -------
> > > --
> > >   1 file changed, 23 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > index a2fdfc8ddb15..3b7392c03b4d 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > > @@ -58,18 +58,18 @@
> > >   
> > >   #define DSI_TXRX_CTRL		0x18
> > >   #define VC_NUM				BIT(1)
> > > -#define LANE_NUM			(0xf << 2)
> > > +#define LANE_NUM			GENMASK(5, 2)
> > >   #define DIS_EOT				BIT(6)
> > >   #define NULL_EN				BIT(7)
> > >   #define TE_FREERUN			BIT(8)
> > >   #define EXT_TE_EN			BIT(9)
> > >   #define EXT_TE_EDGE			BIT(10)
> > > -#define MAX_RTN_SIZE			(0xf << 12)
> > > +#define MAX_RTN_SIZE			GENMASK(15, 12)
> > >   #define HSTX_CKLP_EN			BIT(16)
> > >   
> > >   #define DSI_PSCTRL		0x1c
> > > -#define DSI_PS_WC			0x3fff
> > > -#define DSI_PS_SEL			(3 << 16)
> > > +#define DSI_PS_WC			GENMASK(14, 0)
> > > +#define DSI_PS_SEL			GENMASK(19, 16)
> > 
> > The original definition of DSI_PS_WC/DSI_PS_SEL is correct in
> > MT8173.
> > So both need two definition and let each SoC select its own
> > definition.
> > 
> 
> The additional bits are unused on older SoCs and, if set, will be
> simply ignored;
> if we want to prevent setting bits that don't exist on the old ones,
> that should
> be done as a later commit introducing SoC capabilities for those and
> when the new
> capabilities for the new SoCs are introduced anyway.
> 
> As of now, this doesn't break anything.

The title of this patch is only to use GENMASK(), but here does more
things. I agree this does not break anything, but I would like to
separate this to an independent patch just for new bits. In your later
patch, DSI_PS_WC is not used any more. So maybe after that patch, you
could define as:

#define DSI_PS_WC_MT8173 GENMASK(13, 0)
#define DSI_PS_WC_MT8xxx GENMASK(14, 0)

DSI_PS_SEL is not used now, so it could also define as:

#define DSI_PS_SEL_MT8137 GENMASK(17, 16)
#define DSI_PS_SEL_MT8xxx GENMASK(19, 16)

And add definition of value 4 ~ 15.

Regards,
CK

> 
> Regards,
> Angelo
> 
>