Re: [PATCH v2 1/3] dt-bindings: RNG: Add Rockchip RNG bindings

From: Aurelien Jarno <aurelien@aurel32.net>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Olivia Mackall <olivia@selenic.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Lin Jinhan <troy.lin@rock-chips.com>,
	"open list:HARDWARE RANDOM NUMBER GENERATOR CORE"
	<linux-crypto@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	"moderated list:ARM/Rockchip SoC support"
	<linux-arm-kernel@lists.infradead.org>,
	"open list:ARM/Rockchip SoC support"
	<linux-rockchip@lists.infradead.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] dt-bindings: RNG: Add Rockchip RNG bindings
Date: Fri, 2 Dec 2022 20:20:57 +0100	[thread overview]
Message-ID: <Y4pQGRMzILrkRP/2@aurel32.net> (raw)
In-Reply-To: <89b16ec5-f9a5-f836-f51a-8325448e4775@linaro.org>

Hi,

Thanks for your feedback.

On 2022-11-29 10:24, Krzysztof Kozlowski wrote:
> On 28/11/2022 19:47, Aurelien Jarno wrote:
> > 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".
> 
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  .../bindings/rng/rockchip,rk3568-rng.yaml     | 60 +++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> > new file mode 100644
> > index 000000000000..c2f5ef69cf07
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> > @@ -0,0 +1,60 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/rng/rockchip,rk3568-rng.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip TRNG
> > +
> > +description: True Random Number Generator for some Rockchip SoCs
> 
> s/for some Rockchip SoCs/on Rokchip RK3568 SoC/

My point there is that this driver should also work for other Rockchip
SoCs like the RK3588, but 1) it support for this SoC is being added and
not yet available in the Linux kernel 2) it hasn't been tested.

Should we mark it as RK3568 specific (or rather RK356x) and change that
once a compatible entry is added for the RK3588?

> > +
> > +maintainers:
> > +  - Aurelien Jarno <aurelien@aurel32.net>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - rockchip,rk3568-rng
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: TRNG clock
> > +      - description: TRNG AHB clock
> > +
> > +  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.

Those names are based on <include/dt-bindings/clock/rk3568-cru.h> and
other drivers seems to have used those for the names. But I understand
that broken things could have been merged, so I am fine changing that to
core and ahb.

> > +
> > +  resets:
> > +    maxItems: 1
> > +

Regards
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip