Re: [PATCH v6 02/15] xen/8250-uart: update definitions

[dmukhin@xxxxxxx]

On Sat, Sep 06, 2025 at 05:57:02PM +0300, Mykola Kvach wrote:
> Hi Denis,
> 
> On Sat, Sep 6, 2025 at 2:27 AM <dmukhin@xxxxxxx> wrote:
[..]
> >  /* FIFO Control Register */
> > -#define UART_FCR_ENABLE   0x01    /* enable FIFO          */
> > -#define UART_FCR_CLRX     0x02    /* clear Rx FIFO        */
> > -#define UART_FCR_CLTX     0x04    /* clear Tx FIFO        */
> > -#define UART_FCR_DMA      0x10    /* enter DMA mode       */
> > +#define UART_FCR_ENABLE     BIT(0, U)   /* enable FIFO          */
> > +#define UART_FCR_CLRX       BIT(1, U)   /* clear Rx FIFO        */
> > +#define UART_FCR_CLTX       BIT(2, U)   /* clear Tx FIFO        */
> > +#define UART_FCR_DMA        BIT(3, U)   /* enter DMA mode       */
> > +#define UART_FCR_RESERVED0  BIT(4, U)   /* reserved; always 0   */
> > +#define UART_FCR_RESERVED1  BIT(5, U)   /* reserved; always 0   */
> > +#define UART_FCR_RTB0       BIT(6, U)   /* receiver trigger bit #0 */
> > +#define UART_FCR_RTB1       BIT(7, U)   /* receiver trigger bit #1 */
> > +#define UART_FCR_TRG_MASK   (UART_FCR_RTB0 | UART_FCR_RTB1)
> 
> Thanks for the patch. I noticed that in this changeset some bit
> definitions (e.g. UART_FCR_*) were rewritten using the BIT(n, U)
> macro, while others (e.g. UART_IER_* and rest of UART_FCR_*) are
> still left as plain hex values (0x01, 0x02, etc.), even though they
> are also powers of two.
> 
> Could you clarify the reasoning behind this choice? From a reader’s
> perspective it looks inconsistent. Would it make sense to either:
> 
>   - update all of them to use BIT() for consistency, or
>   - keep the existing style unchanged in this patch and move a full
>     conversion to BIT() into a separate cleanup patch?
> 
> This would make the codebase easier to follow.

I find BIT() notation is more readable than raw bitmasks.
But I agree that definitions should be consistently updated.

Will update to the raw masks instead of BIT() in v7.