Re: [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Implicit 64-bit division using the `/` operator causes link errors on 32-bit architectures.
- [Low] Integer overflow in `dsi->mode.clock * 1000` for high pixel clocks.
- [Low] Redundant picosecond period conversion introduces precision loss and forces 64-bit math.
--

--- Patch [5]: [PATCH v3 5/5] drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing ---
commit 54cf1e5377e7b6795c12c78939b18d1607d54031
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: nwl-dsi: Correct MIPI DSI horizontal sync timing

The commit updates the NWL MIPI Host controller's horizontal sync timing
calculations. It converts hfp, hsa, and hbp to DSI packet bytes and
subtracts the packet header size. This aligns the timing configuration
with the i.MX 8M reference manual.

> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index e90588299c5ea..87d22b3bd79c7 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
[ ... ]
> @@ -317,9 +322,62 @@ static int nwl_dsi_config_dpi(struct nwl_dsi *dsi)
>  			      dsi->mode.hdisplay);
>  	}
>  
> -	nwl_dsi_write(dsi, NWL_DSI_HFP, hfront_porch);
> -	nwl_dsi_write(dsi, NWL_DSI_HBP, hback_porch);
> -	nwl_dsi_write(dsi, NWL_DSI_HSA, hsync_len);
> +	pclk_period = DIV_ROUND_UP_ULL(PSEC_PER_SEC, dsi->mode.clock * 1000);

[Severity: Low]
Could this multiplication overflow on high pixel clocks? dsi->mode.clock is a
signed 32-bit integer, so multiplying by 1000 could overflow if the clock
exceeds roughly 2.147 GHz. The resulting negative number would then be
sign-extended into a large unsigned long long for DIV_ROUND_UP_ULL(). Perhaps
casting to 1000ULL would avoid this?

> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "pclk_period: %llu\n", pclk_period);
> +
> +	hs_period = DIV_ROUND_UP_ULL(PSEC_PER_SEC, dsi->phy_cfg.mipi_dphy.hs_clk_rate);
> +	DRM_DEV_DEBUG_DRIVER(dsi->dev, "hs_period: %llu\n", hs_period);
> +
> +	/*
> +	 * Calculate the bytes needed, according to the RM formula:
> +	 * Time of DPI event = time to transmit x number of bytes on the DSI
> +	 * interface
> +	 * dpi_event_size * dpi_pclk_period = dsi_bytes * 8 * hs_bit_period /
> +	 * num_lanes
> +	 * ===>
> +	 * dsi_bytes = dpi_event_size * dpi_pclk_period * num_lanes /
> +	 * (8 * hs_bit_period)
> +	 */
> +	hfp = hfront_porch * pclk_period * dsi->lanes / (8 * hs_period);
> +	hbp = hback_porch * pclk_period * dsi->lanes / (8 * hs_period);
> +	hsa = hsync_len * pclk_period * dsi->lanes / (8 * hs_period);

[Severity: Low]
Does calculating the period in picoseconds introduce unnecessary precision loss?
By calculating pclk_period and hs_period first and then dividing them, the
math effectively does (10^12 / pclk) / (10^12 / hs_clk). For example, 148.5MHz
becomes 6735ps instead of 6734.0067ps. Using the raw clock rates directly
might eliminate intermediate rounding errors.

[Severity: High]
Will this standard division operator cause build failures on 32-bit platforms?
pclk_period and hs_period are 64-bit variables, so using the / operator here
will result in undefined reference to __udivdi3 during linking on 32-bit
architectures. Using div64_u64() or refactoring to use a 32-bit denominator
would prevent this link error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810-nwl-dsi-fixes-v3-0-256da9d672e1@geanix.com?part=5