On 10/01/2023 03:14, Xingyu Wu wrote: > On 2022/12/23 18:25, Krzysztof Kozlowski wrote: >> On 23/12/2022 10:47, Xingyu Wu wrote: >>> Add bindings for the timer on the JH7110 >>> RISC-V SoC by StarFive Technology Ltd. >> >> Please wrap commit message according to Linux coding style / submission >> process (neither too early nor over the limit): >> https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 >> >> >>> >>> Signed-off-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx> >>> --- >>> .../timer/starfive,jh7110-timers.yaml | 105 ++++++++++++++++++ >>> 1 file changed, 105 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/timer/starfive,jh7110-timers.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/timer/starfive,jh7110-timers.yaml b/Documentation/devicetree/bindings/timer/starfive,jh7110-timers.yaml >>> new file mode 100644 >>> index 000000000000..fe58dc056313 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/timer/starfive,jh7110-timers.yaml >>> @@ -0,0 +1,105 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/timer/starfive,jh7110-timers.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: StarFive Timers >> >> >> Not enough, really not enough. Describe the hardware. > > Will add. Thanks. > >> >>> + >>> +maintainers: >>> + - Samin Guo <samin.guo@xxxxxxxxxxxxxxxx> >>> + - Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx> >>> + >>> +properties: >>> + compatible: >>> + const: starfive,jh7110-timers >> >> Why plural "timers", not "timer"? The module is usually called timer - >> see other hardware that type. >> > > Will fix. Thanks. > >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + interrupts: >>> + items: >>> + - description: timer channel 0 interrupt >>> + - description: timer channel 1 interrupt >>> + - description: timer channel 2 interrupt >>> + - description: timer channel 3 interrupt >>> + >>> + interrupt-names: >>> + items: >>> + - const: timer0 >>> + - const: timer1 >>> + - const: timer2 >>> + - const: timer3 >> >> I would just drop the names, not really useful. Unless you plan to add >> here some generic interrupt (like you did for clock-names)? > > Will drop. Thanks. > >> >>> + >>> + clocks: >>> + items: >>> + - description: timer channel 0 clock >>> + - description: timer channel 1 clock >>> + - description: timer channel 2 clock >>> + - description: timer channel 3 clock >>> + - description: APB clock >>> + >>> + clock-names: >>> + items: >>> + - const: timer0 >>> + - const: timer1 >>> + - const: timer2 >>> + - const: timer3 >>> + - const: apb >>> + >>> + resets: >>> + items: >>> + - description: timer channel 0 reset >>> + - description: timer channel 1 reset >>> + - description: timer channel 2 reset >>> + - description: timer channel 3 reset >>> + - description: APB reset >>> + >>> + reset-names: >>> + items: >>> + - const: timer0 >>> + - const: timer1 >>> + - const: timer2 >>> + - const: timer3 >>> + - const: apb >>> + >>> + clock-frequency: >>> + description: The frequency of the clock that drives the counter, in Hz. >> >> Why do you need it? Use common clk framework to get that frequency. > > Because normally this timer driver is loaded earlier than the clock tree driver, it won't get > that frequency by clk framework and this 'clock-frequency' node is used instead. I don't think that clk framework or fixed clocks are not available at this time... of_clk_init is before timer. > >> >> Also, sort the nodes somehow, e.g. >> compatible/reg/clocks/clock-frequency/interrupts/resets. > > Will reorder. Thanks. > >> >> >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - interrupt-names >>> + - clocks >>> + - clock-names >>> + - resets >>> + - reset-names >>> + - clock-frequency >>> + >>> +unevaluatedProperties: false >> >> Did you test the binding? > > Yes, I had tested by 'dt_binding_check'. Do you mean the 'unevaluatedProperties' is wrong > and use 'additionalProperties'? Yes, previously it was generating a warning but I do not see Rob's bot answer so maybe something changed. Best regards, Krzysztof
