[PATCH 0/2] staging: media: atomisp: Style fixes for vmem.c

[PATCH 0/2] staging: media: atomisp: Style fixes for vmem.c

[Adrian Barnaś]

Style fixes for pci/hive/isp/css/common/host/vmem.c. First patch
triggers check issues in checkpatch.pl that are fixed in the second one.

Adrian Barnaś (2):
  staging: media: atomisp: Remove typedefs for basic types in vmem.c
  staging:media: atomisp: Whitespaces cleanup in vmem.c

 .../pci/hive_isp_css_common/host/vmem.c       | 111 ++++++------------
 1 file changed, 36 insertions(+), 75 deletions(-)

-- 
2.51.0.318.gd7df087d1a-goog

[PATCH 1/2] staging: media: atomisp: Remove typedefs for basic types in vmem.c

[Adrian Barnaś]

Cleared typedefs hiding unsigned long long type, to align with
kernel coding style.

Signed-off-by: Adrian Barnaś <abarnas@google.com>
---
 .../pci/hive_isp_css_common/host/vmem.c       | 42 +++++++++----------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
index 722b684fbc37..fd640e100591 100644
--- a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
+++ b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
@@ -13,35 +13,33 @@
 #endif
 #include "assert_support.h"
 
-typedef unsigned long long hive_uedge;
-typedef hive_uedge *hive_wide;
 
 /* Copied from SDK: sim_semantics.c */
 
 /* subword bits move like this:         MSB[____xxxx____]LSB -> MSB[00000000xxxx]LSB */
-static inline hive_uedge
-subword(hive_uedge w, unsigned int start, unsigned int end)
+static inline unsigned long long
+subword(unsigned long long w, unsigned int start, unsigned int end)
 {
 	return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
 }
 
 /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
-static inline hive_uedge
-inv_subword(hive_uedge w, unsigned int start, unsigned int end)
+static inline unsigned long long
+inv_subword(unsigned long long w, unsigned int start, unsigned int end)
 {
 	return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) | ((1ULL << start) - 1));
 }
 
-#define uedge_bits (8 * sizeof(hive_uedge))
+#define uedge_bits (8 * sizeof(unsigned long long))
 #define move_lower_bits(target, target_bit, src, src_bit) move_subword(target, target_bit, src, 0, src_bit)
 #define move_upper_bits(target, target_bit, src, src_bit) move_subword(target, target_bit, src, src_bit, uedge_bits)
 #define move_word(target, target_bit, src) move_subword(target, target_bit, src, 0, uedge_bits)
 
 static void
 move_subword(
-    hive_uedge *target,
+    unsigned long long *target,
     unsigned int target_bit,
-    hive_uedge src,
+    unsigned long long src,
     unsigned int src_start,
     unsigned int src_end)
 {
@@ -49,18 +47,18 @@ move_subword(
 	unsigned int start_bit  = target_bit % uedge_bits;
 	unsigned int subword_width = src_end - src_start;
 
-	hive_uedge src_subword = subword(src, src_start, src_end);
+	unsigned long long src_subword = subword(src, src_start, src_end);
 
 	if (subword_width + start_bit > uedge_bits) { /* overlap */
-		hive_uedge old_val1;
-		hive_uedge old_val0 = inv_subword(target[start_elem], start_bit, uedge_bits);
+		unsigned long long old_val1;
+		unsigned long long old_val0 = inv_subword(target[start_elem], start_bit, uedge_bits);
 
 		target[start_elem] = old_val0 | (src_subword << start_bit);
 		old_val1 = inv_subword(target[start_elem + 1], 0,
 				       subword_width + start_bit - uedge_bits);
 		target[start_elem + 1] = old_val1 | (src_subword >> (uedge_bits - start_bit));
 	} else {
-		hive_uedge old_val = inv_subword(target[start_elem], start_bit,
+		unsigned long long old_val = inv_subword(target[start_elem], start_bit,
 						 start_bit + subword_width);
 
 		target[start_elem] = old_val | (src_subword << start_bit);
@@ -69,8 +67,8 @@ move_subword(
 
 static void
 hive_sim_wide_unpack(
-    hive_wide vector,
-    hive_wide elem,
+    unsigned long long *vector,
+    unsigned long long *elem,
     hive_uint elem_bits,
     hive_uint index)
 {
@@ -103,8 +101,8 @@ hive_sim_wide_unpack(
 
 static void
 hive_sim_wide_pack(
-    hive_wide vector,
-    hive_wide elem,
+    unsigned long long *vector,
+    unsigned long long *elem,
     hive_uint elem_bits,
     hive_uint index)
 {
@@ -136,7 +134,7 @@ static void load_vector(
     const t_vmem_elem	*from)
 {
 	unsigned int i;
-	hive_uedge *data;
+	unsigned long long *data;
 	unsigned int size = sizeof(short) * ISP_NWAY;
 
 	VMEM_ARRAY(v, 2 * ISP_NWAY); /* Need 2 vectors to work around vmem hss bug */
@@ -146,9 +144,9 @@ static void load_vector(
 #else
 	hrt_master_port_load(ISP_BAMEM_BASE[ID] + (unsigned long)from, &v[0][0], size);
 #endif
-	data = (hive_uedge *)v;
+	data = (unsigned long long *)v;
 	for (i = 0; i < ISP_NWAY; i++) {
-		hive_uedge elem = 0;
+		unsigned long long elem = 0;
 
 		hive_sim_wide_unpack(data, &elem, ISP_VEC_ELEMBITS, i);
 		to[i] = elem;
@@ -166,10 +164,10 @@ static void store_vector(
 
 	VMEM_ARRAY(v, 2 * ISP_NWAY); /* Need 2 vectors to work around vmem hss bug */
 	//load_vector (&v[1][0], &to[ISP_NWAY]); /* Fetch the next vector, since it will be overwritten. */
-	hive_uedge *data = (hive_uedge *)v;
+	unsigned long long *data = (unsigned long long *)v;
 
 	for (i = 0; i < ISP_NWAY; i++) {
-		hive_sim_wide_pack(data, (hive_wide)&from[i], ISP_VEC_ELEMBITS, i);
+		hive_sim_wide_pack(data, (unsigned long long *)&from[i], ISP_VEC_ELEMBITS, i);
 	}
 	assert(ISP_BAMEM_BASE[ID] != (hrt_address) - 1);
 #if !defined(HRT_MEMORY_ACCESS)
-- 
2.51.0.318.gd7df087d1a-goog

[PATCH 2/2] staging:media: atomisp: Whitespaces cleanup in vmem.c

[Adrian Barnaś]

Whitespaces cleanup to conform with kernel style and improve readability.

Signed-off-by: Adrian Barnaś <abarnas@google.com>
---
 .../pci/hive_isp_css_common/host/vmem.c       | 91 ++++++-------------
 1 file changed, 27 insertions(+), 64 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
index fd640e100591..d8a9aaa7b302 100644
--- a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
+++ b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
@@ -13,19 +13,18 @@
 #endif
 #include "assert_support.h"
 
-
 /* Copied from SDK: sim_semantics.c */
 
 /* subword bits move like this:         MSB[____xxxx____]LSB -> MSB[00000000xxxx]LSB */
-static inline unsigned long long
-subword(unsigned long long w, unsigned int start, unsigned int end)
+static inline unsigned long long subword(unsigned long long w, unsigned int start,
+					 unsigned int end)
 {
 	return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
 }
 
 /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
-static inline unsigned long long
-inv_subword(unsigned long long w, unsigned int start, unsigned int end)
+static inline unsigned long long inv_subword(unsigned long long w, unsigned int start,
+					     unsigned int end)
 {
 	return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) | ((1ULL << start) - 1));
 }
@@ -35,13 +34,8 @@ inv_subword(unsigned long long w, unsigned int start, unsigned int end)
 #define move_upper_bits(target, target_bit, src, src_bit) move_subword(target, target_bit, src, src_bit, uedge_bits)
 #define move_word(target, target_bit, src) move_subword(target, target_bit, src, 0, uedge_bits)
 
-static void
-move_subword(
-    unsigned long long *target,
-    unsigned int target_bit,
-    unsigned long long src,
-    unsigned int src_start,
-    unsigned int src_end)
+static void move_subword(unsigned long long *target, unsigned int target_bit,
+			 unsigned long long src, unsigned int src_start, unsigned int src_end)
 {
 	unsigned int start_elem = target_bit / uedge_bits;
 	unsigned int start_bit  = target_bit % uedge_bits;
@@ -51,7 +45,8 @@ move_subword(
 
 	if (subword_width + start_bit > uedge_bits) { /* overlap */
 		unsigned long long old_val1;
-		unsigned long long old_val0 = inv_subword(target[start_elem], start_bit, uedge_bits);
+		unsigned long long old_val0 = inv_subword(target[start_elem],
+							  start_bit, uedge_bits);
 
 		target[start_elem] = old_val0 | (src_subword << start_bit);
 		old_val1 = inv_subword(target[start_elem + 1], 0,
@@ -59,18 +54,14 @@ move_subword(
 		target[start_elem + 1] = old_val1 | (src_subword >> (uedge_bits - start_bit));
 	} else {
 		unsigned long long old_val = inv_subword(target[start_elem], start_bit,
-						 start_bit + subword_width);
+							 start_bit + subword_width);
 
 		target[start_elem] = old_val | (src_subword << start_bit);
 	}
 }
 
-static void
-hive_sim_wide_unpack(
-    unsigned long long *vector,
-    unsigned long long *elem,
-    hive_uint elem_bits,
-    hive_uint index)
+static void hive_sim_wide_unpack(unsigned long long *vector, unsigned long long *elem,
+				 hive_uint elem_bits, hive_uint index)
 {
 	/* pointers into wide_type: */
 	unsigned int start_elem = (elem_bits * index) / uedge_bits;
@@ -99,12 +90,8 @@ hive_sim_wide_unpack(
 	}
 }
 
-static void
-hive_sim_wide_pack(
-    unsigned long long *vector,
-    unsigned long long *elem,
-    hive_uint elem_bits,
-    hive_uint index)
+static void hive_sim_wide_pack(unsigned long long *vector, unsigned long long *elem,
+			       hive_uint elem_bits, hive_uint index)
 {
 	/* pointers into wide_type: */
 	unsigned int start_elem = (elem_bits * index) / uedge_bits;
@@ -128,10 +115,7 @@ hive_sim_wide_pack(
 	}
 }
 
-static void load_vector(
-    const isp_ID_t		ID,
-    t_vmem_elem		*to,
-    const t_vmem_elem	*from)
+static void load_vector(const isp_ID_t ID, t_vmem_elem *to, const t_vmem_elem *from)
 {
 	unsigned int i;
 	unsigned long long *data;
@@ -154,10 +138,7 @@ static void load_vector(
 	udelay(1); /* Spend at least 1 cycles per vector */
 }
 
-static void store_vector(
-    const isp_ID_t		ID,
-    t_vmem_elem		*to,
-    const t_vmem_elem	*from)
+static void store_vector(const isp_ID_t ID, t_vmem_elem *to, const t_vmem_elem *from)
 {
 	unsigned int i;
 	unsigned int size = sizeof(short) * ISP_NWAY;
@@ -166,9 +147,9 @@ static void store_vector(
 	//load_vector (&v[1][0], &to[ISP_NWAY]); /* Fetch the next vector, since it will be overwritten. */
 	unsigned long long *data = (unsigned long long *)v;
 
-	for (i = 0; i < ISP_NWAY; i++) {
+	for (i = 0; i < ISP_NWAY; i++)
 		hive_sim_wide_pack(data, (unsigned long long *)&from[i], ISP_VEC_ELEMBITS, i);
-	}
+
 	assert(ISP_BAMEM_BASE[ID] != (hrt_address) - 1);
 #if !defined(HRT_MEMORY_ACCESS)
 	ia_css_device_store(ISP_BAMEM_BASE[ID] + (unsigned long)to, &v, size);
@@ -179,11 +160,8 @@ static void store_vector(
 	udelay(1); /* Spend at least 1 cycles per vector */
 }
 
-void isp_vmem_load(
-    const isp_ID_t		ID,
-    const t_vmem_elem	*from,
-    t_vmem_elem		*to,
-    unsigned int elems) /* In t_vmem_elem */
+void isp_vmem_load(const isp_ID_t ID, const t_vmem_elem *from, t_vmem_elem *to,
+		   unsigned int elems) /* In t_vmem_elem */
 {
 	unsigned int c;
 	const t_vmem_elem *vp = from;
@@ -197,11 +175,8 @@ void isp_vmem_load(
 	}
 }
 
-void isp_vmem_store(
-    const isp_ID_t		ID,
-    t_vmem_elem		*to,
-    const t_vmem_elem	*from,
-    unsigned int elems) /* In t_vmem_elem */
+void isp_vmem_store(const isp_ID_t ID, t_vmem_elem *to, const t_vmem_elem *from,
+		    unsigned int elems) /* In t_vmem_elem */
 {
 	unsigned int c;
 	t_vmem_elem *vp = to;
@@ -215,15 +190,9 @@ void isp_vmem_store(
 	}
 }
 
-void isp_vmem_2d_load(
-    const isp_ID_t		ID,
-    const t_vmem_elem	*from,
-    t_vmem_elem		*to,
-    unsigned int height,
-    unsigned int width,
-    unsigned int stride_to,  /* In t_vmem_elem */
-
-    unsigned stride_from /* In t_vmem_elem */)
+void isp_vmem_2d_load(const isp_ID_t ID, const t_vmem_elem *from, t_vmem_elem *to,
+		      unsigned int height, unsigned int width,
+		      unsigned int stride_to, unsigned int stride_from) /* In t_vmem_elem */
 {
 	unsigned int h;
 
@@ -244,15 +213,9 @@ void isp_vmem_2d_load(
 	}
 }
 
-void isp_vmem_2d_store(
-    const isp_ID_t		ID,
-    t_vmem_elem		*to,
-    const t_vmem_elem	*from,
-    unsigned int height,
-    unsigned int width,
-    unsigned int stride_to,  /* In t_vmem_elem */
-
-    unsigned stride_from /* In t_vmem_elem */)
+void isp_vmem_2d_store(const isp_ID_t ID, t_vmem_elem *to, const t_vmem_elem *from,
+		       unsigned int height, unsigned int width,
+		       unsigned int stride_to, unsigned int stride_from) /* In t_vmem_elem */
 {
 	unsigned int h;
 
-- 
2.51.0.318.gd7df087d1a-goog

Re: [PATCH 2/2] staging:media: atomisp: Whitespaces cleanup in vmem.c

[Andy Shevchenko]

On Mon, Sep 01, 2025 at 09:10:50AM +0000, Adrian Barnaś wrote:
> Whitespaces cleanup to conform with kernel style and improve readability.

Strange...

...

>  /* subword bits move like this:         MSB[____xxxx____]LSB -> MSB[00000000xxxx]LSB */
> -static inline unsigned long long
> -subword(unsigned long long w, unsigned int start, unsigned int end)
> +static inline unsigned long long subword(unsigned long long w, unsigned int start,
> +					 unsigned int end)
>  {
>  	return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
>  }
>  
>  /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
> -static inline unsigned long long
> -inv_subword(unsigned long long w, unsigned int start, unsigned int end)
> +static inline unsigned long long inv_subword(unsigned long long w, unsigned int start,
> +					     unsigned int end)
>  {
>  	return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) | ((1ULL << start) - 1));
>  }

These two were just "fixed according to the kernel coding style" and here
again. This is odd.

Note, the style after the first patch is okay. I dunno what's wrong with it.

...

> -void isp_vmem_load(
> -    const isp_ID_t		ID,
> -    const t_vmem_elem	*from,
> -    t_vmem_elem		*to,
> -    unsigned int elems) /* In t_vmem_elem */
> +void isp_vmem_load(const isp_ID_t ID, const t_vmem_elem *from, t_vmem_elem *to,
> +		   unsigned int elems) /* In t_vmem_elem */

Please, (re)move trailing comments somewhere else.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] staging: media: atomisp: Remove typedefs for basic types in vmem.c

[Andy Shevchenko]

On Mon, Sep 01, 2025 at 09:10:49AM +0000, Adrian Barnaś wrote:
> Cleared typedefs hiding unsigned long long type, to align with
> kernel coding style.

...

> -static inline hive_uedge
> -subword(hive_uedge w, unsigned int start, unsigned int end)
> +static inline unsigned long long
> +subword(unsigned long long w, unsigned int start, unsigned int end)
>  {
>  	return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
>  }
>  
>  /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
> -static inline hive_uedge
> -inv_subword(hive_uedge w, unsigned int start, unsigned int end)
> +static inline unsigned long long
> +inv_subword(unsigned long long w, unsigned int start, unsigned int end)
>  {
>  	return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) | ((1ULL << start) - 1));
>  }

Also consider to simplify the above (in a separate change).

static inline unsigned long long
subword(unsigned long long w, unsigned int start, unsigned int end)
{
	return (w & GENMASK_ULL(end, 0)) >> start;
}

static inline unsigned long long
inv_subword(unsigned long long w, unsigned int start, unsigned int end)
{
	return w & (~GENMASK_ULL(end, 0) | GENMASK_ULL(start, 0));
}

...if I'm not mistaken, so double check all these.

At least in my case the end == 64 is not allowed while it seems the original
code allows it to be equal to the end == 63 case. Needs testing anyway...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] staging:media: atomisp: Whitespaces cleanup in vmem.c

[Adrian Barnaś]

On Mon, Sep 1, 2025 at 2:23 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Sep 01, 2025 at 09:10:50AM +0000, Adrian Barnaś wrote:
> > Whitespaces cleanup to conform with kernel style and improve readability.
>
> Strange...

You mean the commit description? Should I reword it?

> >  /* subword bits move like this:         MSB[____xxxx____]LSB -> MSB[00000000xxxx]LSB */
> > -static inline unsigned long long
> > -subword(unsigned long long w, unsigned int start, unsigned int end)
> > +static inline unsigned long long subword(unsigned long long w, unsigned int start,
> > +                                      unsigned int end)
> >  {
> >       return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
> >  }
> >
> >  /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
> > -static inline unsigned long long
> > -inv_subword(unsigned long long w, unsigned int start, unsigned int end)
> > +static inline unsigned long long inv_subword(unsigned long long w, unsigned int start,
> > +                                          unsigned int end)
> >  {
> >       return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) | ((1ULL << start) - 1));
> >  }
>
> These two were just "fixed according to the kernel coding style" and here
> again. This is odd.
>
> Note, the style after the first patch is okay. I dunno what's wrong with it.

Those were not violate the kernel code style indeed, but it looks more
consistent this way for me.
Should I revert those?

> ...
>
> > -void isp_vmem_load(
> > -    const isp_ID_t           ID,
> > -    const t_vmem_elem        *from,
> > -    t_vmem_elem              *to,
> > -    unsigned int elems) /* In t_vmem_elem */
> > +void isp_vmem_load(const isp_ID_t ID, const t_vmem_elem *from, t_vmem_elem *to,
> > +                unsigned int elems) /* In t_vmem_elem */
>
> Please, (re)move trailing comments somewhere else.

Those comments are also in header so in my opinion we could get rid of
them here.

Thank you for a review
Adrian Barnaś

Re: [PATCH 1/2] staging: media: atomisp: Remove typedefs for basic types in vmem.c

[Adrian Barnaś]

On Mon, Sep 1, 2025 at 2:35 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Sep 01, 2025 at 09:10:49AM +0000, Adrian Barnaś wrote:
> > Cleared typedefs hiding unsigned long long type, to align with
> > kernel coding style.
>
> ...
>
> > -static inline hive_uedge
> > -subword(hive_uedge w, unsigned int start, unsigned int end)
> > +static inline unsigned long long
> > +subword(unsigned long long w, unsigned int start, unsigned int end)
> >  {
> >       return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
> >  }
> >
> >  /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
> > -static inline hive_uedge
> > -inv_subword(hive_uedge w, unsigned int start, unsigned int end)
> > +static inline unsigned long long
> > +inv_subword(unsigned long long w, unsigned int start, unsigned int end)
> >  {
> >       return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) | ((1ULL << start) - 1));
> >  }
>
> Also consider to simplify the above (in a separate change).
>
> static inline unsigned long long
> subword(unsigned long long w, unsigned int start, unsigned int end)
> {
>         return (w & GENMASK_ULL(end, 0)) >> start;
> }
>
> static inline unsigned long long
> inv_subword(unsigned long long w, unsigned int start, unsigned int end)
> {
>         return w & (~GENMASK_ULL(end, 0) | GENMASK_ULL(start, 0));
> }
>
> ...if I'm not mistaken, so double check all these.
>
> At least in my case the end == 64 is not allowed while it seems the original
> code allows it to be equal to the end == 63 case. Needs testing anyway...

Those functions works odd:
when (end = 8, start = 0) it affects bits 0...7

This should make the same results, will check twice if i not missed
anything and post v2:

static inline unsigned long long _subword(unsigned long long w,
unsigned int start,
unsigned int end)
{
return (w & GENMASK_ULL(end-1, 0)) >> start;
}

static inline unsigned long long _inv_subword(unsigned long long w,
unsigned int start,
unsigned int end)
{
return w & (~GENMASK_ULL(end-1, start));
}


Thank you for a review
Adrian Barnaś

Re: [PATCH 2/2] staging:media: atomisp: Whitespaces cleanup in vmem.c

[Andy Shevchenko]

On Mon, Sep 01, 2025 at 02:46:47PM +0200, Adrian Barnaś wrote:
> On Mon, Sep 1, 2025 at 2:23 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Sep 01, 2025 at 09:10:50AM +0000, Adrian Barnaś wrote:
> > > Whitespaces cleanup to conform with kernel style and improve readability.

> > Strange...
> 
> You mean the commit description? Should I reword it?

No, the case I explained below.

> > Note, the style after the first patch is okay. I dunno what's wrong with it.
> 
> Those were not violate the kernel code style indeed, but it looks more
> consistent this way for me.
> Should I revert those?

If you are talking about the second patch changes in the same functions, yes,
please, keep them untouched _after_ the patch 1.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] staging: media: atomisp: Remove typedefs for basic types in vmem.c

[Andy Shevchenko]

On Mon, Sep 01, 2025 at 03:27:19PM +0200, Adrian Barnaś wrote:
> On Mon, Sep 1, 2025 at 2:35 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Sep 01, 2025 at 09:10:49AM +0000, Adrian Barnaś wrote:

...

> > > -static inline hive_uedge
> > > -subword(hive_uedge w, unsigned int start, unsigned int end)
> > > +static inline unsigned long long
> > > +subword(unsigned long long w, unsigned int start, unsigned int end)
> > >  {
> > >       return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
> > >  }
> > >
> > >  /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
> > > -static inline hive_uedge
> > > -inv_subword(hive_uedge w, unsigned int start, unsigned int end)
> > > +static inline unsigned long long
> > > +inv_subword(unsigned long long w, unsigned int start, unsigned int end)
> > >  {
> > >       return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) | ((1ULL << start) - 1));
> > >  }
> >
> > Also consider to simplify the above (in a separate change).
> >
> > static inline unsigned long long
> > subword(unsigned long long w, unsigned int start, unsigned int end)
> > {
> >         return (w & GENMASK_ULL(end, 0)) >> start;
> > }
> >
> > static inline unsigned long long
> > inv_subword(unsigned long long w, unsigned int start, unsigned int end)
> > {
> >         return w & (~GENMASK_ULL(end, 0) | GENMASK_ULL(start, 0));
> > }
> >
> > ...if I'm not mistaken, so double check all these.
> >
> > At least in my case the end == 64 is not allowed while it seems the original
> > code allows it to be equal to the end == 63 case. Needs testing anyway...
> 
> Those functions works odd:
> when (end = 8, start = 0) it affects bits 0...7

Yes, that's what I meant. But it does the same for 7, 0.
That's why every caller needs to be carefully checked for these corner cases.

64 is special because it will give complete garbage in my case.
I suspect they never extracting anything on the non-aligned byte borders.

> This should make the same results, will check twice if i not missed
> anything and post v2:
> 
> static inline unsigned long long _subword(unsigned long long w,
> unsigned int start,
> unsigned int end)
> {
> return (w & GENMASK_ULL(end-1, 0)) >> start;
> }
> 
> static inline unsigned long long _inv_subword(unsigned long long w,
> unsigned int start,
> unsigned int end)
> {
> return w & (~GENMASK_ULL(end-1, start));
> }

Maybe, but again needs to be carefully checked and tested.

You can propose a separate patch as RFC.

-- 
With Best Regards,
Andy Shevchenko