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 for Android: free password hash cracker in your pocket

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

Message-ID: <5d1f5d01-c7ab-49f9-b754-1c32043668c5@camlingroup.com>
Date: Mon, 26 Aug 2024 14:35:37 +0200
From: Lech Perczak <lech.perczak@...lingroup.com>
To: Andy Shevchenko <andy.shevchenko@...il.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

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

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