Re: RFC: [PATCH Upated]: snd-pmac-gpio interfaces for snd-powermac

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]

Hi,

I have no idea if this is the latest version of this patch, but I
couldn't find any more recent one...

A few of comments inlined below.

> +static struct pmf_function *get_audio_pfunc(const char *name, const char *altname)
> +{
> +	struct device_node *np;
> +	struct pmf_function *pfunc = NULL;
> +
> +	if (! (np = find_devices("i2s-a")))
> +		return NULL;

Can we have this function take an explicit device parameter instead? As
it is now, it can only handle the i2s-a soundcard, while the latest
powermacs have two sound cards (codecs?): i2s-a and i2s-c.

> +int snd_pmac_get_gpio(const char *name, const char *altname,
> +                            snd_pmac_gpio_t *gp)
> +{
> +        memset(gp, 0, sizeof(*gp));
> +
> +	gp->name = name;
> +	gp->altname = altname;
> +
> +        /* Platform functions are prefered */
> +        if ((gp->pfunc = get_audio_pfunc(name, altname)))
> +                return 0;
> +
> +	/* Else, fallback to direct gpio */
> +	return get_audio_gpio(name, altname, gp);

Maybe there ought to be a way to disable the fallback when we're on
newer chips? I don't grok the sound architecture well enough yet to
tell.

> +			/* XXX: pmf_unregister_irq_client doesn't use its
> +			 * first two arguments. We only need to send it
> +			 * the irq_client. WATCH FOR THIS CHANGING!
> +			 */
> +			pmf_unregister_irq_client(NULL, NULL, &gp->irq_client);

Heh, so I'm looking at an old version of this patch. The current
pmf_unregister_irq_client makes this explicit and only takes one
parameter :)

> +int snd_pmac_request_irq(snd_pmac_gpio_t *gp, void (*handler)(void *),
> +			 void *data)
> +{
> +	int ret = -ENODEV;
> +	struct device_node *np;
> +
> +	gp->irq_client.handler = handler;
> +	gp->irq_client.data = data;
> +	gp->irq_client.owner = NULL;
> +
> +	if (gp->pfunc) {
> +		gp->irq_client.owner = THIS_MODULE;
> +
> +		if ((np = find_devices("i2s-a"))) {

same comment here as the first one -- the powermac needs to be able to
access i2s-c through this too, I think.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]