linux-kernel - Re: [PATCH v11 09/16] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0

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: <29992126d39a7f381a516fdb9cd6e39f1e51afdb.camel@mediatek.com>
Date:   Fri, 22 Oct 2021 18:13:22 +0800
From:   Jason-JH Lin <jason-jh.lin@...iatek.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        "Rob Herring" <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>
CC:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        <hsinyi@...omium.org>, <fshao@...omium.org>,
        <moudy.ho@...iatek.com>, <roy-cw.yeh@...iatek.com>,
        Fabien Parent <fparent@...libre.com>,
        "Yongqiang Niu" <yongqiang.niu@...iatek.com>,
        <nancy.lin@...iatek.com>, <singo.chang@...iatek.com>,
        <devicetree@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v11 09/16] soc: mediatek: add mtk-mmsys support for
 mt8195 vdosys0

Hi Angelo,

Thanks for the reviews.


On Thu, 2021-10-14 at 16:05 +0200, AngeloGioacchino Del Regno wrote:
> > Add mt8195 vdosys0 clock driver name and routing table to
> > the driver data of mtk-mmsys.
> > 

[snip]

> >  
> > ---
> 
> Hello Jason,
> thanks for the patch! However, there are a few things to improve:
> 

[snip]

> > +#define MT8195_VDO0_SEL_IN					0xf34
> > +#define MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT		(0 <<
> > 0)
> 
> Bitshifting 0 by 0 bits == 0, so this is simply 0.
> 
> > +#define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1		(1 <<
> > 0)
> 
> I would write 0x1 here
> 
> > +#define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0		(2 <<
> > 0)
> 
> ....and 0x2 here: bitshifting of 0 bits makes little sense.
> 
> > +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0		
> > (0 << 4)
> 
> Bitshifting 0 by 4 bits is still 0, so this is again 0.
> This is repeated too many times, so I will not list it for all of the
> occurrences.
> 
> > +#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE		(1 <<
> > 4)
> 
> This is BIT(4).
> 
> > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1		
> > (0 << 5) > +#define MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE	
> > 	(1 << 5)
> 
> ...and this is BIT(5)
> 
> > +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_VPP_MERGE		(0 <<
> > 8)
> > +#define MT8195_SEL_IN_SINA_VIRTUAL0_FROM_DSC_WRAP1_OUT		
> > (1 << 8)
> 
> BIT(8)
> 
> > +#define MT8195_SEL_IN_SINB_VIRTUAL0_FROM_DSC_WRAP0_OUT		
> > (0 << 9)
> > +#define MT8195_SEL_IN_DP_INTF0_FROM_DSC_WRAP1_OUT		(0 <<
> > 12)
> > +#define MT8195_SEL_IN_DP_INTF0_FROM_VPP_MERGE			
> > (1 << 12)
> 
> BIT(12)
> 
> > +#define MT8195_SEL_IN_DP_INTF0_FROM_VDO1_VIRTUAL0		(2 <<
> > 12)
> 
> BIT(13)
> 
> ... and please, use the BIT(nr) macro for all these bit definitions,
> it's way more
> readable like that.
> 
> Regards,
> - Angelo

Because the HW register design of MT8195_VDO0_SEL_IN 0xf34 is like
this:

bit[1:0] as MT8195_SEL_IN_VPP_MERGE and
  value: 0 as MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT
  value: 1 as MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1
  value: 2 as MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0
bit[4:4] as MT8195_SEL_IN_DSC_WRAP0_IN and
  value 0 as MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0
  value 1 as MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE
bit[5:5] as MT8195_SEL_IN_DSC_WRAP1_IN and
  value 0 as
MT8195_SEL_IN_DSC_WRAP1_IN_FROM_DISP_DITHER1
  value 1 as
MT8195_SEL_IN_DSC_WRAP1_IN_FROM_VPP_MERGE
and so on...

I think using BIT(nr) macro directly is not easy to debug.


Is it better to define another MACRO like this?

#define BIT_VAL(val, bit)  ((val) << (bit))
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0  BIT_VAL(0, 4)
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE  BIT_VAL(1, 4)
...

or

#define MT8195_SEL_IN_DSC_WRAP0_IN (4)
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0  (0
<< MT8195_SEL_IN_DSC_WRAP0_IN)
#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE  (1 <<
MT8195_SEL_IN_DSC_WRAP0_IN)
...

What do you think?


Regards,
Jason-JH Lin <jason-jh.lin@...iatek.com>