Re: [PATCH v6 10/16] drm/rockchip: dw_dp: Implement out-of-band HPD handling

[sashiko-bot@kernel.org]

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 <sebastian.reichel@collabora.com>

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-041d99a19c4e@collabora.com?part=10