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

From: Jason-JH Lin
Date: Mon Oct 25 2021 - 01:33:30 EST


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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >
--
Jason-JH Lin <jason-jh.lin@xxxxxxxxxxxx>