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

[Jason-JH Lin <jason-jh.lin@mediatek.com>]

Hi Fei,

Thanks for the reviews.

On Mon, 2021-10-25 at 13:05 +0800, Fei Shao wrote:
> On Fri, Oct 22, 2021 at 6:13 PM Jason-JH Lin <
> jason-jh.lin@mediatek.com> wrote:
> > 
> > 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?
> 
> Hi Jason,
> 
> If that's the case you can still use BIT(nr) for the definitions and
> describe their usage in the comment, so both code readability and the
> ease of maintenance are preserved, and people can easily tell if
> there
> are duplicated/missing definitions while reading through the code.
> Adding informative comments is never a bad thing.
> 
> I would do something like this (and further split the definitions
> into
> sections by their functionalities with blank lines for visual
> comfort):
> 
> /*
>  * MT8195_VDO0_SEL_IN[1:0]: VPP_MERGE
>  *   0x0 : DSC_WRAP0_OUT
>  *   0x1 : DISP_DITHER1
>  *   0x10: VDO1_VIRTUAL0
>  */
> #define MT8195_SEL_IN_VPP_MERGE_FROM_DSC_WRAP0_OUT           0
> #define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1            BIT(0)
> #define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0           BIT(1)
> 
> /*
>  * MT8195_VDO0_SEL_IN[4:4]: DSC_WRAP0_IN
>  *   0x0: DISP_DITHER0
>  *   0x1: VPP_MERGE
>  */
> #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_DISP_DITHER0         0
> #define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE            BIT(4)
> ... and so on.
> 
> Regards,
> Fei
> 

OK, I'll fix it.

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