On 6/2/23 05:11, Charles Keepax wrote:

Version of the code prior to commit d014688eb373 ("soundwire: stream: remove bus->dev from logs on multiple buses"), used bus->dev in the error message after do_bank_switch, this necessitated some checking to ensure the bus pointer was valid. As the code no longer uses bus->dev said checking is now redundant, so remove it.

Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com

Good cleanup, thanks

Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

---

Changes since v1:

• Left the duplicate error prints in.

Thanks, Charles

drivers/soundwire/stream.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 379228f221869..6595f47b403b5 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1338,7 +1338,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream, bool update_params) { struct sdw_master_runtime *m_rt;

• struct sdw_bus *bus = NULL;

• struct sdw_bus *bus; struct sdw_master_prop *prop; struct sdw_bus_params params; int ret;

@@ -1382,11 +1382,6 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream, } }

• if (!bus) {

• pr_err("Configuration error in %s\n", __func__);
  

• return -EINVAL;
  

• }
• ret = do_bank_switch(stream); if (ret < 0) { pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);

@@ -1467,7 +1462,7 @@ EXPORT_SYMBOL(sdw_prepare_stream); static int _sdw_enable_stream(struct sdw_stream_runtime *stream) { struct sdw_master_runtime *m_rt;

• struct sdw_bus *bus = NULL;

• struct sdw_bus *bus; int ret;

  /* Enable Master(s) and Slave(s) port(s) associated with stream */

@@ -1490,11 +1485,6 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream) } }

• if (!bus) {

• pr_err("Configuration error in %s\n", __func__);
  

• return -EINVAL;
  

• }
• ret = do_bank_switch(stream); if (ret < 0) { pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);

On 6/2/23 05:11, Charles Keepax wrote:

sdw_stream_add_slave/master have flags to indicate if the master or slave runtime where allocated in that call to the function. Currently these flags are cleared on all the paths where the runtime is not allocated, it is more logic and simpler to set the flag on the one path where the runtime is allocated.

Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com

Much easier to review indeed, thanks!

Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

---

Changes since v1:

• Split out of the goto patch to ease review

Also worth noting I guess this patch could be squashed with patch 1 in the series really, but I opted to leave them separate as patch 1 is a much simpler fix to be cherry-picked back to older kernels if someone needs the fixup, rather than mixing the fixup and tidy up.

Thanks, Charles

drivers/soundwire/stream.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 6595f47b403b5..df5600a80c174 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1854,7 +1854,7 @@ int sdw_stream_add_master(struct sdw_bus *bus, struct sdw_stream_runtime *stream) { struct sdw_master_runtime *m_rt;

• bool alloc_master_rt = true;

• bool alloc_master_rt = false; int ret;

  mutex_lock(&bus->bus_lock);

@@ -1876,10 +1876,8 @@ int sdw_stream_add_master(struct sdw_bus *bus, * it first), if so skip allocation and go to configuration */ m_rt = sdw_master_rt_find(bus, stream);

• if (m_rt) {

• alloc_master_rt = false;
  

• if (m_rt) goto skip_alloc_master_rt;

• }

  m_rt = sdw_master_rt_alloc(bus, stream); if (!m_rt) {

@@ -1888,6 +1886,8 @@ int sdw_stream_add_master(struct sdw_bus *bus, ret = -ENOMEM; goto unlock; }

• alloc_master_rt = true;

skip_alloc_master_rt:

if (sdw_master_port_allocated(m_rt)) @@ -1980,8 +1980,8 @@ int sdw_stream_add_slave(struct sdw_slave *slave, { struct sdw_slave_runtime *s_rt; struct sdw_master_runtime *m_rt;

• bool alloc_master_rt = true;
• bool alloc_slave_rt = true;

• bool alloc_master_rt = false;

• bool alloc_slave_rt = false;

  int ret;

@@ -1992,10 +1992,8 @@ int sdw_stream_add_slave(struct sdw_slave *slave, * and go to configuration */ m_rt = sdw_master_rt_find(slave->bus, stream);

• if (m_rt) {

• alloc_master_rt = false;
  

• if (m_rt) goto skip_alloc_master_rt;

• }

  /*

  • If this API is invoked by Slave first then m_rt is not valid.

@@ -2009,21 +2007,22 @@ int sdw_stream_add_slave(struct sdw_slave *slave, goto unlock; }

• alloc_master_rt = true;

skip_alloc_master_rt: s_rt = sdw_slave_rt_find(slave, stream);

• if (s_rt) {

• alloc_slave_rt = false;
  

• if (s_rt) goto skip_alloc_slave_rt;

• }

  s_rt = sdw_slave_rt_alloc(slave, m_rt); if (!s_rt) { dev_err(&slave->dev, "Slave runtime alloc failed for stream:%s\n", stream->name);

• alloc_slave_rt = false;
  

  ret = -ENOMEM; goto alloc_error; }

• alloc_slave_rt = true;

skip_alloc_slave_rt: if (sdw_slave_port_allocated(s_rt)) goto skip_port_alloc;

On 6/2/23 05:11, Charles Keepax wrote:

There is a lot of code using gotos to skip small sections of code, this is a fairly dubious use of a goto, especially when the level of intentation is really low. Most of this code doesn't even breach 80 characters when naively shifted over.

Simplify the code a bit, by replacing these unnecessary gotos with simple ifs.

Signed-off-by: Charles Keepax ckeepax@opensource.cirrus.com

Changes since v1:

• Split out the inversion of the alloc_*_rt logic

Worth noting this patch has no functional corrections it is just a stylistic change, so as Pierre said on v1 we could just leave things as is. Personally, I would prefer to merge it though, whilst the diff is a little more of a pain to review (hopefully eased somewhat by splitting out the alloc_*_rt logic into a separate patch), the resulting code reads much nicer and the code will be read a lot more times than this patch will be.

Indeed, the resulting code is much nicer to look at. I ignored the diffs and just looked at the result to make up my mind.

Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

Thanks, Charles

drivers/soundwire/stream.c | 124 +++++++++++++++++-------------------- 1 file changed, 56 insertions(+), 68 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index df5600a80c174..93baca08a0dea 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1355,25 +1355,23 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream, return -EINVAL; }

• if (!update_params)
  

• 	goto program_params;
  

• /* Increment cumulative bus bandwidth */
  

• /* TODO: Update this during Device-Device support */
  

• bus->params.bandwidth += m_rt->stream->params.rate *
  

• 	m_rt->ch_count * m_rt->stream->params.bps;
  

• /* Compute params */
  

• if (bus->compute_params) {
  

• 	ret = bus->compute_params(bus);
  

• 	if (ret < 0) {
  

• 		dev_err(bus->dev, "Compute params failed: %d\n",
  

• 			ret);
  

• 		goto restore_params;
  

• if (update_params) {
  

• 	/* Increment cumulative bus bandwidth */
  

• 	/* TODO: Update this during Device-Device support */
  

• 	bus->params.bandwidth += m_rt->stream->params.rate *
  

• 		m_rt->ch_count * m_rt->stream->params.bps;
  

• 	/* Compute params */
  

• 	if (bus->compute_params) {
  

• 		ret = bus->compute_params(bus);
  

• 		if (ret < 0) {
  

• 			dev_err(bus->dev, "Compute params failed: %d\n",
  

• 				ret);
  

• 			goto restore_params;
  

• 		}
  }
  

  }

-program_params: /* Program params */ ret = sdw_program_params(bus, true); if (ret < 0) { @@ -1876,30 +1874,25 @@ int sdw_stream_add_master(struct sdw_bus *bus, * it first), if so skip allocation and go to configuration */ m_rt = sdw_master_rt_find(bus, stream);

• if (m_rt)

• goto skip_alloc_master_rt;
  

• m_rt = sdw_master_rt_alloc(bus, stream); if (!m_rt) {

• dev_err(bus->dev, "%s: Master runtime alloc failed for stream:%s\n",
  

• 	__func__, stream->name);
  

• ret = -ENOMEM;
  

• goto unlock;
  

• }
• alloc_master_rt = true;

-skip_alloc_master_rt:

• if (sdw_master_port_allocated(m_rt))

• goto skip_alloc_master_port;
  

• m_rt = sdw_master_rt_alloc(bus, stream);
  

• if (!m_rt) {
  

• 	dev_err(bus->dev, "%s: Master runtime alloc failed for stream:%s\n",
  

• 		__func__, stream->name);
  

• 	ret = -ENOMEM;
  

• 	goto unlock;
  

• }
  
  

• ret = sdw_master_port_alloc(m_rt, num_ports);
• if (ret)

• goto alloc_error;
  

• alloc_master_rt = true;
  

• }

• stream->m_rt_count++;

• if (!sdw_master_port_allocated(m_rt)) {

• ret = sdw_master_port_alloc(m_rt, num_ports);
  

• if (ret)
  

• 	goto alloc_error;
  
  

-skip_alloc_master_port:

• stream->m_rt_count++;
  

• }

  ret = sdw_master_rt_config(m_rt, stream_config); if (ret < 0)

@@ -1992,46 +1985,41 @@ int sdw_stream_add_slave(struct sdw_slave *slave, * and go to configuration */ m_rt = sdw_master_rt_find(slave->bus, stream);

• if (m_rt)

• goto skip_alloc_master_rt;
  

• /*

• * If this API is invoked by Slave first then m_rt is not valid.
  

• * So, allocate m_rt and add Slave to it.
  

• */
  

• m_rt = sdw_master_rt_alloc(slave->bus, stream); if (!m_rt) {

• dev_err(&slave->dev, "%s: Master runtime alloc failed for stream:%s\n",
  

• 	__func__, stream->name);
  

• ret = -ENOMEM;
  

• goto unlock;
  

• }

• /*
  

•  * If this API is invoked by Slave first then m_rt is not valid.
  

•  * So, allocate m_rt and add Slave to it.
  

•  */
  

• m_rt = sdw_master_rt_alloc(slave->bus, stream);
  

• if (!m_rt) {
  

• 	dev_err(&slave->dev, "%s: Master runtime alloc failed for stream:%s\n",
  

• 		__func__, stream->name);
  

• 	ret = -ENOMEM;
  

• 	goto unlock;
  

• }
  
  

• alloc_master_rt = true;

• alloc_master_rt = true;
  

• }

-skip_alloc_master_rt: s_rt = sdw_slave_rt_find(slave, stream);

• if (s_rt)

• goto skip_alloc_slave_rt;
  

• s_rt = sdw_slave_rt_alloc(slave, m_rt); if (!s_rt) {

• dev_err(&slave->dev, "Slave runtime alloc failed for stream:%s\n", stream->name);
  

• ret = -ENOMEM;
  

• goto alloc_error;
  

• }
• alloc_slave_rt = true;

• s_rt = sdw_slave_rt_alloc(slave, m_rt);
  

• if (!s_rt) {
  

• 	dev_err(&slave->dev, "Slave runtime alloc failed for stream:%s\n",
  

• 		stream->name);
  

• 	ret = -ENOMEM;
  

• 	goto alloc_error;
  

• }
  
  

-skip_alloc_slave_rt:

• if (sdw_slave_port_allocated(s_rt))

• goto skip_port_alloc;
  

• alloc_slave_rt = true;
  

• }

• ret = sdw_slave_port_alloc(slave, s_rt, num_ports);
• if (ret)

• goto alloc_error;
  

• if (!sdw_slave_port_allocated(s_rt)) {

• ret = sdw_slave_port_alloc(slave, s_rt, num_ports);
  

• if (ret)
  

• 	goto alloc_error;
  

• }

-skip_port_alloc: ret = sdw_master_rt_config(m_rt, stream_config); if (ret) goto unlock;