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

From: "Heiko Stübner" <heiko@sntech.de>
To: Chris Hofstaedtler <zeha@debian.org>,
	andy.yan@rock-chips.com,
	Guochun Huang <hero.huang@rock-chips.com>
Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org,
	tzimmermann@suse.de, robh@kernel.org, krzk+dt@kernel.org,
	conor+dt@kernel.org, andrzej.hajda@intel.com,
	neil.armstrong@linaro.org, rfoss@kernel.org,
	Laurent.pinchart@ideasonboard.com, jonas@kwiboo.se,
	jernej.skrabec@gmail.com, dri-devel@lists.freedesktop.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	quentin.schulz@cherry.de,
	Heiko Stuebner <heiko.stuebner@cherry.de>,
	Daniel Semkowicz <dse@thaumatec.com>,
	Dmitry Yashin <dmt.yashin@gmail.com>
Subject: Re: [PATCH v4 1/3] drm/bridge/synopsys: Add MIPI DSI2 host controller bridge
Date: Tue, 14 Jan 2025 18:30:30 +0100	[thread overview]
Message-ID: <3556284.QJadu78ljV@diego> (raw)
In-Reply-To: <Z3IDh0TOAKqaovz2@per.namespace.at>

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:
> > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> > 
> > 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 <dse@thaumatec.com>
> > Tested-by: Dmitry Yashin <dmt.yashin@gmail.com>
> > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> [..]
> > +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/