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

[Damon Ding]

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@xxxxxxxxxxxxxx>
> > > > ---
> > >
> > > [ ... 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