devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings
       [not found] <20221207121350.66217-1-sebastian.fricke@collabora.com>
@ 2022-12-07 12:13 ` Sebastian Fricke
  2022-12-07 12:31   ` Krzysztof Kozlowski
  0 siblings, 1 reply; 8+ messages in thread
From: Sebastian Fricke @ 2022-12-07 12:13 UTC (permalink / raw)
  To: linux-media, Rob Herring, Krzysztof Kozlowski
  Cc: kernel, bob.beckett, hverkuil-cisco, nicolas.dufresne, nas.chung,
	Sebastian Fricke, devicetree, linux-kernel

From: Robert Beckett <bob.beckett@collabora.com>

Add bindings for the wave5 chips&media codec driver

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 .../devicetree/bindings/cnm,wave5.yml         | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cnm,wave5.yml

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#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Chips&Media Wave 5 Series multi-standard codec IP
+
+maintainers:
+  - Nas Chung <nas.chung@chipsnmedia.com>
+  - Robert Beckett <bob.beckett@collabora.com>
+  - Sebastian Fricke <sebastian.fricke@collabora.com>
+
+description: |-
+  The Chips&Media WAVE codec IP is a multi format video encoder/decoder
+
+properties:
+  compatible:
+    anyOf:
+      - items:
+        - enum:
+            - cnm,cm511-vpu
+            - cnm,cm517-vpu
+            - cnm,cm521-vpu
+            - cnm,cm521c-vpu
+            - cnm,cm521c-dual-vpu
+            - cnm,cm521e1-vpu
+            - cnm,cm537-vpu
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+    maxItems: 4
+
+  clock-names:
+    minItems: 1
+    maxItems: 4
+
+  interrupts:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  sram:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle pointing to the SRAM device node
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    vpu: video-codec@12345678 {
+        compatible = "cnm,cm521-vpu";
+        reg = <0x12345678 0x1000>;
+        interrupts = <42>;
+        clocks = <&clks 42>;
+        clock-names = "vcodec";
+        sram = <&sram>;
+    };
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings
  2022-12-07 12:13 ` [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings Sebastian Fricke
@ 2022-12-07 12:31   ` Krzysztof Kozlowski
  2022-12-07 13:17     ` Krzysztof Kozlowski
                       ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-07 12:31 UTC (permalink / raw)
  To: Sebastian Fricke, linux-media, Rob Herring, Krzysztof Kozlowski
  Cc: kernel, bob.beckett, hverkuil-cisco, nicolas.dufresne, nas.chung,
	devicetree, linux-kernel

On 07/12/2022 13:13, Sebastian Fricke wrote:
> From: Robert Beckett <bob.beckett@collabora.com>
> 
> Add bindings for the wave5 chips&media codec driver
> 
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.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@chipsnmedia.com>
> +  - Robert Beckett <bob.beckett@collabora.com>
> +  - Sebastian Fricke <sebastian.fricke@collabora.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.

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle pointing to the SRAM device node

And what is it for? Why do you need SRAM?

> +    maxItems: 1

Drop

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

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings
  2022-12-07 12:31   ` Krzysztof Kozlowski
@ 2022-12-07 13:17     ` Krzysztof Kozlowski
  2022-12-07 15:09     ` Sebastian Fricke
  2023-09-04  6:25     ` Sebastian Fricke
  2 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-07 13:17 UTC (permalink / raw)
  To: Sebastian Fricke, linux-media, Rob Herring, Krzysztof Kozlowski
  Cc: kernel, bob.beckett, hverkuil-cisco, nicolas.dufresne, nas.chung,
	devicetree, linux-kernel

On 07/12/2022 13:31, Krzysztof Kozlowski wrote:
> On 07/12/2022 13:13, Sebastian Fricke wrote:
>> From: Robert Beckett <bob.beckett@collabora.com>
>>
>> Add bindings for the wave5 chips&media codec driver
>>
>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.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@chipsnmedia.com>
>> +  - Robert Beckett <bob.beckett@collabora.com>
>> +  - Sebastian Fricke <sebastian.fricke@collabora.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

One more question - why "vpu" suffixes?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings
  2022-12-07 12:31   ` Krzysztof Kozlowski
  2022-12-07 13:17     ` Krzysztof Kozlowski
@ 2022-12-07 15:09     ` Sebastian Fricke
  2022-12-07 15:27       ` Krzysztof Kozlowski
  2023-09-04  6:25     ` Sebastian Fricke
  2 siblings, 1 reply; 8+ messages in thread
From: Sebastian Fricke @ 2022-12-07 15:09 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-media, Rob Herring, Krzysztof Kozlowski, kernel,
	bob.beckett, hverkuil-cisco, nicolas.dufresne, nas.chung,
	devicetree, linux-kernel

Hello Krzysztof, 

On 07.12.2022 13:31, Krzysztof Kozlowski wrote:
>On 07/12/2022 13:13, Sebastian Fricke wrote:
>> From: Robert Beckett <bob.beckett@collabora.com>
>>
>> Add bindings for the wave5 chips&media codec driver
>>
>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>
>What's happening with this patch? Where is the changelog?

The changelog is located in the cover letter.
https://lore.kernel.org/linux-media/20221207121350.66217-1-sebastian.fricke@collabora.com/

>Why it is v11 and first time I see it?

You actually replied to V10:
https://lore.kernel.org/linux-media/20221023085341.s23qinjuw4qls3dn@basti-XPS-13-9310/

>And why it is v11 with basic mistakes and lack of testing?!?
>I would assume that v11 was already seen and tested...

Sorry I don't have a lot of experience with dt-bindings, thank you for
highlighting the issues, I will correct them. And I forgot to build the
documentation during my testing runs.
I took over the patch set from another contributor and as no one
complained about the dt-bindings for the last 10 versions, I concentrated
my energy on other problems.

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

Thanks for the highlight.

I will move it to:
Documentation/devicetree/bindings/media/cnm,wave5.yml

>
>Subject: drop redundant pieces: yaml, devicetree and bindings.

I call it:

dt-bindings: media: chips-media: add wave5 bindings

in V12

Sincerely,
Sebastian Fricke

>
>
>>
>> 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@chipsnmedia.com>
>> +  - Robert Beckett <bob.beckett@collabora.com>
>> +  - Sebastian Fricke <sebastian.fricke@collabora.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.
>
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: phandle pointing to the SRAM device node
>
>And what is it for? Why do you need SRAM?
>
>> +    maxItems: 1
>
>Drop
>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    vpu: video-codec@12345678 {
>> +        compatible = "cnm,cm521-vpu";
>> +        reg = <0x12345678 0x1000>;
>> +        interrupts = <42>;
>> +        clocks = <&clks 42>;
>> +        clock-names = "vcodec";
>> +        sram = <&sram>;
>> +    };
>
>Best regards,
>Krzysztof
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings
  2022-12-07 15:09     ` Sebastian Fricke
@ 2022-12-07 15:27       ` Krzysztof Kozlowski
  2022-12-12 11:32         ` Sebastian Fricke
  0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2022-12-07 15:27 UTC (permalink / raw)
  To: Sebastian Fricke
  Cc: linux-media, Rob Herring, Krzysztof Kozlowski, kernel,
	bob.beckett, hverkuil-cisco, nicolas.dufresne, nas.chung,
	devicetree, linux-kernel

On 07/12/2022 16:09, Sebastian Fricke wrote:
> Hello Krzysztof, 
> 
> On 07.12.2022 13:31, Krzysztof Kozlowski wrote:
>> On 07/12/2022 13:13, Sebastian Fricke wrote:
>>> From: Robert Beckett <bob.beckett@collabora.com>
>>>
>>> Add bindings for the wave5 chips&media codec driver
>>>
>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>
>> What's happening with this patch? Where is the changelog?
> 
> The changelog is located in the cover letter.
> https://lore.kernel.org/linux-media/20221207121350.66217-1-sebastian.fricke@collabora.com/


Which you did not sent to us... so? How does it help us?

> 
>> Why it is v11 and first time I see it?
> 
> You actually replied to V10:
> https://lore.kernel.org/linux-media/20221023085341.s23qinjuw4qls3dn@basti-XPS-13-9310/
> 
>> And why it is v11 with basic mistakes and lack of testing?!?
>> I would assume that v11 was already seen and tested...
> 
> Sorry I don't have a lot of experience with dt-bindings, thank you for
> highlighting the issues, I will correct them. And I forgot to build the
> documentation during my testing runs.
> I took over the patch set from another contributor and as no one
> complained about the dt-bindings for the last 10 versions, I concentrated
> my energy on other problems.

Because they were never sent to maintainers...

> 
>>
>>
>>> ---
>>>  .../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.
> 
> Thanks for the highlight.
> 
> I will move it to:
> Documentation/devicetree/bindings/media/cnm,wave5.yml
> 
>>
>> Subject: drop redundant pieces: yaml, devicetree and bindings.
> 
> I call it:
> 
> dt-bindings: media: chips-media: add wave5 bindings
> 
> in V12
> 
> Sincerely,
> Sebastian Fricke

And the rest questions? Lack of response means agreement, which is fine,
so in v12 questionable parts will be removed?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings
  2022-12-07 15:27       ` Krzysztof Kozlowski
@ 2022-12-12 11:32         ` Sebastian Fricke
  0 siblings, 0 replies; 8+ messages in thread
From: Sebastian Fricke @ 2022-12-12 11:32 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-media, Rob Herring, Krzysztof Kozlowski, kernel,
	bob.beckett, hverkuil-cisco, nicolas.dufresne, nas.chung,
	devicetree, linux-kernel

Hey Krzysztof,

On 07.12.2022 16:27, Krzysztof Kozlowski wrote:
>On 07/12/2022 16:09, Sebastian Fricke wrote:
>> Hello Krzysztof,
>>
>> On 07.12.2022 13:31, Krzysztof Kozlowski wrote:
>>> On 07/12/2022 13:13, Sebastian Fricke wrote:
>>>> From: Robert Beckett <bob.beckett@collabora.com>
>>>>
>>>> Add bindings for the wave5 chips&media codec driver
>>>>
>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>>>
>>> What's happening with this patch? Where is the changelog?
>>
>> The changelog is located in the cover letter.
>> https://lore.kernel.org/linux-media/20221207121350.66217-1-sebastian.fricke@collabora.com/
>
>
>Which you did not sent to us... so? How does it help us?

I completely agree, I simply forgot to add the devicetree@vger.kernel.org mail to the list of receivers.

>
>>
>>> Why it is v11 and first time I see it?
>>
>> You actually replied to V10:
>> https://lore.kernel.org/linux-media/20221023085341.s23qinjuw4qls3dn@basti-XPS-13-9310/
>>
>>> And why it is v11 with basic mistakes and lack of testing?!?
>>> I would assume that v11 was already seen and tested...
>>
>> Sorry I don't have a lot of experience with dt-bindings, thank you for
>> highlighting the issues, I will correct them. And I forgot to build the
>> documentation during my testing runs.
>> I took over the patch set from another contributor and as no one
>> complained about the dt-bindings for the last 10 versions, I concentrated
>> my energy on other problems.
>
>Because they were never sent to maintainers...
>
>>
>>>
>>>
>>>> ---
>>>>  .../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.
>>
>> Thanks for the highlight.
>>
>> I will move it to:
>> Documentation/devicetree/bindings/media/cnm,wave5.yml
>>
>>>
>>> Subject: drop redundant pieces: yaml, devicetree and bindings.
>>
>> I call it:
>>
>> dt-bindings: media: chips-media: add wave5 bindings
>>
>> in V12
>>
>And the rest questions? Lack of response means agreement, which is fine,
>so in v12 questionable parts will be removed?

Yes, I will completely rework this part, thus I try to take all of your
highlights into consideration.

>
>Best regards,
>Krzysztof

Sincerely,
Sebastian Fricke

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings
  2022-12-07 12:31   ` Krzysztof Kozlowski
  2022-12-07 13:17     ` Krzysztof Kozlowski
  2022-12-07 15:09     ` Sebastian Fricke
@ 2023-09-04  6:25     ` Sebastian Fricke
  2023-09-04  7:53       ` Krzysztof Kozlowski
  2 siblings, 1 reply; 8+ messages in thread
From: Sebastian Fricke @ 2023-09-04  6:25 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-media, Rob Herring, kernel, bob.beckett, hverkuil-cisco,
	nicolas.dufresne, nas.chung, devicetree, linux-kernel

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@collabora.com>
>>
>> Add bindings for the wave5 chips&media codec driver
>>
>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.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@chipsnmedia.com>
>> +  - Robert Beckett <bob.beckett@collabora.com>
>> +  - Sebastian Fricke <sebastian.fricke@collabora.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@12345678 {
>> +        compatible = "cnm,cm521-vpu";
>> +        reg = <0x12345678 0x1000>;
>> +        interrupts = <42>;
>> +        clocks = <&clks 42>;
>> +        clock-names = "vcodec";
>> +        sram = <&sram>;
>> +    };
>
>Best regards,
>Krzysztof
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings
  2023-09-04  6:25     ` Sebastian Fricke
@ 2023-09-04  7:53       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2023-09-04  7:53 UTC (permalink / raw)
  To: Sebastian Fricke
  Cc: linux-media, Rob Herring, kernel, bob.beckett, hverkuil-cisco,
	nicolas.dufresne, nas.chung, devicetree, linux-kernel

On 04/09/2023 08:25, Sebastian Fricke wrote:

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

I meant vendor prefix of this device. It does not matter what SoC is
that, however it turns out it is already a generic property, so no
vendor prefix is needed if you use the same property - phandle points to
a node which is a sram.yaml.


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-09-04  7:53 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20221207121350.66217-1-sebastian.fricke@collabora.com>
2022-12-07 12:13 ` [PATCH v11 5/6] dt-bindings: media: wave5: add yaml devicetree bindings Sebastian Fricke
2022-12-07 12:31   ` Krzysztof Kozlowski
2022-12-07 13:17     ` Krzysztof Kozlowski
2022-12-07 15:09     ` Sebastian Fricke
2022-12-07 15:27       ` Krzysztof Kozlowski
2022-12-12 11:32         ` Sebastian Fricke
2023-09-04  6:25     ` Sebastian Fricke
2023-09-04  7:53       ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).