Re: [RFC 2/8] snd-aoa: add aoa core

[Takashi Iwai <tiwai@suse.de>]

At Thu, 01 Jun 2006 13:58:46 +0200,
Johannes Berg wrote:
> 
> --- /dev/null
> +++ b/sound/aoa/core/snd-aoa-core.c
> @@ -0,0 +1,153 @@
> +/*
> + * Apple Onboard Audio driver core
> + *
> + * Copyright 2006 Johannes Berg <johannes@sipsolutions.net>
> + *
> + * GPL v2, can be found in COPYING.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/list.h>
> +#include "../aoa.h"
> +#include "snd-aoa-alsa.h"
> +
> +MODULE_DESCRIPTION("Apple Onboard Audio Sound Driver");
> +MODULE_AUTHOR("Johannes Berg <johannes@sipsolutions.net>");
> +MODULE_LICENSE("GPL");
> +
> +/* We allow only one fabric. This simplifies things,
> + * and more don't really make that much sense */
> +static struct aoa_fabric *fabric;
> +static LIST_HEAD(codec_list);
> +
> +static void attach_codec_to_fabric(struct aoa_codec *c)

Doesn't this need to return an error?
I'm afraid that module count is unblanced in the error path of
aoa_corec_[un]register().

> +{
> +	int err;
> +
> +	if (!try_module_get(c->owner))
> +		return;
> +	/* found_codec has to be assigned */
> +	err = -ENOENT;
> +	if (fabric->found_codec)
> +		err = fabric->found_codec(c);
> +	if (err) {
> +		module_put(c->owner);
> +		printk("snd-aoa: fabric didn't like codec %s\n", c->name);

Add KERN_* prefix.

> +		return;
> +	}
> +	c->fabric = fabric;
> +
> +	err = 0;
> +	if (c->init)
> +		err = c->init(c);
> +	if (err) {
> +		printk("snd-aoa: codec %s didn't init\n", c->name);

Ditto.

> --- /dev/null
> +++ b/sound/aoa/core/snd-aoa-gpio-pmf.c
(snip)
> +static void pmf_gpio_init(struct gpio_runtime *rt)
> +{
> +	pmf_gpio_all_amps_off(rt);
> +	rt->implementation_private = 0;
> +	INIT_WORK(&rt->headphone_notify.work, pmf_handle_notify, &rt->headphone_notify);
> +	INIT_WORK(&rt->line_in_notify.work, pmf_handle_notify, &rt->line_in_notify);
> +	INIT_WORK(&rt->line_out_notify.work, pmf_handle_notify, &rt->line_out_notify);

Too long lines.

> +	mutex_init(&rt->headphone_notify.mutex);
> +	mutex_init(&rt->line_in_notify.mutex);
> +	mutex_init(&rt->line_out_notify.mutex);
> +}
> +
> +static void pmf_gpio_exit(struct gpio_runtime *rt)
> +{
> +	pmf_gpio_all_amps_off(rt);
> +	rt->implementation_private = 0;
> +	if (rt->headphone_notify.gpio_private)
> +		pmf_unregister_irq_client(rt->headphone_notify.gpio_private);
> +	if (rt->line_in_notify.gpio_private)
> +		pmf_unregister_irq_client(rt->line_in_notify.gpio_private);
> +	if (rt->line_out_notify.gpio_private)
> +		pmf_unregister_irq_client(rt->line_out_notify.gpio_private);

Don't need kfree(gpio_private)?

> --- /dev/null
> +++ b/sound/aoa/core/snd-aoa-alsa.c
(snip)
> +int aoa_alsa_init(char *name, struct module *mod)
> +{
> +	struct snd_card *alsa_card;
> +	int err;
> +
> +	if (aoa_card)
> +		/* cannot be EEXIST due to usage in aoa_fabric_register */
> +		return -EBUSY;
> +
> +	alsa_card = snd_card_new(index, name, mod, sizeof(struct aoa_card));
> +	if (!alsa_card)
> +		return -ENOMEM;
> +	aoa_card = alsa_card->private_data;
> +	aoa_card->alsa_card = alsa_card;
> +	strlcpy(alsa_card->driver, "AppleOnbdAudio", sizeof(alsa_card->driver)-1);
> +	strlcpy(alsa_card->shortname, name, sizeof(alsa_card->shortname)-1);
> +	strlcpy(alsa_card->longname, name, sizeof(alsa_card->longname)-1);
> +	strlcpy(alsa_card->mixername, name, sizeof(alsa_card->mixername)-1);

Pass sizeof() without -1.  strlcpy() takes the size of the buffer
including nul-terminator.


Takashi