| Message ID | d12bfcc9cbffb89e27b120668821b3c4f09b6755.1624390584.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | usb: cdnsp: Fix the IMAN_IE_SET and IMAN_IE_CLEAR macro. | expand |
> >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@wanadoo.fr> Acked-by: Pawel Laszczak <pawell@cadence.com> 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
Corrected Peter Chen address - should be peter.chen@kernel.org >>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@wanadoo.fr> > >Acked-by: Pawel Laszczak <pawell@cadence.com> > >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
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. */ /*
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@wanadoo.fr> --- 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(-)