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

[Sebastian Fricke]

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@xxxxxxxxxxxxx>
> >
> > Add bindings for the wave5 chips&media codec driver
> >
> > Signed-off-by: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> > Signed-off-by: Sebastian Fricke <sebastian.fricke@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx/

> 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@xxxxxxxxxxxxxxx>
> > +  - Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
> > +  - Sebastian Fricke <sebastian.fricke@xxxxxxxxxxxxx>
> > +
> > +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