On 02/04/2022 13:53, Krzysztof Kozlowski wrote: > On 01/04/2022 22:17, Corentin Labbe wrote: >> The latest addition to the rockchip crypto driver need to update the >> driver bindings. >> >> Signed-off-by: Corentin Labbe <clabbe@xxxxxxxxxxxx> >> --- >> .../crypto/rockchip,rk3288-crypto.yaml | 68 +++++++++++++++++-- >> 1 file changed, 63 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml >> index 66db671118c3..e6c00bc8bebf 100644 >> --- a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml >> +++ b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml >> @@ -11,8 +11,18 @@ maintainers: >> >> properties: >> compatible: >> - enum: >> - - rockchip,rk3288-crypto >> + oneOf: >> + - description: crypto IP present on RK3288 SoCs >> + items: >> + - const: rockchip,rk3288-crypto >> + - description: crypto IP present on RK3328 SoCs > > These two comments are not helping, so this should be just enum. > >> + items: >> + - const: rockchip,rk3328-crypto >> + - description: crypto IPs present on RK3399. crypto0 is the first IP with >> + RSA support, crypto1 is the second IP without RSA. > > The second part of this comment is helpful, first not. You have chosen > enum in your first patch, so just extend it with comments. Additionally > indexing does not scale. What if next generation reverses it and crypto0 > does not have RSA and crypto1 has? Actually let me re-think this. Is programming model (registers?) same between crypto0 and crypto1? If yes, this should be same compatible and add a dedicated property "rockchip,rsa"? I looked at your driver and you modeled it as main and sub devices. I wonder why - are there some dependencies? It would be helpful to have such information here in commit msg as well. Your commit #26 says that only difference is the RSA. Best regards, Krzysztof
