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

[Fei Shao <fshao@chromium.org>]

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

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