Hi Krzysztof,
On Tue, Mar 12, 2024 at 12:04 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxxxxxx
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
