Re: [PATCH 2/5] dt-bindings: i2c: renesas,riic: Document R9A09G057 support

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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@linaro.org> wrote:
>>
>> On 08/03/2024 18:27, Prabhakar wrote:
>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>
>>> 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@bp.renesas.com>
>>> Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
>>
>> 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