Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Logic error in pointer error checking breaks probe for all existing boards using extcon. - [High] Missing hotplug detection (HPD) mechanism when extcon is absent. --
--- Patch [6]: [PATCH v6 6/7] drm/rockchip: cdn-dp: Support handle lane info without extcon --- commit 2ec5f110f1557ebaec82cbe6b39d4e2ab7f4a88f Author: Chaoyi Chen <[email protected]> drm/rockchip: cdn-dp: Support handle lane info without extcon This patch add support for get PHY lane info without help of extcon. There is no extcon needed if the Type-C controller is present. In this case, the lane info can be get from PHY instead of extcon. The extcon device should still be supported if Type-C controller is not present. > diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c > b/drivers/gpu/drm/rockchip/cdn-dp-core.c > index 190cce9530c82..74e1f5422a6e2 100644 > --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c > +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c [ ... ] > @@ -1027,6 +1033,9 @@ static int cdn_dp_bind(struct device *dev, struct > device *master, void *data) > for (i = 0; i < dp->ports; i++) { > port = dp->port[i]; > > + if (!port->extcon) > + continue; > + [Severity: High] Does skipping the notifier registration when extcon is absent break hotplug detection? The driver relies on cdn_dp_pd_event_work() to transition dp->connected to true, load firmware, and train the link. Without the extcon notifier, event_work is never scheduled on hotplug. Since cdn_dp_bridge_detect() simply returns the static dp->connected state: drivers/gpu/drm/rockchip/cdn-dp-core.c:cdn_dp_bridge_detect() { mutex_lock(&dp->lock); if (dp->connected) status = connector_status_connected; mutex_unlock(&dp->lock); ... } Could KMS polling always see the port as disconnected when hardware is configured without an extcon device? > port->event_nb.notifier_call = cdn_dp_pd_event; > ret = devm_extcon_register_notifier(dp->dev, port->extcon, > EXTCON_DISP_DP, [ ... ] > @@ -1119,14 +1128,14 @@ static int cdn_dp_probe(struct platform_device *pdev) > PTR_ERR(phy) == -EPROBE_DEFER) > return -EPROBE_DEFER; > > - if (IS_ERR(extcon) || IS_ERR(phy)) > + if (IS_ERR(phy) || PTR_ERR(extcon) != -ENODEV) > continue; > [Severity: Critical] Will this condition accidentally skip port initialization for all valid extcon pointers? If extcon is a valid pointer, PTR_ERR(extcon) evaluates to its memory address, which will never equal -ENODEV. This causes the condition to evaluate to true, triggering the continue statement and bypassing port setup completely for any hardware that has an extcon device. > port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); > if (!port) > return -ENOMEM; > > - port->extcon = extcon; > + port->extcon = IS_ERR(extcon) ? NULL : extcon; > port->phy = phy; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
