Re: [PATCH RFC v3 09/10] dt-bindings: display: rockchip: dw-dp: fix sound DAI cells

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Unconditionally changing `#sound-dai-cells` from `const: 0` to `const: 1` breaks backward compatibility with existing device tree binaries and causes `make dtbs_check` failures.
- [High] dw_dp_runtime_resume() fails to check the return value of clk_prepare_enable() for apb_clk and aux_clk, leading to unclocked hardware accesses.
- [High] The driver ignores the return value of pm_runtime_get_sync() and its scoped guard wrappers, leading to potential unclocked register accesses.
- [Medium] The `#sound-dai-cells = <1>` design is mathematically insufficient to support the RK3576's 6 DAI interfaces, ensuring future ABI breakage when MST is fully implemented.
--

commit dcfc80f29db8ade71eb0e49550ea25781716e785
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

dt-bindings: display: rockchip: dw-dp: fix sound DAI cells

This commit updates the #sound-dai-cells property to support the two
possible DAI interfaces (I2S and S/PDIF) on RK3588 and RK3576 DesignWare
DisplayPort controllers. It changes the const value to 1 and discusses
options for future Multi Stream Transport (MST) support.

> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
> index 2b0d9e23e9432..1303d0e2145a9 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
> @@ -83,7 +83,8 @@ properties:
>      maxItems: 1
>  
>    "#sound-dai-cells":
> -    const: 0
> +    const: 1
> +    description: 0 for I2S, 1 for SPDIF

[Severity: Critical]
Does unconditionally changing #sound-dai-cells from const: 0 to const: 1
break backward compatibility with existing device tree binaries?

Existing device trees supplying 0 cells will now fail validation during
make dtbs_check, and older DTBs will no longer conform to the schema.

[Severity: Medium]
Is setting #sound-dai-cells = <1> mathematically sufficient to support the
RK3576 hardware?

The commit message mentions the RK3576 features MST with 6 DAIs, so a single
cell constraint only supports 2 states (I2S and S/PDIF). Will this necessitate
another breaking change when MST is implemented?

>  
>  required:
>    - compatible

[ ... ]

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 36ee6e027af52..7f4f36c614844 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1651,6 +1657,8 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct drm_connector_state *conn_state;
>  	int ret;
>  
> +	pm_runtime_get_sync(dp->dev);
> +

[Severity: High]
Does this code discard the return value of pm_runtime_get_sync()?

If the PM core fails to resume the device, the code proceeds with the enable
sequence, which could risk unclocked register accesses later on.

>  	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
>  	if (!connector) {
>  		dev_err(dp->dev, "failed to get connector\n");

[ ... ]

> @@ -1725,6 +1734,8 @@ static enum drm_connector_status dw_dp_bridge_detect(struct drm_bridge *bridge,
>  {
>  	struct dw_dp *dp = bridge_to_dp(bridge);
>  
> +	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
> +
>  	if (!dw_dp_hpd_detect(dp))

[Severity: High]
Does using this unconditional PM scope guard discard the return value?

If the device failed to resume, the code will proceed to access hardware
registers in dw_dp_hpd_detect(), which could trigger a bus fault.

>  		return connector_status_disconnected;
>  
>  	if (!dw_dp_hpd_detect_link(dp, connector))

[ ... ]

> @@ -2099,6 +2160,26 @@ void dw_dp_unbind(struct dw_dp *dp)
>  }
>  EXPORT_SYMBOL_GPL(dw_dp_unbind);
>  
> +int dw_dp_runtime_suspend(struct dw_dp *dp)
> +{
> +	clk_disable_unprepare(dp->aux_clk);
> +	clk_disable_unprepare(dp->apb_clk);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_dp_runtime_suspend);
> +
> +int dw_dp_runtime_resume(struct dw_dp *dp)
> +{
> +	clk_prepare_enable(dp->apb_clk);
> +	clk_prepare_enable(dp->aux_clk);
> +

[Severity: High]
Are we ignoring the return values of clk_prepare_enable() here?

If the clocks fail to enable, unconditionally calling dw_dp_init_hw() will
access hardware registers while unclocked, which could lead to a bus hang.

> +	dw_dp_init_hw(dp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_dp_runtime_resume);

[ ... ]

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index 0de822360c8db..1f53228e56d9f 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c

[ ... ]

> +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];
> +
> +	dp->hpd_sel = force_hpd_from_sw;
> +
> +	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
> +
> +	regmap_write(dp->vo_grf, hpd_reg,
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, dp->hpd_sel));

[Severity: High]
Does this ignore the possibility that the power domain failed to resume?

If the unconditional PM guard fails, writing to the vo_grf syscon will
trigger a bus fault.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-synopsys-dw-dp-improvements-v3-0-dc61e6352508@collabora.com?part=9