Re: [PATCH phy 13/14] dt-bindings: phy: lynx-28g: add compatible strings per SerDes and instantiation

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3751 bytes --]

On Fri, Sep 05, 2025 at 01:49:21PM +0300, Vladimir Oltean wrote:
> On Thu, Sep 04, 2025 at 08:22:16PM +0100, Conor Dooley wrote:
> > On Thu, Sep 04, 2025 at 06:44:01PM +0300, Vladimir Oltean wrote:
> > > Going by the generic "fsl,lynx-28g" compatible string and expecting all
> > > SerDes instantiations on all SoCs to use it was a mistake.
> > > 
> > > They all share the same register map, sure, but the number of protocol
> > > converters and lanes which are instantiated differs in a way that isn't
> > > detectable by software. So distinguish them by compatible strings.
> > > At the same time, keep "fsl,lynx-28g" as backup.
> > 
> > Why keep the backup? Doesn't sound like you can use it for anything,
> > unless there's some minimum set of capabilities that all devices
> > support. If that's not the case, should it not just be marked deprecated
> > or removed entirely?
> 
> To be honest, I could use some guidance on the best way to handle this.
> 
> When I had written this patch downstream, lx2160a.dtsi only had serdes_1
> defined, as "fsl,lynx-28g", and this patch made more sense. Keep
> "fsl,lynx-28g" as a synonym for "fsl,lx2160a-serdes1", so that new
> device trees still work with old kernels (as is sometimes needed during
> 'git bisect', etc), for some definition of the word "work" (more often
> than not, unsatisfactory - for example, fw_devlink blocks probing the PHY
> consumer driver if the PHY driver doesn't exist, but the 'phys' property
> exists in the device tree).
> 
> Unbeknownst to me, commit 2f2900176b44 ("arm64: dts: lx2160a: describe
> the SerDes block #2") came and defined the second SerDes also with
> "fsl,lynx-28g".
> 
> The second SerDes is less capable than the first one, so the same
> developer then started battling with the fact that the driver doesn't
> know that serdes_2 doesn't support some protocols, and wrote some
> patches like 9bef84d30f1f ("phy: lynx-28g: check return value when
> calling lynx_28g_pll_get"), which in all likelihood could have been
> avoided using a specific compatible string. The lynx_info ::
> lane_supports_mode() method from patch 14/14 is supposed to say what is
> supported per SerDes and what not.

> In terms of implementation, what does "deprecating" the "fsl,lynx-28g"
> compatible string mean, compared to removing it entirely? Would there be
> any remaining driver support for it?

Really it does nothing much. The difference is that removing it entirely
from the binding will cause existing dts users to create warnings
whereas marking it deprecated is more of an attempt to stop
proliferation since it doesn't generate any warnings at the moment but
people using the binding will see that it's not ideal. I personally use
deprecated when using the old binding is only ill-advised because
there's missing features etc and I opt for removal when the old binding
is wrong and actively harmful. In both cases, I'd keep the old
compatible in the driver for compatibility reasons.

> Should I compute the common set of
> capabilities between SerDes #1 and #2, and only support that? What
> impact would this have upon old device trees? Is it acceptable to just
> remove support for them?

Up to you really, if there is a common set between the two, that's
probably the ideal thing to do for the generic compatible. If there
isn't, and shit just ain't working properly at all for either then yeah,
it might be for the better to remove support for it entirely from the
driver too. Just make sure that you're clear about the fact that it just
cannot work at all, and that's why you're axing it. Breaking
compatibility is allowed, when there's justification for doing so, it's
not a complete no-no.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]