| Message ID | 20220927233821.8007-3-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: lcdif: Improve YUV support | expand |
On 9/28/22 01:38, Laurent Pinchart wrote: > The BIT() macro is meant to represent a single bit. Don't use it for > values of register fields that span multiple bits. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h > index 013f2cace2a0..bc4d020aaa7c 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h > +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h > @@ -138,9 +138,9 @@ > > #define DISP_PARA_DISP_ON BIT(31) > #define DISP_PARA_SWAP_EN BIT(30) > -#define DISP_PARA_LINE_PATTERN_UYVY_H (GENMASK(29, 28) | BIT(26)) > -#define DISP_PARA_LINE_PATTERN_RGB565 GENMASK(28, 26) > -#define DISP_PARA_LINE_PATTERN_RGB888 0 > +#define DISP_PARA_LINE_PATTERN_UYVY_H (13 << 26) > +#define DISP_PARA_LINE_PATTERN_RGB565 (7 << 26) > +#define DISP_PARA_LINE_PATTERN_RGB888 (0 << 26) Can we use hex here for the left size of the shift operation, so it's subjectively easier to read ? DTTO for the other values ? That is: -#define DISP_PARA_LINE_PATTERN_UYVY_H (13 << 26) +#define DISP_PARA_LINE_PATTERN_UYVY_H (0xd << 26) [...]
Hi Marek, On Wed, Sep 28, 2022 at 02:10:26AM +0200, Marek Vasut wrote: > On 9/28/22 01:38, Laurent Pinchart wrote: > > The BIT() macro is meant to represent a single bit. Don't use it for > > values of register fields that span multiple bits. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++-------------- > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h > > index 013f2cace2a0..bc4d020aaa7c 100644 > > --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h > > +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h > > @@ -138,9 +138,9 @@ > > > > #define DISP_PARA_DISP_ON BIT(31) > > #define DISP_PARA_SWAP_EN BIT(30) > > -#define DISP_PARA_LINE_PATTERN_UYVY_H (GENMASK(29, 28) | BIT(26)) > > -#define DISP_PARA_LINE_PATTERN_RGB565 GENMASK(28, 26) > > -#define DISP_PARA_LINE_PATTERN_RGB888 0 > > +#define DISP_PARA_LINE_PATTERN_UYVY_H (13 << 26) > > +#define DISP_PARA_LINE_PATTERN_RGB565 (7 << 26) > > +#define DISP_PARA_LINE_PATTERN_RGB888 (0 << 26) > > Can we use hex here for the left size of the shift operation, so it's > subjectively easier to read ? DTTO for the other values ? > > That is: > -#define DISP_PARA_LINE_PATTERN_UYVY_H (13 << 26) > +#define DISP_PARA_LINE_PATTERN_UYVY_H (0xd << 26) > > [...] Sure, I'll fix that.
diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h index 013f2cace2a0..bc4d020aaa7c 100644 --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h @@ -138,9 +138,9 @@ #define DISP_PARA_DISP_ON BIT(31) #define DISP_PARA_SWAP_EN BIT(30) -#define DISP_PARA_LINE_PATTERN_UYVY_H (GENMASK(29, 28) | BIT(26)) -#define DISP_PARA_LINE_PATTERN_RGB565 GENMASK(28, 26) -#define DISP_PARA_LINE_PATTERN_RGB888 0 +#define DISP_PARA_LINE_PATTERN_UYVY_H (13 << 26) +#define DISP_PARA_LINE_PATTERN_RGB565 (7 << 26) +#define DISP_PARA_LINE_PATTERN_RGB888 (0 << 26) #define DISP_PARA_LINE_PATTERN_MASK GENMASK(29, 26) #define DISP_PARA_DISP_MODE_MASK GENMASK(25, 24) #define DISP_PARA_BGND_R_MASK GENMASK(23, 16) @@ -202,18 +202,18 @@ #define CTRLDESCL0_5_EN BIT(31) #define CTRLDESCL0_5_SHADOW_LOAD_EN BIT(30) -#define CTRLDESCL0_5_BPP_16_RGB565 BIT(26) -#define CTRLDESCL0_5_BPP_16_ARGB1555 (BIT(26) | BIT(24)) -#define CTRLDESCL0_5_BPP_16_ARGB4444 (BIT(26) | BIT(25)) -#define CTRLDESCL0_5_BPP_YCbCr422 (BIT(26) | BIT(25) | BIT(24)) -#define CTRLDESCL0_5_BPP_24_RGB888 BIT(27) -#define CTRLDESCL0_5_BPP_32_ARGB8888 (BIT(27) | BIT(24)) -#define CTRLDESCL0_5_BPP_32_ABGR8888 (BIT(27) | BIT(25)) +#define CTRLDESCL0_5_BPP_16_RGB565 (4 << 24) +#define CTRLDESCL0_5_BPP_16_ARGB1555 (5 << 24) +#define CTRLDESCL0_5_BPP_16_ARGB4444 (6 << 24) +#define CTRLDESCL0_5_BPP_YCbCr422 (7 << 24) +#define CTRLDESCL0_5_BPP_24_RGB888 (8 << 24) +#define CTRLDESCL0_5_BPP_32_ARGB8888 (9 << 24) +#define CTRLDESCL0_5_BPP_32_ABGR8888 (10 << 24) #define CTRLDESCL0_5_BPP_MASK GENMASK(27, 24) -#define CTRLDESCL0_5_YUV_FORMAT_Y2VY1U 0 -#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V BIT(14) -#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1 BIT(15) -#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1 (BIT(15) | BIT(14)) +#define CTRLDESCL0_5_YUV_FORMAT_Y2VY1U (0 << 14) +#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V (1 << 14) +#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1 (2 << 14) +#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1 (3 << 14) #define CTRLDESCL0_5_YUV_FORMAT_MASK GENMASK(15, 14) #define CSC0_CTRL_CSC_MODE_RGB2YCbCr GENMASK(2, 1)
The BIT() macro is meant to represent a single bit. Don't use it for values of register fields that span multiple bits. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)