Hi, On 12/08/2025 22:32, Marek Vasut wrote: > On 8/12/25 3:26 PM, 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. 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. 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 =). Tomi
