| Message ID | 20240308172726.225357-3-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add RIIC support for Renesas RZ/V2H SoC | expand |
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? > --- > .../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
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 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
Hi Prabhakar, > -----Original Message----- > From: Prabhakar <prabhakar.csengg@gmail.com> > 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 <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > --- > .../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
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
Hi Krzysztof, On Sun, Mar 10, 2024 at 9:10 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 10/03/2024 00:28, Lad, Prabhakar wrote: > > 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> > >>> --- 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
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
Hi Krzysztof, On Tue, Mar 12, 2024 at 12:04 PM 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 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 -- geert@linux-m68k.org 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
On 12/03/2024 15:06, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Tue, Mar 12, 2024 at 12:04 PM 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 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
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