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