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

["Heiko Stübner" <heiko@sntech.de>]

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.
> 
> > 
> > 
> > 
> > 
> 
> Best regards,
> Damon
> 
>