From: Pierre-Louis Bossart <[email protected]>
There are a couple of duplicate logs which makes harder than needed to
follow the error flows. Add __func__ or make the log unique.
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Ranjani Sridharan <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
drivers/soundwire/stream.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index df3b36670df4..e0eae0b98267 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1389,7 +1389,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
ret = do_bank_switch(stream);
if (ret < 0) {
- dev_err(bus->dev, "Bank switch failed: %d\n", ret);
+ dev_err(bus->dev, "do_bank_switch failed: %d\n", ret);
goto restore_params;
}
@@ -1477,7 +1477,7 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
/* Program params */
ret = sdw_program_params(bus, false);
if (ret < 0) {
- dev_err(bus->dev, "Program params failed: %d\n", ret);
+ dev_err(bus->dev, "%s: Program params failed: %d\n", __func__, ret);
return ret;
}
@@ -1567,7 +1567,7 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
/* Program params */
ret = sdw_program_params(bus, false);
if (ret < 0) {
- dev_err(bus->dev, "Program params failed: %d\n", ret);
+ dev_err(bus->dev, "%s: Program params failed: %d\n", __func__, ret);
return ret;
}
}
@@ -1664,7 +1664,7 @@ static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)
/* Program params */
ret = sdw_program_params(bus, false);
if (ret < 0) {
- dev_err(bus->dev, "Program params failed: %d\n", ret);
+ dev_err(bus->dev, "%s: Program params failed: %d\n", __func__, ret);
return ret;
}
}
@@ -1893,7 +1893,8 @@ int sdw_stream_add_master(struct sdw_bus *bus,
m_rt = sdw_master_rt_alloc(bus, stream);
if (!m_rt) {
- dev_err(bus->dev, "Master runtime alloc failed for stream:%s\n", stream->name);
+ dev_err(bus->dev, "%s: Master runtime alloc failed for stream:%s\n",
+ __func__, stream->name);
ret = -ENOMEM;
goto unlock;
}
@@ -2012,7 +2013,8 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
*/
m_rt = sdw_master_rt_alloc(slave->bus, stream);
if (!m_rt) {
- dev_err(&slave->dev, "Master runtime alloc failed for stream:%s\n", stream->name);
+ dev_err(&slave->dev, "%s: Master runtime alloc failed for stream:%s\n",
+ __func__, stream->name);
ret = -ENOMEM;
goto unlock;
}
--
2.25.1
On 1/13/2023 10:35 AM, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> There are a couple of duplicate logs which makes harder than needed to
> follow the error flows. Add __func__ or make the log unique.
>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> Reviewed-by: Ranjani Sridharan <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> drivers/soundwire/stream.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index df3b36670df4..e0eae0b98267 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1389,7 +1389,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
>
> ret = do_bank_switch(stream);
> if (ret < 0) {
> - dev_err(bus->dev, "Bank switch failed: %d\n", ret);
> + dev_err(bus->dev, "do_bank_switch failed: %d\n", ret);
> goto restore_params;
> }
This one seems bit unrelated to the change and makes error message
inconsistent with:
https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/soundwire.git/tree/drivers/soundwire/stream.c?h=next&id=545c365185a47672b1d5cc13c84057a1e874993c#n1498
and
https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/soundwire.git/tree/drivers/soundwire/stream.c?h=next&id=545c365185a47672b1d5cc13c84057a1e874993c#n1575
which actually brings me to another suggestion, can this error message
perhaps be just moved into do_bank_switch() function itself, instead of
being duplicated multiple times or alternatively just also prefix all of
them with function name?
On 1/13/23 04:22, Amadeusz Sławiński wrote:
> On 1/13/2023 10:35 AM, Bard Liao wrote:
>> From: Pierre-Louis Bossart <[email protected]>
>>
>> There are a couple of duplicate logs which makes harder than needed to
>> follow the error flows. Add __func__ or make the log unique.
>>
>> Signed-off-by: Pierre-Louis Bossart
>> <[email protected]>
>> Reviewed-by: Ranjani Sridharan <[email protected]>
>> Signed-off-by: Bard Liao <[email protected]>
>> ---
>> drivers/soundwire/stream.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
>> index df3b36670df4..e0eae0b98267 100644
>> --- a/drivers/soundwire/stream.c
>> +++ b/drivers/soundwire/stream.c
>> @@ -1389,7 +1389,7 @@ static int _sdw_prepare_stream(struct
>> sdw_stream_runtime *stream,
>> ret = do_bank_switch(stream);
>> if (ret < 0) {
>> - dev_err(bus->dev, "Bank switch failed: %d\n", ret);
>> + dev_err(bus->dev, "do_bank_switch failed: %d\n", ret);
>> goto restore_params;
>> }
>
> This one seems bit unrelated to the change and makes error message
> inconsistent with:
> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/soundwire.git/tree/drivers/soundwire/stream.c?h=next&id=545c365185a47672b1d5cc13c84057a1e874993c#n1498
> and
> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/soundwire.git/tree/drivers/soundwire/stream.c?h=next&id=545c365185a47672b1d5cc13c84057a1e874993c#n1575
> which actually brings me to another suggestion, can this error message
> perhaps be just moved into do_bank_switch() function itself, instead of
> being duplicated multiple times or alternatively just also prefix all of
> them with function name?
well, as you correctly pointed out, there are multiple users of
'do_bank_switch' so we don't want to put the message in the function itself.
We could indeed use __func__ instead, that'd be fine.
Looking at the code, there are also inconsistencies with the use of
pr_err and dev_err. dev_err(bus->dev is wrong actually, this would use
the bus variable assigned in the previous loop, this makes no sense for
multi-segment topologies.
Let's drop this patch and revisit all this, hope Vinod can deal with
patch 1..4 otherwise we'll resend the set.
Thanks Amadeusz for the feedback.