Hi,
On 2024/3/25 22:25, Dan Carpenter wrote:
On Mon, Mar 25, 2024 at 11:40:33AM +0800, Su Hui wrote:
--- a/sound/soc/sti/uniperif.h
+++ b/sound/soc/sti/uniperif.h
@@ -12,17 +12,28 @@
#include <sound/dmaengine_pcm.h>
+#define SR_SHIFT(a, b) ({unsigned long __a = (a); \
+ unsigned int __b = (b); \
+ __b < BITS_PER_LONG ? \
+ __a >> __b : 0; })
The code definitely looks buggy, but how do you know your solution is
correct without testing it?
I only test some cases like SR_SHIFT(1, -1),SR_SHIFT(8,1), it seems have a right result.
Oh, maybe I understand it. When 'a' is a negative value like '(int)-1', SR_SHIFT(a, b) will
have some bugs.
I don't like this solution at all. This is basically a really
complicated way of writing "if (b != -1)". Instead of checking for -1,
the better solution is to just stop passing -1. If you review that
file, every time it uses "-1" that's either dead code or a bug...
Agreed，some are dead codes which can be removed, but what should we do with the
following error codes like this one?
sound/soc/sti/uniperif.h
415 #define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) \
416 ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? 7 : -1)
...
423 #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) \
424 SET_UNIPERIF_REG(ip, \
425 UNIPERIF_CONFIG_OFFSET(ip), \
426 UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
How about this solution? If the condition is false, just skip it.
@@ -412,8 +412,7 @@
UNIPERIF_CONFIG_REPEAT_CHL_STS_MASK(ip), 1)
/* BACK_STALL_REQ */
-#define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) \
- ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? 7 : -1)
+#define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) 7
#define UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip) 0x1
#define GET_UNIPERIF_CONFIG_BACK_STALL_REQ(ip) \
GET_UNIPERIF_REG(ip, \
@@ -421,10 +420,11 @@
UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \
UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip))
#define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) \
+ ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? -1 : \
SET_UNIPERIF_REG(ip, \
UNIPERIF_CONFIG_OFFSET(ip), \
UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \
- UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip), 0)
+ UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip), 0))
#define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_ENABLE(ip) \
Maybe should print some error log here.
I'm not sure about the safety of skipping SET_UNIPERIF_REG when the condition is false,
Would it be better to make the result of undefined shift equal to zero?
regards,
Su Hui
