On 10/03/2024 00:28, Lad, Prabhakar wrote:
> Hi Krzysztof,
>
> Thank you for the review.
>
> On Sat, Mar 9, 2024 at 12:00 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>
>> On 08/03/2024 18:27, Prabhakar wrote:
>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>>>
>>> Document support for the I2C Bus Interface (RIIC) available in the
>>> Renesas RZ/V2H (R9A09G057) SoC.
>>>
>>> The RIIC interface in the Renesas RZ/V2H differs from RZ/A in a
>>> couple of ways:
>>> - Register offsets for the RZ/V2H SoC differ from those of the RZ/A SoC.
>>> - RZ/V2H register access is 8-bit, whereas RZ/A supports 8/16/32-bit.
>>> - RZ/V2H has some bit differences in the slave address register.
>>>
>>> To accommodate these differences in the existing driver, a new compatible
>>> string "renesas,riic-r9a09g057" is added.
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>>> Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
>>
>> I have doubts this are true reviews. What did it even show? Why this
>> review did not point problem with generic compatible?
>>
> As mentioned in path#1 these are "real"!
>
>>> ---
>>> .../devicetree/bindings/i2c/renesas,riic.yaml | 21 ++++++++++++-------
>>> 1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
>>> index 63ac5fe3208d..2a7125688647 100644
>>> --- a/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/renesas,riic.yaml
>>> @@ -15,14 +15,19 @@ allOf:
>>>
>>> properties:
>>> compatible:
>>> - 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
>>> + 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
> compatible hence they fallback to the generic RZ one.
riic-r9a09g057 is also RIIC RZ, isn't it?
>
>> This shows the ridiculousness of these generic compatibles. They are
>> generic till you figure out the truth: oh crap, it's not generic.
>>
> Sorry I lack skills to predict the future of upcoming IP blocks which
> fit in the SoC.
So don't use generic compatibles as fallbacks. That's the point.
>
>> Stop adding generic compatibles when they are not generic.
>>
> BTW I am not adding a generic compatible string here and instead
> adding a SoC specific string. Anyway DT maintainers "should not" have
> been accepting the generic compatibles from day 1 for any binding at
> all.
How can we know that you do not understand the meaning of compatibles? I
assume you do, so we ack your patches. In the same time *MULTIPLE* times
Rob said, and later me as well, people should use SoC specific
compatibles mostly.
>
> Is there a guideline where you can point me to please for when to add
> generic and when not to.
Guideline is: Don't use generic compatible at all, because you cannot
give it any meaning, based on this patch.
Best regards,
Krzysztof
