linux-kernel - Re: [PATCH v4 1/3] dt-bindings: rng: Add Rockchip RNG bindings

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 for Android: free password hash cracker in your pocket

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

Date: Sun, 23 Jun 2024 14:08:35 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Aurelien Jarno <aurelien@...el32.net>,
	Olivia Mackall <olivia@...enic.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Uwe Kleine-König <ukleinek@...ian.org>,
	Sebastian Reichel <sebastian.reichel@...labora.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Dragan Simic <dsimic@...jaro.org>, Martin Kaiser <martin@...ser.cx>,
	Ard Biesheuvel <ardb@...nel.org>, linux-crypto@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] dt-bindings: rng: Add Rockchip RNG bindings

Hi Krzysztof,

thank you for your patiente and repeated review of this series.

On Sun, Jun 23, 2024 at 09:03:15AM +0200, Krzysztof Kozlowski wrote:
> On 23/06/2024 05:32, Daniel Golle wrote:
> > From: Aurelien Jarno <aurelien@...el32.net>
> > 
> > Add the True Random Number Generator on the Rockchip RK3568 SoC.
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@...el32.net>
> > Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> 
> My comments from v2, which I reminded at v3, were not addressed.
> 
> Respond to each of them and acknowledge that you are going to implement
> the change.

Your comments to v1which I'm aware of are:
https://patchwork.kernel.org/comment/25087874/

> > +++ b/Documentation/devicetree/bindings/rng/rockchip-rng.yaml
> Filename matching compatible, so "rockchip,rk3568-rng.yaml"

I've changed the filename.

> > +title: Rockchip TRNG bindings

> Drop "bindings"

I've changed the title accordingly (now: "Rockchip TRNG" in v4).

> > +description:
> > +  This driver interface with the True Random Number Generator present in some
> 
> Drop "This driver interface" and make it a proper sentence. Bindings are
> not about drivers.

This has been addressed by Aurelien and further improved by me in v3.

> > +  clocks:
> > +    minItems: 2

> Drop minItems.

Aurelien did that in v2.

> > +  clock-names:
> > +    items:
> > +      - const: clk
> > +      - const: hclk
> 
> You need to explain what are these in clocks. Also you need better
> names. A clock name "clk" is useless.

Clocks now have meaningful names and descriptions.

> > +  reset-names:
> > +    items:
> > +      - const: reset
> 
> Drop reset-names entirely, not useful.

Aurelien did so in v2.

Your comments to v2 which I'm aware of are:
https://patchwork.kernel.org/comment/25111597/

> > Add the RNG bindings for the RK3568 SoC from Rockchip

> Use subject prefixes matching the subsystem (git log --oneline -- ...),
> so it is rng, not RNG. Also, you are not adding all-Rockhip RNG but a
> specific device.
> 
> Subject: drop second, redundant "bindings".

I've changed 'RNG' into 'rng' in the subject and spelled it out in the
commit message.

> > +description: True Random Number Generator for some Rockchip SoCs
> 
> s/for some Rockchip SoCs/on Rokchip RK3568 SoC/

I've adopted your suggestion in v3 and then fixed the typo in v4.

> 
> > +  clock-names:
> > +    items:
> > +      - const: trng_clk
> > +      - const: trng_hclk

> These are too vague names. Everything is a clk in clock-names, so no
> need usually to add it as name suffix. Give them some descriptive names,
> e.g. core and ahb.

If changed the names to the suggested 'core' and 'ahb'.

Before sending another round of patches, just to make sure we are on
the same page, please confirm that what remains is
Subject: dt-bindings: rng: Add Rockchip RNG bindings
which not only should be 'rng' in small letters but also name the exact
chip, eg.:
Subject: dt-bindings: rng: add TRNG on the Rockchip RK3568 SoC

If there are any other comments you made which I'm not aware of, please
point me to them.


Cheers


Daniel