lkml.org 
[lkml]   [2021]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v11 09/16] soc: mediatek: add mtk-mmsys support for mt8195 vdosys0
From
Date
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@mediatek.com>

\
 
 \ /
  Last update: 2021-10-22 12:13    [W:0.182 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site