Hi Rob, On Thu, 2024-07-11 at 15:23 -0600, Rob Herring wrote: > On Thu, Jul 11, 2024 at 05:09:50PM +0100, André Draszik wrote: > > Hi Rob, > > > > On Thu, 2024-07-11 at 09:51 -0600, Rob Herring wrote: > > > On Wed, Jul 10, 2024 at 7:29 AM André Draszik <andre.draszik@xxxxxxxxxx> wrote: > > > > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > > > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml > > > > @@ -145,6 +145,20 @@ allOf: > > > > - samsung,uart-fifosize > > > > properties: > > > > reg-io-width: false > > > > > > blank line between properties > > > > Do mean before clocks: below and before clock-names: below? > > Yes. > > > We don't do that normally, > > at least none of the bindings I looked at do that. Or did I misunderstand? > > That style is pretty universal. If in doubt, look at example-schema.yaml > for best practices. The exception is only for cases like this: > > foo: true > bar: true example-schema.yaml doesn't have an example for that, so I suspect that's why many (Samsung?) bindings ended up without the blank line. I have fixed it for this schema in the next version in https://lore.kernel.org/all/20240712-gs101-uart-binding-v4-1-24e9f8d4bdcb@xxxxxxxxxx/ > > > > > > > + maxItems: 2 > > > > + clock-names: > > > > + items: > > > > + - const: uart > > > > + - const: clk_uart_baud0 > > > > > > Which clock is pclk and ipclk? > > > > uart is pclk, clk_uart_baud0 is ipclk. > > > > > 'baud' would be sufficient for the > > > name. 'clk_' and 'uart' are redundant because it's all clocks and they > > > are all for the uart. > > > > TBH, this patch is just following the existing style & names as already exist for > > various other SoCs in this same file. Furthermore, up until this patch the default > > from this file applies, which is: > > > > clock-names: > > description: N = 0 is allowed for SoCs without internal baud clock mux. > > minItems: 2 > > items: > > - const: uart > > - pattern: '^clk_uart_baud[0-3]$' > > - pattern: '^clk_uart_baud[0-3]$' > > - pattern: '^clk_uart_baud[0-3]$' > > - pattern: '^clk_uart_baud[0-3]$' > > Then don't duplicate it. Ideally, the names are defined at the top level > and the conditional schema just limits the number of clocks, and this is > an example of why we want it that way. I have no context to see if this > is consistent or not. I've fixed it in https://lore.kernel.org/all/20240712-gs101-uart-binding-v4-1-24e9f8d4bdcb@xxxxxxxxxx/ Hopefully you'll that's more acceptable :-) Cheers, Andre'
