[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


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH RFT v2 1/5] bitops: add generic parity calculation for u8

[Wolfram Sang]

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
+ *
+ * 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
-- 
2.39.2


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[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


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH RFT v2 3/5] i3c: dw: use get_parity8 helper instead of open coding it

[Wolfram Sang]

The kernel has now a generic helper for getting parity with easier to
understand semantics. Make use of it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since v1:

* renamed from 'get_parity8' to 'parity8'
* rebased to 6.13-rc4

 drivers/i3c/master/dw-i3c-master.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index d4b80eb8cecd..0d4d44458c11 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -251,14 +251,6 @@ struct dw_i3c_i2c_dev_data {
 	struct i3c_generic_ibi_pool *ibi_pool;
 };
 
-static u8 even_parity(u8 p)
-{
-	p ^= p >> 4;
-	p &= 0xf;
-
-	return (0x9669 >> p) & 1;
-}
-
 static bool dw_i3c_master_supports_ccc_cmd(struct i3c_master_controller *m,
 					   const struct i3c_ccc_cmd *cmd)
 {
@@ -848,7 +840,7 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 	struct dw_i3c_xfer *xfer;
 	struct dw_i3c_cmd *cmd;
 	u32 olddevs, newdevs;
-	u8 p, last_addr = 0;
+	u8 last_addr = 0;
 	int ret, pos;
 
 	ret = pm_runtime_resume_and_get(master->dev);
@@ -873,9 +865,9 @@ static int dw_i3c_master_daa(struct i3c_master_controller *m)
 		}
 
 		master->devs[pos].addr = ret;
-		p = even_parity(ret);
 		last_addr = ret;
-		ret |= (p << 7);
+
+		ret |= parity8(ret) ? 0 : BIT(7);
 
 		writel(DEV_ADDR_TABLE_DYNAMIC_ADDR(ret),
 		       master->regs +
-- 
2.39.2


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[Wolfram Sang]

The kernel has now a generic helper for getting parity with easier to
understand semantics. Make use of it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since v1:

* renamed from 'get_parity8' to 'parity8'
* rebased to 6.13-rc4

 drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
index 47b9b4d4ed3f..85c4916972e4 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
@@ -40,15 +40,6 @@
 #define dat_w0_write(i, v)	writel(v, hci->DAT_regs + (i) * 8)
 #define dat_w1_write(i, v)	writel(v, hci->DAT_regs + (i) * 8 + 4)
 
-static inline bool dynaddr_parity(unsigned int addr)
-{
-	addr |= 1 << 7;
-	addr += addr >> 4;
-	addr += addr >> 2;
-	addr += addr >> 1;
-	return (addr & 1);
-}
-
 static int hci_dat_v1_init(struct i3c_hci *hci)
 {
 	unsigned int dat_idx;
@@ -123,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) |
-		  (dynaddr_parity(address) ? DAT_0_DYNADDR_PARITY : 0);
+		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);
 	dat_w0_write(dat_idx, dat_w0);
 }
 
-- 
2.39.2


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH RFT v2 5/5] i3c: cdns: use get_parity8 helper instead of open coding it

[Wolfram Sang]

The kernel has now a generic helper for getting parity with easier to
understand semantics. Make use of it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since v1:

* renamed from 'get_parity8' to 'parity8'
* rebased to 6.13-rc4

 drivers/i3c/master/i3c-master-cdns.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
index 06c0592487d3..fedbe6624a1c 100644
--- a/drivers/i3c/master/i3c-master-cdns.c
+++ b/drivers/i3c/master/i3c-master-cdns.c
@@ -889,8 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr)
 	ret |= (addr & GENMASK(9, 7)) << 6;
 
 	/* RR0[0] = ~XOR(addr[6:0]) */
-	if (!(hweight8(addr & 0x7f) & 1))
-		ret |= 1;
+	ret |= parity8(addr & 0x7f) ? 0 : BIT(0);
 
 	return ret;
 }
-- 
2.39.2


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 5/5] i3c: cdns: use get_parity8 helper instead of open coding it

[Geert Uytterhoeven]

Hi Wolfram,

On Sun, Dec 29, 2024 at 11:13 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> The kernel has now a generic helper for getting parity with easier to
> understand semantics. Make use of it.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
> @@ -889,8 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr)
>         ret |= (addr & GENMASK(9, 7)) << 6;
>
>         /* RR0[0] = ~XOR(addr[6:0]) */
> -       if (!(hweight8(addr & 0x7f) & 1))
> -               ret |= 1;
> +       ret |= parity8(addr & 0x7f) ? 0 : BIT(0);

Perhaps keep the if()-construct, to better match the example in the
documentation in 1/5?

>
>         return ret;
>  }

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 1/5] bitops: add generic parity calculation for u8

[David Laight]

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


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 5/5] i3c: cdns: use get_parity8 helper instead of open coding it

[David Laight]

On Sun, 29 Dec 2024 11:49:55 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Wolfram,
> 
> On Sun, Dec 29, 2024 at 11:13 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > The kernel has now a generic helper for getting parity with easier to
> > understand semantics. Make use of it.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>  
> 
> Thanks for your patch!
> 
> > --- a/drivers/i3c/master/i3c-master-cdns.c
> > +++ b/drivers/i3c/master/i3c-master-cdns.c
> > @@ -889,8 +889,7 @@ static u32 prepare_rr0_dev_address(u32 addr)
> >         ret |= (addr & GENMASK(9, 7)) << 6;
> >
> >         /* RR0[0] = ~XOR(addr[6:0]) */
> > -       if (!(hweight8(addr & 0x7f) & 1))
> > -               ret |= 1;
> > +       ret |= parity8(addr & 0x7f) ? 0 : BIT(0);  
> 
> Perhaps keep the if()-construct, to better match the example in the
> documentation in 1/5?

That line is hard to read, with parity8() returning 1 for 'odd' it could be:
	ret |= parity8(addr & 0x7f) ^ 1;
But:
	if (!parity8(addr & 0x7f))
		ret |= 1;
is probably easier to read.
But I'd change the name to parity8_odd() for clarity.
(or _even and return 0x80 for even)

	David

> 
> >
> >         return ret;
> >  }  
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[David Laight]

On Sun, 29 Dec 2024 11:12:32 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> The kernel has now a generic helper for getting parity with easier to
> understand semantics. Make use of it.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> Changes since v1:
> 
> * renamed from 'get_parity8' to 'parity8'
> * rebased to 6.13-rc4
> 
>  drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> index 47b9b4d4ed3f..85c4916972e4 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> @@ -40,15 +40,6 @@
>  #define dat_w0_write(i, v)	writel(v, hci->DAT_regs + (i) * 8)
>  #define dat_w1_write(i, v)	writel(v, hci->DAT_regs + (i) * 8 + 4)
>  
> -static inline bool dynaddr_parity(unsigned int addr)
> -{
> -	addr |= 1 << 7;
> -	addr += addr >> 4;
> -	addr += addr >> 2;
> -	addr += addr >> 1;
> -	return (addr & 1);
> -}
> -
>  static int hci_dat_v1_init(struct i3c_hci *hci)
>  {
>  	unsigned int dat_idx;
> @@ -123,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) |
> -		  (dynaddr_parity(address) ? DAT_0_DYNADDR_PARITY : 0);
> +		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);

NAK - that isn't the same code at all.

	David

>  	dat_w0_write(dat_idx, dat_w0);
>  }
>  


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 1/5] bitops: add generic parity calculation for u8

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1095 bytes --]


> > +/**
> > + * 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.


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

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 661 bytes --]


> > @@ -123,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) |
> > -		  (dynaddr_parity(address) ? DAT_0_DYNADDR_PARITY : 0);
> > +		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);
> 
> NAK - that isn't the same code at all.

But the same algorithm? Please elaborate where you think the new code
will fail compared to the old one. And frankly, are you aware of
different parity calculations? Have you read the link which was in the
kdocs of my new function?


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

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 5/5] i3c: cdns: use get_parity8 helper instead of open coding it

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 427 bytes --]


> >         /* RR0[0] = ~XOR(addr[6:0]) */
> > -       if (!(hweight8(addr & 0x7f) & 1))
> > -               ret |= 1;
> > +       ret |= parity8(addr & 0x7f) ? 0 : BIT(0);
> 
> Perhaps keep the if()-construct, to better match the example in the
> documentation in 1/5?

Can do, don't care super much. I chose this construct because the other
drivers in I3C use the above pattern. You still like the if() better?


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

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 5/5] i3c: cdns: use get_parity8 helper instead of open coding it

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Jan 2, 2025 at 10:04 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > >         /* RR0[0] = ~XOR(addr[6:0]) */
> > > -       if (!(hweight8(addr & 0x7f) & 1))
> > > -               ret |= 1;
> > > +       ret |= parity8(addr & 0x7f) ? 0 : BIT(0);
> >
> > Perhaps keep the if()-construct, to better match the example in the
> > documentation in 1/5?
>
> Can do, don't care super much. I chose this construct because the other
> drivers in I3C use the above pattern. You still like the if() better?

Let's add more bike-shedding in 2025 ;-)

There's also a general dislike for the ternary operator (BTW, I do
agree it has its uses), especially if one of the paths is a no-op,
like ORing with zero.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 5/5] i3c: cdns: use get_parity8 helper instead of open coding it

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 293 bytes --]


> Let's add more bike-shedding in 2025 ;-)

Yaaay... :/

> There's also a general dislike for the ternary operator (BTW, I do
> agree it has its uses), especially if one of the paths is a no-op,
> like ORing with zero.

Well, let's see what Alexandre's view is on this. I'll switch to that.


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

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[Jarkko Nikula]

On 12/29/24 12:12 PM, Wolfram Sang wrote:
> The kernel has now a generic helper for getting parity with easier to
> understand semantics. Make use of it.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> Changes since v1:
> 
> * renamed from 'get_parity8' to 'parity8'
> * rebased to 6.13-rc4
> 
>   drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
> 
Please rename s/get_parity8/parity8/ also in the subject line of patches 
3-5.

For this patch:

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 211 bytes --]


> Please rename s/get_parity8/parity8/ also in the subject line of patches
> 3-5.

Oops, yes, will fix!

> For this patch:
> 
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Thank you!


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

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[David Laight]

On Thu, 2 Jan 2025 10:01:48 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> > > @@ -123,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) |
> > > -		  (dynaddr_parity(address) ? DAT_0_DYNADDR_PARITY : 0);
> > > +		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);  
> > 
> > NAK - that isn't the same code at all.  
> 
> But the same algorithm? Please elaborate where you think the new code
> will fail compared to the old one. And frankly, are you aware of
> different parity calculations? Have you read the link which was in the
> kdocs of my new function?
> 

The old code is:
> -static inline bool dynaddr_parity(unsigned int addr)
> -{
> -	addr |= 1 << 7;
> -	addr += addr >> 4;
> -	addr += addr >> 2;
> -	addr += addr >> 1;
> -	return (addr & 1);
> -}

So:
1) it always sets 0x80.
2) it uses addition not exclusive or.

So just not the same definition of 'parity'.

	David

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 931 bytes --]


> > > > -		  (dynaddr_parity(address) ? DAT_0_DYNADDR_PARITY : 0);
> > > > +		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);  

...

> The old code is:
> > -static inline bool dynaddr_parity(unsigned int addr)
> > -{
> > -	addr |= 1 << 7;
> > -	addr += addr >> 4;
> > -	addr += addr >> 2;
> > -	addr += addr >> 1;
> > -	return (addr & 1);
> > -}
> 
> So:
> 1) it always sets 0x80.

Right, this is why the arguments of the ternary operator above are
exchanged. The old function was basically 'is_odd'.

> 2) it uses addition not exclusive or.

True, but it will work nonetheless because we are only interested in bit
0 of the result. For one bit, XOR and addition are interchangable. The
overflow to other bits is not important.

> So just not the same definition of 'parity'.

I think it is. I mean, I3C wants odd parity, otherwise it will not work.
And Jarkko kindly confirmed it still works.


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

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[David Laight]

On Fri, 3 Jan 2025 11:02:30 +0100
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:

> > > > > -		  (dynaddr_parity(address) ? DAT_0_DYNADDR_PARITY : 0);
> > > > > +		  (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);    
> 
> ...
> 
> > The old code is:  
> > > -static inline bool dynaddr_parity(unsigned int addr)
> > > -{
> > > -	addr |= 1 << 7;
> > > -	addr += addr >> 4;
> > > -	addr += addr >> 2;
> > > -	addr += addr >> 1;
> > > -	return (addr & 1);
> > > -}  
> > 
> > So:
> > 1) it always sets 0x80.  
> 
> Right, this is why the arguments of the ternary operator above are
> exchanged. The old function was basically 'is_odd'.

Provided the high bit isn't already set - which it may not be.

> > 2) it uses addition not exclusive or.  
> 
> True, but it will work nonetheless because we are only interested in bit
> 0 of the result. For one bit, XOR and addition are interchangable. The
> overflow to other bits is not important.

add: 00010001 => xxxx0010 => xx10 => x1
xor: 00010001 => xxxx0000 => 00xx => x0

> 
> > So just not the same definition of 'parity'.  
> 
> I think it is. I mean, I3C wants odd parity, otherwise it will not work.
> And Jarkko kindly confirmed it still works.

I bet the target isn't checking...

So you might be fixing a bug.

	David



-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 4/5] i3c: mipi-i3c-hci: use get_parity8 helper instead of open coding it

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 581 bytes --]


> > Right, this is why the arguments of the ternary operator above are
> > exchanged. The old function was basically 'is_odd'.
> 
> Provided the high bit isn't already set - which it may not be.

Not here. Temporary I3C addresses are in the range 0-0x7f.

> add: 00010001 => xxxx0010 => xx10 => x1
> xor: 00010001 => xxxx0000 => 00xx => x0

This point goes to you :)

> I bet the target isn't checking...

Could be, I can't tell. I don't have this HW.

> So you might be fixing a bug.

Heh, which better argument could one have for a generic implementation.


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

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH RFT v2 5/5] i3c: cdns: use get_parity8 helper instead of open coding it

[Alexandre Belloni]

On 02/01/2025 10:34:05+0100, Wolfram Sang wrote:
> 
> > Let's add more bike-shedding in 2025 ;-)
> 
> Yaaay... :/
> 
> > There's also a general dislike for the ternary operator (BTW, I do
> > agree it has its uses), especially if one of the paths is a no-op,
> > like ORing with zero.
> 
> Well, let's see what Alexandre's view is on this. I'll switch to that.
> 

I'm fine with the ternary operator.

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Wolfram Sang]

[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Wolfram Sang]

[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c