[PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Mitali Borkar]

Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
This BIT() macro from linux/bitops.h is used to define IPU3_UAPI_GRID_Y_START_EN
and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
Use of macro is better and neater. It maintains consistency.
Reported by checkpatch.

Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
---
 drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index edd8edda0647..589d5ccee3a7 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -5,6 +5,7 @@
 #define __IPU3_UAPI_H
 
 #include <linux/types.h>
+#include <linux/bitops.h>
 
 /* from /drivers/staging/media/ipu3/include/videodev2.h */
 
@@ -22,11 +23,11 @@
 #define IPU3_UAPI_MAX_BUBBLE_SIZE			10
 
 #define IPU3_UAPI_GRID_START_MASK			((1 << 12) - 1)
-#define IPU3_UAPI_GRID_Y_START_EN			(1 << 15)
+#define IPU3_UAPI_GRID_Y_START_EN			BIT(15)
 
 /* controls generation of meta_data (like FF enable/disable) */
-#define IPU3_UAPI_AWB_RGBS_THR_B_EN			(1 << 14)
-#define IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT		(1 << 15)
+#define IPU3_UAPI_AWB_RGBS_THR_B_EN			BIT(14)
+#define IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT		BIT(15)
 
 /**
  * struct ipu3_uapi_grid_config - Grid plane config
-- 
2.30.2

Re: [PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Sakari Ailus]

Hi Mitali,

On Mon, Apr 12, 2021 at 04:38:39AM +0530, Mitali Borkar wrote:
> Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
> This BIT() macro from linux/bitops.h is used to define IPU3_UAPI_GRID_Y_START_EN
> and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
> Use of macro is better and neater. It maintains consistency.
> Reported by checkpatch.
> 
> Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> ---
>  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index edd8edda0647..589d5ccee3a7 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> @@ -5,6 +5,7 @@
>  #define __IPU3_UAPI_H
>  
>  #include <linux/types.h>
> +#include <linux/bitops.h>
>  
>  /* from /drivers/staging/media/ipu3/include/videodev2.h */
>  
> @@ -22,11 +23,11 @@
>  #define IPU3_UAPI_MAX_BUBBLE_SIZE			10
>  
>  #define IPU3_UAPI_GRID_START_MASK			((1 << 12) - 1)
> -#define IPU3_UAPI_GRID_Y_START_EN			(1 << 15)
> +#define IPU3_UAPI_GRID_Y_START_EN			BIT(15)

This header is used in user space where you don't have the BIT() macro.

>  
>  /* controls generation of meta_data (like FF enable/disable) */
> -#define IPU3_UAPI_AWB_RGBS_THR_B_EN			(1 << 14)
> -#define IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT		(1 << 15)
> +#define IPU3_UAPI_AWB_RGBS_THR_B_EN			BIT(14)
> +#define IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT		BIT(15)
>  
>  /**
>   * struct ipu3_uapi_grid_config - Grid plane config

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Greg KH]

On Mon, Apr 12, 2021 at 12:42:30PM +0300, Sakari Ailus wrote:
> Hi Mitali,
> 
> On Mon, Apr 12, 2021 at 04:38:39AM +0530, Mitali Borkar wrote:
> > Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
> > This BIT() macro from linux/bitops.h is used to define IPU3_UAPI_GRID_Y_START_EN
> > and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
> > Use of macro is better and neater. It maintains consistency.
> > Reported by checkpatch.
> > 
> > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > ---
> >  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > index edd8edda0647..589d5ccee3a7 100644
> > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > @@ -5,6 +5,7 @@
> >  #define __IPU3_UAPI_H
> >  
> >  #include <linux/types.h>
> > +#include <linux/bitops.h>
> >  
> >  /* from /drivers/staging/media/ipu3/include/videodev2.h */
> >  
> > @@ -22,11 +23,11 @@
> >  #define IPU3_UAPI_MAX_BUBBLE_SIZE			10
> >  
> >  #define IPU3_UAPI_GRID_START_MASK			((1 << 12) - 1)
> > -#define IPU3_UAPI_GRID_Y_START_EN			(1 << 15)
> > +#define IPU3_UAPI_GRID_Y_START_EN			BIT(15)
> 
> This header is used in user space where you don't have the BIT() macro.

If that is true, why is it not in a "uapi" subdir within this driver?

Otherwise it is not obvious at all that this is the case :(

thanks,

greg k-h

Re: [Outreachy kernel] Re: [PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Julia Lawall]

On Mon, 12 Apr 2021, Greg KH wrote:

> On Mon, Apr 12, 2021 at 12:42:30PM +0300, Sakari Ailus wrote:
> > Hi Mitali,
> >
> > On Mon, Apr 12, 2021 at 04:38:39AM +0530, Mitali Borkar wrote:
> > > Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
> > > This BIT() macro from linux/bitops.h is used to define IPU3_UAPI_GRID_Y_START_EN
> > > and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
> > > Use of macro is better and neater. It maintains consistency.
> > > Reported by checkpatch.
> > >
> > > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > > ---
> > >  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > index edd8edda0647..589d5ccee3a7 100644
> > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > @@ -5,6 +5,7 @@
> > >  #define __IPU3_UAPI_H
> > >
> > >  #include <linux/types.h>
> > > +#include <linux/bitops.h>
> > >
> > >  /* from /drivers/staging/media/ipu3/include/videodev2.h */
> > >
> > > @@ -22,11 +23,11 @@
> > >  #define IPU3_UAPI_MAX_BUBBLE_SIZE			10
> > >
> > >  #define IPU3_UAPI_GRID_START_MASK			((1 << 12) - 1)

Mitali,

It's not very relevant given the use of this header, but if the 1 << 15
translation was correct, you could have changed the 1 << 12 computation as
well.

julia

> > > -#define IPU3_UAPI_GRID_Y_START_EN			(1 << 15)
> > > +#define IPU3_UAPI_GRID_Y_START_EN			BIT(15)
> >
> > This header is used in user space where you don't have the BIT() macro.
>
> If that is true, why is it not in a "uapi" subdir within this driver?
>
> Otherwise it is not obvious at all that this is the case :(
>
> thanks,
>
> greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/YHQXty07oAP1L0W9%40kroah.com.
>

Re: [Outreachy kernel] Re: [PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Julia Lawall]

On Mon, 12 Apr 2021, Greg KH wrote:

> On Mon, Apr 12, 2021 at 12:42:30PM +0300, Sakari Ailus wrote:
> > Hi Mitali,
> >
> > On Mon, Apr 12, 2021 at 04:38:39AM +0530, Mitali Borkar wrote:
> > > Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
> > > This BIT() macro from linux/bitops.h is used to define IPU3_UAPI_GRID_Y_START_EN
> > > and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
> > > Use of macro is better and neater. It maintains consistency.
> > > Reported by checkpatch.
> > >
> > > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > > ---
> > >  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > index edd8edda0647..589d5ccee3a7 100644
> > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > @@ -5,6 +5,7 @@
> > >  #define __IPU3_UAPI_H
> > >
> > >  #include <linux/types.h>
> > > +#include <linux/bitops.h>
> > >
> > >  /* from /drivers/staging/media/ipu3/include/videodev2.h */
> > >
> > > @@ -22,11 +23,11 @@
> > >  #define IPU3_UAPI_MAX_BUBBLE_SIZE			10
> > >
> > >  #define IPU3_UAPI_GRID_START_MASK			((1 << 12) - 1)

Mitali,

It's not very relevant given the use of this header, but if the 1 << 15
translation was correct, you could have changed the 1 << 12 computation as
well.

julia

> > > -#define IPU3_UAPI_GRID_Y_START_EN			(1 << 15)
> > > +#define IPU3_UAPI_GRID_Y_START_EN			BIT(15)
> >
> > This header is used in user space where you don't have the BIT() macro.
>
> If that is true, why is it not in a "uapi" subdir within this driver?
>
> Otherwise it is not obvious at all that this is the case :(
>
> thanks,
>
> greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/YHQXty07oAP1L0W9%40kroah.com.
>

Re: [PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Sakari Ailus]

Hi Greg,

On Mon, Apr 12, 2021 at 11:49:43AM +0200, Greg KH wrote:
> On Mon, Apr 12, 2021 at 12:42:30PM +0300, Sakari Ailus wrote:
> > Hi Mitali,
> > 
> > On Mon, Apr 12, 2021 at 04:38:39AM +0530, Mitali Borkar wrote:
> > > Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
> > > This BIT() macro from linux/bitops.h is used to define IPU3_UAPI_GRID_Y_START_EN
> > > and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
> > > Use of macro is better and neater. It maintains consistency.
> > > Reported by checkpatch.
> > > 
> > > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > > ---
> > >  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > index edd8edda0647..589d5ccee3a7 100644
> > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > @@ -5,6 +5,7 @@
> > >  #define __IPU3_UAPI_H
> > >  
> > >  #include <linux/types.h>
> > > +#include <linux/bitops.h>
> > >  
> > >  /* from /drivers/staging/media/ipu3/include/videodev2.h */
> > >  
> > > @@ -22,11 +23,11 @@
> > >  #define IPU3_UAPI_MAX_BUBBLE_SIZE			10
> > >  
> > >  #define IPU3_UAPI_GRID_START_MASK			((1 << 12) - 1)
> > > -#define IPU3_UAPI_GRID_Y_START_EN			(1 << 15)
> > > +#define IPU3_UAPI_GRID_Y_START_EN			BIT(15)
> > 
> > This header is used in user space where you don't have the BIT() macro.
> 
> If that is true, why is it not in a "uapi" subdir within this driver?
> 
> Otherwise it is not obvious at all that this is the case :(

It defines an interface towards the user space and the argument has been a
staging driver shouldn't be doing that (for the lack of ABI stability),
hence leaving it where it is currently.

Also CC Laurent.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Greg KH]

On Mon, Apr 12, 2021 at 01:44:35PM +0300, Sakari Ailus wrote:
> Hi Greg,
> 
> On Mon, Apr 12, 2021 at 11:49:43AM +0200, Greg KH wrote:
> > On Mon, Apr 12, 2021 at 12:42:30PM +0300, Sakari Ailus wrote:
> > > Hi Mitali,
> > > 
> > > On Mon, Apr 12, 2021 at 04:38:39AM +0530, Mitali Borkar wrote:
> > > > Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
> > > > This BIT() macro from linux/bitops.h is used to define IPU3_UAPI_GRID_Y_START_EN
> > > > and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
> > > > Use of macro is better and neater. It maintains consistency.
> > > > Reported by checkpatch.
> > > > 
> > > > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > > > ---
> > > >  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > index edd8edda0647..589d5ccee3a7 100644
> > > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > @@ -5,6 +5,7 @@
> > > >  #define __IPU3_UAPI_H
> > > >  
> > > >  #include <linux/types.h>
> > > > +#include <linux/bitops.h>
> > > >  
> > > >  /* from /drivers/staging/media/ipu3/include/videodev2.h */
> > > >  
> > > > @@ -22,11 +23,11 @@
> > > >  #define IPU3_UAPI_MAX_BUBBLE_SIZE			10
> > > >  
> > > >  #define IPU3_UAPI_GRID_START_MASK			((1 << 12) - 1)
> > > > -#define IPU3_UAPI_GRID_Y_START_EN			(1 << 15)
> > > > +#define IPU3_UAPI_GRID_Y_START_EN			BIT(15)
> > > 
> > > This header is used in user space where you don't have the BIT() macro.
> > 
> > If that is true, why is it not in a "uapi" subdir within this driver?
> > 
> > Otherwise it is not obvious at all that this is the case :(
> 
> It defines an interface towards the user space and the argument has been a
> staging driver shouldn't be doing that (for the lack of ABI stability),
> hence leaving it where it is currently.

I think we are talking past each other here...

If you have a userspace api, then put that in a .h file that has a
"uapi" in the path.  Just because you have this in a staging driver does
not mean you should not have such a thing, otherwise you are going to
constantly fight against people sending you patches like this as there
is no obvious way to determine this.

So how about moving this file to:
	drivers/staging/media/ipu3/include/uapi/intel-ipu3.h

thanks,

greg k-h

Re: [PATCH 1/6] staging: media: intel-ipu3: replace bit shifts with BIT() macro

[Sakari Ailus]

On Mon, Apr 12, 2021 at 12:55:52PM +0200, Greg KH wrote:
> On Mon, Apr 12, 2021 at 01:44:35PM +0300, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > On Mon, Apr 12, 2021 at 11:49:43AM +0200, Greg KH wrote:
> > > On Mon, Apr 12, 2021 at 12:42:30PM +0300, Sakari Ailus wrote:
> > > > Hi Mitali,
> > > > 
> > > > On Mon, Apr 12, 2021 at 04:38:39AM +0530, Mitali Borkar wrote:
> > > > > Added #include <linux/bitops.h> and replaced bit shifts by BIT() macro.
> > > > > This BIT() macro from linux/bitops.h is used to define IPU3_UAPI_GRID_Y_START_EN
> > > > > and IPU3_UAPI_AWB_RGBS_THR_B_* bitmask.
> > > > > Use of macro is better and neater. It maintains consistency.
> > > > > Reported by checkpatch.
> > > > > 
> > > > > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > > > > ---
> > > > >  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > > index edd8edda0647..589d5ccee3a7 100644
> > > > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > > > @@ -5,6 +5,7 @@
> > > > >  #define __IPU3_UAPI_H
> > > > >  
> > > > >  #include <linux/types.h>
> > > > > +#include <linux/bitops.h>
> > > > >  
> > > > >  /* from /drivers/staging/media/ipu3/include/videodev2.h */
> > > > >  
> > > > > @@ -22,11 +23,11 @@
> > > > >  #define IPU3_UAPI_MAX_BUBBLE_SIZE			10
> > > > >  
> > > > >  #define IPU3_UAPI_GRID_START_MASK			((1 << 12) - 1)
> > > > > -#define IPU3_UAPI_GRID_Y_START_EN			(1 << 15)
> > > > > +#define IPU3_UAPI_GRID_Y_START_EN			BIT(15)
> > > > 
> > > > This header is used in user space where you don't have the BIT() macro.
> > > 
> > > If that is true, why is it not in a "uapi" subdir within this driver?
> > > 
> > > Otherwise it is not obvious at all that this is the case :(
> > 
> > It defines an interface towards the user space and the argument has been a
> > staging driver shouldn't be doing that (for the lack of ABI stability),
> > hence leaving it where it is currently.
> 
> I think we are talking past each other here...
> 
> If you have a userspace api, then put that in a .h file that has a
> "uapi" in the path.  Just because you have this in a staging driver does
> not mean you should not have such a thing, otherwise you are going to
> constantly fight against people sending you patches like this as there
> is no obvious way to determine this.
> 
> So how about moving this file to:
> 	drivers/staging/media/ipu3/include/uapi/intel-ipu3.h

Ah, sure! I'll send a patch.

-- 
Sakari Ailus

[PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[Mitali Borkar]

This patch fixes the warning identified by checkpatch.pl by replacing
__attribute__aligned(size) with __aligned(size)

Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
---
 .../staging/media/ipu3/include/intel-ipu3.h   | 74 +++++++++----------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index 589d5ccee3a7..d95ca9ebfafb 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -84,7 +84,7 @@ struct ipu3_uapi_grid_config {
  */
 struct ipu3_uapi_awb_raw_buffer {
 	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
-		__attribute__((aligned(32)));
+		__aligned(32);
 } __packed;
 
 /**
@@ -105,7 +105,7 @@ struct ipu3_uapi_awb_config_s {
 	__u16 rgbs_thr_gb;
 	__u16 rgbs_thr_b;
 	struct ipu3_uapi_grid_config grid;
-} __attribute__((aligned(32))) __packed;
+}	__aligned(32) __packed;
 
 /**
  * struct ipu3_uapi_awb_config - AWB config wrapper
@@ -113,7 +113,7 @@ struct ipu3_uapi_awb_config_s {
  * @config: config for auto white balance as defined by &ipu3_uapi_awb_config_s
  */
 struct ipu3_uapi_awb_config {
-	struct ipu3_uapi_awb_config_s config __attribute__((aligned(32)));
+	struct ipu3_uapi_awb_config_s config __aligned(32);
 } __packed;
 
 #define IPU3_UAPI_AE_COLORS				4	/* R, G, B, Y */
@@ -138,7 +138,7 @@ struct ipu3_uapi_ae_raw_buffer {
  * @buff: &ipu3_uapi_ae_raw_buffer to hold full frame meta data.
  */
 struct ipu3_uapi_ae_raw_buffer_aligned {
-	struct ipu3_uapi_ae_raw_buffer buff __attribute__((aligned(32)));
+	struct ipu3_uapi_ae_raw_buffer buff __aligned(32);
 } __packed;
 
 /**
@@ -244,10 +244,10 @@ struct ipu3_uapi_ae_ccm {
  * Calculate AE grid from image resolution, resample ae weights.
  */
 struct ipu3_uapi_ae_config {
-	struct ipu3_uapi_ae_grid_config grid_cfg __attribute__((aligned(32)));
+	struct ipu3_uapi_ae_grid_config grid_cfg __aligned(32);
 	struct ipu3_uapi_ae_weight_elem weights[
-			IPU3_UAPI_AE_WEIGHTS] __attribute__((aligned(32)));
-	struct ipu3_uapi_ae_ccm ae_ccm __attribute__((aligned(32)));
+			IPU3_UAPI_AE_WEIGHTS] __aligned(32);
+	struct ipu3_uapi_ae_ccm ae_ccm __aligned(32);
 } __packed;
 
 /**
@@ -389,7 +389,7 @@ struct ipu3_uapi_af_filter_config {
  *		each cell.
  */
 struct ipu3_uapi_af_raw_buffer {
-	__u8 y_table[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE] __attribute__((aligned(32)));
+	__u8 y_table[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE] __aligned(32);
 } __packed;
 
 /**
@@ -402,9 +402,9 @@ struct ipu3_uapi_af_raw_buffer {
  *	      grid size for large image and vice versa.
  */
 struct ipu3_uapi_af_config_s {
-	struct ipu3_uapi_af_filter_config filter_config __attribute__((aligned(32)));
+	struct ipu3_uapi_af_filter_config filter_config __aligned(32);
 	__u8 padding[4];
-	struct ipu3_uapi_grid_config grid_cfg __attribute__((aligned(32)));
+	struct ipu3_uapi_grid_config grid_cfg __aligned(32);
 } __packed;
 
 #define IPU3_UAPI_AWB_FR_MAX_SETS			24
@@ -425,7 +425,7 @@ struct ipu3_uapi_af_config_s {
  */
 struct ipu3_uapi_awb_fr_raw_buffer {
 	__u8 meta_data[IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE]
-		__attribute__((aligned(32)));
+		__aligned(32);
 } __packed;
 
 /**
@@ -463,12 +463,12 @@ struct ipu3_uapi_awb_fr_config_s {
  * @awb_fr_config: &ipu3_uapi_awb_fr_config_s, default resolution 16x16
  */
 struct ipu3_uapi_4a_config {
-	struct ipu3_uapi_awb_config_s awb_config __attribute__((aligned(32)));
+	struct ipu3_uapi_awb_config_s awb_config __aligned(32);
 	struct ipu3_uapi_ae_grid_config ae_grd_config;
 	__u8 padding[20];
 	struct ipu3_uapi_af_config_s af_config;
 	struct ipu3_uapi_awb_fr_config_s awb_fr_config
-		__attribute__((aligned(32)));
+		__aligned(32);
 } __packed;
 
 /**
@@ -487,7 +487,7 @@ struct ipu3_uapi_4a_config {
  * @padding3: padding bytes.
  */
 struct ipu3_uapi_bubble_info {
-	__u32 num_of_stripes __attribute__((aligned(32)));
+	__u32 num_of_stripes __aligned(32);
 	__u8 padding[28];
 	__u32 num_sets;
 	__u8 padding1[28];
@@ -519,7 +519,7 @@ struct ipu3_uapi_stats_3a_bubble_info_per_stripe {
  * @padding3: padding config
  */
 struct ipu3_uapi_ff_status {
-	__u32 awb_en __attribute__((aligned(32)));
+	__u32 awb_en __aligned(32);
 	__u8 padding[28];
 	__u32 ae_en;
 	__u8 padding1[28];
@@ -992,8 +992,8 @@ struct ipu3_uapi_gamma_corr_lut {
  * @gc_lut: lookup table of gamma correction &ipu3_uapi_gamma_corr_lut
  */
 struct ipu3_uapi_gamma_config {
-	struct ipu3_uapi_gamma_corr_ctrl gc_ctrl __attribute__((aligned(32)));
-	struct ipu3_uapi_gamma_corr_lut gc_lut __attribute__((aligned(32)));
+	struct ipu3_uapi_gamma_corr_ctrl gc_ctrl __aligned(32);
+	struct ipu3_uapi_gamma_corr_lut gc_lut __aligned(32);
 } __packed;
 
 /**
@@ -1196,8 +1196,8 @@ struct ipu3_uapi_shd_lut {
  * @shd_lut:	shading lookup table &ipu3_uapi_shd_lut
  */
 struct ipu3_uapi_shd_config {
-	struct ipu3_uapi_shd_config_static shd __attribute__((aligned(32)));
-	struct ipu3_uapi_shd_lut shd_lut __attribute__((aligned(32)));
+	struct ipu3_uapi_shd_config_static shd __aligned(32);
+	struct ipu3_uapi_shd_lut shd_lut __aligned(32);
 } __packed;
 
 /* Image Enhancement Filter directed */
@@ -2421,8 +2421,8 @@ struct ipu3_uapi_anr_stitch_config {
  * @stitch: create 4x4 patch from 4 surrounding 8x8 patches.
  */
 struct ipu3_uapi_anr_config {
-	struct ipu3_uapi_anr_transform_config transform __attribute__((aligned(32)));
-	struct ipu3_uapi_anr_stitch_config stitch __attribute__((aligned(32)));
+	struct ipu3_uapi_anr_transform_config transform __aligned(32);
+	struct ipu3_uapi_anr_stitch_config stitch __aligned(32);
 } __packed;
 
 /**
@@ -2463,21 +2463,21 @@ struct ipu3_uapi_anr_config {
 struct ipu3_uapi_acc_param {
 	struct ipu3_uapi_bnr_static_config bnr;
 	struct ipu3_uapi_bnr_static_config_green_disparity
-				green_disparity __attribute__((aligned(32)));
-	struct ipu3_uapi_dm_config dm __attribute__((aligned(32)));
-	struct ipu3_uapi_ccm_mat_config ccm __attribute__((aligned(32)));
-	struct ipu3_uapi_gamma_config gamma __attribute__((aligned(32)));
-	struct ipu3_uapi_csc_mat_config csc __attribute__((aligned(32)));
-	struct ipu3_uapi_cds_params cds __attribute__((aligned(32)));
-	struct ipu3_uapi_shd_config shd __attribute__((aligned(32)));
-	struct ipu3_uapi_yuvp1_iefd_config iefd __attribute__((aligned(32)));
-	struct ipu3_uapi_yuvp1_yds_config yds_c0 __attribute__((aligned(32)));
-	struct ipu3_uapi_yuvp1_chnr_config chnr_c0 __attribute__((aligned(32)));
-	struct ipu3_uapi_yuvp1_y_ee_nr_config y_ee_nr __attribute__((aligned(32)));
-	struct ipu3_uapi_yuvp1_yds_config yds __attribute__((aligned(32)));
-	struct ipu3_uapi_yuvp1_chnr_config chnr __attribute__((aligned(32)));
-	struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32)));
-	struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32)));
+				green_disparity __aligned(32);
+	struct ipu3_uapi_dm_config dm __aligned(32);
+	struct ipu3_uapi_ccm_mat_config ccm __aligned(32);
+	struct ipu3_uapi_gamma_config gamma __aligned(32);
+	struct ipu3_uapi_csc_mat_config csc __aligned(32);
+	struct ipu3_uapi_cds_params cds __aligned(32);
+	struct ipu3_uapi_shd_config shd __aligned(32);
+	struct ipu3_uapi_yuvp1_iefd_config iefd __aligned(32);
+	struct ipu3_uapi_yuvp1_yds_config yds_c0 __aligned(32);
+	struct ipu3_uapi_yuvp1_chnr_config chnr_c0 __aligned(32);
+	struct ipu3_uapi_yuvp1_y_ee_nr_config y_ee_nr __aligned(32);
+	struct ipu3_uapi_yuvp1_yds_config yds __aligned(32);
+	struct ipu3_uapi_yuvp1_chnr_config chnr __aligned(32);
+	struct ipu3_uapi_yuvp1_yds_config yds2 __aligned(32);
+	struct ipu3_uapi_yuvp2_tcc_static_config tcc __aligned(32);
 	struct ipu3_uapi_anr_config anr;
 	struct ipu3_uapi_awb_fr_config_s awb_fr;
 	struct ipu3_uapi_ae_config ae;
@@ -2765,7 +2765,7 @@ struct ipu3_uapi_flags {
  */
 struct ipu3_uapi_params {
 	/* Flags which of the settings below are to be applied */
-	struct ipu3_uapi_flags use __attribute__((aligned(32)));
+	struct ipu3_uapi_flags use __aligned(32);
 
 	/* Accelerator cluster parameters */
 	struct ipu3_uapi_acc_param acc_param;
-- 
2.30.2

Re: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[Sakari Ailus]

Hi Mitali,

On Mon, Apr 12, 2021 at 04:38:59AM +0530, Mitali Borkar wrote:
> This patch fixes the warning identified by checkpatch.pl by replacing
> __attribute__aligned(size) with __aligned(size)

Same comments on this than the 1st patch.

It's a staging driver so even if this is a user space header, it's not
under include/uapi/linux.

-- 
Kind regards,

Sakari Ailus

Re: [Outreachy kernel] Re: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[Mitali Borkar]

On Mon, Apr 12, 2021 at 12:43:15PM +0300, Sakari Ailus wrote:
> Hi Mitali,
> 
> On Mon, Apr 12, 2021 at 04:38:59AM +0530, Mitali Borkar wrote:
> > This patch fixes the warning identified by checkpatch.pl by replacing
> > __attribute__aligned(size) with __aligned(size)
> 
> Same comments on this than the 1st patch.
> 
> It's a staging driver so even if this is a user space header, it's not
> under include/uapi/linux.
>
Sir, I am not able to understandd what you are trying to say in this. As you
mentioned in patch 1/6, I removed and added header where BIT() macro under 
apprpriate userpace, but what should I modify in this patch?

> -- 
> Kind regards,
> 
> Sakari Ailus
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20210412094315.GJ3%40paasikivi.fi.intel.com.

Re: [Outreachy kernel] Re: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[Sakari Ailus]

Hi Mitali,

On Mon, Apr 12, 2021 at 07:59:29PM +0530, Mitali Borkar wrote:
> On Mon, Apr 12, 2021 at 12:43:15PM +0300, Sakari Ailus wrote:
> > Hi Mitali,
> > 
> > On Mon, Apr 12, 2021 at 04:38:59AM +0530, Mitali Borkar wrote:
> > > This patch fixes the warning identified by checkpatch.pl by replacing
> > > __attribute__aligned(size) with __aligned(size)
> > 
> > Same comments on this than the 1st patch.
> > 
> > It's a staging driver so even if this is a user space header, it's not
> > under include/uapi/linux.
> >
> Sir, I am not able to understandd what you are trying to say in this. As you
> mentioned in patch 1/6, I removed and added header where BIT() macro under 
> apprpriate userpace, but what should I modify in this patch?

The comment on the 1st patch and above was a weird way of saying "please
drop patches 1 and 2".

BIT(), __aligned() and __packed are macros in kernel headers that generally
are not available in headers exported for user space consumption.

-- 
Kind regards,

Sakari Ailus

RE: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[David Laight]

From: Mitali Borkar
> Sent: 12 April 2021 00:09
> 
> This patch fixes the warning identified by checkpatch.pl by replacing
> __attribute__aligned(size) with __aligned(size)
> 
> Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> ---
>  .../staging/media/ipu3/include/intel-ipu3.h   | 74 +++++++++----------
>  1 file changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h
> b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index 589d5ccee3a7..d95ca9ebfafb 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> @@ -84,7 +84,7 @@ struct ipu3_uapi_grid_config {
>   */
>  struct ipu3_uapi_awb_raw_buffer {
>  	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> -		__attribute__((aligned(32)));
> +		__aligned(32);
>  } __packed;

WTF?

It either has 1-byte alignment because it is just __u8,
32-byte because of the aligned(32),
or 1 byte because of the outer packed.

What alignment does this (and all the other) structures
actually need?

Specifying 'packed' isn't free.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[David Laight]

From: Mitali Borkar
> Sent: 12 April 2021 00:09
> 
> This patch fixes the warning identified by checkpatch.pl by replacing
> __attribute__aligned(size) with __aligned(size)
> 
> Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> ---
>  .../staging/media/ipu3/include/intel-ipu3.h   | 74 +++++++++----------
>  1 file changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h
> b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index 589d5ccee3a7..d95ca9ebfafb 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> @@ -84,7 +84,7 @@ struct ipu3_uapi_grid_config {
>   */
>  struct ipu3_uapi_awb_raw_buffer {
>  	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> -		__attribute__((aligned(32)));
> +		__aligned(32);
>  } __packed;

WTF?

It either has 1-byte alignment because it is just __u8,
32-byte because of the aligned(32),
or 1 byte because of the outer packed.

What alignment does this (and all the other) structures
actually need?

Specifying 'packed' isn't free.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[sakari.ailus]

Hi David,

On Tue, Apr 13, 2021 at 07:40:12AM +0000, David Laight wrote:
> From: Mitali Borkar
> > Sent: 12 April 2021 00:09
> > 
> > This patch fixes the warning identified by checkpatch.pl by replacing
> > __attribute__aligned(size) with __aligned(size)
> > 
> > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > ---
> >  .../staging/media/ipu3/include/intel-ipu3.h   | 74 +++++++++----------
> >  1 file changed, 37 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > index 589d5ccee3a7..d95ca9ebfafb 100644
> > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > @@ -84,7 +84,7 @@ struct ipu3_uapi_grid_config {
> >   */
> >  struct ipu3_uapi_awb_raw_buffer {
> >  	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> > -		__attribute__((aligned(32)));
> > +		__aligned(32);
> >  } __packed;
> 
> WTF?
> 
> It either has 1-byte alignment because it is just __u8,
> 32-byte because of the aligned(32),
> or 1 byte because of the outer packed.
> 
> What alignment does this (and all the other) structures
> actually need?

32 as noted above. Here packed makes no difference though.

Some of these structs are used embedded in other structs or alone. I
haven't checked this one.

It's also possible to have __packed and __aligned() conflict (in which case
a decent compiler would give you a warning) --- which does not happen
currently AFAIK.

> 
> Specifying 'packed' isn't free.

It may be free if the packed alignment of the fields corresponds to
architecture's packing. Here __aligned() is used to satisfy
hardware alignment requirements and __packed is used to ensure the same
memory layout independently of ABI rules.

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[sakari.ailus]

Hi David,

On Tue, Apr 13, 2021 at 07:40:12AM +0000, David Laight wrote:
> From: Mitali Borkar
> > Sent: 12 April 2021 00:09
> > 
> > This patch fixes the warning identified by checkpatch.pl by replacing
> > __attribute__aligned(size) with __aligned(size)
> > 
> > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > ---
> >  .../staging/media/ipu3/include/intel-ipu3.h   | 74 +++++++++----------
> >  1 file changed, 37 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > index 589d5ccee3a7..d95ca9ebfafb 100644
> > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > @@ -84,7 +84,7 @@ struct ipu3_uapi_grid_config {
> >   */
> >  struct ipu3_uapi_awb_raw_buffer {
> >  	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> > -		__attribute__((aligned(32)));
> > +		__aligned(32);
> >  } __packed;
> 
> WTF?
> 
> It either has 1-byte alignment because it is just __u8,
> 32-byte because of the aligned(32),
> or 1 byte because of the outer packed.
> 
> What alignment does this (and all the other) structures
> actually need?

32 as noted above. Here packed makes no difference though.

Some of these structs are used embedded in other structs or alone. I
haven't checked this one.

It's also possible to have __packed and __aligned() conflict (in which case
a decent compiler would give you a warning) --- which does not happen
currently AFAIK.

> 
> Specifying 'packed' isn't free.

It may be free if the packed alignment of the fields corresponds to
architecture's packing. Here __aligned() is used to satisfy
hardware alignment requirements and __packed is used to ensure the same
memory layout independently of ABI rules.

-- 
Regards,

Sakari Ailus

RE: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[David Laight]

From: sakari.ailus@linux.intel.com
> Sent: 13 April 2021 10:56
> 
> Hi David,
> 
> On Tue, Apr 13, 2021 at 07:40:12AM +0000, David Laight wrote:
> > From: Mitali Borkar
> > > Sent: 12 April 2021 00:09
> > >
> > > This patch fixes the warning identified by checkpatch.pl by replacing
> > > __attribute__aligned(size) with __aligned(size)
> > >
> > > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > > ---
> > >  .../staging/media/ipu3/include/intel-ipu3.h   | 74 +++++++++----------
> > >  1 file changed, 37 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > index 589d5ccee3a7..d95ca9ebfafb 100644
> > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > @@ -84,7 +84,7 @@ struct ipu3_uapi_grid_config {
> > >   */
> > >  struct ipu3_uapi_awb_raw_buffer {
> > >  	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> > > -		__attribute__((aligned(32)));
> > > +		__aligned(32);
> > >  } __packed;
> >
> > WTF?
> >
> > It either has 1-byte alignment because it is just __u8,
> > 32-byte because of the aligned(32),
> > or 1 byte because of the outer packed.
> >
> > What alignment does this (and all the other) structures
> > actually need?
> 
> 32 as noted above. Here packed makes no difference though.

Bollocks - it ought to override the __aligned(32);


> Some of these structs are used embedded in other structs or alone. I
> haven't checked this one.
> 
> It's also possible to have __packed and __aligned() conflict (in which case
> a decent compiler would give you a warning) --- which does not happen
> currently AFAIK.

At least one compiler is objecting to some similar constructs.

> >
> > Specifying 'packed' isn't free.
> 
> It may be free if the packed alignment of the fields corresponds to
> architecture's packing. Here __aligned() is used to satisfy
> hardware alignment requirements and __packed is used to ensure the same
> memory layout independently of ABI rules.

No that isn't what packed is for.
'packed' also tells the compiler that the structure may 'exist' at
an unaligned address.
On many architectures this requires the compiler use byte sized
access and shifts for all members.

If you are worried that the compiler/ABI might have inserted
padding then add a compile-time assert on the structure size.
But most kernel code assumes that structures where everything
is on its natural boundary won't have any padding.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH 2/6] staging: media: intel-ipu3: preferred __aligned(size) over __attribute__aligned(size)

[David Laight]

From: sakari.ailus@linux.intel.com
> Sent: 13 April 2021 10:56
> 
> Hi David,
> 
> On Tue, Apr 13, 2021 at 07:40:12AM +0000, David Laight wrote:
> > From: Mitali Borkar
> > > Sent: 12 April 2021 00:09
> > >
> > > This patch fixes the warning identified by checkpatch.pl by replacing
> > > __attribute__aligned(size) with __aligned(size)
> > >
> > > Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> > > ---
> > >  .../staging/media/ipu3/include/intel-ipu3.h   | 74 +++++++++----------
> > >  1 file changed, 37 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > index 589d5ccee3a7..d95ca9ebfafb 100644
> > > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > > @@ -84,7 +84,7 @@ struct ipu3_uapi_grid_config {
> > >   */
> > >  struct ipu3_uapi_awb_raw_buffer {
> > >  	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> > > -		__attribute__((aligned(32)));
> > > +		__aligned(32);
> > >  } __packed;
> >
> > WTF?
> >
> > It either has 1-byte alignment because it is just __u8,
> > 32-byte because of the aligned(32),
> > or 1 byte because of the outer packed.
> >
> > What alignment does this (and all the other) structures
> > actually need?
> 
> 32 as noted above. Here packed makes no difference though.

Bollocks - it ought to override the __aligned(32);


> Some of these structs are used embedded in other structs or alone. I
> haven't checked this one.
> 
> It's also possible to have __packed and __aligned() conflict (in which case
> a decent compiler would give you a warning) --- which does not happen
> currently AFAIK.

At least one compiler is objecting to some similar constructs.

> >
> > Specifying 'packed' isn't free.
> 
> It may be free if the packed alignment of the fields corresponds to
> architecture's packing. Here __aligned() is used to satisfy
> hardware alignment requirements and __packed is used to ensure the same
> memory layout independently of ABI rules.

No that isn't what packed is for.
'packed' also tells the compiler that the structure may 'exist' at
an unaligned address.
On many architectures this requires the compiler use byte sized
access and shifts for all members.

If you are worried that the compiler/ABI might have inserted
padding then add a compile-time assert on the structure size.
But most kernel code assumes that structures where everything
is on its natural boundary won't have any padding.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH 3/6] staging: media: intel-ipu3: remove unnecessary blank line

[Mitali Borkar]

Removed an unnecessary blank line to meet linux kernel coding style.
Reported by checkpatch.pl

Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
---
 drivers/staging/media/ipu3/include/intel-ipu3.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index d95ca9ebfafb..335522e7fc08 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -75,7 +75,6 @@ struct ipu3_uapi_grid_config {
 	(IPU3_UAPI_AWB_MAX_SETS * \
 	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
 
-
 /**
  * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
  *
-- 
2.30.2

[PATCH 4/6] staging: media: intel-ipu3: reduce length of line

[Mitali Borkar]

Reduced length of line as it was exceeding 100 characters by removing
comments from same line and adding it to previous line. This makes code
neater, and meets linux kernel coding style.
Reported by checkpatch.

Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
---
 drivers/staging/media/ipu3/include/intel-ipu3.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index 335522e7fc08..53f8e5dec8f5 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -10,8 +10,10 @@
 /* from /drivers/staging/media/ipu3/include/videodev2.h */
 
 /* Vendor specific - used for IPU3 camera sub-system */
-#define V4L2_META_FMT_IPU3_PARAMS	v4l2_fourcc('i', 'p', '3', 'p') /* IPU3 processing parameters */
-#define V4L2_META_FMT_IPU3_STAT_3A	v4l2_fourcc('i', 'p', '3', 's') /* IPU3 3A statistics */
+/* IPU3 processing parameters */
+#define V4L2_META_FMT_IPU3_PARAMS	v4l2_fourcc('i', 'p', '3', 'p')
+/* IPU3 3A statistics */
+#define V4L2_META_FMT_IPU3_STAT_3A	v4l2_fourcc('i', 'p', '3', 's')
 
 /* from include/uapi/linux/v4l2-controls.h */
 #define V4L2_CID_INTEL_IPU3_BASE	(V4L2_CID_USER_BASE + 0x10c0)
-- 
2.30.2

[PATCH 5/6] staging: media: intel-ipu3: line should not end with '['

[Mitali Borkar]

Fixed the issue of line should not end with '[' by moving arguments
from next line to line ending with '['
Reported by checkpatch.

Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
---
 drivers/staging/media/ipu3/include/intel-ipu3.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index 53f8e5dec8f5..0451f8b7ba4f 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -246,8 +246,7 @@ struct ipu3_uapi_ae_ccm {
  */
 struct ipu3_uapi_ae_config {
 	struct ipu3_uapi_ae_grid_config grid_cfg __aligned(32);
-	struct ipu3_uapi_ae_weight_elem weights[
-			IPU3_UAPI_AE_WEIGHTS] __aligned(32);
+	struct ipu3_uapi_ae_weight_elem weights[IPU3_UAPI_AE_WEIGHTS] __aligned(32);
 	struct ipu3_uapi_ae_ccm ae_ccm __aligned(32);
 } __packed;
 
-- 
2.30.2

Re: [PATCH 5/6] staging: media: intel-ipu3: line should not end with '['

[Sakari Ailus]

Hi Mitali,

On Mon, Apr 12, 2021 at 04:39:58AM +0530, Mitali Borkar wrote:
> Fixed the issue of line should not end with '[' by moving arguments
> from next line to line ending with '['
> Reported by checkpatch.
> 
> Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
> ---
>  drivers/staging/media/ipu3/include/intel-ipu3.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index 53f8e5dec8f5..0451f8b7ba4f 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> @@ -246,8 +246,7 @@ struct ipu3_uapi_ae_ccm {
>   */
>  struct ipu3_uapi_ae_config {
>  	struct ipu3_uapi_ae_grid_config grid_cfg __aligned(32);
> -	struct ipu3_uapi_ae_weight_elem weights[
> -			IPU3_UAPI_AE_WEIGHTS] __aligned(32);
> +	struct ipu3_uapi_ae_weight_elem weights[IPU3_UAPI_AE_WEIGHTS] __aligned(32);

Could you still wrap it to stay under 80?

>  	struct ipu3_uapi_ae_ccm ae_ccm __aligned(32);
>  } __packed;
>  

-- 
Sakari Ailus

[PATCH 6/6] staging: media: intel-ipu3: remove space before tabs

[Mitali Borkar]

Removed unnecessary space before tabs to adhere to  linux kernel coding
style.
Reported by checkpatch.

Signed-off-by: Mitali Borkar <mitaliborkar810@gmail.com>
---
 drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index 0451f8b7ba4f..340d97160bbb 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -631,7 +631,7 @@ struct ipu3_uapi_bnr_static_config_wb_gains_thr_config {
  * @cg:	Gain coefficient for threshold calculation, [0, 31], default 8.
  * @ci:	Intensity coefficient for threshold calculation. range [0, 0x1f]
  *	default 6.
- * 	format: u3.2 (3 most significant bits represent whole number,
+ *	format: u3.2 (3 most significant bits represent whole number,
  *	2 least significant bits represent the fractional part
  *	with each count representing 0.25)
  *	e.g. 6 in binary format is 00110, that translates to 1.5
-- 
2.30.2