Re: [PATCH v4 1/3] drm/bridge/synopsys: Add MIPI DSI2 host controller bridge

Hi Chris,

Am Montag, 30. Dezember 2024, 03:20:55 CET schrieb Chris Hofstaedtler:

On Tue, Dec 10, 2024 at 12:10:19AM +0100, Heiko Stuebner wrote:

quoted

From: Heiko Stuebner <redacted>

Add a Synopsys Designware MIPI DSI host DRM bridge driver for their
DSI2 host controller, based on the Rockchip version from the driver
rockchip/dw-mipi-dsi2.c in their vendor-kernel with phy & bridge APIs.

While the driver is heavily modelled after the previous IP, the register
set of this DSI2 controller is completely different and there are also
additional properties like the variable-width phy interface.

Tested-by: Daniel Semkowicz <redacted>
Tested-by: Dmitry Yashin <redacted>
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Heiko Stuebner <redacted>

[..]

quoted

+static void dw_mipi_dsi2_set_vid_mode(struct dw_mipi_dsi2 *dsi2)
+{
+	u32 val = 0, mode;
+	int ret;
+
+	if (dsi2->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP)
+		val |= BLK_HFP_HS_EN;
+
+	if (dsi2->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HBP)
+		val |= BLK_HBP_HS_EN;
+
+	if (dsi2->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)
+		val |= BLK_HSA_HS_EN;

For all three of these: is setting an ENable bit the right thing to
turn features *off*?

first of all, thanks a lot for noticing this discrepancy :-) .

Looking at the documentation, all 3 of those hw-bits are described as
  "Enables filling the H.. period with blanking packets. ..."

where the MIPI_DSI_VIDEO_MODE_NO_* flags are described as
  "disable hfront-porch/... area"


So yes, I _think_ "disable front-porch" would _should_ result in
"don't fill the period with blanking packets", but am not fully sure.

I've run the two boards I have with inverting the checks as sounds
sensible right now, aka doing:
	if (!(dsi2->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HFP))
etc and both displays I have ran just fine.

As the driver was originally part of a vendor-kernel based on 5.10, which
I think was before the _NO addition from [0] it could be caused by a
misread of the flags that were named differently back then.


So yes, switching things around does sound like the right thing to do.


Heiko

[0] https://lore.kernel.org/all/20210629074703.v2.1.I629b2366a6591410359c7fcf6d385b474b705ca2@changeid/ (local)