Hi Angelo,

Thanks for the reviews.


On Thu, 2021-10-14 at 16:05 +0200, AngeloGioacchino Del Regno wrote:

quoted

Add mt8195 vdosys0 clock driver name and routing table to
the driver data of mtk-mmsys.

[snip]

quoted

 
---

Hello Jason,
thanks for the patch! However, there are a few things to improve:

[snip]

quoted

+#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.

quoted

+#define MT8195_SEL_IN_VPP_MERGE_FROM_DISP_DITHER1		(1 <<
0)

I would write 0x1 here

quoted

+#define MT8195_SEL_IN_VPP_MERGE_FROM_VDO1_VIRTUAL0		(2 <<
0)

....and 0x2 here: bitshifting of 0 bits makes little sense.

quoted

+#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.

quoted

+#define MT8195_SEL_IN_DSC_WRAP0_IN_FROM_VPP_MERGE		(1 <<
4)

This is BIT(4).

quoted

+#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)

quoted

+#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)

quoted

+#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)

quoted

+#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 [off-list ref]