[PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[Kuan-Wei Chiu]

Several parts of the kernel contain open-coded and redundant
implementations of parity calculation. This patch series introduces
a unified helper, parity_odd(), to simplify and standardize these
cases.

The first patch renames parity8() to parity_odd(), changes its argument
type from u8 to u64 for broader applicability, and updates its return
type from int to bool to make its usage and return semantics more
intuitive-returning true for odd parity and false for even parity. It
also adds __attribute_const__ to enable compiler optimizations.

While more efficient implementations may exist, further optimization is
postponed until a use case in performance-critical paths arises.

Subsequent patches refactor various kernel components to replace
open-coded parity logic with the new helper, reducing code duplication
and improving consistency.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---

To H. Peter:
I understand your preference for a parity8/16/32/64() style interface,
and I agree that such a design would better accommodate potential
arch-specific implementations. However, I suspect there are very few,
if any, users who care about the performance of parity calculations
enough to warrant such optimizations. So my inclination is to defer any
arch-specific or optimized implementations until we see parity_odd()
being used in hot paths.

Changes in v4:
- Rename parity8() to parity_odd().
- Change the argument type from u8 to u64.
- Use a single parity_odd() function.

Changes in v3:
- Avoid using __builtin_parity.
- Change return type to bool.
- Drop parity() macro.
- Change parityXX() << y to !!parityXX() << y.

Changes in v2:
- Provide fallback functions for __builtin_parity() when the compiler
  decides not to inline it
- Use __builtin_parity() when no architecture-specific implementation
  is available
- Optimize for constant folding when val is a compile-time constant
- Add a generic parity() macro
- Drop the x86 bootflag conversion patch since it has been merged into
  the tip tree

v3: https://lore.kernel.org/lkml/20250306162541.2633025-1-visitorckw@gmail.com/
v1: https://lore.kernel.org/lkml/20250223164217.2139331-1-visitorckw@gmail.com/
v2: https://lore.kernel.org/lkml/20250301142409.2513835-1-visitorckw@gmail.com/

Kuan-Wei Chiu (13):
  bitops: Change parity8() to parity_odd() with u64 input and bool
    return type
  media: media/test_drivers: Replace open-coded parity calculation with
    parity_odd()
  media: pci: cx18-av-vbi: Replace open-coded parity calculation with
    parity_odd()
  media: saa7115: Replace open-coded parity calculation with
    parity_odd()
  serial: max3100: Replace open-coded parity calculation with
    parity_odd()
  lib/bch: Replace open-coded parity calculation with parity_odd()
  Input: joystick - Replace open-coded parity calculation with
    parity_odd()
  net: ethernet: oa_tc6: Replace open-coded parity calculation with
    parity_odd()
  wifi: brcm80211: Replace open-coded parity calculation with
    parity_odd()
  drm/bridge: dw-hdmi: Replace open-coded parity calculation with
    parity_odd()
  mtd: ssfdc: Replace open-coded parity calculation with parity_odd()
  fsi: i2cr: Replace open-coded parity calculation with parity_odd()
  nfp: bpf: Replace open-coded parity calculation with parity_odd()

 arch/x86/kernel/bootflag.c                    |  4 +--
 drivers/fsi/fsi-master-i2cr.c                 | 20 +++------------
 .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c   |  8 ++----
 drivers/hwmon/spd5118.c                       |  2 +-
 drivers/i3c/master/dw-i3c-master.c            |  2 +-
 drivers/i3c/master/i3c-master-cdns.c          |  2 +-
 drivers/i3c/master/mipi-i3c-hci/dat_v1.c      |  2 +-
 drivers/input/joystick/grip_mp.c              | 17 ++-----------
 drivers/input/joystick/sidewinder.c           | 25 ++++---------------
 drivers/media/i2c/saa7115.c                   | 12 ++-------
 drivers/media/pci/cx18/cx18-av-vbi.c          | 12 ++-------
 .../media/test-drivers/vivid/vivid-vbi-gen.c  |  8 ++----
 drivers/mtd/ssfdc.c                           | 20 +++------------
 drivers/net/ethernet/netronome/nfp/nfp_asm.c  |  7 +-----
 drivers/net/ethernet/oa_tc6.c                 | 19 +++-----------
 .../broadcom/brcm80211/brcmsmac/dma.c         | 18 ++-----------
 drivers/tty/serial/max3100.c                  |  3 ++-
 include/linux/bitops.h                        | 19 ++++++++------
 lib/bch.c                                     | 14 +----------
 19 files changed, 49 insertions(+), 165 deletions(-)

-- 
2.34.1

[PATCH v4 01/13] bitops: Change parity8() to parity_odd() with u64 input and bool return type

[Kuan-Wei Chiu]

Redesign the parity8() helper as parity_odd(), changing its input type
from u8 to u64 to support broader use cases and its return type from
int to bool to clearly reflect the function's binary output. The
function now returns true for odd parity and false for even parity,
making its behavior more intuitive based on the name.

Also mark the function with __attribute_const__ to enable better
compiler optimization, as the result depends solely on its input and
has no side effects.

While more efficient implementations may exist, further optimization is
postponed until a use case in performance-critical paths arises.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 arch/x86/kernel/bootflag.c               |  4 ++--
 drivers/hwmon/spd5118.c                  |  2 +-
 drivers/i3c/master/dw-i3c-master.c       |  2 +-
 drivers/i3c/master/i3c-master-cdns.c     |  2 +-
 drivers/i3c/master/mipi-i3c-hci/dat_v1.c |  2 +-
 include/linux/bitops.h                   | 19 ++++++++++++-------
 6 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
index 73274d76ce16..86aae4b2bfd5 100644
--- a/arch/x86/kernel/bootflag.c
+++ b/arch/x86/kernel/bootflag.c
@@ -26,7 +26,7 @@ static void __init sbf_write(u8 v)
 	unsigned long flags;
 
 	if (sbf_port != -1) {
-		if (!parity8(v))
+		if (!parity_odd(v))
 			v ^= SBF_PARITY;
 
 		printk(KERN_INFO "Simple Boot Flag at 0x%x set to 0x%x\n",
@@ -57,7 +57,7 @@ static bool __init sbf_value_valid(u8 v)
 {
 	if (v & SBF_RESERVED)		/* Reserved bits */
 		return false;
-	if (!parity8(v))
+	if (!parity_odd(v))
 		return false;
 
 	return true;
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 358152868d96..15761f2ca4e9 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -298,7 +298,7 @@ static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types typ
  */
 static bool spd5118_vendor_valid(u8 bank, u8 id)
 {
-	if (parity8(bank) == 0 || parity8(id) == 0)
+	if (!parity_odd(bank) || !parity_odd(id))
 		return false;
 
 	id &= 0x7f;
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 611c22b72c15..dc61d87fcd94 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -867,7 +867,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 		master->devs[pos].addr = ret;
 		last_addr = ret;
 
-		ret |= parity8(ret) ? 0 : BIT(7);
+		ret |= parity_odd(ret) ? 0 : BIT(7);
 
 		writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(ret),
 		       master->regs +
diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
index fd3752cea654..df14f978a388 100644
--- a/drivers/i3c/master/i3c-master-cdns.c
+++ b/drivers/i3c/master/i3c-master-cdns.c
@@ -889,7 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr)
 	ret |= (addr & GENMASK(9, 7)) << 6;
 
 	/* RR0[0] = ~XOR(addr[6:0]) */
-	ret |= parity8(addr & 0x7f) ? 0 : BIT(0);
+	ret |= parity_odd(addr & 0x7f) ? 0 : BIT(0);
 
 	return ret;
 }
diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
index 85c4916972e4..d692a299607d 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
@@ -114,7 +114,7 @@ static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
 	dat_w0 = dat_w0_read(dat_idx);
 	dat_w0 &= ~(DAT_0_DYNAMIC_ADDRESS | DAT_0_DYNADDR_PARITY);
 	dat_w0 |= FIELD_PREP(DAT_0_DYNAMIC_ADDRESS, address) |
-		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);
+		  (parity_odd(address) ? 0 : DAT_0_DYNADDR_PARITY);
 	dat_w0_write(dat_idx, dat_w0);
 }
 
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index c1cb53cf2f0f..7c4c8afccef1 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -230,35 +230,40 @@ static inline int get_count_order_long(unsigned long l)
 }
 
 /**
- * parity8 - get the parity of an u8 value
- * @value: the value to be examined
+ * parity_odd - get the parity of an u64 value
+ * @val: the value to be examined
  *
- * Determine the parity of the u8 argument.
+ * Determine the parity of the u64 argument.
  *
  * Returns:
- * 0 for even parity, 1 for odd parity
+ * false for even parity, true for odd parity
  *
  * Note: This function informs you about the current parity. Example to bail
  * out when parity is odd:
  *
- *	if (parity8(val) == 1)
+ *	if (parity_odd(val))
  *		return -EBADMSG;
  *
  * If you need to calculate a parity bit, you need to draw the conclusion from
  * this result yourself. Example to enforce odd parity, parity bit is bit 7:
  *
- *	if (parity8(val) == 0)
+ *	if (!parity_odd(val))
  *		val ^= BIT(7);
  */
-static inline int parity8(u8 val)
+#ifndef parity_odd
+static inline __attribute_const__ bool parity_odd(u64 val)
 {
 	/*
 	 * One explanation of this algorithm:
 	 * https://funloop.org/codex/problem/parity/README.html
 	 */
+	val ^= val >> 32;
+	val ^= val >> 16;
+	val ^= val >> 8;
 	val ^= val >> 4;
 	return (0x6996 >> (val & 0xf)) & 1;
 }
+#endif
 
 /**
  * __ffs64 - find first set bit in a 64 bit word
-- 
2.34.1

[PATCH v4 02/13] media: media/test_drivers: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
index 70a4024d461e..5e1b7b1742e4 100644
--- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
+++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
@@ -5,6 +5,7 @@
  * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
  */
 
+#include <linux/bitops.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/ktime.h>
@@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = {
 
 static u8 calc_parity(u8 val)
 {
-	unsigned i;
-	unsigned tot = 0;
-
-	for (i = 0; i < 7; i++)
-		tot += (val & (1 << i)) ? 1 : 0;
-	return val | ((tot & 1) ? 0 : 0x80);
+	return val | (parity_odd(val) ? 0 : 0x80);
 }
 
 static void vivid_vbi_gen_set_time_of_day(u8 *packet)
-- 
2.34.1

[PATCH v4 03/13] media: pci: cx18-av-vbi: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/media/pci/cx18/cx18-av-vbi.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/media/pci/cx18/cx18-av-vbi.c b/drivers/media/pci/cx18/cx18-av-vbi.c
index 65281d40c681..15b515b95956 100644
--- a/drivers/media/pci/cx18/cx18-av-vbi.c
+++ b/drivers/media/pci/cx18/cx18-av-vbi.c
@@ -8,6 +8,7 @@
  */
 
 
+#include <linux/bitops.h>
 #include "cx18-driver.h"
 
 /*
@@ -56,15 +57,6 @@ struct vbi_anc_data {
 	/* u8 fill[]; Variable number of fill bytes */
 };
 
-static int odd_parity(u8 c)
-{
-	c ^= (c >> 4);
-	c ^= (c >> 2);
-	c ^= (c >> 1);
-
-	return c & 1;
-}
-
 static int decode_vps(u8 *dst, u8 *p)
 {
 	static const u8 biphase_tbl[] = {
@@ -278,7 +270,7 @@ int cx18_av_decode_vbi_line(struct v4l2_subdev *sd,
 		break;
 	case 6:
 		sdid = V4L2_SLICED_CAPTION_525;
-		err = !odd_parity(p[0]) || !odd_parity(p[1]);
+		err = !parity_odd(p[0]) || !parity_odd(p[1]);
 		break;
 	case 9:
 		sdid = V4L2_SLICED_VPS;
-- 
2.34.1

[PATCH v4 04/13] media: saa7115: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/media/i2c/saa7115.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
index a1c71187e773..a7886269dcfc 100644
--- a/drivers/media/i2c/saa7115.c
+++ b/drivers/media/i2c/saa7115.c
@@ -25,6 +25,7 @@
 
 #include "saa711x_regs.h"
 
+#include <linux/bitops.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -664,15 +665,6 @@ static const unsigned char saa7115_init_misc[] = {
 	0x00, 0x00
 };
 
-static int saa711x_odd_parity(u8 c)
-{
-	c ^= (c >> 4);
-	c ^= (c >> 2);
-	c ^= (c >> 1);
-
-	return c & 1;
-}
-
 static int saa711x_decode_vps(u8 *dst, u8 *p)
 {
 	static const u8 biphase_tbl[] = {
@@ -1227,7 +1219,7 @@ static int saa711x_decode_vbi_line(struct v4l2_subdev *sd, struct v4l2_decode_vb
 		vbi->type = V4L2_SLICED_TELETEXT_B;
 		break;
 	case 4:
-		if (!saa711x_odd_parity(p[0]) || !saa711x_odd_parity(p[1]))
+		if (!parity_odd(p[0]) || !parity_odd(p[1]))
 			return 0;
 		vbi->type = V4L2_SLICED_CAPTION_525;
 		break;
-- 
2.34.1

[PATCH v4 05/13] serial: max3100: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/tty/serial/max3100.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index f2dd83692b2c..36ed41eef7b1 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -16,6 +16,7 @@
 /* 4 MAX3100s should be enough for everyone */
 #define MAX_MAX3100 4
 
+#include <linux/bitops.h>
 #include <linux/container_of.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -133,7 +134,7 @@ static int max3100_do_parity(struct max3100_port *s, u16 c)
 	else
 		c &= 0xff;
 
-	parity = parity ^ (hweight8(c) & 1);
+	parity = parity ^ parity_odd(c);
 	return parity;
 }
 
-- 
2.34.1

[PATCH v4 06/13] lib/bch: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 lib/bch.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/lib/bch.c b/lib/bch.c
index 1c0cb07cdfeb..3f14d69f373e 100644
--- a/lib/bch.c
+++ b/lib/bch.c
@@ -311,18 +311,6 @@ static inline int deg(unsigned int poly)
 	return fls(poly)-1;
 }
 
-static inline int parity(unsigned int x)
-{
-	/*
-	 * public domain code snippet, lifted from
-	 * http://www-graphics.stanford.edu/~seander/bithacks.html
-	 */
-	x ^= x >> 1;
-	x ^= x >> 2;
-	x = (x & 0x11111111U) * 0x11111111U;
-	return (x >> 28) & 1;
-}
-
 /* Galois field basic operations: multiply, divide, inverse, etc. */
 
 static inline unsigned int gf_mul(struct bch_control *bch, unsigned int a,
@@ -524,7 +512,7 @@ static int solve_linear_system(struct bch_control *bch, unsigned int *rows,
 		tmp = 0;
 		for (r = m-1; r >= 0; r--) {
 			mask = rows[r] & (tmp|1);
-			tmp |= parity(mask) << (m-r);
+			tmp |= parity_odd(mask) << (m-r);
 		}
 		sol[p] = tmp >> 1;
 	}
-- 
2.34.1

[PATCH v4 07/13] Input: joystick - Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/input/joystick/grip_mp.c    | 17 ++---------------
 drivers/input/joystick/sidewinder.c | 25 +++++--------------------
 2 files changed, 7 insertions(+), 35 deletions(-)

diff --git a/drivers/input/joystick/grip_mp.c b/drivers/input/joystick/grip_mp.c
index 5eadb5a3ca37..e7f60a83a89b 100644
--- a/drivers/input/joystick/grip_mp.c
+++ b/drivers/input/joystick/grip_mp.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/proc_fs.h>
 #include <linux/jiffies.h>
+#include <linux/bitops.h>
 
 #define DRIVER_DESC	"Gravis Grip Multiport driver"
 
@@ -112,20 +113,6 @@ static const int axis_map[] = { 5, 9, 1, 5, 6, 10, 2, 6, 4, 8, 0, 4, 5, 9, 1, 5
 
 static int register_slot(int i, struct grip_mp *grip);
 
-/*
- * Returns whether an odd or even number of bits are on in pkt.
- */
-
-static int bit_parity(u32 pkt)
-{
-	int x = pkt ^ (pkt >> 16);
-	x ^= x >> 8;
-	x ^= x >> 4;
-	x ^= x >> 2;
-	x ^= x >> 1;
-	return x & 1;
-}
-
 /*
  * Poll gameport; return true if all bits set in 'onbits' are on and
  * all bits set in 'offbits' are off.
@@ -236,7 +223,7 @@ static int mp_io(struct gameport* gameport, int sendflags, int sendcode, u32 *pa
 		pkt = (pkt >> 2) | 0xf0000000;
 	}
 
-	if (bit_parity(pkt) == 1)
+	if (parity_odd(pkt))
 		return IO_RESET;
 
 	/* Acknowledge packet receipt */
diff --git a/drivers/input/joystick/sidewinder.c b/drivers/input/joystick/sidewinder.c
index 3a5873e5fcb3..fb6f7004a485 100644
--- a/drivers/input/joystick/sidewinder.c
+++ b/drivers/input/joystick/sidewinder.c
@@ -7,6 +7,7 @@
  * Microsoft SideWinder joystick family driver for Linux
  */
 
+#include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -240,22 +241,6 @@ static void sw_init_digital(struct gameport *gameport)
 	local_irq_restore(flags);
 }
 
-/*
- * sw_parity() computes parity of __u64
- */
-
-static int sw_parity(__u64 t)
-{
-	int x = t ^ (t >> 32);
-
-	x ^= x >> 16;
-	x ^= x >> 8;
-	x ^= x >> 4;
-	x ^= x >> 2;
-	x ^= x >> 1;
-	return x & 1;
-}
-
 /*
  * sw_ccheck() checks synchronization bits and computes checksum of nibbles.
  */
@@ -316,7 +301,7 @@ static int sw_parse(unsigned char *buf, struct sw *sw)
 
 			for (i = 0; i < sw->number; i ++) {
 
-				if (sw_parity(GB(i*15,15)))
+				if (parity_odd(GB(i*15,15)))
 					return -1;
 
 				input_report_abs(sw->dev[i], ABS_X, GB(i*15+3,1) - GB(i*15+2,1));
@@ -333,7 +318,7 @@ static int sw_parse(unsigned char *buf, struct sw *sw)
 		case SW_ID_PP:
 		case SW_ID_FFP:
 
-			if (!sw_parity(GB(0,48)) || (hat = GB(42,4)) > 8)
+			if (!parity_odd(GB(0,48)) || (hat = GB(42,4)) > 8)
 				return -1;
 
 			dev = sw->dev[0];
@@ -354,7 +339,7 @@ static int sw_parse(unsigned char *buf, struct sw *sw)
 
 		case SW_ID_FSP:
 
-			if (!sw_parity(GB(0,43)) || (hat = GB(28,4)) > 8)
+			if (!parity_odd(GB(0,43)) || (hat = GB(28,4)) > 8)
 				return -1;
 
 			dev = sw->dev[0];
@@ -379,7 +364,7 @@ static int sw_parse(unsigned char *buf, struct sw *sw)
 
 		case SW_ID_FFW:
 
-			if (!sw_parity(GB(0,33)))
+			if (!parity_odd(GB(0,33)))
 				return -1;
 
 			dev = sw->dev[0];
-- 
2.34.1

[PATCH v4 08/13] net: ethernet: oa_tc6: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/net/ethernet/oa_tc6.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index db200e4ec284..6734b49357e7 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/bitops.h>
 #include <linux/iopoll.h>
 #include <linux/mdio.h>
 #include <linux/phy.h>
@@ -177,19 +178,6 @@ static int oa_tc6_spi_transfer(struct oa_tc6 *tc6,
 	return spi_sync(tc6->spi, &msg);
 }
 
-static int oa_tc6_get_parity(u32 p)
-{
-	/* Public domain code snippet, lifted from
-	 * http://www-graphics.stanford.edu/~seander/bithacks.html
-	 */
-	p ^= p >> 1;
-	p ^= p >> 2;
-	p = (p & 0x11111111U) * 0x11111111U;
-
-	/* Odd parity is used here */
-	return !((p >> 28) & 1);
-}
-
 static __be32 oa_tc6_prepare_ctrl_header(u32 addr, u8 length,
 					 enum oa_tc6_register_op reg_op)
 {
@@ -202,7 +190,7 @@ static __be32 oa_tc6_prepare_ctrl_header(u32 addr, u8 length,
 		 FIELD_PREP(OA_TC6_CTRL_HEADER_ADDR, addr) |
 		 FIELD_PREP(OA_TC6_CTRL_HEADER_LENGTH, length - 1);
 	header |= FIELD_PREP(OA_TC6_CTRL_HEADER_PARITY,
-			     oa_tc6_get_parity(header));
+			     !parity_odd(header));
 
 	return cpu_to_be32(header);
 }
@@ -940,8 +928,7 @@ static __be32 oa_tc6_prepare_data_header(bool data_valid, bool start_valid,
 		     FIELD_PREP(OA_TC6_DATA_HEADER_END_BYTE_OFFSET,
 				end_byte_offset);
 
-	header |= FIELD_PREP(OA_TC6_DATA_HEADER_PARITY,
-			     oa_tc6_get_parity(header));
+	header |= FIELD_PREP(OA_TC6_DATA_HEADER_PARITY, !parity_odd(header));
 
 	return cpu_to_be32(header);
 }
-- 
2.34.1

[PATCH v4 09/13] wifi: brcm80211: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 .../wireless/broadcom/brcm80211/brcmsmac/dma.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
index 80c35027787a..5d7500ee2d3b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/pci.h>
+#include <linux/bitops.h>
 #include <net/cfg80211.h>
 #include <net/mac80211.h>
 
@@ -283,24 +284,9 @@ struct dma_info {
 	bool aligndesc_4k;
 };
 
-/* Check for odd number of 1's */
-static u32 parity32(__le32 data)
-{
-	/* no swap needed for counting 1's */
-	u32 par_data = *(u32 *)&data;
-
-	par_data ^= par_data >> 16;
-	par_data ^= par_data >> 8;
-	par_data ^= par_data >> 4;
-	par_data ^= par_data >> 2;
-	par_data ^= par_data >> 1;
-
-	return par_data & 1;
-}
-
 static bool dma64_dd_parity(struct dma64desc *dd)
 {
-	return parity32(dd->addrlow ^ dd->addrhigh ^ dd->ctrl1 ^ dd->ctrl2);
+	return parity_odd(dd->addrlow ^ dd->addrhigh ^ dd->ctrl1 ^ dd->ctrl2);
 }
 
 /* descriptor bumping functions */
-- 
2.34.1

[PATCH v4 10/13] drm/bridge: dw-hdmi: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index cf1f66b7b192..abf2c72be622 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -4,6 +4,7 @@
  *
  * Written and tested against the Designware HDMI Tx found in iMX6.
  */
+#include <linux/bitops.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
@@ -171,12 +172,7 @@ static void dw_hdmi_reformat_iec958(struct snd_dw_hdmi *dw,
 
 static u32 parity(u32 sample)
 {
-	sample ^= sample >> 16;
-	sample ^= sample >> 8;
-	sample ^= sample >> 4;
-	sample ^= sample >> 2;
-	sample ^= sample >> 1;
-	return (sample & 1) << 27;
+	return parity_odd(sample) << 27;
 }
 
 static void dw_hdmi_reformat_s24(struct snd_dw_hdmi *dw,
-- 
2.34.1

[PATCH v4 11/13] mtd: ssfdc: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/mtd/ssfdc.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/ssfdc.c b/drivers/mtd/ssfdc.c
index 46c01fa2ec46..0db4776424d0 100644
--- a/drivers/mtd/ssfdc.c
+++ b/drivers/mtd/ssfdc.c
@@ -7,6 +7,7 @@
  * Based on NTFL and MTDBLOCK_RO drivers
  */
 
+#include <linux/bitops.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -178,24 +179,11 @@ static int read_raw_oob(struct mtd_info *mtd, loff_t offs, uint8_t *buf)
 	return 0;
 }
 
-/* Parity calculator on a word of n bit size */
-static int get_parity(int number, int size)
-{
- 	int k;
-	int parity;
-
-	parity = 1;
-	for (k = 0; k < size; k++) {
-		parity += (number >> k);
-		parity &= 1;
-	}
-	return parity;
-}
-
 /* Read and validate the logical block address field stored in the OOB */
 static int get_logical_address(uint8_t *oob_buf)
 {
-	int block_address, parity;
+	int block_address;
+	bool parity;
 	int offset[2] = {6, 11}; /* offset of the 2 address fields within OOB */
 	int j;
 	int ok = 0;
@@ -215,7 +203,7 @@ static int get_logical_address(uint8_t *oob_buf)
 			block_address &= 0x7FF;
 			block_address >>= 1;
 
-			if (get_parity(block_address, 10) != parity) {
+			if (parity_odd(block_address & 0x3ff) == parity) {
 				pr_debug("SSFDC_RO: logical address field%d"
 					"parity error(0x%04X)\n", j+1,
 					block_address);
-- 
2.34.1

[PATCH v4 12/13] fsi: i2cr: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/fsi/fsi-master-i2cr.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/fsi/fsi-master-i2cr.c b/drivers/fsi/fsi-master-i2cr.c
index 40f1f4d231e5..692d73e892e3 100644
--- a/drivers/fsi/fsi-master-i2cr.c
+++ b/drivers/fsi/fsi-master-i2cr.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (C) IBM Corporation 2023 */
 
+#include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/fsi.h>
 #include <linux/i2c.h>
@@ -38,27 +39,12 @@ static const u8 i2cr_cfam[] = {
 
 static bool i2cr_check_parity32(u32 v, bool parity)
 {
-	u32 i;
-
-	for (i = 0; i < 32; ++i) {
-		if (v & (1u << i))
-			parity = !parity;
-	}
-
-	return parity;
+	return parity != parity_odd(v);
 }
 
 static bool i2cr_check_parity64(u64 v)
 {
-	u32 i;
-	bool parity = I2CR_INITIAL_PARITY;
-
-	for (i = 0; i < 64; ++i) {
-		if (v & (1llu << i))
-			parity = !parity;
-	}
-
-	return parity;
+	return parity_odd(v) != I2CR_INITIAL_PARITY;
 }
 
 static u32 i2cr_get_command(u32 address, bool parity)
-- 
2.34.1

[PATCH v4 13/13] nfp: bpf: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

Refactor parity calculations to use the standard parity_odd() helper.
This change eliminates redundant implementations.

Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_asm.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_asm.c b/drivers/net/ethernet/netronome/nfp/nfp_asm.c
index 154399c5453f..2f8f78abb6f5 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_asm.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_asm.c
@@ -295,11 +295,6 @@ static const u64 nfp_ustore_ecc_polynomials[NFP_USTORE_ECC_POLY_WORDS] = {
 	0x0daf69a46910ULL,
 };
 
-static bool parity(u64 value)
-{
-	return hweight64(value) & 1;
-}
-
 int nfp_ustore_check_valid_no_ecc(u64 insn)
 {
 	if (insn & ~GENMASK_ULL(NFP_USTORE_OP_BITS, 0))
@@ -314,7 +309,7 @@ u64 nfp_ustore_calc_ecc_insn(u64 insn)
 	int i;
 
 	for (i = 0; i < NFP_USTORE_ECC_POLY_WORDS; i++)
-		ecc |= parity(nfp_ustore_ecc_polynomials[i] & insn) << i;
+		ecc |= parity_odd(nfp_ustore_ecc_polynomials[i] & insn) << i;
 
 	return insn | (u64)ecc << NFP_USTORE_OP_BITS;
 }
-- 
2.34.1

Re: [PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[Yury Norov]

On Wed, Apr 09, 2025 at 11:43:43PM +0800, Kuan-Wei Chiu wrote:
> Several parts of the kernel contain open-coded and redundant
> implementations of parity calculation. This patch series introduces
> a unified helper, parity_odd(), to simplify and standardize these
> cases.
> 
> The first patch renames parity8() to parity_odd(), changes its argument

Alright, if it's an extension of the area of applicability, it should be
renamed to just parity(). I already shared a table that summarized the
drivers authors' view on that, and they clearly prefer not to add the
suffix - 13 vs 2. The __builtin_parity() doesn't care of suffix as well. 

https://lore.kernel.org/all/Z9GtcNJie8TRKywZ@thinkpad/

Yes, the argument that boolean function should explain itself sounds
correct, but in this case, comment on top of the function looks enough
to me.

The existing codebase doesn't care about the suffix as well. If no
strong preference, let's just pick a short and sweet name?

> type from u8 to u64 for broader applicability, and updates its return
> type from int to bool to make its usage and return semantics more
> intuitive-returning true for odd parity and false for even parity. It
> also adds __attribute_const__ to enable compiler optimizations.

That's correct and nice, but can you support it with a bloat-o-meter's
before/after and/or asm snippets? I also think it worth to be a separate
patch, preferably the last patch in the series.

> While more efficient implementations may exist, further optimization is
> postponed until a use case in performance-critical paths arises.
> 
> Subsequent patches refactor various kernel components to replace
> open-coded parity logic with the new helper, reducing code duplication
> and improving consistency.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
> 
> To H. Peter:
> I understand your preference for a parity8/16/32/64() style interface,
> and I agree that such a design would better accommodate potential
> arch-specific implementations. However, I suspect there are very few,
> if any, users who care about the performance of parity calculations
> enough to warrant such optimizations. So my inclination is to defer any
> arch-specific or optimized implementations until we see parity_odd()
> being used in hot paths.
> 
> Changes in v4:
> - Rename parity8() to parity_odd().
> - Change the argument type from u8 to u64.
> - Use a single parity_odd() function.
> 
> Changes in v3:
> - Avoid using __builtin_parity.
> - Change return type to bool.
> - Drop parity() macro.
> - Change parityXX() << y to !!parityXX() << y.
> 
> Changes in v2:
> - Provide fallback functions for __builtin_parity() when the compiler
>   decides not to inline it
> - Use __builtin_parity() when no architecture-specific implementation
>   is available
> - Optimize for constant folding when val is a compile-time constant
> - Add a generic parity() macro
> - Drop the x86 bootflag conversion patch since it has been merged into
>   the tip tree
> 
> v3: https://lore.kernel.org/lkml/20250306162541.2633025-1-visitorckw@gmail.com/
> v1: https://lore.kernel.org/lkml/20250223164217.2139331-1-visitorckw@gmail.com/
> v2: https://lore.kernel.org/lkml/20250301142409.2513835-1-visitorckw@gmail.com/
> 
> Kuan-Wei Chiu (13):
>   bitops: Change parity8() to parity_odd() with u64 input and bool
>     return type
>   media: media/test_drivers: Replace open-coded parity calculation with
>     parity_odd()
>   media: pci: cx18-av-vbi: Replace open-coded parity calculation with
>     parity_odd()
>   media: saa7115: Replace open-coded parity calculation with
>     parity_odd()
>   serial: max3100: Replace open-coded parity calculation with
>     parity_odd()
>   lib/bch: Replace open-coded parity calculation with parity_odd()
>   Input: joystick - Replace open-coded parity calculation with
>     parity_odd()
>   net: ethernet: oa_tc6: Replace open-coded parity calculation with
>     parity_odd()
>   wifi: brcm80211: Replace open-coded parity calculation with
>     parity_odd()
>   drm/bridge: dw-hdmi: Replace open-coded parity calculation with
>     parity_odd()
>   mtd: ssfdc: Replace open-coded parity calculation with parity_odd()
>   fsi: i2cr: Replace open-coded parity calculation with parity_odd()
>   nfp: bpf: Replace open-coded parity calculation with parity_odd()
> 
>  arch/x86/kernel/bootflag.c                    |  4 +--
>  drivers/fsi/fsi-master-i2cr.c                 | 20 +++------------
>  .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c   |  8 ++----
>  drivers/hwmon/spd5118.c                       |  2 +-
>  drivers/i3c/master/dw-i3c-master.c            |  2 +-
>  drivers/i3c/master/i3c-master-cdns.c          |  2 +-
>  drivers/i3c/master/mipi-i3c-hci/dat_v1.c      |  2 +-
>  drivers/input/joystick/grip_mp.c              | 17 ++-----------
>  drivers/input/joystick/sidewinder.c           | 25 ++++---------------
>  drivers/media/i2c/saa7115.c                   | 12 ++-------
>  drivers/media/pci/cx18/cx18-av-vbi.c          | 12 ++-------
>  .../media/test-drivers/vivid/vivid-vbi-gen.c  |  8 ++----
>  drivers/mtd/ssfdc.c                           | 20 +++------------
>  drivers/net/ethernet/netronome/nfp/nfp_asm.c  |  7 +-----
>  drivers/net/ethernet/oa_tc6.c                 | 19 +++-----------
>  .../broadcom/brcm80211/brcmsmac/dma.c         | 18 ++-----------
>  drivers/tty/serial/max3100.c                  |  3 ++-
>  include/linux/bitops.h                        | 19 ++++++++------
>  lib/bch.c                                     | 14 +----------
>  19 files changed, 49 insertions(+), 165 deletions(-)

OK, now it looks like a nice consolidation and simplification of code
base. Thanks for the work.

Thanks,
Yury

Re: [PATCH v4 01/13] bitops: Change parity8() to parity_odd() with u64 input and bool return type

[Yury Norov]

On Wed, Apr 09, 2025 at 11:43:44PM +0800, Kuan-Wei Chiu wrote:
> Redesign the parity8() helper as parity_odd(), changing its input type
> from u8 to u64 to support broader use cases and its return type from
> int to bool to clearly reflect the function's binary output. The
> function now returns true for odd parity and false for even parity,
> making its behavior more intuitive based on the name.
> 
> Also mark the function with __attribute_const__ to enable better
> compiler optimization, as the result depends solely on its input and
> has no side effects.
> 
> While more efficient implementations may exist, further optimization is
> postponed until a use case in performance-critical paths arises.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
>  arch/x86/kernel/bootflag.c               |  4 ++--
>  drivers/hwmon/spd5118.c                  |  2 +-
>  drivers/i3c/master/dw-i3c-master.c       |  2 +-
>  drivers/i3c/master/i3c-master-cdns.c     |  2 +-
>  drivers/i3c/master/mipi-i3c-hci/dat_v1.c |  2 +-
>  include/linux/bitops.h                   | 19 ++++++++++++-------
>  6 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> index 73274d76ce16..86aae4b2bfd5 100644
> --- a/arch/x86/kernel/bootflag.c
> +++ b/arch/x86/kernel/bootflag.c
> @@ -26,7 +26,7 @@ static void __init sbf_write(u8 v)
>  	unsigned long flags;
>  
>  	if (sbf_port != -1) {
> -		if (!parity8(v))
> +		if (!parity_odd(v))
>  			v ^= SBF_PARITY;
>  
>  		printk(KERN_INFO "Simple Boot Flag at 0x%x set to 0x%x\n",
> @@ -57,7 +57,7 @@ static bool __init sbf_value_valid(u8 v)
>  {
>  	if (v & SBF_RESERVED)		/* Reserved bits */
>  		return false;
> -	if (!parity8(v))
> +	if (!parity_odd(v))
>  		return false;
>  
>  	return true;
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 358152868d96..15761f2ca4e9 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -298,7 +298,7 @@ static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types typ
>   */
>  static bool spd5118_vendor_valid(u8 bank, u8 id)
>  {
> -	if (parity8(bank) == 0 || parity8(id) == 0)
> +	if (!parity_odd(bank) || !parity_odd(id))
>  		return false;
>  
>  	id &= 0x7f;
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 611c22b72c15..dc61d87fcd94 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -867,7 +867,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
>  		master->devs[pos].addr = ret;
>  		last_addr = ret;
>  
> -		ret |= parity8(ret) ? 0 : BIT(7);
> +		ret |= parity_odd(ret) ? 0 : BIT(7);
>  
>  		writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(ret),
>  		       master->regs +
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index fd3752cea654..df14f978a388 100644
> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
> @@ -889,7 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr)
>  	ret |= (addr & GENMASK(9, 7)) << 6;
>  
>  	/* RR0[0] = ~XOR(addr[6:0]) */
> -	ret |= parity8(addr & 0x7f) ? 0 : BIT(0);
> +	ret |= parity_odd(addr & 0x7f) ? 0 : BIT(0);
>  
>  	return ret;
>  }
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> index 85c4916972e4..d692a299607d 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> @@ -114,7 +114,7 @@ static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
>  	dat_w0 = dat_w0_read(dat_idx);
>  	dat_w0 &= ~(DAT_0_DYNAMIC_ADDRESS | DAT_0_DYNADDR_PARITY);
>  	dat_w0 |= FIELD_PREP(DAT_0_DYNAMIC_ADDRESS, address) |
> -		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);
> +		  (parity_odd(address) ? 0 : DAT_0_DYNADDR_PARITY);
>  	dat_w0_write(dat_idx, dat_w0);
>  }
>  
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index c1cb53cf2f0f..7c4c8afccef1 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -230,35 +230,40 @@ static inline int get_count_order_long(unsigned long l)
>  }
>  
>  /**
> - * parity8 - get the parity of an u8 value
> - * @value: the value to be examined
> + * parity_odd - get the parity of an u64 value
> + * @val: the value to be examined
>   *
> - * Determine the parity of the u8 argument.
> + * Determine the parity of the u64 argument.
>   *
>   * Returns:
> - * 0 for even parity, 1 for odd parity
> + * false for even parity, true for odd parity
>   *
>   * Note: This function informs you about the current parity. Example to bail
>   * out when parity is odd:
>   *
> - *	if (parity8(val) == 1)
> + *	if (parity_odd(val))
>   *		return -EBADMSG;
>   *
>   * If you need to calculate a parity bit, you need to draw the conclusion from
>   * this result yourself. Example to enforce odd parity, parity bit is bit 7:
>   *
> - *	if (parity8(val) == 0)
> + *	if (!parity_odd(val))
>   *		val ^= BIT(7);
>   */
> -static inline int parity8(u8 val)
> +#ifndef parity_odd

Please don't add this guard. We've got no any arch implementations
so far, and this is a dead code. Those adding arch code will also
add the ifdefery.

> +static inline __attribute_const__ bool parity_odd(u64 val)
>  {
>  	/*
>  	 * One explanation of this algorithm:
>  	 * https://funloop.org/codex/problem/parity/README.html
>  	 */
> +	val ^= val >> 32;
> +	val ^= val >> 16;
> +	val ^= val >> 8;
>  	val ^= val >> 4;
>  	return (0x6996 >> (val & 0xf)) & 1;
>  }
> +#endif
>  
>  /**
>   * __ffs64 - find first set bit in a 64 bit word
> -- 
> 2.34.1

Re: [PATCH v4 02/13] media: media/test_drivers: Replace open-coded parity calculation with parity_odd()

[Yury Norov]

On Wed, Apr 09, 2025 at 11:43:45PM +0800, Kuan-Wei Chiu wrote:
> Refactor parity calculations to use the standard parity_odd() helper.
> This change eliminates redundant implementations.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
>  drivers/media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> index 70a4024d461e..5e1b7b1742e4 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> @@ -5,6 +5,7 @@
>   * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
>   */
>  
> +#include <linux/bitops.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
>  #include <linux/ktime.h>
> @@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = {
>  
>  static u8 calc_parity(u8 val)
>  {
> -	unsigned i;
> -	unsigned tot = 0;
> -
> -	for (i = 0; i < 7; i++)
> -		tot += (val & (1 << i)) ? 1 : 0;
> -	return val | ((tot & 1) ? 0 : 0x80);
> +	return val | (parity_odd(val) ? 0 : 0x80);

So, if val == 0 than parity_odd(val) is also 0, and this can be
simplified just to:
        return parity(val) ? 0 : 0x80;
Or I miss something?

>  }
>  
>  static void vivid_vbi_gen_set_time_of_day(u8 *packet)
> -- 
> 2.34.1

Re: [PATCH v4 05/13] serial: max3100: Replace open-coded parity calculation with parity_odd()

[Yury Norov]

On Wed, Apr 09, 2025 at 11:43:48PM +0800, Kuan-Wei Chiu wrote:
> Refactor parity calculations to use the standard parity_odd() helper.
> This change eliminates redundant implementations.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
>  drivers/tty/serial/max3100.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> index f2dd83692b2c..36ed41eef7b1 100644
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -16,6 +16,7 @@
>  /* 4 MAX3100s should be enough for everyone */
>  #define MAX_MAX3100 4
>  
> +#include <linux/bitops.h>
>  #include <linux/container_of.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> @@ -133,7 +134,7 @@ static int max3100_do_parity(struct max3100_port *s, u16 c)
>  	else
>  		c &= 0xff;
>  
> -	parity = parity ^ (hweight8(c) & 1);
> +	parity = parity ^ parity_odd(c);

This can be simplified for more unless I misunderstand something...

From: Yury Norov <yury.norov@gmail.com>
Date:   Wed Apr 9 13:22:04 2025 -0400

serial: max3100: Replace open-coded parity

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index f2dd83692b2c..07d332b8e87d 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -121,20 +121,12 @@ static DEFINE_MUTEX(max3100s_lock);		   /* race on probe */
 
 static int max3100_do_parity(struct max3100_port *s, u16 c)
 {
-	int parity;
-
-	if (s->parity & MAX3100_PARITY_ODD)
-		parity = 1;
-	else
-		parity = 0;
-
 	if (s->parity & MAX3100_7BIT)
 		c &= 0x7f;
 	else
 		c &= 0xff;
 
-	parity = parity ^ (hweight8(c) & 1);
-	return parity;
+	return s->parity & MAX3100_PARITY_ODD ? !parity(c) : parity(c);
 }
 
 static int max3100_check_parity(struct max3100_port *s, u16 c)

Re: [PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[Kuan-Wei Chiu]

On Wed, Apr 09, 2025 at 12:54:35PM -0400, Yury Norov wrote:
> On Wed, Apr 09, 2025 at 11:43:43PM +0800, Kuan-Wei Chiu wrote:
> > Several parts of the kernel contain open-coded and redundant
> > implementations of parity calculation. This patch series introduces
> > a unified helper, parity_odd(), to simplify and standardize these
> > cases.
> > 
> > The first patch renames parity8() to parity_odd(), changes its argument
> 
> Alright, if it's an extension of the area of applicability, it should be
> renamed to just parity(). I already shared a table that summarized the
> drivers authors' view on that, and they clearly prefer not to add the
> suffix - 13 vs 2. The __builtin_parity() doesn't care of suffix as well. 
> 
> https://lore.kernel.org/all/Z9GtcNJie8TRKywZ@thinkpad/
> 
> Yes, the argument that boolean function should explain itself sounds
> correct, but in this case, comment on top of the function looks enough
> to me.
> 
> The existing codebase doesn't care about the suffix as well. If no
> strong preference, let's just pick a short and sweet name?
> 
I don't have a strong preference for the name, but if I had to guess
the return value from the function prototype, I would intuitively
expect an int to return "0 for even and 1 for odd," and a bool to
return "true for even, false for odd." I recall Jiri and Jacob shared
similar thoughts, which is why I felt adding _odd could provide better
clarity.

However, I agree that if the kernel doc comment is clear, it might not
be a big issue. But David previously mentioned that he doesn't want to
rely on checking the function's documentation every time while reading
the code.

Regardless, I'm flexible as long as we all reach a consensus on the
naming.

> > type from u8 to u64 for broader applicability, and updates its return
> > type from int to bool to make its usage and return semantics more
> > intuitive-returning true for odd parity and false for even parity. It
> > also adds __attribute_const__ to enable compiler optimizations.
> 
> That's correct and nice, but can you support it with a bloat-o-meter's
> before/after and/or asm snippets? I also think it worth to be a separate
> patch, preferably the last patch in the series.
> 
I quickly tested it with the x86 defconfig, and it appears that the
generated code doesn't change. I forgot who requested the addition
during the review process, but I initially thought it would either
improve the generated code or leave it unchanged without significantly
increasing the source code size.

However, if there's no actual difference in the generated code, maybe
let's just remove it?

Regards,
Kuan-Wei

> > While more efficient implementations may exist, further optimization is
> > postponed until a use case in performance-critical paths arises.
> > 
> > Subsequent patches refactor various kernel components to replace
> > open-coded parity logic with the new helper, reducing code duplication
> > and improving consistency.
> > 
> > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> > ---
> > 
> > To H. Peter:
> > I understand your preference for a parity8/16/32/64() style interface,
> > and I agree that such a design would better accommodate potential
> > arch-specific implementations. However, I suspect there are very few,
> > if any, users who care about the performance of parity calculations
> > enough to warrant such optimizations. So my inclination is to defer any
> > arch-specific or optimized implementations until we see parity_odd()
> > being used in hot paths.
> > 
> > Changes in v4:
> > - Rename parity8() to parity_odd().
> > - Change the argument type from u8 to u64.
> > - Use a single parity_odd() function.
> > 
> > Changes in v3:
> > - Avoid using __builtin_parity.
> > - Change return type to bool.
> > - Drop parity() macro.
> > - Change parityXX() << y to !!parityXX() << y.
> > 
> > Changes in v2:
> > - Provide fallback functions for __builtin_parity() when the compiler
> >   decides not to inline it
> > - Use __builtin_parity() when no architecture-specific implementation
> >   is available
> > - Optimize for constant folding when val is a compile-time constant
> > - Add a generic parity() macro
> > - Drop the x86 bootflag conversion patch since it has been merged into
> >   the tip tree
> > 
> > v3: https://lore.kernel.org/lkml/20250306162541.2633025-1-visitorckw@gmail.com/
> > v1: https://lore.kernel.org/lkml/20250223164217.2139331-1-visitorckw@gmail.com/
> > v2: https://lore.kernel.org/lkml/20250301142409.2513835-1-visitorckw@gmail.com/
> > 
> > Kuan-Wei Chiu (13):
> >   bitops: Change parity8() to parity_odd() with u64 input and bool
> >     return type
> >   media: media/test_drivers: Replace open-coded parity calculation with
> >     parity_odd()
> >   media: pci: cx18-av-vbi: Replace open-coded parity calculation with
> >     parity_odd()
> >   media: saa7115: Replace open-coded parity calculation with
> >     parity_odd()
> >   serial: max3100: Replace open-coded parity calculation with
> >     parity_odd()
> >   lib/bch: Replace open-coded parity calculation with parity_odd()
> >   Input: joystick - Replace open-coded parity calculation with
> >     parity_odd()
> >   net: ethernet: oa_tc6: Replace open-coded parity calculation with
> >     parity_odd()
> >   wifi: brcm80211: Replace open-coded parity calculation with
> >     parity_odd()
> >   drm/bridge: dw-hdmi: Replace open-coded parity calculation with
> >     parity_odd()
> >   mtd: ssfdc: Replace open-coded parity calculation with parity_odd()
> >   fsi: i2cr: Replace open-coded parity calculation with parity_odd()
> >   nfp: bpf: Replace open-coded parity calculation with parity_odd()
> > 
> >  arch/x86/kernel/bootflag.c                    |  4 +--
> >  drivers/fsi/fsi-master-i2cr.c                 | 20 +++------------
> >  .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c   |  8 ++----
> >  drivers/hwmon/spd5118.c                       |  2 +-
> >  drivers/i3c/master/dw-i3c-master.c            |  2 +-
> >  drivers/i3c/master/i3c-master-cdns.c          |  2 +-
> >  drivers/i3c/master/mipi-i3c-hci/dat_v1.c      |  2 +-
> >  drivers/input/joystick/grip_mp.c              | 17 ++-----------
> >  drivers/input/joystick/sidewinder.c           | 25 ++++---------------
> >  drivers/media/i2c/saa7115.c                   | 12 ++-------
> >  drivers/media/pci/cx18/cx18-av-vbi.c          | 12 ++-------
> >  .../media/test-drivers/vivid/vivid-vbi-gen.c  |  8 ++----
> >  drivers/mtd/ssfdc.c                           | 20 +++------------
> >  drivers/net/ethernet/netronome/nfp/nfp_asm.c  |  7 +-----
> >  drivers/net/ethernet/oa_tc6.c                 | 19 +++-----------
> >  .../broadcom/brcm80211/brcmsmac/dma.c         | 18 ++-----------
> >  drivers/tty/serial/max3100.c                  |  3 ++-
> >  include/linux/bitops.h                        | 19 ++++++++------
> >  lib/bch.c                                     | 14 +----------
> >  19 files changed, 49 insertions(+), 165 deletions(-)
> 
> OK, now it looks like a nice consolidation and simplification of code
> base. Thanks for the work.
> 
> Thanks,
> Yury

Re: [PATCH v4 02/13] media: media/test_drivers: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

On Wed, Apr 09, 2025 at 01:03:42PM -0400, Yury Norov wrote:
> On Wed, Apr 09, 2025 at 11:43:45PM +0800, Kuan-Wei Chiu wrote:
> > Refactor parity calculations to use the standard parity_odd() helper.
> > This change eliminates redundant implementations.
> > 
> > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> > ---
> >  drivers/media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > index 70a4024d461e..5e1b7b1742e4 100644
> > --- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > +++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > @@ -5,6 +5,7 @@
> >   * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
> >   */
> >  
> > +#include <linux/bitops.h>
> >  #include <linux/errno.h>
> >  #include <linux/kernel.h>
> >  #include <linux/ktime.h>
> > @@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = {
> >  
> >  static u8 calc_parity(u8 val)
> >  {
> > -	unsigned i;
> > -	unsigned tot = 0;
> > -
> > -	for (i = 0; i < 7; i++)
> > -		tot += (val & (1 << i)) ? 1 : 0;
> > -	return val | ((tot & 1) ? 0 : 0x80);
> > +	return val | (parity_odd(val) ? 0 : 0x80);
> 
> So, if val == 0 than parity_odd(val) is also 0, and this can be
> simplified just to:
>         return parity(val) ? 0 : 0x80;
> Or I miss something?
>
If val == 0x01, the return value of calc_parity() will remain 0x01.
If changed to return parity_odd(val) ? 0 : 0x80;, the return value will
be changed to 0x00.

Regards,
Kuan-Wei

> >  }
> >  
> >  static void vivid_vbi_gen_set_time_of_day(u8 *packet)
> > -- 
> > 2.34.1

Re: [PATCH v4 01/13] bitops: Change parity8() to parity_odd() with u64 input and bool return type

[Kuan-Wei Chiu]

On Wed, Apr 09, 2025 at 12:59:14PM -0400, Yury Norov wrote:
> On Wed, Apr 09, 2025 at 11:43:44PM +0800, Kuan-Wei Chiu wrote:
> > Redesign the parity8() helper as parity_odd(), changing its input type
> > from u8 to u64 to support broader use cases and its return type from
> > int to bool to clearly reflect the function's binary output. The
> > function now returns true for odd parity and false for even parity,
> > making its behavior more intuitive based on the name.
> > 
> > Also mark the function with __attribute_const__ to enable better
> > compiler optimization, as the result depends solely on its input and
> > has no side effects.
> > 
> > While more efficient implementations may exist, further optimization is
> > postponed until a use case in performance-critical paths arises.
> > 
> > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> > ---
> >  arch/x86/kernel/bootflag.c               |  4 ++--
> >  drivers/hwmon/spd5118.c                  |  2 +-
> >  drivers/i3c/master/dw-i3c-master.c       |  2 +-
> >  drivers/i3c/master/i3c-master-cdns.c     |  2 +-
> >  drivers/i3c/master/mipi-i3c-hci/dat_v1.c |  2 +-
> >  include/linux/bitops.h                   | 19 ++++++++++++-------
> >  6 files changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> > index 73274d76ce16..86aae4b2bfd5 100644
> > --- a/arch/x86/kernel/bootflag.c
> > +++ b/arch/x86/kernel/bootflag.c
> > @@ -26,7 +26,7 @@ static void __init sbf_write(u8 v)
> >  	unsigned long flags;
> >  
> >  	if (sbf_port != -1) {
> > -		if (!parity8(v))
> > +		if (!parity_odd(v))
> >  			v ^= SBF_PARITY;
> >  
> >  		printk(KERN_INFO "Simple Boot Flag at 0x%x set to 0x%x\n",
> > @@ -57,7 +57,7 @@ static bool __init sbf_value_valid(u8 v)
> >  {
> >  	if (v & SBF_RESERVED)		/* Reserved bits */
> >  		return false;
> > -	if (!parity8(v))
> > +	if (!parity_odd(v))
> >  		return false;
> >  
> >  	return true;
> > diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> > index 358152868d96..15761f2ca4e9 100644
> > --- a/drivers/hwmon/spd5118.c
> > +++ b/drivers/hwmon/spd5118.c
> > @@ -298,7 +298,7 @@ static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types typ
> >   */
> >  static bool spd5118_vendor_valid(u8 bank, u8 id)
> >  {
> > -	if (parity8(bank) == 0 || parity8(id) == 0)
> > +	if (!parity_odd(bank) || !parity_odd(id))
> >  		return false;
> >  
> >  	id &= 0x7f;
> > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> > index 611c22b72c15..dc61d87fcd94 100644
> > --- a/drivers/i3c/master/dw-i3c-master.c
> > +++ b/drivers/i3c/master/dw-i3c-master.c
> > @@ -867,7 +867,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
> >  		master->devs[pos].addr = ret;
> >  		last_addr = ret;
> >  
> > -		ret |= parity8(ret) ? 0 : BIT(7);
> > +		ret |= parity_odd(ret) ? 0 : BIT(7);
> >  
> >  		writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(ret),
> >  		       master->regs +
> > diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> > index fd3752cea654..df14f978a388 100644
> > --- a/drivers/i3c/master/i3c-master-cdns.c
> > +++ b/drivers/i3c/master/i3c-master-cdns.c
> > @@ -889,7 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr)
> >  	ret |= (addr & GENMASK(9, 7)) << 6;
> >  
> >  	/* RR0[0] = ~XOR(addr[6:0]) */
> > -	ret |= parity8(addr & 0x7f) ? 0 : BIT(0);
> > +	ret |= parity_odd(addr & 0x7f) ? 0 : BIT(0);
> >  
> >  	return ret;
> >  }
> > diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> > index 85c4916972e4..d692a299607d 100644
> > --- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> > +++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> > @@ -114,7 +114,7 @@ static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
> >  	dat_w0 = dat_w0_read(dat_idx);
> >  	dat_w0 &= ~(DAT_0_DYNAMIC_ADDRESS | DAT_0_DYNADDR_PARITY);
> >  	dat_w0 |= FIELD_PREP(DAT_0_DYNAMIC_ADDRESS, address) |
> > -		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);
> > +		  (parity_odd(address) ? 0 : DAT_0_DYNADDR_PARITY);
> >  	dat_w0_write(dat_idx, dat_w0);
> >  }
> >  
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index c1cb53cf2f0f..7c4c8afccef1 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -230,35 +230,40 @@ static inline int get_count_order_long(unsigned long l)
> >  }
> >  
> >  /**
> > - * parity8 - get the parity of an u8 value
> > - * @value: the value to be examined
> > + * parity_odd - get the parity of an u64 value
> > + * @val: the value to be examined
> >   *
> > - * Determine the parity of the u8 argument.
> > + * Determine the parity of the u64 argument.
> >   *
> >   * Returns:
> > - * 0 for even parity, 1 for odd parity
> > + * false for even parity, true for odd parity
> >   *
> >   * Note: This function informs you about the current parity. Example to bail
> >   * out when parity is odd:
> >   *
> > - *	if (parity8(val) == 1)
> > + *	if (parity_odd(val))
> >   *		return -EBADMSG;
> >   *
> >   * If you need to calculate a parity bit, you need to draw the conclusion from
> >   * this result yourself. Example to enforce odd parity, parity bit is bit 7:
> >   *
> > - *	if (parity8(val) == 0)
> > + *	if (!parity_odd(val))
> >   *		val ^= BIT(7);
> >   */
> > -static inline int parity8(u8 val)
> > +#ifndef parity_odd
> 
> Please don't add this guard. We've got no any arch implementations
> so far, and this is a dead code. Those adding arch code will also
> add the ifdefery.
>
Ack.
Will do in next version.

Regards,
Kuan-Wei

> > +static inline __attribute_const__ bool parity_odd(u64 val)
> >  {
> >  	/*
> >  	 * One explanation of this algorithm:
> >  	 * https://funloop.org/codex/problem/parity/README.html
> >  	 */
> > +	val ^= val >> 32;
> > +	val ^= val >> 16;
> > +	val ^= val >> 8;
> >  	val ^= val >> 4;
> >  	return (0x6996 >> (val & 0xf)) & 1;
> >  }
> > +#endif
> >  
> >  /**
> >   * __ffs64 - find first set bit in a 64 bit word
> > -- 
> > 2.34.1

Re: [PATCH v4 05/13] serial: max3100: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

On Wed, Apr 09, 2025 at 01:24:08PM -0400, Yury Norov wrote:
> On Wed, Apr 09, 2025 at 11:43:48PM +0800, Kuan-Wei Chiu wrote:
> > Refactor parity calculations to use the standard parity_odd() helper.
> > This change eliminates redundant implementations.
> > 
> > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> > ---
> >  drivers/tty/serial/max3100.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> > index f2dd83692b2c..36ed41eef7b1 100644
> > --- a/drivers/tty/serial/max3100.c
> > +++ b/drivers/tty/serial/max3100.c
> > @@ -16,6 +16,7 @@
> >  /* 4 MAX3100s should be enough for everyone */
> >  #define MAX_MAX3100 4
> >  
> > +#include <linux/bitops.h>
> >  #include <linux/container_of.h>
> >  #include <linux/delay.h>
> >  #include <linux/device.h>
> > @@ -133,7 +134,7 @@ static int max3100_do_parity(struct max3100_port *s, u16 c)
> >  	else
> >  		c &= 0xff;
> >  
> > -	parity = parity ^ (hweight8(c) & 1);
> > +	parity = parity ^ parity_odd(c);
> 
> This can be simplified for more unless I misunderstand something...
> 
I usually don't change the existing coding style since each subsystem
may have its own preferred style.  

But yeah, if this is the preferred way, I can make this change in the
next version.

Regards,
Kuan-Wei

> From: Yury Norov <yury.norov@gmail.com>
> Date:   Wed Apr 9 13:22:04 2025 -0400
> 
> serial: max3100: Replace open-coded parity
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> index f2dd83692b2c..07d332b8e87d 100644
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -121,20 +121,12 @@ static DEFINE_MUTEX(max3100s_lock);		   /* race on probe */
>  
>  static int max3100_do_parity(struct max3100_port *s, u16 c)
>  {
> -	int parity;
> -
> -	if (s->parity & MAX3100_PARITY_ODD)
> -		parity = 1;
> -	else
> -		parity = 0;
> -
>  	if (s->parity & MAX3100_7BIT)
>  		c &= 0x7f;
>  	else
>  		c &= 0xff;
>  
> -	parity = parity ^ (hweight8(c) & 1);
> -	return parity;
> +	return s->parity & MAX3100_PARITY_ODD ? !parity(c) : parity(c);
>  }
>  
>  static int max3100_check_parity(struct max3100_port *s, u16 c)

Re: [PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[Yury Norov]

On Thu, Apr 10, 2025 at 02:15:30AM +0800, Kuan-Wei Chiu wrote:
> On Wed, Apr 09, 2025 at 12:54:35PM -0400, Yury Norov wrote:
> > On Wed, Apr 09, 2025 at 11:43:43PM +0800, Kuan-Wei Chiu wrote:
> > > Several parts of the kernel contain open-coded and redundant
> > > implementations of parity calculation. This patch series introduces
> > > a unified helper, parity_odd(), to simplify and standardize these
> > > cases.
> > > 
> > > The first patch renames parity8() to parity_odd(), changes its argument
> > 
> > Alright, if it's an extension of the area of applicability, it should be
> > renamed to just parity(). I already shared a table that summarized the
> > drivers authors' view on that, and they clearly prefer not to add the
> > suffix - 13 vs 2. The __builtin_parity() doesn't care of suffix as well. 
> > 
> > https://lore.kernel.org/all/Z9GtcNJie8TRKywZ@thinkpad/
> > 
> > Yes, the argument that boolean function should explain itself sounds
> > correct, but in this case, comment on top of the function looks enough
> > to me.
> > 
> > The existing codebase doesn't care about the suffix as well. If no
> > strong preference, let's just pick a short and sweet name?
> > 
> I don't have a strong preference for the name, but if I had to guess
> the return value from the function prototype, I would intuitively
> expect an int to return "0 for even and 1 for odd," and a bool to
> return "true for even, false for odd." I recall Jiri and Jacob shared
> similar thoughts, which is why I felt adding _odd could provide better
> clarity.

I think they said they are convinced that parity should return 1 for
odd because of folding and __builtin_parity() arguments.
 
> However, I agree that if the kernel doc comment is clear, it might not
> be a big issue. But David previously mentioned that he doesn't want to
> rely on checking the function's documentation every time while reading
> the code.

He's wrong. Kernel engineers _must_ read documentation, regardless.
 
> Regardless, I'm flexible as long as we all reach a consensus on the
> naming.
> 
> > > type from u8 to u64 for broader applicability, and updates its return
> > > type from int to bool to make its usage and return semantics more
> > > intuitive-returning true for odd parity and false for even parity. It
> > > also adds __attribute_const__ to enable compiler optimizations.
> > 
> > That's correct and nice, but can you support it with a bloat-o-meter's
> > before/after and/or asm snippets? I also think it worth to be a separate
> > patch, preferably the last patch in the series.
> > 
> I quickly tested it with the x86 defconfig, and it appears that the
> generated code doesn't change. I forgot who requested the addition
> during the review process, but I initially thought it would either
> improve the generated code or leave it unchanged without significantly
> increasing the source code size.

That's what I actually expected, but was shy to guess openly. :). It's
hard to imagine how compiler may improve code generation in this case...

This attribute is used when there's an asm block, or some non-trivial
function call. In this case, the function is self-consistent and makes
no calls. And you see, const annotation raises more questions than
solves problems. Let's drop it.

Thanks,
Yury

Re: [PATCH v4 01/13] bitops: Change parity8() to parity_odd() with u64 input and bool return type

[Guenter Roeck]

On 4/9/25 11:25, Kuan-Wei Chiu wrote:
> On Wed, Apr 09, 2025 at 12:59:14PM -0400, Yury Norov wrote:
>> On Wed, Apr 09, 2025 at 11:43:44PM +0800, Kuan-Wei Chiu wrote:
>>> Redesign the parity8() helper as parity_odd(), changing its input type
>>> from u8 to u64 to support broader use cases and its return type from
>>> int to bool to clearly reflect the function's binary output. The
>>> function now returns true for odd parity and false for even parity,
>>> making its behavior more intuitive based on the name.
>>>
>>> Also mark the function with __attribute_const__ to enable better
>>> compiler optimization, as the result depends solely on its input and
>>> has no side effects.
>>>
>>> While more efficient implementations may exist, further optimization is
>>> postponed until a use case in performance-critical paths arises.
>>>
>>> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
>>> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
>>> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
>>> ---
>>>   arch/x86/kernel/bootflag.c               |  4 ++--
>>>   drivers/hwmon/spd5118.c                  |  2 +-
>>>   drivers/i3c/master/dw-i3c-master.c       |  2 +-
>>>   drivers/i3c/master/i3c-master-cdns.c     |  2 +-
>>>   drivers/i3c/master/mipi-i3c-hci/dat_v1.c |  2 +-
>>>   include/linux/bitops.h                   | 19 ++++++++++++-------
>>>   6 files changed, 18 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
>>> index 73274d76ce16..86aae4b2bfd5 100644
>>> --- a/arch/x86/kernel/bootflag.c
>>> +++ b/arch/x86/kernel/bootflag.c
>>> @@ -26,7 +26,7 @@ static void __init sbf_write(u8 v)
>>>   	unsigned long flags;
>>>   
>>>   	if (sbf_port != -1) {
>>> -		if (!parity8(v))
>>> +		if (!parity_odd(v))

What is the benefit of this change all over the place instead of
adding parity_odd() as new API and keeping the old one (just letting
it call the new API) ?

A simple

static inline int parity8(u8 val)
{
	return parity_odd(val);
}

would have done the trick and be much less invasive.

Guenter

Re: [PATCH v4 02/13] media: media/test_drivers: Replace open-coded parity calculation with parity_odd()

[Yury Norov]

On Thu, Apr 10, 2025 at 02:23:09AM +0800, Kuan-Wei Chiu wrote:
> On Wed, Apr 09, 2025 at 01:03:42PM -0400, Yury Norov wrote:
> > On Wed, Apr 09, 2025 at 11:43:45PM +0800, Kuan-Wei Chiu wrote:
> > > Refactor parity calculations to use the standard parity_odd() helper.
> > > This change eliminates redundant implementations.
> > > 
> > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> > > ---
> > >  drivers/media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++------
> > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > > index 70a4024d461e..5e1b7b1742e4 100644
> > > --- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > > +++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > > @@ -5,6 +5,7 @@
> > >   * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
> > >   */
> > >  
> > > +#include <linux/bitops.h>
> > >  #include <linux/errno.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/ktime.h>
> > > @@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = {
> > >  
> > >  static u8 calc_parity(u8 val)
> > >  {
> > > -	unsigned i;
> > > -	unsigned tot = 0;
> > > -
> > > -	for (i = 0; i < 7; i++)
> > > -		tot += (val & (1 << i)) ? 1 : 0;
> > > -	return val | ((tot & 1) ? 0 : 0x80);
> > > +	return val | (parity_odd(val) ? 0 : 0x80);
> > 
> > So, if val == 0 than parity_odd(val) is also 0, and this can be
> > simplified just to:
> >         return parity(val) ? 0 : 0x80;
> > Or I miss something?
> >
> If val == 0x01, the return value of calc_parity() will remain 0x01.
> If changed to return parity_odd(val) ? 0 : 0x80;, the return value will
> be changed to 0x00.

Sorry, I meant
        return val ? 0 : 0x80;

This 'val | (parity_odd(val)' is only false when val == 0, right?
When val != 0, compiler will return true immediately, not even calling
parity().

I think we need a comment from authors.

Re: [PATCH v4 03/13] media: pci: cx18-av-vbi: Replace open-coded parity calculation with parity_odd()

[Arend van Spriel]

On 4/9/2025 5:43 PM, Kuan-Wei Chiu wrote:
> Refactor parity calculations to use the standard parity_odd() helper.
> This change eliminates redundant implementations.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
>   drivers/media/pci/cx18/cx18-av-vbi.c | 12 ++----------
>   1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/pci/cx18/cx18-av-vbi.c b/drivers/media/pci/cx18/cx18-av-vbi.c
> index 65281d40c681..15b515b95956 100644
> --- a/drivers/media/pci/cx18/cx18-av-vbi.c
> +++ b/drivers/media/pci/cx18/cx18-av-vbi.c

[...]

> @@ -278,7 +270,7 @@ int cx18_av_decode_vbi_line(struct v4l2_subdev *sd,
>   		break;
>   	case 6:
>   		sdid = V4L2_SLICED_CAPTION_525;
> -		err = !odd_parity(p[0]) || !odd_parity(p[1]);
> +		err = !parity_odd(p[0]) || !parity_odd(p[1]);

No need to call parity_odd() twice here. Instead you could do:

		err = !parity_odd(p[0] ^ p[1]);

This is orthogonal to the change to parity_odd() though. More specific 
to the new parity_odd() you can now do following as parity_odd() 
argument is u64:

		err = !parity_odd(*(u16 *)p);

Regards,
Arend

Re: [PATCH v4 02/13] media: media/test_drivers: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

On Wed, Apr 09, 2025 at 02:41:03PM -0400, Yury Norov wrote:
> On Thu, Apr 10, 2025 at 02:23:09AM +0800, Kuan-Wei Chiu wrote:
> > On Wed, Apr 09, 2025 at 01:03:42PM -0400, Yury Norov wrote:
> > > On Wed, Apr 09, 2025 at 11:43:45PM +0800, Kuan-Wei Chiu wrote:
> > > > Refactor parity calculations to use the standard parity_odd() helper.
> > > > This change eliminates redundant implementations.
> > > > 
> > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> > > > ---
> > > >  drivers/media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++------
> > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > > > index 70a4024d461e..5e1b7b1742e4 100644
> > > > --- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > > > +++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> > > > @@ -5,6 +5,7 @@
> > > >   * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
> > > >   */
> > > >  
> > > > +#include <linux/bitops.h>
> > > >  #include <linux/errno.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/ktime.h>
> > > > @@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = {
> > > >  
> > > >  static u8 calc_parity(u8 val)
> > > >  {
> > > > -	unsigned i;
> > > > -	unsigned tot = 0;
> > > > -
> > > > -	for (i = 0; i < 7; i++)
> > > > -		tot += (val & (1 << i)) ? 1 : 0;
> > > > -	return val | ((tot & 1) ? 0 : 0x80);
> > > > +	return val | (parity_odd(val) ? 0 : 0x80);
> > > 
> > > So, if val == 0 than parity_odd(val) is also 0, and this can be
> > > simplified just to:
> > >         return parity(val) ? 0 : 0x80;
> > > Or I miss something?
> > >
> > If val == 0x01, the return value of calc_parity() will remain 0x01.
> > If changed to return parity_odd(val) ? 0 : 0x80;, the return value will
> > be changed to 0x00.
> 
> Sorry, I meant
>         return val ? 0 : 0x80;
> 
> This 'val | (parity_odd(val)' is only false when val == 0, right?
> When val != 0, compiler will return true immediately, not even calling
> parity().
>
I'm still confused.

Maybe you're interpreting the code as:

	(val | parity(val)) ? 0 : 0x80

But what we're trying to do is:

	val | (parity(val) ? 0 : 0x80)

So, for example, if val == 0x06, the return value will be 0x86.
Only returning 0 or 0x80 seems wrong to me.
Or did I misunderstand something?

Regards,
Kuan-Wei

> I think we need a comment from authors.

Re: [PATCH v4 03/13] media: pci: cx18-av-vbi: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

On Wed, Apr 09, 2025 at 08:43:09PM +0200, Arend van Spriel wrote:
> On 4/9/2025 5:43 PM, Kuan-Wei Chiu wrote:
> > Refactor parity calculations to use the standard parity_odd() helper.
> > This change eliminates redundant implementations.
> > 
> > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> > ---
> >   drivers/media/pci/cx18/cx18-av-vbi.c | 12 ++----------
> >   1 file changed, 2 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/pci/cx18/cx18-av-vbi.c b/drivers/media/pci/cx18/cx18-av-vbi.c
> > index 65281d40c681..15b515b95956 100644
> > --- a/drivers/media/pci/cx18/cx18-av-vbi.c
> > +++ b/drivers/media/pci/cx18/cx18-av-vbi.c
> 
> [...]
> 
> > @@ -278,7 +270,7 @@ int cx18_av_decode_vbi_line(struct v4l2_subdev *sd,
> >   		break;
> >   	case 6:
> >   		sdid = V4L2_SLICED_CAPTION_525;
> > -		err = !odd_parity(p[0]) || !odd_parity(p[1]);
> > +		err = !parity_odd(p[0]) || !parity_odd(p[1]);
> 
> No need to call parity_odd() twice here. Instead you could do:
> 
> 		err = !parity_odd(p[0] ^ p[1]);
> 
> This is orthogonal to the change to parity_odd() though. More specific to
> the new parity_odd() you can now do following as parity_odd() argument is
> u64:
> 
> 		err = !parity_odd(*(u16 *)p);
> 
> 
Thanks for the feedback!
Would you prefer this change to be part of the parity() conversion
patch, or in a separate one?

Regards,
Kuan-Wei

Re: [PATCH v4 01/13] bitops: Change parity8() to parity_odd() with u64 input and bool return type

[Kuan-Wei Chiu]

On Wed, Apr 09, 2025 at 11:39:22AM -0700, Guenter Roeck wrote:
> On 4/9/25 11:25, Kuan-Wei Chiu wrote:
> > On Wed, Apr 09, 2025 at 12:59:14PM -0400, Yury Norov wrote:
> > > On Wed, Apr 09, 2025 at 11:43:44PM +0800, Kuan-Wei Chiu wrote:
> > > > Redesign the parity8() helper as parity_odd(), changing its input type
> > > > from u8 to u64 to support broader use cases and its return type from
> > > > int to bool to clearly reflect the function's binary output. The
> > > > function now returns true for odd parity and false for even parity,
> > > > making its behavior more intuitive based on the name.
> > > > 
> > > > Also mark the function with __attribute_const__ to enable better
> > > > compiler optimization, as the result depends solely on its input and
> > > > has no side effects.
> > > > 
> > > > While more efficient implementations may exist, further optimization is
> > > > postponed until a use case in performance-critical paths arises.
> > > > 
> > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> > > > ---
> > > >   arch/x86/kernel/bootflag.c               |  4 ++--
> > > >   drivers/hwmon/spd5118.c                  |  2 +-
> > > >   drivers/i3c/master/dw-i3c-master.c       |  2 +-
> > > >   drivers/i3c/master/i3c-master-cdns.c     |  2 +-
> > > >   drivers/i3c/master/mipi-i3c-hci/dat_v1.c |  2 +-
> > > >   include/linux/bitops.h                   | 19 ++++++++++++-------
> > > >   6 files changed, 18 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> > > > index 73274d76ce16..86aae4b2bfd5 100644
> > > > --- a/arch/x86/kernel/bootflag.c
> > > > +++ b/arch/x86/kernel/bootflag.c
> > > > @@ -26,7 +26,7 @@ static void __init sbf_write(u8 v)
> > > >   	unsigned long flags;
> > > >   	if (sbf_port != -1) {
> > > > -		if (!parity8(v))
> > > > +		if (!parity_odd(v))
> 
> What is the benefit of this change all over the place instead of
> adding parity_odd() as new API and keeping the old one (just letting
> it call the new API) ?
> 
> A simple
> 
> static inline int parity8(u8 val)
> {
> 	return parity_odd(val);
> }
> 
> would have done the trick and be much less invasive.
> 
Yury has previously mentioned that adding multiple fixed-type parity
functions increases his maintenance burden. IIUC, he prefers having a
single interface in bitops.h rather than multiple ones.

He were reluctant to add three more functions like:

static inline bool parity16(u16 val)
{
    return parity8(val ^ (val >> 8));
}

static inline bool parity32(u32 val)
{
    return parity16(val ^ (val >> 16));
}

static inline bool parity64(u64 val)
{
    return parity32(val ^ (val >> 32));
}

But instead, we ended up with:

static inline bool parity(u64 val)
{
    val ^= val >> 32;
	val ^= val >> 16;
	val ^= val >> 8;
	val ^= val >> 4;
	return (0x6996 >> (val & 0xf)) & 1;
}

static inline bool parity8(u8 val)
{
    return parity_odd(val);
}

But in the end, we introduced both parity(u64) and parity8(u8), which,
IMHO, might be even more confusing than having consistent fixed-type
helpers.

Regards,
Kuan-Wei

Re: [PATCH v4 07/13] Input: joystick - Replace open-coded parity calculation with parity_odd()

[Dmitry Torokhov]

On Wed, Apr 09, 2025 at 11:43:50PM +0800, Kuan-Wei Chiu wrote:
> Refactor parity calculations to use the standard parity_odd() helper.
> This change eliminates redundant implementations.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

Re: [PATCH v4 03/13] media: pci: cx18-av-vbi: Replace open-coded parity calculation with parity_odd()

[Johannes Berg]

On Wed, 2025-04-09 at 20:43 +0200, Arend van Spriel wrote:
> 
> This is orthogonal to the change to parity_odd() though. More specific 
> to the new parity_odd() you can now do following as parity_odd() 
> argument is u64:
> 
> 		err = !parity_odd(*(u16 *)p);
> 

Can it though? Need to be careful with alignment with that, I'd think.

johannes

Re: [PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[H. Peter Anvin]

On 4/9/25 11:33, Yury Norov wrote:
>>>
>> I don't have a strong preference for the name, but if I had to guess
>> the return value from the function prototype, I would intuitively
>> expect an int to return "0 for even and 1 for odd," and a bool to
>> return "true for even, false for odd." I recall Jiri and Jacob shared
>> similar thoughts, which is why I felt adding _odd could provide better
>> clarity.
> 
> I think they said they are convinced that parity should return 1 for
> odd because of folding and __builtin_parity() arguments.
> 

And for bool, 0 == false, and 1 == true. In fact, the *definitions* for 
false and true in C (but not C++) is:

<stdbool.h>:
typedef _Bool bool;
#define false	0
#define true	1

If someone wants to make more clear, it would be better to put "typedef 
bool bit_t" in a common header, but that personally seems ridiculous to me.
   >>>> type from u8 to u64 for broader applicability, and updates its 
return
>>>> type from int to bool to make its usage and return semantics more
>>>> intuitive-returning true for odd parity and false for even parity. It
>>>> also adds __attribute_const__ to enable compiler optimizations.
>>>
>>> That's correct and nice, but can you support it with a bloat-o-meter's
>>> before/after and/or asm snippets? I also think it worth to be a separate
>>> patch, preferably the last patch in the series.
>>>
>> I quickly tested it with the x86 defconfig, and it appears that the
>> generated code doesn't change. I forgot who requested the addition
>> during the review process, but I initially thought it would either
>> improve the generated code or leave it unchanged without significantly
>> increasing the source code size.
> 
> That's what I actually expected, but was shy to guess openly. :). It's
> hard to imagine how compiler may improve code generation in this case...
> 
> This attribute is used when there's an asm block, or some non-trivial
> function call. In this case, the function is self-consistent and makes
> no calls. And you see, const annotation raises more questions than
> solves problems. Let's drop it.

Ah yes; one of the quirks about gcc asm is that an asm is implicitly 
assumed "const" (with no memory operands) or "pure" (with memory 
operands) unless declared volatile or given an explicit "memory" clobber.

So yes, the compiler can most definitely derive the constness from the 
form of the function even in the variable case.

I would still like to see __builtin_parity() being used as an 
architecture opt-in; it can, of course, also be unconditionally used in 
the constant case.

So in the end one of these two become my preferred implementation, and I 
really don't think it is very complicated:

#ifndef use_builtin_parity
#define use_builtin_parity(x) __builtin_constant_p(x)
#endif

static inline bool parity8(u8 val)
{
	if (use_builtin_parity(val))
		return __builtin_parity(val);
	val ^= val >> 4;
	return (0x6996 >> (val & 0xf)) & 1;
}

static inline bool parity16(u16 val)
{
	if (use_builtin_parity(val))
		return __builtin_parity(val);
	return parity8(val ^ (val >> 8));
}

static inline bool parity32(u32 val)
{
	if (use_builtin_parity(val))
		return __builtin_parity(val);
	return parity16(val ^ (val >> 16));
}

static inline bool parity64(u64 val)
{
	if (use_builtin_parity(val))
		return __builtin_parityll(val);
	return parity32(val ^ (val >> 32));
}

This means that an architecture -- in particular, x86 -- can still ask 
to use __builtin_parity*() directly. It means that architectures on 
which __builtin_parity*() produces bad code should either complain to 
the gcc/clang team and have it fixed, or we can add additional mechanism 
for them to override the implementation at that time.

The alternative is to stop worrying about overengineering, and just do 
it once and for all:

#ifndef parity8
static inline bool parity8(u8 val)
{
	val ^= val >> 4;
	return (0x6996 >> (val & 0xf)) & 1;
}
#endif

#ifndef parity16
static inline bool parity16(u16 val)
{
	return parity8(val ^ (val >> 8));
}
#endif

#ifndef parity32
static inline bool parity32(u32 val)
{
	return parity16(val ^ (val >> 16));
}
#endif

#ifndef parity64
static inline bool parity64(u64 val)
{
	return parity32(val ^ (val >> 32));
}
#endif

In either case, instead of packing the cascade into one function, make 
good use of it.

In the latter case, __builtin_constant_p() isn't necessary as it puts 
the onus on the architecture to separate out const and non-const cases, 
if it matters -- which it doesn't if the architecture simply wants to 
use __builtin_parity:

#define parity8(x)  ((bool) __builtin_parity((u8)(x)))
#define parity16(x) ((bool) __builtin_parity((u16)(x)))
#define parity32(x) ((bool) __builtin_parity((u32)(x)))
#define parity64(x) ((bool) __builtin_parityll((u64)(x)))

As stated before, I don't really see that the parity function itself 
would be very suitable for a generic helper, but if it were to, then 
using the "standard" macro construct for it would seem to be the better 
option.

(And I would be very much in favor of not open-coding the helper 
everywhere but to macroize it; effectively creating a C++ template 
equivalent. It is out of scope for this project, though.)

	-hpa

Re: [PATCH v4 03/13] media: pci: cx18-av-vbi: Replace open-coded parity calculation with parity_odd()

[Arend Van Spriel]

On April 10, 2025 12:06:52 AM Johannes Berg <johannes@sipsolutions.net> wrote:

> On Wed, 2025-04-09 at 20:43 +0200, Arend van Spriel wrote:
>>
>> This is orthogonal to the change to parity_odd() though. More specific
>> to the new parity_odd() you can now do following as parity_odd()
>> argument is u64:
>>
>> err = !parity_odd(*(u16 *)p);
>
> Can it though? Need to be careful with alignment with that, I'd think.

My bad. You are absolutely right.

Gr. AvS
>

Re: [PATCH v4 02/13] media: media/test_drivers: Replace open-coded parity calculation with parity_odd()

[Yury Norov]

On Thu, Apr 10, 2025 at 02:56:41AM +0800, Kuan-Wei Chiu wrote:
> On Wed, Apr 09, 2025 at 02:41:03PM -0400, Yury Norov wrote:
> > On Thu, Apr 10, 2025 at 02:23:09AM +0800, Kuan-Wei Chiu wrote:
> > > On Wed, Apr 09, 2025 at 01:03:42PM -0400, Yury Norov wrote:
> > > > On Wed, Apr 09, 2025 at 11:43:45PM +0800, Kuan-Wei Chiu wrote:

> > > > So, if val == 0 than parity_odd(val) is also 0, and this can be
> > > > simplified just to:
> > > >         return parity(val) ? 0 : 0x80;
> > > > Or I miss something?
> > > >
> > > If val == 0x01, the return value of calc_parity() will remain 0x01.
> > > If changed to return parity_odd(val) ? 0 : 0x80;, the return value will
> > > be changed to 0x00.
> > 
> > Sorry, I meant
> >         return val ? 0 : 0x80;
> > 
> > This 'val | (parity_odd(val)' is only false when val == 0, right?
> > When val != 0, compiler will return true immediately, not even calling
> > parity().
> >
> I'm still confused.
> 
> Maybe you're interpreting the code as:
> 
> 	(val | parity(val)) ? 0 : 0x80
> 
> But what we're trying to do is:
> 
> 	val | (parity(val) ? 0 : 0x80)
> 
> So, for example, if val == 0x06, the return value will be 0x86.
> Only returning 0 or 0x80 seems wrong to me.
> Or did I misunderstand something?

I misread the whole construction. Sorry, you're right. Scratch this.

Re: [PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[Kuan-Wei Chiu]

On Wed, Apr 09, 2025 at 07:09:28PM -0700, H. Peter Anvin wrote:
> On 4/9/25 11:33, Yury Norov wrote:
> > > > 
> > > I don't have a strong preference for the name, but if I had to guess
> > > the return value from the function prototype, I would intuitively
> > > expect an int to return "0 for even and 1 for odd," and a bool to
> > > return "true for even, false for odd." I recall Jiri and Jacob shared
> > > similar thoughts, which is why I felt adding _odd could provide better
> > > clarity.
> > 
> > I think they said they are convinced that parity should return 1 for
> > odd because of folding and __builtin_parity() arguments.
> > 
> 
> And for bool, 0 == false, and 1 == true. In fact, the *definitions* for
> false and true in C (but not C++) is:
> 
> <stdbool.h>:
> typedef _Bool bool;
> #define false	0
> #define true	1
> 
> If someone wants to make more clear, it would be better to put "typedef bool
> bit_t" in a common header, but that personally seems ridiculous to me.
>   >>>> type from u8 to u64 for broader applicability, and updates its return
> > > > > type from int to bool to make its usage and return semantics more
> > > > > intuitive-returning true for odd parity and false for even parity. It
> > > > > also adds __attribute_const__ to enable compiler optimizations.
> > > > 
> > > > That's correct and nice, but can you support it with a bloat-o-meter's
> > > > before/after and/or asm snippets? I also think it worth to be a separate
> > > > patch, preferably the last patch in the series.
> > > > 
> > > I quickly tested it with the x86 defconfig, and it appears that the
> > > generated code doesn't change. I forgot who requested the addition
> > > during the review process, but I initially thought it would either
> > > improve the generated code or leave it unchanged without significantly
> > > increasing the source code size.
> > 
> > That's what I actually expected, but was shy to guess openly. :). It's
> > hard to imagine how compiler may improve code generation in this case...
> > 
> > This attribute is used when there's an asm block, or some non-trivial
> > function call. In this case, the function is self-consistent and makes
> > no calls. And you see, const annotation raises more questions than
> > solves problems. Let's drop it.
> 
> Ah yes; one of the quirks about gcc asm is that an asm is implicitly assumed
> "const" (with no memory operands) or "pure" (with memory operands) unless
> declared volatile or given an explicit "memory" clobber.
> 
> So yes, the compiler can most definitely derive the constness from the form
> of the function even in the variable case.
> 
> I would still like to see __builtin_parity() being used as an architecture
> opt-in; it can, of course, also be unconditionally used in the constant
> case.
> 
> So in the end one of these two become my preferred implementation, and I
> really don't think it is very complicated:
> 
> #ifndef use_builtin_parity
> #define use_builtin_parity(x) __builtin_constant_p(x)
> #endif
> 
> static inline bool parity8(u8 val)
> {
> 	if (use_builtin_parity(val))
> 		return __builtin_parity(val);
> 	val ^= val >> 4;
> 	return (0x6996 >> (val & 0xf)) & 1;
> }
> 
> static inline bool parity16(u16 val)
> {
> 	if (use_builtin_parity(val))
> 		return __builtin_parity(val);
> 	return parity8(val ^ (val >> 8));
> }
> 
> static inline bool parity32(u32 val)
> {
> 	if (use_builtin_parity(val))
> 		return __builtin_parity(val);
> 	return parity16(val ^ (val >> 16));
> }
> 
> static inline bool parity64(u64 val)
> {
> 	if (use_builtin_parity(val))
> 		return __builtin_parityll(val);
> 	return parity32(val ^ (val >> 32));
> }
> 
> This means that an architecture -- in particular, x86 -- can still ask to
> use __builtin_parity*() directly. It means that architectures on which
> __builtin_parity*() produces bad code should either complain to the
> gcc/clang team and have it fixed, or we can add additional mechanism for
> them to override the implementation at that time.
> 
> The alternative is to stop worrying about overengineering, and just do it
> once and for all:
> 
> #ifndef parity8
> static inline bool parity8(u8 val)
> {
> 	val ^= val >> 4;
> 	return (0x6996 >> (val & 0xf)) & 1;
> }
> #endif
> 
> #ifndef parity16
> static inline bool parity16(u16 val)
> {
> 	return parity8(val ^ (val >> 8));
> }
> #endif
> 
> #ifndef parity32
> static inline bool parity32(u32 val)
> {
> 	return parity16(val ^ (val >> 16));
> }
> #endif
> 
> #ifndef parity64
> static inline bool parity64(u64 val)
> {
> 	return parity32(val ^ (val >> 32));
> }
> #endif
> 
> In either case, instead of packing the cascade into one function, make good
> use of it.
> 
> In the latter case, __builtin_constant_p() isn't necessary as it puts the
> onus on the architecture to separate out const and non-const cases, if it
> matters -- which it doesn't if the architecture simply wants to use
> __builtin_parity:
> 
> #define parity8(x)  ((bool) __builtin_parity((u8)(x)))
> #define parity16(x) ((bool) __builtin_parity((u16)(x)))
> #define parity32(x) ((bool) __builtin_parity((u32)(x)))
> #define parity64(x) ((bool) __builtin_parityll((u64)(x)))
> 
> As stated before, I don't really see that the parity function itself would
> be very suitable for a generic helper, but if it were to, then using the
> "standard" macro construct for it would seem to be the better option.
> 
> (And I would be very much in favor of not open-coding the helper everywhere
> but to macroize it; effectively creating a C++ template equivalent. It is
> out of scope for this project, though.)
> 
IIUC, you prefer using the parity8/16/32/64() interface with
__builtin_parity(), regardless of whether there are users on the hot
path?

If the maintainer has no concerns about maintenance burden, I also lean
toward this interface. While I don't think the implementation is
particularly complex, I'm not the maintainer, so I'd rather not argue
about the maintenance aspect. Also, to be clear, I don't think I'm the
right person to provide evidence that performance or code generation
matters to any user.

OTOH, If we do end up going with one of the two approaches, since
bitops.h is included (directly or indirectly) by many files while
parity is only used in fewer than 20, perhaps we should move the
parity-related code to a separate parity.h. It doesn't necessarily have
to be maintained by Yury - it could be someone else, or me.

Regards,
Kuan-Wei

Re: [PATCH v4 03/13] media: pci: cx18-av-vbi: Replace open-coded parity calculation with parity_odd()

[Kuan-Wei Chiu]

On Thu, Apr 10, 2025 at 07:08:58AM +0200, Arend Van Spriel wrote:
> On April 10, 2025 12:06:52 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> 
> > On Wed, 2025-04-09 at 20:43 +0200, Arend van Spriel wrote:
> > > 
> > > This is orthogonal to the change to parity_odd() though. More specific
> > > to the new parity_odd() you can now do following as parity_odd()
> > > argument is u64:
> > > 
> > > err = !parity_odd(*(u16 *)p);
> > 
> > Can it though? Need to be careful with alignment with that, I'd think.
> 
> My bad. You are absolutely right.
> 
Then maybe we can still go with:

	err = !parity_odd(p[0] ^ p[1]);

I believe this should still be a fairly safe approach?

Regards,
Kuan-Wei

Re: [PATCH v4 03/13] media: pci: cx18-av-vbi: Replace open-coded parity calculation with parity_odd()

[Arend Van Spriel]

On April 11, 2025 6:37:35 PM Kuan-Wei Chiu <visitorckw@gmail.com> wrote:

> On Thu, Apr 10, 2025 at 07:08:58AM +0200, Arend Van Spriel wrote:
>> On April 10, 2025 12:06:52 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>>
>>> On Wed, 2025-04-09 at 20:43 +0200, Arend van Spriel wrote:
>>>>
>>>> This is orthogonal to the change to parity_odd() though. More specific
>>>> to the new parity_odd() you can now do following as parity_odd()
>>>> argument is u64:
>>>>
>>>> err = !parity_odd(*(u16 *)p);
>>>
>>> Can it though? Need to be careful with alignment with that, I'd think.
>>
>> My bad. You are absolutely right.
> Then maybe we can still go with:
>
> err = !parity_odd(p[0] ^ p[1]);
>
> I believe this should still be a fairly safe approach?

Yes. Or whatever the name will be ;-)

Regards,
Arend

Re: [PATCH v4 09/13] wifi: brcm80211: Replace open-coded parity calculation with parity_odd()

[Simon Horman]

On Wed, Apr 09, 2025 at 11:43:52PM +0800, Kuan-Wei Chiu wrote:
> Refactor parity calculations to use the standard parity_odd() helper.
> This change eliminates redundant implementations.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
>  .../wireless/broadcom/brcm80211/brcmsmac/dma.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
> index 80c35027787a..5d7500ee2d3b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/dma.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/delay.h>
>  #include <linux/pci.h>
> +#include <linux/bitops.h>
>  #include <net/cfg80211.h>
>  #include <net/mac80211.h>
>  
> @@ -283,24 +284,9 @@ struct dma_info {
>  	bool aligndesc_4k;
>  };
>  
> -/* Check for odd number of 1's */
> -static u32 parity32(__le32 data)
> -{
> -	/* no swap needed for counting 1's */
> -	u32 par_data = *(u32 *)&data;
> -
> -	par_data ^= par_data >> 16;
> -	par_data ^= par_data >> 8;
> -	par_data ^= par_data >> 4;
> -	par_data ^= par_data >> 2;
> -	par_data ^= par_data >> 1;
> -
> -	return par_data & 1;
> -}
> -
>  static bool dma64_dd_parity(struct dma64desc *dd)
>  {
> -	return parity32(dd->addrlow ^ dd->addrhigh ^ dd->ctrl1 ^ dd->ctrl2);
> +	return parity_odd(dd->addrlow ^ dd->addrhigh ^ dd->ctrl1 ^ dd->ctrl2);
>  }

parity32 expected a little-endian integer as it's argument
while parity_odd expects a host byte order value.

I realise that the existing code just casts-away the endianness
annotation, but this patch adds a Sparse warning.

 .../brcmsmac/dma.c:289:66: warning: incorrect type in argument 1 (different base types)
 .../brcmsmac/dma.c:289:66:    expected unsigned long long [usertype] val
 .../brcmsmac/dma.c:289:66:    got restricted __le32

>  
>  /* descriptor bumping functions */
> -- 
> 2.34.1
> 
> 

Re: [PATCH v4 02/13] media: media/test_drivers: Replace open-coded parity calculation with parity_odd()

[Hans Verkuil]

On 09/04/2025 17:43, Kuan-Wei Chiu wrote:
> Refactor parity calculations to use the standard parity_odd() helper.
> This change eliminates redundant implementations.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> index 70a4024d461e..5e1b7b1742e4 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c
> @@ -5,6 +5,7 @@
>   * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
>   */
>  
> +#include <linux/bitops.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
>  #include <linux/ktime.h>
> @@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = {
>  
>  static u8 calc_parity(u8 val)
>  {
> -	unsigned i;
> -	unsigned tot = 0;
> -
> -	for (i = 0; i < 7; i++)
> -		tot += (val & (1 << i)) ? 1 : 0;
> -	return val | ((tot & 1) ? 0 : 0x80);
> +	return val | (parity_odd(val) ? 0 : 0x80);
>  }
>  
>  static void vivid_vbi_gen_set_time_of_day(u8 *packet)

Re: [PATCH v4 03/13] media: pci: cx18-av-vbi: Replace open-coded parity calculation with parity_odd()

[Hans Verkuil]

On 09/04/2025 20:59, Kuan-Wei Chiu wrote:
> On Wed, Apr 09, 2025 at 08:43:09PM +0200, Arend van Spriel wrote:
>> On 4/9/2025 5:43 PM, Kuan-Wei Chiu wrote:
>>> Refactor parity calculations to use the standard parity_odd() helper.
>>> This change eliminates redundant implementations.
>>>
>>> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
>>> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
>>> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
>>> ---
>>>   drivers/media/pci/cx18/cx18-av-vbi.c | 12 ++----------
>>>   1 file changed, 2 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/cx18/cx18-av-vbi.c b/drivers/media/pci/cx18/cx18-av-vbi.c
>>> index 65281d40c681..15b515b95956 100644
>>> --- a/drivers/media/pci/cx18/cx18-av-vbi.c
>>> +++ b/drivers/media/pci/cx18/cx18-av-vbi.c
>>
>> [...]
>>
>>> @@ -278,7 +270,7 @@ int cx18_av_decode_vbi_line(struct v4l2_subdev *sd,
>>>   		break;
>>>   	case 6:
>>>   		sdid = V4L2_SLICED_CAPTION_525;
>>> -		err = !odd_parity(p[0]) || !odd_parity(p[1]);
>>> +		err = !parity_odd(p[0]) || !parity_odd(p[1]);
>>
>> No need to call parity_odd() twice here. Instead you could do:
>>
>> 		err = !parity_odd(p[0] ^ p[1]);

I prefer the original approach, it is easier to understand. Performance
is not an issue here, clarity of the code is more important.

So for this patch (i.e. this v4 version):

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

>>
>> This is orthogonal to the change to parity_odd() though. More specific to
>> the new parity_odd() you can now do following as parity_odd() argument is
>> u64:
>>
>> 		err = !parity_odd(*(u16 *)p);
>>
>>
> Thanks for the feedback!
> Would you prefer this change to be part of the parity() conversion
> patch, or in a separate one?

Just leave it as-is, as mentioned above.

Regards,

	Hans

> 
> Regards,
> Kuan-Wei

Re: [PATCH v4 04/13] media: saa7115: Replace open-coded parity calculation with parity_odd()

[Hans Verkuil]

On 09/04/2025 17:43, Kuan-Wei Chiu wrote:
> Refactor parity calculations to use the standard parity_odd() helper.
> This change eliminates redundant implementations.
> 
> Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/media/i2c/saa7115.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/saa7115.c b/drivers/media/i2c/saa7115.c
> index a1c71187e773..a7886269dcfc 100644
> --- a/drivers/media/i2c/saa7115.c
> +++ b/drivers/media/i2c/saa7115.c
> @@ -25,6 +25,7 @@
>  
>  #include "saa711x_regs.h"
>  
> +#include <linux/bitops.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -664,15 +665,6 @@ static const unsigned char saa7115_init_misc[] = {
>  	0x00, 0x00
>  };
>  
> -static int saa711x_odd_parity(u8 c)
> -{
> -	c ^= (c >> 4);
> -	c ^= (c >> 2);
> -	c ^= (c >> 1);
> -
> -	return c & 1;
> -}
> -
>  static int saa711x_decode_vps(u8 *dst, u8 *p)
>  {
>  	static const u8 biphase_tbl[] = {
> @@ -1227,7 +1219,7 @@ static int saa711x_decode_vbi_line(struct v4l2_subdev *sd, struct v4l2_decode_vb
>  		vbi->type = V4L2_SLICED_TELETEXT_B;
>  		break;
>  	case 4:
> -		if (!saa711x_odd_parity(p[0]) || !saa711x_odd_parity(p[1]))
> +		if (!parity_odd(p[0]) || !parity_odd(p[1]))
>  			return 0;
>  		vbi->type = V4L2_SLICED_CAPTION_525;
>  		break;

Re: [PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[H. Peter Anvin]

On 4/11/25 09:34, Kuan-Wei Chiu wrote:
>>
>> In either case, instead of packing the cascade into one function, make good
>> use of it.
>>
>> In the latter case, __builtin_constant_p() isn't necessary as it puts the
>> onus on the architecture to separate out const and non-const cases, if it
>> matters -- which it doesn't if the architecture simply wants to use
>> __builtin_parity:
>>
>> #define parity8(x)  ((bool) __builtin_parity((u8)(x)))
>> #define parity16(x) ((bool) __builtin_parity((u16)(x)))
>> #define parity32(x) ((bool) __builtin_parity((u32)(x)))
>> #define parity64(x) ((bool) __builtin_parityll((u64)(x)))
>>
>> As stated before, I don't really see that the parity function itself would
>> be very suitable for a generic helper, but if it were to, then using the
>> "standard" macro construct for it would seem to be the better option.
>>
>> (And I would be very much in favor of not open-coding the helper everywhere
>> but to macroize it; effectively creating a C++ template equivalent. It is
>> out of scope for this project, though.)
>>
> IIUC, you prefer using the parity8/16/32/64() interface with
> __builtin_parity(), regardless of whether there are users on the hot
> path?

As a per-architecture opt-in, yes.

	-hpa

Re: [PATCH v4 00/13] Introduce parity_odd() and refactor redundant parity code

[Kuan-Wei Chiu]

On Fri, Apr 25, 2025 at 12:33:21PM -0700, H. Peter Anvin wrote:
> On 4/11/25 09:34, Kuan-Wei Chiu wrote:
> > > 
> > > In either case, instead of packing the cascade into one function, make good
> > > use of it.
> > > 
> > > In the latter case, __builtin_constant_p() isn't necessary as it puts the
> > > onus on the architecture to separate out const and non-const cases, if it
> > > matters -- which it doesn't if the architecture simply wants to use
> > > __builtin_parity:
> > > 
> > > #define parity8(x)  ((bool) __builtin_parity((u8)(x)))
> > > #define parity16(x) ((bool) __builtin_parity((u16)(x)))
> > > #define parity32(x) ((bool) __builtin_parity((u32)(x)))
> > > #define parity64(x) ((bool) __builtin_parityll((u64)(x)))
> > > 
> > > As stated before, I don't really see that the parity function itself would
> > > be very suitable for a generic helper, but if it were to, then using the
> > > "standard" macro construct for it would seem to be the better option.
> > > 
> > > (And I would be very much in favor of not open-coding the helper everywhere
> > > but to macroize it; effectively creating a C++ template equivalent. It is
> > > out of scope for this project, though.)
> > > 
> > IIUC, you prefer using the parity8/16/32/64() interface with
> > __builtin_parity(), regardless of whether there are users on the hot
> > path?
> 
> As a per-architecture opt-in, yes.
> 
I'd prefer to see Yury agree first, otherwise there's a high risk of a
maintainer NAK after the next submission.

Regards,
Kuan-Wei