Re: [PATCH 2/7] dt-bindings: rng: add binding for Rockchip RK3588 RNG

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Friday 31 January 2025 09:30:51 Central European Standard Time Krzysztof 
Kozlowski wrote:
> On 30/01/2025 17:31, Nicolas Frattaroli wrote:
> > +title: Rockchip RK3588 TRNG
> > +
> > +description: True Random Number Generator on Rockchip RK3588 SoC
> > +
> > +maintainers:
> > +  - Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - rockchip,rk3588-rng
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: TRNG AHB clock
> > +
> > +  # Optional, not used by some driver implementations
> 
> What driver implementations? Downstream? They do not matter, because
> they are full of all sort of crap.

Downstream and this very driver in the series. The patch includes a lengthy 
explanation of why all known implementations have foregone using the 
interrupt.

> Can this block have interrupt really disconnected? This is the question
> you should answer.

I do not have the gift of prescience. If you want me to make the interrupt 
required even though it is pointless just because all hardware that implements 
this that I'm currently aware of does have the interrupt then I will add it as 
a required property.
 
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  # Optional, hardware works without explicit reset
> 
> Just because bootloader did something? With that reasoning nothing is
> ever required because firmware can abstract it. Either you have there a
> reset or not. In this particular case your driver is irrelevant.

No, it's not the bootloader that does it, it's the hardware itself. It's 
power-on reset. Hardware can reset itself without software, I don't know where 
the assumption of it being done by the bootloader comes from.

> > +  resets:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +
> 
> BTW, there is a binding for Rockchip TRNG, with a bit different clocks
> so I have feeling yours is incomplete here.

I know and they're not. As the cover letter states, this is different hardware 
from the other Rockchip TRNG. This hardware only has one clock. It is a 
separate binding with different clocks because it is different hardware.

The patch's message also states this:

> [...] and a standalone RNG that is new to this SoC.
> 
> Add a binding for this new standalone RNG.

It's a standalone RNG that is new to this SoC. It has different registers, and 
different clock requirements.

> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/reset/rockchip,rk3588-cru.h>
> > +    bus {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      rng@fe378000 {
> > +        compatible = "rockchip,rk3588-rng";
> > +        reg = <0x0 0xfe378000 0x0 0x200>;
> > +        interrupts = <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH 0>;
> > +        clocks = <&scmi_clk SCMI_HCLK_SECURE_NS>;
> > +        resets = <&scmi_reset SCMI_SRST_H_TRNG_NS>;
> > +        status = "disabled";
> 
> Examples cannot be disabled.

Okay, I will fix this.

> > +      };
> > +    };
> > +
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index
> > bc8ce7af3303f747e0ef028e5a7b29b0bbba99f4..7daf9bfeb0cb4e9e594b809012c7aa2
> > 43b0558ae 100644 --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -20420,8 +20420,10 @@ F:	include/uapi/linux/rkisp1-config.h
> > 
> >  ROCKCHIP RK3568 RANDOM NUMBER GENERATOR SUPPORT
> >  M:	Daniel Golle <daniel@makrotopia.org>
> >  M:	Aurelien Jarno <aurelien@aurel32.net>
> > 
> > +M:	Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> 
> Like Conor said, this is not really relevant and should be a separate patch.

Should just the added M be a separate patch or also adding the file to the 
MAINTAINERS file? Because if the latter is separate then I think checkpatch 
will yell at me, and if the former is separate then the binding's statement 
that it is maintained by me is disconnected from what's in the MAINTAINERS 
file.

> 
> 
> 
> Best regards,
> Krzysztof