> >IMAN_IE is BIT(1), so these macro are respectively equivalent to BIT(1) >and 0, whatever the value of 'p'. > >The purpose was to set and reset a single bit in 'p'. >Fix these macros to do that correctly. > >Fixes: e93e58d27402 ("usb: cdnsp: Device side header file for CDNSP driver") >Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> Acked-by: Pawel Laszczak <pawell@xxxxxxxxxxx> Currently the bit 1(RW) and bit 0 (W1toClr) are implemented. All other bits are not used and are reserved for future RW implementations. I was a bit afraid about bit 0 which is "write 1 to clear" but I haven't found any issue with this patch during testing so we can simplify these macros Thanks Christophe >--- >This patch is speculative. It is not tested. >Please, review with care. > >Actually, the usage of these macros is: > readl(somewhere) > set or reset the IMAN_IE bit > write(somewhere) >So it is likely that we want to preserve the other bits read. Otherwise, >the code could be much simpler. >--- > drivers/usb/cdns3/cdnsp-gadget.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/usb/cdns3/cdnsp-gadget.h b/drivers/usb/cdns3/cdnsp-gadget.h >index 783ca8ffde00..f740fa6089d8 100644 >--- a/drivers/usb/cdns3/cdnsp-gadget.h >+++ b/drivers/usb/cdns3/cdnsp-gadget.h >@@ -383,8 +383,8 @@ struct cdnsp_intr_reg { > #define IMAN_IE BIT(1) > #define IMAN_IP BIT(0) > /* bits 2:31 need to be preserved */ >-#define IMAN_IE_SET(p) (((p) & IMAN_IE) | 0x2) >-#define IMAN_IE_CLEAR(p) (((p) & IMAN_IE) & ~(0x2)) >+#define IMAN_IE_SET(p) ((p) | IMAN_IE) >+#define IMAN_IE_CLEAR(p) ((p) & ~IMAN_IE) > > /* IMOD - Interrupter Moderation Register - irq_control bitmasks. */ > /* >-- >2.30.2 -- Regards, Pawel Laszczak