[PATCH RFT v2 0/5] i3c: introduce and use generic parity helper

[PATCH RFT v2 0/5] i3c: introduce and use generic parity helper

[Wolfram Sang]

Changes since v1:

* renamed from 'get_parity8' to 'parity8'
* use XOR instead of OR in the kdoc example (Thanks, Rasmus, for both)
* added tag from Guenter (thanks!)
* rebased to 6.13-rc4

Old coverletter follows:

I am currently working on upstreaming another I3C controller driver. As
many others, it needs to ensure odd parity for a dynamically assigned
address. The BSP version of the driver implemented a custom parity
algorithm. Wondering why we don't have a generic helper for this in the
kernel, I found that many I3C controller drivers all implement their
version of handling parity.

So, I sent out an RFC[1] moving the efficient implementation of the
SPD5118 driver to a generic location. The series was well received, but
the path for upstream was not clear. Because I need the implementation
for my I3C controller driver and I3C is a prominent user of this new
function, I got the idea of converting the existing I3C drivers and
resend the series, suggesting this all goes upstream via I3C.

Is this acceptable? The non-I3C patches have all the tags they need,
only the I3C patches would need testing on hardware.

A build-tested (incl. build-bots) branch is here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i3c/get_parity

Looking forward to comments...

[1] https://lore.kernel.org/all/20241214085833.8695-1-wsa+renesas@sang-engineering.com/



Wolfram Sang (5):
  bitops: add generic parity calculation for u8
  hwmon: (spd5118) Use generic parity calculation
  i3c: dw: use get_parity8 helper instead of open coding it
  i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it
  i3c: cdns: use get_parity8 helper instead of open coding it

 drivers/hwmon/spd5118.c                  |  8 +-----
 drivers/i3c/master/dw-i3c-master.c       | 14 +++--------
 drivers/i3c/master/i3c-master-cdns.c     |  3 +--
 drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 11 +--------
 include/linux/bitops.h                   | 31 ++++++++++++++++++++++++
 5 files changed, 37 insertions(+), 30 deletions(-)

-- 
2.39.2

[PATCH RFT v2 2/5] hwmon: (spd5118) Use generic parity calculation

[Wolfram Sang]

Make use of the new generic helper for calculating the parity.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
Changes since v1:

* renamed from 'get_parity8' to 'parity8'
* added tag from Guenter (thanks!)
* rebased to 6.13-rc4

 drivers/hwmon/spd5118.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 6cee48a3e5c3..358152868d96 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -291,12 +291,6 @@ static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types typ
 	}
 }
 
-static inline bool spd5118_parity8(u8 w)
-{
-	w ^= w >> 4;
-	return (0x6996 >> (w & 0xf)) & 1;
-}
-
 /*
  * Bank and vendor id are 8-bit fields with seven data bits and odd parity.
  * Vendor IDs 0 and 0x7f are invalid.
@@ -304,7 +298,7 @@ static inline bool spd5118_parity8(u8 w)
  */
 static bool spd5118_vendor_valid(u8 bank, u8 id)
 {
-	if (!spd5118_parity8(bank) || !spd5118_parity8(id))
+	if (parity8(bank) == 0 || parity8(id) == 0)
 		return false;
 
 	id &= 0x7f;
-- 
2.39.2

Re: [PATCH RFT v2 0/5] i3c: introduce and use generic parity helper

[Alexandre Belloni]

On 29/12/2024 11:12:28+0100, Wolfram Sang wrote:
> Changes since v1:
> 
> * renamed from 'get_parity8' to 'parity8'
> * use XOR instead of OR in the kdoc example (Thanks, Rasmus, for both)
> * added tag from Guenter (thanks!)
> * rebased to 6.13-rc4
> 
> Old coverletter follows:
> 
> I am currently working on upstreaming another I3C controller driver. As
> many others, it needs to ensure odd parity for a dynamically assigned
> address. The BSP version of the driver implemented a custom parity
> algorithm. Wondering why we don't have a generic helper for this in the
> kernel, I found that many I3C controller drivers all implement their
> version of handling parity.
> 
> So, I sent out an RFC[1] moving the efficient implementation of the
> SPD5118 driver to a generic location. The series was well received, but
> the path for upstream was not clear. Because I need the implementation
> for my I3C controller driver and I3C is a prominent user of this new
> function, I got the idea of converting the existing I3C drivers and
> resend the series, suggesting this all goes upstream via I3C.
> 
> Is this acceptable? The non-I3C patches have all the tags they need,
> only the I3C patches would need testing on hardware.
> 
> A build-tested (incl. build-bots) branch is here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i3c/get_parity
> 
> Looking forward to comments...
> 
> [1] https://lore.kernel.org/all/20241214085833.8695-1-wsa+renesas@sang-engineering.com/
> 

I'll apply the series once you get some agreement on the function name.
I don't care to much but the _odd suggestion seems wise to me.

> 
> 
> Wolfram Sang (5):
>   bitops: add generic parity calculation for u8
>   hwmon: (spd5118) Use generic parity calculation
>   i3c: dw: use get_parity8 helper instead of open coding it
>   i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it
>   i3c: cdns: use get_parity8 helper instead of open coding it
> 
>  drivers/hwmon/spd5118.c                  |  8 +-----
>  drivers/i3c/master/dw-i3c-master.c       | 14 +++--------
>  drivers/i3c/master/i3c-master-cdns.c     |  3 +--
>  drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 11 +--------
>  include/linux/bitops.h                   | 31 ++++++++++++++++++++++++
>  5 files changed, 37 insertions(+), 30 deletions(-)
> 
> -- 
> 2.39.2
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH RFT v2 0/5] i3c: introduce and use generic parity helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]


> I'll apply the series once you get some agreement on the function name.

Thanks!

As said in that thread, the function name has already been changed in v2
to the liking of the bitmap.h maintainer (Rasmus) [1]. He has not
responded to this series yet, though.

[1] https://lore.kernel.org/r/CAKwiHFiamZ7FgS3wbyLHo6n6R136LrLVCsih0w+spG55BPxy8g@mail.gmail.com


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFT v2 0/5] i3c: introduce and use generic parity helper

[Yury Norov]

On Fri, Jan 03, 2025 at 11:16:35PM +0100, Wolfram Sang wrote:
> 
> > I'll apply the series once you get some agreement on the function name.
> 
> Thanks!
> 
> As said in that thread, the function name has already been changed in v2
> to the liking of the bitmap.h maintainer (Rasmus) [1]. He has not
> responded to this series yet, though.
> 
> [1] https://lore.kernel.org/r/CAKwiHFiamZ7FgS3wbyLHo6n6R136LrLVCsih0w+spG55BPxy8g@mail.gmail.com
> 

+ Linus Arver

Rasmus is reviewer. I'm - maintainer. But I surely agreed with him.
Shorter name is always better. And we're all kernel developers here,
so we used to read and respect comments. The comment clearly explains
what the function returns.

I'm a bit doubted about adding a web link in sources because it may
become invalid one day, but if Wolfram commits to maintain comments
up-to-date, I'm OK with that.

I already acked the patch, so no need to ack it again.

Thanks,
Yury

Re: [PATCH RFT v2 0/5] i3c: introduce and use generic parity helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

Hi Yury,

> Rasmus is reviewer. I'm - maintainer.

Oh, sorry. I just saw your both entries in MAINTAINERS and missed that
Rasmus has "R:" instead of "M:". No offence!

> I'm a bit doubted about adding a web link in sources because it may
> become invalid one day, but if Wolfram commits to maintain comments
> up-to-date, I'm OK with that.

I just added the page to the wayback machine. We could use this link
now: http://web.archive.org/web/20250105093316/https://funloop.org/codex/problem/parity/README.html

Better?

> I already acked the patch, so no need to ack it again.

Thanks!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]