| Message ID | 20241229101234.2896-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | i3c: introduce and use generic parity helper | expand |
On Sun, 29 Dec 2024 11:12:29 +0100 Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > There are multiple open coded implementations for getting the parity of > a byte in the kernel, even using different approaches. Take the pretty > efficient version from SPD5118 driver and make it generally available by > putting it into the bitops header. As long as there is just one parity > calculation helper, the creation of a distinct 'parity.h' header was > discarded. Also, the usage of hweight8() for architectures having a > popcnt instruction is postponed until a use case within hot paths is > desired. The motivation for this patch is the frequent use of odd parity > in the I3C specification and to simplify drivers there. > > Changes compared to the original SPD5118 version are the addition of > kernel documentation, switching the return type from bool to int, and > renaming the argument of the function. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Tested-by: Guenter Roeck <linux@roeck-us.net> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Acked-by: Yury Norov <yury.norov@gmail.com> > Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com> > Tested-by: Kuan-Wei Chiu <visitorckw@gmail.com> > --- > Changes since v1: > > * renamed from 'get_parity8' to 'parity8' > * use XOR instead of OR in the kdoc example (Thanks, Rasmus, for both) > * rebased to 6.13-rc4 > > include/linux/bitops.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index ba35bbf07798..c1cb53cf2f0f 100644 > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -229,6 +229,37 @@ static inline int get_count_order_long(unsigned long l) > return (int)fls_long(--l); > } > > +/** > + * parity8 - get the parity of an u8 value > + * @value: the value to be examined > + * > + * Determine the parity of the u8 argument. > + * > + * Returns: > + * 0 for even parity, 1 for odd parity I think I'd return 0x80 for even and 0 for odd. That just need the 'magic constant' changing and masking with 0x80. Also rename to parity8_even() - since it returns non-zero for even parity. > + * > + * Note: This function informs you about the current parity. Example to bail > + * out when parity is odd: > + * > + * if (parity8(val) == 1) > + * 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) > + * val ^= BIT(7); That then becomes: val ^= parity8_even(val); which is what a lot of the code seems to want to do. With your definition you could do: val ^= (parity8(val) << 7) ^ 0x80; (and I'm sorry, but IMHO 0x80 is better than BIT(7)) David > + */ > +static inline int parity8(u8 val) > +{ > + /* > + * One explanation of this algorithm: > + * https://funloop.org/codex/problem/parity/README.html > + */ > + val ^= val >> 4; > + return (0x6996 >> (val & 0xf)) & 1; > +} > + > /** > * __ffs64 - find first set bit in a 64 bit word > * @word: The 64 bit word
> > +/** > > + * parity8 - get the parity of an u8 value > > + * @value: the value to be examined > > + * > > + * Determine the parity of the u8 argument. > > + * > > + * Returns: > > + * 0 for even parity, 1 for odd parity > > I think I'd return 0x80 for even and 0 for odd. This would be a very non-intuitive result which makes code more complicated to read. And increases chances that people get it wrong. > That just need the 'magic constant' changing and masking with 0x80. Not all users will have the parity in bit 7, some have it in bit 0, some have it in a different register even. So, returning 0x80 would be a micro-optimization for some cases in a code path which is not hot. > Also rename to parity8_even() - since it returns non-zero for even parity. The name has been agreed on with the maintainer of bitmap.h already. > (and I'm sorry, but IMHO 0x80 is better than BIT(7)) No need to feel sorry, but it is really your HO. We have reasons for switching from 0xXY to BIT() tree-wide. You might want to check commit messages of such changes.
diff --git a/include/linux/bitops.h b/include/linux/bitops.h index ba35bbf07798..c1cb53cf2f0f 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -229,6 +229,37 @@ static inline int get_count_order_long(unsigned long l) return (int)fls_long(--l); } +/** + * parity8 - get the parity of an u8 value + * @value: the value to be examined + * + * Determine the parity of the u8 argument. + * + * Returns: + * 0 for even parity, 1 for odd parity + * + * Note: This function informs you about the current parity. Example to bail + * out when parity is odd: + * + * if (parity8(val) == 1) + * 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) + * val ^= BIT(7); + */ +static inline int parity8(u8 val) +{ + /* + * One explanation of this algorithm: + * https://funloop.org/codex/problem/parity/README.html + */ + val ^= val >> 4; + return (0x6996 >> (val & 0xf)) & 1; +} + /** * __ffs64 - find first set bit in a 64 bit word * @word: The 64 bit word