Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Broken forward compatibility in dp_dp_rockchip_get_vo_grf. - [Low] Typo in the newly introduced function name `dp_dp_rockchip_get_vo_grf`. --
--- Patch [10]: [PATCH v6 10/16] drm/rockchip: dw_dp: Implement out-of-band HPD handling --- commit 75a9965306b4d818663f8d48b44dc43031d1cb8f 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 HPD information from the USB-C state machine via a syscon regmap (vo-grf). It prevents SError problems on RK3576/RK3588 platforms when runtime PM is enabled. > diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c [ ... ] > @@ -71,6 +116,35 @@ static const struct drm_encoder_helper_funcs > dw_dp_encoder_helper_funcs = { > .atomic_check = dw_dp_encoder_atomic_check, > }; > > +static struct regmap *dp_dp_rockchip_get_vo_grf(struct rockchip_dw_dp *dp) ^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: Low] This isn't a bug, but there is a typo in the function prefix. Should this be named dw_dp_rockchip_get_vo_grf to match the rest of the driver's naming conventions? > +{ > + 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; > + } > + > + regmap = syscon_regmap_lookup_by_phandle(args.np, "rockchip,vo-grf"); [Severity: Medium] Will this break forward compatibility for future platforms as described in the comment above? If a future platform omits the "phys" property or uses a different PHY compatible, this function unconditionally returns ERR_PTR(-ENODEV) and aborts. It never attempts to look up the "rockchip,vo-grf" phandle directly from the DisplayPort controller DT node (np) as a fallback. Does this need a fallback path to check syscon_regmap_lookup_by_phandle(np, ...) if the PHY checks fail? > + > +out_put_node: > + of_node_put(args.np); > + return regmap; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260724-synopsys-dw-dp-improvements-v6-0-041d99a19...@collabora.com?part=10
