Re: [PATCH v2 10/15] drm/bridge: starfive: Add JH7110 HDMI controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Naming collision for 'pclk' causes the generic driver to permanently enable the pixel clock and breaks I2C/EDID reads.
- [High] Manual teardown of clocks and reset in remove() and probe error paths violates devres ordering, risking a kernel panic.
- [Medium] PHY power state and pixel clock reference counts are leaked if the driver is unbound while active.
--

commit c6a610527cc7d181c41335780cfb8a12665b7239
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,
acting as a glue layer between the JH7110 HDMI subsystem and the generic
Innosilicon HDMI bridge driver. It handles shared regmaps, manages clock
hierarchies from voutcrg, and configures the PHY's pixel clock rate dynamically.

> 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..be865aa39e33f
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/jh7110-inno-hdmi.c

[ ... ]

> +static int starfive_inno_hdmi_controller_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device *parent = dev->parent;
> +	struct stf_inno_hdmi_controller *ctrl;
> +	const struct inno_hdmi_plat_data *plat_data;
> +	struct regmap *regmap;
> +	struct inno_hdmi *inno;
> +	int ret;
> +
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	ctrl->dev = dev;
> +	platform_set_drvdata(pdev, ctrl);
> +
> +	/* 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;
> +	}
> +
> +	ctrl->phy = devm_phy_get(dev, "hdmi-phy");
> +	if (IS_ERR(ctrl->phy))
> +		return dev_err_probe(dev, PTR_ERR(ctrl->phy), "Failed to get PHY\n");
> +
> +	ctrl->tx_rst = devm_reset_control_get_exclusive(dev, "hdmi_tx");
> +	if (IS_ERR(ctrl->tx_rst))
> +		return dev_err_probe(dev, PTR_ERR(ctrl->tx_rst), "failed to get tx reset\n");
> +
> +	/* Populate the clock names this controller *consumes* */
> +	ctrl->clks[CLK_SYS].id = "sys";
> +	ctrl->clks[CLK_M].id = "mclk";
> +	ctrl->clks[CLK_B].id = "bclk";
> +	ctrl->clks[CLK_PCLK].id = "pclk"; /* Pixel clock *from* PHY */

[Severity: High]
Does this naming collision for 'pclk' cause the generic driver to permanently
enable the pixel clock and break I2C/EDID reads?

The wrapper driver maps the 'pclk' string ID to the dynamically-managed pixel
clock from the PHY. However, the generic driver inno_hdmi_probe() in
drivers/gpu/drm/bridge/inno-hdmi.c expects 'pclk' to be the APB/peripheral
clock and grabs it unconditionally:

inno-hdmi.c:inno_hdmi_probe() {
    ...
    hdmi->pclk = devm_clk_get_enabled(hdmi->dev, "pclk");
    ...
}

Because both query the same device tree node, the generic driver grabs the
pixel clock and permanently enables it at probe time.

Furthermore, inno_hdmi_init_hw() uses clk_get_rate(hdmi->pclk) at probe time
to calculate the DDC I2C clock divider. Since the pixel clock rate isn't set
until the .enable callback is invoked during modesetting, it will return 0 or
a default rate. This results in a 0 Hz DDC bus frequency, causing I2C timeouts
and breaking EDID reading.

> +
> +	ret = devm_clk_bulk_get(dev, CLK_CTRL_NUM, ctrl->clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Unable to get controller clocks\n");
> +
> +	/* pclk is enabled on demand during modeset */
> +	ret = clk_bulk_prepare_enable(CLK_CTRL_NUM - 1, ctrl->clks);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(ctrl->tx_rst);
> +	if (ret) {
> +		clk_bulk_disable_unprepare(CLK_CTRL_NUM - 1, ctrl->clks);
> +		return ret;
> +	}
> +
> +	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);
> +	if (IS_ERR(inno)) {
> +		reset_control_assert(ctrl->tx_rst);
> +		clk_bulk_disable_unprepare(CLK_CTRL_NUM - 1, ctrl->clks);

[Severity: High]
Can this manual teardown of clocks and reset violate devres ordering and risk
a kernel panic?

The driver manually enables bus clocks and deasserts reset in probe before
calling inno_hdmi_probe(). The generic inno_hdmi_probe() registers a shared
interrupt handler (inno_hdmi_hardirq) via devm_request_threaded_irq().

In the probe error path here, and in starfive_inno_hdmi_controller_remove()
below, the driver manually disables the clocks and asserts the reset. Because
the IRQ is managed by devres, it is not freed until after the probe error
path finishes or the remove function returns.

If a shared interrupt fires in the window between the manual clock disable
and devres tearing down the IRQ, inno_hdmi_hardirq() will attempt to read
the HDMI_STATUS register. Accessing registers with the APB clock disabled
causes a bus hang.

The driver should use devm_add_action_or_reset() to manage the clocks and
reset safely.

> +		return PTR_ERR(inno);
> +	}
> +
> +	return 0;
> +}
> +
> +static void starfive_inno_hdmi_controller_remove(struct platform_device *pdev)
> +{
> +	struct stf_inno_hdmi_controller *ctrl = platform_get_drvdata(pdev);
> +
> +	reset_control_assert(ctrl->tx_rst);
> +	clk_bulk_disable_unprepare(CLK_CTRL_NUM - 1, ctrl->clks);
> +}

[Severity: Medium]
Will this remove function leak the PHY power state and pixel clock reference
counts if the driver is unbound while active?

During modesetting, inno_hdmi_starfive_enable() dynamically prepares and
enables the pclk and calls phy_power_on(), recording this in ctrl->enabled.
If the driver is unbound while the display pipeline is active, this remove
function executes but does not check ctrl->enabled.

It omits the required phy_power_off() and clk_disable_unprepare() calls.
While devres unwinds and drops the clock and PHY pointers, it does not
reverse the dynamic power state or decrement the prepare/enable counts,
leaving the hardware permanently on.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-jh7110-clean-send-v2-0-331680c8b9d1@samsung.com?part=10

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy