Re: [PATCH 1/2] dt-bindings: phy: ti,tcan104x-can: Document Microchip ATA6561

[Conor Dooley <conor@kernel.org>]

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

On Mon, Jul 29, 2024 at 10:51:50AM +0200, Geert Uytterhoeven wrote:
> Hi Ilya,
> 
> On Sun, Jul 28, 2024 at 10:52 AM Ilya Orazov <ilordash02@gmail.com> wrote:
> > On Tue, 23 Jul 2024 at 23:14, Conor Dooley <conor@kernel.org> wrote:
> > > On Tue, Jul 23, 2024 at 10:55:17PM +0300, Ilya Orazov wrote:
> > > > On Tue, 23 Jul 2024 at 21:50, Conor Dooley <conor@kernel.org> wrote:
> > > > > On Tue, Jul 23, 2024 at 08:20:04PM +0300, IlorDash wrote:
> > > > > > On Fri, 19 Jul 2024 at 18:07, Conor Dooley <conor@kernel.org> wrote:
> > > > > > >
> > > > > > > On Fri, Jul 19, 2024 at 12:03:21AM +0300, Ilya Orazov wrote:
> > > > > > > > Microchip ATA6561 is High-Speed CAN Transceiver with Standby Mode.
> > > > > > > > It is pin-compatible with TI TCAN1042.
> > > > > > > >
> > > > > > > > Signed-off-by: Ilya Orazov <ilordash02@gmail.com>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > > > > > > index 79dad3e89aa6..03de361849d2 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > > > > > > @@ -18,6 +18,7 @@ properties:
> > > > > > > >        - nxp,tjr1443
> > > > > > > >        - ti,tcan1042
> > > > > > > >        - ti,tcan1043
> > > > > > > > +      - microchip,ata6561
> > > > > > >
> > > > > > > Given that your driver patch has
> > > > > > > | diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> > > > > > > | index ee4ce4249698..dbcd99213ba1 100644
> > > > > > > | --- a/drivers/phy/phy-can-transceiver.c
> > > > > > > | +++ b/drivers/phy/phy-can-transceiver.c
> > > > > > > | @@ -89,6 +89,10 @@ static const struct of_device_id can_transceiver_phy_ids[] = {
> > > > > > > |                 .compatible = "nxp,tjr1443",
> > > > > > > |                 .data = &tcan1043_drvdata
> > > > > > > |         },
> > > > > > > | +       {
> > > > > > > | +               .compatible = "microchip,ata6561",
> > > > > > > | +               .data = &tcan1042_drvdata
> > > > > > > | +       },
> > > > > > > |         { }
> > > > > > > |  };
> > > > > > >
> > > > > > > the driver patch is actually not needed at all, and you just need to
> > > > > > > allow ti,tcan1042 as fallback compatible in the binding, so something
> > > > > > > like:
> > > > > > >
> > > > > > >   compatible:
> > > > > > >     oneOf:
> > > > > > >       - enum:
> > > > > > >           - nxp,tjr1443
> > > > > > >           - ti,tcan1042
> > > > > > >           - ti,tcan1043
> > > > > > >       - items:
> > > > > > >           - const: microchip,ata6561
> > > > > > >           - const: ti,tcan1042
> > > > > > >
> > > > > > >    '#phy-cells':
> > > > > > >      const: 0
> > > > > >
> > > > > > I tested the build with fallback compatible:
> > > > > >
> > > > > > compatible:
> > > > > >   oneOf:
> > > > > >     - items:
> > > > > >       - enum:
> > > > > >         - microchip,ata6561
> > > > > >       - const: ti,tcan1042
> > > > > >     - items:
> > > > > >       - enum:
> > > > > >         - nxp,tjr1443
> > > > > >       - const: ti,tcan1043
> > > > > >
> > > > > > and modified compatible property in DTS:
> > > > > >
> > > > > > compatible = "microchip,ata6561", "ti,tcan1042";
> > > > > >
> > > > > > Build succeeded, phy-can-transceiver driver was used. So I would like
> > > > > > to add a fallback compatible for both "microchip,ata6561" and
> > > > > > "nxp,tjr1443" in this binding and modify other DTS files with
> > > > > > compatible = "nxp,tjr1443". What do you think?
> > > > >
> > > > > This is wrong on two counts. Firstly, were what you have correct, you
> > > > > should
> > > > > squash the two:
> > > > >      - items:
> > > > >          - enum:
> > > > >            - nxp,tjr1443
> > > > >            - microchip,ata6561
> > > > >          - const: ti,tcan1042
> > > > >
> > > > > However, that does not allow the TI compatibles in isolation, so you
> > > > > still need to allow that for the actual TI devices, so you need:
> > > > >
> > > > >    oneOf:
> > > > >      - items:
> > > > >          - enum:
> > > > >            - microchip,ata6561
> > > > >            - nxp,tjr1443
> > > > >            - ti,tcan1043
> > > > >          - const: ti,tcan1042
> > > > >      - const: ti,tcan1042
> > > > >
> > > > > There's probably some devicetrees that would need to be fixed up. I'm
> > > > > just not convinced that this is worth retrofitting however.
> > > >
> > > > But nxp,tjr1443 is pin compatible with ti,tcan1043, so it should
> > > > fallback only to ti,tcan1043 and not ti,tcan1042. That's why I decided
> > > > to split them into different enums.
> > >
> > > Ah, sorry I missed that. I misread the match data. Then you need:
> > >   compatible:
> > >     oneOf:
> > >       - items:
> > >         - enum:
> > >           - microchip,ata6561
> > >         - const: ti,tcan1042
> > >       - items:
> > >         - enum:
> > >           - nxp,tjr1443
> > >         - const: ti,tcan1043
> > >       - enum:
> > >           const: ti,tcan1042
> > >           const: ti,tcan1043
> > >
> > > because the TI devices exist and we still need to be able to
> > > differentiate the TI and NXP devices. If you have
> > >   compatible = "nxp,tjr1443", "ti,tcan1042";
> > > that means the device is an nxp,tjr1443. If you have
> > >   compatible = "ti,tcan1042";
> > > then that's a tcan1042.
> > >
> > > > I made my patch according to a similar one that adds support for
> > > > nxp,tjr1443. You can find it's conversation on
> > > > https://lore.kernel.org/all/6ee5e2ce00019bd3f77d6a702b38bab1a45f3bb0.1674037830.git.geert+renesas@glider.be/t/#u.
> > >
> > > > I thought we want to hold all PHY chip names in one compatible enum
> > > > and each in its own of_device_id struct in driver and extend them
> > > > where appropriate.
> > >
> > > Nah, fallbacks are preferred when the programming model is either
> > > identical or a "compatible superset" of an existing device. New
> > > of_device_id structs should only be used where we need to account for
> > > differences in the programming model.
> >
> > However, I am curious as to why the NXP CAN PHY transceiver was not
> > included as fallback compatible. Geert, could you please share your
> > thoughts on this matter?
> 
> The TJR1443 looked sufficiently similar to the TCAN1043 to use the
> same driver configuration (which is limited to having standby and/or
> enable signals or not).  However, I'm not sure it behaves exactly
> the same, e.g. in case of reporting an error condition (which is not
> yet supported by the driver). The part numbers are also different,
> so this is not a simple case of SN74HCxx vs. CD74HCxx.
> 
> Summary: I don't know if they are identical, or if TJR1443 is a
> compatible superset of TCAN1043, or vice versa. Hence I went for the
> safest way....

If we don't know for sure what the craic is with compatibility, then we
should leave the existing tjr1443 compatible as-is I think.

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