[alsa-devel] Using simple-card to replace kirkwood-t5325.c
Hi Folks
I'm an ALSA newbie, so if i say anything stupid, please let me know.
I'm trying to replace sound/soc/kirkwood/kirkwood-t5325.c with DT, using simple-card. I can get near, but i'm missing two things:
There does not appear to be a way to represent this in DT:
static int t5325_dai_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_enable_pin(dapm, "Mic Jack"); snd_soc_dapm_enable_pin(dapm, "Headphone Jack"); snd_soc_dapm_enable_pin(dapm, "Speaker");
return 0; }
It appears that quite a few drivers need to enable pins. It also seems common to disable pins and to set them to NC. Is this a generic enough feature it could be added to the DT binding?
The second thing i need is:
static int t5325_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; unsigned int freq;
freq = params_rate(params) * 256;
return snd_soc_dai_set_sysclk(codec_dai, 0, freq, SND_SOC_CLOCK_IN);
}
static struct snd_soc_ops t5325_ops = { .hw_params = t5325_hw_params, };
This seems a lot less common requirements. All the Marvell SoCs need it, but not many others. So i don't think it makes sense to add it directly to simple-card, otherwise simple-card quickly becomes complex-card as everybody else wants there quirks adding.
Has there been any thoughts about turning simple-card.c into a library, or adding hooks so that a driver can modify the created dai_link structures to add in ops like this?
Thanks Andrew
On 04/15/2014 06:13 PM, Andrew Lunn wrote:
Hi Folks
Adding a few people to Cc.
I'm an ALSA newbie, so if i say anything stupid, please let me know.
I'm trying to replace sound/soc/kirkwood/kirkwood-t5325.c with DT, using simple-card. I can get near, but i'm missing two things:
There does not appear to be a way to represent this in DT:
static int t5325_dai_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_enable_pin(dapm, "Mic Jack"); snd_soc_dapm_enable_pin(dapm, "Headphone Jack"); snd_soc_dapm_enable_pin(dapm, "Speaker");
return 0; }
It appears that quite a few drivers need to enable pins.
I'm not sure where this got started, but this mostly seems to be cargo-culting. External pins are enabled by default, there is no need to call snd_soc_dapm_enable_pin() unless snd_soc_dapm_disable_pin() has been called before for the same pin.
It also seems common to disable pins and to set them to NC. Is this a generic enough feature it could be added to the DT binding?
We should probably require that the DAPM routes and widgets are always completely specified. In this case the fully_routed flag can be set and unconnected pins will automatically be marked as not connected. Although it might be too late now to require this since there are already users of the simple card out in the wild which may have incomplete constraints.
The second thing i need is:
static int t5325_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; unsigned int freq;
freq = params_rate(params) * 256;
return snd_soc_dai_set_sysclk(codec_dai, 0, freq, SND_SOC_CLOCK_IN);
}
static struct snd_soc_ops t5325_ops = { .hw_params = t5325_hw_params, };
This seems a lot less common requirements. All the Marvell SoCs need it, but not many others. So i don't think it makes sense to add it directly to simple-card, otherwise simple-card quickly becomes complex-card as everybody else wants there quirks adding.
Maybe the drivers can be reworked to not require this anymore. The CODEC driver may be able to figure this out on its own.
Has there been any thoughts about turning simple-card.c into a library, or adding hooks so that a driver can modify the created dai_link structures to add in ops like this?
A few of the function that got added for the simple card are already available as library functions in soc-core.c. More can probably made available to other drivers if useful.
- Lars
On Tue, Apr 15, 2014 at 08:40:49PM +0200, Lars-Peter Clausen wrote:
On 04/15/2014 06:13 PM, Andrew Lunn wrote:
snd_soc_dapm_enable_pin(dapm, "Mic Jack"); snd_soc_dapm_enable_pin(dapm, "Headphone Jack"); snd_soc_dapm_enable_pin(dapm, "Speaker");
I'm not sure where this got started, but this mostly seems to be cargo-culting. External pins are enabled by default, there is no need to call snd_soc_dapm_enable_pin() unless snd_soc_dapm_disable_pin() has been called before for the same pin.
It started before I ever started working on ASoC. Sometimes it's done for documentation.
It also seems common to disable pins and to set them to NC. Is this a generic enough feature it could be added to the DT binding?
We should probably require that the DAPM routes and widgets are always completely specified. In this case the fully_routed flag can be set and unconnected pins will automatically be marked as not connected. Although it might be too late now to require this since there are already users of the simple card out in the wild which may have incomplete constraints.
I think we can do something like say that anything that specifies off chip widgets needs to fully specify.
static int t5325_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) {
This seems a lot less common requirements. All the Marvell SoCs need it, but not many others. So i don't think it makes sense to add it directly to simple-card, otherwise simple-card quickly becomes complex-card as everybody else wants there quirks adding.
Maybe the drivers can be reworked to not require this anymore. The CODEC driver may be able to figure this out on its own.
I don't think it can, that looks like the CODEC MCLK being supplied by the SoC (it's nothing to do with a requirement from the SoC really). Ideally this would be handled through the clock API but that's a bit fail at the minute for architecture neutral code. It's a bit of a hack but specifying the ratio in the DT (which I thought we supported in simple-card already but don't seem to) would sidestep the issue.
Has there been any thoughts about turning simple-card.c into a library, or adding hooks so that a driver can modify the created dai_link structures to add in ops like this?
A few of the function that got added for the simple card are already available as library functions in soc-core.c. More can probably made available to other drivers if useful.
Yes, that's the preferred approach - where it's useful.
static int t5325_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) {
This seems a lot less common requirements. All the Marvell SoCs need it, but not many others. So i don't think it makes sense to add it directly to simple-card, otherwise simple-card quickly becomes complex-card as everybody else wants there quirks adding.
Maybe the drivers can be reworked to not require this anymore. The CODEC driver may be able to figure this out on its own.
I don't think it can, that looks like the CODEC MCLK being supplied by the SoC (it's nothing to do with a requirement from the SoC really). Ideally this would be handled through the clock API but that's a bit fail at the minute for architecture neutral code. It's a bit of a hack but specifying the ratio in the DT (which I thought we supported in simple-card already but don't seem to) would sidestep the issue.
I can go the hack route and add the ratio as a DT property. I just thought i would ask now, rather than get a NACK later when i submit the patch.
Thanks Andrew
On Tue, Apr 15, 2014 at 08:40:49PM +0200, Lars-Peter Clausen wrote:
On 04/15/2014 06:13 PM, Andrew Lunn wrote:
Hi Folks
Adding a few people to Cc.
I'm an ALSA newbie, so if i say anything stupid, please let me know.
I'm trying to replace sound/soc/kirkwood/kirkwood-t5325.c with DT, using simple-card. I can get near, but i'm missing two things:
There does not appear to be a way to represent this in DT:
static int t5325_dai_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_enable_pin(dapm, "Mic Jack"); snd_soc_dapm_enable_pin(dapm, "Headphone Jack"); snd_soc_dapm_enable_pin(dapm, "Speaker");
return 0; }
It appears that quite a few drivers need to enable pins.
I'm not sure where this got started, but this mostly seems to be cargo-culting. External pins are enabled by default, there is no need to call snd_soc_dapm_enable_pin() unless snd_soc_dapm_disable_pin() has been called before for the same pin.
Ah, thanks for saying this. I just tested without, and all is good.
Thanks Andrew
participants (3)
-
Andrew Lunn -
Lars-Peter Clausen -
Mark Brown