[alsa-devel] [PATCH 0/2] ASoC: add new struct snd_soc_chip
Hi Mark, Liam, Stephen, Lars
In my understanding on ASoC,
- We will clean-up ASoC code. - CODEC/CPU will be common struct (platform too ??)
As 1st trigger for this common struct, I created very basic struct snd_soc_chip. I hope it is accepted, and updated to be more useful.
Kuninori Morimoto (2): ASoC: add snd_soc_register_chip() ASoC: fsi: use snd_soc_register_chip() instead of snd_soc_register_dais()
include/sound/soc.h | 19 +++++++++++++ sound/soc/sh/fsi.c | 11 +++++--- sound/soc/soc-core.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 4 deletions(-)
Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC.
As result of ASoC community discussion, we consider new struct snd_soc_chip for CPU/CODEC, and will switch over to use it.
This patch adds very basic struct snd_soc_chip, and register function for it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 19 +++++++++++++ sound/soc/soc-core.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+)
diff --git a/include/sound/soc.h b/include/sound/soc.h index a6a059c..e0a665c 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -324,6 +324,8 @@ struct snd_soc_dai_link; struct snd_soc_platform_driver; struct snd_soc_codec; struct snd_soc_codec_driver; +struct snd_soc_chip; +struct snd_soc_chip_driver; struct soc_enum; struct snd_soc_jack; struct snd_soc_jack_zone; @@ -377,6 +379,10 @@ int snd_soc_register_codec(struct device *dev, const struct snd_soc_codec_driver *codec_drv, struct snd_soc_dai_driver *dai_drv, int num_dai); void snd_soc_unregister_codec(struct device *dev); +int snd_soc_register_chip(struct device *dev, + const struct snd_soc_chip_driver *chip_drv, + struct snd_soc_dai_driver *dai_drv, int num_dai); +void snd_soc_unregister_chip(struct device *dev); int snd_soc_codec_volatile_register(struct snd_soc_codec *codec, unsigned int reg); int snd_soc_codec_readable_register(struct snd_soc_codec *codec, @@ -841,6 +847,19 @@ struct snd_soc_platform { #endif };
+struct snd_soc_chip_driver { +}; + +struct snd_soc_chip { + const char *name; + int id; + int num_dai; + struct device *dev; + struct list_head list; + + const struct snd_soc_chip_driver *driver; +}; + struct snd_soc_dai_link { /* config - must be set by machine driver */ const char *name; /* Codec name */ diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index e02c374..1ac96df 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -58,6 +58,7 @@ static DEFINE_MUTEX(client_mutex); static LIST_HEAD(dai_list); static LIST_HEAD(platform_list); static LIST_HEAD(codec_list); +static LIST_HEAD(chip_list);
/* * This is a timeout to do a DAPM powerdown after a stream is closed(). @@ -4134,6 +4135,82 @@ found: } EXPORT_SYMBOL_GPL(snd_soc_unregister_codec);
+ +/** + * snd_soc_register_chip - Register a chip with the ASoC core + * + */ +int snd_soc_register_chip(struct device *dev, + const struct snd_soc_chip_driver *chip_drv, + struct snd_soc_dai_driver *dai_drv, + int num_dai) +{ + struct snd_soc_chip *chip; + int ret; + + dev_dbg(dev, "chip register %s\n", dev_name(dev)); + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (!chip) { + dev_err(dev, "ASoC: Failed to allocate memory\n"); + return -ENOMEM; + } + + chip->name = fmt_single_name(dev, &chip->id); + if (!chip->name) { + dev_err(dev, "ASoC: Failed to simplifying name\n"); + return -ENOMEM; + } + + chip->dev = dev; + chip->driver = chip_drv; + chip->num_dai = num_dai; + + ret = snd_soc_register_dais(dev, dai_drv, num_dai); + if (ret < 0) { + dev_err(dev, "ASoC: Failed to regster DAIs: %d\n", ret); + goto error_chip_name; + } + + mutex_lock(&client_mutex); + list_add(&chip->list, &chip_list); + mutex_unlock(&client_mutex); + + dev_dbg(chip->dev, "ASoC: Registered chip '%s'\n", chip->name); + + return ret; + +error_chip_name: + kfree(chip->name); + + return ret; +} + +/** + * snd_soc_unregister_chip - Unregister a chip from the ASoC core + * + */ +void snd_soc_unregister_chip(struct device *dev) +{ + struct snd_soc_chip *chip; + + list_for_each_entry(chip, &chip_list, list) { + if (dev == chip->dev) + goto found; + } + return; + +found: + snd_soc_unregister_dais(dev, chip->num_dai); + + mutex_lock(&client_mutex); + list_del(&chip->list); + mutex_unlock(&client_mutex); + + dev_dbg(dev, "ASoC: Unregistered chip '%s'\n", chip->name); + kfree(chip->name); +} + /* Retrieve a card's name from device tree */ int snd_soc_of_parse_card_name(struct snd_soc_card *card, const char *propname)
On 03/07/2013 06:50 PM, Kuninori Morimoto wrote:
Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC.
As result of ASoC community discussion, we consider new struct snd_soc_chip for CPU/CODEC, and will switch over to use it.
This patch adds very basic struct snd_soc_chip, and register function for it.
Conceptually this seems fine.
I would have called this a "component" rather than a "chip", since:
a) That aligns better with the phrase "multi-component" for related ASoC features.
b) This struct doesn't always represent a whole chip, but perhaps just an IP block within a chip. Consider an SoC with 5 completely separate I2S block and an SPDIF block. Those are probably each separate Linux platform devices, and each would register as its own snd_soc_chip.
To be fully useful, we'd have to convert snd_soc_sodec to be a snd_soc_chip too. Should CODEC be a "sub-class" of chip? Should we instead rename snd_soc_codec to snd_soc_chip/component? How do we get there from here?
This patch switches over to use snd_soc_register_chip() instead of snd_soc_register_dais() with very basic settings.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index c724026a..e622760 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -1886,6 +1886,9 @@ static struct snd_soc_platform_driver fsi_soc_platform = { .pcm_free = fsi_pcm_free, };
+static const struct snd_soc_chip_driver fsi_soc_chip = { +}; + /* * platform function */ @@ -2046,10 +2049,10 @@ static int fsi_probe(struct platform_device *pdev) goto exit_fsib; }
- ret = snd_soc_register_dais(&pdev->dev, fsi_soc_dai, - ARRAY_SIZE(fsi_soc_dai)); + ret = snd_soc_register_chip(&pdev->dev, &fsi_soc_chip, + fsi_soc_dai, ARRAY_SIZE(fsi_soc_dai)); if (ret < 0) { - dev_err(&pdev->dev, "cannot snd dai register\n"); + dev_err(&pdev->dev, "cannot snd chip register\n"); goto exit_snd_soc; }
@@ -2074,7 +2077,7 @@ static int fsi_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
- snd_soc_unregister_dais(&pdev->dev, ARRAY_SIZE(fsi_soc_dai)); + snd_soc_unregister_chip(&pdev->dev); snd_soc_unregister_platform(&pdev->dev);
fsi_stream_remove(&master->fsia);
Hi Mark, Liam, Stephen, Lars
These are v2 patches for CPU/CODEC common struct support, but only rename. (chip -> component)
As 1st trigger for this common struct, I created very basic struct snd_soc_chip. I hope it is accepted, and updated to be more useful.
Kuninori Morimoto (2): ASoC: add snd_soc_register_chip() ASoC: fsi: use snd_soc_register_chip() instead of snd_soc_register_dais()
include/sound/soc.h | 19 +++++++++++++ sound/soc/sh/fsi.c | 11 +++++--- sound/soc/soc-core.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 4 deletions(-)
Best regards --- Kuninori Morimoto
Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC.
As result of ASoC community discussion, we consider new struct snd_soc_component for CPU/CODEC, and will switch over to use it.
This patch adds very basic struct snd_soc_component, and register function for it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- v1 -> v2
- rename chip -> component
include/sound/soc.h | 19 +++++++++++++ sound/soc/soc-core.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+)
diff --git a/include/sound/soc.h b/include/sound/soc.h index c84062b..d32915d 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -324,6 +324,8 @@ struct snd_soc_dai_link; struct snd_soc_platform_driver; struct snd_soc_codec; struct snd_soc_codec_driver; +struct snd_soc_component; +struct snd_soc_component_driver; struct soc_enum; struct snd_soc_jack; struct snd_soc_jack_zone; @@ -377,6 +379,10 @@ int snd_soc_register_codec(struct device *dev, const struct snd_soc_codec_driver *codec_drv, struct snd_soc_dai_driver *dai_drv, int num_dai); void snd_soc_unregister_codec(struct device *dev); +int snd_soc_register_component(struct device *dev, + const struct snd_soc_component_driver *cmpnt_drv, + struct snd_soc_dai_driver *dai_drv, int num_dai); +void snd_soc_unregister_component(struct device *dev); int snd_soc_codec_volatile_register(struct snd_soc_codec *codec, unsigned int reg); int snd_soc_codec_readable_register(struct snd_soc_codec *codec, @@ -841,6 +847,19 @@ struct snd_soc_platform { #endif };
+struct snd_soc_component_driver { +}; + +struct snd_soc_component { + const char *name; + int id; + int num_dai; + struct device *dev; + struct list_head list; + + const struct snd_soc_component_driver *driver; +}; + struct snd_soc_dai_link { /* config - must be set by machine driver */ const char *name; /* Codec name */ diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index e02c374..307a621 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -58,6 +58,7 @@ static DEFINE_MUTEX(client_mutex); static LIST_HEAD(dai_list); static LIST_HEAD(platform_list); static LIST_HEAD(codec_list); +static LIST_HEAD(component_list);
/* * This is a timeout to do a DAPM powerdown after a stream is closed(). @@ -4134,6 +4135,82 @@ found: } EXPORT_SYMBOL_GPL(snd_soc_unregister_codec);
+ +/** + * snd_soc_register_component - Register a component with the ASoC core + * + */ +int snd_soc_register_component(struct device *dev, + const struct snd_soc_component_driver *cmpnt_drv, + struct snd_soc_dai_driver *dai_drv, + int num_dai) +{ + struct snd_soc_component *cmpnt; + int ret; + + dev_dbg(dev, "component register %s\n", dev_name(dev)); + + cmpnt = devm_kzalloc(dev, sizeof(*cmpnt), GFP_KERNEL); + if (!cmpnt) { + dev_err(dev, "ASoC: Failed to allocate memory\n"); + return -ENOMEM; + } + + cmpnt->name = fmt_single_name(dev, &cmpnt->id); + if (!cmpnt->name) { + dev_err(dev, "ASoC: Failed to simplifying name\n"); + return -ENOMEM; + } + + cmpnt->dev = dev; + cmpnt->driver = cmpnt_drv; + cmpnt->num_dai = num_dai; + + ret = snd_soc_register_dais(dev, dai_drv, num_dai); + if (ret < 0) { + dev_err(dev, "ASoC: Failed to regster DAIs: %d\n", ret); + goto error_component_name; + } + + mutex_lock(&client_mutex); + list_add(&cmpnt->list, &component_list); + mutex_unlock(&client_mutex); + + dev_dbg(cmpnt->dev, "ASoC: Registered component '%s'\n", cmpnt->name); + + return ret; + +error_component_name: + kfree(cmpnt->name); + + return ret; +} + +/** + * snd_soc_unregister_component - Unregister a component from the ASoC core + * + */ +void snd_soc_unregister_component(struct device *dev) +{ + struct snd_soc_component *cmpnt; + + list_for_each_entry(cmpnt, &component_list, list) { + if (dev == cmpnt->dev) + goto found; + } + return; + +found: + snd_soc_unregister_dais(dev, cmpnt->num_dai); + + mutex_lock(&client_mutex); + list_del(&cmpnt->list); + mutex_unlock(&client_mutex); + + dev_dbg(dev, "ASoC: Unregistered component '%s'\n", cmpnt->name); + kfree(cmpnt->name); +} + /* Retrieve a card's name from device tree */ int snd_soc_of_parse_card_name(struct snd_soc_card *card, const char *propname)
On 03/11/2013 07:27 PM, Kuninori Morimoto wrote:
Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC.
As result of ASoC community discussion, we consider new struct snd_soc_component for CPU/CODEC, and will switch over to use it.
This patch adds very basic struct snd_soc_component, and register function for it.
The series seems reasonable enough to me, as a basis for future work on the DT stuff, so,
Reviewed-by: Stephen Warren swarren@nvidia.com
I wonder how much work it is to convert all users of snd_soc_register_dais() to this new API; is it worth doing so?
On 03/12/2013 07:19 PM, Stephen Warren wrote:
On 03/11/2013 07:27 PM, Kuninori Morimoto wrote:
Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC.
As result of ASoC community discussion, we consider new struct snd_soc_component for CPU/CODEC, and will switch over to use it.
This patch adds very basic struct snd_soc_component, and register function for it.
The series seems reasonable enough to me, as a basis for future work on the DT stuff, so,
Reviewed-by: Stephen Warren swarren@nvidia.com
I wonder how much work it is to convert all users of snd_soc_register_dais() to this new API; is it worth doing so?
If we allow the component driver to be NULL it's pretty much straight forward using coccinelle. Don't know if it is worth it to conver them all right away.
@@ expression dev; @@ -snd_soc_register_dais( +snd_soc_register_component( dev, +NULL, ...)
@@ expression dev; expression num_dais; @@ -snd_soc_unregister_dais( +snd_soc_unregister_component( dev, -num_dais )
- Lars
On Tue, Mar 12, 2013 at 07:57:47PM +0100, Lars-Peter Clausen wrote:
On 03/12/2013 07:19 PM, Stephen Warren wrote:
I wonder how much work it is to convert all users of snd_soc_register_dais() to this new API; is it worth doing so?
If we allow the component driver to be NULL it's pretty much straight forward using coccinelle. Don't know if it is worth it to conver them all right away.
Given that it's automatable...
On 03/11/2013 07:27 PM, Kuninori Morimoto wrote:
Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC.
As result of ASoC community discussion, we consider new struct snd_soc_component for CPU/CODEC, and will switch over to use it.
This patch adds very basic struct snd_soc_component, and register function for it.
Perhaps as a separate patch: It'd be nice to add file /sys/kernel/debug/asoc/components that listed all the registered components. Anywhere else in debugfs/sysfs/... that CODECs are enumerate might want to cover components too.
On Tue, Mar 12, 2013 at 03:35:01PM -0600, Stephen Warren wrote:
On 03/11/2013 07:27 PM, Kuninori Morimoto wrote:
This patch adds very basic struct snd_soc_component, and register function for it.
Perhaps as a separate patch: It'd be nice to add file /sys/kernel/debug/asoc/components that listed all the registered components. Anywhere else in debugfs/sysfs/... that CODECs are enumerate might want to cover components too.
Yup, definitely - this will certainly need to get done at the point where we can bind CODECs as part of the card setup.
On Mon, Mar 11, 2013 at 06:27:21PM -0700, Kuninori Morimoto wrote:
Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC.
Applied, thanks.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- v1 -> v2
- rename chip -> component
sound/soc/sh/fsi.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index c724026a..84067b2 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -1886,6 +1886,9 @@ static struct snd_soc_platform_driver fsi_soc_platform = { .pcm_free = fsi_pcm_free, };
+static const struct snd_soc_component_driver fsi_soc_component = { +}; + /* * platform function */ @@ -2046,10 +2049,10 @@ static int fsi_probe(struct platform_device *pdev) goto exit_fsib; }
- ret = snd_soc_register_dais(&pdev->dev, fsi_soc_dai, - ARRAY_SIZE(fsi_soc_dai)); + ret = snd_soc_register_component(&pdev->dev, &fsi_soc_component, + fsi_soc_dai, ARRAY_SIZE(fsi_soc_dai)); if (ret < 0) { - dev_err(&pdev->dev, "cannot snd dai register\n"); + dev_err(&pdev->dev, "cannot snd component register\n"); goto exit_snd_soc; }
@@ -2074,7 +2077,7 @@ static int fsi_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
- snd_soc_unregister_dais(&pdev->dev, ARRAY_SIZE(fsi_soc_dai)); + snd_soc_unregister_component(&pdev->dev); snd_soc_unregister_platform(&pdev->dev);
fsi_stream_remove(&master->fsia);
On Mon, Mar 11, 2013 at 06:27:46PM -0700, Kuninori Morimoto wrote:
+static const struct snd_soc_component_driver fsi_soc_component = { +};
It'd be good to at least have a name here.
Hi Mark
+static const struct snd_soc_component_driver fsi_soc_component = { +};
It'd be good to at least have a name here.
Easy.
But, current snd_soc_platform / snd_soc_codec have "name", but these *driver* doesn't have it.
It will have double name... Is it OK ?
struct snd_soc_component_driver { const char *name; };
struct snd_soc_component { const char *name; ... const struct snd_soc_component_driver *driver; };
Best regards --- Kuninori Morimoto
This patch adds .name member on snd_soc_component_driver. But this patch doesn't care about whether cmpnt_drv was NULL, and/or its name was NULL in snd_soc_register_component() at this point.
Because, it is easy to switch over to snd_soc_register_component() from snd_soc_register_dais() if it doesn't care cmpnt_drv was NULL.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 8c46d0a..44c9cbd 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -848,6 +848,7 @@ struct snd_soc_platform { };
struct snd_soc_component_driver { + const char *name; };
struct snd_soc_component {
On Thu, Mar 14, 2013 at 12:18:41AM -0700, Kuninori Morimoto wrote:
This patch adds .name member on snd_soc_component_driver. But this patch doesn't care about whether cmpnt_drv was NULL, and/or its name was NULL in snd_soc_register_component() at this point.
Applied both, thanks.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- v1 -> v2
- add .name
sound/soc/sh/fsi.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index c724026a..254c637 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -1886,6 +1886,10 @@ static struct snd_soc_platform_driver fsi_soc_platform = { .pcm_free = fsi_pcm_free, };
+static const struct snd_soc_component_driver fsi_soc_component = { + .name = "fsi", +}; + /* * platform function */ @@ -2046,10 +2050,10 @@ static int fsi_probe(struct platform_device *pdev) goto exit_fsib; }
- ret = snd_soc_register_dais(&pdev->dev, fsi_soc_dai, - ARRAY_SIZE(fsi_soc_dai)); + ret = snd_soc_register_component(&pdev->dev, &fsi_soc_component, + fsi_soc_dai, ARRAY_SIZE(fsi_soc_dai)); if (ret < 0) { - dev_err(&pdev->dev, "cannot snd dai register\n"); + dev_err(&pdev->dev, "cannot snd component register\n"); goto exit_snd_soc; }
@@ -2074,7 +2078,7 @@ static int fsi_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
- snd_soc_unregister_dais(&pdev->dev, ARRAY_SIZE(fsi_soc_dai)); + snd_soc_unregister_component(&pdev->dev); snd_soc_unregister_platform(&pdev->dev);
fsi_stream_remove(&master->fsia);
participants (4)
-
Kuninori Morimoto -
Lars-Peter Clausen -
Mark Brown -
Stephen Warren