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

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

On 4/6/18 3:48 AM, Vinod Koul wrote:
> On Thu, Apr 05, 2018 at 06:40:20PM -0500, Pierre-Louis Bossart wrote:
> 
>>> +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?
> 
> Looking it again, the state transition is from CONFIGURED, so the first
> check is not required and will be removed.
> 
>>> +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?
> 
> So first, we think stream should not be enabled if it is already enabled.
> The code right now doesnt treat it as error, but we should so will fix it
> up...
> 

ok

>>> +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.
> 
> yup, here too :)
>