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

[Vinod Koul <vinod.koul@intel.com>]

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...

> >+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 :)

-- 
~Vinod