Re: [PATCH V2] dt-bindings: watchdog: brcm,bcm7038: add more compatible strings

[Rob Herring <robh@kernel.org>]

On Wed, Feb 09, 2022 at 08:26:00PM +0100, Rafał Miłecki wrote:
> On 9.02.2022 20:09, Rob Herring wrote:
> > On Wed, Jan 26, 2022 at 11:20:34PM +0100, Rafał Miłecki wrote:
> > > From: Rafał Miłecki <rafal@milecki.pl>
> > > 
> > > This hardware block is used on almost all BCM63xx family chipsets and
> > > BCM4908 which reuses a lot of BCM63xx parts. Add relevant compatible
> > > strings and also include a generic one.
> > > 
> > > The only SoC with a different block I found is BCM6838 (thus not included
> > > in this change).
> > > 
> > > It may be worth noting that BCM6338, BCM6345, BCM6348 and BCM63268 don't
> > > include "SoftRst" register but that can be handled by drivers based on
> > > precise compatible string.
> > > 
> > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> > > ---
> > > V2: Sort enum entries & update brcm,twd.yaml
> > > ---
> > >   .../devicetree/bindings/mfd/brcm,twd.yaml     |  2 +-
> > >   .../bindings/watchdog/brcm,bcm7038-wdt.yaml   | 21 +++++++++++++++----
> > >   2 files changed, 18 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml
> > > index 634526f790b8..3f5db1990aba 100644
> > > --- a/Documentation/devicetree/bindings/mfd/brcm,twd.yaml
> > > +++ b/Documentation/devicetree/bindings/mfd/brcm,twd.yaml
> > > @@ -55,7 +55,7 @@ examples:
> > >           #size-cells = <1>;
> > >           watchdog@28 {
> > > -            compatible = "brcm,bcm7038-wdt";
> > > +            compatible = "brcm,bcm4908-wdt", "brcm,bcm63xx-wdt";
> > >               reg = <0x28 0x8>;
> > >           };
> > >       };
> > > diff --git a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
> > > index a926809352b8..4d848442913c 100644
> > > --- a/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
> > > +++ b/Documentation/devicetree/bindings/watchdog/brcm,bcm7038-wdt.yaml
> > > @@ -16,9 +16,22 @@ maintainers:
> > >   properties:
> > >     compatible:
> > > -    enum:
> > > -      - brcm,bcm6345-wdt
> > > -      - brcm,bcm7038-wdt
> > > +    items:
> > > +      - enum:
> > > +          - brcm,bcm4908-wdt
> > > +          - brcm,bcm6338-wdt
> > > +          - brcm,bcm6345-wdt
> > > +          - brcm,bcm6348-wdt
> > > +          - brcm,bcm6848-wdt
> > > +          - brcm,bcm6858-wdt
> > > +          - brcm,bcm7038-wdt
> > > +          - brcm,bcm60333-wdt
> > > +          - brcm,bcm63138-wdt
> > > +          - brcm,bcm63148-wdt
> > > +          - brcm,bcm63268-wdt
> > > +          - brcm,bcm63381-wdt
> > > +          - brcm,bcm68360-wdt
> > > +      - const: brcm,bcm63xx-wdt
> > 
> > Is it really worthwhile to update all these DTs?:
> > 
> > arch/mips/boot/dts/brcm/bcm63268.dtsi:                  compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm6328.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm6358.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm6362.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm6368.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm7125.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm7346.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm7358.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm7360.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm7362.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm7420.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm7425.dtsi:                   compatible = "brcm,bcm7038-wdt";
> > arch/mips/boot/dts/brcm/bcm7435.dtsi:                   compatible = "brcm,bcm7038-wdt";
> 
> I don't have problem handling that.

Even if the dts files above are updated, the driver still has to support 
the above case.

It's pointless to change this. We've already got 1000s of warnings to 
fix and 2000 bindings still to convert if you need something to do.

> > I don't think so.
> So what's the policy for such bindings then? How to select SoCs that
> should have their own bindings? How can I tell which should /borrow/ a
> binding instead?

The way it is supposed to work is the first implementation gets 
'soc1-block'. When the 2nd implementation comes about, it gets 
'soc2-block'. If soc2 implementation is 'the same' or a superset, then 
it should have a fallback to 'soc1-block'. Another way to test that is, 
"I want my existing s/w to work as-is with this new h/w". The process 
repeats on the next SoC, but you could be backwards compatible with 
soc1, soc2, both or none. Is that what you are asking?

If you are doing all this after the fact at once, then it can be a 
bit different in how you do compatibles.

In this case, I would make "brcm,bcm7038-wdt" the fallback to the rest 
(granted, I know nothing about these chips or the relationship between 
them), but you have to keep supporting "brcm,bcm7038-wdt". 6345 is up 
to you I guess as there aren't any upstream dts files using it. Florian 
might care.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel