Re: [PATCH v3 1/2] dt-bindings: serial: samsung: fix maxItems for gs101 & document earlycon requirements

[André Draszik]

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'