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@xxxxxxxxxxxx>
