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

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Caching `tmds_char_rate` as an absolute frequency in `phy_configure()` breaks subsequent `clk_set_rate()` calls during resolution changes.
- [Medium] `inno_hdmi_phy_validate()` artificially restricts configurations to 8-bit color modes by enforcing that the TMDS character rate equals the pixel clock.
--

commit 289f99beadee9ab8172e2d404dc44e1a8de5bc53
Author: Jonas Karlman <jonas at kwiboo.se>

phy: rockchip: inno-hdmi: Add configure() and validate() ops

This commit adds configure() and validate() operations to the Inno HDMI
PHY driver. It allows consumers to configure the TMDS character rate via
phy_configure(), while maintaining fallback support for the legacy bus
width method.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> index 1483907413faa..9cfe956fefe71 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
> @@ -554,7 +555,12 @@ static inline void inno_update_bits(struct inno_hdmi_phy *inno, u8 reg,
>  static unsigned long inno_hdmi_phy_get_tmdsclk(struct inno_hdmi_phy *inno,
>  					       unsigned long rate)
>  {
> -	int bus_width = phy_get_bus_width(inno->phy);
> +	int bus_width;
> +
> +	if (inno->hdmi_cfg.tmds_char_rate)
> +		return inno->hdmi_cfg.tmds_char_rate;

Can caching the absolute tmds_char_rate here break subsequent clk_set_rate()
calls during resolution changes?

During a typical DRM display mode change, the CRTC updates the pixel clock
via clk_set_rate() before the HDMI bridge calls phy_configure() to apply
the new PHY options.

When clk_set_rate() is called with the new pixel clock, this function will
ignore the rate parameter and return the old cached tmds_char_rate.

Will this mismatched combination of new pixclock and old TMDS clock fail to
find a valid entry in pre_pll_cfg_table, causing clk_set_rate() to return
-EINVAL and abort the resolution change?

[ ... ]

> +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;
> +
> +	tmdsclock = opts->hdmi.tmds_char_rate;
> +	for (; cfg->pixclock != 0; cfg++)
> +		if (cfg->pixclock == tmdsclock && cfg->tmdsclock == tmdsclock)
> +			return 0;

Does this restrict configurations to 8-bit color modes by enforcing that the
TMDS character rate equals the pixel clock?

For deep color modes (e.g., 10-bit), the TMDS character rate is strictly
greater than the pixel clock.

Because the validation insists that cfg->pixclock must also equal the
requested tmds_char_rate, will valid deep color configurations fail this
check and return -EINVAL?

> +
> +	return -EINVAL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510095731.1222705-1-jonas@kwiboo.se?part=1