Re: [PATCH RESEND 12/17] drm/spacemit: add Innosilicon DP/eDP controller bridge driver

On Sat, Jul 25, 2026 at 12:51:21AM -0400, Cody Kang via B4 Relay wrote:
> From: Cody Kang <[email protected]>
> 
> Add the DP/eDP controller that sits downstream of the Saturn DPU. Two
> identical instances share one compatible; the eDP-vs-DP role is board
> wiring, so it is taken from the devicetree: an eDP panel always sits
> under an aux-bus child node, an external DP connector never does.
> 
> The link is driven through the generic PHY framework, so the controller
> never touches a PLL register. The controller's HPD interrupt is gated by
> the DP pixel clock, which can be off exactly when a plug has to be
> caught, so the connector is also polled and the interrupt path re-reads
> the live level when it does fire.
> 
> Signed-off-by: Cody Kang <[email protected]>
> ---
>  drivers/gpu/drm/spacemit/Kconfig            |   19 +
>  drivers/gpu/drm/spacemit/Makefile           |    3 +
>  drivers/gpu/drm/spacemit/spacemit_inno_dp.c | 2443 +++++++++++++++++++++++++++
>  drivers/gpu/drm/spacemit/spacemit_inno_dp.h |  328 ++++
>  4 files changed, 2793 insertions(+)

...

> +static int inno_dp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *aux_bus_np;
> +	struct spacemit_dp_dev *dp;
> +	struct resource *res;
> +	int ret;

...

> +	dp->pxclk = devm_clk_get(dev, "pxclk");
> +	if (IS_ERR(dp->pxclk)) {
> +		ret = dev_err_probe(dev, PTR_ERR(dp->pxclk),
> +				    "failed to get pxclk\n");
> +		return ret;
> +	}

It seems pxclk is only enabled in probe() and disabled in remove(),
please consider using devm_clk_get_optional_enabled().

...

> +	if (dp->pxclk) {
> +		ret = clk_prepare_enable(dp->pxclk);
> +		if (ret) {
> +			dev_err(dev, "failed to enable pxclk: %d\n", ret);
> +			goto err_reset;
> +		}
> +	}

So this check could be dropped.

...

> +	/*
> +	 * The PHY exposes its PLL as the APMU pixel-clock mux's external
> +	 * parent.
> +	 */
> +	dp->pll_clk = devm_clk_get(dev, "pll");
> +	if (IS_ERR(dp->pll_clk)) {
> +		ret = dev_err_probe(dev, PTR_ERR(dp->pll_clk),
> +				    "failed to get PHY pixel clock\n");
> +		goto err_clk;
> +	}

Same for the "pll" clock.

> +	if (dp->pxclk) {
> +		ret = clk_set_parent(dp->pxclk, dp->pll_clk);
> +		if (ret) {
> +			dev_err(dev, "failed to route eDP pixel mux to PHY PLL: %d\n", ret);
> +			goto err_clk;
> +		}
> +	}
> +	 */
> +	ret = devm_request_threaded_irq(dev, dp->irq, spacemit_dp_irq_handler,
> +					spacemit_dp_hotplug_event_handler,
> +					IRQF_NO_AUTOEN, dev_name(dev), dp);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq %d: %d\n", dp->irq, ret);
> +		goto err_clk;
> +	}

Since 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()") devm_request_threaded_irq() automatically throws
an error message when it fails, so this error message is redundant.

...

> +err_clk:
> +	clk_disable_unprepare(dp->pxclk);

With devm_clk_get_optional_enabled(), you could remove this and
simplify some error handling paths.

Regards,
Yao Zi