linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/2] ASoC: meson: implement link-name optional property in meson audio card
@ 2024-04-08 16:49 Dmitry Rokosov
  2024-04-08 16:49 ` [PATCH v1 1/2] ASoC: dt-bindings: meson: introduce link-name optional property Dmitry Rokosov
  2024-04-08 16:49 ` [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils Dmitry Rokosov
  0 siblings, 2 replies; 11+ messages in thread
From: Dmitry Rokosov @ 2024-04-08 16:49 UTC (permalink / raw)
  To: neil.armstrong, lgirdwood, jbrunet, broonie, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl
  Cc: kernel, rockosov, linux-amlogic, alsa-devel, linux-sound,
	devicetree, linux-kernel, linux-arm-kernel, Dmitry Rokosov

The 'link-name' is optional feature in the Device Tree that allows users
to customize the name associated with the DAI link and PCM stream.  This
feature provides enhanced flexibility in DAI naming conventions, leading
to improved system integration and a better user experience.

Dmitry Rokosov (2):
  ASoC: dt-bindings: meson: introduce link-name optional property
  ASoC: meson: implement link-name optional property in meson card utils

 .../bindings/sound/amlogic,axg-sound-card.yaml       |  6 ++++++
 .../bindings/sound/amlogic,gx-sound-card.yaml        |  6 ++++++
 sound/soc/meson/meson-card-utils.c                   | 12 ++++++++----
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v1 1/2] ASoC: dt-bindings: meson: introduce link-name optional property
  2024-04-08 16:49 [PATCH v1 0/2] ASoC: meson: implement link-name optional property in meson audio card Dmitry Rokosov
@ 2024-04-08 16:49 ` Dmitry Rokosov
  2024-04-08 18:44   ` Jerome Brunet
  2024-04-08 16:49 ` [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils Dmitry Rokosov
  1 sibling, 1 reply; 11+ messages in thread
From: Dmitry Rokosov @ 2024-04-08 16:49 UTC (permalink / raw)
  To: neil.armstrong, lgirdwood, jbrunet, broonie, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl
  Cc: kernel, rockosov, linux-amlogic, alsa-devel, linux-sound,
	devicetree, linux-kernel, linux-arm-kernel, Dmitry Rokosov

The 'link-name' property is an optional DT property that allows for the
customization of the name associated with the DAI link and PCM stream.
This functionality mirrors the approach commonly utilized in Qualcomm
audio cards, providing flexibility in DAI naming conventions for
improved system integration and userspace experience.

It allows userspace program to easy determine PCM stream purpose, e.g.:
    ~ # cat /proc/asound/pcm
    00-00: speaker (*) :  : playback 1
    00-01: mics (*) :  : capture 1
    00-02: loopback (*) :  : capture 1

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 .../devicetree/bindings/sound/amlogic,axg-sound-card.yaml   | 6 ++++++
 .../devicetree/bindings/sound/amlogic,gx-sound-card.yaml    | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
index 492b41cc8ccd..46774a3e4b1d 100644
--- a/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
+++ b/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
@@ -66,6 +66,11 @@ patternProperties:
         maxItems: 1
         description: phandle of the CPU DAI
 
+      link-name:
+        description: Indicates dai-link name and PCM stream name.
+        $ref: /schemas/types.yaml#/definitions/string
+        maxItems: 1
+
     patternProperties:
       "^dai-tdm-slot-(t|r)x-mask-[0-3]$":
         $ref: /schemas/types.yaml#/definitions/uint32-array
@@ -137,6 +142,7 @@ examples:
 
         dai-link-0 {
             sound-dai = <&frddr_a>;
+            link-name = "speaker";
         };
 
         dai-link-1 {
diff --git a/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml b/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
index d4277d342e69..975c148f9712 100644
--- a/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
+++ b/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
@@ -52,6 +52,11 @@ patternProperties:
         maxItems: 1
         description: phandle of the CPU DAI
 
+      link-name:
+        description: Indicates dai-link name and PCM stream name.
+        $ref: /schemas/types.yaml#/definitions/string
+        maxItems: 1
+
     patternProperties:
       "^codec(-[0-9]+)?$":
         type: object
@@ -89,6 +94,7 @@ examples:
 
         dai-link-0 {
                sound-dai = <&i2s_fifo>;
+               link-name = "speaker";
         };
 
         dai-link-1 {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
  2024-04-08 16:49 [PATCH v1 0/2] ASoC: meson: implement link-name optional property in meson audio card Dmitry Rokosov
  2024-04-08 16:49 ` [PATCH v1 1/2] ASoC: dt-bindings: meson: introduce link-name optional property Dmitry Rokosov
@ 2024-04-08 16:49 ` Dmitry Rokosov
  2024-04-08 18:15   ` Jerome Brunet
  1 sibling, 1 reply; 11+ messages in thread
From: Dmitry Rokosov @ 2024-04-08 16:49 UTC (permalink / raw)
  To: neil.armstrong, lgirdwood, jbrunet, broonie, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl
  Cc: kernel, rockosov, linux-amlogic, alsa-devel, linux-sound,
	devicetree, linux-kernel, linux-arm-kernel, Dmitry Rokosov

The 'link-name' property presents an optional DT feature that empowers
users to customize the name associated with the DAI link and PCM stream.
This functionality reflects the approach often employed in Qualcomm
audio cards, providing enhanced flexibility in DAI naming conventions
for improved system integration and userspace experience.

It allows userspace program to easy determine PCM stream purpose, e.g.:
    ~ # cat /proc/asound/pcm
    00-00: speaker (*) :  : playback 1
    00-01: mics (*) :  : capture 1
    00-02: loopback (*) :  : capture 1

The previous naming approach using auto-generated fe or be strings
continues to be utilized as a fallback.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 sound/soc/meson/meson-card-utils.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
index ed6c7e2f609c..7bae72905a9b 100644
--- a/sound/soc/meson/meson-card-utils.c
+++ b/sound/soc/meson/meson-card-utils.c
@@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
 				    struct device_node *node,
 				    const char *prefix)
 {
-	char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
-				    prefix, node->full_name);
-	if (!name)
-		return -ENOMEM;
+	const char *name;
+
+	if (of_property_read_string(node, "link-name", &name)) {
+		name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
+				      prefix, node->full_name);
+		if (!name)
+			return -ENOMEM;
+	}
 
 	link->name = name;
 	link->stream_name = name;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
  2024-04-08 16:49 ` [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils Dmitry Rokosov
@ 2024-04-08 18:15   ` Jerome Brunet
  2024-04-08 18:40     ` Dmitry Rokosov
  2024-04-08 18:42     ` Jerome Brunet
  0 siblings, 2 replies; 11+ messages in thread
From: Jerome Brunet @ 2024-04-08 18:15 UTC (permalink / raw)
  To: Dmitry Rokosov
  Cc: neil.armstrong, lgirdwood, jbrunet, broonie, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
	linux-sound, devicetree, linux-kernel, linux-arm-kernel


On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> The 'link-name' property presents an optional DT feature that empowers
> users to customize the name associated with the DAI link and PCM stream.
> This functionality reflects the approach often employed in Qualcomm
> audio cards, providing enhanced flexibility in DAI naming conventions
> for improved system integration and userspace experience.
>
> It allows userspace program to easy determine PCM stream purpose, e.g.:
>     ~ # cat /proc/asound/pcm
>     00-00: speaker (*) :  : playback 1
>     00-01: mics (*) :  : capture 1
>     00-02: loopback (*) :  : capture 1

The example above is exactly what you should not do with link names, at
least with the amlogic audio system.

Userspace pcm, otherwise known as DPCM frontend, are merely that:
frontends. What they do is entirely defined by the routing defined by
the userspace (amixer and friends)

So naming the interface in DT (the FW describing the HW) after what the
the userspace SW could possibly set later on is wrong.

Bottom line: I have mixed feeling about this change. It could allow all
sort of bad names to be set.

The only way it could make sense HW wise is if the only allowed names
where (fr|to)ddr_[abcd], which could help maps the interface and the
kcontrol.

Such restriction should be documented in the binding doc.

>
> The previous naming approach using auto-generated fe or be strings
> continues to be utilized as a fallback.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  sound/soc/meson/meson-card-utils.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
> index ed6c7e2f609c..7bae72905a9b 100644
> --- a/sound/soc/meson/meson-card-utils.c
> +++ b/sound/soc/meson/meson-card-utils.c
> @@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
>  				    struct device_node *node,
>  				    const char *prefix)
>  {
> -	char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
> -				    prefix, node->full_name);
> -	if (!name)
> -		return -ENOMEM;
> +	const char *name;
> +
> +	if (of_property_read_string(node, "link-name", &name)) {
> +		name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
> +				      prefix, node->full_name);
> +		if (!name)
> +			return -ENOMEM;
> +	}
>  
>  	link->name = name;
>  	link->stream_name = name;


-- 
Jerome

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
  2024-04-08 18:15   ` Jerome Brunet
@ 2024-04-08 18:40     ` Dmitry Rokosov
  2024-04-08 18:45       ` Mark Brown
  2024-04-08 18:53       ` Jerome Brunet
  2024-04-08 18:42     ` Jerome Brunet
  1 sibling, 2 replies; 11+ messages in thread
From: Dmitry Rokosov @ 2024-04-08 18:40 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: neil.armstrong, lgirdwood, broonie, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
	linux-sound, devicetree, linux-kernel, linux-arm-kernel

On Mon, Apr 08, 2024 at 08:15:54PM +0200, Jerome Brunet wrote:
> 
> On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> 
> > The 'link-name' property presents an optional DT feature that empowers
> > users to customize the name associated with the DAI link and PCM stream.
> > This functionality reflects the approach often employed in Qualcomm
> > audio cards, providing enhanced flexibility in DAI naming conventions
> > for improved system integration and userspace experience.
> >
> > It allows userspace program to easy determine PCM stream purpose, e.g.:
> >     ~ # cat /proc/asound/pcm
> >     00-00: speaker (*) :  : playback 1
> >     00-01: mics (*) :  : capture 1
> >     00-02: loopback (*) :  : capture 1
> 
> The example above is exactly what you should not do with link names, at
> least with the amlogic audio system.
> 
> Userspace pcm, otherwise known as DPCM frontend, are merely that:
> frontends. What they do is entirely defined by the routing defined by
> the userspace (amixer and friends)
> 
> So naming the interface in DT (the FW describing the HW) after what the
> the userspace SW could possibly set later on is wrong.
> 
> Bottom line: I have mixed feeling about this change. It could allow all
> sort of bad names to be set.
> 
> The only way it could make sense HW wise is if the only allowed names
> where (fr|to)ddr_[abcd], which could help maps the interface and the
> kcontrol.
> 
> Such restriction should be documented in the binding doc.
> 

The link-name is an optional parameter. Yes, you are right, it can be
routed in a way that it no longer functions as a speaker in most cases.
However, if you plan to use your board's dt for common purposes, you
should not change the common names for DAI links. But if you know that
you have a static setup for speakers, microphones, loopback, or other
references (you 100% know it, because you are HW developer of this
board), why not help the user understand the PCM device assignment in
the easiest way?

Ultimately, it is the responsibility of the DT board developer to define
specific DAIs and name them based on their own knowledge about HW and
understanding of the board's usage purposes.

> >
> > The previous naming approach using auto-generated fe or be strings
> > continues to be utilized as a fallback.
> >
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  sound/soc/meson/meson-card-utils.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
> > index ed6c7e2f609c..7bae72905a9b 100644
> > --- a/sound/soc/meson/meson-card-utils.c
> > +++ b/sound/soc/meson/meson-card-utils.c
> > @@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
> >  				    struct device_node *node,
> >  				    const char *prefix)
> >  {
> > -	char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
> > -				    prefix, node->full_name);
> > -	if (!name)
> > -		return -ENOMEM;
> > +	const char *name;
> > +
> > +	if (of_property_read_string(node, "link-name", &name)) {
> > +		name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
> > +				      prefix, node->full_name);
> > +		if (!name)
> > +			return -ENOMEM;
> > +	}
> >  
> >  	link->name = name;
> >  	link->stream_name = name;
> 
> 
> -- 
> Jerome

-- 
Thank you,
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
  2024-04-08 18:15   ` Jerome Brunet
  2024-04-08 18:40     ` Dmitry Rokosov
@ 2024-04-08 18:42     ` Jerome Brunet
  1 sibling, 0 replies; 11+ messages in thread
From: Jerome Brunet @ 2024-04-08 18:42 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: Dmitry Rokosov, neil.armstrong, lgirdwood, broonie, conor+dt,
	robh+dt, krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
	linux-sound, devicetree, linux-kernel, linux-arm-kernel


On Mon 08 Apr 2024 at 20:15, Jerome Brunet <jbrunet@baylibre.com> wrote:

> On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>
>> The 'link-name' property presents an optional DT feature that empowers
>> users to customize the name associated with the DAI link and PCM stream.
>> This functionality reflects the approach often employed in Qualcomm
>> audio cards, providing enhanced flexibility in DAI naming conventions
>> for improved system integration and userspace experience.
>>
>> It allows userspace program to easy determine PCM stream purpose, e.g.:
>>     ~ # cat /proc/asound/pcm
>>     00-00: speaker (*) :  : playback 1
>>     00-01: mics (*) :  : capture 1
>>     00-02: loopback (*) :  : capture 1
>
> The example above is exactly what you should not do with link names, at
> least with the amlogic audio system.
>
> Userspace pcm, otherwise known as DPCM frontend, are merely that:
> frontends. What they do is entirely defined by the routing defined by
> the userspace (amixer and friends)
>
> So naming the interface in DT (the FW describing the HW) after what the
> the userspace SW could possibly set later on is wrong.
>
> Bottom line: I have mixed feeling about this change. It could allow all
> sort of bad names to be set.
>
> The only way it could make sense HW wise is if the only allowed names
> where (fr|to)ddr_[abcd], which could help maps the interface and the
> kcontrol.
>
> Such restriction should be documented in the binding doc.
>

Thinking about it further, even this does not make a lot of sense.
The information is already available from dai_name, prefixes and all.

Please use that instead if you must rename the userspace pcm, not DT.

>>
>> The previous naming approach using auto-generated fe or be strings
>> continues to be utilized as a fallback.
>>
>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> ---
>>  sound/soc/meson/meson-card-utils.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
>> index ed6c7e2f609c..7bae72905a9b 100644
>> --- a/sound/soc/meson/meson-card-utils.c
>> +++ b/sound/soc/meson/meson-card-utils.c
>> @@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
>>  				    struct device_node *node,
>>  				    const char *prefix)
>>  {
>> -	char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
>> -				    prefix, node->full_name);
>> -	if (!name)
>> -		return -ENOMEM;
>> +	const char *name;
>> +
>> +	if (of_property_read_string(node, "link-name", &name)) {
>> +		name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
>> +				      prefix, node->full_name);
>> +		if (!name)
>> +			return -ENOMEM;
>> +	}
>>  
>>  	link->name = name;
>>  	link->stream_name = name;


-- 
Jerome

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/2] ASoC: dt-bindings: meson: introduce link-name optional property
  2024-04-08 16:49 ` [PATCH v1 1/2] ASoC: dt-bindings: meson: introduce link-name optional property Dmitry Rokosov
@ 2024-04-08 18:44   ` Jerome Brunet
  0 siblings, 0 replies; 11+ messages in thread
From: Jerome Brunet @ 2024-04-08 18:44 UTC (permalink / raw)
  To: Dmitry Rokosov
  Cc: neil.armstrong, lgirdwood, jbrunet, broonie, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
	linux-sound, devicetree, linux-kernel, linux-arm-kernel


On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> The 'link-name' property is an optional DT property that allows for the
> customization of the name associated with the DAI link and PCM stream.
> This functionality mirrors the approach commonly utilized in Qualcomm
> audio cards, providing flexibility in DAI naming conventions for
> improved system integration and userspace experience.

As explained in patch #2, I think this change is wrong.
The names below describe a possible userspace usage of the interface,
not HW. This does not belong in DT.

The only valid name for the interface is one that helps map the PCM
interface and the kcontrols exposed in userspace.

This information is already available through dai_name, prefixes, etc.

Using DT for this is bad, in the context of these particular sound
cards at least.

>
> It allows userspace program to easy determine PCM stream purpose, e.g.:
>     ~ # cat /proc/asound/pcm
>     00-00: speaker (*) :  : playback 1
>     00-01: mics (*) :  : capture 1
>     00-02: loopback (*) :  : capture 1
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  .../devicetree/bindings/sound/amlogic,axg-sound-card.yaml   | 6 ++++++
>  .../devicetree/bindings/sound/amlogic,gx-sound-card.yaml    | 6 ++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml b/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
> index 492b41cc8ccd..46774a3e4b1d 100644
> --- a/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
> +++ b/Documentation/devicetree/bindings/sound/amlogic,axg-sound-card.yaml
> @@ -66,6 +66,11 @@ patternProperties:
>          maxItems: 1
>          description: phandle of the CPU DAI
>  
> +      link-name:
> +        description: Indicates dai-link name and PCM stream name.
> +        $ref: /schemas/types.yaml#/definitions/string
> +        maxItems: 1
> +
>      patternProperties:
>        "^dai-tdm-slot-(t|r)x-mask-[0-3]$":
>          $ref: /schemas/types.yaml#/definitions/uint32-array
> @@ -137,6 +142,7 @@ examples:
>  
>          dai-link-0 {
>              sound-dai = <&frddr_a>;
> +            link-name = "speaker";
>          };
>  
>          dai-link-1 {
> diff --git a/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml b/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
> index d4277d342e69..975c148f9712 100644
> --- a/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
> +++ b/Documentation/devicetree/bindings/sound/amlogic,gx-sound-card.yaml
> @@ -52,6 +52,11 @@ patternProperties:
>          maxItems: 1
>          description: phandle of the CPU DAI
>  
> +      link-name:
> +        description: Indicates dai-link name and PCM stream name.
> +        $ref: /schemas/types.yaml#/definitions/string
> +        maxItems: 1
> +
>      patternProperties:
>        "^codec(-[0-9]+)?$":
>          type: object
> @@ -89,6 +94,7 @@ examples:
>  
>          dai-link-0 {
>                 sound-dai = <&i2s_fifo>;
> +               link-name = "speaker";
>          };
>  
>          dai-link-1 {


-- 
Jerome

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
  2024-04-08 18:40     ` Dmitry Rokosov
@ 2024-04-08 18:45       ` Mark Brown
  2024-04-08 18:47         ` Dmitry Rokosov
  2024-04-08 18:53       ` Jerome Brunet
  1 sibling, 1 reply; 11+ messages in thread
From: Mark Brown @ 2024-04-08 18:45 UTC (permalink / raw)
  To: Dmitry Rokosov
  Cc: Jerome Brunet, neil.armstrong, lgirdwood, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
	linux-sound, devicetree, linux-kernel, linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Mon, Apr 08, 2024 at 09:40:41PM +0300, Dmitry Rokosov wrote:
> On Mon, Apr 08, 2024 at 08:15:54PM +0200, Jerome Brunet wrote:

> > Userspace pcm, otherwise known as DPCM frontend, are merely that:
> > frontends. What they do is entirely defined by the routing defined by
> > the userspace (amixer and friends)

> > So naming the interface in DT (the FW describing the HW) after what the
> > the userspace SW could possibly set later on is wrong.

> > Bottom line: I have mixed feeling about this change. It could allow all
> > sort of bad names to be set.

> > The only way it could make sense HW wise is if the only allowed names
> > where (fr|to)ddr_[abcd], which could help maps the interface and the
> > kcontrol.

> The link-name is an optional parameter. Yes, you are right, it can be
> routed in a way that it no longer functions as a speaker in most cases.
> However, if you plan to use your board's dt for common purposes, you
> should not change the common names for DAI links. But if you know that
> you have a static setup for speakers, microphones, loopback, or other
> references (you 100% know it, because you are HW developer of this
> board), why not help the user understand the PCM device assignment in
> the easiest way?

I would expect that the place to fix names based on the userspace
configuration is in whatever userspace is using to define it's
configurations, like a UCM config.

> Ultimately, it is the responsibility of the DT board developer to define
> specific DAIs and name them based on their own knowledge about HW and
> understanding of the board's usage purposes.

DT seems like the wrong abstraction layer here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
  2024-04-08 18:45       ` Mark Brown
@ 2024-04-08 18:47         ` Dmitry Rokosov
  2024-04-08 18:53           ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Rokosov @ 2024-04-08 18:47 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jerome Brunet, neil.armstrong, lgirdwood, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
	linux-sound, devicetree, linux-kernel, linux-arm-kernel

On Mon, Apr 08, 2024 at 07:45:00PM +0100, Mark Brown wrote:
> On Mon, Apr 08, 2024 at 09:40:41PM +0300, Dmitry Rokosov wrote:
> > On Mon, Apr 08, 2024 at 08:15:54PM +0200, Jerome Brunet wrote:
> 
> > > Userspace pcm, otherwise known as DPCM frontend, are merely that:
> > > frontends. What they do is entirely defined by the routing defined by
> > > the userspace (amixer and friends)
> 
> > > So naming the interface in DT (the FW describing the HW) after what the
> > > the userspace SW could possibly set later on is wrong.
> 
> > > Bottom line: I have mixed feeling about this change. It could allow all
> > > sort of bad names to be set.
> 
> > > The only way it could make sense HW wise is if the only allowed names
> > > where (fr|to)ddr_[abcd], which could help maps the interface and the
> > > kcontrol.
> 
> > The link-name is an optional parameter. Yes, you are right, it can be
> > routed in a way that it no longer functions as a speaker in most cases.
> > However, if you plan to use your board's dt for common purposes, you
> > should not change the common names for DAI links. But if you know that
> > you have a static setup for speakers, microphones, loopback, or other
> > references (you 100% know it, because you are HW developer of this
> > board), why not help the user understand the PCM device assignment in
> > the easiest way?
> 
> I would expect that the place to fix names based on the userspace
> configuration is in whatever userspace is using to define it's
> configurations, like a UCM config.
> 

Honestly, I have tried to find a way to rename the PCM device name or
mark it in some way (such as using a metainformation tag or any other
method), but unfortunately, my search has been unsuccessful.

> > Ultimately, it is the responsibility of the DT board developer to define
> > specific DAIs and name them based on their own knowledge about HW and
> > understanding of the board's usage purposes.
> 
> DT seems like the wrong abstraction layer here.

-- 
Thank you,
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
  2024-04-08 18:47         ` Dmitry Rokosov
@ 2024-04-08 18:53           ` Mark Brown
  0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2024-04-08 18:53 UTC (permalink / raw)
  To: Dmitry Rokosov
  Cc: Jerome Brunet, neil.armstrong, lgirdwood, conor+dt, robh+dt,
	krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
	linux-sound, devicetree, linux-kernel, linux-arm-kernel

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Mon, Apr 08, 2024 at 09:47:44PM +0300, Dmitry Rokosov wrote:
> On Mon, Apr 08, 2024 at 07:45:00PM +0100, Mark Brown wrote:

> > I would expect that the place to fix names based on the userspace
> > configuration is in whatever userspace is using to define it's
> > configurations, like a UCM config.

> Honestly, I have tried to find a way to rename the PCM device name or
> mark it in some way (such as using a metainformation tag or any other
> method), but unfortunately, my search has been unsuccessful.

I'd not be at all surprised if there's no such facility yet in whatever
userspace you're using and that it would need implementing, I'm just
saying that that seems like a better place to solve the problem you're
seeing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils
  2024-04-08 18:40     ` Dmitry Rokosov
  2024-04-08 18:45       ` Mark Brown
@ 2024-04-08 18:53       ` Jerome Brunet
  1 sibling, 0 replies; 11+ messages in thread
From: Jerome Brunet @ 2024-04-08 18:53 UTC (permalink / raw)
  To: Dmitry Rokosov
  Cc: Jerome Brunet, neil.armstrong, lgirdwood, broonie, conor+dt,
	robh+dt, krzysztof.kozlowski+dt, perex, tiwai, khilman,
	martin.blumenstingl, kernel, rockosov, linux-amlogic, alsa-devel,
	linux-sound, devicetree, linux-kernel, linux-arm-kernel


On Mon 08 Apr 2024 at 21:40, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> On Mon, Apr 08, 2024 at 08:15:54PM +0200, Jerome Brunet wrote:
>> 
>> On Mon 08 Apr 2024 at 19:49, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>> 
>> > The 'link-name' property presents an optional DT feature that empowers
>> > users to customize the name associated with the DAI link and PCM stream.
>> > This functionality reflects the approach often employed in Qualcomm
>> > audio cards, providing enhanced flexibility in DAI naming conventions
>> > for improved system integration and userspace experience.
>> >
>> > It allows userspace program to easy determine PCM stream purpose, e.g.:
>> >     ~ # cat /proc/asound/pcm
>> >     00-00: speaker (*) :  : playback 1
>> >     00-01: mics (*) :  : capture 1
>> >     00-02: loopback (*) :  : capture 1
>> 
>> The example above is exactly what you should not do with link names, at
>> least with the amlogic audio system.
>> 
>> Userspace pcm, otherwise known as DPCM frontend, are merely that:
>> frontends. What they do is entirely defined by the routing defined by
>> the userspace (amixer and friends)
>> 
>> So naming the interface in DT (the FW describing the HW) after what the
>> the userspace SW could possibly set later on is wrong.
>> 
>> Bottom line: I have mixed feeling about this change. It could allow all
>> sort of bad names to be set.
>> 
>> The only way it could make sense HW wise is if the only allowed names
>> where (fr|to)ddr_[abcd], which could help maps the interface and the
>> kcontrol.
>> 
>> Such restriction should be documented in the binding doc.
>> 
>
> The link-name is an optional parameter. Yes, you are right, it can be
> routed in a way that it no longer functions as a speaker in most cases.
> However, if you plan to use your board's dt for common purposes, you
> should not change the common names for DAI links. But if you know that
> you have a static setup for speakers, microphones, loopback, or other
> references (you 100% know it, because you are HW developer of this
> board), why not help the user understand the PCM device assignment in
> the easiest way?
>
> Ultimately, it is the responsibility of the DT board developer to define
> specific DAIs and name them based on their own knowledge about HW and
> understanding of the board's usage purposes.

Speaker and mics are NOT statically tied to a frontend. They are tied to a
codec (... possibly). The routing from the frontend to the backend is
dynamic, even while streaming.

So defining FW names based on usage in wrong.
As Mark pointed out as well, DT is not the place for this.

>
>> >
>> > The previous naming approach using auto-generated fe or be strings
>> > continues to be utilized as a fallback.
>> >
>> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > ---
>> >  sound/soc/meson/meson-card-utils.c | 12 ++++++++----
>> >  1 file changed, 8 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c
>> > index ed6c7e2f609c..7bae72905a9b 100644
>> > --- a/sound/soc/meson/meson-card-utils.c
>> > +++ b/sound/soc/meson/meson-card-utils.c
>> > @@ -94,10 +94,14 @@ static int meson_card_set_link_name(struct snd_soc_card *card,
>> >  				    struct device_node *node,
>> >  				    const char *prefix)
>> >  {
>> > -	char *name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
>> > -				    prefix, node->full_name);
>> > -	if (!name)
>> > -		return -ENOMEM;
>> > +	const char *name;
>> > +
>> > +	if (of_property_read_string(node, "link-name", &name)) {
>> > +		name = devm_kasprintf(card->dev, GFP_KERNEL, "%s.%s",
>> > +				      prefix, node->full_name);
>> > +		if (!name)
>> > +			return -ENOMEM;
>> > +	}
>> >  
>> >  	link->name = name;
>> >  	link->stream_name = name;
>> 
>> 
>> -- 
>> Jerome


-- 
Jerome

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-04-08 19:00 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-08 16:49 [PATCH v1 0/2] ASoC: meson: implement link-name optional property in meson audio card Dmitry Rokosov
2024-04-08 16:49 ` [PATCH v1 1/2] ASoC: dt-bindings: meson: introduce link-name optional property Dmitry Rokosov
2024-04-08 18:44   ` Jerome Brunet
2024-04-08 16:49 ` [PATCH v1 2/2] ASoC: meson: implement link-name optional property in meson card utils Dmitry Rokosov
2024-04-08 18:15   ` Jerome Brunet
2024-04-08 18:40     ` Dmitry Rokosov
2024-04-08 18:45       ` Mark Brown
2024-04-08 18:47         ` Dmitry Rokosov
2024-04-08 18:53           ` Mark Brown
2024-04-08 18:53       ` Jerome Brunet
2024-04-08 18:42     ` Jerome Brunet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).