At Sat, 23 Aug 2008 00:38:41 +1000, Ben Stanley wrote: > > Dear list, > > This patch provides the ability to play back digital audio via SPDIF at > a sampling rate of 44100Hz. It also implements rate constraints to > ensure that the hardware restrictions of the card are not violated. Note > that if 44100Hz is used, then all used channels must use that frequency. > For example, opening hw:0,0 at 48000 and then opening hw:0,1 at 44100 > will fail due to constraints. However, you may successfully open both > hw:0,0 and hw:0,1 at 44100 (presume no other channels open). Further > details of the restriction may be found in the thread > http://thread.gmane.org/gmane.linux.alsa.devel/52149/focus=52345 > > This patch also implements playback format restrictions, such that only > one playback format may be in use at any time. > > I tested driver playback using the attached script > (Test_ca0106_driver.sh). I get good audio output for all combinations of > sampling rates and formats except for 192kHz S16 (as noted at the end of > the script). Sometimes it works and sometimes it doesn't. The reason for > the failure is not apparent at this time. It could have something to do > with my receiver. I don't know. > > The attached script also checks and validates that the rate and format > constraints operate as required. > > I periodically have trouble with this driver not producing sound output. > This has been investigated and the cause is explained here > http://thread.gmane.org/gmane.linux.alsa.devel/55384/focus=55410 > I intend to submit a new patch to address this in the future. > > This patch has been in use for three months on my MythTV system (Ubuntu > 8.04). I find it works reliably for me. I also use it with xine, aplay, > mplayer, mame, and childsplay. > > I have checked with checkpatch.pl . Patch is against v1.0.18rc1. > > Signed-off-by: Ben Stanley Ben.Stanley@xxxxxxxxxxxxxx Thanks for the patch! Through a quick review, I find it almost fine, but slight concerns below. > +static int hw_rule_playback_rate( > + struct snd_pcm_hw_params *params, > + struct snd_pcm_hw_rule *rule) Put arguments right after the open parenthesis. It's more common style. > +{ > + struct snd_ca0106 *chip = rule->private; > + int chi, any_44100 = 0, any_non_44100 = 0, mask = 0; No need to initialize here (at least for mask). > + struct snd_ca0106_channel *chp = 0; Ditto. > + struct snd_pcm_runtime *runtime; > + if (snd_BUG_ON(!chip)) > + return -EINVAL; > + > + if (chip->spdif_enable) { > + for (chi = 0; chi < 4; ++chi) { > + chp = &(chip->playback_channels[chi]); I'm afraid it's racy. The chip->playback_channels[] can be changed at any time. Try to put a mutex to protect this check. > + if (!chp->use) > + continue; > + if (snd_BUG_ON(!chp->epcm)) > + return -EINVAL; > + if (!chp->epcm->hw_reserved) > + continue; > + if (snd_BUG_ON(!chp->epcm->substream)) > + return -EINVAL; > + if (snd_BUG_ON(!chp->epcm->substream->runtime)) > + return -EINVAL; > + runtime = chp->epcm->substream->runtime; > + snd_printd("snd_hw_rule_playback_rate: ch=%d, " > + "rate=%d.\n", chi, runtime->rate); Use snd_printdd() instead. CONFIG_SND_DEBUG=y on most cases, and this would be too annoying. > + any_44100 += runtime->rate == 44100; > + any_non_44100 += runtime->rate != 44100; > + } > + if (snd_BUG_ON(any_44100 && any_non_44100)) > + return -EINVAL; > + if (any_44100) > + mask = 0x1; > + else if (any_non_44100) > + mask = 0xE; > + else > + mask = 0xF; This mask value is a bit cryptic, but OK... > + } else { > + /* 44100Hz is not supported for DAC (FIXME Why?) */ > + mask = 0xE; > + } > + snd_printd("snd_hw_rule_playback_rate: any_44100=%d, " > + "any_non_44100=%d, mask=0x%X, spdif=%d\n", > + any_44100, any_non_44100, mask, chip->spdif_enable); Use snd_printdd(). > + return snd_interval_list( > + hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE), > + ARRAY_SIZE(all_spdif_playback_rates), > + all_spdif_playback_rates, mask); Put arguments after the open parenthesis. > +} > + > +static int hw_rule_playback_format( > + struct snd_pcm_hw_params *params, > + struct snd_pcm_hw_rule *rule) Ditto. > +{ > + struct snd_ca0106 *chip = rule->private; > + int chi, any_S16 = 0, any_S32 = 0; > + struct snd_ca0106_channel *chp = 0; No need to initialize. > + struct snd_pcm_runtime *runtime; > + struct snd_mask fmt, *f = hw_param_mask( > + params, SNDRV_PCM_HW_PARAM_FORMAT); > + int result; > + if (snd_BUG_ON(!chip)) > + return -EINVAL; > + snd_mask_none(&fmt); > + > + for (chi = 0; chi < 4; ++chi) { > + chp = &(chip->playback_channels[chi]); This also needs a mutex as well. > + if (!chp->use) > + continue; > + if (snd_BUG_ON(!chp->epcm)) > + return -EINVAL; > + if (!chp->epcm->hw_reserved) > + continue; > + if (snd_BUG_ON(!chp->epcm->substream)) > + return -EINVAL; > + if (snd_BUG_ON(!chp->epcm->substream->runtime)) > + return -EINVAL; > + runtime = chp->epcm->substream->runtime; > + snd_printd("snd_hw_rule_playback_format: ch=%d, format=%d.\n", > + chi, runtime->format); Use snd_printdd(). > + any_S16 += runtime->format == SNDRV_PCM_FORMAT_S16_LE; > + any_S32 += runtime->format == SNDRV_PCM_FORMAT_S32_LE; > + } > + if (snd_BUG_ON(any_S16 && any_S32)) > + return -EINVAL; > + if (any_S16) > + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE); > + else if (any_S32) > + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE); > + else { > + /* No format yet chosen, so both formats are available. */ > + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S16_LE); > + snd_mask_set(&fmt, SNDRV_PCM_FORMAT_S32_LE); > + } > + result = snd_mask_refine(f, &fmt); > + snd_printd("snd_hw_rule_playback_format: any_S16=%d, any_S32=%d, " > + "refined_fmt=0x%X, avail_fmt=0x%X, changed=%d\n", > + any_S16, any_S32, f->bits[0], fmt.bits[0], result); Use snd_printdd(). > + return result; > +} > + > unsigned int snd_ca0106_ptr_read(struct snd_ca0106 * emu, > unsigned int reg, > unsigned int chn) > @@ -508,10 +619,24 @@ static int snd_ca0106_pcm_open_playback_channel(struct snd_pcm_substream *substr > //printk("open:channel_id=%d, chip=%p, channel=%p\n",channel_id, chip, channel); > //channel->interrupt = snd_ca0106_pcm_channel_interrupt; > channel->epcm = epcm; > - if ((err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)) < 0) > + err = snd_pcm_hw_constraint_integer( > + runtime, SNDRV_PCM_HW_PARAM_PERIODS); > + if (err < 0) > return err; > - if ((err = snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64)) < 0) > + err = snd_pcm_hw_constraint_step( > + runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 64); > + if (err < 0) > return err; > + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, > + hw_rule_playback_rate, (void *)chip, > + SNDRV_PCM_HW_PARAM_RATE, -1); > + if (err < 0) > + return err; > + err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_FORMAT, > + hw_rule_playback_format, (void *)chip, > + SNDRV_PCM_HW_PARAM_FORMAT, -1); > + if (err < 0) > + return err; > snd_pcm_set_sync(substream); > > if (chip->details->spi_dac && channel_id != PCM_FRONT_CHANNEL) { > @@ -646,6 +771,9 @@ static int snd_ca0106_pcm_hw_params_playback(struct snd_pcm_substream *substream > /* hw_free callback */ > static int snd_ca0106_pcm_hw_free_playback(struct snd_pcm_substream *substream) > { > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_ca0106_pcm *epcm = runtime->private_data; > + epcm->hw_reserved = 0; > return snd_pcm_lib_free_pages(substream); > } > > @@ -667,53 +795,103 @@ static int snd_ca0106_pcm_hw_free_capture(struct snd_pcm_substream *substream) > static int snd_ca0106_pcm_prepare_playback(struct snd_pcm_substream *substream) > { > struct snd_ca0106 *emu = snd_pcm_substream_chip(substream); > - struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_pcm_runtime *runtime = substream->runtime, *runtimei = 0; No need to initialize. > struct snd_ca0106_pcm *epcm = runtime->private_data; > - int channel = epcm->channel_id; > + struct snd_ca0106_channel *chp = 0; Ditto. > + int channel = epcm->channel_id, chi, any_44100 = 0, any_non_44100 = 0; > u32 *table_base = (u32 *)(emu->buffer.area+(8*16*channel)); > u32 period_size_bytes = frames_to_bytes(runtime, runtime->period_size); > u32 hcfg_mask = HCFG_PLAYBACK_S32_LE; > u32 hcfg_set = 0x00000000; > u32 hcfg; > - u32 reg40_mask = 0x30000 << (channel<<1); > + u32 reg40_mask = 0xFF0000; > u32 reg40_set = 0; > u32 reg40; > - /* FIXME: Depending on mixer selection of SPDIF out or not, select the spdif rate or the DAC rate. */ > - u32 reg71_mask = 0x03030000 ; /* Global. Set SPDIF rate. We only support 44100 to spdif, not to DAC. */ > + u32 reg71_mask; > + u32 reg71_shift; > u32 reg71_set = 0; > u32 reg71; > int i; > > - //snd_printk("prepare:channel_number=%d, rate=%d, format=0x%x, channels=%d, buffer_size=%ld, period_size=%ld, periods=%u, frames_to_bytes=%d\n",channel, runtime->rate, runtime->format, runtime->channels, runtime->buffer_size, runtime->period_size, runtime->periods, frames_to_bytes(runtime, 1)); > - //snd_printk("dma_addr=%x, dma_area=%p, table_base=%p\n",runtime->dma_addr, runtime->dma_area, table_base); > - //snd_printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n",emu->buffer.addr, emu->buffer.area, emu->buffer.bytes); > - /* Rate can be set per channel. */ > - /* reg40 control host to fifo */ > - /* reg71 controls DAC rate. */ > - switch (runtime->rate) { > - case 44100: > - reg40_set = 0x10000 << (channel<<1); > - reg71_set = 0x01010000; > - break; > - case 48000: > - reg40_set = 0; > - reg71_set = 0; > - break; > - case 96000: > - reg40_set = 0x20000 << (channel<<1); > - reg71_set = 0x02020000; > - break; > - case 192000: > - reg40_set = 0x30000 << (channel<<1); > - reg71_set = 0x03030000; > - break; > - default: > - reg40_set = 0; > - reg71_set = 0; > - break; > + epcm->hw_reserved = 1; > + /* FIXME CLEAN UP IF spdif_enable IS CHANGED WHILE CHANNELS ARE OPENED > + * OR PREVENT THIS FROM HAPPENING. */ > + if (emu->spdif_enable) > + reg71_shift = 24; /* SPDIF Output Rate */ > + else > + reg71_shift = 16; /* I2S Output Rate */ > + reg71_mask = 0x3 << reg71_shift; > + > + printk(KERN_DEBUG DRVNAME ": prepare_playback: " > + "channel_number=%d, rate=%d, format=0x%x, channels=%d, " > + "buffer_size=%ld,period_size=%ld, periods=%u, " > + "frames_to_bytes=%d\n", > + channel, runtime->rate, runtime->format, runtime->channels, > + runtime->buffer_size, runtime->period_size, runtime->periods, > + frames_to_bytes(runtime, 1)); Use snd_printdd() (if any). > + /*printk("dma_addr=%x, dma_area=%p, table_base=%p\n", > + runtime->dma_addr,runtime->dma_area, table_base);*/ > + /*printk("dma_addr=%x, dma_area=%p, dma_bytes(size)=%x\n", > + emu->buffer.addr,emu->buffer.area, emu->buffer.bytes);*/ > + /* We are forced to build the settings for all the channels. */ > + for (chi = 0; chi < 4; ++chi) { > + chp = &(emu->playback_channels[chi]); Use a mutex in this function, too. > + if (!chp->use) > + continue; > + if (snd_BUG_ON(!chp->epcm)) > + return -EINVAL; > + if (chi != channel && !chp->epcm->hw_reserved) > + continue; > + if (snd_BUG_ON(!chp->epcm->substream)) > + return -EINVAL; > + if (snd_BUG_ON(!chp->epcm->substream->runtime)) > + return -EINVAL; > + runtimei = chp->epcm->substream->runtime; > + any_44100 += runtimei->rate == 44100; > + any_non_44100 += runtimei->rate != 44100; > + /* Rate can be set per channel. */ > + /* reg40 control host to fifo */ > + /* reg71 controls DAC rate. */ > + switch (runtimei->rate) { > + case 44100: > + /* We only support 44100 to spdif, not to DAC. > + (FIXME WHY?)*/ > + if (emu->spdif_enable) { > + /* When using 44100, *all* channels > + must be set to that rate. */ > + reg40_set |= 0x550000; > + reg71_set |= 0x1 << reg71_shift; > + break; > + } else { > + printk(KERN_ERR DRVNAME > + "prepare_playback: " > + "44100Hz is invalid for DAC.\n"); > + } > + case 48000: > + /* reg40_set &= !(0x1 << (chi<<1)); */ > + /* reg71_set &= !(0x1 << reg71_shift); */ > + break; > + case 96000: > + reg40_set |= 0x20000 << (chi<<1); > + reg71_set |= 0x2 << reg71_shift; > + break; > + case 192000: > + reg40_set |= 0x30000 << (chi<<1); > + reg71_set |= 0x3 << reg71_shift; > + break; > + default: > + printk(KERN_ERR DRVNAME > + ": prepare_playback: " > + "Bad sampling frequency %d.\n", > + runtimei->rate); > + } > } > - /* Format is a global setting */ > - /* FIXME: Only let the first channel accessed set this. */ > + printk(KERN_DEBUG DRVNAME ": prepare_playback: any_44100=%d, " > + "any_non_44100=%d, spdif=%d.\n", > + any_44100, any_non_44100, emu->spdif_enable); Use snd_printdd(). > + /* Format is a global setting. */ > + /* Only the first channel accessed can set this > + (enforced by constraints). */ > switch (runtime->format) { > case SNDRV_PCM_FORMAT_S16_LE: > hcfg_set = 0; > @@ -1363,8 +1541,8 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card, > pci_read_config_dword(pci, PCI_SUBSYSTEM_VENDOR_ID, &chip->serial); > pci_read_config_word(pci, PCI_SUBSYSTEM_ID, &chip->model); > #if 1 > - printk(KERN_INFO "snd-ca0106: Model %04x Rev %08x Serial %08x\n", chip->model, > - pci->revision, chip->serial); > + printk(KERN_INFO DRVNAME ": Model %04x Rev %08x Serial %08x\n", > + chip->model, pci->revision, chip->serial); > #endif > strcpy(card->driver, "CA0106"); > strcpy(card->shortname, "CA0106"); > @@ -1378,7 +1556,9 @@ static int __devinit snd_ca0106_create(int dev, struct snd_card *card, > } > chip->details = c; > if (subsystem[dev]) { > - printk(KERN_INFO "snd-ca0106: Sound card name=%s, subsystem=0x%x. Forced to subsystem=0x%x\n", > + printk(KERN_INFO DRVNAME > + ": Sound card name=%s, subsystem=0x%x. " > + "Forced to subsystem=0x%x\n", > c->name, chip->serial, subsystem[dev]); > } Could you fix these and repost? Thanks! Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
