On 2016年03月27日 21:38, zhaoxiu.zeng wrote:

On 2016/3/27 20:44, Sam Ravnborg wrote:

quoted

Hi Zeng.

Looking through the arch specific implementations of __arch_parity().
Some architectures uses #defines, other uses inline static functions.

Any particular reason that you select one approach over the other
in the different cases?

ia64:
+#define __arch_parity32(x) ((unsigned int) __arch_parity64((x) & 0xfffffffful))
+#define __arch_parity16(x) ((unsigned int) __arch_parity64((x) & 0xfffful))
+#define __arch_parity8(x)  ((unsigned int) __arch_parity64((x) & 0xfful))
+#define __arch_parity4(x)  ((unsigned int) __arch_parity64((x) & 0xful))

tile:
+static inline unsigned int __arch_parity32(unsigned int w)
+{
+	return __builtin_popcount(w) & 1;
+}
+
+static inline unsigned int __arch_parity16(unsigned int w)
+{
+	return __arch_parity32(w & 0xffff);
+}
+
+static inline unsigned int __arch_parity8(unsigned int w)
+{
+	return __arch_parity32(w & 0xff);
+}
+
+static inline unsigned int __arch_parity4(unsigned int w)
+{
+	return __arch_parity32(w & 0xf);
+}

No particular reason, just like the architecture's __arch_hweightN.

quoted

Just two examples.

Adding the parity helpers seems like veny nice simplifications.

A few comments to some of those I looked at.
(I am not subscribed to lkml, so you get it as comments here)

I think the conversion is simple and readable.

quoted

[PATCH 21/31] mtd: use parity16 in ssfdc.c
The original code semes to check that the parity equals the
value of first bit in the address.
This seems lost after the conversion.

The original get_parity return 1 if the number is even, so
if block_address is valid, "block_address & 0x7ff" must be odd.

Make corrections:

The original get_parity return 1 if hweight of the input number is even, so
if block_address is valid, hweight of "block_address & 0x7ff" must be odd.

quoted

[PATCH 20/31] scsi: use parity32 in isci/phy.c
+	if (parity32(phy_cap.all))
  		phy_cap.parity = 1;
Could be written like this - simpler IMO:
	phy_cap.parity = parity32(phy_cap.all);


	Sam

Yes. Thanks!