linux-kernel - Re: [PATCH v3 3/3] serial: sc16is7xx: convert bitmask definitions to use BIT() macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VeqOV7GEqMs6fMi2Fax-97zt+ykEXaptng=pi_BDKU5Bg@mail.gmail.com>
Date: Fri, 23 Aug 2024 20:58:56 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Lech Perczak <lech.perczak@...lingroup.com>
Cc: linux-serial@...r.kernel.org, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Hugo Villeneuve <hvilleneuve@...onoff.com>, Andy Shevchenko <andy@...nel.org>, 
	Krzysztof Drobiński <k.drobinski@...lintechnologies.com>, 
	Paweł Lenkow <pawel.lenkow@...lingroup.com>, 
	Kirill Yatsenko <kirill.yatsenko@...lingroup.com>
Subject: Re: [PATCH v3 3/3] serial: sc16is7xx: convert bitmask definitions to
 use BIT() macro

On Fri, Aug 23, 2024 at 7:55 PM Lech Perczak
<lech.perczak@...lingroup.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 | ...


-- 
With Best Regards,
Andy Shevchenko