Re: [PATCH v3 14/19] drm/bridge: starfive: Add JH7110 HDMI controller driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `regmap` variable is assigned and checked but never used or passed to the generic driver, contradicting the commit message.
- [Medium] Missing mandatory `phy_init()` and `phy_exit()` calls for the HDMI PHY.
--

commit a25b9a0e71017577ee552276d0b9a0aa22a5f9f0
Author: Michal Wilczynski [off-list ref]
drm/bridge: starfive: Add JH7110 HDMI controller driver

This commit adds the HDMI controller (bridge) driver for the StarFive
JH7110. It registers as a DRM bridge, manages clock rates and PHY power,
and configures the mux setting based on the port graph.

The driver calls the generic inno_hdmi_probe function and passes the
shared regmap to it, registering as a DRM bridge.

[Severity: Medium]
Is this description accurate regarding the regmap? Looking at the
starfive_inno_hdmi_controller_probe() function below, the regmap is fetched
but doesn't appear to actually be passed to inno_hdmi_probe() or used
anywhere else.

[ ... ]

+static void inno_hdmi_starfive_enable(struct device *dev,
+				      struct drm_display_mode *mode)
+{

[ ... ]

+	/*
+	 * 3. Power on the PHY. This calls the PHY driver's .power_on op,
+	 * which configures the Post-PLL and analog blocks.
+	 */
+	ret = phy_power_on(ctrl->phy);

[Severity: Medium]
Does this code omit a required initialization step? The generic PHY API
strictly requires phy_init() to be called before phy_power_on(). Even if
the current starfive-inno-hdmi-phy driver lacks an .init callback, omitting
phy_init() here violates the API contract and could lead to initialization
failures if the PHY driver is updated or a different PHY is bound.

+	if (ret) {
+		dev_err(dev, "Failed to power on PHY: %d\n", ret);
+		clk_disable_unprepare(ctrl->clks[CLK_PCLK].clk);
+		return;
+	}
+
+	ctrl->enabled = true;
+}
+
+static void inno_hdmi_starfive_disable(struct device *dev)
+{
+	struct stf_inno_hdmi_controller *ctrl = dev_get_drvdata(dev);
+
+	/*
+	 * .enable bails out early if the pixel clock rate is unsupported or
+	 * the PHY fails to power on, leaving pclk and the PHY untouched.
+	 * Only tear down what was actually brought up, otherwise the clock
+	 * refcount underflows.
+	 */
+	if (!ctrl->enabled)
+		return;
+
+	phy_power_off(ctrl->phy);

[Severity: Medium]
Should there be a call to phy_exit() here? Following the same API contract,
phy_exit() should generally be invoked to clean up resources after
phy_power_off() completes.

+	clk_disable_unprepare(ctrl->clks[CLK_PCLK].clk);
+	ctrl->enabled = false;
+}

[ ... ]

+static int starfive_inno_hdmi_controller_probe(struct platform_device *pdev)
+{

[ ... ]

+	/* Get the shared regmap from the parent */
+	regmap = dev_get_regmap(parent, NULL);
+	if (!regmap) {
+		dev_err(dev, "Failed to get parent regmap\n");
+		return -ENODEV;
+	}

[Severity: Medium]
Could this be dead code? The regmap is fetched and checked here, but
subsequent code within starfive_inno_hdmi_controller_probe() never uses it.

[ ... ]

+	plat_data = of_device_get_match_data(dev);
+
+	/* Hand off to the generic library to create the bridge. */
+	inno = inno_hdmi_probe(pdev, plat_data);

[Severity: Medium]
As mentioned regarding the commit message, should the regmap have been
passed to inno_hdmi_probe() here?

+	if (IS_ERR(inno))
+		return PTR_ERR(inno);
+
+	return 0;
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-jh7110-clean-send-v3-0-484f9ae72715@samsung.com?part=14