Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm/bridge: missing GENERIC_PHY_MIPI_DPHY and MFD_SYSCON select in Kconfig - [Medium] drm/bridge: uninitialized ret in th1520_dw_mipi_dsi_probe() error path - [Medium] drm/bridge: 32-bit signed overflow in calculate_lane_mbps() --
commit 1daf55a66c6c757f635f5bb068b6e103e284e178 Author: Icenowy Zheng <[email protected]> drm/bridge: add TH1520 DesignWare MIPI DSI host controller This commit adds a glue driver for the Synopsys DesignWare MIPI DSI host controller and internal D-PHY on the T-Head TH1520 SoC. > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 4a57d49b4c6d3..b005623ba26bf 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -375,6 +375,16 @@ config DRM_THEAD_TH1520_DW_HDMI > Choose this to enable support for the internal HDMI bridge found > on the T-Head TH1520 SoC. > > +config DRM_THEAD_TH1520_DW_MIPI_DSI > + tristate "T-Head TH1520 DesignWare MIPI DSI bridge" > + depends on OF > + depends on COMMON_CLK > + depends on ARCH_THEAD || COMPILE_TEST > + select DRM_DW_MIPI_DSI > + help > + Choose this to enable support for the internal MIPI DSI bridge > + found on the T-Head TH1520 SoC. [Severity: Medium] Should DRM_THEAD_TH1520_DW_MIPI_DSI in drivers/gpu/drm/bridge/Kconfig also select GENERIC_PHY_MIPI_DPHY and MFD_SYSCON? In drivers/gpu/drm/bridge/th1520-dw-mipi-dsi.c, calculate_lane_mbps() calls phy_mipi_dphy_get_default_config(), which has no inline stub fallback when CONFIG_GENERIC_PHY_MIPI_DPHY is disabled. Because GENERIC_PHY_MIPI_DPHY is a promptless symbol and DRM_DW_MIPI_DSI does not select it, building with CONFIG_DRM_THEAD_TH1520_DW_MIPI_DSI enabled will fail at link time if no other enabled driver selects GENERIC_PHY_MIPI_DPHY. Similarly, th1520_dw_mipi_dsi_probe() calls syscon_regmap_lookup_by_phandle() and dsi_phy_init() calls regmap_update_bits(). When CONFIG_MFD_SYSCON is not enabled, syscon_regmap_lookup_by_phandle() falls back to a stub returning ERR_PTR(-ENOTSUPP), causing probe to unconditionally fail. > diff --git a/drivers/gpu/drm/bridge/th1520-dw-mipi-dsi.c > b/drivers/gpu/drm/bridge/th1520-dw-mipi-dsi.c > new file mode 100644 > index 0000000000000..e5c7c5d6dcc71 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/th1520-dw-mipi-dsi.c [ ... ] > +static unsigned int > +calculate_lane_mbps(const struct drm_display_mode *mode, > + unsigned long mode_flags, > + u32 lanes, u32 format) > +{ > + int bpp = mipi_dsi_pixel_format_to_bpp(format); > + union phy_configure_opts phy_opts; > + > + /* > + * FIXME: the extra bandwidth headroom logic mimics what the Rockchip > + * driver does, and test shows that it's necessary for the hardware > + * to work with the mainline dw-mipi-dsi common code. > + */ > + phy_mipi_dphy_get_default_config(mode->clock * 1000 * 10 / 8, > + bpp, lanes, > + &phy_opts.mipi_dphy); [Severity: Medium] Can the pixel clock multiplication in calculate_lane_mbps() overflow a 32-bit signed int? Since mode->clock in struct drm_display_mode is an int (in kHz) and 1000, 10, and 8 are signed int literals, mode->clock * 1000 * 10 is evaluated in 32-bit signed arithmetic before being converted to the unsigned long parameter of phy_mipi_dphy_get_default_config(). When mode->clock is 214749 kHz or higher, mode->clock * 1000 * 10 exceeds INT_MAX. With 4 lanes and up to 2500 Mbps per lane in hsfreqrange_table, the hardware supports 24-bpp modes up to 333333 kHz, which will wrap to a negative value, sign-extend to a large unsigned long, and be rejected by dsi_mode_valid() and dsi_get_lane_mbps(). Higher out-of-range pixel clocks (such as 594000 kHz) can also wrap around to a small positive value and falsely pass dsi_mode_valid(). Would using 1000UL (or mul_u32_u32()) avoid this overflow? > + > + return DIV_ROUND_UP(phy_opts.mipi_dphy.hs_clk_rate, USEC_PER_SEC); > +} [ ... ] > +static int th1520_dw_mipi_dsi_probe(struct platform_device *pdev) > +{ > + const struct dsi_chip_data *cdata; > + struct device *dev = &pdev->dev; > + struct th1520_dw_mipi_dsi *dsi; > + struct resource *res; > + void __iomem *base; > + int i, ret; > + > + dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); > + if (!dsi) > + return -ENOMEM; > + > + dsi->dev = dev; > + > + dsi->syscon = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon"); > + if (IS_ERR(dsi->syscon)) { > + ret = PTR_ERR(dsi->syscon); > + dev_err_probe(dev, ret, "Unable to get system controller > regmap\n"); > + return ret; > + } > + > + base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > + if (IS_ERR(base)) { > + dev_err_probe(dev, ret, "Unable to get DSI registers\n"); > + return PTR_ERR(base);\n> + } [Severity: Medium] In th1520_dw_mipi_dsi_probe(), if syscon_regmap_lookup_by_phandle() succeeds and devm_platform_get_and_ioremap_resource() fails, isn't ret uninitialized when passed to dev_err_probe()? Should this pass PTR_ERR(base) (or return dev_err_probe(dev, PTR_ERR(base), ...)) instead? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
