Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Missing runtime PM acquisition in out-of-band HPD callbacks defeats the patch's purpose. - [Medium] dw_dp_rockchip_get_vo_grf() hard-fails instead of implementing the documented DT fallback for future platforms. --
--- Patch [13]: [PATCH v11 13/21] drm/rockchip: dw_dp: Implement out-of-band HPD handling --- commit 7506de24244286fa5e8735e9092296ca64dbef67 Author: Sebastian Reichel <[email protected]> drm/rockchip: dw_dp: Implement out-of-band HPD handling This commit implements out-of-band hotplug handling to receive external hotplug information from the USB-C state machine. It aims to prevent massive SError problems once runtime PM is implemented in the DP driver by moving the register access into the DP driver. > diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > index 38e8fe75718e4..9e49e7dbf420f 100644 > --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c [ ... ] > @@ -23,12 +26,48 @@ > > #include "rockchip_drm_drv.h" > > +#define ROCKCHIP_MAX_CTRLS 2 > + > +#define ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL BIT(10) > +#define ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG BIT(11) [ ... ] > +static void dw_dp_rockchip_hpd_sw_sel(void *data, bool force_hpd_from_sw) > +{ > + struct rockchip_dw_dp *dp = data; > + u32 hpd_reg = dp->pdata->hpd_reg[dp->id]; > + > + regmap_write(dp->vo_grf, hpd_reg, > + FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, > force_hpd_from_sw)); [Severity: Medium] Does this need to acquire a runtime PM reference before accessing the register? Since this callback can be triggered asynchronously by an out-of-band event from the Type-C TCPM, writing to the vo_grf regmap while the DP controller is suspended might trigger the exact SError this patch intends to fix, once runtime PM is enabled. > +} > + > +static void dw_dp_rockchip_hpd_sw_cfg(void *data, bool hpd) > +{ > + struct rockchip_dw_dp *dp = data; > + u32 hpd_reg = dp->pdata->hpd_reg[dp->id]; > + > + dev_dbg(dp->dev, "Force HPD connected=%s\n", str_yes_no(hpd)); > + > + regmap_write(dp->vo_grf, hpd_reg, > + FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG, hpd)); [Severity: Medium] Similarly, does this asynchronous callback need to ensure the DP power domain is active? If an asynchronous hotplug event occurs without a runtime PM guarantee, could this register access crash the system? > +} [ ... ] > +static struct regmap *dw_dp_rockchip_get_vo_grf(struct rockchip_dw_dp *dp) > +{ > + struct device_node *np = dev_of_node(dp->dev); > + struct of_phandle_args args; > + struct regmap *regmap; > + int ret; > + > + ret = of_parse_phandle_with_args(np, "phys", "#phy-cells", 0, &args); > + if (ret) > + return ERR_PTR(-ENODEV); > + > + /* > + * Limit this workaround to RK3576 and RK3588, potential future > platforms > + * reusing the driver should just add a VO GRF phandle in the > DisplayPort > + * controller DT node. > + */ > + if (!of_device_is_compatible(args.np, "rockchip,rk3576-usbdp-phy") && > + !of_device_is_compatible(args.np, "rockchip,rk3588-usbdp-phy")) { > + regmap = ERR_PTR(-ENODEV); > + goto out_put_node; > + } [Severity: Medium] Does this hard-fail prevent the documented fallback? The comment mentions future platforms should add a VO GRF phandle directly in the DisplayPort controller node, but it looks like we return ERR_PTR(-ENODEV) here without ever attempting to parse the VO GRF property from np. If a future platform is missing the legacy phys property or uses a different PHY compatible string, it will hit these ERR_PTR(-ENODEV) returns and probe will fail instead of falling back to the documented scheme. > + > + regmap = syscon_regmap_lookup_by_phandle(args.np, "rockchip,vo-grf"); > + > +out_put_node: > + of_node_put(args.np); > + return regmap; > +} [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v11-0-0d508505f...@collabora.com?part=13
