[alsa-devel] RFC: ASoC driver for S3C24XX Simtec boards

[Ben Dooks]

On Thu, Jun 25, 2009 at 12:26:59PM +0100, Mark Brown wrote:
> On Wed, Jun 24, 2009 at 12:11:55PM +0100, Ben Dooks wrote:
> > I'd like some comments on the following driver which
> > is currently being developed to get ASoC working on
> > the TLV320AIC23 based Simtec boards.
> 
> Please do remember to CC maintainers on things.
> 
> > 1) I'm not seeing a control to enable/disable the
> >    'Speaker Amp', do I need to change the controls
> >    or how this is exported?
> 
> If you need to give manual control of this to user space use a
> DAPM_SOC_PIN_SWITCH widget.  Normally there's no need to do this since
> it falls out of the internal routing within the CODEC and having an
> explicit control for the output itself just adds an extra knob for
> userspace to twiddle.  Depending on the technology it may not even be
> desirable since having inputs connected with the amplifiers powered off
> can leave audible leakage paths.

We've not seen any problems with out boards, then the amps are coupled
with an R/C network so leakage from HP->AMP should be negligible.

Our old ALSA drivers always gave the user the choice to have the
speaker powered up or not.
 
> > static const struct snd_soc_dapm_widget tlv320aic23_dapm_widgets[] = {
> > 	SND_SOC_DAPM_HP("Headphone Jack", NULL),
> > 	SND_SOC_DAPM_LINE("Line In", NULL),
> > 	SND_SOC_DAPM_LINE("Line Out", NULL),
> > 	SND_SOC_DAPM_MIC("Mic Jack", NULL),
> 
> > 	SND_SOC_DAPM_PGA_E("Speaker Amp", SND_SOC_NOPM,
> > 			   0, 0, NULL, 0, simtec_spk_event,
> > 			   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
> 
> This looks like a SND_SOC_DAPM_SPK(); using that will help with
> sequencing.

thanks, changed. still no output in the alsactl control dump though.
 
> > static const struct snd_soc_dapm_route audio_map[] = {
> 
> This wants a better name - base_map or something?

thanks, changed.
 
> > 	if (pdata->amp_gpio > 0) {
> > 		printk(KERN_DEBUG "%s: adding amp routes\n", __func__);
> 
> pr_dbg() or dev_dbg()

changed to pr_debug, we're not keeping the platform device around
after the probe.

> > static int simtec_hw_startup(struct snd_pcm_substream *substream)
> > {
> > 	/* call any board supplied startup code, this currently only
> > 	 * covers the bast/vr1000 which have a CPLD in the way of the
> > 	 * LRCLK */
> > 	if (pdata->startup)
> > 		pdata->startup();
> > 
> > 	return 0;
> > }
> 
> This looks odd.  Does this really need to be done each time the audio
> device can be opened or should it be done once when the driver is being
> set up?  There doesn't appear to be any similar code to reverse the
> startup() on teardown so I suspect that just doing it when the driver
> starts should be fine.

I'll have to add it in the resume path as well.

> > 	/* attach gpio amp gain (if any) */
> > 	if (pdata->amp_gain[0] > 0) {
> > 		ret = gpio_request(pd->amp_gain[0], "gpio-amp-gain");
> > 		if (ret) {
> > 			dev_err(dev, "cannot get amp gpio gain0\n");
> > 			return ret;
> > 		}
> > 
> > 		ret = gpio_request(pd->amp_gain[1], "gpio-amp-gain");
> > 		if (ret) {
> > 			dev_err(dev, "cannot get amp gpio gain0\n");
> 
> gain1.

thanks, changed.

> > static int __devinit simtec_audio_probe(struct platform_device *pdev)
> > {
> > 	struct platform_device *snd_dev;
> > 	int ret;
> > 
> > 	dev_info(&pdev->dev, "probe called\n");
> 
> dev_dbg() at most.

> > 	pdata = pdev->dev.platform_data;
> > 	if (!pdata) {
> > 		dev_err(&pdev->dev, "no platform data supplied\n");
> > 		return -EINVAL;
> > 	}
> 
> Every individual bit of platform data looks optional; this suggests that
> either additional validation is desirable here or that platform data
> itself should be optional.

As such, pretty much all the systems we've got pass platform data for
some from of information, so I decided that a lack of it was a good
enough indication that there was something wrong.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.