| Message ID | 20241229101234.2896-6-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i3c: introduce and use generic parity helper | expand |
Hi Wolfram, On Sun, Dec 29, 2024 at 11:13 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > The kernel has now a generic helper for getting parity with easier to > understand semantics. Make use of it. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for your patch! > --- a/drivers/i3c/master/i3c-master-cdns.c > +++ b/drivers/i3c/master/i3c-master-cdns.c > @@ -889,8 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr) > ret |= (addr & GENMASK(9, 7)) << 6; > > /* RR0[0] = ~XOR(addr[6:0]) */ > - if (!(hweight8(addr & 0x7f) & 1)) > - ret |= 1; > + ret |= parity8(addr & 0x7f) ? 0 : BIT(0); Perhaps keep the if()-construct, to better match the example in the documentation in 1/5? > > return ret; > } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Sun, 29 Dec 2024 11:49:55 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Wolfram, > > On Sun, Dec 29, 2024 at 11:13 AM Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: > > The kernel has now a generic helper for getting parity with easier to > > understand semantics. Make use of it. > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Thanks for your patch! > > > --- a/drivers/i3c/master/i3c-master-cdns.c > > +++ b/drivers/i3c/master/i3c-master-cdns.c > > @@ -889,8 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr) > > ret |= (addr & GENMASK(9, 7)) << 6; > > > > /* RR0[0] = ~XOR(addr[6:0]) */ > > - if (!(hweight8(addr & 0x7f) & 1)) > > - ret |= 1; > > + ret |= parity8(addr & 0x7f) ? 0 : BIT(0); > > Perhaps keep the if()-construct, to better match the example in the > documentation in 1/5? That line is hard to read, with parity8() returning 1 for 'odd' it could be: ret |= parity8(addr & 0x7f) ^ 1; But: if (!parity8(addr & 0x7f)) ret |= 1; is probably easier to read. But I'd change the name to parity8_odd() for clarity. (or _even and return 0x80 for even) David > > > > > return ret; > > } > > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
> > /* RR0[0] = ~XOR(addr[6:0]) */ > > - if (!(hweight8(addr & 0x7f) & 1)) > > - ret |= 1; > > + ret |= parity8(addr & 0x7f) ? 0 : BIT(0); > > Perhaps keep the if()-construct, to better match the example in the > documentation in 1/5? Can do, don't care super much. I chose this construct because the other drivers in I3C use the above pattern. You still like the if() better?
Hi Wolfram, On Thu, Jan 2, 2025 at 10:04 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > /* RR0[0] = ~XOR(addr[6:0]) */ > > > - if (!(hweight8(addr & 0x7f) & 1)) > > > - ret |= 1; > > > + ret |= parity8(addr & 0x7f) ? 0 : BIT(0); > > > > Perhaps keep the if()-construct, to better match the example in the > > documentation in 1/5? > > Can do, don't care super much. I chose this construct because the other > drivers in I3C use the above pattern. You still like the if() better? Let's add more bike-shedding in 2025 ;-) There's also a general dislike for the ternary operator (BTW, I do agree it has its uses), especially if one of the paths is a no-op, like ORing with zero. Gr{oetje,eeting}s, Geert
> Let's add more bike-shedding in 2025 ;-) Yaaay... :/ > There's also a general dislike for the ternary operator (BTW, I do > agree it has its uses), especially if one of the paths is a no-op, > like ORing with zero. Well, let's see what Alexandre's view is on this. I'll switch to that.
diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c index 06c0592487d3..fedbe6624a1c 100644 --- a/drivers/i3c/master/i3c-master-cdns.c +++ b/drivers/i3c/master/i3c-master-cdns.c @@ -889,8 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr) ret |= (addr & GENMASK(9, 7)) << 6; /* RR0[0] = ~XOR(addr[6:0]) */ - if (!(hweight8(addr & 0x7f) & 1)) - ret |= 1; + ret |= parity8(addr & 0x7f) ? 0 : BIT(0); return ret; }
The kernel has now a generic helper for getting parity with easier to understand semantics. Make use of it. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Changes since v1: * renamed from 'get_parity8' to 'parity8' * rebased to 6.13-rc4 drivers/i3c/master/i3c-master-cdns.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)