[1/3] dt-bindings: rockchip: Add rk809 support for rk817 audio codec

  Rockchip RK809 shares the same audio codec as the rk817 mfd, it is also
using the same rk817_codec driver. However it is missing from the
bindings.

Update dt-binding documentation for rk809 to include the audio codec
properties. This fixes the following warning from dtb check:

pmic@20: '#sound-dai-cells', 'assigned-clock-parents', 'assigned-clocks',
   'clock-names', 'clocks', 'codec' do not match any of the regexes:
   'pinctrl-[0-9]+'

Signed-off-by: Tim Lunn <tim@feathertop.org>
---

 .../bindings/mfd/rockchip,rk809.yaml          | 30 ++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)
  

On Tue, Jan 16, 2024 at 07:46:16PM +1100, Tim Lunn wrote:
> Rockchip RK809 shares the same audio codec as the rk817 mfd, it is also
> using the same rk817_codec driver. However it is missing from the
> bindings.
> 
> Update dt-binding documentation for rk809 to include the audio codec
> properties. This fixes the following warning from dtb check:
> 
> pmic@20: '#sound-dai-cells', 'assigned-clock-parents', 'assigned-clocks',
>    'clock-names', 'clocks', 'codec' do not match any of the regexes:
>    'pinctrl-[0-9]+'
> 
> Signed-off-by: Tim Lunn <tim@feathertop.org>
> ---
> 
>  .../bindings/mfd/rockchip,rk809.yaml          | 30 ++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml
> index 839c0521f1e5..bac2e751e2f2 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.

How many clocks? (maxItems: 1)

You can drop the description.

> +
> +  clock-names:
> +    description:
> +      The clock name for the codec clock.

Drop.

> +    items:
> +      - const: mclk
> +
> +  '#sound-dai-cells':
> +    description:
> +      Needed for the interpretation of sound dais.

Common property, don't need the description.


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

Why do you need a child node here? Just put the 1 property in the parent 
node.

> +    type: object
> +    additionalProperties: false
> +    properties:
> +      rockchip,mic-in-differential:
> +        type: boolean
> +        description:
> +          Describes if the microphone uses differential mode.
> +
>  allOf:
>    - if:
>        properties:
> -- 
> 2.40.1
>
  

Hi Rob,

On 1/17/24 06:37, Rob Herring wrote:
> On Tue, Jan 16, 2024 at 07:46:16PM +1100, Tim Lunn wrote:
>> Rockchip RK809 shares the same audio codec as the rk817 mfd, it is also
>> using the same rk817_codec driver. However it is missing from the
>> bindings.
>>
>> Update dt-binding documentation for rk809 to include the audio codec
>> properties. This fixes the following warning from dtb check:
>>
>> pmic@20: '#sound-dai-cells', 'assigned-clock-parents', 'assigned-clocks',
>>     'clock-names', 'clocks', 'codec' do not match any of the regexes:
>>     'pinctrl-[0-9]+'
>>
>> Signed-off-by: Tim Lunn <tim@feathertop.org>
>> ---
>>
>>   .../bindings/mfd/rockchip,rk809.yaml          | 30 ++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk809.yaml
>> index 839c0521f1e5..bac2e751e2f2 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.
> How many clocks? (maxItems: 1)
>
> You can drop the description.
Yes just 1 clock, i will fix this.
>
>> +
>> +  clock-names:
>> +    description:
>> +      The clock name for the codec clock.
> Drop.
Just drop the description? I dont think can drop the clock names as the 
driver use the name to lookup clock:

devm_clk_get(pdev->dev.parent, "mclk");
>
>> +    items:
>> +      - const: mclk
>> +
>> +  '#sound-dai-cells':
>> +    description:
>> +      Needed for the interpretation of sound dais.
> Common property, don't need the description.
Ok
>
>> +    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.
> Why do you need a child node here? Just put the 1 property in the parent
> node.
This is how the existing rk817 codec driver was setup. I suppose it was 
copied from downstream, where there are more properties than just the 
one. I don't know if there was any intention (or need) to implement 
those other properties.
>
>> +    type: object
>> +    additionalProperties: false
>> +    properties:
>> +      rockchip,mic-in-differential:
>> +        type: boolean
>> +        description:
>> +          Describes if the microphone uses differential mode.
>> +
>>   allOf:
>>     - if:
>>         properties:
>> -- 
>> 2.40.1
>>
  

On 17/01/2024 10:37, Tim Lunn wrote:
>> You can drop the description.
> Yes just 1 clock, i will fix this.
>>
>>> +
>>> +  clock-names:
>>> +    description:
>>> +      The clock name for the codec clock.
>> Drop.
> Just drop the description? I dont think can drop the clock names as the 
> driver use the name to lookup clock:

Description. But anyway the problem is that adding clocks should be
separate patch with its own explanation.


> 
> devm_clk_get(pdev->dev.parent, "mclk");
>>
>>> +    items:
>>> +      - const: mclk
>>> +
>>> +  '#sound-dai-cells':
>>> +    description:
>>> +      Needed for the interpretation of sound dais.
>> Common property, don't need the description.
> Ok
>>
>>> +    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.
>> Why do you need a child node here? Just put the 1 property in the parent
>> node.
> This is how the existing rk817 codec driver was setup. I suppose it was 
> copied from downstream, where there are more properties than just the 
> one. I don't know if there was any intention (or need) to implement 
> those other properties.
>>

You need to clearly express ABI requirements in the commit msg.
Otherwise you will get a review like for new bindings.

Best regards,
Krzysztof
  

On 1/17/24 21:12, Krzysztof Kozlowski wrote:
> On 17/01/2024 10:37, Tim Lunn wrote:
>>> You can drop the description.
>> Yes just 1 clock, i will fix this.
>>>> +
>>>> +  clock-names:
>>>> +    description:
>>>> +      The clock name for the codec clock.
>>> Drop.
>> Just drop the description? I dont think can drop the clock names as the
>> driver use the name to lookup clock:
> Description. But anyway the problem is that adding clocks should be
> separate patch with its own explanation.
>
Right, but I am not actually adding any clocks, just documenting what is 
already there.
There are already boards using this codec with rk809 in dts files and is 
working fine from driver side.
>
>
>> devm_clk_get(pdev->dev.parent, "mclk");
>>>> +    items:
>>>> +      - const: mclk
>>>> +
>>>> +  '#sound-dai-cells':
>>>> +    description:
>>>> +      Needed for the interpretation of sound dais.
>>> Common property, don't need the description.
>> Ok
>>>> +    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.
>>> Why do you need a child node here? Just put the 1 property in the parent
>>> node.
>> This is how the existing rk817 codec driver was setup. I suppose it was
>> copied from downstream, where there are more properties than just the
>> one. I don't know if there was any intention (or need) to implement
>> those other properties.
> You need to clearly express ABI requirements in the commit msg.
> Otherwise you will get a review like for new bindings.
Got it, I will clarify this and future commit messages

Regards
    Tim
>
> Best regards,
> Krzysztof
>
  

On 17/01/2024 11:38, Tim Lunn wrote:
> 
> On 1/17/24 21:12, Krzysztof Kozlowski wrote:
>> On 17/01/2024 10:37, Tim Lunn wrote:
>>>> You can drop the description.
>>> Yes just 1 clock, i will fix this.
>>>>> +
>>>>> +  clock-names:
>>>>> +    description:
>>>>> +      The clock name for the codec clock.
>>>> Drop.
>>> Just drop the description? I dont think can drop the clock names as the
>>> driver use the name to lookup clock:
>> Description. But anyway the problem is that adding clocks should be
>> separate patch with its own explanation.
>>
> Right, but I am not actually adding any clocks, just documenting what is 
> already there.

You are. Binding did not have any clocks, now it has.

> There are already boards using this codec with rk809 in dts files and is 
> working fine from driver side.



Best regards,
Krzysztof
  

On 1/17/24 21:57, Krzysztof Kozlowski wrote:
> On 17/01/2024 11:38, Tim Lunn wrote:
>> On 1/17/24 21:12, Krzysztof Kozlowski wrote:
>>> On 17/01/2024 10:37, Tim Lunn wrote:
>>>>> You can drop the description.
>>>> Yes just 1 clock, i will fix this.
>>>>>> +
>>>>>> +  clock-names:
>>>>>> +    description:
>>>>>> +      The clock name for the codec clock.
>>>>> Drop.
>>>> Just drop the description? I dont think can drop the clock names as the
>>>> driver use the name to lookup clock:
>>> Description. But anyway the problem is that adding clocks should be
>>> separate patch with its own explanation.
>>>
>> Right, but I am not actually adding any clocks, just documenting what is
>> already there.
> You are. Binding did not have any clocks, now it has.
Ok, I will split the clocks into a separate patch.
>
>> There are already boards using this codec with rk809 in dts files and is
>> working fine from driver side.
>
>
> Best regards,
> Krzysztof
>