Re: [ALSA] AdLib FM card driver

[Takashi Iwai <tiwai@suse.de>]

At Mon, 27 Mar 2006 00:07:29 +0200,
Rene Herman wrote:
> 
> Hi Takashi
> 
> Adrian: this was one of the cards you listed as having an OSS but not an 
> ALSA driver a while ago.
> 
> Attached you'll find an ALSA driver for AdLib FM cards. An AdLib card is 
> just an OPL2, which was already supported by sound/drivers/opl3, so only 
> very minimal bus-glue is needed. The patch applies cleanly to both 
> 2.6.16 and 2.6.16-mm1.
> 
> The driver has been tested with an actual ancient 8-bit ISA AdLib card 
> and works fine. It also works fine for an OPL3 {,emulation} as still 
> found on many ISA soundcards but given that AdLib cards don't have their 
> own mixer, upping the volume from 0 might be a problem without the card 
> driver already loaded and driving the OPL3.
> 
> As far as I am concerned, this does not need additional testing and can 
> go to alsa-kernel after a review. It's very unlikely that anyone still 
> has this ancient card installed anyway. The one I have here was lent to 
> me and will be returned shortly, but given the minimal nature of this 
> driver, maintenance should not be a problem.
> 
> I also stuck a very tiny HOWTO in ALSA-Configuration.txt, assuming quite 
> a few people would have no idea how to operate the thing, even if they 
> do happen across a card. Is it okay there?

This amount of description should be OK.

> Takashi: As the card->shortname, I use "AdLib FM", which includes a 
> space. sound/core/init.c:choose_default_id() goes to lengths to not 
> allow this (it turns it into "FM") but when I manually give:
> 
>    modprobe snd-adlib id="AdLib FM" port=0x388
> 
> then the ID is used as is and everything seems fine. I assume that 
> either the space check could go or that a passed in ID should be 
> subjected to it as well? (that function also deletes all !isalnum's from 
> the shortname such as <underscore> which, again, is fine when passed in 
> manually).

The space check is there to retrieve the second word from the
shortname, because the length of id string is much shorter.  In most
cases, you likely have names like "Vendor DeviceXXXX".  Then only
DeviceXXX is extracted.  If it matters, pass a string without space in
the shortname field.

> Comments appreciated. I'll go listen to this AdLib render "Master of 
> Puppets" again now. Heavens, what fun...

> +static int __devinit snd_adlib_probe(struct platform_device *device)
(snip)
> +	card->private_data = request_region(port[i], 4, CRD_NAME);
> +	if (!card->private_data) {
> +		snd_printk(KERN_ERR DRV_NAME ": could not grab ports\n");
> +		snd_card_free(card);
> +		return -EINVAL;
> +	}

-EBUSY would be more suitable (although it's ignored later).

> +	card->private_free = snd_adlib_free;
> +
> +	if (snd_opl3_create(card, port[i], port[i] + 2, OPL3_HW_AUTO, 1, &opl3) < 0) {
> +		snd_printk(KERN_ERR DRV_NAME ": could not create OPL\n");
> +		snd_card_free(card);
> +		return -EINVAL;
> +	}

Better to keep the original error value?

> +
> +	if (snd_opl3_hwdep_new(opl3, 0, 0, NULL) < 0) {
> +		snd_printk(KERN_ERR DRV_NAME ": could not create FM\n");
> +		snd_card_free(card);
> +		return -EINVAL;
> +	}

Ditto.

> +
> +	strcpy(card->driver, DRV_NAME);
> +	strcpy(card->shortname, CRD_NAME);
> +	sprintf(card->longname, CRD_NAME " at %#lx", port[i]);
> +
> +	snd_card_set_dev(card, &device->dev);
> +
> +	if (snd_card_register(card) < 0) {
> +		snd_printk(KERN_ERR DRV_NAME ": could not register card\n");
> +		snd_card_free(card);
> +		return -EINVAL;
> +	}

Ditto.  Maybe better to have a single error exit with snd_card_free()?


The other parts look good to me.


Takashi