Changes in v4:
- Added a fix for RGB666 formats setting and for wrong register
definitions for packed vs loosely packed formats
- Added a commit to make use of mipi_dsi_pixel_format_to_bpp() helper
instead of open coding the same
Changes in v3:
- Rebased over next-20240131
- Added bitfield.h inclusion in patch 3
- Added three more cleanup commits to the mix to simplify
the probe function and remove gotos.
Changes in v2:
- Rebased over next-20231213
This series performs some cleanups for mtk_dsi, enhancing human
readability, using kernel provided macros where possible and
also reducing code size.
Tested on MT8173 and MT8192 Chromebooks (using a DSI<->DP bridge)
and on MT6795 Sony Xperia M5 (DSI video mode panel).
AngeloGioacchino Del Regno (9):
drm/mediatek: dsi: Use GENMASK() for register mask definitions
drm/mediatek: dsi: Fix DSI RGB666 formats and definitions
drm/mediatek: dsi: Cleanup functions mtk_dsi_ps_control{_vact}()
drm/mediatek: dsi: Use bitfield macros where useful
drm/mediatek: dsi: Replace open-coded instance of HZ_PER_MHZ
drm/mediatek: dsi: Register DSI host after acquiring clocks and PHY
drm/mediatek: dsi: Simplify with dev_err_probe and remove gotos
drm/mediatek: dsi: Compress of_device_id entries and add sentinel
drm/mediatek: dsi: Use mipi_dsi_pixel_format_to_bpp() helper function
drivers/gpu/drm/mediatek/mtk_dsi.c | 310 ++++++++++++-----------------
1 file changed, 126 insertions(+), 184 deletions(-)
--
2.43.0
The register bits definitions for RGB666 formats are wrong in multiple
ways: first, in the DSI_PS_SEL bits region, the Packed 18-bits RGB666
format is selected with bit 1, while the Loosely Packed one is bit 2,
and second - the definition name "LOOSELY_PS_18BIT_RGB666" is wrong
because the loosely packed format is 24 bits instead!
Either way, functions mtk_dsi_ps_control_vact() and mtk_dsi_ps_control()
do not even agree on the DSI_PS_SEL bit to set in DSI_PSCTRL: one sets
loosely packed (24) on RGB666, the other sets packed (18), and the other
way around for RGB666_PACKED.
Fixing this entire stack of issues is done in one go:
- Use the correct bit for the Loosely Packed RGB666 definition
- Rename LOOSELY_PS_18BIT_RGB666 to LOOSELY_PS_24BIT_RGB666
- Change ps_bpp_mode in mtk_dsi_ps_control_vact() to set:
- Loosely Packed, 24-bits for MIPI_DSI_FMT_RGB666
- Packed, 18-bits for MIPI_DSI_FMT_RGB666_PACKED
Fixes: 2e54c14e310f ("drm/mediatek: Add DSI sub driver")
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 3b7392c03b4d..9fbf293db1c8 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -71,8 +71,8 @@
#define DSI_PS_WC GENMASK(14, 0)
#define DSI_PS_SEL GENMASK(19, 16)
#define PACKED_PS_16BIT_RGB565 (0 << 16)
-#define LOOSELY_PS_18BIT_RGB666 (1 << 16)
-#define PACKED_PS_18BIT_RGB666 (2 << 16)
+#define PACKED_PS_18BIT_RGB666 (1 << 16)
+#define LOOSELY_PS_24BIT_RGB666 (2 << 16)
#define PACKED_PS_24BIT_RGB888 (3 << 16)
#define DSI_VSA_NL 0x20
@@ -370,10 +370,10 @@ static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
break;
case MIPI_DSI_FMT_RGB666:
- ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
+ ps_bpp_mode |= LOOSELY_PS_24BIT_RGB666;
break;
case MIPI_DSI_FMT_RGB666_PACKED:
- ps_bpp_mode |= LOOSELY_PS_18BIT_RGB666;
+ ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
break;
case MIPI_DSI_FMT_RGB565:
ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
@@ -427,7 +427,7 @@ static void mtk_dsi_ps_control(struct mtk_dsi *dsi)
dsi_tmp_buf_bpp = 3;
break;
case MIPI_DSI_FMT_RGB666:
- tmp_reg = LOOSELY_PS_18BIT_RGB666;
+ tmp_reg = LOOSELY_PS_24BIT_RGB666;
dsi_tmp_buf_bpp = 3;
break;
case MIPI_DSI_FMT_RGB666_PACKED:
--
2.43.0
Instead of open coding bitshifting for various register fields,
use the bitfield macro FIELD_PREP(): this allows to enhance the
human readability, decrease likeliness of mistakes (and register
field overflowing) and also to simplify the code.
The latter is especially seen in mtk_dsi_rxtx_control(), where
it was possible to change a switch to a short for loop and to
also remove the need to check for maximum DSI lanes == 4 thanks
to the FIELD_PREP macro masking the value.
While at it, also add the missing DA_HS_SYNC bitmask, used in
mtk_dsi_phy_timconfig().
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 97 ++++++++++++++++--------------
1 file changed, 52 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index b025886be680..26c221737387 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -3,6 +3,7 @@
* Copyright (c) 2015 MediaTek Inc.
*/
+#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/component.h>
#include <linux/iopoll.h>
@@ -70,16 +71,19 @@
#define DSI_PSCTRL 0x1c
#define DSI_PS_WC GENMASK(14, 0)
#define DSI_PS_SEL GENMASK(19, 16)
-#define PACKED_PS_16BIT_RGB565 (0 << 16)
-#define PACKED_PS_18BIT_RGB666 (1 << 16)
-#define LOOSELY_PS_24BIT_RGB666 (2 << 16)
-#define PACKED_PS_24BIT_RGB888 (3 << 16)
+#define PACKED_PS_16BIT_RGB565 0
+#define PACKED_PS_18BIT_RGB666 1
+#define LOOSELY_PS_24BIT_RGB666 2
+#define PACKED_PS_24BIT_RGB888 3
#define DSI_VSA_NL 0x20
#define DSI_VBP_NL 0x24
#define DSI_VFP_NL 0x28
#define DSI_VACT_NL 0x2C
+#define VACT_NL GENMASK(14, 0)
#define DSI_SIZE_CON 0x38
+#define DSI_HEIGHT GENMASK(30, 16)
+#define DSI_WIDTH GENMASK(14, 0)
#define DSI_HSA_WC 0x50
#define DSI_HBP_WC 0x54
#define DSI_HFP_WC 0x58
@@ -122,6 +126,7 @@
#define DSI_PHY_TIMECON2 0x118
#define CONT_DET GENMASK(7, 0)
+#define DA_HS_SYNC GENMASK(15, 8)
#define CLK_ZERO GENMASK(23, 16)
#define CLK_TRAIL GENMASK(31, 24)
@@ -253,14 +258,23 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
timing->clk_hs_zero = timing->clk_hs_trail * 4;
timing->clk_hs_exit = 2 * timing->clk_hs_trail;
- timcon0 = timing->lpx | timing->da_hs_prepare << 8 |
- timing->da_hs_zero << 16 | timing->da_hs_trail << 24;
- timcon1 = timing->ta_go | timing->ta_sure << 8 |
- timing->ta_get << 16 | timing->da_hs_exit << 24;
- timcon2 = 1 << 8 | timing->clk_hs_zero << 16 |
- timing->clk_hs_trail << 24;
- timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 |
- timing->clk_hs_exit << 16;
+ timcon0 = FIELD_PREP(LPX, timing->lpx) |
+ FIELD_PREP(HS_PREP, timing->da_hs_prepare) |
+ FIELD_PREP(HS_ZERO, timing->da_hs_zero) |
+ FIELD_PREP(HS_TRAIL, timing->da_hs_trail);
+
+ timcon1 = FIELD_PREP(TA_GO, timing->ta_go) |
+ FIELD_PREP(TA_SURE, timing->ta_sure) |
+ FIELD_PREP(TA_GET, timing->ta_get) |
+ FIELD_PREP(DA_HS_EXIT, timing->da_hs_exit);
+
+ timcon2 = FIELD_PREP(DA_HS_SYNC, 1) |
+ FIELD_PREP(CLK_ZERO, timing->clk_hs_zero) |
+ FIELD_PREP(CLK_TRAIL, timing->clk_hs_trail);
+
+ timcon3 = FIELD_PREP(CLK_HS_PREP, timing->clk_hs_prepare) |
+ FIELD_PREP(CLK_HS_POST, timing->clk_hs_post) |
+ FIELD_PREP(CLK_HS_EXIT, timing->clk_hs_exit);
writel(timcon0, dsi->regs + DSI_PHY_TIMECON0);
writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);
@@ -353,69 +367,61 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
{
- u32 tmp_reg;
+ u32 regval, tmp_reg = 0;
+ u8 i;
- switch (dsi->lanes) {
- case 1:
- tmp_reg = 1 << 2;
- break;
- case 2:
- tmp_reg = 3 << 2;
- break;
- case 3:
- tmp_reg = 7 << 2;
- break;
- case 4:
- tmp_reg = 0xf << 2;
- break;
- default:
- tmp_reg = 0xf << 2;
- break;
- }
+ /* Number of DSI lanes (max 4 lanes), each bit enables one DSI lane. */
+ for (i = 0; i < dsi->lanes; i++)
+ tmp_reg |= BIT(i);
+
+ regval = FIELD_PREP(LANE_NUM, tmp_reg);
if (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)
- tmp_reg |= HSTX_CKLP_EN;
+ regval |= HSTX_CKLP_EN;
if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
- tmp_reg |= DIS_EOT;
+ regval |= DIS_EOT;
- writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
+ writel(regval, dsi->regs + DSI_TXRX_CTRL);
}
static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
{
- struct videomode *vm = &dsi->vm;
- u32 dsi_buf_bpp, ps_wc;
- u32 ps_bpp_mode;
+ u32 dsi_buf_bpp, ps_val, ps_wc, vact_nl;
if (dsi->format == MIPI_DSI_FMT_RGB565)
dsi_buf_bpp = 2;
else
dsi_buf_bpp = 3;
- ps_wc = vm->hactive * dsi_buf_bpp;
- ps_bpp_mode = ps_wc;
+ /* Word count */
+ ps_wc = FIELD_PREP(DSI_PS_WC, dsi->vm.hactive * dsi_buf_bpp);
+ ps_val = ps_wc;
+ /* Pixel Stream type */
switch (dsi->format) {
+ default:
+ fallthrough;
case MIPI_DSI_FMT_RGB888:
- ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
+ ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_24BIT_RGB888);
break;
case MIPI_DSI_FMT_RGB666:
- ps_bpp_mode |= LOOSELY_PS_24BIT_RGB666;
+ ps_val |= FIELD_PREP(DSI_PS_SEL, LOOSELY_PS_24BIT_RGB666);
break;
case MIPI_DSI_FMT_RGB666_PACKED:
- ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
+ ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_18BIT_RGB666);
break;
case MIPI_DSI_FMT_RGB565:
- ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
+ ps_val |= FIELD_PREP(DSI_PS_SEL, PACKED_PS_16BIT_RGB565);
break;
}
if (config_vact) {
- writel(vm->vactive, dsi->regs + DSI_VACT_NL);
+ vact_nl = FIELD_PREP(VACT_NL, dsi->vm.vactive);
+ writel(vact_nl, dsi->regs + DSI_VACT_NL);
writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
}
- writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
+ writel(ps_val, dsi->regs + DSI_PSCTRL);
}
static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
@@ -442,7 +448,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
writel(vm->vactive, dsi->regs + DSI_VACT_NL);
if (dsi->driver_data->has_size_ctl)
- writel(vm->vactive << 16 | vm->hactive,
+ writel(FIELD_PREP(DSI_HEIGHT, vm->vactive) |
+ FIELD_PREP(DSI_WIDTH, vm->hactive),
dsi->regs + DSI_SIZE_CON);
horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10);
--
2.43.0
Instead of open coding, use the mipi_dsi_pixel_format_to_bpp() helper
function from drm_mipi_dsi.h in mtk_dsi_poweron() and for validation
in mtk_dsi_bridge_mode_valid().
Note that this function changes the behavior of this driver: previously,
in case of unknown formats, it would (wrongly) assume that it should
account for a 24-bits format - now it will return an error and refuse
to set clocks and/or enable the DSI.
This is done because setting the wrong data rate will only produce a
garbage output that the display will misinterpret both because this
driver doesn't actually provide any extra-spec format support and/or
because the data rate (hence, the HS clock) will be wrong.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 545c5cc071d9..d844a0500e2b 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -598,19 +598,12 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
if (++dsi->refcount != 1)
return 0;
- switch (dsi->format) {
- case MIPI_DSI_FMT_RGB565:
- bit_per_pixel = 16;
- break;
- case MIPI_DSI_FMT_RGB666_PACKED:
- bit_per_pixel = 18;
- break;
- case MIPI_DSI_FMT_RGB666:
- case MIPI_DSI_FMT_RGB888:
- default:
- bit_per_pixel = 24;
- break;
+ ret = mipi_dsi_pixel_format_to_bpp(dsi->format);
+ if (ret < 0) {
+ dev_err(dev, "Unknown MIPI DSI format %d\n", dsi->format);
+ return ret;
}
+ bit_per_pixel = ret;
dsi->data_rate = DIV_ROUND_UP_ULL(dsi->vm.pixelclock * bit_per_pixel,
dsi->lanes);
@@ -793,12 +786,11 @@ mtk_dsi_bridge_mode_valid(struct drm_bridge *bridge,
const struct drm_display_mode *mode)
{
struct mtk_dsi *dsi = bridge_to_dsi(bridge);
- u32 bpp;
+ int bpp;
- if (dsi->format == MIPI_DSI_FMT_RGB565)
- bpp = 16;
- else
- bpp = 24;
+ bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+ if (bpp < 0)
+ return MODE_ERROR;
if (mode->clock * bpp / dsi->lanes > 1500000)
return MODE_CLOCK_HIGH;
--
2.43.0
All entries fit in 82 columns, which is acceptable: compress all of
the mtk_dsi_of_match[] entries to a single line for each.
While at it, also add the usual sentinel comment to the last entry.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 8d407d71e9db..545c5cc071d9 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -1204,17 +1204,12 @@ static const struct mtk_dsi_driver_data mt8188_dsi_driver_data = {
};
static const struct of_device_id mtk_dsi_of_match[] = {
- { .compatible = "mediatek,mt2701-dsi",
- .data = &mt2701_dsi_driver_data },
- { .compatible = "mediatek,mt8173-dsi",
- .data = &mt8173_dsi_driver_data },
- { .compatible = "mediatek,mt8183-dsi",
- .data = &mt8183_dsi_driver_data },
- { .compatible = "mediatek,mt8186-dsi",
- .data = &mt8186_dsi_driver_data },
- { .compatible = "mediatek,mt8188-dsi",
- .data = &mt8188_dsi_driver_data },
- { },
+ { .compatible = "mediatek,mt2701-dsi", .data = &mt2701_dsi_driver_data },
+ { .compatible = "mediatek,mt8173-dsi", .data = &mt8173_dsi_driver_data },
+ { .compatible = "mediatek,mt8183-dsi", .data = &mt8183_dsi_driver_data },
+ { .compatible = "mediatek,mt8186-dsi", .data = &mt8186_dsi_driver_data },
+ { .compatible = "mediatek,mt8188-dsi", .data = &mt8188_dsi_driver_data },
+ { /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mtk_dsi_of_match);
--
2.43.0
Change magic numerical masks with usage of the GENMASK() macro
to improve readability.
While at it, also fix the DSI_PS_SEL mask to include all bits instead
of just a subset of them.
This commit brings no functional changes.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++---------------
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index a2fdfc8ddb15..3b7392c03b4d 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -58,18 +58,18 @@
#define DSI_TXRX_CTRL 0x18
#define VC_NUM BIT(1)
-#define LANE_NUM (0xf << 2)
+#define LANE_NUM GENMASK(5, 2)
#define DIS_EOT BIT(6)
#define NULL_EN BIT(7)
#define TE_FREERUN BIT(8)
#define EXT_TE_EN BIT(9)
#define EXT_TE_EDGE BIT(10)
-#define MAX_RTN_SIZE (0xf << 12)
+#define MAX_RTN_SIZE GENMASK(15, 12)
#define HSTX_CKLP_EN BIT(16)
#define DSI_PSCTRL 0x1c
-#define DSI_PS_WC 0x3fff
-#define DSI_PS_SEL (3 << 16)
+#define DSI_PS_WC GENMASK(14, 0)
+#define DSI_PS_SEL GENMASK(19, 16)
#define PACKED_PS_16BIT_RGB565 (0 << 16)
#define LOOSELY_PS_18BIT_RGB666 (1 << 16)
#define PACKED_PS_18BIT_RGB666 (2 << 16)
@@ -109,26 +109,26 @@
#define LD0_WAKEUP_EN BIT(2)
#define DSI_PHY_TIMECON0 0x110
-#define LPX (0xff << 0)
-#define HS_PREP (0xff << 8)
-#define HS_ZERO (0xff << 16)
-#define HS_TRAIL (0xff << 24)
+#define LPX GENMASK(7, 0)
+#define HS_PREP GENMASK(15, 8)
+#define HS_ZERO GENMASK(23, 16)
+#define HS_TRAIL GENMASK(31, 24)
#define DSI_PHY_TIMECON1 0x114
-#define TA_GO (0xff << 0)
-#define TA_SURE (0xff << 8)
-#define TA_GET (0xff << 16)
-#define DA_HS_EXIT (0xff << 24)
+#define TA_GO GENMASK(7, 0)
+#define TA_SURE GENMASK(15, 8)
+#define TA_GET GENMASK(23, 16)
+#define DA_HS_EXIT GENMASK(31, 24)
#define DSI_PHY_TIMECON2 0x118
-#define CONT_DET (0xff << 0)
-#define CLK_ZERO (0xff << 16)
-#define CLK_TRAIL (0xff << 24)
+#define CONT_DET GENMASK(7, 0)
+#define CLK_ZERO GENMASK(23, 16)
+#define CLK_TRAIL GENMASK(31, 24)
#define DSI_PHY_TIMECON3 0x11c
-#define CLK_HS_PREP (0xff << 0)
-#define CLK_HS_POST (0xff << 8)
-#define CLK_HS_EXIT (0xff << 16)
+#define CLK_HS_PREP GENMASK(7, 0)
+#define CLK_HS_POST GENMASK(15, 8)
+#define CLK_HS_EXIT GENMASK(23, 16)
#define DSI_VM_CMD_CON 0x130
#define VM_CMD_EN BIT(0)
@@ -138,13 +138,14 @@
#define FORCE_COMMIT BIT(0)
#define BYPASS_SHADOW BIT(1)
-#define CONFIG (0xff << 0)
+/* CMDQ related bits */
+#define CONFIG GENMASK(7, 0)
#define SHORT_PACKET 0
#define LONG_PACKET 2
#define BTA BIT(2)
-#define DATA_ID (0xff << 8)
-#define DATA_0 (0xff << 16)
-#define DATA_1 (0xff << 24)
+#define DATA_ID GENMASK(15, 8)
+#define DATA_0 GENMASK(23, 16)
+#define DATA_1 GENMASK(31, 24)
#define NS_TO_CYCLE(n, c) ((n) / (c) + (((n) % (c)) ? 1 : 0))
--
2.43.0
In mtk_dsi_phy_timconfig(), we're dividing the `data_rate` variable,
expressed in Hz to retrieve a value in MHz: instead of open-coding,
use the HZ_PER_MHZ definition, available in linux/units.h.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 26c221737387..5e383ca00ba8 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -13,6 +13,7 @@
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/reset.h>
+#include <linux/units.h>
#include <video/mipi_display.h>
#include <video/videomode.h>
@@ -238,7 +239,7 @@ static void mtk_dsi_mask(struct mtk_dsi *dsi, u32 offset, u32 mask, u32 data)
static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
{
u32 timcon0, timcon1, timcon2, timcon3;
- u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, 1000000);
+ u32 data_rate_mhz = DIV_ROUND_UP(dsi->data_rate, HZ_PER_MHZ);
struct mtk_phy_timing *timing = &dsi->phy_timing;
timing->lpx = (60 * data_rate_mhz / (8 * 1000)) + 1;
--
2.43.0
Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact():
merge the two in one mtk_dsi_ps_control() function by adding one
function parameter `config_vact` which, when true, writes the VACT
related registers.
Reviewed-by: Fei Shao <[email protected]>
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 76 +++++++++---------------------
1 file changed, 23 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 9fbf293db1c8..b025886be680 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -351,40 +351,6 @@ static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
}
-static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
-{
- struct videomode *vm = &dsi->vm;
- u32 dsi_buf_bpp, ps_wc;
- u32 ps_bpp_mode;
-
- if (dsi->format == MIPI_DSI_FMT_RGB565)
- dsi_buf_bpp = 2;
- else
- dsi_buf_bpp = 3;
-
- ps_wc = vm->hactive * dsi_buf_bpp;
- ps_bpp_mode = ps_wc;
-
- switch (dsi->format) {
- case MIPI_DSI_FMT_RGB888:
- ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
- break;
- case MIPI_DSI_FMT_RGB666:
- ps_bpp_mode |= LOOSELY_PS_24BIT_RGB666;
- break;
- case MIPI_DSI_FMT_RGB666_PACKED:
- ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
- break;
- case MIPI_DSI_FMT_RGB565:
- ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
- break;
- }
-
- writel(vm->vactive, dsi->regs + DSI_VACT_NL);
- writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
- writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
-}
-
static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
{
u32 tmp_reg;
@@ -416,36 +382,40 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
}
-static void mtk_dsi_ps_control(struct mtk_dsi *dsi)
+static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
{
- u32 dsi_tmp_buf_bpp;
- u32 tmp_reg;
+ struct videomode *vm = &dsi->vm;
+ u32 dsi_buf_bpp, ps_wc;
+ u32 ps_bpp_mode;
+
+ if (dsi->format == MIPI_DSI_FMT_RGB565)
+ dsi_buf_bpp = 2;
+ else
+ dsi_buf_bpp = 3;
+
+ ps_wc = vm->hactive * dsi_buf_bpp;
+ ps_bpp_mode = ps_wc;
switch (dsi->format) {
case MIPI_DSI_FMT_RGB888:
- tmp_reg = PACKED_PS_24BIT_RGB888;
- dsi_tmp_buf_bpp = 3;
+ ps_bpp_mode |= PACKED_PS_24BIT_RGB888;
break;
case MIPI_DSI_FMT_RGB666:
- tmp_reg = LOOSELY_PS_24BIT_RGB666;
- dsi_tmp_buf_bpp = 3;
+ ps_bpp_mode |= LOOSELY_PS_24BIT_RGB666;
break;
case MIPI_DSI_FMT_RGB666_PACKED:
- tmp_reg = PACKED_PS_18BIT_RGB666;
- dsi_tmp_buf_bpp = 3;
+ ps_bpp_mode |= PACKED_PS_18BIT_RGB666;
break;
case MIPI_DSI_FMT_RGB565:
- tmp_reg = PACKED_PS_16BIT_RGB565;
- dsi_tmp_buf_bpp = 2;
- break;
- default:
- tmp_reg = PACKED_PS_24BIT_RGB888;
- dsi_tmp_buf_bpp = 3;
+ ps_bpp_mode |= PACKED_PS_16BIT_RGB565;
break;
}
- tmp_reg += dsi->vm.hactive * dsi_tmp_buf_bpp & DSI_PS_WC;
- writel(tmp_reg, dsi->regs + DSI_PSCTRL);
+ if (config_vact) {
+ writel(vm->vactive, dsi->regs + DSI_VACT_NL);
+ writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
+ }
+ writel(ps_bpp_mode, dsi->regs + DSI_PSCTRL);
}
static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
@@ -521,7 +491,7 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC);
- mtk_dsi_ps_control(dsi);
+ mtk_dsi_ps_control(dsi, false);
}
static void mtk_dsi_start(struct mtk_dsi *dsi)
@@ -666,7 +636,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
mtk_dsi_reset_engine(dsi);
mtk_dsi_phy_timconfig(dsi);
- mtk_dsi_ps_control_vact(dsi);
+ mtk_dsi_ps_control(dsi, true);
mtk_dsi_set_vm_cmd(dsi);
mtk_dsi_config_vdo_timing(dsi);
mtk_dsi_set_interrupt_enable(dsi);
--
2.43.0
Registering the dsi host with its ops before getting dsi->regs is
simply wrong: even though there's nothing (for now) asynchronously
calling those ops before the end of the probe function, installing
ops that are using iospace(s) and clocks before even initializing
those is too fragile.
Register the DSI host after getting clocks, iospace and PHY.
This wil also allow to simplify the error paths in a later commit.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 5e383ca00ba8..6ee01626d55c 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -1114,14 +1114,6 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (!dsi)
return -ENOMEM;
- dsi->host.ops = &mtk_dsi_ops;
- dsi->host.dev = dev;
- ret = mipi_dsi_host_register(&dsi->host);
- if (ret < 0) {
- dev_err(dev, "failed to register DSI host: %d\n", ret);
- return ret;
- }
-
dsi->driver_data = of_device_get_match_data(dev);
dsi->engine_clk = devm_clk_get(dev, "engine");
@@ -1130,7 +1122,7 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (ret != -EPROBE_DEFER)
dev_err(dev, "Failed to get engine clock: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
dsi->digital_clk = devm_clk_get(dev, "digital");
@@ -1139,14 +1131,14 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (ret != -EPROBE_DEFER)
dev_err(dev, "Failed to get digital clock: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
dsi->hs_clk = devm_clk_get(dev, "hs");
if (IS_ERR(dsi->hs_clk)) {
ret = PTR_ERR(dsi->hs_clk);
dev_err(dev, "Failed to get hs clock: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1154,20 +1146,28 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (IS_ERR(dsi->regs)) {
ret = PTR_ERR(dsi->regs);
dev_err(dev, "Failed to ioremap memory: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
dsi->phy = devm_phy_get(dev, "dphy");
if (IS_ERR(dsi->phy)) {
ret = PTR_ERR(dsi->phy);
dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
- goto err_unregister_host;
+ return ret;
}
irq_num = platform_get_irq(pdev, 0);
if (irq_num < 0) {
ret = irq_num;
- goto err_unregister_host;
+ return ret;
+ }
+
+ dsi->host.ops = &mtk_dsi_ops;
+ dsi->host.dev = dev;
+ ret = mipi_dsi_host_register(&dsi->host);
+ if (ret < 0) {
+ dev_err(dev, "failed to register DSI host: %d\n", ret);
+ return ret;
}
ret = devm_request_irq(&pdev->dev, irq_num, mtk_dsi_irq,
--
2.43.0
Most of the functions that are called in the probe callback are
devm managed, or all but mipi_dsi_host_register(): simplify the probe
function's error paths with dev_err_probe() and remove the lonely
instance of `goto err_unregister_host` by just directly calling the
mipi_dsi_host_unregister() function in the devm_request_irq() error
path, allowing to also remove the same label.
Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 60 +++++++++---------------------
1 file changed, 18 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 6ee01626d55c..8d407d71e9db 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -1117,64 +1117,44 @@ static int mtk_dsi_probe(struct platform_device *pdev)
dsi->driver_data = of_device_get_match_data(dev);
dsi->engine_clk = devm_clk_get(dev, "engine");
- if (IS_ERR(dsi->engine_clk)) {
- ret = PTR_ERR(dsi->engine_clk);
+ if (IS_ERR(dsi->engine_clk))
+ return dev_err_probe(dev, PTR_ERR(dsi->engine_clk),
+ "Failed to get engine clock\n");
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "Failed to get engine clock: %d\n", ret);
- return ret;
- }
dsi->digital_clk = devm_clk_get(dev, "digital");
- if (IS_ERR(dsi->digital_clk)) {
- ret = PTR_ERR(dsi->digital_clk);
-
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "Failed to get digital clock: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(dsi->digital_clk))
+ return dev_err_probe(dev, PTR_ERR(dsi->digital_clk),
+ "Failed to get digital clock\n");
dsi->hs_clk = devm_clk_get(dev, "hs");
- if (IS_ERR(dsi->hs_clk)) {
- ret = PTR_ERR(dsi->hs_clk);
- dev_err(dev, "Failed to get hs clock: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(dsi->hs_clk))
+ return dev_err_probe(dev, PTR_ERR(dsi->hs_clk), "Failed to get hs clock\n");
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dsi->regs = devm_ioremap_resource(dev, regs);
- if (IS_ERR(dsi->regs)) {
- ret = PTR_ERR(dsi->regs);
- dev_err(dev, "Failed to ioremap memory: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(dsi->regs))
+ return dev_err_probe(dev, PTR_ERR(dsi->regs), "Failed to ioremap memory\n");
dsi->phy = devm_phy_get(dev, "dphy");
- if (IS_ERR(dsi->phy)) {
- ret = PTR_ERR(dsi->phy);
- dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(dsi->phy))
+ return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get MIPI-DPHY\n");
irq_num = platform_get_irq(pdev, 0);
- if (irq_num < 0) {
- ret = irq_num;
- return ret;
- }
+ if (irq_num < 0)
+ return irq_num;
dsi->host.ops = &mtk_dsi_ops;
dsi->host.dev = dev;
ret = mipi_dsi_host_register(&dsi->host);
- if (ret < 0) {
- dev_err(dev, "failed to register DSI host: %d\n", ret);
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to register DSI host\n");
ret = devm_request_irq(&pdev->dev, irq_num, mtk_dsi_irq,
IRQF_TRIGGER_NONE, dev_name(&pdev->dev), dsi);
if (ret) {
- dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
- goto err_unregister_host;
+ mipi_dsi_host_unregister(&dsi->host);
+ return dev_err_probe(&pdev->dev, ret, "Failed to request DSI irq\n");
}
init_waitqueue_head(&dsi->irq_wait_queue);
@@ -1186,10 +1166,6 @@ static int mtk_dsi_probe(struct platform_device *pdev)
dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
return 0;
-
-err_unregister_host:
- mipi_dsi_host_unregister(&dsi->host);
- return ret;
}
static void mtk_dsi_remove(struct platform_device *pdev)
--
2.43.0
Reviewed-by: Alexandre Mergnat <[email protected]>
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> The register bits definitions for RGB666 formats are wrong in multiple
> ways: first, in the DSI_PS_SEL bits region, the Packed 18-bits RGB666
> format is selected with bit 1, while the Loosely Packed one is bit 2,
> and second - the definition name "LOOSELY_PS_18BIT_RGB666" is wrong
> because the loosely packed format is 24 bits instead!
>
> Either way, functions mtk_dsi_ps_control_vact() and mtk_dsi_ps_control()
> do not even agree on the DSI_PS_SEL bit to set in DSI_PSCTRL: one sets
> loosely packed (24) on RGB666, the other sets packed (18), and the other
> way around for RGB666_PACKED.
>
> Fixing this entire stack of issues is done in one go:
> - Use the correct bit for the Loosely Packed RGB666 definition
> - Rename LOOSELY_PS_18BIT_RGB666 to LOOSELY_PS_24BIT_RGB666
> - Change ps_bpp_mode in mtk_dsi_ps_control_vact() to set:
> - Loosely Packed, 24-bits for MIPI_DSI_FMT_RGB666
> - Packed, 18-bits for MIPI_DSI_FMT_RGB666_PACKED
--
Regards,
Alexandre
Reviewed-by: Alexandre Mergnat <[email protected]>
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> Function mtk_dsi_ps_control() is a subset of mtk_dsi_ps_control_vact():
> merge the two in one mtk_dsi_ps_control() function by adding one
> function parameter `config_vact` which, when true, writes the VACT
> related registers.
--
Regards,
Alexandre
Reviewed-by: Alexandre Mergnat <[email protected]>
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> Instead of open coding bitshifting for various register fields,
> use the bitfield macro FIELD_PREP(): this allows to enhance the
> human readability, decrease likeliness of mistakes (and register
> field overflowing) and also to simplify the code.
> The latter is especially seen in mtk_dsi_rxtx_control(), where
> it was possible to change a switch to a short for loop and to
> also remove the need to check for maximum DSI lanes == 4 thanks
> to the FIELD_PREP macro masking the value.
>
> While at it, also add the missing DA_HS_SYNC bitmask, used in
> mtk_dsi_phy_timconfig().
--
Regards,
Alexandre
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> Change magic numerical masks with usage of the GENMASK() macro
> to improve readability.
>
> While at it, also fix the DSI_PS_SEL mask to include all bits instead
> of just a subset of them.
>
> This commit brings no functional changes.
>
> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++---------------
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index a2fdfc8ddb15..3b7392c03b4d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -58,18 +58,18 @@
>
..snip...
> #define DSI_PSCTRL 0x1c
> -#define DSI_PS_WC 0x3fff
> -#define DSI_PS_SEL (3 << 16)
> +#define DSI_PS_WC GENMASK(14, 0)
> +#define DSI_PS_SEL GENMASK(19, 16)
0011 0000 0000 0000 0000 => GENMASK(17, 16)
> #define PACKED_PS_16BIT_RGB565 (0 << 16)
> #define LOOSELY_PS_18BIT_RGB666 (1 << 16)
> #define PACKED_PS_18BIT_RGB666 (2 << 16)
> @@ -109,26 +109,26 @@
> #define LD0_WAKEUP_EN BIT(2)
--
Regards,
Alexandre
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> Registering the dsi host with its ops before getting dsi->regs is
> simply wrong: even though there's nothing (for now) asynchronously
> calling those ops before the end of the probe function, installing
> ops that are using iospace(s) and clocks before even initializing
> those is too fragile.
>
> Register the DSI host after getting clocks, iospace and PHY.
> This wil also allow to simplify the error paths in a later commit.
wil => will
Reviewed-by: Alexandre Mergnat <[email protected]>
--
Regards,
Alexandre
Reviewed-by: Alexandre Mergnat <[email protected]>
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> Instead of open coding, use the mipi_dsi_pixel_format_to_bpp() helper
> function from drm_mipi_dsi.h in mtk_dsi_poweron() and for validation
> in mtk_dsi_bridge_mode_valid().
>
> Note that this function changes the behavior of this driver: previously,
> in case of unknown formats, it would (wrongly) assume that it should
> account for a 24-bits format - now it will return an error and refuse
> to set clocks and/or enable the DSI.
>
> This is done because setting the wrong data rate will only produce a
> garbage output that the display will misinterpret both because this
> driver doesn't actually provide any extra-spec format support and/or
> because the data rate (hence, the HS clock) will be wrong.
--
Regards,
Alexandre
Reviewed-by: Alexandre Mergnat <[email protected]>
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> In mtk_dsi_phy_timconfig(), we're dividing the `data_rate` variable,
> expressed in Hz to retrieve a value in MHz: instead of open-coding,
> use the HZ_PER_MHZ definition, available in linux/units.h.
--
Regards,
Alexandre
Reviewed-by: Alexandre Mergnat <[email protected]>
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> All entries fit in 82 columns, which is acceptable: compress all of
> the mtk_dsi_of_match[] entries to a single line for each.
>
> While at it, also add the usual sentinel comment to the last entry.
--
Regards,
Alexandre
Reviewed-by: Alexandre Mergnat <[email protected]>
On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
> Most of the functions that are called in the probe callback are
> devm managed, or all but mipi_dsi_host_register(): simplify the probe
> function's error paths with dev_err_probe() and remove the lonely
> instance of `goto err_unregister_host` by just directly calling the
> mipi_dsi_host_unregister() function in the devm_request_irq() error
> path, allowing to also remove the same label.
--
Regards,
Alexandre
Il 06/02/24 15:47, Alexandre Mergnat ha scritto:
>
>
> On 06/02/2024 13:07, AngeloGioacchino Del Regno wrote:
>> Change magic numerical masks with usage of the GENMASK() macro
>> to improve readability.
>>
>> While at it, also fix the DSI_PS_SEL mask to include all bits instead
>> of just a subset of them.
>>
>> This commit brings no functional changes.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <[email protected]>
>> ---
>> drivers/gpu/drm/mediatek/mtk_dsi.c | 45 +++++++++++++++---------------
>> 1 file changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index a2fdfc8ddb15..3b7392c03b4d 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> @@ -58,18 +58,18 @@
>
> ...snip...
>
>> #define DSI_PSCTRL 0x1c
>> -#define DSI_PS_WC 0x3fff
>> -#define DSI_PS_SEL (3 << 16)
>> +#define DSI_PS_WC GENMASK(14, 0)
>> +#define DSI_PS_SEL GENMASK(19, 16)
>
> 0011 0000 0000 0000 0000 => GENMASK(17, 16)
Alexandre, the reason for that is in the commit description :-P
"While at it, also fix the DSI_PS_SEL mask to include all bits instead
of just a subset of them."
Thanks for the reviews, btw!
Cheers!
Angelo
>
>> #define PACKED_PS_16BIT_RGB565 (0 << 16)
>> #define LOOSELY_PS_18BIT_RGB666 (1 << 16)
>> #define PACKED_PS_18BIT_RGB666 (2 << 16)
>> @@ -109,26 +109,26 @@
>> #define LD0_WAKEUP_EN BIT(2)
>
On 06/02/2024 16:53, AngeloGioacchino Del Regno wrote:
>>> #define DSI_PSCTRL 0x1c
>>> -#define DSI_PS_WC 0x3fff
>>> -#define DSI_PS_SEL (3 << 16)
>>> +#define DSI_PS_WC GENMASK(14, 0)
>>> +#define DSI_PS_SEL GENMASK(19, 16)
>>
>> 0011 0000 0000 0000 0000 => GENMASK(17, 16)
>
> Alexandre, the reason for that is in the commit description :-P
>
> "While at it, also fix the DSI_PS_SEL mask to include all bits instead
> of just a subset of them."
Oh sorry...
Reviewed-by: Alexandre Mergnat <[email protected]>
--
Regards,
Alexandre