| Message ID | 20230915-wave5_v12_on_media_master-v12-6-92fc66cd685d@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Wave5 codec driver | expand |
On Fri, 15 Sep 2023 23:11:35 +0200, 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> > --- > .../devicetree/bindings/media/cnm,wave5.yaml | 66 ++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/media/cnm,wave5.yaml:19:9: [warning] wrong indentation: expected 6 but found 8 (indentation) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230915-wave5_v12_on_media_master-v12-6-92fc66cd685d@collabora.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On 15/09/2023 23:11, 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> So this is v12 and still no tested? A nit, subject: drop second/last, redundant "yaml devicetree indings". The "dt-bindings" prefix is already stating that these are bindings. Basically three words bringing zero information. > --- > .../devicetree/bindings/media/cnm,wave5.yaml | 66 ++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/cnm,wave5.yaml b/Documentation/devicetree/bindings/media/cnm,wave5.yaml > new file mode 100644 > index 000000000000..b8f383621805 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/cnm,wave5.yaml > @@ -0,0 +1,66 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/cnm,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> > + - Jackson Lee <jackson.lee@chipsnmedia.com> > + > +description: |- Do not need '|-' unless you need to preserve formatting. > + The Chips&Media WAVE codec IP is a multi format video encoder/decoder > + > +properties: > + compatible: > + enum: > + - cnm,cm521c-vpu It does not look like you tested the bindings, at least after quick look. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. Missing blank line > + reg: > + maxItems: 1 > + > + clocks: > + items: > + - description: VCODEC clock > + > + clock-names: > + items: > + - const: vcodec Drop clock-names, not really useful for one entry. > + > + interrupts: > + maxItems: 1 > + > + power-domains: > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > + sram: > + $ref: /schemas/types.yaml#/definitions/phandle > + Drop blank line > + description: > + 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. > + > +required: > + - compatible > + - reg > + - interrupts Keep the same order as listed in properties: > + - clocks > + - clock-names > + > +additionalProperties: false > + Best regards, Krzysztof
Hey Krzysztof, thanks for your review. On 17.09.2023 09:56, Krzysztof Kozlowski wrote: >On 15/09/2023 23:11, 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> > >So this is v12 and still no tested? I have tested it, multiple times actually since V11. (For some reason that indentation issue slipped by me though ...) If you mean the tested by tag, the patch was completely unnoticed until v10 by the community, which was partially because me and the previous commiters didn't use the right recipients for this patch. So from that point of view this is more like v2. > >A nit, subject: drop second/last, redundant "yaml devicetree indings". >The "dt-bindings" prefix is already stating that these are bindings. >Basically three words bringing zero information. Okay so: `dt-bindings: media: wave5: add devicetree` ? > >> --- >> .../devicetree/bindings/media/cnm,wave5.yaml | 66 ++++++++++++++++++++++ >> 1 file changed, 66 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/media/cnm,wave5.yaml b/Documentation/devicetree/bindings/media/cnm,wave5.yaml >> new file mode 100644 >> index 000000000000..b8f383621805 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/cnm,wave5.yaml >> @@ -0,0 +1,66 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/cnm,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> >> + - Jackson Lee <jackson.lee@chipsnmedia.com> >> + >> +description: |- > >Do not need '|-' unless you need to preserve formatting. Ack. > >> + The Chips&Media WAVE codec IP is a multi format video encoder/decoder >> + >> +properties: >> + compatible: >> + enum: >> + - cnm,cm521c-vpu > >It does not look like you tested the bindings, at least after quick >look. Please run `make dt_binding_check` (see >Documentation/devicetree/bindings/writing-schema.rst for instructions). >Maybe you need to update your dtschema and yamllint. Here my testing output: ``` ❯ make dt_binding_check DT_SCHEMA_FILES=cnm,wave5.yaml HOSTCC scripts/basic/fixdep HOSTCC scripts/dtc/dtc.o HOSTCC scripts/dtc/flattree.o HOSTCC scripts/dtc/fstree.o HOSTCC scripts/dtc/data.o HOSTCC scripts/dtc/livetree.o HOSTCC scripts/dtc/treesource.o HOSTCC scripts/dtc/srcpos.o HOSTCC scripts/dtc/checks.o HOSTCC scripts/dtc/util.o LEX scripts/dtc/dtc-lexer.lex.c YACC scripts/dtc/dtc-parser.tab.[ch] HOSTCC scripts/dtc/dtc-lexer.lex.o HOSTCC scripts/dtc/dtc-parser.tab.o HOSTLD scripts/dtc/dtc LINT Documentation/devicetree/bindings ./Documentation/devicetree/bindings/media/cnm,wave5.yaml:19:9: [warning] wrong indentation: expected 6 but found 8 (indentation) CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTEX Documentation/devicetree/bindings/media/cnm,wave5.example.dts DTC_CHK Documentation/devicetree/bindings/media/cnm,wave5.example.dtb ``` Again sorry about missing the indentation warning, but nothing else was highlighted. Both dtschema and yamllint seem to be up-to-date: ``` ❯ python3 -m pip --version pip 23.2.1 from /home/basti/.local/lib/python3.8/site-packages/pip (python 3.8) ❯ pip3 show dtschema Name: dtschema Version: 2023.7 Summary: DeviceTree validation schema and tools Home-page: https://github.com/devicetree-org/dt-schema Author: Rob Herring Author-email: robh@kernel.org License: BSD Location: /home/basti/.local/lib/python3.8/site-packages Requires: jsonschema, pylibfdt, rfc3987, ruamel.yaml Required-by: ❯ pip3 show yamllint Name: yamllint Version: 1.32.0 Summary: A linter for YAML files. Home-page: Author: Adrien Vergé Author-email: License: GPL-3.0-only Location: /home/basti/.local/lib/python3.8/site-packages Requires: pathspec, pyyaml Required-by: ``` > >Missing blank line Ack, will add that. > >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + items: >> + - description: VCODEC clock >> + >> + clock-names: >> + items: >> + - const: vcodec > >Drop clock-names, not really useful for one entry. Ack > >> + >> + interrupts: >> + maxItems: 1 >> + >> + power-domains: >> + maxItems: 1 >> + >> + resets: >> + maxItems: 1 >> + >> + sram: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + > >Drop blank line Ack > >> + description: >> + 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. >> + >> +required: >> + - compatible >> + - reg >> + - interrupts > >Keep the same order as listed in properties: Ack > >> + - clocks >> + - clock-names >> + >> +additionalProperties: false >> + > >Best regards, >Krzysztof Sincerely, Sebastian > >_______________________________________________ >Kernel mailing list -- kernel@mailman.collabora.com >To unsubscribe send an email to kernel-leave@mailman.collabora.com
On 18/09/2023 08:49, Sebastian Fricke wrote: > Hey Krzysztof, > > thanks for your review. > > On 17.09.2023 09:56, Krzysztof Kozlowski wrote: >> On 15/09/2023 23:11, 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> >> >> So this is v12 and still no tested? > > I have tested it, multiple times actually since V11. (For some reason > that indentation issue slipped by me though ...) > If you mean the tested by tag, the patch was completely unnoticed until > v10 by the community, which was partially because me and the previous > commiters didn't use the right recipients for this patch. So from that > point of view this is more like v2. > >> >> A nit, subject: drop second/last, redundant "yaml devicetree indings". >> The "dt-bindings" prefix is already stating that these are bindings. >> Basically three words bringing zero information. > > Okay so: > `dt-bindings: media: wave5: add devicetree` Still not, because devicetree is duplicating "dt". It's redundant. Instead should be (with correct order of prefixes): media: dt-bindings: wave5: add AzureWaveFooBar XYL ABC10 (whatever company and full product name it is) Best regards, Krzysztof
Le lundi 18 septembre 2023 à 14:02 +0200, Krzysztof Kozlowski a écrit : > On 18/09/2023 08:49, Sebastian Fricke wrote: > > Hey Krzysztof, > > > > thanks for your review. > > > > On 17.09.2023 09:56, Krzysztof Kozlowski wrote: > > > On 15/09/2023 23:11, 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> > > > > > > So this is v12 and still no tested? > > > > I have tested it, multiple times actually since V11. (For some reason > > that indentation issue slipped by me though ...) > > If you mean the tested by tag, the patch was completely unnoticed until > > v10 by the community, which was partially because me and the previous > > commiters didn't use the right recipients for this patch. So from that > > point of view this is more like v2. > > > > > > > > A nit, subject: drop second/last, redundant "yaml devicetree indings". > > > The "dt-bindings" prefix is already stating that these are bindings. > > > Basically three words bringing zero information. > > > > Okay so: > > `dt-bindings: media: wave5: add devicetree` > > Still not, because devicetree is duplicating "dt". It's redundant. > > Instead should be (with correct order of prefixes): > > media: dt-bindings: wave5: add AzureWaveFooBar XYL ABC10 (whatever > company and full product name it is) So maybe this one ? media: dt-bindings: wave5: add Chips&Media 521c codec IP support > > > Best regards, > Krzysztof >
On 18/09/2023 21:16, Nicolas Dufresne wrote: >>>> >>>> A nit, subject: drop second/last, redundant "yaml devicetree indings". >>>> The "dt-bindings" prefix is already stating that these are bindings. >>>> Basically three words bringing zero information. >>> >>> Okay so: >>> `dt-bindings: media: wave5: add devicetree` >> >> Still not, because devicetree is duplicating "dt". It's redundant. >> >> Instead should be (with correct order of prefixes): >> >> media: dt-bindings: wave5: add AzureWaveFooBar XYL ABC10 (whatever >> company and full product name it is) > > So maybe this one ? > > media: dt-bindings: wave5: add Chips&Media 521c codec IP support Sure, sounds good for me. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/media/cnm,wave5.yaml b/Documentation/devicetree/bindings/media/cnm,wave5.yaml new file mode 100644 index 000000000000..b8f383621805 --- /dev/null +++ b/Documentation/devicetree/bindings/media/cnm,wave5.yaml @@ -0,0 +1,66 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/cnm,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> + - Jackson Lee <jackson.lee@chipsnmedia.com> + +description: |- + The Chips&Media WAVE codec IP is a multi format video encoder/decoder + +properties: + compatible: + enum: + - cnm,cm521c-vpu + reg: + maxItems: 1 + + clocks: + items: + - description: VCODEC clock + + clock-names: + items: + - const: vcodec + + interrupts: + maxItems: 1 + + power-domains: + maxItems: 1 + + resets: + maxItems: 1 + + sram: + $ref: /schemas/types.yaml#/definitions/phandle + + description: + 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. + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + vpu: video-codec@12345678 { + compatible = "cnm,cm521c-vpu"; + reg = <0x12345678 0x1000>; + interrupts = <42>; + clocks = <&clks 42>; + clock-names = "vcodec"; + sram = <&sram>; + };