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@xxxxxxxxxxxxxxx> 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
