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

[sashiko-bot@kernel.org]

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 <m.wilczynski@samsung.com>
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.

> diff --git a/drivers/gpu/drm/bridge/jh7110-inno-hdmi.c b/drivers/gpu/drm/bridge/jh7110-inno-hdmi.c
> new file mode 100644
> index 0000000000000..b0bf6abaa55fb
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/jh7110-inno-hdmi.c
[ ... ]
> +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