Re: [PATCH v3 02/06] devicetree: bindings: R-Car Gen2 CMT0 and CMT1 bindings

[Magnus Damm]

Hi Simon,

On Wed, Feb 17, 2016 at 3:28 PM, Simon Horman <horms@xxxxxxxxxxxx> wrote:
> On Wed, Feb 17, 2016 at 11:33:27AM +0900, Magnus Damm wrote:
>> Hi Geert,
>>
>> On Tue, Feb 16, 2016 at 10:11 PM, Geert Uytterhoeven
>> <geert@xxxxxxxxxxxxxx> wrote:
>> > On Tue, Feb 16, 2016 at 8:17 AM, Magnus Damm <magnus.damm@xxxxxxxxx> wrote:
>> >> From: Magnus Damm <damm+renesas@xxxxxxxxxxxxx>
>> >>
>> >> Add documentation for new separate CMT0 and CMT1 DT compatible strings
>> >> for R-Car Gen2. These compat strings allow us to enable CMT1-specific
>> >> features in the driver. The old compat strings will be deprecated in
>> >> the not so distant future.
>> >>
>> >> Signed-off-by: Magnus Damm <damm+renesas@xxxxxxxxxxxxx>
>> >> Acked-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>> >> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> >> Acked-by: Rob Herring <robh@xxxxxxxxxx>
>> >> ---
>> >>
>> >>  Changes since V2:
>> >>  - Added Acked-by from Rob
>> >>  - Removed Tested-by tag from DT binding patch - duh!
>> >>
>> >>  Changes since V1:
>> >>  - Added Acked-by and Tested-by from Geert
>> >>  - Added Acked-by from Laurent
>> >>
>> >>  Documentation/devicetree/bindings/timer/renesas,cmt.txt |    3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> --- 0002/Documentation/devicetree/bindings/timer/renesas,cmt.txt
>> >> +++ work/Documentation/devicetree/bindings/timer/renesas,cmt.txt        2015-09-17 17:26:57.440513000 +0900
>> >> @@ -36,6 +36,9 @@ Required Properties:
>> >>                 (CMT1 on sh73a0 and r8a7740)
>> >>                 This is a fallback for the above renesas,cmt-48-* entries.
>> >>
>> >> +    - "renesas,cmt0-rcar-gen2" for 32-bit CMT0 devices included in R-Car Gen2.
>> >> +    - "renesas,cmt1-rcar-gen2" for 48-bit CMT1 devices included in R-Car Gen2.
>> >
>> > (advancing a few months always means more comments ;-)
>>
>> Indeed!
>>
>> > I'm wondering whether we should use e.g. "renesas,rcar-gen2-cmt0" instead?
>>
>> I have no strong feelings one way or the other, but I agree that
>> aiming to make things more consistent must be good.
>
> FWIW, I agree with Geert's suggestion.
> But I also think it is not particularly important.
>
>> Your proposal makes the fallback match with what we do for a bunch
>> other devices on R-Car Gen2 like:
>> "rcar-gen2-cpg-clocks"
>> "rcar-gen2-scif"
>> But we also seem to have:
>> "pci-rcar-gen2"
>
> Bother, it looks like I got pci backwards :(
>
>> On R-Car Gen3 we seem to have the following per-SoC compat strings:
>> "dmac-r8a7795"
>> "etheravb-r8a7795"
>> "gpio-r8a7795"
>> "scif-r8a7795"
>
> I believe the above are to follow the existing pattern for
> per-SoC compat strings in the same driver, which seems sane.
>
>> But we also have:
>> "r8a7795-cpg-mssr"
>>
>> My only feeling is that it looks a tad odd if we follow
>> "<vendor>,<family-generation>-<device>" for fallback strings but
>> "<vendor>,<device>-<part-number>" for the per-soc binding. Why not
>> using the same order? Maybe this is specified in some document
>> somewhere?
>
> I believe that the problem is a historical one. Perhaps when
> we started adding bindings for our hardware there were no clear
> guidelines. But regardless we ended up with a mix as you describe.

Right, that seems to be the way things have happened.

> In the mean time guidelines have emerged and we (or at least I) have
> agreed with the device tree people (probably Rob) to use the
> <vendor>,<chip>-<device> format for new bindings. My reading is that
> applies even if it results in fallback and non-fallback bindings for the
> same driver have different orders. Some precedence for this can now be found
> in renesas,rcar-dmac.txt.

Some sort of agreed format sounds good.

Your proposal of <vendor>,<chip>-<device> sounds good to me.

I'm however slightly more confused by seeing that your example
renesas,rcar-dmac.txt included in renesas-drivers-2016-02-16-v4.5-rc4
does not match the proposed format:

$ grep "renesas," Documentation/devicetree/bindings/dma/renesas,rcar-dmac.txt
- compatible: "renesas,dmac-<soctype>", "renesas,rcar-dmac" as fallback.
        - "renesas,dmac-r8a7790" (R-Car H2)
        - "renesas,dmac-r8a7791" (R-Car M2-W)
        - "renesas,dmac-r8a7792" (R-Car V2H)
        - "renesas,dmac-r8a7793" (R-Car M2-N)
        - "renesas,dmac-r8a7794" (R-Car E2)
        - "renesas,dmac-r8a7795" (R-Car H3)
        compatible = "renesas,dmac-r8a7790", "renesas,rcar-dmac";
        compatible = "renesas,dmac-r8a7790", "renesas,rcar-dmac";
$

> I don't, however, think it applies where we add more soc-specific to a
> driver that already has such bindings. Or new fallback bindings to a driver
> that already has such bindings.

Right, but for rcar-dmac.c there is no matching on per-SoC strings in
the driver so I guess we can pick anything we want?

$ grep "renesas," drivers/dma/sh/rcar-dmac.c
    { .compatible = "renesas,rcar-dmac", },
$

>> I guess your take with "r8a7795-cpg-mssr" above is to follow the same
>> order as for the fallback case? This seems sane to me, and if so then
>> perhaps the per-soc compat strings for the CMT should also be updated?
>> Same for other devices too then, like the recently added
>> "dmac-r8a7795"?
>
> From my point of view it would be nice to clean things up and
> re-order all the bindings. But I think the drivers would
> need to maintain compatibility with the old strings. And I wonder
> if it is really worth the effort.

No need to rework existing stuff IMO. However once we rework DT
bindings (CMT) or add new ones (SYS-DMAC) then we have a good
opportunity to clean things up.

Thanks,

/ magnus