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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 02/12/2022 20:20, Aurelien Jarno wrote:
> 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)

Driver maybe less, but bindings might not.

> 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?

Describe what you are adding here, not something else.

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

clock-names is not for the actual name of the clock feeding it, but
rather name of input of the device. Reader-friendly.

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

Best regards,
Krzysztof