| Message ID | 20220928005812.21060-3-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: lcdif: Improve YUV support | expand |
On 9/28/22 02:58, 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> Reviewed-by: Marek Vasut <marex@denx.de>
Quoting Laurent Pinchart (2022-09-28 01:58:10) > 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> > --- > Changes since v1: > > - Use hex for field values > --- > 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..0d5d9bedd94a 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)) I mean, I like BIT (and sometimes GENMASK) but ... What was going on there! Defintely better this way. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > -#define DISP_PARA_LINE_PATTERN_RGB565 GENMASK(28, 26) > -#define DISP_PARA_LINE_PATTERN_RGB888 0 > +#define DISP_PARA_LINE_PATTERN_UYVY_H (0xd << 26) > +#define DISP_PARA_LINE_PATTERN_RGB565 (0x7 << 26) > +#define DISP_PARA_LINE_PATTERN_RGB888 (0x0 << 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 (0x4 << 24) > +#define CTRLDESCL0_5_BPP_16_ARGB1555 (0x5 << 24) > +#define CTRLDESCL0_5_BPP_16_ARGB4444 (0x6 << 24) > +#define CTRLDESCL0_5_BPP_YCbCr422 (0x7 << 24) > +#define CTRLDESCL0_5_BPP_24_RGB888 (0x8 << 24) > +#define CTRLDESCL0_5_BPP_32_ARGB8888 (0x9 << 24) > +#define CTRLDESCL0_5_BPP_32_ABGR8888 (0xa << 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 (0x0 << 14) > +#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V (0x1 << 14) > +#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1 (0x2 << 14) > +#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1 (0x3 << 14) > #define CTRLDESCL0_5_YUV_FORMAT_MASK GENMASK(15, 14) > > #define CSC0_CTRL_CSC_MODE_RGB2YCbCr GENMASK(2, 1) > -- > Regards, > > Laurent Pinchart >
On Wed, 2022-09-28 at 03:58 +0300, 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> > --- > Changes since v1: > > - Use hex for field values > --- > drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) One option is to use FIELD_RPEP() macro for register field values. But I think this patch is ok without using it, so: Reviewed-by: Liu Ying <victor.liu@nxp.com>
diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h index 013f2cace2a0..0d5d9bedd94a 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 (0xd << 26) +#define DISP_PARA_LINE_PATTERN_RGB565 (0x7 << 26) +#define DISP_PARA_LINE_PATTERN_RGB888 (0x0 << 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 (0x4 << 24) +#define CTRLDESCL0_5_BPP_16_ARGB1555 (0x5 << 24) +#define CTRLDESCL0_5_BPP_16_ARGB4444 (0x6 << 24) +#define CTRLDESCL0_5_BPP_YCbCr422 (0x7 << 24) +#define CTRLDESCL0_5_BPP_24_RGB888 (0x8 << 24) +#define CTRLDESCL0_5_BPP_32_ARGB8888 (0x9 << 24) +#define CTRLDESCL0_5_BPP_32_ABGR8888 (0xa << 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 (0x0 << 14) +#define CTRLDESCL0_5_YUV_FORMAT_Y2UY1V (0x1 << 14) +#define CTRLDESCL0_5_YUV_FORMAT_VY2UY1 (0x2 << 14) +#define CTRLDESCL0_5_YUV_FORMAT_UY2VY1 (0x3 << 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> --- Changes since v1: - Use hex for field values --- drivers/gpu/drm/mxsfb/lcdif_regs.h | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)