| Message ID | 7deb753f-bf86-47ce-89bf-8277aca4293e@camlingroup.com |
|---|---|
| Headers | show |
| Series | serial: sc16is7xx: cosmetic cleanup | expand |
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 | ...
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
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.
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