| Message ID | 20240505134120.2828885-3-jonas@kwiboo.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: rockchip: Add Radxa ROCK 3B | expand |
On Sun, 05 May 2024 13:41:12 +0000, Jonas Karlman wrote: > Similar to RK817 the RK809 also integrates a complete audio system. > > Add audio codec properties to binding to reflect this. > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > --- > .../bindings/mfd/rockchip,rk809.yaml | 34 ++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletion(-) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/rockchip,rk809.example.dtb: pmic@1b: 'codec' does not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/mfd/rockchip,rk808.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240505134120.2828885-3-jonas@kwiboo.se 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 05/05/2024 15:41, Jonas Karlman wrote: > Similar to RK817 the RK809 also integrates a complete audio system. > > Add audio codec properties to binding to reflect this. > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> Except sending untested patches... > --- > .../bindings/mfd/rockchip,rk809.yaml | 34 ++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml > index c951056b8b4d..b78e1b090105 100644 > --- a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml > +++ b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml > @@ -12,7 +12,7 @@ maintainers: > > description: | > Rockchip RK809 series PMIC. This device consists of an i2c controlled MFD > - that includes regulators, an RTC, and power button. > + that includes regulators, an RTC, a power button and an audio codec. > > properties: > compatible: > @@ -93,6 +93,34 @@ properties: > unevaluatedProperties: false > unevaluatedProperties: false > > + clocks: > + description: > + The input clock for the audio codec. No, this allows anything. You must be here specific, see example-schema. maxItems: 1 Drop description, redundant. > + > + clock-names: > + description: > + The clock name for the codec clock. Drop description, redundant. > + items: > + - const: mclk > + > + '#sound-dai-cells': > + description: > + Needed for the interpretation of sound dais. Drop description, redundant. Do you see it anywhere for such properties? > + const: 0 Missing ref to dai-common in your allOf (again: take a look how other bindings are doing it). > + > + codec: > + description: | Do not need '|' unless you need to preserve formatting. > + The child node for the codec to hold additional properties. If no > + additional properties are required for the codec, this node can be > + omitted. That's useless description. Describe hardware, not syntax. This must say what this node represents. Anyway drop it. You do not have any resources there, so put properties in top level. > + type: object > + additionalProperties: false > + properties: > + rockchip,mic-in-differential: > + type: boolean > + description: > + Describes if the microphone uses differential mode. Your description copies property name. Do not describe the syntax "Description describes", but say what is it. > + > allOf: > - if: > properties: > @@ -284,5 +312,9 @@ examples: > }; > }; > }; > + > + rk809_codec: codec { > + rockchip,mic-in-differential; Missing all other properties. Make your example complete. Best regards, Krzysztof
Hi Krzysztof, On 2024-05-06 12:47, Krzysztof Kozlowski wrote: > On 05/05/2024 15:41, Jonas Karlman wrote: >> Similar to RK817 the RK809 also integrates a complete audio system. >> >> Add audio codec properties to binding to reflect this. >> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > > Except sending untested patches... This patch was a 1:1 copy from rockchip,rk817.yaml so I expected everything to already be correct, my bad. Guess rockchip,rk817.yaml also needs same fixes/changes as listed below. Will send a v2 with example fixed in a separate patch and try to fix your remarks on this patch in v2. > >> --- >> .../bindings/mfd/rockchip,rk809.yaml | 34 ++++++++++++++++++- >> 1 file changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml >> index c951056b8b4d..b78e1b090105 100644 >> --- a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml >> +++ b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml >> @@ -12,7 +12,7 @@ maintainers: >> >> description: | >> Rockchip RK809 series PMIC. This device consists of an i2c controlled MFD >> - that includes regulators, an RTC, and power button. >> + that includes regulators, an RTC, a power button and an audio codec. >> >> properties: >> compatible: >> @@ -93,6 +93,34 @@ properties: >> unevaluatedProperties: false >> unevaluatedProperties: false >> >> + clocks: >> + description: >> + The input clock for the audio codec. > > No, this allows anything. You must be here specific, see example-schema. > maxItems: 1 > > Drop description, redundant. > >> + >> + clock-names: >> + description: >> + The clock name for the codec clock. > > Drop description, redundant. > >> + items: >> + - const: mclk >> + >> + '#sound-dai-cells': >> + description: >> + Needed for the interpretation of sound dais. > > Drop description, redundant. Do you see it anywhere for such properties? > >> + const: 0 > > > Missing ref to dai-common in your allOf (again: take a look how other > bindings are doing it). > > >> + >> + codec: >> + description: | > > Do not need '|' unless you need to preserve formatting. > >> + The child node for the codec to hold additional properties. If no >> + additional properties are required for the codec, this node can be >> + omitted. > > That's useless description. Describe hardware, not syntax. This must say > what this node represents. > > Anyway drop it. You do not have any resources there, so put properties > in top level. This just tries to follow the rockchip,rk817 binding, not fully sure about the reasoning behind this node in the the rk817 binding. RK809/RK817 are very similar and their schema files could possible be merged. > > >> + type: object >> + additionalProperties: false >> + properties: >> + rockchip,mic-in-differential: >> + type: boolean >> + description: >> + Describes if the microphone uses differential mode. > > Your description copies property name. Do not describe the syntax > "Description describes", but say what is it. > >> + >> allOf: >> - if: >> properties: >> @@ -284,5 +312,9 @@ examples: >> }; >> }; >> }; >> + >> + rk809_codec: codec { >> + rockchip,mic-in-differential; > > Missing all other properties. Make your example complete. Noticed that the example used in this schema file is for RK808 and not RK809 so will also add a patch that replaces/fixes the example in v2. Regards, Jonas > > Best regards, > Krzysztof >
On 06/05/2024 18:14, Jonas Karlman wrote: >> >>> + >>> + codec: >>> + description: | >> >> Do not need '|' unless you need to preserve formatting. >> >>> + The child node for the codec to hold additional properties. If no >>> + additional properties are required for the codec, this node can be >>> + omitted. >> >> That's useless description. Describe hardware, not syntax. This must say >> what this node represents. >> >> Anyway drop it. You do not have any resources there, so put properties >> in top level. > > This just tries to follow the rockchip,rk817 binding, not fully sure > about the reasoning behind this node in the the rk817 binding. > > RK809/RK817 are very similar and their schema files could possible be > merged. That binding was a conversion from something older, so it might not be in good shape. At least new binding should follow usual rules/style. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml index c951056b8b4d..b78e1b090105 100644 --- a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml +++ b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml @@ -12,7 +12,7 @@ maintainers: description: | Rockchip RK809 series PMIC. This device consists of an i2c controlled MFD - that includes regulators, an RTC, and power button. + that includes regulators, an RTC, a power button and an audio codec. properties: compatible: @@ -93,6 +93,34 @@ properties: unevaluatedProperties: false unevaluatedProperties: false + clocks: + description: + The input clock for the audio codec. + + clock-names: + description: + The clock name for the codec clock. + items: + - const: mclk + + '#sound-dai-cells': + description: + Needed for the interpretation of sound dais. + const: 0 + + codec: + description: | + The child node for the codec to hold additional properties. If no + additional properties are required for the codec, this node can be + omitted. + type: object + additionalProperties: false + properties: + rockchip,mic-in-differential: + type: boolean + description: + Describes if the microphone uses differential mode. + allOf: - if: properties: @@ -284,5 +312,9 @@ examples: }; }; }; + + rk809_codec: codec { + rockchip,mic-in-differential; + }; }; };
Similar to RK817 the RK809 also integrates a complete audio system. Add audio codec properties to binding to reflect this. Signed-off-by: Jonas Karlman <jonas@kwiboo.se> --- .../bindings/mfd/rockchip,rk809.yaml | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)