Received-SPF: pass (google.com: domain of linux-kernel+bounces-100317-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240308172726.225357-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20240308172726.225357-3-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <7082ed3b-d6d0-4228-b7a6-7c0e0e46b8e2@linaro.org> <CA+V-a8tM29h10DULurMJtBZBnLK_ZF7pH_Y0bhZTvWO0O7-G-Q@mail.gmail.com>
 <2974085a-d9b4-4a66-b60f-c02a06a74647@linaro.org> <CAMuHMdVgp_vFnWr5ruzdyc1vNQKoCdM=pLZmgmkDjmUHvQBJJw@mail.gmail.com>
 <b04f9c39-9797-40b8-a25b-8154ad559cd5@linaro.org>
In-Reply-To: <b04f9c39-9797-40b8-a25b-8154ad559cd5@linaro.org>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Tue, 12 Mar 2024 15:06:16 +0100
Message-ID: <CAMuHMdV6yHcTaZKMJxS7sabzhCGKt4i6bjKJiNDaCoHkeZXUvA@mail.gmail.com>
Subject: Re: [PATCH 2/5] dt-bindings: i2c: renesas,riic: Document R9A09G057 support
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>, Chris Brandt <chris.brandt@renesas.com>, 
	Andi Shyti <andi.shyti@kernel.org>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Magnus Damm <magnus.damm@gmail.com>, Wolfram Sang <wsa+renesas@sang-engineering.com>, 
	linux-renesas-soc@vger.kernel.org, linux-i2c@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Fabrizio Castro <fabrizio.castro.jz@renesas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Krzysztof,

On Tue, Mar 12, 2024 at 12:04=E2=80=AFPM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.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 al=
l
> >>>> RIIC then how can you add a new RIIC compatible which does not follo=
w
> >>>> 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?) par=
ts.
>
> To give you some sort of guidance for any future submission:
> 1. Use SoC-like fallback compatible, prepended with SoC-specific compatib=
le.
> 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 =3D "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 =3D "vendor,socB-foo", "vendor,socA-foo";

    but

        compatible =3D "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 =3D "vendor,socC-foo", "vendor,socA-foo";

    or

        compatible =3D "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=E2=82=ACc....

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
org

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