Date: Tue, 12 Mar 2024 15:06:16 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: "Lad, Prabhakar" <prabhakar.csengg@...il.com>, Chris Brandt <chris.brandt@...esas.com>, 
	Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Magnus Damm <magnus.damm@...il.com>, Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	linux-renesas-soc@...r.kernel.org, linux-i2c@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 2/5] dt-bindings: i2c: renesas,riic: Document R9A09G057 support

Hi Krzysztof,

On Tue, Mar 12, 2024 at 12:04 PM Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
> On 11/03/2024 10:00, Geert Uytterhoeven wrote:
> >>>>> -          - renesas,riic-r9a07g054  # RZ/V2L
> >>>>> -      - const: renesas,riic-rz      # generic RIIC compatible
> >>>>> +    oneOf:
> >>>>> +      - items:
> >>>>> +          - enum:
> >>>>> +              - renesas,riic-r7s72100   # RZ/A1H
> >>>>> +              - renesas,riic-r7s9210    # RZ/A2M
> >>>>> +              - renesas,riic-r9a07g043  # RZ/G2UL and RZ/Five
> >>>>> +              - renesas,riic-r9a07g044  # RZ/G2{L,LC}
> >>>>> +              - renesas,riic-r9a07g054  # RZ/V2L
> >>>>> +          - const: renesas,riic-rz      # generic RIIC compatible
> >>>>> +
> >>>>> +      - items:
> >>>>> +          - enum:
> >>>>> +              - renesas,riic-r9a09g057  # RZ/V2H(P)
> >>>>
> >>>> No, that does not look right. If you added generic compatible for all
> >>>> RIIC then how can you add a new RIIC compatible which does not follow
> >>>> generic one?
> >>>>
> >>> The generic compatible above which was added previously was for the
> >>> RZ/(A) SoCs and not for all the RIICs. The RZ/G2L family was also
> >>
> >> No, it said: "generic RIIC compatible". It did not say "RIIC RZ/A". It
> >> said RIIC RZ
> >
> > At the time the original bindings were written, only RZ/A1, RZ/T1,
> > and RZ/N1 existed, and all RIIC modules present in these SoCs were
> > identical.  Later, we got RZ/A2, which also included a compatible
> > RIIC block.
> >
> > Somewhere along the timeline, the marketing department became creative,
> > and we got RZ/G1 (RZ/G1[HMNEC]) and RZ/G2 (RZ/G2[HMNE]), which were
> > unrelated to earlier RZ series :-(  When marketing started smoking
> > something different, we got RZ/G2L, which is unrelated to RZ/G2,
> > but reuses some parts from RZ/A.  Recently, we got RZ/G3S, which is
> > similar to RZ/G2L...
>
> That's fine, but then the comment "generic RIIC compatible" is confusing
> for anyone not knowing this. Commit msg could also mention why the
> generic compatible covers actually entirely different hardware. The
> commit msg so far focused on the differences between these hardwares,
> thus my questions - why do you create generic compatibles which are not
> generic?

I agree the comment should be updated when adding a new variant which
is not compatible with the old generic variant (i.e. in this patch).

> >> So don't use generic compatibles as fallbacks. That's the point.
> >
> > It's indeed difficult to predict the future. So SoC-specific compatible
> > values are safer.
> > At the same time, we want to avoid having to add compatible values for
> > each and every SoC to each driver, so we try to group SoCs per family.
> > For R-Car that worked out reasonably well, however, for RZ...
>
> I did not propose that. Nothing changes in your driver with my proposal.
> Use SoC-compatibles only: for fallbacks and for specific(frontbacks?) parts.
>
> To give you some sort of guidance for any future submission:
> 1. Use SoC-like fallback compatible, prepended with SoC-specific compatible.
> 2. If you insist on generic fallback compatible, its usage should be
> limited to the cases where you can guarantee for 99.9% that future
> devices will be compatible with this. I doubt anyone can guarantee that,
> thus we keep repeating on mailing lists the same: go to point (1).

Personally, I am not such a big fan of method 1, for several reasons:

  - Support for new SoCs is not always added in chronological SoC
    release date order.  So you could end up with:

        compatible = "vendor,socB-foo", "vendor,socA-foo";

     with socA being (much) newer than socB.

  - Worse, adding support for different modules in different SoCs
    can be unordered, too, leading to

        compatible = "vendor,socB-foo", "vendor,socA-foo";

    but

        compatible = "vendor,socA-bar", "vendor,socB-bar";

    Which is inconsistent.  Fortunately we now have "make dtbs_check"
    to catch mistakes there.

  - When a third SoC arrives, which one do you pick?

        compatible = "vendor,socC-foo", "vendor,socA-foo";

    or

        compatible = "vendor,socC-foo", "vendor,socB-foo";

    Obviously you pick the former (unless you detected the issues
    below first ;-)

  - When socA-foo turns out to be slightly different from socB-foo,
    socC-foo, ... you have to add of_device_id entries for all
    socX-foo to the driver.  With a family-specific fallback, you'd
    be limited to one entry for the family-specific callback and
    a second entry for the misbehaving socA.

So far my 5€c....

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists