On 08/03/2023 15:39, Biju Das wrote: >>> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml >> >> 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. >> >>> + >>> + 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] https://lore.kernel.org/linux-renesas-soc/833d3837892f0879233695636291af97a746e584.1643968653.git.michal.simek@xxxxxxxxxx/ I don't see Rob's review on this, so what do you prove exactly? > >> >>> + >>> + 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? >> >>> + >>> + 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. Best regards, Krzysztof
