1. Patchew
  2. linux
  3. [PATCH 0/2] Adjust Stream Name and DT Bindings Updates
  4. View patch

[PATCH 2/2] ASoC: dt-bindings: microchip,sama7g5-i2smcc: Add 'sound-name-prefix' property

Andrei Simion posted 2 patches 2 months, 3 weeks ago
[PATCH 2/2] ASoC: dt-bindings: microchip,sama7g5-i2smcc: Add 'sound-name-prefix' property
Posted by Andrei Simion 2 months, 3 weeks ago 
From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Add 'sound-name-prefix' property to differentiate between interfaces in
DPCM use-cases. Property is optional.

[andrei.simion@microchip.com: Adjust the commit title and message.
Reword the description for 'sound-name-prefix'.]

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
---
 .../bindings/sound/microchip,sama7g5-i2smcc.yaml           | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
index fb630a184350..ad34df67c7c0 100644
--- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
+++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
@@ -52,6 +52,13 @@ properties:
       - const: gclk
     minItems: 1
 
+  sound-name-prefix:
+    pattern: "^I2SMCC[0-9]$"
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      Unique prefixes for the sink/source names of the component, ensuring
+      distinct identification among multiple instances.
+
   dmas:
     items:
       - description: TX DMA Channel
-- 
2.34.1
Re: [PATCH 2/2] ASoC: dt-bindings: microchip,sama7g5-i2smcc: Add 'sound-name-prefix' property
Posted by Krzysztof Kozlowski 2 months, 3 weeks ago 
On 09/09/2024 10:35, Andrei Simion wrote:
> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> 
> Add 'sound-name-prefix' property to differentiate between interfaces in
> DPCM use-cases. Property is optional.
> 
> [andrei.simion@microchip.com: Adjust the commit title and message.
> Reword the description for 'sound-name-prefix'.]
> 
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
> ---
>  .../bindings/sound/microchip,sama7g5-i2smcc.yaml           | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
> index fb630a184350..ad34df67c7c0 100644
> --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
> +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
> @@ -52,6 +52,13 @@ properties:
>        - const: gclk
>      minItems: 1
>  
> +  sound-name-prefix:
> +    pattern: "^I2SMCC[0-9]$"

This does not look correct. Name/prefix can be anything matching real
hardware, why are you restricting it? How can you predict all names?

> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      Unique prefixes for the sink/source names of the component, ensuring
> +      distinct identification among multiple instances.

You are duplicating property definitions. This is not needed at all.
Maybe your schema misses $ref to common schema.


Best regards,
Krzysztof
Re: [PATCH 2/2] ASoC: dt-bindings: microchip,sama7g5-i2smcc: Add 'sound-name-prefix' property
Posted by Krzysztof Kozlowski 2 months, 3 weeks ago 
On 09/09/2024 10:38, Krzysztof Kozlowski wrote:
> On 09/09/2024 10:35, Andrei Simion wrote:
>> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>>
>> Add 'sound-name-prefix' property to differentiate between interfaces in
>> DPCM use-cases. Property is optional.
>>
>> [andrei.simion@microchip.com: Adjust the commit title and message.
>> Reword the description for 'sound-name-prefix'.]
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>> ---
>>  .../bindings/sound/microchip,sama7g5-i2smcc.yaml           | 7 +++++++
>>  1 file changed, 7 insertions(+)

One more point for future (I missed this before) - be sure you CC
necessary lists. Use get_maintainers or b4 for this. Skipping DT list
means skipping automation, so this would be a NAK anyway.

Best regards,
Krzysztof
Re: [PATCH 2/2] ASoC: dt-bindings: microchip,sama7g5-i2smcc: Add 'sound-name-prefix' property
Posted by Andrei.Simion@microchip.com 2 months, 3 weeks ago 
On 10.09.2024 10:17, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 09/09/2024 10:38, Krzysztof Kozlowski wrote:
>> On 09/09/2024 10:35, Andrei Simion wrote:
>>> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>>>
>>> Add 'sound-name-prefix' property to differentiate between interfaces in
>>> DPCM use-cases. Property is optional.
>>>
>>> [andrei.simion@microchip.com: Adjust the commit title and message.
>>> Reword the description for 'sound-name-prefix'.]
>>>
>>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>>> ---
>>>  .../bindings/sound/microchip,sama7g5-i2smcc.yaml           | 7 +++++++
>>>  1 file changed, 7 insertions(+)
> 
> One more point for future (I missed this before) - be sure you CC
> necessary lists. Use get_maintainers or b4 for this. Skipping DT list
> means skipping automation, so this would be a NAK anyway.
> 

yes, I will be much more careful. thank you.

Best regards,
Andrei Simion

> Best regards,
> Krzysztof
>
Re: [PATCH 2/2] ASoC: dt-bindings: microchip,sama7g5-i2smcc: Add 'sound-name-prefix' property
Posted by Andrei.Simion@microchip.com 2 months, 3 weeks ago 
On 09.09.2024 11:38, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 09/09/2024 10:35, Andrei Simion wrote:
>> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>>
>> Add 'sound-name-prefix' property to differentiate between interfaces in
>> DPCM use-cases. Property is optional.
>>
>> [andrei.simion@microchip.com: Adjust the commit title and message.
>> Reword the description for 'sound-name-prefix'.]
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>> ---
>>  .../bindings/sound/microchip,sama7g5-i2smcc.yaml           | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
>> index fb630a184350..ad34df67c7c0 100644
>> --- a/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
>> +++ b/Documentation/devicetree/bindings/sound/microchip,sama7g5-i2smcc.yaml
>> @@ -52,6 +52,13 @@ properties:
>>        - const: gclk
>>      minItems: 1
>>
>> +  sound-name-prefix:
>> +    pattern: "^I2SMCC[0-9]$"
> 
> This does not look correct. Name/prefix can be anything matching real
> hardware, why are you restricting it? How can you predict all names?
> 
Based on the datasheet, the SoC(s) have the following naming conventions:
  - sama7g5: I2SMCC0 and I2SMCC1
  - sam9x60/sam9x75: I2SMCC

To accommodate these variations, I propose using a more relaxed pattern: "^I2SMCC(0-9)?$".
This pattern allows for both the fixed prefix and an optional single digit at the end.
What are your thoughts on this approach?


>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description:
>> +      Unique prefixes for the sink/source names of the component, ensuring
>> +      distinct identification among multiple instances.
> 
> You are duplicating property definitions. This is not needed at all.
> Maybe your schema misses $ref to common schema.
> 

I understand the concern about duplicating property definitions.
In the current file, I have referenced `dai-common` as shown here:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/sound/microchip%2Csama7g5-i2smcc.yaml#L74C1-L75C27

Could you please confirm if this reference is correctly implemented,
or suggest any adjustments needed to align with the common schema?


Best Regards,
Andrei Simion

> 
> Best regards,
> Krzysztof
>
Re: [PATCH 2/2] ASoC: dt-bindings: microchip,sama7g5-i2smcc: Add 'sound-name-prefix' property
Posted by Krzysztof Kozlowski 2 months, 3 weeks ago 
On 09/09/2024 14:28, Andrei.Simion@microchip.com wrote:
>>>      minItems: 1
>>>
>>> +  sound-name-prefix:
>>> +    pattern: "^I2SMCC[0-9]$"
>>
>> This does not look correct. Name/prefix can be anything matching real
>> hardware, why are you restricting it? How can you predict all names?
>>
> Based on the datasheet, the SoC(s) have the following naming conventions:
>   - sama7g5: I2SMCC0 and I2SMCC1
>   - sam9x60/sam9x75: I2SMCC
> 
> To accommodate these variations, I propose using a more relaxed pattern: "^I2SMCC(0-9)?$".
> This pattern allows for both the fixed prefix and an optional single digit at the end.
> What are your thoughts on this approach?

I understand this does not differ per board, because it is component of
the SoC, yet still I do not see any value in enforcing name.

> 
> 
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +    description:
>>> +      Unique prefixes for the sink/source names of the component, ensuring
>>> +      distinct identification among multiple instances.
>>
>> You are duplicating property definitions. This is not needed at all.
>> Maybe your schema misses $ref to common schema.
>>
> 
> I understand the concern about duplicating property definitions.
> In the current file, I have referenced `dai-common` as shown here:
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/sound/microchip%2Csama7g5-i2smcc.yaml#L74C1-L75C27
> 
> Could you please confirm if this reference is correctly implemented,

Yeah, the dai-common $ref is correct, so you should not need it. Do you
see any warning?

> or suggest any adjustments needed to align with the common schema?

I claim nothing has to be done and entire patch is redundant or not much
helpful. Your commit msg did not explain *why* this is needed and what
problem you are fixing, so what I can say? I don't know why should be
aligned because I do not understand the problem being fixed.

Best regards,
Krzysztof

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.