Re: [alsa-devel] [PATCH 3/3] sound/soc/lapis: add platform driver
On Mon, Nov 21, 2011 at 01:08:52PM +0900, Tomoya MORINAGA wrote:
+/* =================== I2S CH0 config =================== */ +#define I2S_CH0_MCLK (12288000) /* Master Clock Frequency[Hz] */
This looks like it should be board specific?
- #if IOH_I2S_USE_PARAM
- #define I2S_CH0_FS_16000 16000
- #define I2S_CH0_FS_32000 32000
- #define I2S_CH0_FS_48000 48000
- #else
- #define I2S_CH0_FS_8000 8000
- #define I2S_CH0_FS_11025 11025
- #define I2S_CH0_FS_22050 22050
- #define I2S_CH0_FS_44100 44100
- #endif
What are these ifdefs for? This should be configured at runtime.
+/* select master or slave. The value is ioh_mssel_t */ +#define I2S_CH0_MSSEL (ioh_mssel_master) +/* select MCLK or MLBCLK into Master Clock. The value is enum ioh_masterclk_t */ +#define I2S_CH0_MASTERCLKSEL (ioh_masterclksel_mclk)
This and most of the following defines also look like compile time configuration for things that should be runtime configured. As with the previous patches in the series the big picture issue here is that you need to update your code to reflect the practices of modern mainline Linux code - here the major issue is that you appear to be doing all your configuration with #defines rather than by using runtime configuration from the machine driver as is normal for ASoC code.
I got through a bunch more defines but stopped reviewing at some point as there was so much of this issue.
Searching git log, tegra_i2s.c seems modern. I have a question.
According to platfom.txt, describes like below. struct snd_soc_ops { ... };
On the other hand, tegra_i2s.c describes like below.
static const struct snd_soc_dai_ops tegra_i2s_dai_ops = { ... };
Which is true as modern driver ? or case by case ?
+#define I2S_CH0_MCLK (12288000) /* Master Clock Frequency[Hz] */
This looks like it should be board specific?
Should our platform driver use "clk_get()" ? If no, how should our driver get the value ?
thanks in advance.
tomoya
On Mon, Dec 12, 2011 at 05:28:13PM +0900, Tomoya MORINAGA wrote:
struct snd_soc_ops { ... };
On the other hand, tegra_i2s.c describes like below.
static const struct snd_soc_dai_ops tegra_i2s_dai_ops = { ... };
Which is true as modern driver ? or case by case ?
The latter. With things like this it would be *really* helpful if you could take a step back and think about what the differences mean and why they are different.
+#define I2S_CH0_MCLK (12288000) /* Master Clock Frequency[Hz] */
This looks like it should be board specific?
Should our platform driver use "clk_get()" ? If no, how should our driver get the value ?
Again, with things like this it would be really helpful if you could attempt to answer questions for yourself. Have you looked at how other platforms configure clock rates from machine drivers?
2011/12/12 Mark Brown broonie@opensource.wolfsonmicro.com:
On Mon, Dec 12, 2011 at 05:28:13PM +0900, Tomoya MORINAGA wrote:
struct snd_soc_ops { ... };
On the other hand, tegra_i2s.c describes like below.
static const struct snd_soc_dai_ops tegra_i2s_dai_ops = { ... };
Which is true as modern driver ? or case by case ?
The latter. With things like this it would be *really* helpful if you could take a step back and think about what the differences mean and why they are different.
Our driver needs ".pointer" method. However, "struct snd_soc_dai_ops" doesn't have the method. So, I think "struct snd_soc_dai_ops" cannot be applied to our driver. Searching other drivers, "blackfin/bf5xx-i2s-pcm.c" uses "struct snd_soc_ops" not "struct snd_soc_dai_ops". Let me know your opinion.
Thanks,
tomoya
On Tue, Dec 13, 2011 at 01:38:25PM +0900, Tomoya MORINAGA wrote:
2011/12/12 Mark Brown broonie@opensource.wolfsonmicro.com:
The latter. With things like this it would be *really* helpful if you could take a step back and think about what the differences mean and why they are different.
Our driver needs ".pointer" method. However, "struct snd_soc_dai_ops" doesn't have the method. So, I think "struct snd_soc_dai_ops" cannot be applied to our driver.
What makes you claim this - *why* does your DAI driver need a pointer method?
Searching other drivers, "blackfin/bf5xx-i2s-pcm.c" uses "struct snd_soc_ops" not "struct snd_soc_dai_ops". Let me know your opinion.
Of course the DMA driver uses the ops for DMA drivers! Alternatively if your question above is about your DMA driver then why is your DMA driver using DAI ops?
To repeat what I said above in a different way *please* put more effort into understanding things and trying to solve problems for yourself, or improving the way you ask questions.
participants (2)
-
Mark Brown -
Tomoya MORINAGA