| Message ID | 20240408164947.30717-3-ddrokosov@salutedevices.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: meson: implement link-name optional property in meson audio card | expand |
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;
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
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;
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.
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.
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.
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
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;
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(-)