Re: [PATCH 4/4] soundwire: stream: Tidy do_bank_switch error messages

On 6/1/23 11:16, Charles Keepax wrote:

Every error path in do_bank_switch prints an error message so there is no need for the callers to also print error messages. Indeed in multi-master cases these extra messages are confusing because they print out against a random bus device whereas the do_bank_switch messages print against the bus that actually failed.

Errm, what?

There is no way to know which bus failed in multi-master mode, and the 'stream' can span multiple buses so the use of pr_err was intentional. There's just no other way to report issues, and I don't see why one would remove such logs and fail silently.

I just don't get what you are trying to address.

This also allows clean up of a couple of if's and variable initialisations that were only there to silence potentially uninitialised variable warnings on the bus variable.

That should be a separate patch IMHO.

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

drivers/soundwire/stream.c | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 248ab243ec6e4..b5c7a52aac19e 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;

@@ -1380,16 +1380,9 @@ 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);
  

• if (ret < 0) goto restore_params;

• }

  list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus;

@@ -1465,7 +1458,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 */

@@ -1488,16 +1481,9 @@ 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);
  

• if (ret < 0) return ret;

• }

  stream->state = SDW_STREAM_ENABLED; return 0;

@@ -1571,10 +1557,8 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) }

ret = do_bank_switch(stream);

• if (ret < 0) {

• pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);
  

• if (ret < 0) return ret;

• }

  /* make sure alternate bank (previous current) is also disabled */ list_for_each_entry(m_rt, &stream->master_list, stream_node) {