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

["Rafał Miłecki" <zajec5@gmail.com>]

On 9.02.2022 21:02, Rob Herring wrote:
> 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.

I think I got it now, thanks for explaining!