[PATCH v2] soc/lapis: add machine driver

[PATCH v2] soc/lapis: add machine driver

[Tomoya MORINAGA]

Add machine driver for LAPIS Semiconductor ML7213 IOH.

Signed-off-by: Tomoya MORINAGA <tomoya.rohm@gmail.com>
---
V2: Updates Mark's comments.
 - Sorted in Makefile and Kconfig position
 - Import I2C configuration processing from CODEC driver
 - Add widget and route map
 - Add initialization processing (ml26124_set_dai_sysclk)
 - Modify codec_dai_name settins
 - Use snd_soc_register_card not platform_device_alloc
 - Use return platform_driver_register not platform_driver_probe
---
 sound/soc/Kconfig                   |    1 +
 sound/soc/Makefile                  |    1 +
 sound/soc/lapis/Kconfig             |    6 +
 sound/soc/lapis/Makefile            |    4 +
 sound/soc/lapis/ml7213ioh-machine.c |  197 +++++++++++++++++++++++++++++++++++
 5 files changed, 209 insertions(+), 0 deletions(-)
 create mode 100644 sound/soc/lapis/Kconfig
 create mode 100644 sound/soc/lapis/Makefile
 create mode 100644 sound/soc/lapis/ml7213ioh-machine.c

diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 1381db8..e8339e6 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -52,6 +52,7 @@ source "sound/soc/jz4740/Kconfig"
 source "sound/soc/nuc900/Kconfig"
 source "sound/soc/omap/Kconfig"
 source "sound/soc/kirkwood/Kconfig"
+source "sound/soc/lapis/Kconfig"
 source "sound/soc/mid-x86/Kconfig"
 source "sound/soc/mxs/Kconfig"
 source "sound/soc/pxa/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 9ea8ac8..9592f41 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_SND_SOC)	+= ep93xx/
 obj-$(CONFIG_SND_SOC)	+= fsl/
 obj-$(CONFIG_SND_SOC)   += imx/
 obj-$(CONFIG_SND_SOC)	+= jz4740/
+obj-$(CONFIG_SND_SOC)	+= lapis/
 obj-$(CONFIG_SND_SOC)	+= mid-x86/
 obj-$(CONFIG_SND_SOC)	+= mxs/
 obj-$(CONFIG_SND_SOC)	+= nuc900/
diff --git a/sound/soc/lapis/Kconfig b/sound/soc/lapis/Kconfig
new file mode 100644
index 0000000..1bd5200
--- /dev/null
+++ b/sound/soc/lapis/Kconfig
@@ -0,0 +1,6 @@
+config SND_SOC_ML7213_MACHINE
+	tristate "ML7213 IOH ASoC machine driver"
+	depends on SND_SOC_TXX9ACLC
+	select SND_SOC_ML7213_PLATFORM
+	help
+	  This is ASoC machine driver for ML7213 IOH
diff --git a/sound/soc/lapis/Makefile b/sound/soc/lapis/Makefile
new file mode 100644
index 0000000..468d922
--- /dev/null
+++ b/sound/soc/lapis/Makefile
@@ -0,0 +1,4 @@
+# Platform
+snd-soc-ml7213-machine-objs := ml7213ioh-machine.o
+
+obj-$(CONFIG_SND_SOC_ML7213_MACHINE) += snd-soc-ml7213-plat.o
diff --git a/sound/soc/lapis/ml7213ioh-machine.c b/sound/soc/lapis/ml7213ioh-machine.c
new file mode 100644
index 0000000..9e4690e
--- /dev/null
+++ b/sound/soc/lapis/ml7213ioh-machine.c
@@ -0,0 +1,197 @@
+/*
+ * ml7213ioh-machine.c -- SoC Audio for LAPIS Semiconductor ML7213 IOH CRB
+ *
+ * Copyright (C) 2011 LAPIS Semiconductor Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
+ */
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/soc.h>
+#include <linux/i2c.h>
+#include "../codecs/ml26124.h"
+
+#define CODEC_DEV_ADDR 0x1A
+static struct i2c_board_info ioh_hwmon_info[] = {
+	{I2C_BOARD_INFO("ioh_i2c-0", CODEC_DEV_ADDR + 1)},
+	{I2C_BOARD_INFO("ioh_i2c-1", CODEC_DEV_ADDR + 2)},
+	{I2C_BOARD_INFO("ioh_i2c-2", CODEC_DEV_ADDR + 3)},
+	{I2C_BOARD_INFO("ioh_i2c-3", CODEC_DEV_ADDR + 4)},
+	{I2C_BOARD_INFO("ioh_i2c-4", CODEC_DEV_ADDR + 5)},
+	{I2C_BOARD_INFO("ioh_i2c-5", CODEC_DEV_ADDR + 0)},
+};
+
+static const struct snd_soc_dapm_widget ml7213_dapm_widgets[] = {
+	SND_SOC_DAPM_SPK("Speaker", NULL),
+	SND_SOC_DAPM_LINE("LINEOUT", NULL),
+	SND_SOC_DAPM_LINE("LINEIN", NULL),
+	SND_SOC_DAPM_MIC("AnalogMIC", NULL),
+	SND_SOC_DAPM_MIC("DigitalMIC", NULL),
+};
+
+static const struct snd_soc_dapm_route ml7213_routes[] = {
+	{"Speaker", NULL, "SPOUT"},
+	{"LINEOUT", NULL, "LOUT"},
+	{"MIN", NULL, "AnalogMIC"},
+	{"MDIN", NULL, "DigitalMIC"},
+	{"MIN", NULL, "LINEIN"},
+};
+
+static int ml7213_ioh_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_codec *codec = rtd->codec;
+	struct snd_soc_dapm_context *dapm = &codec->dapm;
+
+	snd_soc_dapm_new_controls(dapm, ml7213_dapm_widgets,
+				  ARRAY_SIZE(ml7213_dapm_widgets));
+
+	snd_soc_dapm_add_routes(dapm, ml7213_routes, ARRAY_SIZE(ml7213_routes));
+	snd_soc_dapm_sync(dapm);
+
+	return 0;
+}
+
+static int ml7213_startup(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_codec *codec = rtd->codec;
+	struct i2c_client *i2c;
+	struct i2c_adapter *adapter;
+
+	mutex_lock(&codec->mutex);
+
+	adapter = i2c_get_adapter(1);
+	if (!adapter)
+		return -ENODEV;
+
+	i2c = i2c_new_device(adapter, &ioh_hwmon_info[substream->number]);
+	if (!i2c) {
+		mutex_unlock(&codec->mutex);
+		return -1;
+	}
+	i2c_put_adapter(adapter);
+	mutex_unlock(&codec->mutex);
+
+	return 0;
+}
+
+static int ml7213_hw_params(struct snd_pcm_substream *substream,
+			    struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *codec_dai = rtd->codec_dai;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	unsigned int clk = 0;
+	int ret = 0;
+
+	switch (params_rate(params)) {
+	case 16000:
+	case 32000:
+	case 48000:
+		clk = 12288800;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* set codec DAI configuration */
+	ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
+				SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
+	if (ret < 0)
+		return ret;
+
+	/* set cpu DAI configuration */
+	ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_I2S |
+				SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
+	if (ret < 0)
+		return ret;
+
+	/* set the codec system clock for DAC and ADC */
+	ret = snd_soc_dai_set_sysclk(codec_dai, ML26124_USE_PLL, clk,
+				     SND_SOC_CLOCK_IN);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static struct snd_soc_ops ml7213_ops = {
+	.startup = ml7213_startup,
+	.hw_params = ml7213_hw_params,
+};
+
+static struct snd_soc_dai_link ioh_i2s_dai = {
+	.name = "ML7213",
+	.stream_name = "I2S HiFi",
+	.cpu_dai_name = "ml7213-i2s",
+	.codec_dai_name = "ml26124-hifi",
+	.platform_name	= "ml7213-pcm-audio",
+	.codec_name	= "ml26124-00GD",
+	.init = ml7213_ioh_init,
+	.ops = &ml7213_ops,
+};
+
+static struct snd_soc_card ioh_i2s_card = {
+	.name		= "ml7213",
+	.dai_link	= &ioh_i2s_dai,
+	.num_links	= 1,
+	.dapm_widgets	= ml7213_dapm_widgets,
+	.num_dapm_widgets	= ARRAY_SIZE(ml7213_dapm_widgets),
+	.dapm_routes	= ml7213_routes,
+	.num_dapm_routes	= ARRAY_SIZE(ml7213_routes),
+};
+
+static int __init ioh_i2s_probe(struct platform_device *pdev)
+{
+	ioh_i2s_card.dev = &pdev->dev;
+	snd_soc_register_card(&ioh_i2s_card);
+
+	return 0;
+}
+
+static int __exit ioh_i2s_remove(struct platform_device *pdev)
+{
+	snd_soc_unregister_card(&ioh_i2s_card);
+
+	return 0;
+}
+
+static struct platform_driver ioh_i2s_driver = {
+	.remove = ioh_i2s_remove,
+	.driver = {
+		.name = "ml7213ioh-machine",
+		.owner = THIS_MODULE,
+	},
+	.probe	= ioh_i2s_probe,
+	.remove	= __devexit_p(ioh_i2s_remove),
+};
+
+static int __init ioh_i2s_init(void)
+{
+	return platform_driver_register(&ioh_i2s_driver);
+}
+
+static void __exit ioh_i2s_exit(void)
+{
+	return platform_driver_unregister(&ioh_i2s_driver);
+}
+
+module_init(ioh_i2s_init);
+module_exit(ioh_i2s_exit);
+
+MODULE_AUTHOR("Tomoya MORINAGA <tomoya.rohm@gmail.com>");
+MODULE_DESCRIPTION("LAPIS Semiconductor ML7213 IOH ALSA SoC machine driver");
+MODULE_LICENSE("GPL");
-- 
1.7.4.4

Re: [PATCH v2] soc/lapis: add machine driver

[Mark Brown]

On Fri, Dec 02, 2011 at 06:45:15PM +0900, Tomoya MORINAGA wrote:

> +#define CODEC_DEV_ADDR 0x1A
> +static struct i2c_board_info ioh_hwmon_info[] = {
> +	{I2C_BOARD_INFO("ioh_i2c-0", CODEC_DEV_ADDR + 1)},
> +	{I2C_BOARD_INFO("ioh_i2c-1", CODEC_DEV_ADDR + 2)},
> +	{I2C_BOARD_INFO("ioh_i2c-2", CODEC_DEV_ADDR + 3)},
> +	{I2C_BOARD_INFO("ioh_i2c-3", CODEC_DEV_ADDR + 4)},
> +	{I2C_BOARD_INFO("ioh_i2c-4", CODEC_DEV_ADDR + 5)},
> +	{I2C_BOARD_INFO("ioh_i2c-5", CODEC_DEV_ADDR + 0)},

This looks completely wrong.  I'd not expect to see any I2C_BOARD_INFO
usage at all in a machine driver (that should be done by whatever
enumerates the system as a whole) and I'm not clear what the intended
effect of the above is - the names look like the names of I2C
controllers...

> +static int ml7213_ioh_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_soc_codec *codec = rtd->codec;
> +	struct snd_soc_dapm_context *dapm = &codec->dapm;
> +
> +	snd_soc_dapm_new_controls(dapm, ml7213_dapm_widgets,
> +				  ARRAY_SIZE(ml7213_dapm_widgets));
> +
> +	snd_soc_dapm_add_routes(dapm, ml7213_routes, ARRAY_SIZE(ml7213_routes));

Use table based init for these.

> +static int ml7213_startup(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_codec *codec = rtd->codec;
> +	struct i2c_client *i2c;
> +	struct i2c_adapter *adapter;
> +
> +	mutex_lock(&codec->mutex);
> +
> +	adapter = i2c_get_adapter(1);
> +	if (!adapter)
> +		return -ENODEV;
> +
> +	i2c = i2c_new_device(adapter, &ioh_hwmon_info[substream->number]);
> +	if (!i2c) {
> +		mutex_unlock(&codec->mutex);
> +		return -1;
> +	}
> +	i2c_put_adapter(adapter);
> +	mutex_unlock(&codec->mutex);

No, this is definitely wrong - we certainly shouldn't be registering new
devices whenever someone starts an audio stream.  I'm struggling to
understand what the intended effect is though.

> +	switch (params_rate(params)) {
> +	case 16000:
> +	case 32000:
> +	case 48000:
> +		clk = 12288800;
> +		break;

You may as well just set the CODEC clock rate once on init() and let the
CODEC driver worry about what it can do with the clock it's got, if the
values aren't going to change there's no need to set them every time.

> +	ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> +				SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
> +	if (ret < 0)
> +		return ret;

Use the dai_fmt field in the dai_link to set this.

> +MODULE_AUTHOR("Tomoya MORINAGA <tomoya.rohm@gmail.com>");
> +MODULE_DESCRIPTION("LAPIS Semiconductor ML7213 IOH ALSA SoC machine driver");
> +MODULE_LICENSE("GPL");

Should have MODULE_ALIAS too.

Re: [PATCH v2] soc/lapis: add machine driver

[Tomoya MORINAGA]

Hi Mark,

First of all, let me confirm my understanding for machine driver.
According to "machine.txt", I created this machine driver referring
corgi.c/spitz.c.
However, corgi.c/spitz.c. don't look obeying your saying.

e.g.
2011/12/3 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>> +     snd_soc_dapm_add_routes(dapm, ml7213_routes, ARRAY_SIZE(ml7213_routes));
>
> Use table based init for these.
>
I  couldn't see any table based init processing at corgi.c/spitz.c.

>
>> +     switch (params_rate(params)) {
>> +     case 16000:
>> +     case 32000:
>> +     case 48000:
>> +             clk = 12288800;
>> +             break;
>
> You may as well just set the CODEC clock rate once on init() and let the
> CODEC driver worry about what it can do with the clock it's got, if the
> values aren't going to change there's no need to set them every time.
>

corgi.c/spitz.c look coded like above.

So, I'm confused.
Is this procedure true ?


Thanks,

tomoya

Re: [PATCH v2] soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/3 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Fri, Dec 02, 2011 at 06:45:15PM +0900, Tomoya MORINAGA wrote:
>
>> +#define CODEC_DEV_ADDR 0x1A
>> +static struct i2c_board_info ioh_hwmon_info[] = {
>> +     {I2C_BOARD_INFO("ioh_i2c-0", CODEC_DEV_ADDR + 1)},
>> +     {I2C_BOARD_INFO("ioh_i2c-1", CODEC_DEV_ADDR + 2)},
>> +     {I2C_BOARD_INFO("ioh_i2c-2", CODEC_DEV_ADDR + 3)},
>> +     {I2C_BOARD_INFO("ioh_i2c-3", CODEC_DEV_ADDR + 4)},
>> +     {I2C_BOARD_INFO("ioh_i2c-4", CODEC_DEV_ADDR + 5)},
>> +     {I2C_BOARD_INFO("ioh_i2c-5", CODEC_DEV_ADDR + 0)},
>
> This looks completely wrong.  I'd not expect to see any I2C_BOARD_INFO
> usage at all in a machine driver (that should be done by whatever
> enumerates the system as a whole)
Do you mean machine driver must not use I2C_BOARD_INFO ? Is this true ?
Grepping I2C_BOARD_INFO at sound/soc,  the following drivers use
I2C_BOARD_INFO.
pxa/raumfeld.c: I2C_BOARD_INFO("max9485", 0x63),
pxa/magician.c:         I2C_BOARD_INFO("uda1380", 0x18),
s6000/s6105-ipcam.c:    { I2C_BOARD_INFO("tlv320aic33", 0x18), }

> and I'm not clear what the intended
> effect of the above is - the names look like the names of I2C
> controllers...
You are right.
"ioh_i2c-0" must be "ml26124".


>
>> +static int ml7213_startup(struct snd_pcm_substream *substream)
>> +{
>> +     struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +     struct snd_soc_codec *codec = rtd->codec;
>> +     struct i2c_client *i2c;
>> +     struct i2c_adapter *adapter;
>> +
>> +     mutex_lock(&codec->mutex);
>> +
>> +     adapter = i2c_get_adapter(1);
>> +     if (!adapter)
>> +             return -ENODEV;
>> +
>> +     i2c = i2c_new_device(adapter, &ioh_hwmon_info[substream->number]);
>> +     if (!i2c) {
>> +             mutex_unlock(&codec->mutex);
>> +             return -1;
>> +     }
>> +     i2c_put_adapter(adapter);
>> +     mutex_unlock(&codec->mutex);
>
> No, this is definitely wrong - we certainly shouldn't be registering new
> devices whenever someone starts an audio stream.  I'm struggling to
> understand what the intended effect is though.
Do you mean this registering must be at xx_init processing ?
OK, I'll move this processing into init().

>> +     ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
>> +                             SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
>> +     if (ret < 0)
>> +             return ret;
>
> Use the dai_fmt field in the dai_link to set this.
Sorry, I can't understand your saying.
Let me know in more detail.

>
>> +MODULE_AUTHOR("Tomoya MORINAGA <tomoya.rohm@gmail.com>");
>> +MODULE_DESCRIPTION("LAPIS Semiconductor ML7213 IOH ALSA SoC machine driver");
>> +MODULE_LICENSE("GPL");
>
> Should have MODULE_ALIAS too.
Do you mean machine driver should have MODULE_ALIAS ?
Grepping  MODULE_ALIAS at sound/soc, it seems MODULE_ALIAS is used in
platform driver only like below.
pxa/pxa2xx-i2s.c:MODULE_ALIAS("platform:pxa2xx-i2s");

Does machine driver need like "MODULE_ALIAS("machine:ml7213CRB")" ?

Thanks in advance

tomoya

Re: [PATCH v2] soc/lapis: add machine driver

[Mark Brown]

On Mon, Dec 05, 2011 at 02:28:29PM +0900, Tomoya MORINAGA wrote:

> According to "machine.txt", I created this machine driver referring
> corgi.c/spitz.c.
> However, corgi.c/spitz.c. don't look obeying your saying.

You really need to look at current drivers to make sure you're following
current best practice - older drivers, particularly for obsolte hardware
like the Zaurus machines, may well not reflect the current best practices.

Re: [PATCH v2] soc/lapis: add machine driver

[Mark Brown]

On Mon, Dec 05, 2011 at 06:33:03PM +0900, Tomoya MORINAGA wrote:
> 2011/12/3 Mark Brown <broonie@opensource.wolfsonmicro.com>:

> >> +     {I2C_BOARD_INFO("ioh_i2c-5", CODEC_DEV_ADDR + 0)},

> > This looks completely wrong.  I'd not expect to see any I2C_BOARD_INFO
> > usage at all in a machine driver (that should be done by whatever
> > enumerates the system as a whole)

> Do you mean machine driver must not use I2C_BOARD_INFO ? Is this true ?
> Grepping I2C_BOARD_INFO at sound/soc,  the following drivers use
> I2C_BOARD_INFO.
> pxa/raumfeld.c: I2C_BOARD_INFO("max9485", 0x63),
> pxa/magician.c:         I2C_BOARD_INFO("uda1380", 0x18),
> s6000/s6105-ipcam.c:    { I2C_BOARD_INFO("tlv320aic33", 0x18), }

You'll note that this is *really* rare; there are vastly more machine
drivers that don't do this.  If there's some specific reason then
there's nothing technical stopping this happening but it's very unusual.

> >> +     ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> >> +                             SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
> >> +     if (ret < 0)
> >> +             return ret;

> > Use the dai_fmt field in the dai_link to set this.

> Sorry, I can't understand your saying.
> Let me know in more detail.

I'm not sure what more to say...  Have you looked at that field and how
it is both implemented and used in other drivers, or at the commit logs
for relevant changes?  What do you find unclear?

> > Should have MODULE_ALIAS too.

> Do you mean machine driver should have MODULE_ALIAS ?
> Grepping  MODULE_ALIAS at sound/soc, it seems MODULE_ALIAS is used in
> platform driver only like below.
> pxa/pxa2xx-i2s.c:MODULE_ALIAS("platform:pxa2xx-i2s");

I don't think you're looking at a current kernel...

> Does machine driver need like "MODULE_ALIAS("machine:ml7213CRB")" ?

No, it should match the name of the platform driver you created.  The
platform: refers to the bus type.

Re: [PATCH v2] soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/6 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>> According to "machine.txt", I created this machine driver referring
>> corgi.c/spitz.c.
>> However, corgi.c/spitz.c. don't look obeying your saying.
>
> You really need to look at current drivers to make sure you're following
> current best practice - older drivers, particularly for obsolte hardware
> like the Zaurus machines, may well not reflect the current best practices.

I see.
I won't read this docs.

BTW, Let me know the best practice of machine driver?
I can't identify which driver is modern or not.


>> 2011/12/3 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>
>> >> +     {I2C_BOARD_INFO("ioh_i2c-5", CODEC_DEV_ADDR + 0)},
>
>> > This looks completely wrong.  I'd not expect to see any I2C_BOARD_INFO
>> > usage at all in a machine driver (that should be done by whatever
>> > enumerates the system as a whole)
>
>> Do you mean machine driver must not use I2C_BOARD_INFO ? Is this true ?
>> Grepping I2C_BOARD_INFO at sound/soc,  the following drivers use
>> I2C_BOARD_INFO.
>> pxa/raumfeld.c: I2C_BOARD_INFO("max9485", 0x63),
>> pxa/magician.c:         I2C_BOARD_INFO("uda1380", 0x18),
>> s6000/s6105-ipcam.c:    { I2C_BOARD_INFO("tlv320aic33", 0x18), }
>
> You'll note that this is *really* rare; there are vastly more machine
> drivers that don't do this.  If there's some specific reason then
> there's nothing technical stopping this happening but it's very unusual.
I see.
I want to know how to use i2c device.
Let me know the best practice driver uses i2c device.

>
>> >> +     ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
>> >> +                             SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
>> >> +     if (ret < 0)
>> >> +             return ret;
>
>> > Use the dai_fmt field in the dai_link to set this.
>
>> Sorry, I can't understand your saying.
>> Let me know in more detail.
>
> I'm not sure what more to say...  Have you looked at that field and how
> it is both implemented and used in other drivers, or at the commit logs
> for relevant changes?  What do you find unclear?

You said "Use the dai_fmt field in the dai_link to set this."
However, both dai_fmt and dai_link are already implemented like below.
static struct snd_soc_dai_link ioh_i2s_dai = {
...
};

static struct snd_soc_card ioh_i2s_card = {
...
	.dai_link	= &ioh_i2s_dai,
...
};
So, I can't understand your saying.

>
>> > Should have MODULE_ALIAS too.
>
>> Do you mean machine driver should have MODULE_ALIAS ?
>> Grepping  MODULE_ALIAS at sound/soc, it seems MODULE_ALIAS is used in
>> platform driver only like below.
>> pxa/pxa2xx-i2s.c:MODULE_ALIAS("platform:pxa2xx-i2s");
>
> I don't think you're looking at a current kernel...
>
>> Does machine driver need like "MODULE_ALIAS("machine:ml7213CRB")" ?
>
> No, it should match the name of the platform driver you created.  The
> platform: refers to the bus type.

I understood.

thanks,

tomoya

Re: [alsa-devel] [PATCH v2] soc/lapis: add machine driver

[Austin, Brian]

On Dec 5, 2011, at 8:01 PM, "Tomoya MORINAGA" <tomoya.rohm@gmail.com> wrote:

> 2011/12/6 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>>> According to "machine.txt", I created this machine driver referring
>>> corgi.c/spitz.c.
>>> However, corgi.c/spitz.c. don't look obeying your saying.
>> 
>> You really need to look at current drivers to make sure you're following
>> current best practice - older drivers, particularly for obsolte hardware
>> like the Zaurus machines, may well not reflect the current best practices.
> 
> I see.
> I won't read this docs.
> 
> BTW, Let me know the best practice of machine driver?
> I can't identify which driver is modern or not.
> 
> 
Check the OMAP and Samsung directories. Those are pretty current

Re: [alsa-devel] [PATCH v2] soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/6 Austin, Brian <Brian.Austin@cirrus.com>:
>
> On Dec 5, 2011, at 8:01 PM, "Tomoya MORINAGA" <tomoya.rohm@gmail.com> wrote:
>
>> 2011/12/6 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>>>> According to "machine.txt", I created this machine driver referring
>>>> corgi.c/spitz.c.
>>>> However, corgi.c/spitz.c. don't look obeying your saying.
>>>
>>> You really need to look at current drivers to make sure you're following
>>> current best practice - older drivers, particularly for obsolte hardware
>>> like the Zaurus machines, may well not reflect the current best practices.
>>
>> I see.
>> I won't read this docs.
>>
>> BTW, Let me know the best practice of machine driver?
>> I can't identify which driver is modern or not.
>>
>>
> Check the OMAP and Samsung directories. Those are pretty current
>
Thanks.  However, i2c access code is not included in Samsung.
Though OMAP includes i2c access code like below,
  omap/sdp3430.c: twl_i2c_read_u8(TWL4030_MODULE_INTBR, &pin_mux,
  omap/sdp3430.c: twl_i2c_write_u8(TWL4030_MODULE_INTBR, pin_mux,
this code doesn't seem applicable for me.

Could you show best practice for i2c access of machine driver?

thanks,
tomoya

Re: [alsa-devel] [PATCH v2] soc/lapis: add machine driver

[Mark Brown]

On Tue, Dec 06, 2011 at 11:37:52AM +0900, Tomoya MORINAGA wrote:

> Thanks.  However, i2c access code is not included in Samsung.
> Though OMAP includes i2c access code like below,
>   omap/sdp3430.c: twl_i2c_read_u8(TWL4030_MODULE_INTBR, &pin_mux,
>   omap/sdp3430.c: twl_i2c_write_u8(TWL4030_MODULE_INTBR, pin_mux,
> this code doesn't seem applicable for me.

> Could you show best practice for i2c access of machine driver?

I2C access by machine drivers is not good practice.  If you must do it
for some reason use the standard ASoC functions to read and write
registers.

Re: [PATCH v2] soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/6 Tomoya MORINAGA <tomoya.rohm@gmail.com>:
>>> >> +     ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
>>> >> +                             SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
>>> >> +     if (ret < 0)
>>> >> +             return ret;
>>
>>> > Use the dai_fmt field in the dai_link to set this.
>>
>>> Sorry, I can't understand your saying.
>>> Let me know in more detail.
>>
>> I'm not sure what more to say...  Have you looked at that field and how
>> it is both implemented and used in other drivers, or at the commit logs
>> for relevant changes?  What do you find unclear?
>
> You said "Use the dai_fmt field in the dai_link to set this."
> However, both dai_fmt and dai_link are already implemented like below.
> static struct snd_soc_dai_link ioh_i2s_dai = {
> ...
> };
>
> static struct snd_soc_card ioh_i2s_card = {
> ...
>        .dai_link       = &ioh_i2s_dai,
> ...
> };
> So, I can't understand your saying.

Could you give me your answer ?

thanks,

tomoya

Re: [PATCH v2] soc/lapis: add machine driver

[Mark Brown]

On Wed, Dec 07, 2011 at 01:42:45PM +0900, Tomoya MORINAGA wrote:
> 2011/12/6 Tomoya MORINAGA <tomoya.rohm@gmail.com>:

You're complaining that you didn't a response within 24 hours!  This is
really not reasonable for something which is essentially volunteer
driven project.

> > You said "Use the dai_fmt field in the dai_link to set this."
> > However, both dai_fmt and dai_link are already implemented like below.
> > static struct snd_soc_dai_link ioh_i2s_dai = {

> > static struct snd_soc_card ioh_i2s_card = {
> > ...
> >        .dai_link       = &ioh_i2s_dai,
> > ...
> > };

> > So, I can't understand your saying.

> Could you give me your answer ?

Your above code has no references to dai_fmt.  Have you looked at the
dai_fmt field in the dai_link structure or at the existing examples of
its use?

Re: [PATCH v2] soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/7 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> Your above code has no references to dai_fmt.  Have you looked at the
> dai_fmt field in the dai_link structure or at the existing examples of
> its use?

Still I can't understand your saying. Let me clarify.

Firstly, the above "dai_link structure" means below ? If yes, there is
no member "dai_fmt", right ?
697 struct snd_soc_dai_link {
698         /* config - must be set by machine driver */
699         const char *name;                       /* Codec name */
700         const char *stream_name;                /* Stream name */
701         const char *codec_name;         /* for multi-codec */
702         const char *platform_name;      /* for multi-platform */
703         const char *cpu_dai_name;
704         const char *codec_dai_name;
705
706         /* Keep DAI active over suspend */
707         unsigned int ignore_suspend:1;
708
709         /* Symmetry requirements */
710         unsigned int symmetric_rates:1;
711
712         /* codec/machine specific init - e.g. add machine controls */
713         int (*init)(struct snd_soc_pcm_runtime *rtd);
714
715         /* machine stream operations */
716         struct snd_soc_ops *ops;
717

> +     ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> +                             SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
At the above point, do you mean I shouldn't  set these formats(
SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS)
directly ?

Anyway,let me know your best reference driver obeys your saying about it.

thanks,
tomoya

Re: [PATCH v2] soc/lapis: add machine driver

[Mark Brown]

On Fri, Dec 09, 2011 at 05:38:57PM +0900, Tomoya MORINAGA wrote:

> Firstly, the above "dai_link structure" means below ? If yes, there is
> no member "dai_fmt", right ?
> 697 struct snd_soc_dai_link {

As previously advised you should work against current development
kernels.  dai_fmt is present in current development kernels (and Linus'
tree for that matter).

> > +     ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> > +                             SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);

> At the above point, do you mean I shouldn't  set these formats(
> SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS)
> directly ?

Yes.

Re: [PATCH v2] soc/lapis: add machine driver

[Tomoya MORINAGA]

2011/12/9 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Fri, Dec 09, 2011 at 05:38:57PM +0900, Tomoya MORINAGA wrote:
>
>> Firstly, the above "dai_link structure" means below ? If yes, there is
>> no member "dai_fmt", right ?
>> 697 struct snd_soc_dai_link {
>
> As previously advised you should work against current development
> kernels.  dai_fmt is present in current development kernels (and Linus'
> tree for that matter).
>
>> > +     ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
>> > +                             SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
>
>> At the above point, do you mean I shouldn't  set these formats(
>> SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS)
>> directly ?
>
> Yes.

Seeing linux-next tree, I could understand your saying.
I saw linux tree(3.2-rc4).

thanks,
tomoya