Re: [PATCH v6 06/11] drm/mediatek: mtk_dsi: Transfer register offsets to per-SoC const

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Wed, 2026-07-15 at 15:56 +0200, AngeloGioacchino Del Regno wrote:
> As of now, the all of the supported SoCs have small differences in
> the register offsets for their version of the DSI IP, at least for
> the VM_CMD_CON, SHADOW_DEBUG and CMDQ offsets.
> 
> As a preparation for introducing support for newer generation DSI
> IPs, having even more differences in the register offsets (but not
> in the layout of their fields, nor in the actual programming), as
> found on Dimensity 9400 MT6991, Kompanio Ultra MT8196 and Genio
> Pro 5100 MT8894, transfer all the register offsets to two const
> arrays, splitting the DSI IP version specific registers from the
> SoC specific ones (as those depend on interfacing with CMDQ and
> other IPs external to DSI, but internal to the SoC, and embedded
> in DSI).
> 
> This change brings no functional difference, as it only changes
> how the register offsets are retrieved and nothing else.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

[snip]

>  
> -#define DSI_CON_CTRL		0x10
> +/* DSI_CON_CTRL */
>  #define DSI_RESET			BIT(0)
>  #define DSI_EN				BIT(1)
>  #define DPHY_RESET			BIT(2)
> +#define CMDMODE_WAIT_DATA_EVERY_LINE_EN	BIT(24)

This does not relate to this patch, so drop it.

>  
> -#define DSI_MODE_CTRL		0x14
> +/* DSI_MODE_CTRL */
>  #define MODE				(3)
>  #define CMD_MODE			0
>  #define SYNC_PULSE_MODE			1
> @@ -60,7 +63,7 @@
>  #define FRM_MODE			BIT(16)
>  #define MIX_MODE			BIT(17)
>  

[snip]

>  static bool mtk_dsi_clk_hs_state(struct mtk_dsi *dsi)
>  {
> -	return readl(dsi->regs + DSI_PHY_LCCON) & LC_HS_TX_EN;
> +	const u16 *regoff = dsi->driver_data->reg_main;

Other function use 'reg_main', so align to other function.

> +
> +	return readl(dsi->regs + regoff[DSI_PHY_LCCON]) & LC_HS_TX_EN;
>  }
>  

[snip]

>  
>  static void mtk_dsi_set_mode(struct mtk_dsi *dsi)
>  {
> -	u32 vid_mode = CMD_MODE;
> +	const u16 *reg_main = dsi->driver_data->reg_main;
> +	u32 vid_mode;

Remove initial setting does not relate to this patch, so drop this.

>  
>  	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
>  		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> @@ -367,19 +485,24 @@ static void mtk_dsi_set_mode(struct mtk_dsi *dsi)
>  			vid_mode = SYNC_PULSE_MODE;
>  		else
>  			vid_mode = SYNC_EVENT_MODE;
> +	} else {
> +		vid_mode = CMD_MODE;
>  	}
>  
> -	writel(vid_mode, dsi->regs + DSI_MODE_CTRL);
> +	writel(vid_mode, dsi->regs + reg_main[DSI_MODE_CTRL]);
>  }
>  

[snip]

> @@ -426,6 +549,8 @@ static void mtk_dsi_ps_control_dsc(struct mtk_dsi *dsi, bool config_vact)
>  
>  static void mtk_dsi_ps_control_uncompressed(struct mtk_dsi *dsi, bool config_vact)
>  {
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
> +	const u16 *reg_main = dsi->driver_data->reg_main;
>  	u32 dsi_buf_bpp, ps_val, ps_wc, size_val, vact_nl;
>  
>  	if (dsi->format == MIPI_DSI_FMT_RGB565)
> @@ -457,16 +582,16 @@ static void mtk_dsi_ps_control_uncompressed(struct mtk_dsi *dsi, bool config_vac
>  
>  	if (config_vact) {
>  		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(vact_nl, dsi->regs + reg_main[DSI_VACT_NL]);
> +		writel(ps_wc, dsi->regs + reg_main[DSI_HSTX_CKL_WC]);
>  	}
> -	writel(ps_val, dsi->regs + DSI_PSCTRL);
> +	writel(ps_val, dsi->regs + reg_main[DSI_PSCTRL]);
>  
> -	if (dsi->driver_data->has_size_ctl) {
> +	if (data->has_size_ctl) {

data is used only once, drop it.

>  		size_val = FIELD_PREP(DSI_HEIGHT, dsi->vm.vactive);
>  		size_val |= FIELD_PREP(DSI_WIDTH, dsi->vm.hactive);
>  
> -		writel(size_val, dsi->regs + DSI_SIZE_CON);
> +		writel(size_val, dsi->regs + reg_main[DSI_SIZE_CON]);
>  	}
>  }
>  

[snip]

> @@ -649,15 +776,17 @@ static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi)
>  
>  static int mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>  {
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
> +	const u16 *reg_main = data->reg_main;
>  	struct videomode *vm = &dsi->vm;
>  	int ret;
>  
> -	writel(vm->vsync_len, dsi->regs + DSI_VSA_NL);
> -	writel(vm->vback_porch, dsi->regs + DSI_VBP_NL);
> -	writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL);
> -	writel(vm->vactive, dsi->regs + DSI_VACT_NL);
> +	writel(vm->vsync_len, dsi->regs + reg_main[DSI_VSA_NL]);
> +	writel(vm->vback_porch, dsi->regs + reg_main[DSI_VBP_NL]);
> +	writel(vm->vfront_porch, dsi->regs + reg_main[DSI_VFP_NL]);
> +	writel(vm->vactive, dsi->regs + reg_main[DSI_VACT_NL]);
>  
> -	if (dsi->driver_data->support_per_frame_lp)
> +	if (data->support_per_frame_lp)

data is used only once, drop it.

>  		mtk_dsi_config_vdo_timing_per_frame_lp(dsi);
>  	else
>  		mtk_dsi_config_vdo_timing_per_line_lp(dsi);
> @@ -677,25 +806,25 @@ static int mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>  

[snip]

> @@ -730,19 +859,20 @@ static s32 mtk_dsi_wait_for_irq_done(struct mtk_dsi *dsi, u32 irq_flag,
>  
>  static irqreturn_t mtk_dsi_irq(int irq, void *dev_id)
>  {
> -	struct mtk_dsi *dsi = dev_id;
>  	u32 status, tmp;
> -	u32 flag = LPRX_RD_RDY_INT_FLAG | CMD_DONE_INT_FLAG | VM_DONE_INT_FLAG;
> +	struct mtk_dsi *dsi = dev_id;

Place struct before u32.

> +	const u16 *reg_main = dsi->driver_data->reg_main;
> +	const u32 flag = LPRX_RD_RDY_INT_FLAG | CMD_DONE_INT_FLAG | VM_DONE_INT_FLAG;
>  
> -	status = readl(dsi->regs + DSI_INTSTA) & flag;
> +	status = readl(dsi->regs + dsi->driver_data->reg_main[DSI_INTSTA]) & flag;

You already define reg_main.

>  
>  	if (status) {
>  		do {
> -			mtk_dsi_mask(dsi, DSI_RACK, RACK, RACK);
> -			tmp = readl(dsi->regs + DSI_INTSTA);
> +			mtk_dsi_mask(dsi, reg_main[DSI_RACK], RACK, RACK);
> +			tmp = readl(dsi->regs + reg_main[DSI_INTSTA]);
>  		} while (tmp & DSI_BUSY);
>  
> -		mtk_dsi_mask(dsi, DSI_INTSTA, status, 0);
> +		mtk_dsi_mask(dsi, reg_main[DSI_INTSTA], status, 0);
>  		mtk_dsi_irq_data_set(dsi, status);
>  		wake_up_interruptible(&dsi->irq_wait_queue);
>  	}
> @@ -781,9 +911,10 @@ static void mtk_dsi_lane_ready(struct mtk_dsi *dsi)
>  
>  static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  {
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
>  	struct device *dev = dsi->host.dev;
> -	int ret;
>  	u32 bit_per_pixel;
> +	int ret;

This is not related to this patch.

>  
>  	if (++dsi->refcount != 1)
>  		return 0;
> @@ -820,9 +951,10 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
>  
>  	mtk_dsi_enable(dsi);
>  
> -	if (dsi->driver_data->has_shadow_ctl)
> +	/* Bypass shadow and force commit only if the register is present */
> +	if (data->reg_adv[DSI_SHADOW_DEBUG])
>  		writel(FORCE_COMMIT | BYPASS_SHADOW,
> -		       dsi->regs + dsi->driver_data->reg_shadow_dbg_off);
> +		       dsi->regs + data->reg_adv[DSI_SHADOW_DEBUG]);
>  
>  	mtk_dsi_reset_engine(dsi);
>  	mtk_dsi_phy_timconfig(dsi);
> @@ -873,7 +1005,7 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
>  	mtk_dsi_lane0_ulp_mode_enter(dsi);
>  	mtk_dsi_clk_ulp_mode_enter(dsi);
>  	/* set the lane number as 0 to pull down mipi */
> -	writel(0, dsi->regs + DSI_TXRX_CTRL);
> +	writel(0, dsi->regs + dsi->driver_data->reg_main[DSI_TXRX_CTRL]);

You have defined data. Use data.

>  
>  	mtk_dsi_disable(dsi);
>  
> @@ -1173,9 +1305,10 @@ static void mtk_dsi_wait_for_idle(struct mtk_dsi *dsi)
>  	int ret;
>  	u32 val;
>  	struct drm_device *drm = dsi->bridge.dev;
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
>  
> -	ret = readl_poll_timeout(dsi->regs + DSI_INTSTA, val, !(val & DSI_BUSY),
> -				 4, 2000000);
> +	ret = readl_poll_timeout(dsi->regs + data->reg_main[DSI_INTSTA],
> +				 val, !(val & DSI_BUSY), 4, 2000000);
>  	if (ret) {
>  		drm_warn(drm, "polling dsi wait not busy timeout!\n");
>  
> @@ -1209,10 +1342,12 @@ static u32 mtk_dsi_recv_cnt(u8 type, u8 *read_data)
>  
>  static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
>  {
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
>  	const char *tx_buf = msg->tx_buf;
> -	u8 config, cmdq_size, cmdq_off, type = msg->type;
> +	const u8 type = msg->type;

Making 'type' to be const is not related to this patch.

Regards,
CK


>  	u32 reg_val, cmdq_mask, i;
> -	u32 reg_cmdq_off = dsi->driver_data->reg_cmdq_off;
> +	u8 cmdq_size, cmdq_off;
> +	u8 config;
>  
>  	if (MTK_DSI_HOST_IS_READ(type))
>  		config = BTA;
> @@ -1235,15 +1370,15 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
>  	}
>