Message-ID: <8e4874bc-893f-44b5-9775-4581d628ba1e@camlingroup.com>
Date: Tue, 27 Aug 2024 17:38:50 +0200
From: Lech Perczak <lech.perczak@...lingroup.com>
To: Andy Shevchenko <andy@...nel.org>, linux-serial@...r.kernel.org,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Jiri Slaby <jirislaby@...nel.org>, Hugo Villeneuve
 <hvilleneuve@...onoff.com>,
 Krzysztof Drobiński <k.drobinski@...lintechnologies.com>,
 Pawel Lenkow <p.lenkow@...lintechnologies.com>,
 Kirill Yatsenko <kirill.yatsenko@...lingroup.com>
Subject: Re: [PATCH v4 0/3] serial: sc16is7xx: cosmetic cleanup

Hi Andy,

W dniu 26.08.2024 o 19:40, Andy Shevchenko pisze:
> On Mon, Aug 26, 2024 at 05:40:28PM +0200, Lech Perczak wrote:
>> When submitting previous, functional fixes, Tomasz Moń omitted those
>> two cosmetic patches, that kept lurking in our company tree - likely
>> by oversight. Let's submit them.
> 
> Reviewed-by: Andy Shevchenko <andy@...nel.org>
> 
> I haven't looked into the details of the changes, but feels good.
> What you can do to confirm is to run this via C preprocessor and
> show the diff here or assure that it's empty.
> 

Great tip for checking such changes the correctness.
Up to patch v2 there were no significant changes, as expected, but for patch 3,
diff is quite substantial, due to BIT() being more explicit,
than open-coded constants.

Only the single expansion of GENMASK proves very hard to analyze in the
diff - so I double-checked with a calculator,
though all BIT() expansions do match, as does the updated definition.
of SC16IS7XX_LSR_BRK_ERROR_MASK.

Anyway, for completeness here is the diff in full for completeness. It's huge 
and has lines way over 80 characters, but that's how it is.
I obtained inputs for the diff by 'make drivers/tty/serial/sc16is7xx.i'.

46,48c46,80
< # 1 "./include/linux/clk.h" 1
< # 12 "./include/linux/clk.h"
< # 1 "./include/linux/err.h" 1
---
> # 1 "./include/linux/bits.h" 1
> 
> 
> 
> 
> # 1 "./include/linux/const.h" 1
> 
> 
> 
> # 1 "./include/vdso/const.h" 1
> 
> 
> 
> 
> # 1 "./include/uapi/linux/const.h" 1
> # 6 "./include/vdso/const.h" 2
> # 5 "./include/linux/const.h" 2
> # 6 "./include/linux/bits.h" 2
> # 1 "./include/vdso/bits.h" 1
> # 7 "./include/linux/bits.h" 2
> # 1 "./include/uapi/linux/bits.h" 1
> # 8 "./include/linux/bits.h" 2
> # 1 "./arch/x86/include/uapi/asm/bitsperlong.h" 1
> # 11 "./arch/x86/include/uapi/asm/bitsperlong.h"
> # 1 "./include/asm-generic/bitsperlong.h" 1
> 
> 
> 
> 
> # 1 "./include/uapi/asm-generic/bitsperlong.h" 1
> # 6 "./include/asm-generic/bitsperlong.h" 2
> # 12 "./arch/x86/include/uapi/asm/bitsperlong.h" 2
> # 9 "./include/linux/bits.h" 2
> # 22 "./include/linux/bits.h"
> # 1 "./include/linux/build_bug.h" 1
99,117c131
< # 12 "./include/uapi/asm-generic/int-ll64.h"
< # 1 "./arch/x86/include/uapi/asm/bitsperlong.h" 1
< # 11 "./arch/x86/include/uapi/asm/bitsperlong.h"
< # 1 "./include/asm-generic/bitsperlong.h" 1
< 
< 
< 
< 
< # 1 "./include/uapi/asm-generic/bitsperlong.h" 1
< # 6 "./include/asm-generic/bitsperlong.h" 2
< # 12 "./arch/x86/include/uapi/asm/bitsperlong.h" 2
< # 13 "./include/uapi/asm-generic/int-ll64.h" 2
< 
< 
< 
< 
< 
< 
< 
---
> # 20 "./include/uapi/asm-generic/int-ll64.h"
534c548,558
< # 6 "./include/linux/err.h" 2
---
> # 6 "./include/linux/build_bug.h" 2
> # 23 "./include/linux/bits.h" 2
> # 14 "drivers/tty/serial/sc16is7xx.c" 2
> # 1 "./include/linux/clk.h" 1
> # 12 "./include/linux/clk.h"
> # 1 "./include/linux/err.h" 1
> 
> 
> 
> 
> 
608,624d631
< 
< 
< 
< 
< # 1 "./include/linux/const.h" 1
< 
< 
< 
< # 1 "./include/vdso/const.h" 1
< 
< 
< 
< 
< # 1 "./include/uapi/linux/const.h" 1
< # 6 "./include/vdso/const.h" 2
< # 5 "./include/linux/const.h" 2
< # 6 "./include/linux/align.h" 2
706,711d712
< 
< 
< 
< 
< # 1 "./include/linux/build_bug.h" 1
< # 6 "./include/linux/container_of.h" 2
720,721d720
< # 1 "./include/linux/bits.h" 1
< 
723,730d721
< 
< 
< 
< # 1 "./include/vdso/bits.h" 1
< # 7 "./include/linux/bits.h" 2
< # 1 "./include/uapi/linux/bits.h" 1
< # 8 "./include/linux/bits.h" 2
< # 7 "./include/linux/bitops.h" 2
21426c21417
< # 14 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 15 "drivers/tty/serial/sc16is7xx.c" 2
21491c21482
< # 15 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 16 "drivers/tty/serial/sc16is7xx.c" 2
48375c48366
< # 16 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 17 "drivers/tty/serial/sc16is7xx.c" 2
53866c53857
< # 18 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 19 "drivers/tty/serial/sc16is7xx.c" 2
54000c53991
< # 20 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 21 "drivers/tty/serial/sc16is7xx.c" 2
54680c54671
< # 24 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 25 "drivers/tty/serial/sc16is7xx.c" 2
62770c62761
< # 26 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 27 "drivers/tty/serial/sc16is7xx.c" 2
62833c62824
< # 30 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 31 "drivers/tty/serial/sc16is7xx.c" 2
62904c62895
< # 32 "drivers/tty/serial/sc16is7xx.c" 2
---
> # 33 "drivers/tty/serial/sc16is7xx.c" 2
62934,62935c62925,62926
< # 34 "drivers/tty/serial/sc16is7xx.c" 2
< # 315 "drivers/tty/serial/sc16is7xx.c"
---
> # 35 "drivers/tty/serial/sc16is7xx.c" 2
> # 320 "drivers/tty/serial/sc16is7xx.c"
63029,63030c63020,63021
<          (1 << 4),
<          on ? 0 : (1 << 4));
---
>          ((((1UL))) << (4)),
>          on ? 0 : ((((1UL))) << (4)));
63032c63023
< # 426 "drivers/tty/serial/sc16is7xx.c"
---
> # 431 "drivers/tty/serial/sc16is7xx.c"
63069c63060
<  one->config.flags |= (1 << 1);
---
>  one->config.flags |= ((((1UL))) << (1));
63082c63073
<  one->config.flags |= (1 << 1);
---
>  one->config.flags |= ((((1UL))) << (1));
63090c63081
<  sc16is7xx_ier_clear(port, (1 << 1));
---
>  sc16is7xx_ier_clear(port, ((((1UL))) << (1)));
63095c63086
<  sc16is7xx_ier_clear(port, (1 << 0));
---
>  sc16is7xx_ier_clear(port, ((((1UL))) << (0)));
63164c63155
< # 570 "drivers/tty/serial/sc16is7xx.c"
---
> # 575 "drivers/tty/serial/sc16is7xx.c"
63180,63181c63171,63172
<          (1 << 4),
<          (1 << 4));
---
>          ((((1UL))) << (4)),
>          ((((1UL))) << (4)));
63186,63187c63177,63178
<          (1 << 7),
<          prescaler == 1 ? 0 : (1 << 7));
---
>          ((((1UL))) << (7)),
>          prescaler == 1 ? 0 : ((((1UL))) << (7)));
63192c63183
<         (1 << 7));
---
>         ((((1UL))) << (7)));
63227c63218
<    if (!(lsr & (1 << 7)))
---
>    if (!(lsr & ((((1UL))) << (7))))
63240c63231
<   lsr &= 0x1E;
---
>   lsr &= (((((1UL))) << (1)) | ((((1UL))) << (2)) | ((((1UL))) << (3)) | ((((1UL))) << (4)));
63246c63237
<    if (lsr & (1 << 4)) {
---
>    if (lsr & ((((1UL))) << (4))) {
63250c63241
<    } else if (lsr & (1 << 2))
---
>    } else if (lsr & ((((1UL))) << (2)))
63252c63243
<    else if (lsr & (1 << 3))
---
>    else if (lsr & ((((1UL))) << (3)))
63254c63245
<    else if (lsr & (1 << 1))
---
>    else if (lsr & ((((1UL))) << (1)))
63258c63249
<    if (lsr & (1 << 4))
---
>    if (lsr & ((((1UL))) << (4)))
63260c63251
<    else if (lsr & (1 << 2))
---
>    else if (lsr & ((((1UL))) << (2)))
63262c63253
<    else if (lsr & (1 << 3))
---
>    else if (lsr & ((((1UL))) << (3)))
63264c63255
<    else if (lsr & (1 << 1))
---
>    else if (lsr & ((((1UL))) << (1)))
63276c63267
<    uart_insert_char(port, lsr, (1 << 1), ch,
---
>    uart_insert_char(port, lsr, ((((1UL))) << (1)), ch,
63325c63316
<   sc16is7xx_ier_set(port, (1 << 1));
---
>   sc16is7xx_ier_set(port, ((((1UL))) << (1)));
63334,63337c63325,63328
<  mctrl |= (msr & (1 << 4)) ? 0x020 : 0;
<  mctrl |= (msr & (1 << 5)) ? 0x100 : 0;
<  mctrl |= (msr & (1 << 7)) ? 0x040 : 0;
<  mctrl |= (msr & (1 << 6)) ? 0x080 : 0;
---
>  mctrl |= (msr & ((((1UL))) << (4))) ? 0x020 : 0;
>  mctrl |= (msr & ((((1UL))) << (5))) ? 0x100 : 0;
>  mctrl |= (msr & ((((1UL))) << (7))) ? 0x040 : 0;
>  mctrl |= (msr & ((((1UL))) << (6))) ? 0x080 : 0;
63381c63372
<  if (iir & (1 << 0)) {
---
>  if (iir & 0x01) {
63386c63377
<  iir &= 0x3e;
---
>  iir &= ((((int)(sizeof(struct { int:(-!!(__builtin_choose_expr( (sizeof(int) == sizeof(*(8 ? ((void *)((long)((1) > (5)) * 0l)) : (int *)8))), (1) > (5), 0))); })))) + (((~((0UL))) - (((1UL)) << (1)) + 1) & (~((0UL)) >> (64 - 1 - (5)))));
63394c63385
< # 808 "drivers/tty/serial/sc16is7xx.c"
---
> # 813 "drivers/tty/serial/sc16is7xx.c"
63456,63457c63447,63448
<  const u32 mask = (1 << 4) |
<     (1 << 5);
---
>  const u32 mask = ((((1UL))) << (4)) |
>     ((((1UL))) << (5));
63464c63455
<   efcr |= (1 << 4);
---
>   efcr |= ((((1UL))) << (4));
63467c63458
<    efcr |= (1 << 5);
---
>    efcr |= ((((1UL))) << (5));
63485c63476
<  if (config.flags & (1 << 0)) {
---
>  if (config.flags & ((((1UL))) << (0))) {
63490c63481
<    mcr |= (1 << 1);
---
>    mcr |= ((((1UL))) << (1));
63493c63484
<    mcr |= (1 << 0);
---
>    mcr |= ((((1UL))) << (0));
63496c63487
<    mcr |= (1 << 4);
---
>    mcr |= ((((1UL))) << (4));
63498,63500c63489,63491
<           (1 << 1) |
<           (1 << 0) |
<           (1 << 4),
---
>           ((((1UL))) << (1)) |
>           ((((1UL))) << (0)) |
>           ((((1UL))) << (4)),
63504c63495
<  if (config.flags & (1 << 1))
---
>  if (config.flags & ((((1UL))) << (1)))
63508c63499
<  if (config.flags & (1 << 2))
---
>  if (config.flags & ((((1UL))) << (2)))
63554c63545
<  sc16is7xx_ier_clear(port, (1 << 0));
---
>  sc16is7xx_ier_clear(port, ((((1UL))) << (0)));
63563c63554
<  sc16is7xx_ier_set(port, (1 << 0));
---
>  sc16is7xx_ier_set(port, ((((1UL))) << (0)));
63573c63564
<  return (lsr & (1 << 6)) ? 0x01 : 0;
---
>  return (lsr & ((((1UL))) << (6))) ? 0x01 : 0;
63589c63580
<  one->config.flags |= (1 << 0);
---
>  one->config.flags |= ((((1UL))) << (0));
63596,63597c63587,63588
<          (1 << 6),
<          break_state ? (1 << 6) : 0);
---
>          ((((1UL))) << (6)),
>          break_state ? ((((1UL))) << (6)) : 0);
63637c63628
<   lcr |= (1 << 3);
---
>   lcr |= ((((1UL))) << (3));
63639c63630
<    lcr |= (1 << 4);
---
>    lcr |= ((((1UL))) << (4));
63644c63635
<   lcr |= (1 << 2);
---
>   lcr |= ((((1UL))) << (2));
63647c63638
<  port->read_status_mask = (1 << 1);
---
>  port->read_status_mask = ((((1UL))) << (1));
63649,63650c63640,63641
<   port->read_status_mask |= (1 << 2) |
<        (1 << 3);
---
>   port->read_status_mask |= ((((1UL))) << (2)) |
>        ((((1UL))) << (3));
63652c63643
<   port->read_status_mask |= (1 << 4);
---
>   port->read_status_mask |= ((((1UL))) << (4));
63657c63648
<   port->ignore_status_mask |= (1 << 4);
---
>   port->ignore_status_mask |= ((((1UL))) << (4));
63659c63650
<   port->ignore_status_mask |= 0x1E;
---
>   port->ignore_status_mask |= (((((1UL))) << (1)) | ((((1UL))) << (2)) | ((((1UL))) << (3)) | ((((1UL))) << (4)));
63664,63665c63655,63656
<   flow |= (1 << 7) |
<    (1 << 6);
---
>   flow |= ((((1UL))) << (7)) |
>    ((((1UL))) << (6));
63669c63660
<   flow |= (1 << 3);
---
>   flow |= ((((1UL))) << (3));
63671c63662
<   flow |= (1 << 1);
---
>   flow |= ((((1UL))) << (1));
63681c63672
<          ((1 << 6) | (1 << 7) | (1 << 5) | (1 << 3) | (1 << 2) | (1 << 1) | (1 << 0)), flow);
---
>          (((((1UL))) << (6)) | ((((1UL))) << (7)) | ((((1UL))) << (5)) | ((((1UL))) << (3)) | ((((1UL))) << (2)) | ((((1UL))) << (1)) | ((((1UL))) << (0))), flow);
63719c63710
<  one->config.flags |= (1 << 2);
---
>  one->config.flags |= ((((1UL))) << (2));
63734c63725
<  val = (1 << 1) | (1 << 2);
---
>  val = ((((1UL))) << (1)) | ((((1UL))) << (2));
63738c63729
<         (1 << 0));
---
>         ((((1UL))) << (0)));
63748,63749c63739,63740
<          (1 << 4),
<          (1 << 4));
---
>          ((((1UL))) << (4)),
>          ((((1UL))) << (4)));
63753,63754c63744,63745
<          (1 << 2),
<          (1 << 2));
---
>          ((((1UL))) << (2)),
>          ((((1UL))) << (2)));
63770c63761
<          (1 << 6),
---
>          ((((1UL))) << (6)),
63772c63763
<     (1 << 6) : 0);
---
>     ((((1UL))) << (6)) : 0);
63776,63777c63767,63768
<          (1 << 1) |
<          (1 << 2),
---
>          ((((1UL))) << (1)) |
>          ((((1UL))) << (2)),
63781,63782c63772,63773
<  val = (1 << 0) | (1 << 7) |
<        (1 << 3);
---
>  val = ((((1UL))) << (0)) | ((((1UL))) << (7)) |
>        ((((1UL))) << (3));
63804,63807c63795,63798
<          (1 << 1) |
<          (1 << 2),
<          (1 << 1) |
<          (1 << 2));
---
>          ((((1UL))) << (1)) |
>          ((((1UL))) << (2)),
>          ((((1UL))) << (1)) |
>          ((((1UL))) << (2)));
63876c63867
< # 1400 "drivers/tty/serial/sc16is7xx.c"
---
> # 1405 "drivers/tty/serial/sc16is7xx.c"
63926c63917
<    s->mctrl_mask |= (1 << 1);
---
>    s->mctrl_mask |= ((((1UL))) << (1));
63928c63919
<    s->mctrl_mask |= (1 << 2);
---
>    s->mctrl_mask |= ((((1UL))) << (2));
63935,63936c63926,63927
<    (1 << 1) |
<    (1 << 2), s->mctrl_mask);
---
>    ((((1UL))) << (1)) |
>    ((((1UL))) << (2)), s->mctrl_mask);
63960c63951
< # 1493 "drivers/tty/serial/sc16is7xx.c"
---
> # 1498 "drivers/tty/serial/sc16is7xx.c"
64009c64000
<        (1 << 3));
---
>        ((((1UL))) << (3)));
64056,64057c64047,64048
<          (1 << 1) |
<          (1 << 2));
---
>          ((((1UL))) << (1)) |
>          ((((1UL))) << (2)));
64079c64070
<          (1 << 4));
---
>          ((((1UL))) << (4)));
64095c64086
< # 1640 "drivers/tty/serial/sc16is7xx.c"
---
> # 1645 "drivers/tty/serial/sc16is7xx.c"
64188c64179
<   ({ int __ret_warn_on = !!(true); if (__builtin_expect(!!(__ret_warn_on), 0)) do { __auto_type __flags = (1 << 0)|(((9) << 8)); ({ asm volatile("406" ": nop\n\t" ".pushsection .discard.instr_begin\n\t" ".long " "406" "b - .\n\t" ".popsection\n\t" : : "i" (406)); }); do { asm __inline volatile("1:\t" ".byte 0x0f, 0x0b" "\n" ".pushsection __bug_table,\"aw\"\n" "2:\t" ".long " "1b" " - ." "\t# bug_entry::bug_addr\n" "\t" ".long " "%c0" " - ." "\t# bug_entry::file\n" "\t.word %c1" "\t# bug_entry::line\n" "\t.word %c2" "\t# bug_entry::flags\n" "\t.org 2b+%c3\n" ".popsection\n" "998:\n\t" ".pushsection .discard.reachable\n\t" ".long 998b\n\t" ".popsection\n\t" : : "i" ("drivers/tty/serial/sc16is7xx.c"), "i" (1732), "i" (__flags), "i" (sizeof(struct bug_entry))); } while (0); ({ asm volatile("407" ": nop\n\t" ".pushsection .discard.instr_end\n\t" ".long " "407" "b - .\n\t" ".popsection\n\t" : : "i" (407)); }); } while (0); __builtin_expect(!!(__ret_warn_on), 0); });
---
>   ({ int __ret_warn_on = !!(true); if (__builtin_expect(!!(__ret_warn_on), 0)) do { __auto_type __flags = (1 << 0)|(((9) << 8)); ({ asm volatile("406" ": nop\n\t" ".pushsection .discard.instr_begin\n\t" ".long " "406" "b - .\n\t" ".popsection\n\t" : : "i" (406)); }); do { asm __inline volatile("1:\t" ".byte 0x0f, 0x0b" "\n" ".pushsection __bug_table,\"aw\"\n" "2:\t" ".long " "1b" " - ." "\t# bug_entry::bug_addr\n" "\t" ".long " "%c0" " - ." "\t# bug_entry::file\n" "\t.word %c1" "\t# bug_entry::line\n" "\t.word %c2" "\t# bug_entry::flags\n" "\t.org 2b+%c3\n" ".popsection\n" "998:\n\t" ".pushsection .discard.reachable\n\t" ".long 998b\n\t" ".popsection\n\t" : : "i" ("drivers/tty/serial/sc16is7xx.c"), "i" (1737), "i" (__flags), "i" (sizeof(struct bug_entry))); } while (0); ({ asm volatile("407" ": nop\n\t" ".pushsection .discard.instr_end\n\t" ".long " "407" "b - .\n\t" ".popsection\n\t" : : "i" (407)); }); } while (0); __builtin_expect(!!(__ret_warn_on), 0); });
64205c64196
< static void * __attribute__((__used__)) __attribute__((__section__(".discard.addressable"))) __UNIQUE_ID___addressable_sc16is7xx_init411 = (void *)(uintptr_t)&sc16is7xx_init; asm(".section	\"" ".initcall6" ".init" "\", \"a\"		\n" "__initcall__kmod_sc16is7xx__410_1749_sc16is7xx_init6" ":			\n" ".long	" "sc16is7xx_init" " - .	\n" ".previous					\n"); _Static_assert(__builtin_types_compatible_p(typeof(initcall_t), typeof(&sc16is7xx_init)), "__same_type(initcall_t, &sc16is7xx_init)");;;
---
> static void * __attribute__((__used__)) __attribute__((__section__(".discard.addressable"))) __UNIQUE_ID___addressable_sc16is7xx_init411 = (void *)(uintptr_t)&sc16is7xx_init; asm(".section	\"" ".initcall6" ".init" "\", \"a\"		\n" "__initcall__kmod_sc16is7xx__410_1754_sc16is7xx_init6" ":			\n" ".long	" "sc16is7xx_init" " - .	\n" ".previous					\n"); _Static_assert(__builtin_types_compatible_p(typeof(initcall_t), typeof(&sc16is7xx_init)), "__same_type(initcall_t, &sc16is7xx_init)");;;


-- 
Pozdrawiam/With kind regards,
Lech Perczak

Sr. Software Engineer
Camlin Technologies Poland Limited Sp. z o.o.

Powered by blists - more mailing lists