Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ee72df94-aa01-9ac1-f74e-fbfa1e71d2a2@linaro.org>
Date:   Sat, 3 Dec 2022 11:21:45 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH v2 1/3] dt-bindings: RNG: Add Rockchip RNG bindings
To:     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>
References: <20221128184718.1963353-1-aurelien@aurel32.net>
 <20221128184718.1963353-2-aurelien@aurel32.net>
 <89b16ec5-f9a5-f836-f51a-8325448e4775@linaro.org>
 <Y4pQGRMzILrkRP/2@aurel32.net>
Content-Language: en-US
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <Y4pQGRMzILrkRP/2@aurel32.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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