Re: [PATCH v2 1/3] dt-bindings: clock: add rk3576 cru bindings

[Detlev Casanova <detlev.casanova@collabora.com>]

Hi Krzysztof,

On Sunday, 4 August 2024 05:52:53 EDT Krzysztof Kozlowski wrote:
> On 02/08/2024 23:35, Detlev Casanova wrote:
> > Document the device tree bindings of the rockchip rk3576 SoC
> > clock and reset unit.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> 
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bi
> ndings/submitting-patches.rst#L18
> > ---
> > 
> >  .../bindings/clock/rockchip,rk3576-cru.yaml   | 73 +++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/rockchip,rk3576-cru.yaml> 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/rockchip,rk3576-cru.yaml
> > b/Documentation/devicetree/bindings/clock/rockchip,rk3576-cru.yaml new
> > file mode 100644
> > index 0000000000000..929eb6183bf18
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3576-cru.yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/rockchip,rk3576-cru.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip rk3576 Family Clock and Reset Control Module
> > +
> > +maintainers:
> > +  - Elaine Zhang <zhangqing@rock-chips.com>
> > +  - Heiko Stuebner <heiko@sntech.de>
> > +
> > +description: |
> > +  The RK3576 clock controller generates the clock and also implements a
> > reset +  controller for SoC peripherals. For example it provides
> > SCLK_UART2 and +  PCLK_UART2, as well as SRST_P_UART2 and SRST_S_UART2
> > for the second UART +  module.
> > +  Each clock is assigned an identifier and client nodes can use this
> > identifier +  to specify the clock which they consume. All available
> > clock and reset IDs +  are defined as preprocessor macros in dt-binding
> > headers.
> 
> Drop paragraph, it is obvious. You could provide here the name of the
> header...
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - rockchip,rk3576-cru
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  "#reset-cells":
> > +    const: 1
> > +
> > +  clocks:
> > +    minItems: 2
> 
> You can drop minitems
> 
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    items:
> > +      - const: xin24m
> > +      - const: xin32k
> > +
> > +  assigned-clocks: true
> > +
> > +  assigned-clock-rates: true
> > +
> > +  assigned-clock-parents: true
> 
> Drop  all these three

Why dro pthese if I need them in the device tree ? Should I remove them from 
there as well ? It seems to be working without it.

> > +
> > +  rockchip,grf:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: >
> > +      phandle to the syscon managing the "general register files". It is
> > used +      for GRF muxes, if missing any muxes present in the GRF will
> > not be +      available.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#clock-cells"
> > +  - "#reset-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    cru: clock-controller@27200000 {
> 
> Drop unused label
> 
> > +      compatible = "rockchip,rk3576-cru";
> > +      reg = <0xfd7c0000 0x5c000>;
> > +      #clock-cells = <1>;
> > +      #reset-cells = <1>;
> 
> Make the example complete.
> 
> > +    };
> 

Detlev.