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

[Philipp Zabel <p.zabel@pengutronix.de>]

On Sa, 2026-07-25 at 00:51 -0400, Cody Kang via B4 Relay wrote:
> From: Cody Kang <codykang.hk@gmail.com>
> 
> 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 <codykang.hk@gmail.com>
> ---
>  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(+)
> 
[...]
> diff --git a/drivers/gpu/drm/spacemit/spacemit_inno_dp.c b/drivers/gpu/drm/spacemit/spacemit_inno_dp.c
> new file mode 100644
> index 000000000000..e88731d9b18b
> --- /dev/null
> +++ b/drivers/gpu/drm/spacemit/spacemit_inno_dp.c
> @@ -0,0 +1,2443 @@
[...]
> +static int inno_dp_probe(struct platform_device *pdev)
> +{
[...]
> +	dp->reset = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);

Since this is only asserted in probe and deasserted in remove, you
could use devm_reset_control_get_optional_exclusive_deasserted() [1].

[1] https://docs.kernel.org/driver-api/reset.html#c.devm_reset_control_get_optional_exclusive_deasserted

> +	if (IS_ERR(dp->reset)) {
> +		ret = dev_err_probe(dev, PTR_ERR(dp->reset),
> +				    "failed to get reset\n");
> +		return 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;
> +	}
> +
> +	if (dp->edp_mode) {
> +		dp->bridge.type = DRM_MODE_CONNECTOR_eDP;
> +		/*
> +		 * OP_DETECT is required: without it
> +		 * drm_bridge_connector_detect() hard-codes CONNECTED for any
> +		 * eDP connector and fbdev wedges at bind.
> +		 */
> +		dp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> +				 DRM_BRIDGE_OP_HPD;
> +	} else {
> +		dp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> +		/*
> +		 * No OP_HPD: the HPD interrupt is gated by the DP pixel clock.
> +		 * Leaving it out marks the connector POLL_CONNECT |
> +		 * POLL_DISCONNECT instead.
> +		 */
> +		dp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> +				 DRM_BRIDGE_OP_MODES;
> +	}
> +
> +	if (dp->reset) {

This check is not necessary, reset_control_deassert() handles NULL
pointers.

> +		ret = reset_control_deassert(dp->reset);

If the _deasserted() variant is used above, this can be removed and the
reset control doesn't need to be stored in dp.

> +		if (ret) {
> +			dev_err(dev, "failed to deassert reset: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
[...]
> +	return 0;
> +
> +err_clk:
> +	clk_disable_unprepare(dp->pxclk);
> +err_reset:
> +	if (dp->reset)
> +		reset_control_assert(dp->reset);

Same as above. Remove the unnecessary check or manual assertion
entirely.

> +
> +	return ret;
> +}
> +
> +static void inno_dp_remove(struct platform_device *pdev)
> +{
> +	struct spacemit_dp_dev *dp = platform_get_drvdata(pdev);
> +
> +	component_del(&pdev->dev, &spacemit_dp_ops);
> +
> +	if (dp) {
> +		clk_disable_unprepare(dp->pxclk);
> +		if (dp->reset)
> +			reset_control_assert(dp->reset);

Same as above.

> +	}
> +}

regards
Philipp