[alsa-devel] [PATCH] ASoC: Add TXx9 AC link controller driver

[Mark Brown]

On Thu, May 14, 2009 at 11:50:04PM +0900, Atsushi Nemoto wrote:

This all looks basically fine - just a few comments below, the main one
being the way you're registering things.

> +#ifdef CONFIG_PM
> +static int txx9aclc_ac97_suspend(struct snd_soc_dai *dai)
> +{
> +	return 0;
> +}
> +
> +static int txx9aclc_ac97_resume(struct snd_soc_dai *dai)
> +{
> +	return 0;
> +}
> +#else
> +#define txx9aclc_ac97_suspend	NULL
> +#define txx9aclc_ac97_resume	NULL
> +#endif

Just remove all this if there's no implementation.

> +static int __init txx9aclc_ac97_init(void)
> +{
> +	return snd_soc_register_dai(&txx9aclc_ac97_dai);
> +}
> +
> +static void __exit txx9aclc_ac97_exit(void)
> +{
> +	snd_soc_unregister_dai(&txx9aclc_ac97_dai);
> +}

Ideally you'd be registering a platform device in your arch code and
then the DAI would only be registered when the device is probed.  This
(and similar stuff for the DMA) would mean that...

> +static int __init txx9aclc_generic_probe(struct platform_device *pdev)
> +{
> +	struct txx9aclc_soc_device *dev = &txx9aclc_generic_soc_device;
> +	struct platform_device *soc_pdev;
> +	struct resource *r;
> +	int ret;
> +	int i;
> +	int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +	dev->irq = irq;

...all this resource stuff wouldn't need to be done by the machine
driver, it'd be done by your DAI and DMA drivers.  That means less
duplication of code for multiple machines both in the machine driver and
in registering the resources along with the platform device.

> +static int __init txx9aclc_soc_platform_init(void)
> +{
> +	return snd_soc_register_platform(&txx9aclc_soc_platform);
> +}
> +
> +static void __exit txx9aclc_soc_platform_exit(void)
> +{
> +	snd_soc_unregister_platform(&txx9aclc_soc_platform);
> +}

Same comment applies here.