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 <[email protected]> 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/[email protected]?part=5
