On 8/13/25 9:42 AM, Tomi Valkeinen wrote:
Hi,
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h b/
drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
index a6b276f1d6ee..b3e57217ae63 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi_regs.h
[...]
@@ -51,11 +51,11 @@
#define TXVMVPRMSET0R 0x1d0
#define TXVMVPRMSET0R_HSPOL_HIG (0 << 17)
-#define TXVMVPRMSET0R_HSPOL_LOW (1 << 17)
+#define TXVMVPRMSET0R_HSPOL_LOW BIT(17)
I'm not sure about this (and below). We have two defines for the HSPOL,
high and low. If one of them is (x << y), shouldn't the other one be of
that style too?
It is inconsistent, but one macro describes bit set to 0 and the other
bit set to 1 (i.e. the actual bit) which is converted to BIT(n) macro. I
would be tempted to remove the bits set to 0, that's probably the real
discussion that should happen here. But that would also be a much bigger
patch. What do you think ?
In my mind if you have defines for both HIGH and LOW, you have a
bitfield with a value, the values being 0 and 1, and for values you use
(0 << 17) and (1 << 17). It just happens here that the bitfield value is
only one bit long.
I am not a big fan of that, it seems overcomplicated, hence this clean up.
But I'm also fine with having only "TXVMVPRMSET0R_HSPOL_LOW
BIT(17)", and then the interpretation is that we have a enable/disable
style bit.
I think this would work, yes.
In the end, I'm fine with any of these, or the current one in the patch.
Although the current one does rub me the wrong way just enough for me to
comment about it =).
I can also drop this patch from the series and do full conversion of the
driver to TXVMVPRMSET0R_HSPOL_LOW BIT(17) style afterward. This patch is
not strictly necessary for the follow up patches.
