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

Commit Message

Lad, Prabhakar March 8, 2024, 5:27 p.m. UTC

From: Lad Prabhakar <[e-mail]>

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 <[e-mail]>
Reviewed-by: Fabrizio Castro <[e-mail]>
---
 .../devicetree/bindings/i2c/renesas,riic.yaml | 21 ++++++++++++-------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Krzysztof Kozlowski March 9, 2024, noon UTC | #1

On 08/03/2024 18:27, Prabhakar wrote:
> From: Lad Prabhakar <[e-mail]>
> 
> 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 <[e-mail]>
> Reviewed-by: Fabrizio Castro <[e-mail]>

I have doubts this are true reviews. What did it even show? Why this
review did not point problem with generic compatible?

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

This shows the ridiculousness of these generic compatibles. They are
generic till you figure out the truth: oh crap, it's not generic.

Stop adding generic compatibles when they are not generic.

Best regards,
Krzysztof

Lad, Prabhakar March 9, 2024, 11:28 p.m. UTC | #2

Hi Krzysztof,

Thank you for the review.

On Sat, Mar 9, 2024 at 12:00 PM Krzysztof Kozlowski
<[e-mail]> wrote:
>
> On 08/03/2024 18:27, Prabhakar wrote:
> > From: Lad Prabhakar <[e-mail]>
> >
> > 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 <[e-mail]>
> > Reviewed-by: Fabrizio Castro <[e-mail]>
>
> 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
compatible hence they fallback to the generic RZ one.

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

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

Is there a guideline where you can point me to please for when to add
generic and when not to.

Cheers,
Prabhakar

Biju Das March 10, 2024, 8:05 a.m. UTC | #3

Hi Prabhakar,

> -----Original Message-----
> From: Prabhakar <[e-mail]>
> Sent: Friday, March 8, 2024 5:27 PM
> Subject: [PATCH 2/5] dt-bindings: i2c: renesas,riic: Document R9A09G057 support
> 
> 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 <[e-mail]>
> Reviewed-by: Fabrizio Castro <[e-mail]>
> ---
>  .../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)


Based on [1], this will be

     oneOf:
	   - const: renesas,riic-r9a09g057   # RZ/V2H(P)
         - 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

[]1
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20240308&id=fddee1e686de077c80ad9dd61f4a50fa1d8b6605

Cheers,
Biju

Krzysztof Kozlowski March 10, 2024, 8:10 a.m. UTC | #4

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
> <[e-mail]> wrote:
>>
>> On 08/03/2024 18:27, Prabhakar wrote:
>>> From: Lad Prabhakar <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Reviewed-by: Fabrizio Castro <[e-mail]>
>>
>> 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

Geert Uytterhoeven March 11, 2024, 9 a.m. UTC | #5

Hi Krzysztof,

On Sun, Mar 10, 2024 at 9:10 AM Krzysztof Kozlowski
<[e-mail]> wrote:
> On 10/03/2024 00:28, Lad, Prabhakar wrote:
> > On Sat, Mar 9, 2024 at 12:00 PM Krzysztof Kozlowski
> > <[e-mail]> wrote:
> >> On 08/03/2024 18:27, Prabhakar wrote:
> >>> From: Lad Prabhakar <[e-mail]>
> >>>
> >>> 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 <[e-mail]>
> >>> Reviewed-by: Fabrizio Castro <[e-mail]>

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

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

> > compatible hence they fallback to the generic RZ one.
>
> riic-r9a09g057 is also RIIC RZ, isn't it?

Yes, as in "it comes from the division that calls its products
RZ/something". But...

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

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

Gr{oetje,eeting}s,

                        Geert

Krzysztof Kozlowski March 12, 2024, 11:04 a.m. UTC | #6

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?

> 
>>> compatible hence they fallback to the generic RZ one.
>>
>> riic-r9a09g057 is also RIIC RZ, isn't it?
> 
> Yes, as in "it comes from the division that calls its products
> RZ/something". But...
> 
>>>> 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.
> 
> 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).

Best regards,
Krzysztof

Geert Uytterhoeven March 12, 2024, 2:06 p.m. UTC | #7

Hi Krzysztof,

On Tue, Mar 12, 2024 at 12:04 PM Krzysztof Kozlowski
<[e-mail]> 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 -- [e-mail]

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

Krzysztof Kozlowski March 12, 2024, 3:05 p.m. UTC | #8

On 12/03/2024 15:06, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Tue, Mar 12, 2024 at 12:04 PM Krzysztof Kozlowski
> <[e-mail]> 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

Patch

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)
 
   reg:
     maxItems: 1