linux-kernel - Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230904062502.qtajep4zyslnouxv@basti-XPS-13-9310>
Date:   Mon, 4 Sep 2023 08:25:02 +0200
From:   Sebastian Fricke <sebastian.fricke@...labora.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     linux-media@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        kernel@...labora.com, bob.beckett@...labora.com,
        hverkuil-cisco@...all.nl, nicolas.dufresne@...labora.com,
        nas.chung@...psnmedia.com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree
 bindings

Hey Krzysztof,

sorry for digging this mail out of the tombs.
We have worked for quite a while on a new version of the driver and we
are currently finishing it up. I have a few questions before I can
finish the patchset.

Please see below...

On 07.12.2022 13:31, Krzysztof Kozlowski wrote:
>On 07/12/2022 13:13, Sebastian Fricke wrote:
>> From: Robert Beckett <bob.beckett@...labora.com>
>>
>> Add bindings for the wave5 chips&media codec driver
>>
>> Signed-off-by: Robert Beckett <bob.beckett@...labora.com>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@...labora.com>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke@...labora.com>
>
>What's happening with this patch? Where is the changelog? Why it is v11
>and first time I see it? And why it is v11 with basic mistakes and lack
>of testing?!? I would assume that v11 was already seen and tested...
>
>
>> ---
>>  .../devicetree/bindings/cnm,wave5.yml         | 72 +++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/cnm,wave5.yml
>
>Wrong directory. It wasn't here at all before, so I am really confused
>how this could happen.
>
>Subject: drop redundant pieces: yaml, devicetree and bindings.
>
>
>>
>> diff --git a/Documentation/devicetree/bindings/cnm,wave5.yml b/Documentation/devicetree/bindings/cnm,wave5.yml
>> new file mode 100644
>> index 000000000000..01dddebb162e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cnm,wave5.yml
>> @@ -0,0 +1,72 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/wave5.yaml#
>
>You clearly did not test them before sending.
>
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Chips&Media Wave 5 Series multi-standard codec IP
>> +
>> +maintainers:
>> +  - Nas Chung <nas.chung@...psnmedia.com>
>> +  - Robert Beckett <bob.beckett@...labora.com>
>> +  - Sebastian Fricke <sebastian.fricke@...labora.com>
>> +
>> +description: |-
>> +  The Chips&Media WAVE codec IP is a multi format video encoder/decoder
>> +
>> +properties:
>> +  compatible:
>> +    anyOf:
>
>Please start from example-schema or other recently approved bindings. No
>anyOf.
>
>> +      - items:
>
>No items...
>
>> +        - enum:
>> +            - cnm,cm511-vpu
>> +            - cnm,cm517-vpu
>> +            - cnm,cm521-vpu
>> +            - cnm,cm521c-vpu
>> +            - cnm,cm521c-dual-vpu
>
>What's the difference between this and one above?
>
>> +            - cnm,cm521e1-vpu
>> +            - cnm,cm537-vpu
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    minItems: 1
>> +    maxItems: 4
>
>This has to be specific.
>
>> +
>> +  clock-names:
>> +    minItems: 1
>> +    maxItems: 4
>
>You need to list the names.
>
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  sram:
>
>Missing vendor prefix.

After some discussion with the the manufacturer of this CODEC chip, the SRAM
is not fixed to the CODEC chip but instead part of the SoC, thus the
vendor can vary. It sounds like the policy is to use the vendor prefix
of the SoC, that was used for upstreaming. But that policy sounds a bit
like a potential for future confusion to me, so I wanted to ask what you
would like to see. The SoC we develop on is from TI and the CODEC chip is from
C&M, so I could either call it: `ti,sram` or `cnm,sram`

>
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: phandle pointing to the SRAM device node
>
>And what is it for? Why do you need SRAM?

I would write the following here:
The VPU uses the SRAM to store some of the reference data instead of storing it on DMA memory. It is mainly used for the purpose of reducing bandwidth.

Sincerely,
Sebastian

>
>> +    maxItems: 1
>
>Drop
>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    vpu: video-codec@...45678 {
>> +        compatible = "cnm,cm521-vpu";
>> +        reg = <0x12345678 0x1000>;
>> +        interrupts = <42>;
>> +        clocks = <&clks 42>;
>> +        clock-names = "vcodec";
>> +        sram = <&sram>;
>> +    };
>
>Best regards,
>Krzysztof
>