Hi Krzysztof Kozlowski, Thanks for the feedback. > Subject: Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock > generator bindings > > On 08/03/2023 15:39, Biju Das wrote: > > >>> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.ya > >>> +++ ml > >> > >> Filename usually is based on the compatible. Why these two are so > different? > > > > Versa3 clock generator has the following variants. > > > > 5P35023, 5L35021, 5L35023 and 5P35021 > > > > RZ/G2L SMARC EVK uses 5P35023. So I used generic one as file name. > > And added compatible for specific one. > > And what about other devices? Since you do not add them, just keep > compatible as filename. OK. > > >> > >>> + > >>> + clocks: > >>> + maxItems: 1 > >>> + > >>> + renesas,settings: > >>> + description: Optional, complete register map of the device. > >>> + Optimized settings for the device must be provided in full > >>> + and are written during initialization. > >>> + $ref: /schemas/types.yaml#/definitions/uint8-array > >>> + minItems: 37 > >> > >> maxItems instead... but I am not sure that we allow register settings > >> in DT in general. > > > > Agreed. I guess it is allowed [1] > > [1] > > I don't see Rob's review on this, so what do you prove exactly? Subject: [PATCH v9 1/2] dt-bindings: Add binding for Renesas 8T49N241 Date: Fri, 4 Feb 2022 10:57:38 +0100 [thread overview] Message-ID: <833d3837892f0879233695636291af97a746e584.1643968653.git.michal.simek@xxxxxxxxxx> (raw) In-Reply-To: <cover.1643968653.git.michal.simek@xxxxxxxxxx> From: Alex Helms <alexander.helms.jy@xxxxxxxxxxx> Renesas 8T49N241 has 4 outputs, 1 integral and 3 fractional dividers. The 8T49N241 accepts up to two differential or single-ended input clocks and a fundamental-mode crystal input. The internal PLL can lock to either of the input reference clocks or to the crystal to behave as a frequency synthesizer. Signed-off-by: Alex Helms <alexander.helms.jy@xxxxxxxxxxx> Reviewed-by: Rob Herring <robh@xxxxxxxxxx> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx> --- > > > > >> > >>> + > >>> + assigned-clocks: > >>> + minItems: 6 > >> > >> Drop. > > > > OK. > > > >> > >>> + > >>> + assigned-clock-rates: > >>> + minItems: 6 > >> > >> Drop. > > > > OK. > > > >> > >>> + > >>> + renesas,clock-divider-read-only: > >>> + description: Flag for setting divider in read only mode. > >> > >> Flag? Then type: boolean. > > > > Agreed. > >> > >>> + $ref: /schemas/types.yaml#/definitions/uint32-array > >>> + minItems: 5 > >> > >> This is broken... > > OK you mean maxItems. Based on Boolean type I will update this > > I mean, it does not match the description. Maybe I just don't understand > here something, but flag is boolean. Anyway, minItems means you can have > million of items, so was it intended? It is a mistake. > > >> > >>> + > >>> + renesas,clock-flags: > >>> + description: Flags used in common clock frame work for configuring > >>> + clk outputs. See include/linux/clk-provider.h > >> > >> These are not bindings, so why do you non-bindings constants as bindings? > >> They can change anytime. Choose one: > >> 1. Drop entire property, > >> 2. Make it a proper binding property, so an ABI and explain why this > >> is DT specific. None of clock providers have to do it, so you need > >> here good explanation. > > > > I will choose 2 and will explain as user should be allowed to > > configure the output clock from clk generator, so that it has > > flexibility for > > 1) changing the rates (propagate rate change up one level) > > 2) fixed always > > 3) don't gate the ouput clk at all. > > User's choice is task for user-space, so not a good explanation for DT. I don't think clock generator HW has any business with user space. It is clk generator HW specific. Clk generator is vital component which provides clocks to the system. We are providing some hardware feature which is exposed as dt properties. Like clock output is fixed rate clock or dynamic rate clock/ Cheers, Biju
