Re: [PATCH v2 07/13] soundwire: Add stream configuration APIs

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> +/**
> + * sdw_prepare_stream: Prepare SoundWire stream
> + *
> + * @stream: Soundwire stream
> + *
> + * Documentation/soundwire/stream.txt explains this API in detail
> + */
> +int sdw_prepare_stream(struct sdw_stream_runtime *stream)
> +{
> +	int ret = 0;
> +
> +	if (!stream) {
> +		pr_err("SoundWire: Handle not found for stream");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&stream->m_rt->bus->bus_lock);
> +
> +	if (stream->state == SDW_STREAM_DISABLED)
> +		goto error;
> +
> +	if (stream->state != SDW_STREAM_CONFIGURED) {
> +		ret = -EINVAL;
> +		goto error;
> +	}

this seems to be a new pattern in this file.
Why is the first test even needed?

> +
> +	ret = _sdw_prepare_stream(stream);
> +	if (ret < 0) {
> +		pr_err("Prepare for stream:%s failed: %d", stream->name, ret);
> +		goto error;
> +	}
> +
> +error:
> +	mutex_unlock(&stream->m_rt->bus->bus_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(sdw_prepare_stream);
> +
> +static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
> +{
> +	struct sdw_master_runtime *m_rt = stream->m_rt;
> +	struct sdw_bus *bus = m_rt->bus;
> +	int ret;
> +
> +	/* Program params */
> +	ret = sdw_program_params(bus);
> +	if (ret < 0) {
> +		dev_err(bus->dev, "Program params failed: %d", ret);
> +		return ret;
> +	}
> +
> +	/* Enable port(s) */
> +	ret = sdw_enable_disable_ports(m_rt, true);
> +	if (ret < 0) {
> +		dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = do_bank_switch(stream);
> +	if (ret < 0) {
> +		dev_err(bus->dev, "Bank switch failed: %d", ret);
> +		return ret;
> +	}
> +
> +	stream->state = SDW_STREAM_ENABLED;
> +	return 0;
> +}
> +
> +/**
> + * sdw_enable_stream: Enable SoundWire stream
> + *
> + * @stream: Soundwire stream
> + *
> + * Documentation/soundwire/stream.txt explains this API in detail
> + */
> +int sdw_enable_stream(struct sdw_stream_runtime *stream)
> +{
> +	int ret = 0;
> +
> +	if (!stream) {
> +		pr_err("SoundWire: Handle not found for stream");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&stream->m_rt->bus->bus_lock);
> +
> +	if (stream->state == SDW_STREAM_ENABLED)
> +		goto error;
> +
> +	if ((stream->state != SDW_STREAM_PREPARED) &&
> +		(stream->state != SDW_STREAM_DISABLED)) {
> +		ret = -EINVAL;
> +		goto error;
> +	}

same here, why would you enable a stream that's already enabled? Why is 
this an error that returns 0?

> +
> +	ret = _sdw_enable_stream(stream);
> +	if (ret < 0) {
> +		pr_err("Enable for stream:%s failed: %d", stream->name, ret);
> +		goto error;
> +	}
> +
> +error:
> +	mutex_unlock(&stream->m_rt->bus->bus_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(sdw_enable_stream);
> +
> +static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
> +{
> +	struct sdw_master_runtime *m_rt = stream->m_rt;
> +	struct sdw_bus *bus = m_rt->bus;
> +	int ret;
> +
> +	/* Disable port(s) */
> +	ret = sdw_enable_disable_ports(m_rt, false);
> +	if (ret < 0) {
> +		dev_err(bus->dev, "Disable port(s) failed: %d", ret);
> +		return ret;
> +	}
> +
> +	stream->state = SDW_STREAM_DISABLED;
> +
> +	/* Program params */
> +	ret = sdw_program_params(bus);
> +	if (ret < 0) {
> +		dev_err(bus->dev, "Program params failed: %d", ret);
> +		return ret;
> +	}
> +
> +	return do_bank_switch(stream);
> +}
> +
> +/**
> + * sdw_disable_stream: Disable SoundWire stream
> + *
> + * @stream: Soundwire stream
> + *
> + * Documentation/soundwire/stream.txt explains this API in detail
> + */
> +int sdw_disable_stream(struct sdw_stream_runtime *stream)
> +{
> +	int ret = 0;
> +
> +	if (!stream) {
> +		pr_err("SoundWire: Handle not found for stream");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&stream->m_rt->bus->bus_lock);
> +
> +	if (stream->state == SDW_STREAM_DISABLED)
> +		goto error;
> +
> +	if (stream->state != SDW_STREAM_ENABLED) {
> +		ret = -EINVAL;
> +		goto error;
> +	}

and here to.

> +
> +	ret = _sdw_disable_stream(stream);
> +	if (ret < 0) {
> +		pr_err("Disable for stream:%s failed: %d", stream->name, ret);
> +		goto error;
> +	}
> +
> +error:
> +	mutex_unlock(&stream->m_rt->bus->bus_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(sdw_disable_stream);
> +
> +static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)
> +{
> +	struct sdw_master_runtime *m_rt = stream->m_rt;
> +	struct sdw_bus *bus = m_rt->bus;
> +	int ret = 0;
> +
> +	/* De-prepare port(s) */
> +	ret = sdw_prep_deprep_ports(m_rt, false);
> +	if (ret < 0) {
> +		dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
> +		return ret;
> +	}
> +
> +	bus->params.bandwidth -= m_rt->stream->params.rate *
> +		m_rt->ch_count * m_rt->stream->params.bps;
> +
> +	if (!bus->params.bandwidth) {
> +		bus->params.row = 0;
> +		bus->params.col = 0;
> +		goto exit;
> +
> +	}
> +
> +	/* Program params */
> +	ret = sdw_program_params(bus);
> +	if (ret < 0) {
> +		dev_err(bus->dev, "Program params failed: %d", ret);
> +		return ret;
> +	}
> +
> +	return do_bank_switch(stream);
> +
> +exit:
> +	stream->state = SDW_STREAM_DEPREPARED;
> +
> +	return ret;
> +}
> +
> +/**
> + * sdw_deprepare_stream: Deprepare SoundWire stream
> + *
> + * @stream: Soundwire stream
> + *
> + * Documentation/soundwire/stream.txt explains this API in detail
> + */
> +int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
> +{
> +	int ret = 0;
> +
> +	if (!stream) {
> +		pr_err("SoundWire: Handle not found for stream");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&stream->m_rt->bus->bus_lock);
> +
> +	if (stream->state != SDW_STREAM_DISABLED) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
> +	ret = _sdw_deprepare_stream(stream);
> +	if (ret < 0) {
> +		pr_err("De-prepare for stream:%d failed: %d", ret, ret);
> +		goto error;
> +	}
> +
> +error:
> +	mutex_unlock(&stream->m_rt->bus->bus_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(sdw_deprepare_stream);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 4120a220ae42..4d9c3f86d0f0 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -791,6 +791,10 @@ int sdw_stream_remove_master(struct sdw_bus *bus,
>   		struct sdw_stream_runtime *stream);
>   int sdw_stream_remove_slave(struct sdw_slave *slave,
>   		struct sdw_stream_runtime *stream);
> +int sdw_prepare_stream(struct sdw_stream_runtime *stream);
> +int sdw_enable_stream(struct sdw_stream_runtime *stream);
> +int sdw_disable_stream(struct sdw_stream_runtime *stream);
> +int sdw_deprepare_stream(struct sdw_stream_runtime *stream);
>   
>   /* messaging and data APIs */
>   
>