[PATCH 0/2][RFC] snd_soc_codec include snd_soc_component

[PATCH 0/2][RFC] snd_soc_codec include snd_soc_component

[Kuninori Morimoto]

Hi Mark, Lars

These are very rough patch for
merging snd_soc_codec <-> snd_soc_component.
As Lars idea, "codec" includes "component" on this patch,
but it doesn't care about "platform" here,
since "platform" doesn't have "dai".
Current component is caring "dai", but... ?
(and what about "card" ?)

Anyway, this patch focus only codec <-> component.
The "driver code" was not big difference,
since current component is very small at this point.

Attached 1st patch is rough version of ASoC core code.
the main topic on this patch is that snd_soc_register_dai[s]
will be called via component.

But, we will need many "codec dai name exchange", becouse
 1) the dai name rule on 1-dai / multi-dai are different,
    it was depend on snd_soc_register_dai[s].
 1) component is using both snd_soc_register_dai[s] inside
    (this was for "cpu" side limit)
 2) codec have been used snd_soc_register_dais even though
    it was not multi-dai

2nd patch is rough version of "code dai name exchange" for it.
Of course we need more and more codec driver patches,
but it is wm8978 only as trial now.

I guess "codec driver" should have "component driver" pointer too ?
(it has .name only at this point, and 2nd patch includes it)

We can move some feature from codec to component
after this step ?

In my quick check,
below codec have 0 dai (= we need care it on component ?)

	sound/soc/codecs/lm4857.c
	sound/soc/codecs/max9768.c
	sound/soc/codecs/max9877.c
	sound/soc/codecs/wm2000.c
	sound/soc/codecs/wm9090.c

below codecs have 1 dai (= we need name exchange on platform)

	sound/soc/codecs/ac97.c
	sound/soc/codecs/ad1836.c
	sound/soc/codecs/ad193x.c
	sound/soc/codecs/ad193x.c
	sound/soc/codecs/ad1980.c
	sound/soc/codecs/ad73311.c
	sound/soc/codecs/adau1701.c
	sound/soc/codecs/ads117x.c
	sound/soc/codecs/ak4104.c
	sound/soc/codecs/ak4535.c
	sound/soc/codecs/ak4554.c
	sound/soc/codecs/ak4642.c
	sound/soc/codecs/ak4671.c
	sound/soc/codecs/ak5386.c
	sound/soc/codecs/alc5623.c
	sound/soc/codecs/alc5632.c
	sound/soc/codecs/bt-sco.c
	sound/soc/codecs/cq93vc.c
	sound/soc/codecs/cs4270.c
	sound/soc/codecs/cs4271.c
	sound/soc/codecs/cs4271.c
	sound/soc/codecs/cs42l51.c
	sound/soc/codecs/cs42l52.c
	sound/soc/codecs/cx20442.c
	sound/soc/codecs/da7210.c
	sound/soc/codecs/da7210.c
	sound/soc/codecs/da7213.c
	sound/soc/codecs/da9055.c
	sound/soc/codecs/dmic.c
	sound/soc/codecs/hdmi.c
	sound/soc/codecs/jz4740.c
	sound/soc/codecs/max9850.c
	sound/soc/codecs/ml26124.c
	sound/soc/codecs/pcm1681.c
	sound/soc/codecs/pcm1792a.c
	sound/soc/codecs/pcm3008.c
	sound/soc/codecs/sgtl5000.c
	sound/soc/codecs/si476x.c
	sound/soc/codecs/spdif_receiver.c
	sound/soc/codecs/spdif_transmitter.c
	sound/soc/codecs/ssm2518.c
	sound/soc/codecs/ssm2602.c
	sound/soc/codecs/ssm2602.c
	sound/soc/codecs/sta32x.c
	sound/soc/codecs/sta529.c
	sound/soc/codecs/tas5086.c
	sound/soc/codecs/tlv320aic23.c
	sound/soc/codecs/tlv320aic26.c
	sound/soc/codecs/tlv320aic32x4.c
	sound/soc/codecs/tlv320aic3x.c
	sound/soc/codecs/tlv320dac33.c
	sound/soc/codecs/uda134x.c
	sound/soc/codecs/wl1273.c
	sound/soc/codecs/wm1250-ev1.c
	sound/soc/codecs/wm2200.c
	sound/soc/codecs/wm8350.c
	sound/soc/codecs/wm8400.c
	sound/soc/codecs/wm8510.c
	sound/soc/codecs/wm8510.c
	sound/soc/codecs/wm8523.c
	sound/soc/codecs/wm8711.c
	sound/soc/codecs/wm8711.c
	sound/soc/codecs/wm8727.c
	sound/soc/codecs/wm8728.c
	sound/soc/codecs/wm8728.c
	sound/soc/codecs/wm8731.c
	sound/soc/codecs/wm8731.c
	sound/soc/codecs/wm8737.c
	sound/soc/codecs/wm8737.c
	sound/soc/codecs/wm8741.c
	sound/soc/codecs/wm8741.c
	sound/soc/codecs/wm8750.c
	sound/soc/codecs/wm8750.c
	sound/soc/codecs/wm8770.c
	sound/soc/codecs/wm8782.c
	sound/soc/codecs/wm8804.c
	sound/soc/codecs/wm8804.c
	sound/soc/codecs/wm8900.c
	sound/soc/codecs/wm8900.c
	sound/soc/codecs/wm8903.c
	sound/soc/codecs/wm8904.c
	sound/soc/codecs/wm8940.c
	sound/soc/codecs/wm8955.c
	sound/soc/codecs/wm8960.c
	sound/soc/codecs/wm8961.c
	sound/soc/codecs/wm8962.c
	sound/soc/codecs/wm8971.c
	sound/soc/codecs/wm8974.c
	sound/soc/codecs/wm8978.c
	sound/soc/codecs/wm8983.c
	sound/soc/codecs/wm8983.c
	sound/soc/codecs/wm8985.c
	sound/soc/codecs/wm8985.c
	sound/soc/codecs/wm8988.c
	sound/soc/codecs/wm8988.c
	sound/soc/codecs/wm8990.c
	sound/soc/codecs/wm8991.c
	sound/soc/codecs/wm8993.c
	sound/soc/codecs/wm9081.c
	sound/soc/codecs/max98090.c
	sound/soc/codecs/rt5631.c

below codecs have multi dai

	sound/soc/codecs/88pm860x-codec.c
	sound/soc/codecs/ab8500-codec.c
	sound/soc/codecs/adau1373.c
	sound/soc/codecs/adav80x.c
	sound/soc/codecs/ak4641.c
	sound/soc/codecs/cs42l73.c
	sound/soc/codecs/da732x.c
	sound/soc/codecs/isabelle.c
	sound/soc/codecs/lm49453.c
	sound/soc/codecs/max98088.c
	sound/soc/codecs/max98095.c
	sound/soc/codecs/rt5640.c
	sound/soc/codecs/sn95031.c
	sound/soc/codecs/stac9766.c
	sound/soc/codecs/twl4030.c
	sound/soc/codecs/twl6040.c
	sound/soc/codecs/uda1380.c
	sound/soc/codecs/wm0010.c
	sound/soc/codecs/wm5100.c
	sound/soc/codecs/wm5102.c
	sound/soc/codecs/wm5110.c
	sound/soc/codecs/wm8580.c
	sound/soc/codecs/wm8753.c
	sound/soc/codecs/wm8776.c
	sound/soc/codecs/wm8994.c
	sound/soc/codecs/wm8995.c
	sound/soc/codecs/wm8996.c
	sound/soc/codecs/wm8997.c
	sound/soc/codecs/wm9705.c
	sound/soc/codecs/wm9712.c
	sound/soc/codecs/wm9713.c

and, below codec has both 1-dai, and multi-dai

	sound/soc/codecs/mc13783.c



Kuninori Morimoto (2):
      [RFC] ASoC: snd_soc_codec include snd_soc_component
      [RFC] ASoC: wm8978: use component

 arch/arm/mach-shmobile/board-armadillo800eva.c |    2 +-
 include/sound/soc.h                            |   33 ++++++++++--------
 sound/soc/codecs/wm8978.c                      |    6 ++++
 sound/soc/soc-core.c                           |   44 ++++++++++++++++++------
 4 files changed, 59 insertions(+), 26 deletions(-)

[PATCH 1/2][RFC] ASoC: snd_soc_codec include snd_soc_component

[Kuninori Morimoto]

This is rough version patch for merging codec <-> component.
The main topic is that snd_soc_register_dais() will be called
via component.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h  |   33 +++++++++++++++++++--------------
 sound/soc/soc-core.c |   44 +++++++++++++++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 8e2ad52..e312b97 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -670,6 +670,21 @@ struct snd_soc_cache_ops {
 	int (*sync)(struct snd_soc_codec *codec);
 };
 
+/* component interface */
+struct snd_soc_component_driver {
+	const char *name;
+};
+
+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;
+};
+
 /* SoC Audio Codec device */
 struct snd_soc_codec {
 	const char *name;
@@ -717,6 +732,9 @@ struct snd_soc_codec {
 	struct mutex cache_rw_mutex;
 	int val_bytes;
 
+	/* component */
+	struct snd_soc_component component;
+
 	/* dapm */
 	struct snd_soc_dapm_context dapm;
 	unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */
@@ -736,6 +754,7 @@ struct snd_soc_codec_driver {
 	int (*remove)(struct snd_soc_codec *);
 	int (*suspend)(struct snd_soc_codec *);
 	int (*resume)(struct snd_soc_codec *);
+	struct snd_soc_component_driver *component_driver;
 
 	/* Default control and setup, added after probe() is run */
 	const struct snd_kcontrol_new *controls;
@@ -853,20 +872,6 @@ struct snd_soc_platform {
 #endif
 };
 
-struct snd_soc_component_driver {
-	const char *name;
-};
-
-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 7d13c4f..1b76ef0 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -4154,6 +4154,11 @@ static void fixup_codec_formats(struct snd_soc_pcm_stream *stream)
 			stream->formats |= codec_format_map[i];
 }
 
+static int __snd_soc_register_component(struct device *dev,
+				 struct snd_soc_component *cmpnt,
+				 const struct snd_soc_component_driver *cmpnt_drv,
+				 struct snd_soc_dai_driver *dai_drv,
+				 int num_dai);
 /**
  * snd_soc_register_codec - Register a codec with the ASoC core
  *
@@ -4240,10 +4245,12 @@ int snd_soc_register_codec(struct device *dev,
 	list_add(&codec->list, &codec_list);
 	mutex_unlock(&client_mutex);
 
-	/* register any DAIs */
-	ret = snd_soc_register_dais(dev, dai_drv, num_dai);
+	/* register component */
+	ret = __snd_soc_register_component(dev, &codec->component,
+					   codec_drv->component_driver,
+					   dai_drv, num_dai);
 	if (ret < 0) {
-		dev_err(codec->dev, "ASoC: Failed to regster DAIs: %d\n", ret);
+		dev_err(codec->dev, "ASoC: Failed to regster component: %d\n", ret);
 		goto fail_codec_name;
 	}
 
@@ -4293,24 +4300,23 @@ 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)
+static int
+__snd_soc_register_component(struct device *dev,
+			     struct snd_soc_component *cmpnt,
+			     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");
+		dev_err(dev, "ASoC: Failed to connecting component\n");
 		return -ENOMEM;
 	}
 
@@ -4351,6 +4357,22 @@ error_component_name:
 
 	return ret;
 }
+
+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;
+
+	cmpnt = devm_kzalloc(dev, sizeof(*cmpnt), GFP_KERNEL);
+	if (!cmpnt) {
+		dev_err(dev, "ASoC: Failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	return __snd_soc_register_component(dev, cmpnt, cmpnt_drv, dai_drv, num_dai);
+}
 EXPORT_SYMBOL_GPL(snd_soc_register_component);
 
 /**
-- 
1.7.9.5

[PATCH 2/2][RFC] ASoC: wm8978: use component

[Kuninori Morimoto]

This is very rough patch for maging codec <-> component.
we need exchange codec_dai name on platform
if codec driver was 1-dai.

this patch includes component_driver on codec.
it has .name only now...

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 arch/arm/mach-shmobile/board-armadillo800eva.c |    2 +-
 sound/soc/codecs/wm8978.c                      |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index c5be60d..dd2b9e0 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -950,7 +950,7 @@ static struct asoc_simple_card_info fsi_wm8978_info = {
 		.fmt	= SND_SOC_DAIFMT_CBS_CFS | SND_SOC_DAIFMT_IB_NF,
 	},
 	.codec_dai = {
-		.name	= "wm8978-hifi",
+		.name	= "wm8978.0-001a",
 		.fmt	= SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_NB_NF,
 		.sysclk	= 12288000,
 	},
diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c
index d8fc531..352f8ff 100644
--- a/sound/soc/codecs/wm8978.c
+++ b/sound/soc/codecs/wm8978.c
@@ -1009,6 +1009,10 @@ static int wm8978_remove(struct snd_soc_codec *codec)
 	return 0;
 }
 
+static struct snd_soc_component_driver soc_component_dev = {
+	.name = "wm8978",
+};
+
 static struct snd_soc_codec_driver soc_codec_dev_wm8978 = {
 	.probe =	wm8978_probe,
 	.remove =	wm8978_remove,
@@ -1022,6 +1026,8 @@ static struct snd_soc_codec_driver soc_codec_dev_wm8978 = {
 	.num_dapm_widgets = ARRAY_SIZE(wm8978_dapm_widgets),
 	.dapm_routes = wm8978_dapm_routes,
 	.num_dapm_routes = ARRAY_SIZE(wm8978_dapm_routes),
+
+	.component_driver = &soc_component_dev,
 };
 
 static const struct regmap_config wm8978_regmap_config = {
-- 
1.7.9.5

Re: [PATCH 0/2][RFC] snd_soc_codec include snd_soc_component

[Lars-Peter Clausen]

On 09/02/2013 08:02 AM, Kuninori Morimoto wrote:
> 
> Hi Mark, Lars
> 
> These are very rough patch for
> merging snd_soc_codec <-> snd_soc_component.
> As Lars idea, "codec" includes "component" on this patch,
> but it doesn't care about "platform" here,
> since "platform" doesn't have "dai".
> Current component is caring "dai", but... ?
> (and what about "card" ?)
> 
> Anyway, this patch focus only codec <-> component.
> The "driver code" was not big difference,
> since current component is very small at this point.
> 
> Attached 1st patch is rough version of ASoC core code.
> the main topic on this patch is that snd_soc_register_dai[s]
> will be called via component.
> 
> But, we will need many "codec dai name exchange", becouse
>  1) the dai name rule on 1-dai / multi-dai are different,
>     it was depend on snd_soc_register_dai[s].
>  1) component is using both snd_soc_register_dai[s] inside
>     (this was for "cpu" side limit)
>  2) codec have been used snd_soc_register_dais even though
>     it was not multi-dai
> 

We only added the distinction between components with one DAIs and multiple
DAIs since most of the CPU DAI drivers with only one DAI used
snd_soc_register_dai() and we wanted to avoid having to update all the DAI
links. How about adding a boolean flag to __snd_soc_register_component()
which if set causes the function to always use snd_soc_register_dais(). This
will allow us to keep the existing naming for CODEC dai.


> 2nd patch is rough version of "code dai name exchange" for it.
> Of course we need more and more codec driver patches,
> but it is wm8978 only as trial now.
> 
> I guess "codec driver" should have "component driver" pointer too ?
> (it has .name only at this point, and 2nd patch includes it)

In my opinion it is better to embed the component_driver struct into the
codec_driver struct just like the component struct is embedded in the codec
struct. Otherwise your patch looks fine to me. Maybe move
__snd_soc_register_component() up, so the forward declaration is not needed
anymore.

> 
> We can move some feature from codec to component
> after this step ?
> 

Yes. I think most of the fields managed by the core which are not touched by
any drivers can be moved easily.

- Lars

Re: [PATCH 0/2][RFC] snd_soc_codec include snd_soc_component

[Kuninori Morimoto]

Hi Lars

Thank you for your feedback

> > But, we will need many "codec dai name exchange", becouse
> >  1) the dai name rule on 1-dai / multi-dai are different,
> >     it was depend on snd_soc_register_dai[s].
> >  1) component is using both snd_soc_register_dai[s] inside
> >     (this was for "cpu" side limit)
> >  2) codec have been used snd_soc_register_dais even though
> >     it was not multi-dai
> > 
> 
> We only added the distinction between components with one DAIs and multiple
> DAIs since most of the CPU DAI drivers with only one DAI used
> snd_soc_register_dai() and we wanted to avoid having to update all the DAI
> links. How about adding a boolean flag to __snd_soc_register_component()
> which if set causes the function to always use snd_soc_register_dais(). This
> will allow us to keep the existing naming for CODEC dai.

It is easy hack.
OK, we can try it.

> > 2nd patch is rough version of "code dai name exchange" for it.
> > Of course we need more and more codec driver patches,
> > but it is wm8978 only as trial now.
> > 
> > I guess "codec driver" should have "component driver" pointer too ?
> > (it has .name only at this point, and 2nd patch includes it)
> 
> In my opinion it is better to embed the component_driver struct into the
> codec_driver struct just like the component struct is embedded in the codec
> struct. Otherwise your patch looks fine to me. Maybe move

I see.

> __snd_soc_register_component() up, so the forward declaration is not needed
> anymore.

This was the reason why I used "rough" naming here :P

> > We can move some feature from codec to component
> > after this step ?
> > 
> 
> Yes. I think most of the fields managed by the core which are not touched by
> any drivers can be moved easily.

Nice !


As my previous email said, this patch cares only codec <-> component,
but we re-consider about "component"
if component will be used from "platform" (and "card" too ?) ?
(since it is controlling "dai" inside)
Or is it 2nd step like above ?

I can say current style is quick-hack, but it is easy to go to next step.
I'm not sure this is good plan

Best regards
---
Kuninori Morimoto

Re: [PATCH 0/2][RFC] snd_soc_codec include snd_soc_component

[Lars-Peter Clausen]

On 09/02/2013 10:52 AM, Kuninori Morimoto wrote:
> 
> Hi Lars
> 
> Thank you for your feedback
> 
>>> But, we will need many "codec dai name exchange", becouse
>>>  1) the dai name rule on 1-dai / multi-dai are different,
>>>     it was depend on snd_soc_register_dai[s].
>>>  1) component is using both snd_soc_register_dai[s] inside
>>>     (this was for "cpu" side limit)
>>>  2) codec have been used snd_soc_register_dais even though
>>>     it was not multi-dai
>>>
>>
>> We only added the distinction between components with one DAIs and multiple
>> DAIs since most of the CPU DAI drivers with only one DAI used
>> snd_soc_register_dai() and we wanted to avoid having to update all the DAI
>> links. How about adding a boolean flag to __snd_soc_register_component()
>> which if set causes the function to always use snd_soc_register_dais(). This
>> will allow us to keep the existing naming for CODEC dai.
> 
> It is easy hack.
> OK, we can try it.
> 
>>> 2nd patch is rough version of "code dai name exchange" for it.
>>> Of course we need more and more codec driver patches,
>>> but it is wm8978 only as trial now.
>>>
>>> I guess "codec driver" should have "component driver" pointer too ?
>>> (it has .name only at this point, and 2nd patch includes it)
>>
>> In my opinion it is better to embed the component_driver struct into the
>> codec_driver struct just like the component struct is embedded in the codec
>> struct. Otherwise your patch looks fine to me. Maybe move
> 
> I see.
> 
>> __snd_soc_register_component() up, so the forward declaration is not needed
>> anymore.
> 
> This was the reason why I used "rough" naming here :P
> 
>>> We can move some feature from codec to component
>>> after this step ?
>>>
>>
>> Yes. I think most of the fields managed by the core which are not touched by
>> any drivers can be moved easily.
> 
> Nice !
> 
> 
> As my previous email said, this patch cares only codec <-> component,
> but we re-consider about "component"
> if component will be used from "platform" (and "card" too ?) ?
> (since it is controlling "dai" inside)
> Or is it 2nd step like above ?
> 

I think it can be done as a second step, as long as we keep in mind that we
want to do this eventually and don't built up any road blocks that will
prevent us form doing so.

- Lars

Re: [PATCH 0/2][RFC] snd_soc_codec include snd_soc_component

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 781 bytes --]

On Mon, Sep 02, 2013 at 11:34:15AM +0200, Lars-Peter Clausen wrote:
> On 09/02/2013 10:52 AM, Kuninori Morimoto wrote:

> > As my previous email said, this patch cares only codec <-> component,
> > but we re-consider about "component"
> > if component will be used from "platform" (and "card" too ?) ?
> > (since it is controlling "dai" inside)
> > Or is it 2nd step like above ?

> I think it can be done as a second step, as long as we keep in mind that we
> want to do this eventually and don't built up any road blocks that will
> prevent us form doing so.

Yes, this is the approach I'd take too - just move things into the
component drivers gradually.  So long as we don't cause problems for
ourselves further down the road any improvements we make now mean less
work later.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/2][RFC] snd_soc_codec include snd_soc_component

[Kuninori Morimoto]

Hi Lars, Mark


Thank you for your feedback

> > > As my previous email said, this patch cares only codec <-> component,
> > > but we re-consider about "component"
> > > if component will be used from "platform" (and "card" too ?) ?
> > > (since it is controlling "dai" inside)
> > > Or is it 2nd step like above ?
> 
> > I think it can be done as a second step, as long as we keep in mind that we
> > want to do this eventually and don't built up any road blocks that will
> > prevent us form doing so.
> 
> Yes, this is the approach I'd take too - just move things into the
> component drivers gradually.  So long as we don't cause problems for
> ourselves further down the road any improvements we make now mean less
> work later.

It is my favorite style too.
OK, I will re-try codec <-> component step,
and report it again.
Thank you

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 0/2][RFC] snd_soc_codec include snd_soc_component

[Kuninori Morimoto, :]

Hi Mark, Lars

This is 2nd version of codec <-> component merge patch.
Basically, 1st patch is almost same as v1 version,
but the differences are...
 - "component" moves to upside of "codec" (to avoid extra declaration)
 - register_component had "allow_single_dai" flag
   to control dai/dais function.
   Now, we can keep codec dai name

2nd patch adds component_driver on wm8978 (as trial).
It is not needed at this point, since current component doesn't
check whether component_driver was NULL.
But we will need/use it in the future.

What do you think about this series ?
I can send "use component" patch for all codecs
if you want me it.

Kuninori Morimoto (2):
      [RFC] ASoC: snd_soc_codec includes snd_soc_component
      [RFC] ASoC: wm8978: use component

 include/sound/soc.h       |   33 ++++----
 sound/soc/codecs/wm8978.c |    4 +
 sound/soc/soc-core.c      |  200 +++++++++++++++++++++++++--------------------
 3 files changed, 134 insertions(+), 103 deletions(-)

Best regards
---
Kuninori Morimoto

[PATCH v2 1/2][RFC] ASoC: snd_soc_codec includes snd_soc_component

[Kuninori Morimoto]

Codec includes component by this patch,
and component moved to upside of codec
to avoid extra declaration.
Codec dai will be registered via component
by this patch.

Current component register function
is used for cpu, and it is using
dai/dais functions properly to keep
existing cpu dai name.

And now, it will be used from codec also.
But codec driver had been used dais function only
even though it was single dai.
This patch adds new flag which can selects
dai/dais function on component register
function to keep existing codec dai name.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h  |   33 +++++----
 sound/soc/soc-core.c |  200 ++++++++++++++++++++++++++++----------------------
 2 files changed, 130 insertions(+), 103 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 8e2ad52..28e2d6a 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -670,6 +670,21 @@ struct snd_soc_cache_ops {
 	int (*sync)(struct snd_soc_codec *codec);
 };
 
+/* component interface */
+struct snd_soc_component_driver {
+	const char *name;
+};
+
+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;
+};
+
 /* SoC Audio Codec device */
 struct snd_soc_codec {
 	const char *name;
@@ -717,6 +732,9 @@ struct snd_soc_codec {
 	struct mutex cache_rw_mutex;
 	int val_bytes;
 
+	/* component */
+	struct snd_soc_component component;
+
 	/* dapm */
 	struct snd_soc_dapm_context dapm;
 	unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */
@@ -736,6 +754,7 @@ struct snd_soc_codec_driver {
 	int (*remove)(struct snd_soc_codec *);
 	int (*suspend)(struct snd_soc_codec *);
 	int (*resume)(struct snd_soc_codec *);
+	struct snd_soc_component_driver component_driver;
 
 	/* Default control and setup, added after probe() is run */
 	const struct snd_kcontrol_new *controls;
@@ -853,20 +872,6 @@ struct snd_soc_platform {
 #endif
 };
 
-struct snd_soc_component_driver {
-	const char *name;
-};
-
-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 7d13c4f..62e0b4c 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -4019,6 +4019,112 @@ static void snd_soc_unregister_dais(struct device *dev, size_t count)
 }
 
 /**
+ * snd_soc_register_component - Register a component with the ASoC core
+ *
+ */
+static int
+__snd_soc_register_component(struct device *dev,
+			     struct snd_soc_component *cmpnt,
+			     const struct snd_soc_component_driver *cmpnt_drv,
+			     struct snd_soc_dai_driver *dai_drv,
+			     int num_dai, bool allow_single_dai)
+{
+	int ret;
+
+	dev_dbg(dev, "component register %s\n", dev_name(dev));
+
+	if (!cmpnt) {
+		dev_err(dev, "ASoC: Failed to connecting component\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;
+
+	/*
+	 * snd_soc_register_dai()  uses fmt_single_name(), and
+	 * snd_soc_register_dais() uses fmt_multiple_name()
+	 * for dai->name which is used for name based matching
+	 *
+	 * this function is used from cpu/codec.
+	 * allow_single_dai flag can ignore "codec" driver reworking
+	 * since it had been used snd_soc_register_dais(),
+	 */
+	if ((1 == num_dai) && allow_single_dai)
+		ret = snd_soc_register_dai(dev, dai_drv);
+	else
+		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;
+}
+
+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;
+
+	cmpnt = devm_kzalloc(dev, sizeof(*cmpnt), GFP_KERNEL);
+	if (!cmpnt) {
+		dev_err(dev, "ASoC: Failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	return __snd_soc_register_component(dev, cmpnt, cmpnt_drv,
+					    dai_drv, num_dai, true);
+}
+EXPORT_SYMBOL_GPL(snd_soc_register_component);
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
+
+/**
  * snd_soc_add_platform - Add a platform to the ASoC core
  * @dev: The parent device for the platform
  * @platform: The platform to add
@@ -4240,10 +4346,12 @@ int snd_soc_register_codec(struct device *dev,
 	list_add(&codec->list, &codec_list);
 	mutex_unlock(&client_mutex);
 
-	/* register any DAIs */
-	ret = snd_soc_register_dais(dev, dai_drv, num_dai);
+	/* register component */
+	ret = __snd_soc_register_component(dev, &codec->component,
+					   &codec_drv->component_driver,
+					   dai_drv, num_dai, false);
 	if (ret < 0) {
-		dev_err(codec->dev, "ASoC: Failed to regster DAIs: %d\n", ret);
+		dev_err(codec->dev, "ASoC: Failed to regster component: %d\n", ret);
 		goto fail_codec_name;
 	}
 
@@ -4293,92 +4401,6 @@ 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;
-
-	/*
-	 * snd_soc_register_dai()  uses fmt_single_name(), and
-	 * snd_soc_register_dais() uses fmt_multiple_name()
-	 * for dai->name which is used for name based matching
-	 */
-	if (1 == num_dai)
-		ret = snd_soc_register_dai(dev, dai_drv);
-	else
-		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;
-}
-EXPORT_SYMBOL_GPL(snd_soc_register_component);
-
-/**
- * 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);
-}
-EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
-
 /* Retrieve a card's name from device tree */
 int snd_soc_of_parse_card_name(struct snd_soc_card *card,
 			       const char *propname)
-- 
1.7.9.5

[PATCH v2 2/2][RFC] ASoC: wm8978: use component

[Kuninori Morimoto]

wm8978 codec includes component by this patch.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/codecs/wm8978.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c
index d8fc531..71a04ab 100644
--- a/sound/soc/codecs/wm8978.c
+++ b/sound/soc/codecs/wm8978.c
@@ -1022,6 +1022,10 @@ static struct snd_soc_codec_driver soc_codec_dev_wm8978 = {
 	.num_dapm_widgets = ARRAY_SIZE(wm8978_dapm_widgets),
 	.dapm_routes = wm8978_dapm_routes,
 	.num_dapm_routes = ARRAY_SIZE(wm8978_dapm_routes),
+
+	.component_driver = {
+		.name = "wm8978",
+	},
 };
 
 static const struct regmap_config wm8978_regmap_config = {
-- 
1.7.9.5

Re: [PATCH v2 0/2][RFC] snd_soc_codec include snd_soc_component

[Kuninori Morimoto]

Hi Mark, Lars

# Am I missed sending email ??
# This is re-try

This is 2nd version of codec <-> component merge patch.
Basically, 1st patch is almost same as v1 version,
but the differences are...
 - "component" moves to upside of "codec" (to avoid extra declaration)
 - register_component had "allow_single_dai" flag
   to control dai/dais function.
   Now, we can keep codec dai name

2nd patch adds component_driver on wm8978 (as trial).
It is not needed at this point, since current component doesn't
check whether component_driver was NULL.
But we will need/use it in the future.

What do you think about this series ?
I can send "use component" patch for all codecs
if you want me it.

Kuninori Morimoto (2):
      [RFC] ASoC: snd_soc_codec includes snd_soc_component
      [RFC] ASoC: wm8978: use component

 include/sound/soc.h       |   33 ++++----
 sound/soc/codecs/wm8978.c |    4 +
 sound/soc/soc-core.c      |  200 +++++++++++++++++++++++++--------------------
 3 files changed, 134 insertions(+), 103 deletions(-)

Best regards
---
Kuninori Morimoto

[PATCH v2 1/2][RFC] ASoC: snd_soc_codec includes snd_soc_component

[Kuninori Morimoto]

Codec includes component by this patch,
and component moved to upside of codec
to avoid extra declaration.
Codec dai will be registered via component
by this patch.

Current component register function
is used for cpu, and it is using
dai/dais functions properly to keep
existing cpu dai name.

And now, it will be used from codec also.
But codec driver had been used dais function only
even though it was single dai.
This patch adds new flag which can selects
dai/dais function on component register
function to keep existing codec dai name.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h  |   33 +++++----
 sound/soc/soc-core.c |  200 ++++++++++++++++++++++++++++----------------------
 2 files changed, 130 insertions(+), 103 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 8e2ad52..28e2d6a 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -670,6 +670,21 @@ struct snd_soc_cache_ops {
 	int (*sync)(struct snd_soc_codec *codec);
 };
 
+/* component interface */
+struct snd_soc_component_driver {
+	const char *name;
+};
+
+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;
+};
+
 /* SoC Audio Codec device */
 struct snd_soc_codec {
 	const char *name;
@@ -717,6 +732,9 @@ struct snd_soc_codec {
 	struct mutex cache_rw_mutex;
 	int val_bytes;
 
+	/* component */
+	struct snd_soc_component component;
+
 	/* dapm */
 	struct snd_soc_dapm_context dapm;
 	unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */
@@ -736,6 +754,7 @@ struct snd_soc_codec_driver {
 	int (*remove)(struct snd_soc_codec *);
 	int (*suspend)(struct snd_soc_codec *);
 	int (*resume)(struct snd_soc_codec *);
+	struct snd_soc_component_driver component_driver;
 
 	/* Default control and setup, added after probe() is run */
 	const struct snd_kcontrol_new *controls;
@@ -853,20 +872,6 @@ struct snd_soc_platform {
 #endif
 };
 
-struct snd_soc_component_driver {
-	const char *name;
-};
-
-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 7d13c4f..62e0b4c 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -4019,6 +4019,112 @@ static void snd_soc_unregister_dais(struct device *dev, size_t count)
 }
 
 /**
+ * snd_soc_register_component - Register a component with the ASoC core
+ *
+ */
+static int
+__snd_soc_register_component(struct device *dev,
+			     struct snd_soc_component *cmpnt,
+			     const struct snd_soc_component_driver *cmpnt_drv,
+			     struct snd_soc_dai_driver *dai_drv,
+			     int num_dai, bool allow_single_dai)
+{
+	int ret;
+
+	dev_dbg(dev, "component register %s\n", dev_name(dev));
+
+	if (!cmpnt) {
+		dev_err(dev, "ASoC: Failed to connecting component\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;
+
+	/*
+	 * snd_soc_register_dai()  uses fmt_single_name(), and
+	 * snd_soc_register_dais() uses fmt_multiple_name()
+	 * for dai->name which is used for name based matching
+	 *
+	 * this function is used from cpu/codec.
+	 * allow_single_dai flag can ignore "codec" driver reworking
+	 * since it had been used snd_soc_register_dais(),
+	 */
+	if ((1 == num_dai) && allow_single_dai)
+		ret = snd_soc_register_dai(dev, dai_drv);
+	else
+		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;
+}
+
+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;
+
+	cmpnt = devm_kzalloc(dev, sizeof(*cmpnt), GFP_KERNEL);
+	if (!cmpnt) {
+		dev_err(dev, "ASoC: Failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	return __snd_soc_register_component(dev, cmpnt, cmpnt_drv,
+					    dai_drv, num_dai, true);
+}
+EXPORT_SYMBOL_GPL(snd_soc_register_component);
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
+
+/**
  * snd_soc_add_platform - Add a platform to the ASoC core
  * @dev: The parent device for the platform
  * @platform: The platform to add
@@ -4240,10 +4346,12 @@ int snd_soc_register_codec(struct device *dev,
 	list_add(&codec->list, &codec_list);
 	mutex_unlock(&client_mutex);
 
-	/* register any DAIs */
-	ret = snd_soc_register_dais(dev, dai_drv, num_dai);
+	/* register component */
+	ret = __snd_soc_register_component(dev, &codec->component,
+					   &codec_drv->component_driver,
+					   dai_drv, num_dai, false);
 	if (ret < 0) {
-		dev_err(codec->dev, "ASoC: Failed to regster DAIs: %d\n", ret);
+		dev_err(codec->dev, "ASoC: Failed to regster component: %d\n", ret);
 		goto fail_codec_name;
 	}
 
@@ -4293,92 +4401,6 @@ 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;
-
-	/*
-	 * snd_soc_register_dai()  uses fmt_single_name(), and
-	 * snd_soc_register_dais() uses fmt_multiple_name()
-	 * for dai->name which is used for name based matching
-	 */
-	if (1 == num_dai)
-		ret = snd_soc_register_dai(dev, dai_drv);
-	else
-		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;
-}
-EXPORT_SYMBOL_GPL(snd_soc_register_component);
-
-/**
- * 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);
-}
-EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
-
 /* Retrieve a card's name from device tree */
 int snd_soc_of_parse_card_name(struct snd_soc_card *card,
 			       const char *propname)
-- 
1.7.9.5

[PATCH v2 2/2][RFC] ASoC: wm8978: use component

[Kuninori Morimoto]

wm8978 codec includes component by this patch.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/codecs/wm8978.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c
index d8fc531..71a04ab 100644
--- a/sound/soc/codecs/wm8978.c
+++ b/sound/soc/codecs/wm8978.c
@@ -1022,6 +1022,10 @@ static struct snd_soc_codec_driver soc_codec_dev_wm8978 = {
 	.num_dapm_widgets = ARRAY_SIZE(wm8978_dapm_widgets),
 	.dapm_routes = wm8978_dapm_routes,
 	.num_dapm_routes = ARRAY_SIZE(wm8978_dapm_routes),
+
+	.component_driver = {
+		.name = "wm8978",
+	},
 };
 
 static const struct regmap_config wm8978_regmap_config = {
-- 
1.7.9.5

Re: [PATCH v2 0/2][RFC] snd_soc_codec include snd_soc_component

[Lars-Peter Clausen]

On 09/03/2013 03:51 AM, Kuninori Morimoto wrote:
> 
> Hi Mark, Lars
> 
> # Am I missed sending email ??
> # This is re-try

I got the mails from the first try as well.

> 
> This is 2nd version of codec <-> component merge patch.
> Basically, 1st patch is almost same as v1 version,
> but the differences are...
>  - "component" moves to upside of "codec" (to avoid extra declaration)
>  - register_component had "allow_single_dai" flag
>    to control dai/dais function.
>    Now, we can keep codec dai name
> 
> 2nd patch adds component_driver on wm8978 (as trial).
> It is not needed at this point, since current component doesn't
> check whether component_driver was NULL.
> But we will need/use it in the future.
> 
> What do you think about this series ?

Looks good to me. I think the only thing missing is a call to
snd_soc_component_unregister() from snd_soc_codec_unregister().

> I can send "use component" patch for all codecs
> if you want me it.

While nice to have I don't think it is a requirement to do this before we
can continue with the changes to the core. So if you want to do it go for
it, but it's also not a problem if it is not done.

- Lars

Re: [PATCH v2 0/2][RFC] snd_soc_codec include snd_soc_component

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]

On Tue, Sep 03, 2013 at 01:26:50PM +0200, Lars-Peter Clausen wrote:
> On 09/03/2013 03:51 AM, Kuninori Morimoto wrote:

> > I can send "use component" patch for all codecs
> > if you want me it.

> While nice to have I don't think it is a requirement to do this before we
> can continue with the changes to the core. So if you want to do it go for
> it, but it's also not a problem if it is not done.

I was going to add a devm_snd_soc_register_component() since so many
devices only need to do the CODEC unregistration in their remove
functions - it's probably best to wait until that's done then go through
and update them all once rather than having to go through twice.
Equally well it won't hurt.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 0/2][RFC] snd_soc_codec include snd_soc_component

[Kuninori Morimoto]

Hi Mark, Lars

Thank you for your feedback

> > > I can send "use component" patch for all codecs
> > > if you want me it.
> 
> > While nice to have I don't think it is a requirement to do this before we
> > can continue with the changes to the core. So if you want to do it go for
> > it, but it's also not a problem if it is not done.
> 
> I was going to add a devm_snd_soc_register_component() since so many
> devices only need to do the CODEC unregistration in their remove
> functions - it's probably best to wait until that's done then go through
> and update them all once rather than having to go through twice.
> Equally well it won't hurt.

I see.
OK I will send codec <-> component code only at this point,
and it will happen after devm_snd_soc_register_component() support.

# it can help me if devm_snd_soc_register_component() support (?) patch
# move component functions to upside of codec if possible

Best regards
---
Kuninori Morimoto

Re: [PATCH v3 0/1] snd_soc_codec include snd_soc_component

[Kuninori Morimoto]

Hi Mark, Lars

This is 3rd and formal version of codec <-> component merge patch.
I know that Mark sent devm_snd_soc_register_component() patch,
but I noticed that codec can't use it since it uses __snd_soc_register_component().
So, this patch still using above, and I added missing snd_soc_unregister_component()

And this patch keeps all codec "as-is" now,
since "component" doesn't check/care NULL name at this point.

Now, I created simple-card DT patch on my local environment.
It is based on this patch, and new component :: of_xlate_get_dai_name.
I will send it if this patch got positive response

Kuninori Morimoto (1):
      ASoC: snd_soc_codec includes snd_soc_component

 include/sound/soc.h  |   33 +++++----
 sound/soc/soc-core.c |  202 ++++++++++++++++++++++++++++----------------------
 2 files changed, 131 insertions(+), 104 deletions(-)

Best regards
---
Kuninori Morimoto

[PATCH 1/1] ASoC: snd_soc_codec includes snd_soc_component

[Kuninori Morimoto]

Codec includes component by this patch,
and component moved to upside of codec
to avoid extra declaration.
Codec dai will be registered via component
by this patch.

Current component register function
is used for cpu, and it is using
dai/dais functions properly to keep
existing cpu dai name.

And now, it will be used from codec also.
But codec driver had been used dais function only
even though it was single dai.
This patch adds new flag which can selects
dai/dais function on component register
function to keep existing codec dai name.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h  |   33 +++++----
 sound/soc/soc-core.c |  202 ++++++++++++++++++++++++++++----------------------
 2 files changed, 131 insertions(+), 104 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 8e2ad52..28e2d6a 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -670,6 +670,21 @@ struct snd_soc_cache_ops {
 	int (*sync)(struct snd_soc_codec *codec);
 };
 
+/* component interface */
+struct snd_soc_component_driver {
+	const char *name;
+};
+
+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;
+};
+
 /* SoC Audio Codec device */
 struct snd_soc_codec {
 	const char *name;
@@ -717,6 +732,9 @@ struct snd_soc_codec {
 	struct mutex cache_rw_mutex;
 	int val_bytes;
 
+	/* component */
+	struct snd_soc_component component;
+
 	/* dapm */
 	struct snd_soc_dapm_context dapm;
 	unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */
@@ -736,6 +754,7 @@ struct snd_soc_codec_driver {
 	int (*remove)(struct snd_soc_codec *);
 	int (*suspend)(struct snd_soc_codec *);
 	int (*resume)(struct snd_soc_codec *);
+	struct snd_soc_component_driver component_driver;
 
 	/* Default control and setup, added after probe() is run */
 	const struct snd_kcontrol_new *controls;
@@ -853,20 +872,6 @@ struct snd_soc_platform {
 #endif
 };
 
-struct snd_soc_component_driver {
-	const char *name;
-};
-
-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 7d13c4f..6ecb3e5 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -4019,6 +4019,112 @@ static void snd_soc_unregister_dais(struct device *dev, size_t count)
 }
 
 /**
+ * snd_soc_register_component - Register a component with the ASoC core
+ *
+ */
+static int
+__snd_soc_register_component(struct device *dev,
+			     struct snd_soc_component *cmpnt,
+			     const struct snd_soc_component_driver *cmpnt_drv,
+			     struct snd_soc_dai_driver *dai_drv,
+			     int num_dai, bool allow_single_dai)
+{
+	int ret;
+
+	dev_dbg(dev, "component register %s\n", dev_name(dev));
+
+	if (!cmpnt) {
+		dev_err(dev, "ASoC: Failed to connecting component\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;
+
+	/*
+	 * snd_soc_register_dai()  uses fmt_single_name(), and
+	 * snd_soc_register_dais() uses fmt_multiple_name()
+	 * for dai->name which is used for name based matching
+	 *
+	 * this function is used from cpu/codec.
+	 * allow_single_dai flag can ignore "codec" driver reworking
+	 * since it had been used snd_soc_register_dais(),
+	 */
+	if ((1 == num_dai) && allow_single_dai)
+		ret = snd_soc_register_dai(dev, dai_drv);
+	else
+		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;
+}
+
+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;
+
+	cmpnt = devm_kzalloc(dev, sizeof(*cmpnt), GFP_KERNEL);
+	if (!cmpnt) {
+		dev_err(dev, "ASoC: Failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	return __snd_soc_register_component(dev, cmpnt, cmpnt_drv,
+					    dai_drv, num_dai, true);
+}
+EXPORT_SYMBOL_GPL(snd_soc_register_component);
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
+
+/**
  * snd_soc_add_platform - Add a platform to the ASoC core
  * @dev: The parent device for the platform
  * @platform: The platform to add
@@ -4240,10 +4346,12 @@ int snd_soc_register_codec(struct device *dev,
 	list_add(&codec->list, &codec_list);
 	mutex_unlock(&client_mutex);
 
-	/* register any DAIs */
-	ret = snd_soc_register_dais(dev, dai_drv, num_dai);
+	/* register component */
+	ret = __snd_soc_register_component(dev, &codec->component,
+					   &codec_drv->component_driver,
+					   dai_drv, num_dai, false);
 	if (ret < 0) {
-		dev_err(codec->dev, "ASoC: Failed to regster DAIs: %d\n", ret);
+		dev_err(codec->dev, "ASoC: Failed to regster component: %d\n", ret);
 		goto fail_codec_name;
 	}
 
@@ -4278,7 +4386,7 @@ void snd_soc_unregister_codec(struct device *dev)
 	return;
 
 found:
-	snd_soc_unregister_dais(dev, codec->num_dai);
+	snd_soc_unregister_component(dev);
 
 	mutex_lock(&client_mutex);
 	list_del(&codec->list);
@@ -4293,92 +4401,6 @@ 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;
-
-	/*
-	 * snd_soc_register_dai()  uses fmt_single_name(), and
-	 * snd_soc_register_dais() uses fmt_multiple_name()
-	 * for dai->name which is used for name based matching
-	 */
-	if (1 == num_dai)
-		ret = snd_soc_register_dai(dev, dai_drv);
-	else
-		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;
-}
-EXPORT_SYMBOL_GPL(snd_soc_register_component);
-
-/**
- * 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);
-}
-EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
-
 /* Retrieve a card's name from device tree */
 int snd_soc_of_parse_card_name(struct snd_soc_card *card,
 			       const char *propname)
-- 
1.7.9.5

[PATCH 0/2][RFC] ASoC: add .of_xlate_dai_name and DT support

[Kuninori Morimoto]

Hi Mark, Lars

This is very flying RFC patches since previous
"ASoC: snd_soc_codec includes snd_soc_component"
has no response at this point.
But I want any opinion about .of_xlate_dai_name implementation
and DT support method

This patch adds .of_xlate_dai_name on component driver,
and simple-card use it via snd_soc_of_get_dai_name()

Of course these are based on
"ASoC: snd_soc_codec includes snd_soc_component"
but, what do you think about these patches ?

Thank you

[PATCH 1/2][RFC] ASoC: add .of_xlate_dai_name on snd_soc_component_driver

[Kuninori Morimoto]

ASoC sound driver requires CPU/CODEC drivers for probing,
and each CPU/CODEC has some DAI on it.
Then, "dai name matching" have been used to identify
CPU-CODEC DAI pair on ASoC.

But, the "dai port number matching" is now required from DeviceTree.
The solution of this issue is to replace
the dai port number into dai name.
Now, CPU/CODEC are based on struct snd_soc_component,
and it can care above as common issue.

This patch adds .of_xlate_dai_name callback interface
on struct snd_soc_component_driver,
and snd_soc_of_get_dai_name() which is using .of_xlate_dai_name.

Then, #sound-dai-cells which enables DAI specifier is required
on CPU/CODEC device tree properties.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h  |    8 ++++++++
 sound/soc/soc-core.c |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 28e2d6a..01cdbed 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -13,6 +13,7 @@
 #ifndef __LINUX_SND_SOC_H
 #define __LINUX_SND_SOC_H
 
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 #include <linux/notifier.h>
@@ -673,6 +674,11 @@ struct snd_soc_cache_ops {
 /* component interface */
 struct snd_soc_component_driver {
 	const char *name;
+
+	/* DT */
+	int (*of_xlate_dai_name)(struct snd_soc_component *component,
+				 struct of_phandle_args *args,
+				 const char **dai_name);
 };
 
 struct snd_soc_component {
@@ -1206,6 +1212,8 @@ int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 				   const char *propname);
 unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
 				     const char *prefix);
+int snd_soc_of_get_dai_name(struct device_node *of_node,
+			    const char **dai_name);
 
 #include <sound/soc-dai.h>
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 6ecb3e5..fe0f6b8 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -4588,6 +4588,41 @@ unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(snd_soc_of_parse_daifmt);
 
+int snd_soc_of_get_dai_name(struct device_node *of_node,
+			    const char **dai_name)
+{
+	struct snd_soc_component *pos;
+	struct of_phandle_args args;
+	int ret;
+
+	ret = of_parse_phandle_with_args(of_node, "sound-dai",
+					 "#sound-dai-cells", 0, &args);
+	if (ret)
+		return ret;
+
+	ret = -EPROBE_DEFER;
+
+	mutex_lock(&client_mutex);
+	list_for_each_entry(pos, &component_list, list) {
+		if (pos->dev->of_node != args.np)
+			continue;
+
+		if (!pos->driver->of_xlate_dai_name) {
+			ret = -ENOSYS;
+			break;
+		}
+
+		ret = pos->driver->of_xlate_dai_name(pos, &args, dai_name);
+		break;
+	}
+	mutex_unlock(&client_mutex);
+
+	of_node_put(args.np);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_soc_of_get_dai_name);
+
 static int __init snd_soc_init(void)
 {
 #ifdef CONFIG_DEBUG_FS
-- 
1.7.9.5

[PATCH 2/2][RFC] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via devicetree.
It requests cpu/codec information,
and .of_xlate_dai_name support on each component driver.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++++
 sound/soc/generic/simple-card.c                    |  133 +++++++++++++++++++-
 2 files changed, 201 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..d1a0c89
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,73 @@
+Simple-Card:
+
+Required properties:
+
+- compatible				: "simple-audio"
+- simple-audio,card-name		: simple-audio card name
+- simple-audio,format			: see below
+- simple-audio,cpu			: CPU sub-node, see below
+- simple-audio,codec			: CODEC sub-node, see below
+
+Optional properties:
+
+- simple-audio,system-clock-frequency	: system clock rate if it is connected to both CPU/CODEC
+
+Required cpu/codec subnode properties:
+
+- sound-dai				: phandle and port for CPU/CODEC
+- #sound-dai-cells			: sound-dai phandle's node
+- frame-master				: frame master
+- bitclock-master			: bitclock master
+
+Optional subnode properties:
+
+- bitclock-inversion			: if CPU/CODEC needs clock inversion
+- frame-inversion			: if CPU/CODEC needs frame inversion
+- system-clock-frequency		: system clock rate for each CPU/CODEC
+
+simple-audio,format
+	"i2s"
+	"right_j"
+	"left_j"
+	"dsp_a"
+	"dsp_b"
+	"ac97"
+	"pdm"
+	"msb"
+	"lsb"
+
+Example:
+
+sound {
+	compatible = "simple-audio";
+
+	simple-audio,card-name = "FSI2A-AK4648";
+	simple-audio,format = "left_j";
+
+	simple-audio,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		system-clock-frequency = <11289600>;
+	};
+};
+
+&i2c0 {
+	ak4648: ak4648@0x12 {
+		#sound-dai-cells = <0>;
+		compatible = "asahi-kasei,ak4648";
+		reg = <0x12>;
+	};
+};
+
+sh_fsi2: sh_fsi2@0xec230000 {
+	#sound-dai-cells = <1>;
+	compatible = "renesas,sh_fsi2";
+	reg = <0xec230000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 146 0x4>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 8c49147..d153145 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>
@@ -52,11 +53,123 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+__asoc_simple_card_parse_of(struct device_node *np,
+			    struct asoc_simple_dai *dai,
+			    struct device_node **node)
+{
+	int ret;
+
+	/* get "sound-dai = <&phandle port>" */
+	*node = of_parse_phandle(np, "sound-dai", 0);
+	if (!*node)
+		return -ENODEV;
+
+	of_node_put(*node);
+
+	/* get dai-name  */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		return ret;
+
+	/* get dai specific format */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/* get "simple-audio,system-clock-frequency = <xxx>" */
+	of_property_read_u32(np,
+			     "system-clock-frequency",
+			     &dai->sysclk);
+
+	return 0;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	u32 sysclk = 0;
+	int ret = 0;
+
+	of_property_read_string(node, "simple-audio,card-name", &info->card);
+	info->name = info->card;
+
+	/* cpu/codec common daifmt */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* cpu/codec common clock */
+	of_property_read_u32(node,
+			     "simple-audio,system-clock-frequency", &sysclk);
+	info->cpu_dai.sysclk = sysclk;
+	info->codec_dai.sysclk = sysclk;
+
+	/* cpu/codec sub-node */
+	for_each_child_of_node(node, np) {
+		if (0 == strcmp("simple-audio,cpu", np->name))
+			ret = __asoc_simple_card_parse_of(np,
+							  &info->cpu_dai,
+							  of_cpu);
+		if (0 == strcmp("simple-audio,codec", np->name))
+			ret = __asoc_simple_card_parse_of(np,
+							  &info->codec_dai,
+							  of_codec);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x / %p\n",
+		info->daifmt,
+		*of_platform);
+	dev_dbg(dev, "cpu : %s / %04x / %d / %p\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk,
+		*of_cpu);
+	dev_dbg(dev, "codec : %s / %04x / %d / %p\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk,
+		*of_codec);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -64,10 +177,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -81,6 +194,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -102,10 +218,17 @@ 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 = "simple-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

Re: [PATCH 2/2][RFC] ASoC: simple-card: add Device Tree support

[Fabio Estevam]

On Tue, Sep 10, 2013 at 9:40 PM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:

> +       simple-audio,codec {
> +               sound-dai = <&ak4648>;
> +               bitclock-master;
> +               frame-master;
> +               system-clock-frequency = <11289600>;

Wouldn't it be better to pass a real clock here instead?

Re: [PATCH 2/2][RFC] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Fabio

> > +       simple-audio,codec {
> > +               sound-dai = <&ak4648>;
> > +               bitclock-master;
> > +               frame-master;
> > +               system-clock-frequency = <11289600>;
> 
> Wouldn't it be better to pass a real clock here instead?

Indeed
Thank you for pointing it.
I will try to use it in next patch

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/2][RFC] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 667 bytes --]

On Tue, Sep 10, 2013 at 09:52:12PM -0300, Fabio Estevam wrote:
> On Tue, Sep 10, 2013 at 9:40 PM, Kuninori Morimoto

> > +       simple-audio,codec {
> > +               sound-dai = <&ak4648>;
> > +               bitclock-master;
> > +               frame-master;
> > +               system-clock-frequency = <11289600>;

> Wouldn't it be better to pass a real clock here instead?

So, ideally.  However we have to consider the fact that the clock API
isn't reliably available makes this harder than it should be.  Even
among the DT using platforms at least PowerPC still uses a custom clock
API.  We could just use this as a carrot to push people to convert
though.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2][RFC] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi

> > > +       simple-audio,codec {
> > > +               sound-dai = <&ak4648>;
> > > +               bitclock-master;
> > > +               frame-master;
> > > +               system-clock-frequency = <11289600>;
> 
> > Wouldn't it be better to pass a real clock here instead?
> 
> So, ideally.  However we have to consider the fact that the clock API
> isn't reliably available makes this harder than it should be.  Even
> among the DT using platforms at least PowerPC still uses a custom clock
> API.  We could just use this as a carrot to push people to convert
> though.

Actually SH can't use common clock API either.
We are working for it now, but it need more time.
OK, I will try to have 2 method on next version.
one is above style, and one is common clock API style

Best regards
---
Kuninori Morimoto

Re: [PATCH 0/2][RFC] ASoC: add .of_xlate_dai_name and DT support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 444 bytes --]

On Tue, Sep 10, 2013 at 05:38:33PM -0700, Kuninori Morimoto wrote:

> This is very flying RFC patches since previous
> "ASoC: snd_soc_codec includes snd_soc_component"
> has no response at this point.
> But I want any opinion about .of_xlate_dai_name implementation
> and DT support method

This all looks basically OK to me, though we will need to go and update
binding documents to say which DAI is which on the CODECs that use the
bindings.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2][RFC] ASoC: simple-card: add Device Tree support

[Sebastian Hesselbarth]

On 09/11/13 02:40, Kuninori Morimoto wrote:
> Support for loading the simple-card module via devicetree.
> It requests cpu/codec information,
> and .of_xlate_dai_name support on each component driver.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
[...]
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..d1a0c89
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,73 @@
> +Simple-Card:
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +- simple-audio,card-name		: simple-audio card name
> +- simple-audio,format			: see below
> +- simple-audio,cpu			: CPU sub-node, see below
> +- simple-audio,codec			: CODEC sub-node, see below
> +
> +Optional properties:
> +
> +- simple-audio,system-clock-frequency	: system clock rate if it is connected to both CPU/CODEC
> +
> +Required cpu/codec subnode properties:
> +
> +- sound-dai				: phandle and port for CPU/CODEC
> +- #sound-dai-cells			: sound-dai phandle's node

IMHO, "sound-dai" is too ASoC specific for this property name.
To make it more generic, what about e.g. "sound-link" and
"#sound-link-cells" instead?

> +- frame-master				: frame master
> +- bitclock-master			: bitclock master
> +
> +Optional subnode properties:
> +
> +- bitclock-inversion			: if CPU/CODEC needs clock inversion
> +- frame-inversion			: if CPU/CODEC needs frame inversion
> +- system-clock-frequency		: system clock rate for each CPU/CODEC

Instead of "system-clock-frequency" use standard property name
"clock-frequency". In the driver, you can check for this property
and use it, if no common clocks are available on the arch running.

Sebastian

Re: [PATCH 2/2][RFC] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 887 bytes --]

On Thu, Sep 12, 2013 at 01:31:19PM +0200, Sebastian Hesselbarth wrote:
> On 09/11/13 02:40, Kuninori Morimoto wrote:

> >+- sound-dai				: phandle and port for CPU/CODEC
> >+- #sound-dai-cells			: sound-dai phandle's node

> IMHO, "sound-dai" is too ASoC specific for this property name.
> To make it more generic, what about e.g. "sound-link" and
> "#sound-link-cells" instead?

DAI is an acronym for digital audio interface which is a fairly widely
used term for these things.

> >+- system-clock-frequency		: system clock rate for each CPU/CODEC

> Instead of "system-clock-frequency" use standard property name
> "clock-frequency". In the driver, you can check for this property
> and use it, if no common clocks are available on the arch running.

We've been round this loop several times, audio devices tend to have
multiple clocks so it's important to say which one is being set.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/1] ASoC: snd_soc_codec includes snd_soc_component

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 265 bytes --]

On Wed, Sep 04, 2013 at 07:39:03PM -0700, Kuninori Morimoto wrote:
> Codec includes component by this patch,
> and component moved to upside of codec
> to avoid extra declaration.
> Codec dai will be registered via component
> by this patch.

Applied both, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH v2] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information,
and .of_xlate_dai_name support on each component driver.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - add common clock support, system-clock-frequency can over-write it
 - add some comment on code

This patch is based on asoc/topic/component branch


 .../devicetree/bindings/sound/simple-card.txt      |   85 ++++++++++++
 sound/soc/generic/simple-card.c                    |  146 +++++++++++++++++++-
 2 files changed, 226 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..75bbc5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,85 @@
+Simple-Card:
+
+Required properties:
+
+- compatible				: "simple-audio"
+- simple-audio,card-name		: simple-audio card name
+- simple-audio,cpu			: CPU sub-node, see below
+- simple-audio,codec			: CODEC sub-node, see below
+
+Optional properties:
+
+- simple-audio,format			: CPU/CODEC common format, see below
+
+Required cpu/codec subnode properties:
+
+- sound-dai				: phandle and port for CPU/CODEC
+- #sound-dai-cells			: sound-dai phandle's node
+
+Optional subnode properties:
+
+- format				: specific format if needed, see below
+- frame-master				: frame master
+- bitclock-master			: bitclock master
+- bitclock-inversion			: clock inversion
+- frame-inversion			: frame inversion
+- clocks				: phandle for system clock rate
+- system-clock-frequency		: system clock rate
+					  it will overwrite clocks's rate
+
+simple-audio,format
+	"i2s"
+	"right_j"
+	"left_j"
+	"dsp_a"
+	"dsp_b"
+	"ac97"
+	"pdm"
+	"msb"
+	"lsb"
+
+Example:
+
+clock {
+	osc: oscillator {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <11289600>;
+	};
+};
+
+sound {
+	compatible = "simple-audio";
+
+	simple-audio,card-name = "FSI2A-AK4648";
+	simple-audio,format = "left_j";
+
+	simple-audio,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+		/* it can use this instead of clocks
+		 * system-clock-frequency = <11289600>; */
+	};
+};
+
+&i2c0 {
+	ak4648: ak4648@0x12 {
+		#sound-dai-cells = <0>;
+		compatible = "asahi-kasei,ak4648";
+		reg = <0x12>;
+	};
+};
+
+sh_fsi2: sh_fsi2@0xec230000 {
+	#sound-dai-cells = <1>;
+	compatible = "renesas,sh_fsi2";
+	reg = <0xec230000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 146 0x4>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 8c49147..62befbd 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -9,6 +9,8 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+__asoc_simple_card_parse_of(struct device_node *np,
+			    struct asoc_simple_dai *dai,
+			    struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * get node via "sound-dai = <&phandle port>"
+	 * it will be used as xxx_of_node on soc_bind_dai_link()
+	 */
+	*node = of_parse_phandle(np, "sound-dai", 0);
+	if (!*node)
+		return -ENODEV;
+
+	of_node_put(*node);
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/* dai->sysclk via "clolks = <xxx>" */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		dai->sysclk = 0;
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	/*
+	 * overwrite dai->sysclk if it has
+	 * "system-clock-frequency = <xxx>"
+	 */
+	of_property_read_u32(np,
+			     "system-clock-frequency",
+			     &dai->sysclk);
+
+	return 0;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	/* get card name */
+	of_property_read_string(node, "simple-audio,card-name", &info->card);
+	info->name = info->card;
+
+	/* get CPU/CODEC common format via simple-audio,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU/CODEC sub-node */
+	for_each_child_of_node(node, np) {
+		if (0 == strcmp("simple-audio,cpu", np->name))
+			ret = __asoc_simple_card_parse_of(np,
+							  &info->cpu_dai,
+							  of_cpu);
+		if (0 == strcmp("simple-audio,codec", np->name))
+			ret = __asoc_simple_card_parse_of(np,
+							  &info->codec_dai,
+							  of_codec);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x / %p\n",
+		info->daifmt,
+		*of_platform);
+	dev_dbg(dev, "cpu : %s / %04x / %d / %p\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk,
+		*of_cpu);
+	dev_dbg(dev, "codec : %s / %04x / %d / %p\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk,
+		*of_codec);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -64,10 +190,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -81,6 +207,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -102,10 +231,17 @@ 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 = "simple-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

Re: [PATCH v2] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 255 bytes --]

On Thu, Oct 03, 2013 at 02:33:55AM -0700, Kuninori Morimoto wrote:

> ping ?

Don't send contentless pings.  I'm waiting for review from the device
tree guys but I see now that you didn't CC them so they're unlikely to
respond - please resend CCing them.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH v3] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information,
and .of_xlate_dai_name support on each component driver.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v1 -> v2

 - add common clock support, system-clock-frequency can over-write it
 - add some comment on code

v2 -> v3

 - add devicetree ML address

 .../devicetree/bindings/sound/simple-card.txt      |   85 ++++++++++++
 sound/soc/generic/simple-card.c                    |  146 +++++++++++++++++++-
 2 files changed, 226 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..75bbc5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,85 @@
+Simple-Card:
+
+Required properties:
+
+- compatible				: "simple-audio"
+- simple-audio,card-name		: simple-audio card name
+- simple-audio,cpu			: CPU sub-node, see below
+- simple-audio,codec			: CODEC sub-node, see below
+
+Optional properties:
+
+- simple-audio,format			: CPU/CODEC common format, see below
+
+Required cpu/codec subnode properties:
+
+- sound-dai				: phandle and port for CPU/CODEC
+- #sound-dai-cells			: sound-dai phandle's node
+
+Optional subnode properties:
+
+- format				: specific format if needed, see below
+- frame-master				: frame master
+- bitclock-master			: bitclock master
+- bitclock-inversion			: clock inversion
+- frame-inversion			: frame inversion
+- clocks				: phandle for system clock rate
+- system-clock-frequency		: system clock rate
+					  it will overwrite clocks's rate
+
+simple-audio,format
+	"i2s"
+	"right_j"
+	"left_j"
+	"dsp_a"
+	"dsp_b"
+	"ac97"
+	"pdm"
+	"msb"
+	"lsb"
+
+Example:
+
+clock {
+	osc: oscillator {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <11289600>;
+	};
+};
+
+sound {
+	compatible = "simple-audio";
+
+	simple-audio,card-name = "FSI2A-AK4648";
+	simple-audio,format = "left_j";
+
+	simple-audio,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+		/* it can use this instead of clocks
+		 * system-clock-frequency = <11289600>; */
+	};
+};
+
+&i2c0 {
+	ak4648: ak4648@0x12 {
+		#sound-dai-cells = <0>;
+		compatible = "asahi-kasei,ak4648";
+		reg = <0x12>;
+	};
+};
+
+sh_fsi2: sh_fsi2@0xec230000 {
+	#sound-dai-cells = <1>;
+	compatible = "renesas,sh_fsi2";
+	reg = <0xec230000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 146 0x4>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 8c49147..62befbd 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -9,6 +9,8 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+__asoc_simple_card_parse_of(struct device_node *np,
+			    struct asoc_simple_dai *dai,
+			    struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * get node via "sound-dai = <&phandle port>"
+	 * it will be used as xxx_of_node on soc_bind_dai_link()
+	 */
+	*node = of_parse_phandle(np, "sound-dai", 0);
+	if (!*node)
+		return -ENODEV;
+
+	of_node_put(*node);
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/* dai->sysclk via "clolks = <xxx>" */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		dai->sysclk = 0;
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	/*
+	 * overwrite dai->sysclk if it has
+	 * "system-clock-frequency = <xxx>"
+	 */
+	of_property_read_u32(np,
+			     "system-clock-frequency",
+			     &dai->sysclk);
+
+	return 0;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	/* get card name */
+	of_property_read_string(node, "simple-audio,card-name", &info->card);
+	info->name = info->card;
+
+	/* get CPU/CODEC common format via simple-audio,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU/CODEC sub-node */
+	for_each_child_of_node(node, np) {
+		if (0 == strcmp("simple-audio,cpu", np->name))
+			ret = __asoc_simple_card_parse_of(np,
+							  &info->cpu_dai,
+							  of_cpu);
+		if (0 == strcmp("simple-audio,codec", np->name))
+			ret = __asoc_simple_card_parse_of(np,
+							  &info->codec_dai,
+							  of_codec);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x / %p\n",
+		info->daifmt,
+		*of_platform);
+	dev_dbg(dev, "cpu : %s / %04x / %d / %p\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk,
+		*of_cpu);
+	dev_dbg(dev, "codec : %s / %04x / %d / %p\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk,
+		*of_codec);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -64,10 +190,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -81,6 +207,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -102,10 +231,17 @@ 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 = "simple-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 9060 bytes --]

On Thu, Oct 03, 2013 at 05:04:41PM -0700, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information,
> and .of_xlate_dai_name support on each component driver.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

CCing in the DT maintainers directly.

> ---
> v1 -> v2
> 
>  - add common clock support, system-clock-frequency can over-write it
>  - add some comment on code
> 
> v2 -> v3
> 
>  - add devicetree ML address
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   85 ++++++++++++
>  sound/soc/generic/simple-card.c                    |  146 +++++++++++++++++++-
>  2 files changed, 226 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..75bbc5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,85 @@
> +Simple-Card:
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +- simple-audio,card-name		: simple-audio card name
> +- simple-audio,cpu			: CPU sub-node, see below
> +- simple-audio,codec			: CODEC sub-node, see below
> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common format, see below
> +
> +Required cpu/codec subnode properties:
> +
> +- sound-dai				: phandle and port for CPU/CODEC
> +- #sound-dai-cells			: sound-dai phandle's node
> +
> +Optional subnode properties:
> +
> +- format				: specific format if needed, see below
> +- frame-master				: frame master
> +- bitclock-master			: bitclock master
> +- bitclock-inversion			: clock inversion
> +- frame-inversion			: frame inversion
> +- clocks				: phandle for system clock rate
> +- system-clock-frequency		: system clock rate
> +					  it will overwrite clocks's rate
> +
> +simple-audio,format
> +	"i2s"
> +	"right_j"
> +	"left_j"
> +	"dsp_a"
> +	"dsp_b"
> +	"ac97"
> +	"pdm"
> +	"msb"
> +	"lsb"
> +
> +Example:
> +
> +clock {
> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};
> +
> +sound {
> +	compatible = "simple-audio";
> +
> +	simple-audio,card-name = "FSI2A-AK4648";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +		/* it can use this instead of clocks
> +		 * system-clock-frequency = <11289600>; */
> +	};
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@0x12 {
> +		#sound-dai-cells = <0>;
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +	};
> +};
> +
> +sh_fsi2: sh_fsi2@0xec230000 {
> +	#sound-dai-cells = <1>;
> +	compatible = "renesas,sh_fsi2";
> +	reg = <0xec230000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupts = <0 146 0x4>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 8c49147..62befbd 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +__asoc_simple_card_parse_of(struct device_node *np,
> +			    struct asoc_simple_dai *dai,
> +			    struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * get node via "sound-dai = <&phandle port>"
> +	 * it will be used as xxx_of_node on soc_bind_dai_link()
> +	 */
> +	*node = of_parse_phandle(np, "sound-dai", 0);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	of_node_put(*node);
> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/* dai->sysclk via "clolks = <xxx>" */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		dai->sysclk = 0;
> +	else
> +		dai->sysclk = clk_get_rate(clk);
> +
> +	/*
> +	 * overwrite dai->sysclk if it has
> +	 * "system-clock-frequency = <xxx>"
> +	 */
> +	of_property_read_u32(np,
> +			     "system-clock-frequency",
> +			     &dai->sysclk);
> +
> +	return 0;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	int ret = 0;
> +
> +	/* get card name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	info->name = info->card;
> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU/CODEC sub-node */
> +	for_each_child_of_node(node, np) {
> +		if (0 == strcmp("simple-audio,cpu", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->cpu_dai,
> +							  of_cpu);
> +		if (0 == strcmp("simple-audio,codec", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->codec_dai,
> +							  of_codec);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x / %p\n",
> +		info->daifmt,
> +		*of_platform);
> +	dev_dbg(dev, "cpu : %s / %04x / %d / %p\n",
> +		info->cpu_dai.name,
> +		info->cpu_dai.fmt,
> +		info->cpu_dai.sysclk,
> +		*of_cpu);
> +	dev_dbg(dev, "codec : %s / %04x / %d / %p\n",
> +		info->codec_dai.name,
> +		info->codec_dai.fmt,
> +		info->codec_dai.sysclk,
> +		*of_codec);
> +
> +	return 0;
> +}
> +
>  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_node *of_cpu, *of_codec, *of_platform;
>  	struct device *dev = &pdev->dev;
>  
> +	cinfo		= NULL;
> +	of_cpu		= NULL;
> +	of_codec	= NULL;
> +	of_platform	= NULL;
> +	if (np && of_device_is_available(np)) {
> +		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
> +		if (cinfo) {
> +			int ret;
> +			ret = asoc_simple_card_parse_of(np, cinfo, dev,
> +							&of_cpu,
> +							&of_codec,
> +							&of_platform);
> +			if (ret < 0) {
> +				dev_err(dev, "parse error %d\n", ret);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		cinfo = pdev->dev.platform_data;
> +	}
> +
>  	if (!cinfo) {
>  		dev_err(dev, "no info for asoc-simple-card\n");
>  		return -EINVAL;
> @@ -64,10 +190,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  
>  	if (!cinfo->name	||
>  	    !cinfo->card	||
> -	    !cinfo->codec	||
> -	    !cinfo->platform	||
> -	    !cinfo->cpu_dai.name ||
> -	    !cinfo->codec_dai.name) {
> +	    !cinfo->codec_dai.name	||
> +	    !(cinfo->codec		|| of_codec)	||
> +	    !(cinfo->platform		|| of_platform)	||
> +	    !(cinfo->cpu_dai.name	|| of_cpu)) {
>  		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
>  		return -EINVAL;
>  	}
> @@ -81,6 +207,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  	cinfo->snd_link.platform_name	= cinfo->platform;
>  	cinfo->snd_link.codec_name	= cinfo->codec;
>  	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
> +	cinfo->snd_link.cpu_of_node	= of_cpu;
> +	cinfo->snd_link.codec_of_node	= of_codec;
> +	cinfo->snd_link.platform_of_node = of_platform;
>  	cinfo->snd_link.init		= asoc_simple_card_dai_init;
>  
>  	/*
> @@ -102,10 +231,17 @@ 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 = "simple-audio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
> +
>  static struct platform_driver asoc_simple_card = {
>  	.driver = {
>  		.name	= "asoc-simple-card",
>  		.owner = THIS_MODULE,
> +		.of_match_table = asoc_simple_of_match,
>  	},
>  	.probe		= asoc_simple_card_probe,
>  	.remove		= asoc_simple_card_remove,
> -- 
> 1.7.9.5
> 
> 

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v3] ASoC: simple-card: add Device Tree support

[Mark Rutland]

Hi,

On Fri, Oct 04, 2013 at 01:04:41AM +0100, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information,
> and .of_xlate_dai_name support on each component driver.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v1 -> v2
> 
>  - add common clock support, system-clock-frequency can over-write it
>  - add some comment on code
> 
> v2 -> v3
> 
>  - add devicetree ML address
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   85 ++++++++++++
>  sound/soc/generic/simple-card.c                    |  146 +++++++++++++++++++-
>  2 files changed, 226 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..75bbc5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,85 @@
> +Simple-Card:

It's really difficult to review this without a description. Please explain what
this binding represents.

What hardware is this applicable to?

Does this inherit from some class of binding?

> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +- simple-audio,card-name		: simple-audio card name

What's this used for?

> +- simple-audio,cpu			: CPU sub-node, see below
> +- simple-audio,codec			: CODEC sub-node, see below

Describe these as required subnodes. Nodes are not properties.

> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common format, see below
> +
> +Required cpu/codec subnode properties:
> +
> +- sound-dai				: phandle and port for CPU/CODEC
> +- #sound-dai-cells			: sound-dai phandle's node

This description makes no sense, the organisation seems structurally wrong.

What does this mean? What does it affect?

> +
> +Optional subnode properties:
> +
> +- format				: specific format if needed, see below
> +- frame-master				: frame master
> +- bitclock-master			: bitclock master
> +- bitclock-inversion			: clock inversion
> +- frame-inversion			: frame inversion

What do these mean? Repeating the name without a dash is completely unhelpful.
Describe what these imply.

What type are they?

Which subnode(s) do they apply to?

> +- clocks				: phandle for system clock rate

Just one clock?

Nit: clocks are specified with a phandle + clock-specifier pair, not just a phandle.

> +- system-clock-frequency		: system clock rate
> +					  it will overwrite clocks's rate

This seems very odd.

Why do you want to overwrite a clock's rate?

> +
> +simple-audio,format
> +	"i2s"
> +	"right_j"
> +	"left_j"
> +	"dsp_a"
> +	"dsp_b"
> +	"ac97"
> +	"pdm"
> +	"msb"
> +	"lsb"

What do these mean? Why are they not described when the property was defined above?

Does this also apply for the "format" property?

> +
> +Example:
> +
> +clock {
> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};
> +
> +sound {
> +	compatible = "simple-audio";
> +
> +	simple-audio,card-name = "FSI2A-AK4648";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +		/* it can use this instead of clocks
> +		 * system-clock-frequency = <11289600>; */

This just confuses matters. If ou want to show this off, have two examples.

> +	};
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@0x12 {

Nit: remove the 0x on the unit-address

> +		#sound-dai-cells = <0>;
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +	};
> +};
> +
> +sh_fsi2: sh_fsi2@0xec230000 {

Again, fix the unit-address please.

> +	#sound-dai-cells = <1>;
> +	compatible = "renesas,sh_fsi2";
> +	reg = <0xec230000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupts = <0 146 0x4>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 8c49147..62befbd 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +__asoc_simple_card_parse_of(struct device_node *np,
> +			    struct asoc_simple_dai *dai,
> +			    struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * get node via "sound-dai = <&phandle port>"
> +	 * it will be used as xxx_of_node on soc_bind_dai_link()
> +	 */
> +	*node = of_parse_phandle(np, "sound-dai", 0);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	of_node_put(*node);

Why?

You're refrering to it, so why do you not want to have it refcounted?

It could disappear under your feet.

> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/* dai->sysclk via "clolks = <xxx>" */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		dai->sysclk = 0;
> +	else
> +		dai->sysclk = clk_get_rate(clk);

This seems like an odd assumption to make. Why not error?

> +
> +	/*
> +	 * overwrite dai->sysclk if it has
> +	 * "system-clock-frequency = <xxx>"
> +	 */
> +	of_property_read_u32(np,
> +			     "system-clock-frequency",
> +			     &dai->sysclk);

Is dai->sysclk defined as a u32?

> +
> +	return 0;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	int ret = 0;
> +
> +	/* get card name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	info->name = info->card;

What if the string is not in the DT?

Does the core code handle these being NULL?

> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU/CODEC sub-node */
> +	for_each_child_of_node(node, np) {
> +		if (0 == strcmp("simple-audio,cpu", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->cpu_dai,
> +							  of_cpu);
> +		if (0 == strcmp("simple-audio,codec", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->codec_dai,
> +							  of_codec);

of_get_child_by_name?

> +		if (ret < 0)
> +			return ret;
> +	}

What if there were no children?

> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x / %p\n",
> +		info->daifmt,
> +		*of_platform);

Why is the pointer helpful, rather than the info it points to?

Thanks,
Mark.

[PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v3 -> v4

 - explain detail of each properties on simple-card.txt
 - fixup odd examples on simple-card.txt
 - remove "simple-card,card-name". create it from cpu/codec name
 - use of_get_child_by_name()
 - remove odd pointer info from dev_dbg()
 - remove subnode format which are no longer needed

This is based on asoc/topic/simple branch

 .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++
 sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
 2 files changed, 223 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..4871e91
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,73 @@
+Simple-Card:
+
+Simple-Card specifies audio DAI connection of SoC <-> codec.
+
+Required properties:
+
+- compatible				: "simple-audio"
+
+Optional properties:
+
+- simple-audio,format			: CPU/CODEC common audio format.
+					"i2s", "right_j", "left_j" , "dsp_a"
+					"dsp_b", "ac97", "pdm", "msb", "lsb"
+Required subnodes:
+
+- simple-audio,cpu			: CPU   sub-node
+- simple-audio,codec			: CODEC sub-node
+
+Required CPU/CODEC subnodes properties:
+
+- sound-dai				: phandle and port of CPU/CODEC
+
+Optional CPU/CODEC subnodes properties:
+
+- frame-master				: bool property. add this if subnode was frame master
+- bitclock-master			: bool property. add this if subnode was bitclock master
+- bitclock-inversion			: bool property. add this if subnode has clock inversion
+- frame-inversion			: bool property. add this if subnode has frame inversion
+- clocks / system-clock-frequency	: specify subnode's clock if needed.
+					  it can be specified via "clocks" if system has clock node,
+                                          or "system-clock-frequency" if system doesn't have it.
+
+Example:
+
+clock {
+	osc: oscillator {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <11289600>;
+	};
+};
+
+sound {
+	compatible = "simple-audio";
+	simple-audio,format = "left_j";
+
+	simple-audio,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+	};
+};
+
+&i2c0 {
+	ak4648: ak4648@12 {
+		#sound-dai-cells = <0>;
+		compatible = "asahi-kasei,ak4648";
+		reg = <0x12>;
+	};
+};
+
+sh_fsi2: sh_fsi2@ec230000 {
+	#sound-dai-cells = <1>;
+	compatible = "renesas,sh_fsi2";
+	reg = <0xec230000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 146 0x4>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index b2fbb70..c0fb635 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,7 +8,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -57,11 +58,144 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+asoc_simple_card_sub_parse_of(struct device_node *np,
+			      struct asoc_simple_dai *dai,
+			      struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * get node via "sound-dai = <&phandle port>"
+	 * it will be used as xxx_of_node on soc_bind_dai_link()
+	 */
+	*node = of_parse_phandle(np, "sound-dai", 0);
+	if (!*node)
+		return -ENODEV;
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		goto parse_error;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/*
+	 * dai->sysclk come from
+	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
+	 */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		of_property_read_u32(np,
+				     "system-clock-frequency",
+				     &dai->sysclk);
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	ret = 0;
+
+parse_error:
+	of_node_put(*node);
+
+	return ret;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	char *name;
+	int ret = 0;
+
+	/* get CPU/CODEC common format via simple-audio,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio,cpu");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->cpu_dai,
+						  of_cpu);
+	if (ret < 0)
+		return ret;
+
+	/* CODEC sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio,codec");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->codec_dai,
+						  of_codec);
+	if (ret < 0)
+		return ret;
+
+	/* card name is created from CPU/CODEC dai name */
+	of_property_read_string(node, "simple-audio,card-name", &info->card);
+	name = devm_kzalloc(dev,
+			    strlen(info->cpu_dai.name)   +
+			    strlen(info->codec_dai.name) + 2,
+			    GFP_KERNEL);
+	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
+	info->name = info->card = name;
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x\n", info->daifmt);
+	dev_dbg(dev, "cpu : %s / %04x / %d\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk);
+	dev_dbg(dev, "codec : %s / %04x / %d\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -69,10 +203,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -86,6 +220,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -107,10 +244,17 @@ 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 = "simple-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2092 bytes --]

On Thu, Oct 24, 2013 at 06:17:59PM +0100, Mark Rutland wrote:
> On Fri, Oct 04, 2013 at 01:04:41AM +0100, Kuninori Morimoto wrote:

> > +- simple-audio,card-name		: simple-audio card name

> What's this used for?

This serves two useful functions.  One is that this is used for display
to users so they have a friendly name for the sound card (it is
relatively common to have multiple sound cards in the system).  The
other is that it is essentially a compatibility string for configuration
- you get a lot of sound devices that are electrically identical and
hence look the same from a driver point of view but due different
physical form factors should be configured differently.

> > +- format				: specific format if needed, see below
> > +- frame-master				: frame master
> > +- bitclock-master			: bitclock master
> > +- bitclock-inversion			: clock inversion
> > +- frame-inversion			: frame inversion

> What do these mean? Repeating the name without a dash is completely unhelpful.
> Describe what these imply.

These are all boolean propeties.  The meanings should be obvious or at
least very easily discoverable to anyone with any familiarity with audio
hardware; if you can understand what to do with them they should be OK.

> > +- clocks				: phandle for system clock rate

> Just one clock?

This is a limitation from the simple card, anything that needs more
complex clocking should be using a different binding.

> > +- system-clock-frequency		: system clock rate
> > +					  it will overwrite clocks's rate

> This seems very odd.

> Why do you want to overwrite a clock's rate?

It's relatively common to derive the audio clock from a general purpose
programmable clock which needs to be configured appropriately for use.

> > +simple-audio,format
> > +	"i2s"
> > +	"right_j"
> > +	"left_j"
> > +	"dsp_a"
> > +	"dsp_b"
> > +	"ac97"
> > +	"pdm"
> > +	"msb"
> > +	"lsb"

> What do these mean? Why are they not described when the property was defined above?

This is another one where the names should be clear for people familiar
with the hardware, they're well known terms.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH v3] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark (Brown), Mark (Rutland)

I had sent v4 patch, but I removed simple-card,card-name on it.
But, as Mark (Brown) explained, I think simple-card,card-name
is very helpful for users.
I can send v5 patch (with it), or incremental patch if we can have it.
So, I need your comment

> [1  <multipart/signed (7bit)>]
> [1.1  <text/plain; us-ascii (7bit)>]
> On Thu, Oct 24, 2013 at 06:17:59PM +0100, Mark Rutland wrote:
> > On Fri, Oct 04, 2013 at 01:04:41AM +0100, Kuninori Morimoto wrote:
> 
> > > +- simple-audio,card-name		: simple-audio card name
> 
> > What's this used for?
> 
> This serves two useful functions.  One is that this is used for display
> to users so they have a friendly name for the sound card (it is
> relatively common to have multiple sound cards in the system).  The
> other is that it is essentially a compatibility string for configuration
> - you get a lot of sound devices that are electrically identical and
> hence look the same from a driver point of view but due different
> physical form factors should be configured differently.
> 
> > > +- format				: specific format if needed, see below
> > > +- frame-master				: frame master
> > > +- bitclock-master			: bitclock master
> > > +- bitclock-inversion			: clock inversion
> > > +- frame-inversion			: frame inversion
> 
> > What do these mean? Repeating the name without a dash is completely unhelpful.
> > Describe what these imply.
> 
> These are all boolean propeties.  The meanings should be obvious or at
> least very easily discoverable to anyone with any familiarity with audio
> hardware; if you can understand what to do with them they should be OK.
> 
> > > +- clocks				: phandle for system clock rate
> 
> > Just one clock?
> 
> This is a limitation from the simple card, anything that needs more
> complex clocking should be using a different binding.
> 
> > > +- system-clock-frequency		: system clock rate
> > > +					  it will overwrite clocks's rate
> 
> > This seems very odd.
> 
> > Why do you want to overwrite a clock's rate?
> 
> It's relatively common to derive the audio clock from a general purpose
> programmable clock which needs to be configured appropriately for use.
> 
> > > +simple-audio,format
> > > +	"i2s"
> > > +	"right_j"
> > > +	"left_j"
> > > +	"dsp_a"
> > > +	"dsp_b"
> > > +	"ac97"
> > > +	"pdm"
> > > +	"msb"
> > > +	"lsb"
> 
> > What do these mean? Why are they not described when the property was defined above?
> 
> This is another one where the names should be clear for people familiar
> with the hardware, they're well known terms.
> [1.2 Digital signature <application/pgp-signature (7bit)>]
> 
> [2  <text/plain; us-ascii (7bit)>]
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel


Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v3] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Tue, Oct 29, 2013 at 05:39:17PM -0700, Kuninori Morimoto wrote:

> I had sent v4 patch, but I removed simple-card,card-name on it.
> But, as Mark (Brown) explained, I think simple-card,card-name
> is very helpful for users.
> I can send v5 patch (with it), or incremental patch if we can have it.
> So, I need your comment

Let's review the existing bindings as-is unless there's a need to revise
for some other reason, it's easy enough to add the property back later.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH v3] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark

> > I had sent v4 patch, but I removed simple-card,card-name on it.
> > But, as Mark (Brown) explained, I think simple-card,card-name
> > is very helpful for users.
> > I can send v5 patch (with it), or incremental patch if we can have it.
> > So, I need your comment
> 
> Let's review the existing bindings as-is unless there's a need to revise
> for some other reason, it's easy enough to add the property back later.

OK, I see
Thank you.

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi

I would like to know the current status of this patch

> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
> v3 -> v4
> 
>  - explain detail of each properties on simple-card.txt
>  - fixup odd examples on simple-card.txt
>  - remove "simple-card,card-name". create it from cpu/codec name
>  - use of_get_child_by_name()
>  - remove odd pointer info from dev_dbg()
>  - remove subnode format which are no longer needed
> 
> This is based on asoc/topic/simple branch
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++
>  sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
>  2 files changed, 223 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..4871e91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,73 @@
> +Simple-Card:
> +
> +Simple-Card specifies audio DAI connection of SoC <-> codec.
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common audio format.
> +					"i2s", "right_j", "left_j" , "dsp_a"
> +					"dsp_b", "ac97", "pdm", "msb", "lsb"
> +Required subnodes:
> +
> +- simple-audio,cpu			: CPU   sub-node
> +- simple-audio,codec			: CODEC sub-node
> +
> +Required CPU/CODEC subnodes properties:
> +
> +- sound-dai				: phandle and port of CPU/CODEC
> +
> +Optional CPU/CODEC subnodes properties:
> +
> +- frame-master				: bool property. add this if subnode was frame master
> +- bitclock-master			: bool property. add this if subnode was bitclock master
> +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> +- frame-inversion			: bool property. add this if subnode has frame inversion
> +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> +					  it can be specified via "clocks" if system has clock node,
> +                                          or "system-clock-frequency" if system doesn't have it.
> +
> +Example:
> +
> +clock {
> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};
> +
> +sound {
> +	compatible = "simple-audio";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +	};
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@12 {
> +		#sound-dai-cells = <0>;
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +	};
> +};
> +
> +sh_fsi2: sh_fsi2@ec230000 {
> +	#sound-dai-cells = <1>;
> +	compatible = "renesas,sh_fsi2";
> +	reg = <0xec230000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupts = <0 146 0x4>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index b2fbb70..c0fb635 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -8,7 +8,8 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> -
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -57,11 +58,144 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +asoc_simple_card_sub_parse_of(struct device_node *np,
> +			      struct asoc_simple_dai *dai,
> +			      struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * get node via "sound-dai = <&phandle port>"
> +	 * it will be used as xxx_of_node on soc_bind_dai_link()
> +	 */
> +	*node = of_parse_phandle(np, "sound-dai", 0);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		goto parse_error;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/*
> +	 * dai->sysclk come from
> +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
> +	 */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		of_property_read_u32(np,
> +				     "system-clock-frequency",
> +				     &dai->sysclk);
> +	else
> +		dai->sysclk = clk_get_rate(clk);
> +
> +	ret = 0;
> +
> +parse_error:
> +	of_node_put(*node);
> +
> +	return ret;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	char *name;
> +	int ret = 0;
> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,cpu");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->cpu_dai,
> +						  of_cpu);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* CODEC sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,codec");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->codec_dai,
> +						  of_codec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* card name is created from CPU/CODEC dai name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	name = devm_kzalloc(dev,
> +			    strlen(info->cpu_dai.name)   +
> +			    strlen(info->codec_dai.name) + 2,
> +			    GFP_KERNEL);
> +	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
> +	info->name = info->card = name;
> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x\n", info->daifmt);
> +	dev_dbg(dev, "cpu : %s / %04x / %d\n",
> +		info->cpu_dai.name,
> +		info->cpu_dai.fmt,
> +		info->cpu_dai.sysclk);
> +	dev_dbg(dev, "codec : %s / %04x / %d\n",
> +		info->codec_dai.name,
> +		info->codec_dai.fmt,
> +		info->codec_dai.sysclk);
> +
> +	return 0;
> +}
> +
>  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_node *of_cpu, *of_codec, *of_platform;
>  	struct device *dev = &pdev->dev;
>  
> +	cinfo		= NULL;
> +	of_cpu		= NULL;
> +	of_codec	= NULL;
> +	of_platform	= NULL;
> +	if (np && of_device_is_available(np)) {
> +		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
> +		if (cinfo) {
> +			int ret;
> +			ret = asoc_simple_card_parse_of(np, cinfo, dev,
> +							&of_cpu,
> +							&of_codec,
> +							&of_platform);
> +			if (ret < 0) {
> +				if (ret != -EPROBE_DEFER)
> +					dev_err(dev, "parse error %d\n", ret);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		cinfo = pdev->dev.platform_data;
> +	}
> +
>  	if (!cinfo) {
>  		dev_err(dev, "no info for asoc-simple-card\n");
>  		return -EINVAL;
> @@ -69,10 +203,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  
>  	if (!cinfo->name	||
>  	    !cinfo->card	||
> -	    !cinfo->codec	||
> -	    !cinfo->platform	||
> -	    !cinfo->cpu_dai.name ||
> -	    !cinfo->codec_dai.name) {
> +	    !cinfo->codec_dai.name	||
> +	    !(cinfo->codec		|| of_codec)	||
> +	    !(cinfo->platform		|| of_platform)	||
> +	    !(cinfo->cpu_dai.name	|| of_cpu)) {
>  		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
>  		return -EINVAL;
>  	}
> @@ -86,6 +220,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  	cinfo->snd_link.platform_name	= cinfo->platform;
>  	cinfo->snd_link.codec_name	= cinfo->codec;
>  	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
> +	cinfo->snd_link.cpu_of_node	= of_cpu;
> +	cinfo->snd_link.codec_of_node	= of_codec;
> +	cinfo->snd_link.platform_of_node = of_platform;
>  	cinfo->snd_link.init		= asoc_simple_card_dai_init;
>  
>  	/*
> @@ -107,10 +244,17 @@ 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 = "simple-audio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
> +
>  static struct platform_driver asoc_simple_card = {
>  	.driver = {
>  		.name	= "asoc-simple-card",
>  		.owner = THIS_MODULE,
> +		.of_match_table = asoc_simple_of_match,
>  	},
>  	.probe		= asoc_simple_card_probe,
>  	.remove		= asoc_simple_card_remove,
> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Thu, Nov 14, 2013 at 09:13:24PM -0800, Kuninori Morimoto wrote:

> I would like to know the current status of this patch

I'm still hoping that some of the DT maintainers will review things.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Rutland]

On Fri, Oct 25, 2013 at 03:14:20AM +0100, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v3 -> v4
> 
>  - explain detail of each properties on simple-card.txt
>  - fixup odd examples on simple-card.txt
>  - remove "simple-card,card-name". create it from cpu/codec name
>  - use of_get_child_by_name()
>  - remove odd pointer info from dev_dbg()
>  - remove subnode format which are no longer needed
> 
> This is based on asoc/topic/simple branch
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++
>  sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
>  2 files changed, 223 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..4871e91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,73 @@
> +Simple-Card:
> +
> +Simple-Card specifies audio DAI connection of SoC <-> codec.
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common audio format.
> +					"i2s", "right_j", "left_j" , "dsp_a"
> +					"dsp_b", "ac97", "pdm", "msb", "lsb"
> +Required subnodes:
> +
> +- simple-audio,cpu			: CPU   sub-node
> +- simple-audio,codec			: CODEC sub-node
> +
> +Required CPU/CODEC subnodes properties:
> +
> +- sound-dai				: phandle and port of CPU/CODEC

Is there a class binding for audio devices this derives from?

> +
> +Optional CPU/CODEC subnodes properties:

Do these all apply to both sub-nodes?

> +- frame-master				: bool property. add this if subnode was frame master
> +- bitclock-master			: bool property. add this if subnode was bitclock master

s/was/is/

> +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> +- frame-inversion			: bool property. add this if subnode has frame inversion
> +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> +					  it can be specified via "clocks" if system has clock node,
> +                                          or "system-clock-frequency" if system doesn't have it.

What does "if system doesn't have it" mean? If it doesn't have a clock,
how does said non-existent clock have a frequency?

It would be possible to use a fixed-clock in place of
system-clock-frequency, which would make the binding more consistent and
the driver simpler, at the cost of making the dt marginally more
complex.

> +
> +Example:
> +
> +clock {

Why is this container here? It's confusing and unnecessary.

> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};

[...]

> +static int
> +asoc_simple_card_sub_parse_of(struct device_node *np,
> +			      struct asoc_simple_dai *dai,
> +			      struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * get node via "sound-dai = <&phandle port>"
> +	 * it will be used as xxx_of_node on soc_bind_dai_link()
> +	 */
> +	*node = of_parse_phandle(np, "sound-dai", 0);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		goto parse_error;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */

This comment looks confusing to me. I'm not sure it's all that helpful.

> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);

As a general note, I'm surprised there isn't a helper that does all of
the above, from of_parse_phandle to here.

> +
> +	/*
> +	 * dai->sysclk come from
> +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"

s/clolks/clocks/

> +	 */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		of_property_read_u32(np,
> +				     "system-clock-frequency",
> +				     &dai->sysclk);

What it this isn't present?

> +	else
> +		dai->sysclk = clk_get_rate(clk);
> +
> +	ret = 0;
> +
> +parse_error:
> +	of_node_put(*node);
> +
> +	return ret;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	char *name;
> +	int ret = 0;
> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,cpu");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->cpu_dai,
> +						  of_cpu);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* CODEC sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,codec");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->codec_dai,
> +						  of_codec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* card name is created from CPU/CODEC dai name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	name = devm_kzalloc(dev,
> +			    strlen(info->cpu_dai.name)   +
> +			    strlen(info->codec_dai.name) + 2,
> +			    GFP_KERNEL);
> +	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
> +	info->name = info->card = name;
> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;

Why? What does this imply?

Thanks,
Mark.

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark Rutland

Thank you for your feedback

> > +- frame-master				: bool property. add this if subnode was frame master
> > +- bitclock-master			: bool property. add this if subnode was bitclock master
> 
> s/was/is/

Oops, I will fix it on v5

> > +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> > +- frame-inversion			: bool property. add this if subnode has frame inversion
> > +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> > +					  it can be specified via "clocks" if system has clock node,
> > +                                          or "system-clock-frequency" if system doesn't have it.
> 
> What does "if system doesn't have it" mean? If it doesn't have a clock,
> how does said non-existent clock have a frequency?

It means "if system doesn't support common clock".
I will fix it

> > +static int
> > +asoc_simple_card_sub_parse_of(struct device_node *np,
> > +			      struct asoc_simple_dai *dai,
> > +			      struct device_node **node)
> > +{
> > +	struct clk *clk;
> > +	int ret;
> > +
> > +	/*
> > +	 * get node via "sound-dai = <&phandle port>"
> > +	 * it will be used as xxx_of_node on soc_bind_dai_link()
> > +	 */
> > +	*node = of_parse_phandle(np, "sound-dai", 0);
> > +	if (!*node)
> > +		return -ENODEV;
> > +
> > +	/* get dai->name */
> > +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> > +	if (ret < 0)
> > +		goto parse_error;
> > +
> > +	/*
> > +	 * bitclock-inversion, frame-inversion
> > +	 * bitclock-master,    frame-master
> > +	 * and specific "format" if it has
> > +	 */
> 
> This comment looks confusing to me. I'm not sure it's all that helpful.
> 
> > +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> 
> As a general note, I'm surprised there isn't a helper that does all of
> the above, from of_parse_phandle to here.

snd_soc_of_parse_daifmt() is the helper fucntion,
and above comment is explaining it.
Or, am I misunderstanding your review ?

> > +	/*
> > +	 * dai->sysclk come from
> > +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
> 
> s/clolks/clocks/

Grr, thank you

> > +	clk = of_clk_get(np, 0);
> > +	if (IS_ERR(clk))
> > +		of_property_read_u32(np,
> > +				     "system-clock-frequency",
> > +				     &dai->sysclk);
> 
> What it this isn't present?

If sysclk doesn't have common clock support

> > +	/* simple-card assumes platform == cpu */
> > +	*of_platform = *of_cpu;
> 
> Why? What does this imply?

This is the one of reason why this driver is "simple" card

Best regards
---
Kuninori Morimoto

[PATCH v5] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v4 -> v5

 - fixup spell miss
 - removed un-needed "clock node" example from simple-card.txt
 - add explain that clocks can be used if system has "common clock"

 .../devicetree/bindings/sound/simple-card.txt      |   66 ++++++++
 sound/soc/generic/simple-card.c                    |  157 +++++++++++++++++++-
 2 files changed, 217 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..615a655
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,66 @@
+Simple-Card:
+
+Simple-Card specifies audio DAI connection of SoC <-> codec.
+
+Required properties:
+
+- compatible				: "simple-audio"
+
+Optional properties:
+
+- simple-audio,format			: CPU/CODEC common audio format.
+					"i2s", "right_j", "left_j" , "dsp_a"
+					"dsp_b", "ac97", "pdm", "msb", "lsb"
+Required subnodes:
+
+- simple-audio,cpu			: CPU   sub-node
+- simple-audio,codec			: CODEC sub-node
+
+Required CPU/CODEC subnodes properties:
+
+- sound-dai				: phandle and port of CPU/CODEC
+
+Optional CPU/CODEC subnodes properties:
+
+- frame-master				: bool property. add this if subnode is frame master
+- bitclock-master			: bool property. add this if subnode is bitclock master
+- bitclock-inversion			: bool property. add this if subnode has clock inversion
+- frame-inversion			: bool property. add this if subnode has frame inversion
+- clocks / system-clock-frequency	: specify subnode's clock if needed.
+					  it can be specified via "clocks" if system has
+					  clock node (= common clock),
+					  or "system-clock-frequency" if system can't use it.
+
+Example:
+
+sound {
+	compatible = "simple-audio";
+	simple-audio,format = "left_j";
+
+	simple-audio,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+	};
+};
+
+&i2c0 {
+	ak4648: ak4648@12 {
+		#sound-dai-cells = <0>;
+		compatible = "asahi-kasei,ak4648";
+		reg = <0x12>;
+	};
+};
+
+sh_fsi2: sh_fsi2@ec230000 {
+	#sound-dai-cells = <1>;
+	compatible = "renesas,sh_fsi2";
+	reg = <0xec230000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 146 0x4>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index b2fbb70..da1fd7e 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,7 +8,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -57,11 +58,145 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+asoc_simple_card_sub_parse_of(struct device_node *np,
+			      struct asoc_simple_dai *dai,
+			      struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * get node via "sound-dai = <&phandle port>"
+	 * it will be used as xxx_of_node on soc_bind_dai_link()
+	 */
+	*node = of_parse_phandle(np, "sound-dai", 0);
+	if (!*node)
+		return -ENODEV;
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		goto parse_error;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/*
+	 * dai->sysclk come from
+	 *  "clocks = <&xxx>" (if system has common clock)
+	 *  or "system-clock-frequency = <xxx>"
+	 */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		of_property_read_u32(np,
+				     "system-clock-frequency",
+				     &dai->sysclk);
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	ret = 0;
+
+parse_error:
+	of_node_put(*node);
+
+	return ret;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	char *name;
+	int ret = 0;
+
+	/* get CPU/CODEC common format via simple-audio,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio,cpu");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->cpu_dai,
+						  of_cpu);
+	if (ret < 0)
+		return ret;
+
+	/* CODEC sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio,codec");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->codec_dai,
+						  of_codec);
+	if (ret < 0)
+		return ret;
+
+	/* card name is created from CPU/CODEC dai name */
+	of_property_read_string(node, "simple-audio,card-name", &info->card);
+	name = devm_kzalloc(dev,
+			    strlen(info->cpu_dai.name)   +
+			    strlen(info->codec_dai.name) + 2,
+			    GFP_KERNEL);
+	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
+	info->name = info->card = name;
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x\n", info->daifmt);
+	dev_dbg(dev, "cpu : %s / %04x / %d\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk);
+	dev_dbg(dev, "codec : %s / %04x / %d\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -69,10 +204,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -86,6 +221,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -107,10 +245,17 @@ 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 = "simple-audio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Rutland]

On Mon, Nov 18, 2013 at 12:42:23AM +0000, Kuninori Morimoto wrote:
> 
> Hi Mark Rutland
> 
> Thank you for your feedback
> 
> > > +- frame-master				: bool property. add this if subnode was frame master
> > > +- bitclock-master			: bool property. add this if subnode was bitclock master
> > 
> > s/was/is/
> 
> Oops, I will fix it on v5
> 
> > > +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> > > +- frame-inversion			: bool property. add this if subnode has frame inversion
> > > +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> > > +					  it can be specified via "clocks" if system has clock node,
> > > +                                          or "system-clock-frequency" if system doesn't have it.
> > 
> > What does "if system doesn't have it" mean? If it doesn't have a clock,
> > how does said non-existent clock have a frequency?
> 
> It means "if system doesn't support common clock".
> I will fix it

When you say "doesn't support common clock", you mean the code for that
platform is incompatible with the common clock framework? It seems very
messy to allow a Linux-internal implementation detail (which is expected
to change) to leak into a binding...

> 
> > > +static int
> > > +asoc_simple_card_sub_parse_of(struct device_node *np,
> > > +			      struct asoc_simple_dai *dai,
> > > +			      struct device_node **node)
> > > +{
> > > +	struct clk *clk;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * get node via "sound-dai = <&phandle port>"
> > > +	 * it will be used as xxx_of_node on soc_bind_dai_link()
> > > +	 */
> > > +	*node = of_parse_phandle(np, "sound-dai", 0);
> > > +	if (!*node)
> > > +		return -ENODEV;
> > > +
> > > +	/* get dai->name */
> > > +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> > > +	if (ret < 0)
> > > +		goto parse_error;
> > > +
> > > +	/*
> > > +	 * bitclock-inversion, frame-inversion
> > > +	 * bitclock-master,    frame-master
> > > +	 * and specific "format" if it has
> > > +	 */
> > 
> > This comment looks confusing to me. I'm not sure it's all that helpful.
> > 
> > > +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> > 
> > As a general note, I'm surprised there isn't a helper that does all of
> > the above, from of_parse_phandle to here.
> 
> snd_soc_of_parse_daifmt() is the helper fucntion,
> and above comment is explaining it.
> Or, am I misunderstanding your review ?
> 

What I meant was that I am surprised there isn't a helper doing all of:
* of_parse_phandle
* snd_soc_of_get_dai_name
* snd_soc_of_parse_daifmt

It looks like a common pattern that could be factored out.

> > > +	/*
> > > +	 * dai->sysclk come from
> > > +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
> > 
> > s/clolks/clocks/
> 
> Grr, thank you
> 
> > > +	clk = of_clk_get(np, 0);
> > > +	if (IS_ERR(clk))
> > > +		of_property_read_u32(np,
> > > +				     "system-clock-frequency",
> > > +				     &dai->sysclk);
> > 
> > What it this isn't present?
> 
> If sysclk doesn't have common clock support

Huh? That's not what I asked.

What if the dt has neither a clock or a system-clock-frequency property?

> 
> > > +	/* simple-card assumes platform == cpu */
> > > +	*of_platform = *of_cpu;
> > 
> > Why? What does this imply?
> 
> This is the one of reason why this driver is "simple" card

The issue here is that I'm almost totally unfamiliar with audio
hardware, nor the ASoC bindings. If this makes sense to you and Mark
Brown then I'm happy to continue not understanding either. :)

Thanks,
Mark.

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Mon, Nov 18, 2013 at 11:36:18AM +0000, Mark Rutland wrote:

> What I meant was that I am surprised there isn't a helper doing all of:
> * of_parse_phandle
> * snd_soc_of_get_dai_name
> * snd_soc_of_parse_daifmt

> It looks like a common pattern that could be factored out.

This is the factoring out of the common pattern, other drivers will only
need the compatible string to work out the format.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Rob Herring]

On Fri, Nov 15, 2013 at 9:50 AM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Fri, Oct 25, 2013 at 03:14:20AM +0100, Kuninori Morimoto wrote:
>> Support for loading the simple-card module via DeviceTree.
>> It requests CPU/CODEC information.
>>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
>> ---
>> v3 -> v4
>>
>>  - explain detail of each properties on simple-card.txt
>>  - fixup odd examples on simple-card.txt
>>  - remove "simple-card,card-name". create it from cpu/codec name
>>  - use of_get_child_by_name()
>>  - remove odd pointer info from dev_dbg()
>>  - remove subnode format which are no longer needed
>>
>> This is based on asoc/topic/simple branch
>>
>>  .../devicetree/bindings/sound/simple-card.txt      |   73 +++++++++
>>  sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
>>  2 files changed, 223 insertions(+), 6 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
>> new file mode 100644
>> index 0000000..4871e91
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
>> @@ -0,0 +1,73 @@
>> +Simple-Card:
>> +
>> +Simple-Card specifies audio DAI connection of SoC <-> codec.
>> +
>> +Required properties:
>> +
>> +- compatible                         : "simple-audio"

Still not really a fan of this generic name. Can we define in the
description above what simple means.

>> +
>> +Optional properties:
>> +
>> +- simple-audio,format                        : CPU/CODEC common audio format.
>> +                                     "i2s", "right_j", "left_j" , "dsp_a"
>> +                                     "dsp_b", "ac97", "pdm", "msb", "lsb"
>> +Required subnodes:
>> +
>> +- simple-audio,cpu                   : CPU   sub-node
>> +- simple-audio,codec                 : CODEC sub-node
>> +
>> +Required CPU/CODEC subnodes properties:
>> +
>> +- sound-dai                          : phandle and port of CPU/CODEC
>
> Is there a class binding for audio devices this derives from?
>
>> +
>> +Optional CPU/CODEC subnodes properties:
>
> Do these all apply to both sub-nodes?
>
>> +- frame-master                               : bool property. add this if subnode was frame master
>> +- bitclock-master                    : bool property. add this if subnode was bitclock master
>
> s/was/is/
>
>> +- bitclock-inversion                 : bool property. add this if subnode has clock inversion
>> +- frame-inversion                    : bool property. add this if subnode has frame inversion
>> +- clocks / system-clock-frequency    : specify subnode's clock if needed.
>> +                                       it can be specified via "clocks" if system has clock node,
>> +                                          or "system-clock-frequency" if system doesn't have it.
>
> What does "if system doesn't have it" mean? If it doesn't have a clock,
> how does said non-existent clock have a frequency?
>
> It would be possible to use a fixed-clock in place of
> system-clock-frequency, which would make the binding more consistent and
> the driver simpler, at the cost of making the dt marginally more
> complex.

Just plain "clock-frequency" is fairly standard, so please use that
instead. Unless there is need for a fixed-clock to be routed to
several nodes, then I think using the more simple clock-frequency here
is fine.

Can both sub-nodes really have different clocks? Seems like that would
exceed the definition of simple.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Mon, Nov 18, 2013 at 08:12:06AM -0600, Rob Herring wrote:

> Can both sub-nodes really have different clocks? Seems like that would
> exceed the definition of simple.

In theory, though it would be unusual.  What is more normal that the
clock is only connected to one of the nodes and we need to know which it
is.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark Rutland

Thank you for your feedback

> > It means "if system doesn't support common clock".
> > I will fix it
> 
> When you say "doesn't support common clock", you mean the code for that
> platform is incompatible with the common clock framework? It seems very
> messy to allow a Linux-internal implementation detail (which is expected
> to change) to leak into a binding...

Some CPU doesn't support common clock, like PowerPC (?)
This is Mark (Brown) comment

--------------------
So, ideally.  However we have to consider the fact that the clock API
isn't reliably available makes this harder than it should be.  Even
among the DT using platforms at least PowerPC still uses a custom clock
API.  We could just use this as a carrot to push people to convert
though.
---------------------


> > > > +		of_property_read_u32(np,
> > > > +				     "system-clock-frequency",
> > > > +				     &dai->sysclk);
> > > 
> > > What it this isn't present?
> > 
> > If sysclk doesn't have common clock support
> 
> Huh? That's not what I asked.
> 
> What if the dt has neither a clock or a system-clock-frequency property?

OK, sorry for my English

This happen if cpu/codec was slave, not strange things.
Please check "Example". in there, "simple-audio,codec" has clocks
but, "simple-audio,cpu" doesn't have it, and "simple-audio,codec" has
"bitclock-master" and "frame-master";
This case, codec chip creates audio play/capture clock from system clock,
and "cpu" use it.
This is the reason why the name is "system-clock-frequency" instead of "clock-frequency"
The image is like this

 clocks /    +- simple card --+
system clock |                |-> playback
-------------+-[codec]--[cpu] |
             |                |<- capture
             +----------------+

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Rob, Mark

> >> +Required properties:
> >> +
> >> +- compatible                         : "simple-audio"
> 
> Still not really a fan of this generic name. Can we define in the
> description above what simple means.

So, how about "simple-audio-card" ?

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Support for loading the simple-card module via DeviceTree.
It requests CPU/CODEC information.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v4 -> v5

 - fixup spell miss
 - removed un-needed "clock node" example from simple-card.txt
 - add explain that clocks can be used if system has "common clock"

v5 -> v6

 - simple-audio => simple-audio-card

 .../devicetree/bindings/sound/simple-card.txt      |   68 +++++++++
 sound/soc/generic/simple-card.c                    |  156 +++++++++++++++++++-
 2 files changed, 218 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
new file mode 100644
index 0000000..769a346
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -0,0 +1,68 @@
+Simple-Card:
+
+Simple-Card specifies audio DAI connection of SoC <-> codec.
+
+Required properties:
+
+- compatible				: "simple-audio-card"
+
+Optional properties:
+
+- simple-audio-card,format		: CPU/CODEC common audio format.
+					"i2s", "right_j", "left_j" , "dsp_a"
+					"dsp_b", "ac97", "pdm", "msb", "lsb"
+Required subnodes:
+
+- simple-audio-card,cpu			: CPU   sub-node
+- simple-audio-card,codec		: CODEC sub-node
+
+Required CPU/CODEC subnodes properties:
+
+- sound-dai				: phandle and port of CPU/CODEC
+
+Optional CPU/CODEC subnodes properties:
+
+- format				: CPU/CODEC specific audio format if needed.
+					  see simple-audio-card,format
+- frame-master				: bool property. add this if subnode is frame master
+- bitclock-master			: bool property. add this if subnode is bitclock master
+- bitclock-inversion			: bool property. add this if subnode has clock inversion
+- frame-inversion			: bool property. add this if subnode has frame inversion
+- clocks / system-clock-frequency	: specify subnode's clock if needed.
+					  it can be specified via "clocks" if system has
+					  clock node (= common clock), or "system-clock-frequency"
+					  (if system doens't support common clock)
+
+Example:
+
+sound {
+	compatible = "simple-audio-card";
+	simple-audio-card,format = "left_j";
+
+	simple-audio-card,cpu {
+		sound-dai = <&sh_fsi2 0>;
+	};
+
+	simple-audio-card,codec {
+		sound-dai = <&ak4648>;
+		bitclock-master;
+		frame-master;
+		clocks = <&osc>;
+	};
+};
+
+&i2c0 {
+	ak4648: ak4648@12 {
+		#sound-dai-cells = <0>;
+		compatible = "asahi-kasei,ak4648";
+		reg = <0x12>;
+	};
+};
+
+sh_fsi2: sh_fsi2@ec230000 {
+	#sound-dai-cells = <1>;
+	compatible = "renesas,sh_fsi2";
+	reg = <0xec230000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 146 0x4>;
+};
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index b2fbb70..7a9b6b4 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,7 +8,8 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-
+#include <linux/clk.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <sound/simple_card.h>
@@ -57,11 +58,144 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int
+asoc_simple_card_sub_parse_of(struct device_node *np,
+			      struct asoc_simple_dai *dai,
+			      struct device_node **node)
+{
+	struct clk *clk;
+	int ret;
+
+	/*
+	 * get node via "sound-dai = <&phandle port>"
+	 * it will be used as xxx_of_node on soc_bind_dai_link()
+	 */
+	*node = of_parse_phandle(np, "sound-dai", 0);
+	if (!*node)
+		return -ENODEV;
+
+	/* get dai->name */
+	ret = snd_soc_of_get_dai_name(np, &dai->name);
+	if (ret < 0)
+		goto parse_error;
+
+	/*
+	 * bitclock-inversion, frame-inversion
+	 * bitclock-master,    frame-master
+	 * and specific "format" if it has
+	 */
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+
+	/*
+	 * dai->sysclk come from
+	 *  "clocks = <&xxx>" (if system has common clock)
+	 *  or "system-clock-frequency = <xxx>"
+	 */
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		of_property_read_u32(np,
+				     "system-clock-frequency",
+				     &dai->sysclk);
+	else
+		dai->sysclk = clk_get_rate(clk);
+
+	ret = 0;
+
+parse_error:
+	of_node_put(*node);
+
+	return ret;
+}
+
+static int asoc_simple_card_parse_of(struct device_node *node,
+				     struct asoc_simple_card_info *info,
+				     struct device *dev,
+				     struct device_node **of_cpu,
+				     struct device_node **of_codec,
+				     struct device_node **of_platform)
+{
+	struct device_node *np;
+	char *name;
+	int ret = 0;
+
+	/* get CPU/CODEC common format via simple-audio-card,format */
+	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
+		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
+
+	/* CPU sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio-card,cpu");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->cpu_dai,
+						  of_cpu);
+	if (ret < 0)
+		return ret;
+
+	/* CODEC sub-node */
+	ret = -EINVAL;
+	np = of_get_child_by_name(node, "simple-audio-card,codec");
+	if (np)
+		ret = asoc_simple_card_sub_parse_of(np,
+						  &info->codec_dai,
+						  of_codec);
+	if (ret < 0)
+		return ret;
+
+	/* card name is created from CPU/CODEC dai name */
+	name = devm_kzalloc(dev,
+			    strlen(info->cpu_dai.name)   +
+			    strlen(info->codec_dai.name) + 2,
+			    GFP_KERNEL);
+	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
+	info->name = info->card = name;
+
+	/* simple-card assumes platform == cpu */
+	*of_platform = *of_cpu;
+
+	dev_dbg(dev, "card-name : %s\n", info->card);
+	dev_dbg(dev, "platform : %04x\n", info->daifmt);
+	dev_dbg(dev, "cpu : %s / %04x / %d\n",
+		info->cpu_dai.name,
+		info->cpu_dai.fmt,
+		info->cpu_dai.sysclk);
+	dev_dbg(dev, "codec : %s / %04x / %d\n",
+		info->codec_dai.name,
+		info->codec_dai.fmt,
+		info->codec_dai.sysclk);
+
+	return 0;
+}
+
 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_node *of_cpu, *of_codec, *of_platform;
 	struct device *dev = &pdev->dev;
 
+	cinfo		= NULL;
+	of_cpu		= NULL;
+	of_codec	= NULL;
+	of_platform	= NULL;
+	if (np && of_device_is_available(np)) {
+		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
+		if (cinfo) {
+			int ret;
+			ret = asoc_simple_card_parse_of(np, cinfo, dev,
+							&of_cpu,
+							&of_codec,
+							&of_platform);
+			if (ret < 0) {
+				if (ret != -EPROBE_DEFER)
+					dev_err(dev, "parse error %d\n", ret);
+				return ret;
+			}
+		}
+	} else {
+		cinfo = pdev->dev.platform_data;
+	}
+
 	if (!cinfo) {
 		dev_err(dev, "no info for asoc-simple-card\n");
 		return -EINVAL;
@@ -69,10 +203,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (!cinfo->name	||
 	    !cinfo->card	||
-	    !cinfo->codec	||
-	    !cinfo->platform	||
-	    !cinfo->cpu_dai.name ||
-	    !cinfo->codec_dai.name) {
+	    !cinfo->codec_dai.name	||
+	    !(cinfo->codec		|| of_codec)	||
+	    !(cinfo->platform		|| of_platform)	||
+	    !(cinfo->cpu_dai.name	|| of_cpu)) {
 		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 		return -EINVAL;
 	}
@@ -86,6 +220,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	cinfo->snd_link.platform_name	= cinfo->platform;
 	cinfo->snd_link.codec_name	= cinfo->codec;
 	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
+	cinfo->snd_link.cpu_of_node	= of_cpu;
+	cinfo->snd_link.codec_of_node	= of_codec;
+	cinfo->snd_link.platform_of_node = of_platform;
 	cinfo->snd_link.init		= asoc_simple_card_dai_init;
 
 	/*
@@ -107,10 +244,17 @@ 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 = "simple-audio-card", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
+
 static struct platform_driver asoc_simple_card = {
 	.driver = {
 		.name	= "asoc-simple-card",
 		.owner = THIS_MODULE,
+		.of_match_table = asoc_simple_of_match,
 	},
 	.probe		= asoc_simple_card_probe,
 	.remove		= asoc_simple_card_remove,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Rob Herring]

On Mon, Nov 18, 2013 at 8:36 PM, Kuninori Morimoto
<kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
>
> Hi Rob, Mark
>
>> >> +Required properties:
>> >> +
>> >> +- compatible                         : "simple-audio"
>>
>> Still not really a fan of this generic name. Can we define in the
>> description above what simple means.
>
> So, how about "simple-audio-card" ?

That's missing my point. First, I think you should be defining the
actual h/w in the DT and doing the mapping of that to a simple audio
driver in the kernel. Otherwise how do you fix some quirk on a
particular platform later on without updating the DTB? I'm fine with
this being the default compatible string, but you should also require
a more specific name. Perhaps it is just <soc>-simple-audio or
<board>-simple-audio.

Second, you need to define in this binding document what simple means.
What properties of the audio subsystem make it simple? The h/w has and
doesn't have what? How do I decide if my platform can or should use
this binding?

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Wed, Nov 20, 2013 at 08:20:19AM -0600, Rob Herring wrote:

> Second, you need to define in this binding document what simple means.
> What properties of the audio subsystem make it simple? The h/w has and
> doesn't have what? How do I decide if my platform can or should use
> this binding?

I think it's reasonable to just say that a simple device is one that can
use this binding and that if that is not possible we need to make a
taste based decision at the time between extending this binding or using
a new one.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Rutland]

On Tue, Nov 19, 2013 at 02:03:21AM +0000, Kuninori Morimoto wrote:
> 
> Hi Mark Rutland
> 
> Thank you for your feedback
> 
> > > It means "if system doesn't support common clock".
> > > I will fix it
> > 
> > When you say "doesn't support common clock", you mean the code for that
> > platform is incompatible with the common clock framework? It seems very
> > messy to allow a Linux-internal implementation detail (which is expected
> > to change) to leak into a binding...
> 
> Some CPU doesn't support common clock, like PowerPC (?)
> This is Mark (Brown) comment
> 
> --------------------
> So, ideally.  However we have to consider the fact that the clock API
> isn't reliably available makes this harder than it should be.  Even
> among the DT using platforms at least PowerPC still uses a custom clock
> API.  We could just use this as a carrot to push people to convert
> though.
> ---------------------

I would be happier if we could unify the common clock infrastructure, it
would make this kind of thing a lot lessy messy. However, I'm not
against the system-clock-frequency property for now.

> 
> 
> > > > > +		of_property_read_u32(np,
> > > > > +				     "system-clock-frequency",
> > > > > +				     &dai->sysclk);
> > > > 
> > > > What it this isn't present?
> > > 
> > > If sysclk doesn't have common clock support
> > 
> > Huh? That's not what I asked.
> > 
> > What if the dt has neither a clock or a system-clock-frequency property?
> 
> OK, sorry for my English

Sorry for the confusion, I'll try to be less ambiguous in future :)

What I was trying to get at here is that if there is neither a clock or
a system-clock-frequency property in the device tree, dai->sysclk will
not have been initialised in this path. Is this a valid case, and will
dai->sysclk have a well-defined, sane value?

Thanks,
Mark.

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark Rutland

> > --------------------
> > So, ideally.  However we have to consider the fact that the clock API
> > isn't reliably available makes this harder than it should be.  Even
> > among the DT using platforms at least PowerPC still uses a custom clock
> > API.  We could just use this as a carrot to push people to convert
> > though.
> > ---------------------
> 
> I would be happier if we could unify the common clock infrastructure, it
> would make this kind of thing a lot lessy messy. However, I'm not
> against the system-clock-frequency property for now.

Thank you

> > OK, sorry for my English
> 
> Sorry for the confusion, I'll try to be less ambiguous in future :)
> 
> What I was trying to get at here is that if there is neither a clock or
> a system-clock-frequency property in the device tree, dai->sysclk will
> not have been initialised in this path. Is this a valid case, and will
> dai->sysclk have a well-defined, sane value?

My understanding, this "dai" itself is created by devm_kzalloc()
So, default dai->sysclk is 0.
And, if there is no clocks, no system-clock-frequency property,
it try of_property_read_u32() side. but it will do nothing to dai->sysclk
in such case. so dai->sysclk is still 0, and it is very sane on this driver.
Is this good answer ?

+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk))
+		of_property_read_u32(np,
+				     "system-clock-frequency",
+				     &dai->sysclk);
+	else
+		dai->sysclk = clk_get_rate(clk);


Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Rob

> >> Still not really a fan of this generic name. Can we define in the
> >> description above what simple means.
> >
> > So, how about "simple-audio-card" ?
> 
> That's missing my point. First, I think you should be defining the
> actual h/w in the DT and doing the mapping of that to a simple audio
> driver in the kernel. Otherwise how do you fix some quirk on a
> particular platform later on without updating the DTB? I'm fine with
> this being the default compatible string, but you should also require
> a more specific name. Perhaps it is just <soc>-simple-audio or
> <board>-simple-audio.
> 
> Second, you need to define in this binding document what simple means.
> What properties of the audio subsystem make it simple? The h/w has and
> doesn't have what? How do I decide if my platform can or should use
> this binding?

Basically, on ASoC case, SoC/board needs <soc>-<codec>-audio-card
(= not simple card) for matching each other, and this is start point.
This means we need many <soc>-<codec>-audio-card.c driver.
But, in some case, the difference between
<socA>-<codecA> <-> <socA>-<codecB> <-> <socB>-<codecA> <-> <socB>-<codecB>
was just "name". creating too many such driver was not sane for me.
This simple-audio is used in such case.
Of course we can update simple-audio feature
(if it is very simple/common feature)
but, if you need <soc/board>-audio-card which needs special feature,
you need to create such driver without using simple-card.
This is very normal approach on ASoC and there are many such driver.

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi

I would like to know current status of this patch

> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
> v4 -> v5
> 
>  - fixup spell miss
>  - removed un-needed "clock node" example from simple-card.txt
>  - add explain that clocks can be used if system has "common clock"
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   66 ++++++++
>  sound/soc/generic/simple-card.c                    |  157 +++++++++++++++++++-
>  2 files changed, 217 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..615a655
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,66 @@
> +Simple-Card:
> +
> +Simple-Card specifies audio DAI connection of SoC <-> codec.
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common audio format.
> +					"i2s", "right_j", "left_j" , "dsp_a"
> +					"dsp_b", "ac97", "pdm", "msb", "lsb"
> +Required subnodes:
> +
> +- simple-audio,cpu			: CPU   sub-node
> +- simple-audio,codec			: CODEC sub-node
> +
> +Required CPU/CODEC subnodes properties:
> +
> +- sound-dai				: phandle and port of CPU/CODEC
> +
> +Optional CPU/CODEC subnodes properties:
> +
> +- frame-master				: bool property. add this if subnode is frame master
> +- bitclock-master			: bool property. add this if subnode is bitclock master
> +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> +- frame-inversion			: bool property. add this if subnode has frame inversion
> +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> +					  it can be specified via "clocks" if system has
> +					  clock node (= common clock),
> +					  or "system-clock-frequency" if system can't use it.
> +
> +Example:
> +
> +sound {
> +	compatible = "simple-audio";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +	};
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@12 {
> +		#sound-dai-cells = <0>;
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +	};
> +};
> +
> +sh_fsi2: sh_fsi2@ec230000 {
> +	#sound-dai-cells = <1>;
> +	compatible = "renesas,sh_fsi2";
> +	reg = <0xec230000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupts = <0 146 0x4>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index b2fbb70..da1fd7e 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -8,7 +8,8 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> -
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -57,11 +58,145 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +asoc_simple_card_sub_parse_of(struct device_node *np,
> +			      struct asoc_simple_dai *dai,
> +			      struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * get node via "sound-dai = <&phandle port>"
> +	 * it will be used as xxx_of_node on soc_bind_dai_link()
> +	 */
> +	*node = of_parse_phandle(np, "sound-dai", 0);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		goto parse_error;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/*
> +	 * dai->sysclk come from
> +	 *  "clocks = <&xxx>" (if system has common clock)
> +	 *  or "system-clock-frequency = <xxx>"
> +	 */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		of_property_read_u32(np,
> +				     "system-clock-frequency",
> +				     &dai->sysclk);
> +	else
> +		dai->sysclk = clk_get_rate(clk);
> +
> +	ret = 0;
> +
> +parse_error:
> +	of_node_put(*node);
> +
> +	return ret;
> +}
> +
> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	char *name;
> +	int ret = 0;
> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,cpu");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->cpu_dai,
> +						  of_cpu);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* CODEC sub-node */
> +	ret = -EINVAL;
> +	np = of_get_child_by_name(node, "simple-audio,codec");
> +	if (np)
> +		ret = asoc_simple_card_sub_parse_of(np,
> +						  &info->codec_dai,
> +						  of_codec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* card name is created from CPU/CODEC dai name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	name = devm_kzalloc(dev,
> +			    strlen(info->cpu_dai.name)   +
> +			    strlen(info->codec_dai.name) + 2,
> +			    GFP_KERNEL);
> +	sprintf(name, "%s-%s", info->cpu_dai.name, info->codec_dai.name);
> +	info->name = info->card = name;
> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x\n", info->daifmt);
> +	dev_dbg(dev, "cpu : %s / %04x / %d\n",
> +		info->cpu_dai.name,
> +		info->cpu_dai.fmt,
> +		info->cpu_dai.sysclk);
> +	dev_dbg(dev, "codec : %s / %04x / %d\n",
> +		info->codec_dai.name,
> +		info->codec_dai.fmt,
> +		info->codec_dai.sysclk);
> +
> +	return 0;
> +}
> +
>  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_node *of_cpu, *of_codec, *of_platform;
>  	struct device *dev = &pdev->dev;
>  
> +	cinfo		= NULL;
> +	of_cpu		= NULL;
> +	of_codec	= NULL;
> +	of_platform	= NULL;
> +	if (np && of_device_is_available(np)) {
> +		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
> +		if (cinfo) {
> +			int ret;
> +			ret = asoc_simple_card_parse_of(np, cinfo, dev,
> +							&of_cpu,
> +							&of_codec,
> +							&of_platform);
> +			if (ret < 0) {
> +				if (ret != -EPROBE_DEFER)
> +					dev_err(dev, "parse error %d\n", ret);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		cinfo = pdev->dev.platform_data;
> +	}
> +
>  	if (!cinfo) {
>  		dev_err(dev, "no info for asoc-simple-card\n");
>  		return -EINVAL;
> @@ -69,10 +204,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  
>  	if (!cinfo->name	||
>  	    !cinfo->card	||
> -	    !cinfo->codec	||
> -	    !cinfo->platform	||
> -	    !cinfo->cpu_dai.name ||
> -	    !cinfo->codec_dai.name) {
> +	    !cinfo->codec_dai.name	||
> +	    !(cinfo->codec		|| of_codec)	||
> +	    !(cinfo->platform		|| of_platform)	||
> +	    !(cinfo->cpu_dai.name	|| of_cpu)) {
>  		dev_err(dev, "insufficient asoc_simple_card_info settings\n");
>  		return -EINVAL;
>  	}
> @@ -86,6 +221,9 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
>  	cinfo->snd_link.platform_name	= cinfo->platform;
>  	cinfo->snd_link.codec_name	= cinfo->codec;
>  	cinfo->snd_link.codec_dai_name	= cinfo->codec_dai.name;
> +	cinfo->snd_link.cpu_of_node	= of_cpu;
> +	cinfo->snd_link.codec_of_node	= of_codec;
> +	cinfo->snd_link.platform_of_node = of_platform;
>  	cinfo->snd_link.init		= asoc_simple_card_dai_init;
>  
>  	/*
> @@ -107,10 +245,17 @@ 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 = "simple-audio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
> +
>  static struct platform_driver asoc_simple_card = {
>  	.driver = {
>  		.name	= "asoc-simple-card",
>  		.owner = THIS_MODULE,
> +		.of_match_table = asoc_simple_of_match,
>  	},
>  	.probe		= asoc_simple_card_probe,
>  	.remove		= asoc_simple_card_remove,
> -- 
> 1.7.9.5
> 


Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] ASoC: simple-card: add Device Tree support

[Mark Brown]

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

On Thu, Nov 28, 2013 at 05:11:34PM -0800, Kuninori Morimoto wrote:

> I would like to know current status of this patch

Still hoping for DT review :/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Rob

ping ?

> > >> Still not really a fan of this generic name. Can we define in the
> > >> description above what simple means.
> > >
> > > So, how about "simple-audio-card" ?
> > 
> > That's missing my point. First, I think you should be defining the
> > actual h/w in the DT and doing the mapping of that to a simple audio
> > driver in the kernel. Otherwise how do you fix some quirk on a
> > particular platform later on without updating the DTB? I'm fine with
> > this being the default compatible string, but you should also require
> > a more specific name. Perhaps it is just <soc>-simple-audio or
> > <board>-simple-audio.
> > 
> > Second, you need to define in this binding document what simple means.
> > What properties of the audio subsystem make it simple? The h/w has and
> > doesn't have what? How do I decide if my platform can or should use
> > this binding?
> 
> Basically, on ASoC case, SoC/board needs <soc>-<codec>-audio-card
> (= not simple card) for matching each other, and this is start point.
> This means we need many <soc>-<codec>-audio-card.c driver.
> But, in some case, the difference between
> <socA>-<codecA> <-> <socA>-<codecB> <-> <socB>-<codecA> <-> <socB>-<codecB>
> was just "name". creating too many such driver was not sane for me.
> This simple-audio is used in such case.
> Of course we can update simple-audio feature
> (if it is very simple/common feature)
> but, if you need <soc/board>-audio-card which needs special feature,
> you need to create such driver without using simple-card.
> This is very normal approach on ASoC and there are many such driver.
> 
> Best regards
> ---
> Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] ASoC: simple-card: add Device Tree support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 427 bytes --]

On Wed, Nov 20, 2013 at 03:25:02PM +0900, Kuninori Morimoto wrote:
> Support for loading the simple-card module via DeviceTree.
> It requests CPU/CODEC information.

I had a look at the recent DT reviews and it seems like the review here
is both getting slower and focusing on more and more minor issues so I
think that really everything is OK here and I've applied this.

Thanks for your hard work and perseverence with this.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v4] ASoC: simple-card: add Device Tree support

[Mark Rutland]

On Thu, Nov 21, 2013 at 12:12:13AM +0000, Kuninori Morimoto wrote:
> 
> Hi Mark Rutland
> 
> > > --------------------
> > > So, ideally.  However we have to consider the fact that the clock API
> > > isn't reliably available makes this harder than it should be.  Even
> > > among the DT using platforms at least PowerPC still uses a custom clock
> > > API.  We could just use this as a carrot to push people to convert
> > > though.
> > > ---------------------
> > 
> > I would be happier if we could unify the common clock infrastructure, it
> > would make this kind of thing a lot lessy messy. However, I'm not
> > against the system-clock-frequency property for now.
> 
> Thank you
> 
> > > OK, sorry for my English
> > 
> > Sorry for the confusion, I'll try to be less ambiguous in future :)
> > 
> > What I was trying to get at here is that if there is neither a clock or
> > a system-clock-frequency property in the device tree, dai->sysclk will
> > not have been initialised in this path. Is this a valid case, and will
> > dai->sysclk have a well-defined, sane value?
> 
> My understanding, this "dai" itself is created by devm_kzalloc()
> So, default dai->sysclk is 0.
> And, if there is no clocks, no system-clock-frequency property,
> it try of_property_read_u32() side. but it will do nothing to dai->sysclk
> in such case. so dai->sysclk is still 0, and it is very sane on this driver.
> Is this good answer ?

That sounds fine to me. Just wanted to make sure. :)

Thanks,
Mark.

Re: [alsa-devel] [PATCH v6] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark

> > Support for loading the simple-card module via DeviceTree.
> > It requests CPU/CODEC information.
> 
> I had a look at the recent DT reviews and it seems like the review here
> is both getting slower and focusing on more and more minor issues so I
> think that really everything is OK here and I've applied this.
> 
> Thanks for your hard work and perseverence with this.

Thank you !!
Phew, we spent 1 year for this support ;P

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html