Re: [PATCH 2/4] ASoC sst v2: Add mid platform driver

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

On Tue, Jan 04, 2011 at 08:16:32PM +0530, Koul, Vinod wrote:

> This patch adds the platform driver for mid asoc drivers
> This platfrom driver sends commands to sst dsp engine driver for the dai operations.
> For this purpose it depends on intel_sst driver which is currently in staging tree

Wrap lines within changelogs to something less than 80 columns.

> +
> +	spin_lock(&stream->status_lock);
> +	if (stream->stream_status != SST_PLATFORM_RUNNING) {
> +		spin_unlock(&stream->status_lock);
> +		return;
> +	}
> +	spin_unlock(&stream->status_lock);
> +	snd_pcm_period_elapsed(substream);
> +	return;
> +}

This return statement is pointless.  The above code would be much
clearer if you just said something like:

	spin_lock()
	set flag if running
	spin_unlock()

	period_elapsed()

> +	ret_val = sst_platform_init_stream(substream);
> +	if (ret_val)
> +		return ret_val;
> +	substream->runtime->hw.info = SNDRV_PCM_INFO_BLOCK_TRANSFER;
> +	return ret_val;

As with much of the driver the use of blank lines between blocks seems
intermittent.

> +	spin_lock(&stream->status_lock);
> +	if (stream->stream_status == SST_PLATFORM_INIT) {
> +		spin_unlock(&stream->status_lock);
> +		return 0;
> +	}
> +	spin_unlock(&stream->status_lock);

A helper function to do the lock/check would probably make life a lot
easier if you're going to do this a lot so callers could do:

	if (sst_stream_state(stream, SST_PLATFORM_INIT)
		return 0;

or whatever.

> +	return stream->stream_info.buffer_ptr;

I thought you were going to clarify that this isn't exactly a pointer?

> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platfrom: sst-platform");

Typos here - extra space and platform is spelt wrongly.