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
...
> /* 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
