Re: [PATCH 2/3] dt: bindings: add ralink RT2880 resets device tree binding documentation

[Sergio Paracuellos <sergio.paracuellos@gmail.com>]

Hi Rob,

On Tue, Oct 5, 2021 at 3:27 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Oct 4, 2021 at 1:23 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Hi Rob,
> >
> > On Mon, Oct 4, 2021 at 8:06 PM Rob Herring <robh@kernel.org> 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@gmail.com>
> > > > ---
> > > >  .../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@gmail.com>
> > > > +
> > > > +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