[alsa-devel] [RFC][PATCH 0/2] ASoC: add DT support on simple-card
Hi Mark
These patches add DT support for soc-simple-card driver.
Kuninori Morimoto (2): ASoC: add snd_soc_of_parse_daifmt() ASoC: simple-card: add DT support
include/sound/soc.h | 2 + sound/soc/generic/simple-card.c | 90 +++++++++++++++++++++++++++++++++++++-- sound/soc/soc-core.c | 72 +++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 3 deletions(-)
1st patch adds snd_soc_of_parse_daifmt() to select SND_SOC_DAIFMT_xxx, but I'm not sure whether this is good idea. So these are [RFC] patches.
I thought that it will be trouble if... 1) platform/driver DT used snd_fmt = <0xYYYY> style for SND_SOC_DAIFMT_xxx 2) someone might update SND_SOC_DAIFMT_xxx value for some reasons. 3) bootloader should update DT value according to kernel version if 2) happened
I put snd_soc_of_parse_daifmt() on soc-core.c because I thought that other driver can reuse it. I need your comment for it.
Best regards --- Kuninori Morimoto
ALSA SoC system is using SND_SOC_DAIFMT_xxx flags on each platform, and its value might be updated for some reason. This means that if platform is using Device Tree and if it gets parameter value directly, it is difficult to keep compatible on each platform Device Tree. This patch adds snd_soc_of_parse_daifmt() to solve this issue. Each platform can use [prefix]snd,soc,daifmt,xxx to set SND_SOC_DAIFMT_XXX on Device Tree.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 2 ++ sound/soc/soc-core.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 91244a0..af64632 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1168,6 +1168,8 @@ int snd_soc_of_parse_card_name(struct snd_soc_card *card, const char *propname); int snd_soc_of_parse_audio_routing(struct snd_soc_card *card, const char *propname); +int snd_soc_of_parse_daifmt(struct device_node *np, + const char *prefix, unsigned int *fmt);
#include <sound/soc-dai.h>
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 9c768bc..0142742 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -4179,6 +4179,78 @@ int snd_soc_of_parse_audio_routing(struct snd_soc_card *card, } EXPORT_SYMBOL_GPL(snd_soc_of_parse_audio_routing);
+static int __snd_soc_of_parse_daifmt(struct device_node *np, + const char *prefix, unsigned int *fmt, + const char *propname, unsigned int val) +{ + char str[128]; + int ret; + + snprintf(str, 128, "%ssnd,soc,daifmt,%s", prefix, propname); + ret = of_property_read_bool(np, str); + if (ret) + *fmt |= val; + + return ret; +} + +int snd_soc_of_parse_daifmt(struct device_node *np, + const char *prefix, unsigned int *fmt) +{ + int ret = 0; + char pre[] = ""; + + if (!prefix) + prefix = pre; + + /* + * it will find "[prefix]snd,soc,daifmt,xxx" from device_node, + * and set SND_SOC_DAIFMT_XXX + */ + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "i2s", SND_SOC_DAIFMT_I2S); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "right_j", SND_SOC_DAIFMT_RIGHT_J); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "left_j", SND_SOC_DAIFMT_LEFT_J); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "dsp_a", SND_SOC_DAIFMT_DSP_A); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "dsp_b", SND_SOC_DAIFMT_DSP_B); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "ac97", SND_SOC_DAIFMT_AC97); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "pdm", SND_SOC_DAIFMT_PDM); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "msb", SND_SOC_DAIFMT_MSB); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "lsb", SND_SOC_DAIFMT_LSB); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "cont", SND_SOC_DAIFMT_CONT); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "gated", SND_SOC_DAIFMT_GATED); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "nb_nf", SND_SOC_DAIFMT_NB_NF); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "nb_if", SND_SOC_DAIFMT_NB_IF); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "ib_nf", SND_SOC_DAIFMT_IB_NF); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "ib_if", SND_SOC_DAIFMT_IB_IF); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "cbm_cfm", SND_SOC_DAIFMT_CBM_CFM); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "cbs_cfm", SND_SOC_DAIFMT_CBS_CFM); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "cbm_cfs", SND_SOC_DAIFMT_CBM_CFS); + ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt, + "cbs_cfs", SND_SOC_DAIFMT_CBS_CFS); + + return ret; +} +EXPORT_SYMBOL_GPL(snd_soc_of_parse_daifmt); + + static int __init snd_soc_init(void) { #ifdef CONFIG_DEBUG_FS
On 11/28/2012 09:31 PM, Kuninori Morimoto wrote:
ALSA SoC system is using SND_SOC_DAIFMT_xxx flags on each platform, and its value might be updated for some reason. This means that if platform is using Device Tree and if it gets parameter value directly, it is difficult to keep compatible on each platform Device Tree. This patch adds snd_soc_of_parse_daifmt() to solve this issue. Each platform can use [prefix]snd,soc,daifmt,xxx to set SND_SOC_DAIFMT_XXX on Device Tree.
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
+static int __snd_soc_of_parse_daifmt(struct device_node *np,
const char *prefix, unsigned int *fmt,const char *propname, unsigned int val)+{
- char str[128];
- int ret;
- snprintf(str, 128, "%ssnd,soc,daifmt,%s", prefix, propname);
- ret = of_property_read_bool(np, str);
- if (ret)
*fmt |= val;- return ret;
+}
+int snd_soc_of_parse_daifmt(struct device_node *np,
const char *prefix, unsigned int *fmt)+{
- int ret = 0;
- char pre[] = "";
- if (!prefix)
prefix = pre;- /*
* it will find "[prefix]snd,soc,daifmt,xxx" from device_node,* and set SND_SOC_DAIFMT_XXX*/- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"i2s", SND_SOC_DAIFMT_I2S);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"right_j", SND_SOC_DAIFMT_RIGHT_J);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"left_j", SND_SOC_DAIFMT_LEFT_J);
...
I think it'd be more typical to represent as a single integer property, where the value is an enumeration indicating the type.
But isn't there a lot more to the DAI format than just the format enum itself?
On Wed, Nov 28, 2012 at 10:21:59PM -0700, Stephen Warren wrote:
But isn't there a lot more to the DAI format than just the format enum itself?
There is but it should be OK to represent that as separate properties rather than trying to glom all the combinations into a single property. For the frame inversion flags we can assume a reasonable default of non inverted so it seems OK to leave them for now.
Hi Stephen
I think it'd be more typical to represent as a single integer property, where the value is an enumeration indicating the type.
Sorry, I couldn't understand correctly this. Do you mean like this ?
snd.soc.daifmt.format = <3> /* SND_SOC_DAIFMT_LEFT_J */ snd.soc.daifmt.clock_gate = <1> /* SND_SOC_DAIFMT_CONT */ snd.soc.daifmt.inversion = <3> /* SND_SOC_DAIFMT_IB_NF */ snd.soc.daifmt.hw_clock = <2> /* SND_SOC_DAIFMT_CBS_CFM */
I added SND_SOC_DAIFMT_xxx here, but this style is not readable/understandable for me. And it is difficult to keep compatible if the number was changed. (never happen ? I'm not sure)
How about to use string ?
snd.soc.daifmt.format = "left_j" snd.soc.daifmt.clock_gate = "cont" snd.soc.daifmt.inversion = "ib_nf" snd.soc.daifmt.hw_clock = "cbs_cfm"
Best regards --- Kuninori Morimoto
On 11/29/2012 05:35 PM, Kuninori Morimoto wrote:
Hi Stephen
I think it'd be more typical to represent as a single integer property, where the value is an enumeration indicating the type.
Sorry, I couldn't understand correctly this. Do you mean like this ?
snd.soc.daifmt.format = <3> /* SND_SOC_DAIFMT_LEFT_J */ snd.soc.daifmt.clock_gate = <1> /* SND_SOC_DAIFMT_CONT */ snd.soc.daifmt.inversion = <3> /* SND_SOC_DAIFMT_IB_NF */ snd.soc.daifmt.hw_clock = <2> /* SND_SOC_DAIFMT_CBS_CFM */
Sorry for the slow response.
What I meant was that rather than:
- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"i2s", SND_SOC_DAIFMT_I2S);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"right_j", SND_SOC_DAIFMT_RIGHT_J);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"left_j", SND_SOC_DAIFMT_LEFT_J);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"dsp_a", SND_SOC_DAIFMT_DSP_A);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"dsp_b", SND_SOC_DAIFMT_DSP_B);
I'd expect to see something more like:
fmt = of_property_read_u32_array(np, "bit-format");
But perhaps your binding is representing the set of possible legal formats? I had assumed it was attempting to represent the single specific format to choose.
Related, I also wasn't suggesting combining format, clock-gate, inversion, ... into a single property.
I added SND_SOC_DAIFMT_xxx here, but this style is not readable/understandable for me. And it is difficult to keep compatible if the number was changed. (never happen ? I'm not sure)
Well, once a DT binding is created, you can't change the numbers, or you would break the ability for an old DT to work with a newer kernel.
How about to use string ?
snd.soc.daifmt.format = "left_j" snd.soc.daifmt.clock_gate = "cont" snd.soc.daifmt.inversion = "ib_nf" snd.soc.daifmt.hw_clock = "cbs_cfm"
That's probably the best we can do for now. Using a pre-processor would be best:
#define SND_SOC_DAIFMT_LEFT_J 3
snd.soc.daifmt.format = <SND_SOC_DAIFMT_LEFT_J>;
... but we can't do that yet...
Hi Stephen Cc Mark
Thank you for your reply
- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"i2s", SND_SOC_DAIFMT_I2S);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"right_j", SND_SOC_DAIFMT_RIGHT_J);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"left_j", SND_SOC_DAIFMT_LEFT_J);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"dsp_a", SND_SOC_DAIFMT_DSP_A);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"dsp_b", SND_SOC_DAIFMT_DSP_B);I'd expect to see something more like:
fmt = of_property_read_u32_array(np, "bit-format");
Ahh... I see.
Well, once a DT binding is created, you can't change the numbers, or you would break the ability for an old DT to work with a newer kernel.
OK. I understand.
How about to use string ?
snd.soc.daifmt.format = "left_j" snd.soc.daifmt.clock_gate = "cont" snd.soc.daifmt.inversion = "ib_nf" snd.soc.daifmt.hw_clock = "cbs_cfm"
That's probably the best we can do for now. Using a pre-processor would be best:
#define SND_SOC_DAIFMT_LEFT_J 3
snd.soc.daifmt.format = <SND_SOC_DAIFMT_LEFT_J>;
... but we can't do that yet...
Thank you.
I tried v2 of snd_soc_of_parse_daifmt() which is using "string" and "array" style
[prefix]snd,soc,daifmt = "i2c", "nb_if", "cbm_cfm";
I would like to know your opinion.
---------------------------------------------- This patch adds snd_soc_of_parse_daifmt() and supports below style on DT.
[prefix]snd,soc,daifmt = "i2c", "nb_if", "cbm_cfm";
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
diff --git a/include/sound/soc.h b/include/sound/soc.h index 91244a0..af64632 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1168,6 +1168,8 @@ int snd_soc_of_parse_card_name(struct snd_soc_card *card, const char *propname); int snd_soc_of_parse_audio_routing(struct snd_soc_card *card, const char *propname); +int snd_soc_of_parse_daifmt(struct device_node *np, + const char *prefix, unsigned int *fmt);
#include <sound/soc-dai.h>
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 9c768bc..6cac98a 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -4179,6 +4179,74 @@ int snd_soc_of_parse_audio_routing(struct snd_soc_card *card, } EXPORT_SYMBOL_GPL(snd_soc_of_parse_audio_routing);
+int snd_soc_of_parse_daifmt(struct device_node *np, + const char *prefix, unsigned int *fmt) +{ + int num, i, j; + int ret; + char prop[128]; + unsigned int format; + const char *str; + struct { + char *name; + unsigned int val; + unsigned int msk; + } of_parse_table[] = { + { "i2s", SND_SOC_DAIFMT_I2S, SND_SOC_DAIFMT_FORMAT_MASK }, + { "right_j", SND_SOC_DAIFMT_RIGHT_J, SND_SOC_DAIFMT_FORMAT_MASK }, + { "left_j", SND_SOC_DAIFMT_LEFT_J, SND_SOC_DAIFMT_FORMAT_MASK }, + { "dsp_a", SND_SOC_DAIFMT_DSP_A, SND_SOC_DAIFMT_FORMAT_MASK }, + { "dsp_b", SND_SOC_DAIFMT_DSP_B, SND_SOC_DAIFMT_FORMAT_MASK }, + { "ac97", SND_SOC_DAIFMT_AC97, SND_SOC_DAIFMT_FORMAT_MASK }, + { "pdm", SND_SOC_DAIFMT_PDM, SND_SOC_DAIFMT_FORMAT_MASK}, + { "msb", SND_SOC_DAIFMT_MSB, SND_SOC_DAIFMT_FORMAT_MASK }, + { "lsb", SND_SOC_DAIFMT_LSB, SND_SOC_DAIFMT_FORMAT_MASK }, + { "cont", SND_SOC_DAIFMT_CONT, SND_SOC_DAIFMT_CLOCK_MASK }, + { "gated", SND_SOC_DAIFMT_GATED, SND_SOC_DAIFMT_CLOCK_MASK }, + /* SND_SOC_DAIFMT_NB_NF is default */ + { "nb_if", SND_SOC_DAIFMT_NB_IF, SND_SOC_DAIFMT_INV_MASK }, + { "ib_nf", SND_SOC_DAIFMT_IB_NF, SND_SOC_DAIFMT_INV_MASK }, + { "ib_if", SND_SOC_DAIFMT_IB_IF, SND_SOC_DAIFMT_INV_MASK }, + { "cbm_cfm", SND_SOC_DAIFMT_CBM_CFM, SND_SOC_DAIFMT_MASTER_MASK }, + { "cbs_cfm", SND_SOC_DAIFMT_CBS_CFM, SND_SOC_DAIFMT_MASTER_MASK }, + { "cbm_cfs", SND_SOC_DAIFMT_CBM_CFS, SND_SOC_DAIFMT_MASTER_MASK }, + { "cbs_cfs", SND_SOC_DAIFMT_CBS_CFS, SND_SOC_DAIFMT_MASTER_MASK }, + }; + + if (!prefix) + prefix = ""; + + /* + * it finds "[prefix]snd,soc,daifmt = xxx" from device_node, + * and set SND_SOC_DAIFMT_XXX + */ + snprintf(prop, sizeof(prop), "%ssnd,soc,daifmt", prefix); + num = of_property_count_strings(np, prop); + if (num < 0) + return num; + + format = 0; + for (i = 0; i < num; i++) { + ret = of_property_read_string_index(np, prop, i, &str); + if (ret < 0) + return ret; + + for (j = 0; j < ARRAY_SIZE(of_parse_table); j++) { + if (strcmp(str, of_parse_table[j].name) == 0) { + if (format & of_parse_table[j].msk) + return -EINVAL; + + format |= of_parse_table[j].val; + } + } + } + + *fmt = format; + + return num; +} +EXPORT_SYMBOL_GPL(snd_soc_of_parse_daifmt); + static int __init snd_soc_init(void) { #ifdef CONFIG_DEBUG_FS
On 12/05/2012 12:55 AM, Kuninori Morimoto wrote:
Hi Stephen Cc Mark
Thank you for your reply
- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"i2s", SND_SOC_DAIFMT_I2S);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"right_j", SND_SOC_DAIFMT_RIGHT_J);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"left_j", SND_SOC_DAIFMT_LEFT_J);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"dsp_a", SND_SOC_DAIFMT_DSP_A);- ret |= __snd_soc_of_parse_daifmt(np, prefix, fmt,
"dsp_b", SND_SOC_DAIFMT_DSP_B);I'd expect to see something more like:
fmt = of_property_read_u32_array(np, "bit-format");
Ahh... I see.
Well, once a DT binding is created, you can't change the numbers, or you would break the ability for an old DT to work with a newer kernel.
OK. I understand.
How about to use string ?
snd.soc.daifmt.format = "left_j" snd.soc.daifmt.clock_gate = "cont" snd.soc.daifmt.inversion = "ib_nf" snd.soc.daifmt.hw_clock = "cbs_cfm"
That's probably the best we can do for now. Using a pre-processor would be best:
#define SND_SOC_DAIFMT_LEFT_J 3
snd.soc.daifmt.format = <SND_SOC_DAIFMT_LEFT_J>;
... but we can't do that yet...
Thank you.
I tried v2 of snd_soc_of_parse_daifmt() which is using "string" and "array" style
[prefix]snd,soc,daifmt = "i2c", "nb_if", "cbm_cfm";
I assume you mean i2s not i2c there.
That seems to be overloading one property so that it contains a lot of separate data items. I'd expect separate properties for the format, the bitclock inversion, the frame inversion, the bitclock master, and the frame master.
Hi Stephen
I tried v2 of snd_soc_of_parse_daifmt() which is using "string" and "array" style
[prefix]snd,soc,daifmt = "i2c", "nb_if", "cbm_cfm";
I assume you mean i2s not i2c there.
Oops, indeed.
That seems to be overloading one property so that it contains a lot of separate data items. I'd expect separate properties for the format, the bitclock inversion, the frame inversion, the bitclock master, and the frame master.
OK. I'll try fix it.
Best regards --- Kuninori Morimoto
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/generic/simple-card.c | 90 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 3 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index b4b4cab..a59e88c 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -9,6 +9,7 @@ * published by the Free Software Foundation. */
+#include <linux/of.h> #include <linux/platform_device.h> #include <linux/module.h> #include <sound/simple_card.h> @@ -47,12 +48,89 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd) return 0; }
+static void asoc_simple_card_parse_of(struct device_node *np, + struct asoc_simple_card_info *cinfo, + struct device *dev) +{ + struct asoc_simple_dai_init_info *iinfo; + unsigned int cpu_daifmt = 0; + unsigned int codec_daifmt = 0; + unsigned int sysclk = 0; + + /* + * it will find + * + * iinfo,cpu,snd,soc,daifmt,xxx + * iinfo,codec,snd,soc,daifmt,xxx + * iinfo,sysclk + */ + snd_soc_of_parse_daifmt(np, "iinfo,cpu,", &cpu_daifmt); + snd_soc_of_parse_daifmt(np, "iinfo,codec,", &codec_daifmt); + of_property_read_u32(np, "iinfo,sysclk", &sysclk); + + if (cpu_daifmt || codec_daifmt || sysclk) { + iinfo = devm_kzalloc(dev, sizeof(*iinfo), GFP_KERNEL); + if (!iinfo) + return; + + cinfo->init = iinfo; + iinfo->cpu_daifmt = cpu_daifmt; + iinfo->codec_daifmt = codec_daifmt; + iinfo->sysclk = sysclk; + } + + /* + * it will find + * + * cinfo,xxx + */ + of_property_read_string(np, "cinfo,name", &cinfo->name); + of_property_read_string(np, "cinfo,card", &cinfo->card); + of_property_read_string(np, "cinfo,cpu_dai", &cinfo->cpu_dai); + of_property_read_string(np, "cinfo,codec", &cinfo->codec); + of_property_read_string(np, "cinfo,platform", &cinfo->platform); + of_property_read_string(np, "cinfo,codec_dai", &cinfo->codec_dai); + + /* + * debug info + */ + if (cinfo->name) + dev_dbg(dev, "name = %s\n", cinfo->name); + if (cinfo->card) + dev_dbg(dev, "card = %s\n", cinfo->card); + if (cinfo->cpu_dai) + dev_dbg(dev, "cpu_dai = %s\n", cinfo->cpu_dai); + if (cinfo->codec) + dev_dbg(dev, "codec = %s\n", cinfo->codec); + if (cinfo->platform) + dev_dbg(dev, "platform = %s\n", cinfo->platform); + if (cinfo->codec_dai) + dev_dbg(dev, "codec_dai = %s\n", cinfo->codec_dai); + if (iinfo && iinfo->cpu_daifmt) + dev_dbg(dev, "cpu_daifmt = %08x\n", iinfo->cpu_daifmt); + if (iinfo && iinfo->codec_daifmt) + dev_dbg(dev, "codec_daifmt = %08x\n", iinfo->codec_daifmt); + if (iinfo && iinfo->sysclk) + dev_dbg(dev, "iinfo,sysclk = %d\n", iinfo->sysclk); +} + static int asoc_simple_card_probe(struct platform_device *pdev) { - struct asoc_simple_card_info *cinfo = pdev->dev.platform_data; + struct asoc_simple_card_info *cinfo; + struct device_node *np = pdev->dev.of_node; + struct device *dev = &pdev->dev; + + cinfo = NULL; + if (np && of_device_is_available(np)) { + cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL); + if (cinfo) + asoc_simple_card_parse_of(np, cinfo, dev); + } else { + cinfo = pdev->dev.platform_data; + }
if (!cinfo) { - dev_err(&pdev->dev, "no info for asoc-simple-card\n"); + dev_err(dev, "no info for asoc-simple-card\n"); return -EINVAL; }
@@ -62,7 +140,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev) !cinfo->codec || !cinfo->platform || !cinfo->codec_dai) { - dev_err(&pdev->dev, "insufficient asoc_simple_card_info settings\n"); + dev_err(dev, "insufficient asoc_simple_card_info settings\n"); return -EINVAL; }
@@ -99,9 +177,15 @@ static int asoc_simple_card_remove(struct platform_device *pdev) return snd_soc_unregister_card(&cinfo->snd_card); }
+static const struct of_device_id asoc_simple_of_match[] = { + { .compatible = "asoc,simple-card", }, + {}, +}; + static struct platform_driver asoc_simple_card = { .driver = { .name = "asoc-simple-card", + .of_match_table = asoc_simple_of_match, }, .probe = asoc_simple_card_probe, .remove = asoc_simple_card_remove,
On 11/28/2012 09:32 PM, Kuninori Morimoto wrote:
This could benefit from a patch description.
A file in Documentation/devicetree/bindings is required to specify the format of the device tree.
Property name prefixes such as "cinfo," aren't very descriptive; what does that mean?
A property name prefix such as "asoc," sounds Linux- (ASoC-) specific; DT is supposed to represent HW, and hence shouldn't be influenced by OS naming, etc.
Looking at the code, I think the machine driver is binding to the other components by string name. With DT, it should be using phandles to point at them instead.
The sysclk value that's parsed from DT doesn't appear to be used. How does clocking work with this driver; what about when the sample-rate changes, etc.?
Hi Stephen
This could benefit from a patch description.
A file in Documentation/devicetree/bindings is required to specify the format of the device tree.
Property name prefixes such as "cinfo," aren't very descriptive; what does that mean?
A property name prefix such as "asoc," sounds Linux- (ASoC-) specific; DT is supposed to represent HW, and hence shouldn't be influenced by OS naming, etc.
Looking at the code, I think the machine driver is binding to the other components by string name. With DT, it should be using phandles to point at them instead.
I see. I fix it up. Thank you for your description.
Best regards --- Kuninori Morimoto
Hi Kuninori,
On 29.11.2012 05:32, Kuninori Morimoto wrote:
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/generic/simple-card.c | 90 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 3 deletions(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index b4b4cab..a59e88c 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -9,6 +9,7 @@
- published by the Free Software Foundation.
*/
+#include <linux/of.h> #include <linux/platform_device.h> #include <linux/module.h> #include <sound/simple_card.h> @@ -47,12 +48,89 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd) return 0; }
+static void asoc_simple_card_parse_of(struct device_node *np,
struct asoc_simple_card_info *cinfo,struct device *dev)+{
- struct asoc_simple_dai_init_info *iinfo;
- unsigned int cpu_daifmt = 0;
- unsigned int codec_daifmt = 0;
- unsigned int sysclk = 0;
- /*
* it will find** iinfo,cpu,snd,soc,daifmt,xxx* iinfo,codec,snd,soc,daifmt,xxx* iinfo,sysclk*/- snd_soc_of_parse_daifmt(np, "iinfo,cpu,", &cpu_daifmt);
- snd_soc_of_parse_daifmt(np, "iinfo,codec,", &codec_daifmt);
- of_property_read_u32(np, "iinfo,sysclk", &sysclk);
- if (cpu_daifmt || codec_daifmt || sysclk) {
iinfo = devm_kzalloc(dev, sizeof(*iinfo), GFP_KERNEL);if (!iinfo)return;cinfo->init = iinfo;iinfo->cpu_daifmt = cpu_daifmt;iinfo->codec_daifmt = codec_daifmt;iinfo->sysclk = sysclk;- }
- /*
* it will find** cinfo,xxx*/- of_property_read_string(np, "cinfo,name", &cinfo->name);
- of_property_read_string(np, "cinfo,card", &cinfo->card);
- of_property_read_string(np, "cinfo,cpu_dai", &cinfo->cpu_dai);
- of_property_read_string(np, "cinfo,codec", &cinfo->codec);
- of_property_read_string(np, "cinfo,platform", &cinfo->platform);
- of_property_read_string(np, "cinfo,codec_dai", &cinfo->codec_dai);
CPUs, codecs and platforms should be referenced by phandles rather than by string. The ASoC core is well prepared for this, by using the dai_link's *_of_node members.
- /*
* debug info*/- if (cinfo->name)
dev_dbg(dev, "name = %s\n", cinfo->name);- if (cinfo->card)
dev_dbg(dev, "card = %s\n", cinfo->card);- if (cinfo->cpu_dai)
dev_dbg(dev, "cpu_dai = %s\n", cinfo->cpu_dai);- if (cinfo->codec)
dev_dbg(dev, "codec = %s\n", cinfo->codec);- if (cinfo->platform)
dev_dbg(dev, "platform = %s\n", cinfo->platform);- if (cinfo->codec_dai)
dev_dbg(dev, "codec_dai = %s\n", cinfo->codec_dai);- if (iinfo && iinfo->cpu_daifmt)
dev_dbg(dev, "cpu_daifmt = %08x\n", iinfo->cpu_daifmt);- if (iinfo && iinfo->codec_daifmt)
dev_dbg(dev, "codec_daifmt = %08x\n", iinfo->codec_daifmt);- if (iinfo && iinfo->sysclk)
dev_dbg(dev, "iinfo,sysclk = %d\n", iinfo->sysclk);+}
static int asoc_simple_card_probe(struct platform_device *pdev) {
- struct asoc_simple_card_info *cinfo = pdev->dev.platform_data;
struct asoc_simple_card_info *cinfo;
struct device_node *np = pdev->dev.of_node;
struct device *dev = &pdev->dev;
cinfo = NULL;
if (np && of_device_is_available(np)) {
cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);if (cinfo)asoc_simple_card_parse_of(np, cinfo, dev);} else {
cinfo = pdev->dev.platform_data;}
if (!cinfo) {
dev_err(&pdev->dev, "no info for asoc-simple-card\n");
dev_err(dev, "no info for asoc-simple-card\n");@@ -62,7 +140,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev) !cinfo->codec || !cinfo->platform || !cinfo->codec_dai) {
dev_err(&pdev->dev, "insufficient asoc_simple_card_info settings\n");
dev_err(dev, "insufficient asoc_simple_card_info settings\n");
That's an unrelated change that should probably go into a separate commit.
Daniel
Hi Daniel
Thank you for checking this patch.
- of_property_read_string(np, "cinfo,name", &cinfo->name);
- of_property_read_string(np, "cinfo,card", &cinfo->card);
- of_property_read_string(np, "cinfo,cpu_dai", &cinfo->cpu_dai);
- of_property_read_string(np, "cinfo,codec", &cinfo->codec);
- of_property_read_string(np, "cinfo,platform", &cinfo->platform);
- of_property_read_string(np, "cinfo,codec_dai", &cinfo->codec_dai);
CPUs, codecs and platforms should be referenced by phandles rather than by string. The ASoC core is well prepared for this, by using the dai_link's *_of_node members.
OK. I fix it in v2
@@ -62,7 +140,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev) !cinfo->codec || !cinfo->platform || !cinfo->codec_dai) {
dev_err(&pdev->dev, "insufficient asoc_simple_card_info settings\n");
dev_err(dev, "insufficient asoc_simple_card_info settings\n");That's an unrelated change that should probably go into a separate commit.
OK. I'll do it in v2
Best regards --- Kuninori Morimoto
participants (4)
-
Daniel Mack -
Kuninori Morimoto -
Mark Brown -
Stephen Warren