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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <58e355454db3670157645c0b6e727d8058ef0324.camel@linaro.org>
Date: Fri, 12 Jul 2024 15:54:50 +0100
From: André Draszik <andre.draszik@...aro.org>
To: Rob Herring <robh@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby
 <jirislaby@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Peter Griffin <peter.griffin@...aro.org>,
 Sylwester Nawrocki <s.nawrocki@...sung.com>, Chanwoo Choi
 <cw00.choi@...sung.com>, Alim Akhtar <alim.akhtar@...sung.com>, Michael
 Turquette <mturquette@...libre.com>,  Stephen Boyd <sboyd@...nel.org>, Sam
 Protsenko <semen.protsenko@...aro.org>, Tudor Ambarus
 <tudor.ambarus@...aro.org>, Will McVicker <willmcvicker@...gle.com>, 
 kernel-team@...roid.com, linux-kernel@...r.kernel.org, 
 linux-serial@...r.kernel.org, devicetree@...r.kernel.org, 
 linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org, 
 linux-clk@...r.kernel.org
Subject: Re: [PATCH v3 1/2] dt-bindings: serial: samsung: fix maxItems for
 gs101 & document earlycon requirements

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@...aro.org> 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@linaro.org/

> > > 
> > > > +          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@linaro.org/

Hopefully you'll that's more acceptable :-)

Cheers,
Andre'