Hi Rob, On Tue, Oct 5, 2021 at 3:27 PM Rob Herring <robh@xxxxxxxxxx> wrote: > > On Mon, Oct 4, 2021 at 1:23 PM Sergio Paracuellos > <sergio.paracuellos@xxxxxxxxx> wrote: > > > > Hi Rob, > > > > On Mon, Oct 4, 2021 at 8:06 PM Rob Herring <robh@xxxxxxxxxx> wrote: > > > > > > On Sun, Sep 26, 2021 at 04:59:30PM +0200, Sergio Paracuellos wrote: > > > > Adds device tree binding documentation for resets in the ralink RT2880 SoCs. > > > > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> > > > > --- > > > > .../bindings/reset/ralink,rt2880-reset.yaml | 39 +++++++++++++++++++ > > > > 1 file changed, 39 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/reset/ralink,rt2880-reset.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/reset/ralink,rt2880-reset.yaml b/Documentation/devicetree/bindings/reset/ralink,rt2880-reset.yaml > > > > new file mode 100644 > > > > index 000000000000..88eddeb4ee45 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/reset/ralink,rt2880-reset.yaml > > > > @@ -0,0 +1,39 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/reset/ralink,rt2880-reset.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Ralink RT2880 Reset Controller Device Tree Bindings > > > > + > > > > +maintainers: > > > > + - Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> > > > > + > > > > +description: | > > > > + Ralink RT2880 reset controller driver which supports the SoC > > > > + system controller supplied reset registers for the various peripherals > > > > + of the SoC. > > > > + > > > > + See also: > > > > + - dt-bindings/reset/ralink-rt2880.h > > > > + > > > > +properties: > > > > + compatible: > > > > + const: ralink,rt2880-reset > > > > + > > > > + '#reset-cells': > > > > + const: 1 > > > > + > > > > +required: > > > > + - '#reset-cells' > > > > + - compatible > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + #include <dt-bindings/reset/ralink-rt2880.h> > > > > + rstctrl: reset-controller { > > > > + compatible = "ralink,rt2880-reset"; > > > > + #reset-cells = <1>; > > > > > > How is this h/w controlled? If this is part of a system controller, then > > > it needs to be documented as such. IOW, you need to document the binding > > > for the whole block. > > > > > > Do you really need a child node here? All you need to make a system > > > controller a reset provider is add '#reset-cells' to it. > > > > I am just documenting what is already mainlined (see [0]) in order to > > get mt7621-dts out of staging at some point of my life. What am I > > supposed to do? Should I rewrite all already mainlined code? Because > > if that is the case we need to rewrite tons of things from the ralink > > platform... > > On the flip side, am I not supposed to review bindings because the dts > is already in staging? Code dependent on DT bindings shouldn't have > been mainlined without any documented binding. Thanks for reviewing this. I guess I should have sent a complete patchset with all remaining bindings and the move for the complete binding instead of sending single binding doc patches. > > Looks like the resets are part of "mediatek,mt7621-sysc" to answer my > question. Add a #reset-cell to that node (and binding) and then change > this line to "mediatek,mt7621-sysc": > > reset_dev.of_node = of_find_compatible_node(NULL, NULL, > "ralink,rt2880-reset"); > > That's the minimal change, but really I would move the reset code to > the clock driver as that is what handles the sysc node. It is not that easy since the code in reset.c is shared for all ralink platforms and the mediatek,mt7621-sysc node is only for mt7621. So I guess I have to "duplicate" the reset code and put it in the clock driver for mt7621 as you are pointing out here. I have to also review how other drivers are using the reset, using reset apis or directly through the syscon. > > > > I'd also like to know what we should do with those nodes already added > > to the dtsi file that have not got associated compatible driver > > mainlined. Can we just get rid of them? > > Yes. Typically dts files start with minimal support. > > A dts file in staging is odd. We shouldn't be adding them there. Thanks for clarification. Best regards, Sergio Paracuellos > > Rob
