Re: [PATCH 05/21] drm: mxsfb: Clarify format and bus width configuration
On 2020-03-09 20:52, Laurent Pinchart wrote:
> Replace the convoluted way to set the format and bus width through
> difficult to read macros with more explicit ones. Also remove the
> outdated comment related to the limitations on bus width setting as it
Nice catch.
> doesn't apply anymore (the bus width can be specified through the
> display_info bus format).
Reviewed-by: Stefan Agner
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 17 +
> drivers/gpu/drm/mxsfb/mxsfb_regs.h | 17 -
> 2 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> index b69ace8bf526..8b6339316929 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> @@ -52,13 +52,6 @@ static int mxsfb_set_pixel_fmt(struct
> mxsfb_drm_private *mxsfb)
>
> ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER;
>
> - /*
> - * WARNING: The bus width, CTRL_SET_BUS_WIDTH(), is configured to
> - * match the selected mode here. This differs from the original
> - * MXSFB driver, which had the option to configure the bus width
> - * to arbitrary value. This limitation should not pose an issue.
> - */
> -
> /* CTRL1 contains IRQ config and status bits, preserve those. */
> ctrl1 = readl(mxsfb->base + LCDC_CTRL1);
> ctrl1 &= CTRL1_CUR_FRAME_DONE_IRQ_EN | CTRL1_CUR_FRAME_DONE_IRQ;
> @@ -66,12 +59,12 @@ static int mxsfb_set_pixel_fmt(struct
> mxsfb_drm_private *mxsfb)
> switch (format) {
> case DRM_FORMAT_RGB565:
> dev_dbg(drm->dev, "Setting up RGB565 mode\n");
> - ctrl |= CTRL_SET_WORD_LENGTH(0);
> + ctrl |= CTRL_WORD_LENGTH_16;
> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf);
> break;
> case DRM_FORMAT_XRGB:
> dev_dbg(drm->dev, "Setting up XRGB mode\n");
> - ctrl |= CTRL_SET_WORD_LENGTH(3);
> + ctrl |= CTRL_WORD_LENGTH_24;
> /* Do not use packed pixels = one pixel per word instead. */
> ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7);
> break;
> @@ -104,13 +97,13 @@ static void mxsfb_set_bus_fmt(struct
> mxsfb_drm_private *mxsfb)
> reg &= ~CTRL_BUS_WIDTH_MASK;
> switch (bus_format) {
> case MEDIA_BUS_FMT_RGB565_1X16:
> - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
> + reg |= CTRL_BUS_WIDTH_16;
> break;
> case MEDIA_BUS_FMT_RGB666_1X18:
> - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT);
> + reg |= CTRL_BUS_WIDTH_18;
> break;
> case MEDIA_BUS_FMT_RGB888_1X24:
> - reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
> + reg |= CTRL_BUS_WIDTH_24;
> break;
> default:
> dev_err(drm->dev, "Unknown media bus format %d\n", bus_format);
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> index 78e6cb754712..8ebb52bb1b46 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> @@ -34,11 +34,15 @@
> #define CTRL_VSYNC_MODE BIT(18)
> #define CTRL_DOTCLK_MODE BIT(17)
> #define CTRL_DATA_SELECT BIT(16)
> -#define CTRL_SET_BUS_WIDTH(x)(((x) & 0x3) << 10)
> -#define CTRL_GET_BUS_WIDTH(x)(((x) >> 10) & 0x3)
> +#define CTRL_BUS_WIDTH_16(0 << 10)
> +#define CTRL_BUS_WIDTH_8 (1 << 10)
> +#define CTRL_BUS_WIDTH_18(2 << 10)
> +#define CTRL_BUS_WIDTH_24(3 << 10)
> #define CTRL_BUS_WIDTH_MASK (0x3 << 10)
> -#define CTRL_SET_WORD_LENGTH(x) (((x) & 0x3) << 8)
> -#define CTRL_GET_WORD_LENGTH(x) (((x) >> 8) & 0x3)
> +#define CTRL_WORD_LENGTH_16 (0 << 8)
> +#define CTRL_WORD_LENGTH_8 (1 << 8)
> +#define CTRL_WORD_LENGTH_18 (2 << 8)
> +#define CTRL_WORD_LENGTH_24 (3 << 8)
> #define CTRL_MASTER BIT(5)
> #define CTRL_DF16BIT(3)
> #define CTRL_DF18BIT(2)
> @@ -91,9 +95,4 @@
> #define MXSFB_MAX_XRES 0x
> #define MXSFB_MAX_YRES 0x
>
> -#define STMLCDIF_8BIT 1 /* pixel data bus to the display is of 8 bit width
> */
> -#define STMLCDIF_16BIT 0 /* pixel data bus to the display is of 16 bit width
> */
> -#define STMLCDIF_18BIT 2 /* pixel data bus to the display is of 18 bit width
> */
> -#define STMLCDIF_24BIT 3 /* pixel data bus to the display is of 24 bit width
> */
> -
> #endif /* __MXSFB_REGS_H__ */
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 05/21] drm: mxsfb: Clarify format and bus width configuration
Replace the convoluted way to set the format and bus width through
difficult to read macros with more explicit ones. Also remove the
outdated comment related to the limitations on bus width setting as it
doesn't apply anymore (the bus width can be specified through the
display_info bus format).
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 17 +
drivers/gpu/drm/mxsfb/mxsfb_regs.h | 17 -
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
index b69ace8bf526..8b6339316929 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
@@ -52,13 +52,6 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private
*mxsfb)
ctrl = CTRL_BYPASS_COUNT | CTRL_MASTER;
- /*
-* WARNING: The bus width, CTRL_SET_BUS_WIDTH(), is configured to
-* match the selected mode here. This differs from the original
-* MXSFB driver, which had the option to configure the bus width
-* to arbitrary value. This limitation should not pose an issue.
-*/
-
/* CTRL1 contains IRQ config and status bits, preserve those. */
ctrl1 = readl(mxsfb->base + LCDC_CTRL1);
ctrl1 &= CTRL1_CUR_FRAME_DONE_IRQ_EN | CTRL1_CUR_FRAME_DONE_IRQ;
@@ -66,12 +59,12 @@ static int mxsfb_set_pixel_fmt(struct mxsfb_drm_private
*mxsfb)
switch (format) {
case DRM_FORMAT_RGB565:
dev_dbg(drm->dev, "Setting up RGB565 mode\n");
- ctrl |= CTRL_SET_WORD_LENGTH(0);
+ ctrl |= CTRL_WORD_LENGTH_16;
ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0xf);
break;
case DRM_FORMAT_XRGB:
dev_dbg(drm->dev, "Setting up XRGB mode\n");
- ctrl |= CTRL_SET_WORD_LENGTH(3);
+ ctrl |= CTRL_WORD_LENGTH_24;
/* Do not use packed pixels = one pixel per word instead. */
ctrl1 |= CTRL1_SET_BYTE_PACKAGING(0x7);
break;
@@ -104,13 +97,13 @@ static void mxsfb_set_bus_fmt(struct mxsfb_drm_private
*mxsfb)
reg &= ~CTRL_BUS_WIDTH_MASK;
switch (bus_format) {
case MEDIA_BUS_FMT_RGB565_1X16:
- reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
+ reg |= CTRL_BUS_WIDTH_16;
break;
case MEDIA_BUS_FMT_RGB666_1X18:
- reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_18BIT);
+ reg |= CTRL_BUS_WIDTH_18;
break;
case MEDIA_BUS_FMT_RGB888_1X24:
- reg |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
+ reg |= CTRL_BUS_WIDTH_24;
break;
default:
dev_err(drm->dev, "Unknown media bus format %d\n", bus_format);
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
index 78e6cb754712..8ebb52bb1b46 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
+++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
@@ -34,11 +34,15 @@
#define CTRL_VSYNC_MODEBIT(18)
#define CTRL_DOTCLK_MODE BIT(17)
#define CTRL_DATA_SELECT BIT(16)
-#define CTRL_SET_BUS_WIDTH(x) (((x) & 0x3) << 10)
-#define CTRL_GET_BUS_WIDTH(x) (((x) >> 10) & 0x3)
+#define CTRL_BUS_WIDTH_16 (0 << 10)
+#define CTRL_BUS_WIDTH_8 (1 << 10)
+#define CTRL_BUS_WIDTH_18 (2 << 10)
+#define CTRL_BUS_WIDTH_24 (3 << 10)
#define CTRL_BUS_WIDTH_MASK(0x3 << 10)
-#define CTRL_SET_WORD_LENGTH(x)(((x) & 0x3) << 8)
-#define CTRL_GET_WORD_LENGTH(x)(((x) >> 8) & 0x3)
+#define CTRL_WORD_LENGTH_16(0 << 8)
+#define CTRL_WORD_LENGTH_8 (1 << 8)
+#define CTRL_WORD_LENGTH_18(2 << 8)
+#define CTRL_WORD_LENGTH_24(3 << 8)
#define CTRL_MASTERBIT(5)
#define CTRL_DF16 BIT(3)
#define CTRL_DF18 BIT(2)
@@ -91,9 +95,4 @@
#define MXSFB_MAX_XRES 0x
#define MXSFB_MAX_YRES 0x
-#define STMLCDIF_8BIT 1 /* pixel data bus to the display is of 8 bit width */
-#define STMLCDIF_16BIT 0 /* pixel data bus to the display is of 16 bit width */
-#define STMLCDIF_18BIT 2 /* pixel data bus to the display is of 18 bit width */
-#define STMLCDIF_24BIT 3 /* pixel data bus to the display is of 24 bit width */
-
#endif /* __MXSFB_REGS_H__ */
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
