Re: [PATCH net] dt-bindings: net: tja11xx: fix the broken binding

[Andrew Lunn]

On Tue, Sep 03, 2024 at 02:17:04AM +0000, Wei Fang wrote:
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - ethernet-phy-id0180.dc40
> > > +      - ethernet-phy-id0180.dd00
> > > +      - ethernet-phy-id0180.dc80
> > > +      - ethernet-phy-id001b.b010
> > > +      - ethernet-phy-id001b.b031
> > 
> > This shows the issues with using a compatible. The driver has:
> > 
> > #define PHY_ID_TJA_1120                 0x001BB031
> > 
> >                 PHY_ID_MATCH_MODEL(PHY_ID_TJA_1120),
> > 
> > which means the lowest nibble is ignored. The driver will quite happy also probe
> > for hardware using 001b.b030, 001b.b032, 001b.b033, ... 001b.b03f
> > 
> > Given you are inside NXP, do any of these exist? Was 001b.b030 too broken it
> > never left the QA lab? Are there any hardware issues which might result in a
> > new silicon stepping?
> 
> Yes, some of the revisions do exist, but the driver should be compatible with
> these different revisions.
> 
> For 001b.b030, I don't think it is broken, based on the latest data sheet of
> TJA1120 (Rev 0.6 26 January 2023), the PHY ID is 001b.b030. I don't know
> why it is defined as 001b.b031 in the driver, it may be a typo.

More likely, the board Radu Pirea has does have a device with this ID.

> > 
> > Does ethernet-phy-id0180.dc41 exist? etc.
> I think other TJA PHYs should also have different revisions.
> 
> Because the driver ignores the lowest nibble of the PHY ID, I think it is fine to
> define the lowest nibble of the PHY ID in these compatible strings as 0, and
> there is no need to list all revisions. And I don't know which revisions exist,
> because I haven't found or have no permission to download some PHY data
> sheets. I think what I can do is to modify "ethernet-phy-id001b.b031" to
> "ethernet-phy-id001b.b030".

You have to be careful here. Stating a compatible forces the PHY
ID. So if the compatible is "ethernet-phy-id001b.b031", but the board
actually has a "ethernet-phy-id001b.b030". phydev->phy_id is going to
be set to 0x001bb031. Any behaviour in the driver which look at that
revision nibble is then going to be wrong.

Maybe, now, today, that does not matter, because the driver never
looks at the revision. But it does mean developers might put the wrong
compatible in DT. And then when you do need to add code looking at the
revision, it does not always work, because there are some boards with
the wrong compatible in DT.

Listing all possible compatibles suggests to developers they need to
be careful and use the correct value. Or add a comment in the DT
bindings that not using a compatible is probably safer.

	Andrew