Hi Philipp, On Mon, 27 Jul 2026 09:49:58 +0200, Philipp Zabel wrote: > On Sa, 2026-07-25 at 00:51 -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(+) > > > [...] > > 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].
Thanks for the review. Will do in v2. The binding marks resets as required, which means I shouldn't use _optional in the first place. I will use the non-optional devm_reset_control_get_exclusive_deasserted(). > [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. Agree, all of these will be fixed in v2. Cody
