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

["Heiko Stübner" <heiko@sntech.de>]

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/