Re: [PATCH v1 04/10] phy: phy-rockchip-samsung-hdptx: Add support for eDP mode

[Damon Ding <damon.ding@rock-chips.com>]

Hi Heiko,

On 2024/11/27 19:04, Heiko Stübner wrote:
> Hi Damon,
> 
> Am Mittwoch, 27. November 2024, 12:00:10 CET schrieb Damon Ding:
>> Hi Heiko:
>>
>> On 2024/11/27 17:29, Heiko Stübner wrote:
>>> Hi Damon,
>>>
>>> Am Mittwoch, 27. November 2024, 08:51:51 CET schrieb Damon Ding:
>>>> Add basic support for RBR/HBR/HBR2 link rates, and the voltage swing and
>>>> pre-emphasis configurations of each link rate have been verified according
>>>> to the eDP 1.3 requirements.
>>>>
>>>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>>>> ---
>>>
>>> [ ... huge block of DP phy support ...]
>>>
>>> yes that block was huge, but I also don't see a way to split that up in a
>>> useful way, so it should be fine.
>>>
>>
>> As for the huge block of DP phy support, I will try to use the existing
>> rk_hdptx_multi_reg_write() to set regs in next version, maybe the way
>> can make the codes more concise.
> 
> I actually did like the the dp-side of the phy code.
> 
> That you need to add all the DP stuff can't be helped and I actually find
> real functions nicer than having anonymous register writes.
> 
> I.e. the hdmi-side with its register lists does write "magic" values to
> registers.
> 
> So personally I'd just leave the dp-functions as is please, until someone
> does complain (I was not trying to complain, just mentioned why I cut
> it from the reply :-) )
> 
> 
> Thanks
> Heiko
> 
> 
>>>> +static int rk_hdptx_phy_set_mode(struct phy *phy, enum phy_mode mode,
>>>> +				 int submode)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>
>>> I think it might make sense to go the same way as the DCPHY and also
>>> naneng combophy, to use #phy-cells = 1 to select the phy-mode via DT .
>>>
>>> See [0] for Sebastians initial suggestion regarding the DC-PHY.
>>> The naneng combophy already uses that scheme of mode-selection too.
>>>
>>> There is of course the issue of backwards-compatibility, but that can be
>>> worked around in the binding with something like:
>>>
>>>    '#phy-cells':
>>>       enum: [0, 1]
>>>       description: |
>>>         If #phy-cells is 0, PHY mode is set to PHY_TYPE_HDMI
>>>         If #phy-cells is 1 mode is set in the PHY cells. Supported modes are:
>>>           - PHY_TYPE_HDMI
>>>           - PHY_TYPE_DP
>>>         See include/dt-bindings/phy/phy.h for constants.
>>>
>>> PHY_TYPE_HDMI needs to be added to include/dt-bindings/phy/phy.h
>>> but PHY_TYPE_DP is already there.
>>>
>>> That way we would standardize on one form of accessing phy-types
>>> on rk3588 :-) .
>>>
>>> Also see the Mediatek CSI rx phy doing this too already [1]
>>>
>>>
>>> Heiko
>>>
>>> [0] https://lore.kernel.org/linux-rockchip/udad4qf3o7kt45nuz6gxsvsmprh4rnyfxfogopmih6ucznizih@7oj2jrnlfonz/
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/phy/mediatek,mt8365-csi-rx.yaml
>>>
>>
>> It is really a nice way to separate HDMI and DP modes.

I apologize for reopening the discussion about the phy-types setting.

With the .set_mode() of struct phy_ops, the HDMI and eDP dynamic 
switching can be achieved, which just depends on the right setting of
enum phy_mode in include/linux/phy/phy.h. So the previous way of 
configuring phy mode may be also good.

And other phys may want to support dynamic switching too, like the 
Rockchip USBDP combo phy.

>>
>>>
>>>
>>>
>>>
>>
>> Best regards,
>> Damon
>>
>>
> 
> 
> 
> 
> 
> 
Best regards,
Damon

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip