Re: [alsa-devel] [PATCH v2] soc/lapis: add machine driver
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.
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
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.
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
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.
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
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
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
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.
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
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?
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
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.
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
participants (3)
-
Austin, Brian -
Mark Brown -
Tomoya MORINAGA