At Mon, 12 Mar 2007 00:24:36 +0100,
Rask Ingemann Lambertsen wrote:
>
> Below is a patch against linux-2.6.20 to add support for Jazz16 sound
> cards. It consists of changes to ALSA's SoundBlaster support and a new PnP
> protocol for detecting the card, setting resources and such. Before
> submitting a patch for inclusion into Linux, I would like to have a few
> comments and perhaps a test report from someone else.
At a quick review, it's mostly OK. There are small coding-style
issues (see below), but the code itself looks fine.
And congrats, the jazz16 pnp driver seems to be the first one that
uses struct pnp_protocol except for pnp-core ;) I'll wait for Adam's
review wrt this part.
> The Jazz16 is an SB Pro clone with higher stereo sample rates of upto
> 45454 Hz and 16-bit sound. As such, I have modified the SoundBlaster code to
> make use of the features of the Jazz16.
>
> In stereo mode, the denominator used for setting the sample rate must be
> even. Some silly(?) apps
Or a silly hardware? ;)
> like timidity++ set the playback rate before
> seeting the number of channels, which causes needless rounding of the mono
> playback rate. E.g. "timidity -s 40000 -osM" results in a playback rate of
> 38461 Hz (a denominator of 26) even though 40000 Hz (a denominator of 25) is
> fine in mono mode. This is not a big problem, but is there an easy way of
> fixing it?
Very unlikely, so far. We wanted to have a "one-short" configuration
for such cases, but it's not implemented yet.
> I tried to include u-law support, but it doesn't sound right and playback
> speed is about one third of what it should be. I could really use some
> documentation here. I'll remove it before submitting a final patch if I
> don't get it to work.
OK.
> Setting the irq of the MPU-401 port requires the SB part to be active and
> even worse, DSP commands need to be sent. Would it be better to just use the
> MPU-401 port without an irq and avoid the complexity?
Isn't SB part already active? It's fine with non-irq version (which
uses timer) if the code gets too complicated, of course.
Some review comments below:
> +static irqreturn_t jazz16_interrupt(int irq, void *dev_id)
> +{
> + struct snd_sb *chip = (struct snd_sb *) dev_id;
> + return (snd_sb8dsp_interrupt(chip));
> +}
You don't need parentheses for return here.
> +static struct pnp_device_id snd_jazz16_pnpids[] = {
> + { .id = "PNPb00f" },
> + { .id = "" }
> +};
> +MODULE_DEVICE_TABLE(pnp, snd_jazz16_pnpids);
> +
> +static int __devinit snd_jazz16_pnp_probe(struct pnp_dev *pnp_dev,
> + const struct pnp_device_id *pnp_id)
> +{ struct snd_sb *chip;
> + struct snd_card *card;
> + struct snd_opl3 *opl3;
> + int err;
> + uint sbport, sbirq, sbdma8, sbdma16;
> + static uint dev_num = 0;
> +
> + if (enable[dev_num])
> + card = snd_card_new(index[dev_num], id[dev_num], THIS_MODULE, 0);
> + else
> + card = NULL;
> + dev_num ++;
> + if (card == NULL)
> + return -ENOMEM;
> + pnp_set_drvdata (pnp_dev, card);
> + /* FIXME use pnp_port_valid(), pnp_port_flags(), pnp_port_length()... */
> + sbport = pnp_port_start (pnp_dev, 0);
> + sbirq = pnp_irq (pnp_dev, 0);
> + sbdma8 = pnp_dma (pnp_dev, 0);
> + sbdma16 = pnp_dma (pnp_dev, 1);
> + if ((err = snd_sbdsp_create (card, sbport, sbirq, jazz16_interrupt,
> + sbdma8, sbdma16, SB_HW_AUTO, &chip)) < 0) {
Try to avoid if ((err = foo()) < 0) style. Rather split to two lines,
err = foo();
if (err < 0)
...
> +static void __devexit snd_jazz16_pnp_remove(struct pnp_dev *dev)
> +{
> + struct snd_card *card = (struct snd_card *) pnp_get_drvdata(dev);
No need for cast, but...
> +
> + snd_card_disconnect(card);
> + snd_card_free_when_closed(card);
> +}
This should be simply:
snd_card_free(pnp_get_drvdata(dev));
pnp_set_drvdata(dev, NULL);
> @@ -134,8 +191,26 @@ static int snd_sb8_playback_prepare(stru
> }
> size = chip->p_dma_size = snd_pcm_lib_buffer_bytes(substream);
> count = chip->p_period_size = snd_pcm_lib_period_bytes(substream);
> + switch (runtime->format)
> + {
> + case SNDRV_PCM_FORMAT_U8:
> + case SNDRV_PCM_FORMAT_MU_LAW:
"case" should be at the same indent level as '{'.
> @@ -278,14 +377,14 @@ static int snd_sb8_capture_prepare(struc
> } else {
> snd_sbdsp_command(chip, 256 - runtime->rate_den);
> }
> - if (chip->capture_format != SB_DSP_OUTPUT) {
> + if (chip->capture_format != SB_DSP_INPUT) {
Is this a bug in the current code?
Thanks,
Takashi
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
