On Thu, Apr 01, 2021 at 01:43:53PM -0500, Pierre-Louis Bossart wrote:
My bigger issue with this is that this macro is crazy. Why do you need debugging here at all for this type of thing? That's what ftrace is for, do not sprinkle code with "we got this return value from here!" all over the place like what this does.
We are not sprinkling the code all over the place with any new logs, they exist already in the SoundWire code and this patch helps filter them out. See e.g. patch 2/2
dev_err(&slave->dev,"Clk Stop type =%d failed: %d\n", type, ret);
sdw_dev_dbg_or_err(&slave->dev, ret != -ENODATA,"Clk Stop mode %d type =%d failed: %d\n",mode, type, ret);You just added a debug log for no reason.
The number of logs is lower when dynamic debug is not enabled, and equal when it is. there's no addition.
The previous behavior was unconditional dev_err that everyone sees.
Now it's dev_err ONLY when the code is NOT -ENODATA, and dev_dgb otherwise, meaning it will seen ONLY be seen IF dynamic debug is enabled for drivers/soundwire/bus.c
Allow me to use another example from patch2:
if (ret == -ENODATA)dev_dbg(bus->dev,"ClockStopNow Broadcast msg ignored %d", ret);elsedev_err(bus->dev,"ClockStopNow Broadcast msg failed %d", ret);
sdw_dev_dbg_or_err(bus->dev, ret != -ENODATA,"ClockStopNow Broadcast msg failed %d\n", ret);There's no new log, is there?
No, but that is not what you showed above which was just an error message being replaced with both a debug and an error message.
Just drop the debug messages, they are pointless, right?
thanks,
greg k-h