diff mbox series

[2/4] dt-bindings: mfd: rk809: Add audio codec properties

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

Commit Message

Jonas Karlman May 5, 2024, 1:41 p.m. UTC
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(-)

Comments

Rob Herring (Arm) May 5, 2024, 2:21 p.m. UTC | #1
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.
Krzysztof Kozlowski May 6, 2024, 10:47 a.m. UTC | #2
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
Jonas Karlman May 6, 2024, 4:14 p.m. UTC | #3
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
>
Krzysztof Kozlowski May 6, 2024, 8:08 p.m. UTC | #4
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 mbox series

Patch

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;
+            };
         };
     };