Re: [PATCH v3 1/2] phy: rockchip: inno-hdmi: Add configure() and validate() ops

[Vinod Koul <vkoul@kernel.org>]

On 15-05-26, 23:04, Jonas Karlman wrote:
> Hi,
> 
> On 5/15/2026 9:55 PM, Jonas Karlman wrote:
> > The commit 10ed34d6eaaf ("phy: Add HDMI configuration options")
> > introduced a way for HDMI PHYs to be configured through the generic
> > phy_configure() function.
> > 
> > This driver derives the TMDS character rate from the pixel clock and the
> > PHY bus width setting. However, no in-tree consumer of this PHY has ever
> > called phy_set_bus_width() to change the TMDS character rate as only
> > 8-bit RGB output is supported by the HDMI display driver.
> > 
> > Add configure() and validate() ops to allow consumers to configure the
> > TMDS character rate using phy_configure(). Fallback to the deprecated
> > way of using the PHY bus width to configure the TMDS character rate.
> > 
> > A typical call chain during DRM modeset on a RK3328 device:
> > 
> >   dw_hdmi_rockchip_encoder_atomic_check():
> >   - inno_hdmi_phy_validate(): pixclock 148500000 tmdsclock 594000000
> > 
> >   dw_hdmi_rockchip_encoder_atomic_mode_set():
> >   - inno_hdmi_phy_configure(): pixclock 148500000
> >     - inno_hdmi_phy_validate(): pixclock 148500000 tmdsclock 594000000
> > 
> >   vop_crtc_atomic_enable():
> >   - inno_hdmi_phy_rk3328_clk_set_rate(): rate 594000000 tmdsclk 594000000
> >   - inno_hdmi_phy_rk3328_clk_set_rate(): pixclock 594000000 tmdsclock 594000000
> >   - inno_hdmi_phy_rk3328_clk_recalc_rate(): pixclock 594000000 vco 594000000
> > 
> >   dw_hdmi_rockchip_encoder_enable():
> >   - inno_hdmi_phy_power_on(): Inno HDMI PHY Power On
> >   - inno_hdmi_phy_rk3328_clk_set_rate(): rate 594000000 tmdsclk 594000000
> > 
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > ---
> > Changes in v3:
> > - Change validate() ops to only validate tmdsclock
> > - Add comments about expected consumer usage
> > - Update commit message with a typical call chain
> > Changes in v2:
> > - Add validate() ops to validate that the TMDS rate is supported
> > - Split out parts that remove the old workaround into a separate patch
> > 
> > Patch "drm/rockchip: dw_hdmi: Configure HDMI PHY in atomic_mode_set()"
> > at [1] adds phy_validate() and phy_configure() calls for this HDMI PHY.
> > 
> > [1] https://lore.kernel.org/dri-devel/20260510183114.1248840-10-jonas@kwiboo.se/
> > ---
> >  drivers/phy/rockchip/phy-rockchip-inno-hdmi.c | 60 ++++++++++++++++++-
> >  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> [snip]
> 
> > +static int inno_hdmi_phy_validate(struct phy *phy, enum phy_mode mode,
> > +				  int submode, union phy_configure_opts *opts)
> > +{
> > +	const struct pre_pll_config *cfg = pre_pll_cfg_table;
> > +	unsigned long tmdsclock;
> > +
> > +	if (!(mode == PHY_MODE_HDMI && submode == PHY_HDMI_MODE_TMDS))
> > +		return -EINVAL;
> > +
> > +	if (!opts->hdmi.tmds_char_rate || opts->hdmi.tmds_char_rate > 594000000)
> > +		return -EINVAL;
> 
> Sashiko reasoning log pointed out that a consumer of phy_validate() or
> phy_configure() can make a call with opts=NULL, so I may likely send a
> v4 of this series to fix such possible NULL pointer dereference here.

Yes please

> 
> Or is that something that possible should be checked before phy core
> calls the .validate()/.configure() ops?

The opts is an optional argument depending upon the mode to be valid. So
we cant do that in the core...

> 
> Multiple other phy .configure() ops seem to dereference opts members
> without any type of opts NULL check. (next-20260508)

We should fix these


PS: Any reason why you are sending encypted emails?

-- 
~Vinod