Re: [PATCH v4 07/12] phy: rockchip: samsung-hdptx: Setup TMDS char rate via phy_configure_opts_hdmi

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

On 3/4/25 10:15 AM, Maxime Ripard wrote:
> On Tue, Mar 04, 2025 at 03:44:06AM +0200, Cristian Ciocaltea wrote:
>> The current workaround to setup the TMDS character rate relies on the
>> unconventional usage of phy_set_bus_width().
>>
>> Make use of the recently introduced HDMI PHY configuration API to
>> properly handle the setup.  The workaround will be dropped as soon as
>> the switch has been completed on both ends.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 54 ++++++++++++++++-------
>>  1 file changed, 38 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> index 2bf525514c1991a1299265d12e1e85f66333c604..7e1d1c10758249aa5bbddbdaae0108bba04f30df 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> @@ -394,6 +394,7 @@ struct rk_hdptx_phy {
>>  
>>  	int phy_id;
>>  	struct phy *phy;
>> +	struct phy_configure_opts_hdmi hdmi_cfg;
>>  	struct clk_bulk_data *clks;
>>  	int nr_clks;
>>  	struct reset_control_bulk_data rsts[RST_MAX];
>> @@ -1409,19 +1410,25 @@ static int rk_hdptx_dp_aux_init(struct rk_hdptx_phy *hdptx)
>>  static int rk_hdptx_phy_power_on(struct phy *phy)
>>  {
>>  	struct rk_hdptx_phy *hdptx = phy_get_drvdata(phy);
>> -	int bus_width = phy_get_bus_width(hdptx->phy);
>>  	enum phy_mode mode = phy_get_mode(phy);
>> +	unsigned int rate = 0;
>>  	int ret, lane;
>>  
>> -	/*
>> -	 * FIXME: Temporary workaround to pass pixel_clk_rate
>> -	 * from the HDMI bridge driver until phy_configure_opts_hdmi
>> -	 * becomes available in the PHY API.
>> -	 */
>> -	unsigned int rate = bus_width & 0xfffffff;
>> -
>> -	dev_dbg(hdptx->dev, "%s bus_width=%x rate=%u\n",
>> -		__func__, bus_width, rate);
>> +	if (mode != PHY_MODE_DP) {
>> +		if (!hdptx->hdmi_cfg.tmds_char_rate) {
>> +			/*
>> +			 * FIXME: Temporary workaround to setup TMDS char rate
>> +			 * from the RK DW HDMI QP bridge driver.
>> +			 * Will be removed as soon the switch to the HDMI PHY
>> +			 * configuration API has been completed on both ends.
>> +			 */
>> +			rate = phy_get_bus_width(hdptx->phy) & 0xfffffff;
>> +			hdptx->hdmi_cfg.tmds_char_rate = rate * 100;
>> +		} else {
>> +			rate = hdptx->hdmi_cfg.tmds_char_rate / 100;
>> +		}
>> +		dev_dbg(hdptx->dev, "%s rate=%u\n", __func__, rate);
>> +	}
> 
> Some story here, I can't make sense of a variable in hHz. If it's
> actually needed and not a bug, this should be very explictly documented.

Not a bug - as explained earlier, phy_set_bus_width() on the other end
passes this in hHz.  I agree it should have been properly documented,
but eventually we got this cleaned up in the last patch of the series.

Thanks,
Cristian