On 20/03/2023 18:24, Sergio Paracuellos wrote: > Hi Krzysztof, > > On Mon, Mar 20, 2023 at 5:36 PM Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> wrote: >> >> On 20/03/2023 17:18, Sergio Paracuellos wrote: >>> Adds device tree binding documentation for clocks and resets in the >>> Mediatek MIPS and Ralink SOCs. This covers RT2880, RT3050, RT3052, RT3350, >>> RT3883, RT5350, MT7620, MT7628 and MT7688 SoCs. >> >> Use subject prefixes matching the subsystem (which you can get for >> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory >> your patch is touching). >> >> Subject: drop second/last, redundant "device tree binding >> documentation". The "dt-bindings" prefix is already stating that these >> are bindings. > > Sure, will do. Sorry for the inconvenience. > >> (BTW, that's the longest redundant component I ever saw) > > I thought it was better to just list compatible strings inside one > single file than adding the same binding in multiple files. I don't understand how this is answers about redundant piece of subject. Amount of files are not related to repeating pieces of subject prefix. > >> >>> >>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> >>> --- >>> .../bindings/clock/mtmips-clock.yaml | 68 +++++++++++++++++++ >>> 1 file changed, 68 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/clock/mtmips-clock.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/clock/mtmips-clock.yaml b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml >>> new file mode 100644 >>> index 000000000000..c92969ce231d >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml >> >> Filename matching compatible, so vendor prefix and device name (or >> family of names). > > I used mtmips here but list compatibles starting with ralink. As I > have said in the cover letter I am inspired by the last merged pinctrl > series for these SoCs. > See: > - https://lore.kernel.org/linux-gpio/e9e6ad87-2db5-9767-ff39-64a302b06185@xxxxxxxxxx/T/#t 21 patches, so what exactly I should see (except that I was involved in that discussions)? Plus nothing from this thread warrants here exception from naming style. > > Not all of compatible currently exist. Then clearly state this. > All of these are at the end the > way we can properly match compatible-data to write a proper driver. > The current ralink dtsi files which are in tree now > are totally incomplete and not documented so we are planning to align Nothing like this was said in commit msg, so how can we know? > all of this with openWRT used files and others soon. That's the reason > we are not touching > 'arch/mips/boot/dts' at all now. I don't think anybody is using any of > this but mt7621 which is properly completed and documented. Anyway, none of this explains exception from naming convention - vendor, device or family name. > >> >>> @@ -0,0 +1,68 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/clock/mtmips-clock.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: MTMIPS SoCs Clock >> >> One clock? Are you sure these describe exactly one clock? > > I will change this to 'Clocks'. Then clock provider, but are you sure? You included there syscon and reset controller. > >> >>> + >>> +maintainers: >>> + - Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> >>> + >>> +description: | >>> + MediaTek MIPS and Ralink SoCs have an XTAL from where the cpu clock is >>> + provided as well as derived clocks for the bus and the peripherals. >>> + >>> + Each clock is assigned an identifier and client nodes use this identifier >>> + to specify the clock which they consume. >> >> Drop useless or obvious pieces of description. Describe the hardware. >> >>> + >>> + The clocks are provided inside a system controller node. > >> >> ??? > > I meant, this node is a syscon from where both clock and reset related > registers are used. I think writing in this way was enough since it > has a pretty similar description like the one in > 'mediatek,mt7621-sysc.yaml'. But what is a system controller node? Some separate device? This is description for this device - called "Clock" or "Clocks" - and "system controller" appears for the first time. > >> >>> + >>> + This node is also a reset provider for all the peripherals. >> >> ??? Does it mean it is not only "Clock" but also reset controller? > > Yes, this node is a clock and reset controller for all the SoC. > >> >>> + >>> +properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - ralink,rt2880-sysc >>> + - ralink,rt3050-sysc >>> + - ralink,rt3052-sysc >>> + - ralink,rt3352-sysc >>> + - ralink,rt3883-sysc >>> + - ralink,rt5350-sysc >>> + - ralink,mt7620-sysc >>> + - ralink,mt7620a-sysc >>> + - ralink,mt7628-sysc' >>> + - ralink,mt7688-sysc >>> + - ralink,rt2880-reset >> >> That's odd. rt2880 is sysc and reset? One device with two compatibles? > > This 'ralink,rt2880-reset' is for compatibility reasons. I don't understand why. It is used in DTS, so what this node represents there? > Reset related > code was inside 'arch/mips/ralink' folder reset.c file but it is moved > to this new driver, so we have maintained this reset stuff for the > reset compatibility. All of the rest are the new possible stuff for > both reset and clocks. We talk here about hardware, not drivers, so moving driver code around does not help me understand the rationale behind bindings. > Clock driver is instantiated in two phases. The > earlier one set up the clocks via CLK_OF_DECLARE macro. Resets are set > up as a platform driver. Is only inside this where > 'ralink,rt2880-reset' is used. See patch 2 of the series for details. Sure, but it is not related to bindings. > >> >> Also, order these by name. > > All are ordered but I maintained the 'ralink,rt2880-reset' at the end. No, it is not. m is before r in alphabet. Best regards, Krzysztof
