| Message ID | 20241214085833.8695-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | add generic parity calculation for u8 | expand |
On 12/14/24 00:58, Wolfram Sang 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. > Curious: Why not bool ? Thanks, Guenter
On 12/14/24 06:19, Guenter Roeck wrote: > On 12/14/24 00:58, Wolfram Sang 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. >> > > Curious: Why not bool ? > Never mind. It returns the parity, after all, not "parity is odd". Thanks, Guenter
On Sat, Dec 14, 2024 at 09:58:31AM +0100, Wolfram Sang 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> > --- > include/linux/bitops.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index ba35bbf07798..4ed430934ffc 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); > } > > +/** > + * get_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 (get_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 (get_parity8(val) == 0) > + * val |= BIT(7); > + */ > +static inline int get_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 > -- > 2.45.2 > >
> > Curious: Why not bool ? > > Never mind. It returns the parity, after all, not "parity is odd". Yes, this was my reasoning.
On Sat, Dec 14, 2024 at 9:59 AM 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> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Sat, Dec 14, 2024 at 09:58:31AM +0100, Wolfram Sang 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> Acked-by: Yury Norov <yury.norov@gmail.com> Would you like me to move this patch in bitmap-for-next? > --- > include/linux/bitops.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index ba35bbf07798..4ed430934ffc 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); > } > > +/** > + * get_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 (get_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 (get_parity8(val) == 0) > + * val |= BIT(7); > + */ > +static inline int get_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 > -- > 2.45.2
On Mon, Dec 16, 2024 at 06:24:43PM -0800, Yury Norov wrote: > On Sat, Dec 14, 2024 at 09:58:31AM +0100, Wolfram Sang 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> > > Acked-by: Yury Norov <yury.norov@gmail.com> Cool, thank you! > Would you like me to move this patch in bitmap-for-next? I hope that both patches can be applied in one go to avoid a dependency. I'd think the hwmon-tree is a tad more suitable, but I am also fine with bitmap as long as both patches go in. What do you maintainers thing?
On Sat, Dec 14, 2024 at 09:58:31AM +0100, Wolfram Sang 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> Compare the results with hweight8(x) & 1 for correctness when x is in the range [0, 255]. Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com> Tested-by: Kuan-Wei Chiu <visitorckw@gmail.com> Regards, Kuan-Wei
> Compare the results with hweight8(x) & 1 for correctness when x is in > the range [0, 255]. Thank you!
> I hope that both patches can be applied in one go to avoid a dependency. > I'd think the hwmon-tree is a tad more suitable, but I am also fine with > bitmap as long as both patches go in. What do you maintainers thing? Second thought, we can ask I3C to take it. Together with the cleanups for the I3C drivers I would add then. If this is not to your likings, then an immutable branch for I3C to pull in would be helpful, though.
On 12/17/24 00:41, Wolfram Sang wrote: > >> I hope that both patches can be applied in one go to avoid a dependency. >> I'd think the hwmon-tree is a tad more suitable, but I am also fine with >> bitmap as long as both patches go in. What do you maintainers thing? > > Second thought, we can ask I3C to take it. Together with the cleanups > for the I3C drivers I would add then. > > If this is not to your likings, then an immutable branch for I3C to pull > in would be helpful, though. > Anything is fine with me. No need for an immutable branch from my perspective. Guenter
diff --git a/include/linux/bitops.h b/include/linux/bitops.h index ba35bbf07798..4ed430934ffc 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); } +/** + * get_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 (get_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 (get_parity8(val) == 0) + * val |= BIT(7); + */ +static inline int get_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
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> --- include/linux/bitops.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)