On Tue, 23 Jul 2024 at 21:50, Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Tue, Jul 23, 2024 at 08:20:04PM +0300, IlorDash wrote:
> > On Fri, 19 Jul 2024 at 18:07, Conor Dooley <conor@xxxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > > 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.
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@xxxxxxxxx/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.
--
Best regards,
Ilya Orazov
