diff mbox series

[v4,1/3] dt-bindings: rng: Add Rockchip RNG bindings

Message ID b28ccedac0a51f8a437f7ceb5175e3b70696c8c2.1719106472.git.daniel@makrotopia.org
State Superseded
Headers show
Series hwrng: add hwrng support for Rockchip RK3568 | expand

Commit Message

Daniel Golle June 23, 2024, 3:32 a.m. UTC
From: Aurelien Jarno <aurelien@aurel32.net>

Add the True Random Number Generator on the Rockchip RK3568 SoC.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../bindings/rng/rockchip,rk3568-rng.yaml     | 61 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml

Comments

Krzysztof Kozlowski June 23, 2024, 7:03 a.m. UTC | #1
On 23/06/2024 05:32, Daniel Golle wrote:
> From: Aurelien Jarno <aurelien@aurel32.net>
> 
> Add the True Random Number Generator on the Rockchip RK3568 SoC.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Daniel Golle <daniel@makrotopia.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.

Best regards,
Krzysztof
Daniel Golle June 23, 2024, 1:08 p.m. UTC | #2
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@aurel32.net>
> > 
> > Add the True Random Number Generator on the Rockchip RK3568 SoC.
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > Signed-off-by: Daniel Golle <daniel@makrotopia.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
Krzysztof Kozlowski June 26, 2024, 7:30 a.m. UTC | #3
On 23/06/2024 15:08, Daniel Golle wrote:
> 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@aurel32.net>
>>>
>>> Add the True Random Number Generator on the Rockchip RK3568 SoC.
>>>
>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.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/

We talk about comments from v2, not v1.

> 
>>> +++ 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.

And where did you drop the redundant bindings? You just quoted the
sentence and ignored it.


Best regards,
Krzysztof
diff mbox series

Patch

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..ad3648b96f82
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
@@ -0,0 +1,61 @@ 
+# 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 on Rockchip RK3568 SoC
+
+maintainers:
+  - Aurelien Jarno <aurelien@aurel32.net>
+  - Daniel Golle <daniel@makrotopia.org>
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3568-rng
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: TRNG clock
+      - description: TRNG AHB clock
+
+  clock-names:
+    items:
+      - const: core
+      - const: ahb
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3568-cru.h>
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      rng@fe388000 {
+        compatible = "rockchip,rk3568-rng";
+        reg = <0x0 0xfe388000 0x0 0x4000>;
+        clocks = <&cru CLK_TRNG_NS>, <&cru HCLK_TRNG_NS>;
+        clock-names = "core", "ahb";
+        resets = <&cru SRST_TRNG_NS>;
+      };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 807feae089c4..5cd3bc2b034f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19498,6 +19498,12 @@  F:	Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst
 F:	drivers/media/platform/rockchip/rkisp1
 F:	include/uapi/linux/rkisp1-config.h
 
+ROCKCHIP RANDOM NUMBER GENERATOR SUPPORT
+M:	Daniel Golle <daniel@makrotopia.org>
+M:	Aurelien Jarno <aurelien@aurel32.net>
+S:	Maintained
+F:	Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
+
 ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
 M:	Jacob Chen <jacob-chen@iotwrt.com>
 M:	Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>