Re: [PATCH 1/2] ASoC: sirf: Add sirf audio hub driver for sharing same register address space

[Mark Brown <broonie@kernel.org>]

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

On Thu, Mar 13, 2014 at 06:38:30PM +0800, RongJun Ying wrote:
> @@ -283,9 +jc83,10 @@ static int sirf_audio_codec_trigger(struct snd_pcm_substream *substream,
>  		int cmd,
>  		struct snd_soc_dai *dai)
>  {
> -	int playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
>  	struct snd_soc_codec *codec = dai->codec;
>  	u32 val = 0;
> +	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
> +		return 0;
>  
>  	/*
>  	 * This is a workaround, When stop playback,
> @@ -299,15 +300,13 @@ static int sirf_audio_codec_trigger(struct snd_pcm_substream *substream,
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> -		if (playback)
> -			val = IC_HSLEN | IC_HSREN;
> +		val = IC_HSLEN | IC_HSREN;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	if (playback)
> -		snd_soc_update_bits(codec, AUDIO_IC_CODEC_CTRL0,
> +	snd_soc_update_bits(codec, AUDIO_IC_CODEC_CTRL0,
>  			IC_HSLEN | IC_HSREN, val);
>  	return 0;
>  }

All the changes in this file appear to be unrelated stylistic changes.
They're fine but you should do such things as separate commits, this
makes things harder to review.

> +/* This regmap is shared all child audio drivers */
> +struct regmap *regmap;

Why is this a global variable?  I'd expect it to be in the driver data
for the device.  This is also a global symbol so would at the very least
would need better namespacing.

> +
> +static const struct regmap_config sirf_audio_regmap_config = 
> +	.reg_bits =
> +	.val_bits = 32,
> +	.cache_type = REGCACHE_NONE,

None is the default.

Looking at this code I'm not clear how the function devices get
instantiated?

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

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