Re: [PATCH] asoc: bfin: add i2s driver for blackfin bf60x processor

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2300 bytes --]

On Mon, Jun 04, 2012 at 04:03:19PM -0400, Scott Jiang wrote:
> This driver includes i2s dai, dma and sport driver. It enables i2s
> mode support on blackfin bf60x platform.

This should be a patch series, for example the DAI, DMA and SPORT
drivers would normally all be split out.

Overall this is mostly good, the main thing that's jumping out at me is
that the SPORT API looks like it doesn't really have too many device
specifics in it so it's a bit surprising that we need completely
separate I2S and DMA drivers for the two chip generations.

> +config SND_BF6XX_I2S
> +	tristate "SoC I2S Audio for the ADI BF6xx chip"
> +	depends on BLACKFIN && BF60x

Shouldn't BF60x already depend on BLACKFIN?

>  config SND_BF5XX_SOC_SSM2602
>  	tristate "SoC SSM2602 Audio support for BF52x ezkit"

This bit also needs updating...

>  config SND_BF5XX_SPORT_NUM
>  	int "Set a SPORT for Sound chip"
> -	depends on (SND_BF5XX_I2S || SND_BF5XX_AC97 || SND_BF5XX_TDM)
> +	depends on (BLACKFIN && SND_SOC)

Shouldn't have a dependency on SND_SOC - this menu can't be entered
without ASoC being enabled.

> +static int __init snd_bfin_i2s_pcm_init(void)
> +{
> +	return platform_driver_register(&bfin_i2s_pcm_driver);
> +}
> +module_init(snd_bfin_i2s_pcm_init);

module_platform_driver().

> +#ifdef CONFIG_PM
> +static int bfin_i2s_suspend(struct snd_soc_dai *dai)
> +{
> +	struct sport_device *sport = snd_soc_dai_get_drvdata(dai);
> +
> +	if (dai->capture_active)
> +		sport_rx_stop(sport);
> +	if (dai->playback_active)
> +		sport_tx_stop(sport);
> +	return 0;
> +}

The framework should do this for you, by the time we get to suspending
there shouldn't be any active streams that aren't supposed to stay
active over the time the device is suspended.

> +static int __init bfin_i2s_init(void)
> +{
> +	return platform_driver_register(&bfin_i2s_driver);
> +}
> +
> +static void __exit bfin_i2s_exit(void)
> +{
> +	platform_driver_unregister(&bfin_i2s_driver);
> +}

module_platform_driver().

> +void sport_tx_start(struct sport_device *sport)
> +{

Is it not possible to make the SPORT API the same between the 5xx and
the 6xx such that we can share all the drivers between the two chip
generations with just the SPORT code varying?

> +	dev_info(dev, "SPORT create success\n");

Make this dev_dbg at most.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]