[PATCH v3 0/3] serial: sc16is7xx: cosmetic cleanup

[PATCH v3 0/3] serial: sc16is7xx: cosmetic cleanup

[Lech Perczak]

When submitting previous, functional fixes, Tomasz Moń omitted those
two cosmetic patches, that kept lurking in our company tree - likely
by oversight. Let's submit them.

Signed-off-by: Lech Perczak <lech.perczak@camlingroup.com>
---
v3:
No code changes in patches 1 and 2.
- Pick up Reviewed-by from Andy in patch 1
- Adjust commit message in patch 2
- Perform further cleanup in bit constants,
  use GENMASK for SC16IS7XX_IIR_* and reuse bit definitions in
  SC16IS7XX_LSR_BRK_ERROR_MASK in patch 3.

v2:
- Converted bitmask definitions to use BIT macro
  (thanks Jiri Slaby for the idea)
- Removed redundant comments in patch 2 altogether
- Fixed commit messages (thanks Andy Shevchenko for
  thorough review)

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Cc: Andy Shevchenko <andy@kernel.org>

Lech Perczak (3):
  serial: sc16is7xx: remove SC16IS7XX_MSR_DELTA_MASK
  serial: sc16is7xx: fix copy-paste errors in EFR_SWFLOWx_BIT constants
  serial: sc16is7xx: convert bitmask definitions to use BIT() macro

 drivers/tty/serial/sc16is7xx.c | 181 +++++++++++++++++----------------
 1 file changed, 92 insertions(+), 89 deletions(-)


base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
-- 
2.34.1

[PATCH v3 1/3] serial: sc16is7xx: remove SC16IS7XX_MSR_DELTA_MASK

[Lech Perczak]

This definition isn't used anywhere anymore, let's delete it.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Lech Perczak <lech.perczak@camlingroup.com>
---
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Cc: Andy Shevchenko <andy@kernel.org>
---
 drivers/tty/serial/sc16is7xx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index bf0065d1c8e9..8a2020f9930e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -207,7 +207,6 @@
 #define SC16IS7XX_MSR_CD_BIT		(1 << 7) /* CD (IO6)
 						  * - only on 75x/76x
 						  */
-#define SC16IS7XX_MSR_DELTA_MASK	0x0F     /* Any of the delta bits! */
 
 /*
  * TCR register bits
-- 
2.34.1

[PATCH v3 2/3] serial: sc16is7xx: fix copy-paste errors in EFR_SWFLOWx_BIT constants

[Lech Perczak]

Comments attached to bits 0 and 1 incorrectly referenced bits 2 and 3,
which don't match the datasheet - fix them.
At the same time remove comments for individual constants, as they add
nothing to the definitions themselves.

Signed-off-by: Lech Perczak <lech.perczak@camlingroup.com>
---
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Cc: Andy Shevchenko <andy@kernel.org>
---
 drivers/tty/serial/sc16is7xx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 8a2020f9930e..36b7c682ae94 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -266,9 +266,9 @@
 						  * and writing to IER[7:4],
 						  * FCR[5:4], MCR[7:5]
 						  */
-#define SC16IS7XX_EFR_SWFLOW3_BIT	(1 << 3) /* SWFLOW bit 3 */
-#define SC16IS7XX_EFR_SWFLOW2_BIT	(1 << 2) /* SWFLOW bit 2
-						  *
+#define SC16IS7XX_EFR_SWFLOW3_BIT	(1 << 3)
+#define SC16IS7XX_EFR_SWFLOW2_BIT	(1 << 2)
+						 /*
 						  * SWFLOW bits 3 & 2 table:
 						  * 00 -> no transmitter flow
 						  *       control
@@ -280,10 +280,10 @@
 						  *       XON1, XON2, XOFF1 and
 						  *       XOFF2
 						  */
-#define SC16IS7XX_EFR_SWFLOW1_BIT	(1 << 1) /* SWFLOW bit 2 */
-#define SC16IS7XX_EFR_SWFLOW0_BIT	(1 << 0) /* SWFLOW bit 3
-						  *
-						  * SWFLOW bits 3 & 2 table:
+#define SC16IS7XX_EFR_SWFLOW1_BIT	(1 << 1)
+#define SC16IS7XX_EFR_SWFLOW0_BIT	(1 << 0)
+						 /*
+						  * SWFLOW bits 1 & 0 table:
 						  * 00 -> no received flow
 						  *       control
 						  * 01 -> receiver compares
-- 
2.34.1

[PATCH v3 3/3] serial: sc16is7xx: convert bitmask definitions to use BIT() macro

[Lech Perczak]

Now that bit definition comments were cleaned up, convert bitmask
definitions to use BIT() macro for clarity.
Convert SC16IS7XX_IIR_* bitmask constants, to use GENMASK() macro, where
applicable - while at that, realign comments.
Compose SC16IS7XX_LSR_BRK_ERROR_MASK using aforementioned constants,
instead of open-coding it, and remove now unneeded comment.

Signed-off-by: Lech Perczak <lech.perczak@camlingroup.com>
---
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Cc: Andy Shevchenko <andy@kernel.org>
---
 drivers/tty/serial/sc16is7xx.c | 174 +++++++++++++++++----------------
 1 file changed, 89 insertions(+), 85 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 36b7c682ae94..68fdb2dee414 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -10,6 +10,7 @@
 #undef DEFAULT_SYMBOL_NAMESPACE
 #define DEFAULT_SYMBOL_NAMESPACE SERIAL_NXP_SC16IS7XX
 
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -77,52 +78,52 @@
 #define SC16IS7XX_XOFF2_REG		(0x07) /* Xoff2 word */
 
 /* IER register bits */
-#define SC16IS7XX_IER_RDI_BIT		(1 << 0) /* Enable RX data interrupt */
-#define SC16IS7XX_IER_THRI_BIT		(1 << 1) /* Enable TX holding register
+#define SC16IS7XX_IER_RDI_BIT		BIT(0)   /* Enable RX data interrupt */
+#define SC16IS7XX_IER_THRI_BIT		BIT(1)   /* Enable TX holding register
 						  * interrupt */
-#define SC16IS7XX_IER_RLSI_BIT		(1 << 2) /* Enable RX line status
+#define SC16IS7XX_IER_RLSI_BIT		BIT(2)   /* Enable RX line status
 						  * interrupt */
-#define SC16IS7XX_IER_MSI_BIT		(1 << 3) /* Enable Modem status
+#define SC16IS7XX_IER_MSI_BIT		BIT(3)   /* Enable Modem status
 						  * interrupt */
 
 /* IER register bits - write only if (EFR[4] == 1) */
-#define SC16IS7XX_IER_SLEEP_BIT		(1 << 4) /* Enable Sleep mode */
-#define SC16IS7XX_IER_XOFFI_BIT		(1 << 5) /* Enable Xoff interrupt */
-#define SC16IS7XX_IER_RTSI_BIT		(1 << 6) /* Enable nRTS interrupt */
-#define SC16IS7XX_IER_CTSI_BIT		(1 << 7) /* Enable nCTS interrupt */
+#define SC16IS7XX_IER_SLEEP_BIT		BIT(4)   /* Enable Sleep mode */
+#define SC16IS7XX_IER_XOFFI_BIT		BIT(5)   /* Enable Xoff interrupt */
+#define SC16IS7XX_IER_RTSI_BIT		BIT(6)   /* Enable nRTS interrupt */
+#define SC16IS7XX_IER_CTSI_BIT		BIT(7)   /* Enable nCTS interrupt */
 
 /* FCR register bits */
-#define SC16IS7XX_FCR_FIFO_BIT		(1 << 0) /* Enable FIFO */
-#define SC16IS7XX_FCR_RXRESET_BIT	(1 << 1) /* Reset RX FIFO */
-#define SC16IS7XX_FCR_TXRESET_BIT	(1 << 2) /* Reset TX FIFO */
-#define SC16IS7XX_FCR_RXLVLL_BIT	(1 << 6) /* RX Trigger level LSB */
-#define SC16IS7XX_FCR_RXLVLH_BIT	(1 << 7) /* RX Trigger level MSB */
+#define SC16IS7XX_FCR_FIFO_BIT		BIT(0)   /* Enable FIFO */
+#define SC16IS7XX_FCR_RXRESET_BIT	BIT(1)   /* Reset RX FIFO */
+#define SC16IS7XX_FCR_TXRESET_BIT	BIT(2)   /* Reset TX FIFO */
+#define SC16IS7XX_FCR_RXLVLL_BIT	BIT(6)   /* RX Trigger level LSB */
+#define SC16IS7XX_FCR_RXLVLH_BIT	BIT(7)   /* RX Trigger level MSB */
 
 /* FCR register bits - write only if (EFR[4] == 1) */
-#define SC16IS7XX_FCR_TXLVLL_BIT	(1 << 4) /* TX Trigger level LSB */
-#define SC16IS7XX_FCR_TXLVLH_BIT	(1 << 5) /* TX Trigger level MSB */
+#define SC16IS7XX_FCR_TXLVLL_BIT	BIT(4)   /* TX Trigger level LSB */
+#define SC16IS7XX_FCR_TXLVLH_BIT	BIT(5)   /* TX Trigger level MSB */
 
 /* IIR register bits */
-#define SC16IS7XX_IIR_NO_INT_BIT	(1 << 0) /* No interrupts pending */
-#define SC16IS7XX_IIR_ID_MASK		0x3e     /* Mask for the interrupt ID */
-#define SC16IS7XX_IIR_THRI_SRC		0x02     /* TX holding register empty */
-#define SC16IS7XX_IIR_RDI_SRC		0x04     /* RX data interrupt */
-#define SC16IS7XX_IIR_RLSE_SRC		0x06     /* RX line status error */
-#define SC16IS7XX_IIR_RTOI_SRC		0x0c     /* RX time-out interrupt */
-#define SC16IS7XX_IIR_MSI_SRC		0x00     /* Modem status interrupt
-						  * - only on 75x/76x
-						  */
-#define SC16IS7XX_IIR_INPIN_SRC		0x30     /* Input pin change of state
-						  * - only on 75x/76x
-						  */
-#define SC16IS7XX_IIR_XOFFI_SRC		0x10     /* Received Xoff */
-#define SC16IS7XX_IIR_CTSRTS_SRC	0x20     /* nCTS,nRTS change of state
-						  * from active (LOW)
-						  * to inactive (HIGH)
-						  */
+#define SC16IS7XX_IIR_NO_INT_BIT	BIT(0)   	/* No interrupts pending */
+#define SC16IS7XX_IIR_ID_MASK		GENMASK(5,1)	/* Mask for the interrupt ID */
+#define SC16IS7XX_IIR_THRI_SRC		BIT(1)		/* TX holding register empty */
+#define SC16IS7XX_IIR_RDI_SRC		BIT(2)		/* RX data interrupt */
+#define SC16IS7XX_IIR_RLSE_SRC		GENMASK(2,1)	/* RX line status error */
+#define SC16IS7XX_IIR_RTOI_SRC		GENMASK(3,2)    /* RX time-out interrupt */
+#define SC16IS7XX_IIR_MSI_SRC		0x00		/* Modem status interrupt
+							 * - only on 75x/76x
+							 */
+#define SC16IS7XX_IIR_INPIN_SRC		GENMASK(5,4)	/* Input pin change of state
+							 * - only on 75x/76x
+							 */
+#define SC16IS7XX_IIR_XOFFI_SRC		BIT(4)		/* Received Xoff */
+#define SC16IS7XX_IIR_CTSRTS_SRC	BIT(5)		/* nCTS,nRTS change of state
+							 * from active (LOW)
+							 * to inactive (HIGH)
+							 */
 /* LCR register bits */
-#define SC16IS7XX_LCR_LENGTH0_BIT	(1 << 0) /* Word length bit 0 */
-#define SC16IS7XX_LCR_LENGTH1_BIT	(1 << 1) /* Word length bit 1
+#define SC16IS7XX_LCR_LENGTH0_BIT	BIT(0)   /* Word length bit 0 */
+#define SC16IS7XX_LCR_LENGTH1_BIT	BIT(1)   /* Word length bit 1
 						  *
 						  * Word length bits table:
 						  * 00 -> 5 bit words
@@ -130,7 +131,7 @@
 						  * 10 -> 7 bit words
 						  * 11 -> 8 bit words
 						  */
-#define SC16IS7XX_LCR_STOPLEN_BIT	(1 << 2) /* STOP length bit
+#define SC16IS7XX_LCR_STOPLEN_BIT	BIT(2)   /* STOP length bit
 						  *
 						  * STOP length bit table:
 						  * 0 -> 1 stop bit
@@ -138,11 +139,11 @@
 						  *      word length is 5,
 						  *      2 stop bits otherwise
 						  */
-#define SC16IS7XX_LCR_PARITY_BIT	(1 << 3) /* Parity bit enable */
-#define SC16IS7XX_LCR_EVENPARITY_BIT	(1 << 4) /* Even parity bit enable */
-#define SC16IS7XX_LCR_FORCEPARITY_BIT	(1 << 5) /* 9-bit multidrop parity */
-#define SC16IS7XX_LCR_TXBREAK_BIT	(1 << 6) /* TX break enable */
-#define SC16IS7XX_LCR_DLAB_BIT		(1 << 7) /* Divisor Latch enable */
+#define SC16IS7XX_LCR_PARITY_BIT	BIT(3)   /* Parity bit enable */
+#define SC16IS7XX_LCR_EVENPARITY_BIT	BIT(4)   /* Even parity bit enable */
+#define SC16IS7XX_LCR_FORCEPARITY_BIT	BIT(5)   /* 9-bit multidrop parity */
+#define SC16IS7XX_LCR_TXBREAK_BIT	BIT(6)   /* TX break enable */
+#define SC16IS7XX_LCR_DLAB_BIT		BIT(7)   /* Divisor Latch enable */
 #define SC16IS7XX_LCR_WORD_LEN_5	(0x00)
 #define SC16IS7XX_LCR_WORD_LEN_6	(0x01)
 #define SC16IS7XX_LCR_WORD_LEN_7	(0x02)
@@ -153,58 +154,61 @@
 								* reg set */
 
 /* MCR register bits */
-#define SC16IS7XX_MCR_DTR_BIT		(1 << 0) /* DTR complement
+#define SC16IS7XX_MCR_DTR_BIT		BIT(0)   /* DTR complement
 						  * - only on 75x/76x
 						  */
-#define SC16IS7XX_MCR_RTS_BIT		(1 << 1) /* RTS complement */
-#define SC16IS7XX_MCR_TCRTLR_BIT	(1 << 2) /* TCR/TLR register enable */
-#define SC16IS7XX_MCR_LOOP_BIT		(1 << 4) /* Enable loopback test mode */
-#define SC16IS7XX_MCR_XONANY_BIT	(1 << 5) /* Enable Xon Any
+#define SC16IS7XX_MCR_RTS_BIT		BIT(1)   /* RTS complement */
+#define SC16IS7XX_MCR_TCRTLR_BIT	BIT(2)   /* TCR/TLR register enable */
+#define SC16IS7XX_MCR_LOOP_BIT		BIT(4)   /* Enable loopback test mode */
+#define SC16IS7XX_MCR_XONANY_BIT	BIT(5)   /* Enable Xon Any
 						  * - write enabled
 						  * if (EFR[4] == 1)
 						  */
-#define SC16IS7XX_MCR_IRDA_BIT		(1 << 6) /* Enable IrDA mode
+#define SC16IS7XX_MCR_IRDA_BIT		BIT(6)   /* Enable IrDA mode
 						  * - write enabled
 						  * if (EFR[4] == 1)
 						  */
-#define SC16IS7XX_MCR_CLKSEL_BIT	(1 << 7) /* Divide clock by 4
+#define SC16IS7XX_MCR_CLKSEL_BIT	BIT(7)   /* Divide clock by 4
 						  * - write enabled
 						  * if (EFR[4] == 1)
 						  */
 
 /* LSR register bits */
-#define SC16IS7XX_LSR_DR_BIT		(1 << 0) /* Receiver data ready */
-#define SC16IS7XX_LSR_OE_BIT		(1 << 1) /* Overrun Error */
-#define SC16IS7XX_LSR_PE_BIT		(1 << 2) /* Parity Error */
-#define SC16IS7XX_LSR_FE_BIT		(1 << 3) /* Frame Error */
-#define SC16IS7XX_LSR_BI_BIT		(1 << 4) /* Break Interrupt */
-#define SC16IS7XX_LSR_BRK_ERROR_MASK	0x1E     /* BI, FE, PE, OE bits */
-#define SC16IS7XX_LSR_THRE_BIT		(1 << 5) /* TX holding register empty */
-#define SC16IS7XX_LSR_TEMT_BIT		(1 << 6) /* Transmitter empty */
-#define SC16IS7XX_LSR_FIFOE_BIT		(1 << 7) /* Fifo Error */
+#define SC16IS7XX_LSR_DR_BIT		BIT(0)   /* Receiver data ready */
+#define SC16IS7XX_LSR_OE_BIT		BIT(1)   /* Overrun Error */
+#define SC16IS7XX_LSR_PE_BIT		BIT(2)   /* Parity Error */
+#define SC16IS7XX_LSR_FE_BIT		BIT(3)   /* Frame Error */
+#define SC16IS7XX_LSR_BI_BIT		BIT(4)   /* Break Interrupt */
+#define SC16IS7XX_LSR_BRK_ERROR_MASK	(SC16IS7XX_LSR_OE_BIT | \
+					SC16IS7XX_LSR_PE_BIT | \
+					SC16IS7XX_LSR_FE_BIT | \
+					SC16IS7XX_LSR_BI_BIT)
+#define SC16IS7XX_LSR_THRE_BIT		BIT(5)   /* TX holding register empty */
+#define SC16IS7XX_LSR_TEMT_BIT		BIT(6)   /* Transmitter empty */
+#define SC16IS7XX_LSR_FIFOE_BIT		BIT(7)   /* Fifo Error */
 
 /* MSR register bits */
-#define SC16IS7XX_MSR_DCTS_BIT		(1 << 0) /* Delta CTS Clear To Send */
-#define SC16IS7XX_MSR_DDSR_BIT		(1 << 1) /* Delta DSR Data Set Ready
+#define SC16IS7XX_MSR_DCTS_BIT		BIT(0)   /* Delta CTS Clear To Send */
+#define SC16IS7XX_MSR_DDSR_BIT		BIT(1)   /* Delta DSR Data Set Ready
 						  * or (IO4)
 						  * - only on 75x/76x
 						  */
-#define SC16IS7XX_MSR_DRI_BIT		(1 << 2) /* Delta RI Ring Indicator
+#define SC16IS7XX_MSR_DRI_BIT		BIT(2)   /* Delta RI Ring Indicator
 						  * or (IO7)
 						  * - only on 75x/76x
 						  */
-#define SC16IS7XX_MSR_DCD_BIT		(1 << 3) /* Delta CD Carrier Detect
+#define SC16IS7XX_MSR_DCD_BIT		BIT(3)   /* Delta CD Carrier Detect
 						  * or (IO6)
 						  * - only on 75x/76x
 						  */
-#define SC16IS7XX_MSR_CTS_BIT		(1 << 4) /* CTS */
-#define SC16IS7XX_MSR_DSR_BIT		(1 << 5) /* DSR (IO4)
+#define SC16IS7XX_MSR_CTS_BIT		BIT(4)   /* CTS */
+#define SC16IS7XX_MSR_DSR_BIT		BIT(5)   /* DSR (IO4)
 						  * - only on 75x/76x
 						  */
-#define SC16IS7XX_MSR_RI_BIT		(1 << 6) /* RI (IO7)
+#define SC16IS7XX_MSR_RI_BIT		BIT(6)   /* RI (IO7)
 						  * - only on 75x/76x
 						  */
-#define SC16IS7XX_MSR_CD_BIT		(1 << 7) /* CD (IO6)
+#define SC16IS7XX_MSR_CD_BIT		BIT(7)   /* CD (IO6)
 						  * - only on 75x/76x
 						  */
 
@@ -239,19 +243,19 @@
 #define SC16IS7XX_TLR_RX_TRIGGER(words)	((((words) / 4) & 0x0f) << 4)
 
 /* IOControl register bits (Only 75x/76x) */
-#define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
-#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
-#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
-#define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
+#define SC16IS7XX_IOCONTROL_LATCH_BIT	BIT(0)   /* Enable input latching */
+#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	BIT(1)   /* Enable GPIO[7:4] as modem A pins */
+#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	BIT(2)   /* Enable GPIO[3:0] as modem B pins */
+#define SC16IS7XX_IOCONTROL_SRESET_BIT	BIT(3)   /* Software Reset */
 
 /* EFCR register bits */
-#define SC16IS7XX_EFCR_9BIT_MODE_BIT	(1 << 0) /* Enable 9-bit or Multidrop
+#define SC16IS7XX_EFCR_9BIT_MODE_BIT	BIT(0)   /* Enable 9-bit or Multidrop
 						  * mode (RS485) */
-#define SC16IS7XX_EFCR_RXDISABLE_BIT	(1 << 1) /* Disable receiver */
-#define SC16IS7XX_EFCR_TXDISABLE_BIT	(1 << 2) /* Disable transmitter */
-#define SC16IS7XX_EFCR_AUTO_RS485_BIT	(1 << 4) /* Auto RS485 RTS direction */
-#define SC16IS7XX_EFCR_RTS_INVERT_BIT	(1 << 5) /* RTS output inversion */
-#define SC16IS7XX_EFCR_IRDA_MODE_BIT	(1 << 7) /* IrDA mode
+#define SC16IS7XX_EFCR_RXDISABLE_BIT	BIT(1)   /* Disable receiver */
+#define SC16IS7XX_EFCR_TXDISABLE_BIT	BIT(2)   /* Disable transmitter */
+#define SC16IS7XX_EFCR_AUTO_RS485_BIT	BIT(4)   /* Auto RS485 RTS direction */
+#define SC16IS7XX_EFCR_RTS_INVERT_BIT	BIT(5)   /* RTS output inversion */
+#define SC16IS7XX_EFCR_IRDA_MODE_BIT	BIT(7)   /* IrDA mode
 						  * 0 = rate upto 115.2 kbit/s
 						  *   - Only 75x/76x
 						  * 1 = rate upto 1.152 Mbit/s
@@ -259,15 +263,15 @@
 						  */
 
 /* EFR register bits */
-#define SC16IS7XX_EFR_AUTORTS_BIT	(1 << 6) /* Auto RTS flow ctrl enable */
-#define SC16IS7XX_EFR_AUTOCTS_BIT	(1 << 7) /* Auto CTS flow ctrl enable */
-#define SC16IS7XX_EFR_XOFF2_DETECT_BIT	(1 << 5) /* Enable Xoff2 detection */
-#define SC16IS7XX_EFR_ENABLE_BIT	(1 << 4) /* Enable enhanced functions
+#define SC16IS7XX_EFR_AUTORTS_BIT	BIT(6)   /* Auto RTS flow ctrl enable */
+#define SC16IS7XX_EFR_AUTOCTS_BIT	BIT(7)   /* Auto CTS flow ctrl enable */
+#define SC16IS7XX_EFR_XOFF2_DETECT_BIT	BIT(5)   /* Enable Xoff2 detection */
+#define SC16IS7XX_EFR_ENABLE_BIT	BIT(4)   /* Enable enhanced functions
 						  * and writing to IER[7:4],
 						  * FCR[5:4], MCR[7:5]
 						  */
-#define SC16IS7XX_EFR_SWFLOW3_BIT	(1 << 3)
-#define SC16IS7XX_EFR_SWFLOW2_BIT	(1 << 2)
+#define SC16IS7XX_EFR_SWFLOW3_BIT	BIT(3)
+#define SC16IS7XX_EFR_SWFLOW2_BIT	BIT(2)
 						 /*
 						  * SWFLOW bits 3 & 2 table:
 						  * 00 -> no transmitter flow
@@ -280,8 +284,8 @@
 						  *       XON1, XON2, XOFF1 and
 						  *       XOFF2
 						  */
-#define SC16IS7XX_EFR_SWFLOW1_BIT	(1 << 1)
-#define SC16IS7XX_EFR_SWFLOW0_BIT	(1 << 0)
+#define SC16IS7XX_EFR_SWFLOW1_BIT	BIT(1)
+#define SC16IS7XX_EFR_SWFLOW0_BIT	BIT(0)
 						 /*
 						  * SWFLOW bits 1 & 0 table:
 						  * 00 -> no received flow
@@ -307,9 +311,9 @@
 #define SC16IS7XX_FIFO_SIZE		(64)
 #define SC16IS7XX_GPIOS_PER_BANK	4
 
-#define SC16IS7XX_RECONF_MD		(1 << 0)
-#define SC16IS7XX_RECONF_IER		(1 << 1)
-#define SC16IS7XX_RECONF_RS485		(1 << 2)
+#define SC16IS7XX_RECONF_MD		BIT(0)
+#define SC16IS7XX_RECONF_IER		BIT(1)
+#define SC16IS7XX_RECONF_RS485		BIT(2)
 
 struct sc16is7xx_one_config {
 	unsigned int			flags;
-- 
2.34.1

Re: [PATCH v3 3/3] serial: sc16is7xx: convert bitmask definitions to use BIT() macro

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 7:55 PM Lech Perczak
<lech.perczak@camlingroup.com> wrote:
>
> Now that bit definition comments were cleaned up, convert bitmask
> definitions to use BIT() macro for clarity.
> Convert SC16IS7XX_IIR_* bitmask constants, to use GENMASK() macro, where
> applicable - while at that, realign comments.
> Compose SC16IS7XX_LSR_BRK_ERROR_MASK using aforementioned constants,
> instead of open-coding it, and remove now unneeded comment.

comments

...

>  /* IIR register bits */
> -#define SC16IS7XX_IIR_NO_INT_BIT       (1 << 0) /* No interrupts pending */
> -#define SC16IS7XX_IIR_ID_MASK          0x3e     /* Mask for the interrupt ID */
> -#define SC16IS7XX_IIR_THRI_SRC         0x02     /* TX holding register empty */
> -#define SC16IS7XX_IIR_RDI_SRC          0x04     /* RX data interrupt */
> -#define SC16IS7XX_IIR_RLSE_SRC         0x06     /* RX line status error */
> -#define SC16IS7XX_IIR_RTOI_SRC         0x0c     /* RX time-out interrupt */
> -#define SC16IS7XX_IIR_MSI_SRC          0x00     /* Modem status interrupt
> -                                                 * - only on 75x/76x
> -                                                 */
> -#define SC16IS7XX_IIR_INPIN_SRC                0x30     /* Input pin change of state
> -                                                 * - only on 75x/76x
> -                                                 */
> -#define SC16IS7XX_IIR_XOFFI_SRC                0x10     /* Received Xoff */
> -#define SC16IS7XX_IIR_CTSRTS_SRC       0x20     /* nCTS,nRTS change of state
> -                                                 * from active (LOW)
> -                                                 * to inactive (HIGH)
> -                                                 */
> +#define SC16IS7XX_IIR_NO_INT_BIT       BIT(0)          /* No interrupts pending */

> +#define SC16IS7XX_IIR_ID_MASK          GENMASK(5,1)    /* Mask for the interrupt ID */

This is okay, but the rest of the bit combinations are better to have
to be plain numbers as usually they are listed in this way in the
datasheets. Note as well that 0x00 is a valid value which you can't
express using BIT() or GENMASK() (and this is usually the main point
to *not* convert them to these macros).

> +#define SC16IS7XX_IIR_THRI_SRC         BIT(1)          /* TX holding register empty */
> +#define SC16IS7XX_IIR_RDI_SRC          BIT(2)          /* RX data interrupt */
> +#define SC16IS7XX_IIR_RLSE_SRC         GENMASK(2,1)    /* RX line status error */
> +#define SC16IS7XX_IIR_RTOI_SRC         GENMASK(3,2)    /* RX time-out interrupt */
> +#define SC16IS7XX_IIR_MSI_SRC          0x00            /* Modem status interrupt
> +                                                        * - only on 75x/76x
> +                                                        */
> +#define SC16IS7XX_IIR_INPIN_SRC                GENMASK(5,4)    /* Input pin change of state
> +                                                        * - only on 75x/76x
> +                                                        */
> +#define SC16IS7XX_IIR_XOFFI_SRC                BIT(4)          /* Received Xoff */
> +#define SC16IS7XX_IIR_CTSRTS_SRC       BIT(5)          /* nCTS,nRTS change of state
> +                                                        * from active (LOW)
> +                                                        * to inactive (HIGH)
> +                                                        */

...

> +#define SC16IS7XX_LSR_BRK_ERROR_MASK   (SC16IS7XX_LSR_OE_BIT | \
> +                                       SC16IS7XX_LSR_PE_BIT | \
> +                                       SC16IS7XX_LSR_FE_BIT | \
> +                                       SC16IS7XX_LSR_BI_BIT)

It's better to start from the next line

#define SC16IS7XX_LSR_BRK_ERROR_MASK     \
        (SC16IS7XX_LSR_OE_BIT | ...


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] serial: sc16is7xx: convert bitmask definitions to use BIT() macro

[Lech Perczak]

Hi Andy,

Thanks for thorough review.

W dniu 23.08.2024 o 19:58, Andy Shevchenko pisze:
> On Fri, Aug 23, 2024 at 7:55 PM Lech Perczak
> <lech.perczak@camlingroup.com> wrote:
>>
>> Now that bit definition comments were cleaned up, convert bitmask
>> definitions to use BIT() macro for clarity.
>> Convert SC16IS7XX_IIR_* bitmask constants, to use GENMASK() macro, where
>> applicable - while at that, realign comments.
>> Compose SC16IS7XX_LSR_BRK_ERROR_MASK using aforementioned constants,
>> instead of open-coding it, and remove now unneeded comment.
> 
> comments

Noted.

> 
> ...
> 
>>  /* IIR register bits */
>> -#define SC16IS7XX_IIR_NO_INT_BIT       (1 << 0) /* No interrupts pending */
>> -#define SC16IS7XX_IIR_ID_MASK          0x3e     /* Mask for the interrupt ID */
>> -#define SC16IS7XX_IIR_THRI_SRC         0x02     /* TX holding register empty */
>> -#define SC16IS7XX_IIR_RDI_SRC          0x04     /* RX data interrupt */
>> -#define SC16IS7XX_IIR_RLSE_SRC         0x06     /* RX line status error */
>> -#define SC16IS7XX_IIR_RTOI_SRC         0x0c     /* RX time-out interrupt */
>> -#define SC16IS7XX_IIR_MSI_SRC          0x00     /* Modem status interrupt
>> -                                                 * - only on 75x/76x
>> -                                                 */
>> -#define SC16IS7XX_IIR_INPIN_SRC                0x30     /* Input pin change of state
>> -                                                 * - only on 75x/76x
>> -                                                 */
>> -#define SC16IS7XX_IIR_XOFFI_SRC                0x10     /* Received Xoff */
>> -#define SC16IS7XX_IIR_CTSRTS_SRC       0x20     /* nCTS,nRTS change of state
>> -                                                 * from active (LOW)
>> -                                                 * to inactive (HIGH)
>> -                                                 */
>> +#define SC16IS7XX_IIR_NO_INT_BIT       BIT(0)          /* No interrupts pending */
> 
>> +#define SC16IS7XX_IIR_ID_MASK          GENMASK(5,1)    /* Mask for the interrupt ID */
> 
> This is okay, but the rest of the bit combinations are better to have
> to be plain numbers as usually they are listed in this way in the
> datasheets. Note as well that 0x00 is a valid value which you can't
> express using BIT() or GENMASK() (and this is usually the main point
> to *not* convert them to these macros).
> 
>> +#define SC16IS7XX_IIR_THRI_SRC         BIT(1)          /* TX holding register empty */
>> +#define SC16IS7XX_IIR_RDI_SRC          BIT(2)          /* RX data interrupt */
>> +#define SC16IS7XX_IIR_RLSE_SRC         GENMASK(2,1)    /* RX line status error */
>> +#define SC16IS7XX_IIR_RTOI_SRC         GENMASK(3,2)    /* RX time-out interrupt */
>> +#define SC16IS7XX_IIR_MSI_SRC          0x00            /* Modem status interrupt
>> +                                                        * - only on 75x/76x
>> +                                                        */
>> +#define SC16IS7XX_IIR_INPIN_SRC                GENMASK(5,4)    /* Input pin change of state
>> +                                                        * - only on 75x/76x
>> +                                                        */
>> +#define SC16IS7XX_IIR_XOFFI_SRC                BIT(4)          /* Received Xoff */
>> +#define SC16IS7XX_IIR_CTSRTS_SRC       BIT(5)          /* nCTS,nRTS change of state
>> +                                                        * from active (LOW)
>> +                                                        * to inactive (HIGH)
>> +                                                        */
>
Before I send out v4, do I get it right, that I should convert back SC16IS7XX_*_SRC
(i.e. interrupt source constants), and leave the rest as in v3?
 
> ...
> 
>> +#define SC16IS7XX_LSR_BRK_ERROR_MASK   (SC16IS7XX_LSR_OE_BIT | \
>> +                                       SC16IS7XX_LSR_PE_BIT | \
>> +                                       SC16IS7XX_LSR_FE_BIT | \
>> +                                       SC16IS7XX_LSR_BI_BIT)
> 
> It's better to start from the next line
> 
> #define SC16IS7XX_LSR_BRK_ERROR_MASK     \
>         (SC16IS7XX_LSR_OE_BIT | ...

Makes sense, noted.
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

-- 
Pozdrawiam/With kind regards,
Lech Perczak

Sr. Software Engineer
Camlin Technologies Poland Limited Sp. z o.o.
Strzegomska 54,
53-611 Wroclaw

Re: [PATCH v3 3/3] serial: sc16is7xx: convert bitmask definitions to use BIT() macro

[Andy Shevchenko]

On Mon, Aug 26, 2024 at 02:35:37PM +0200, Lech Perczak wrote:
> W dniu 23.08.2024 o 19:58, Andy Shevchenko pisze:
> > On Fri, Aug 23, 2024 at 7:55 PM Lech Perczak
> > <lech.perczak@camlingroup.com> wrote:

...

> >>  /* IIR register bits */
> >> -#define SC16IS7XX_IIR_NO_INT_BIT       (1 << 0) /* No interrupts pending */
> >> -#define SC16IS7XX_IIR_ID_MASK          0x3e     /* Mask for the interrupt ID */
> >> -#define SC16IS7XX_IIR_THRI_SRC         0x02     /* TX holding register empty */
> >> -#define SC16IS7XX_IIR_RDI_SRC          0x04     /* RX data interrupt */
> >> -#define SC16IS7XX_IIR_RLSE_SRC         0x06     /* RX line status error */
> >> -#define SC16IS7XX_IIR_RTOI_SRC         0x0c     /* RX time-out interrupt */
> >> -#define SC16IS7XX_IIR_MSI_SRC          0x00     /* Modem status interrupt
> >> -                                                 * - only on 75x/76x
> >> -                                                 */
> >> -#define SC16IS7XX_IIR_INPIN_SRC                0x30     /* Input pin change of state
> >> -                                                 * - only on 75x/76x
> >> -                                                 */
> >> -#define SC16IS7XX_IIR_XOFFI_SRC                0x10     /* Received Xoff */
> >> -#define SC16IS7XX_IIR_CTSRTS_SRC       0x20     /* nCTS,nRTS change of state
> >> -                                                 * from active (LOW)
> >> -                                                 * to inactive (HIGH)
> >> -                                                 */
> >> +#define SC16IS7XX_IIR_NO_INT_BIT       BIT(0)          /* No interrupts pending */

> >> +#define SC16IS7XX_IIR_ID_MASK          GENMASK(5,1)    /* Mask for the interrupt ID */

This is the only change (one definition / line) makes sense in this block.

> > This is okay, but the rest of the bit combinations are better to have
> > to be plain numbers as usually they are listed in this way in the
> > datasheets. Note as well that 0x00 is a valid value which you can't
> > express using BIT() or GENMASK() (and this is usually the main point
> > to *not* convert them to these macros).
> > 
> >> +#define SC16IS7XX_IIR_THRI_SRC         BIT(1)          /* TX holding register empty */
> >> +#define SC16IS7XX_IIR_RDI_SRC          BIT(2)          /* RX data interrupt */
> >> +#define SC16IS7XX_IIR_RLSE_SRC         GENMASK(2,1)    /* RX line status error */
> >> +#define SC16IS7XX_IIR_RTOI_SRC         GENMASK(3,2)    /* RX time-out interrupt */
> >> +#define SC16IS7XX_IIR_MSI_SRC          0x00            /* Modem status interrupt
> >> +                                                        * - only on 75x/76x
> >> +                                                        */
> >> +#define SC16IS7XX_IIR_INPIN_SRC                GENMASK(5,4)    /* Input pin change of state
> >> +                                                        * - only on 75x/76x
> >> +                                                        */
> >> +#define SC16IS7XX_IIR_XOFFI_SRC                BIT(4)          /* Received Xoff */
> >> +#define SC16IS7XX_IIR_CTSRTS_SRC       BIT(5)          /* nCTS,nRTS change of state
> >> +                                                        * from active (LOW)
> >> +                                                        * to inactive (HIGH)
> >> +                                                        */
> >
> Before I send out v4, do I get it right, that I should convert back SC16IS7XX_*_SRC
> (i.e. interrupt source constants), and leave the rest as in v3?

See above. I.o.w. change only _MASK and leave the rest as is.

-- 
With Best Regards,
Andy Shevchenko