Re: [PATCH v3 phy-next 10/24] drm/rockchip: dw_hdmi: avoid direct dereference of phy->dev.of_node

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Mar 10, 2026 at 09:24:43AM +0100, Heiko Stuebner wrote:
> Am Montag, 9. März 2026, 20:08:28 Mitteleuropäische Normalzeit schrieb Vladimir Oltean:
> > The dw_hdmi-rockchip driver validates pixel clock rates against the
> > HDMI PHY's internal clock provider on certain SoCs like RK3328.
> > This is currently achieved by dereferencing hdmi->phy->dev.of_node
> > to obtain the provider node, which violates the Generic PHY API's
> > encapsulation (the goal is for struct phy to be an opaque pointer).
> > 
> > Refactor dw_hdmi_rockchip_bind() to perform a manual phandle lookup
> > on the "hdmi" PHY index within the controller's DT node. This provides
> > a parallel path to the clock provider's OF node without relying on the
> > internal structure of the struct phy handle.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> > Cc: Sandy Huang <hjc@rock-chips.com>
> > Cc: "Heiko Stübner" <heiko@sntech.de>
> > Cc: Andy Yan <andy.yan@rock-chips.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@gmail.com>
> > Cc: Simona Vetter <simona@ffwll.ch>
> > 
> > v1->v3: none
> > ---
> 
> [...]
> 
> > @@ -588,13 +589,17 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
> >  		return dev_err_probe(hdmi->dev, ret, "failed to get phy\n");
> >  	}
> >  
> > -	if (hdmi->phy) {
> 
> nit: a comment would be nice here. I.e. hdmi->phy being an opaque pointer
> so checking hdmi->phy != NULL is not possible.
> 
> With that being a "goal", I assume that information is not widely spread
> so this would prevent the next developer trying to change it back to
> "if (hdmi->phy)" while that handling change trickles down.

Testing the NULL quality of "struct phy *phy" is still possible and legal.
It means that you called an "optional" variant of phy_get(), and there
was no PHY.

Just that here, the ultimate intention isn't that. It is to abuse the
struct phy to get to something completely unrelated to the PHY API.
I wouldn't have had any problem if there was "just" a hdmi->phy NULL
pointer check.

> apart from that:
> 
> Reviewed-by: Heiko Stueber <heiko@sntech.de>

Thanks for the review.