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