Message-ID: <0df9adfe-c869-4502-93e1-fb3516aedc0c@linaro.org>
Date: Tue, 12 Mar 2024 16:05:49 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Geert Uytterhoeven <geert@...ux-m68k.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

On 12/03/2024 15:06, Geert Uytterhoeven wrote:
> 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.

Which is not a problem. We already have such examples in Qualcomm and
once you get used to seeing it, no one wonders. Of course it's not like
we target such reversed compatibility, but if it happens - does not
matter. compatible expresses compatibility, not timeframe.

> 
>   - 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";

Yeah, that looks not nice, indeed, but it's a rare case and still does
not matter for actual meaning. compatible does not express timeframe.

> 
>     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";

compatibility defines this.

> 
>     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.

Yes and we have exactly that kind of argument from Bjorn A. for
Qualcomm. I repeat: this means your generic family-fallback changes
meaning, which is not really correct. Bindings (and DTS) are used for
outside projects, so if you need to change the driver for generic
fallback, all outside projects might be affected. You basically changed
ABI without telling it to anyone.

Best regards,
Krzysztof

Powered by blists - more mailing lists