Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-03 Thread Jeremy Sowden 
On 2017-12-03, at 08:39:21 +0300, Dan Carpenter wrote:
> On Sat, Dec 02, 2017 at 08:41:48PM +, Jeremy Sowden wrote:
> > On 2017-12-02, at 10:35:06 +, Jeremy Sowden wrote:
> > > On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> > > > On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > > > > -#define DEFAULT_PIPE_INFO \
> > > > > -{ \
> > > > > - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > > > > - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > > > > - IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > > > > - { 0, 0},/* output system in res 
> > > > > */ \
> > > > > - DEFAULT_SHADING_INFO,   /* shading_info */ \
> > > > > - DEFAULT_GRID_INFO,  /* grid_info */ \
> > > > > - 0   /* num_invalid_frames 
> > > > > */ \
> > > > > -}
> > > > > +#define DEFAULT_PIPE_INFO ( \
> > > >
> > > > Why does this have a ( now?  That can't compile can it??
> > >
> > > It does.
> >
> > That was a bit terse: the macros expand to compound-literals, so
> > putting parens around them is no different from:
> >
> >   #define THREE (3)
>
> Yeah.  Thanks.  I figured it out despite the terseness...  I try
> review as fast as I can, so it means you get the stream of
> conciousness output that often has mistakes.  Sorry about that.

No worries.  The feedback has been very helpful.

J.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Dan Carpenter 
On Sat, Dec 02, 2017 at 08:41:48PM +, Jeremy Sowden wrote:
> On 2017-12-02, at 10:35:06 +, Jeremy Sowden wrote:
> > On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> > > On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > > > -#define DEFAULT_PIPE_INFO \
> > > > -{ \
> > > > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > > > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > > > -   IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > > > -   { 0, 0},/* output system in res 
> > > > */ \
> > > > -   DEFAULT_SHADING_INFO,   /* shading_info */ \
> > > > -   DEFAULT_GRID_INFO,  /* grid_info */ \
> > > > -   0   /* num_invalid_frames 
> > > > */ \
> > > > -}
> > > > +#define DEFAULT_PIPE_INFO ( \
> > >
> > > Why does this have a ( now?  That can't compile can it??
> >
> > It does.
> 
> That was a bit terse: the macros expand to compound-literals, so
> putting parens around them is no different from:
> 
>   #define THREE (3)

Yeah.  Thanks.  I figured it out despite the terseness...  I try review
as fast as I can, so it means you get the stream of conciousness output
that often has mistakes.  Sorry about that.

regards,
dan carpenter

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Jeremy Sowden 
On 2017-12-02, at 20:41:48 +, Jeremy Sowden wrote:
> On 2017-12-02, at 10:35:06 +, Jeremy Sowden wrote:
> > On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> > > On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > > > -#define DEFAULT_PIPE_INFO \
> > > > -{ \
> > > > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > > > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > > > -   IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > > > -   { 0, 0},/* output system in res 
> > > > */ \
> > > > -   DEFAULT_SHADING_INFO,   /* shading_info */ \
> > > > -   DEFAULT_GRID_INFO,  /* grid_info */ \
> > > > -   0   /* num_invalid_frames 
> > > > */ \
> > > > -}
> > > > +#define DEFAULT_PIPE_INFO ( \
> > >
> > > Why does this have a ( now?  That can't compile can it??
> > >
> > > > +   (struct ia_css_pipe_info) { \
> > > > +   .output_info= 
> > > > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > > > +   .vf_output_info = 
> > > > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > > > +   .raw_output_info= 
> > > > IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> > > > +   .output_system_in_res_info  = { 0, 0 }, \
> > > > +   .shading_info   = DEFAULT_SHADING_INFO, 
> > > > \
> > > > +   .grid_info  = DEFAULT_GRID_INFO, \
> > > > +   .num_invalid_frames = 0 \
> > > > +   } \
> > > > +)
> >
> > Checkpatch got quite shouty, e.g.:
> >
> >   ERROR: Macros with complex values should be enclosed in parentheses
> >   #826: FILE: 
> > drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h:215:
> >   +#define DEFAULT_DVS_STAT_PUBLIC_DVS_GLOBAL_CFG \
> >   +(struct dvs_stat_public_dvs_global_cfg) { \
> >   +   .kappa  = 0, \
> >   +   .match_shift= 0, \
> >   +   .ybin_mode  = 0, \
> >   +}
> >
> > so I just wrapped all of them.
>
> I've run checkpatch.pl against the unparenthesized patches, and it
> only objects to some of the macros.  I've also taken a look at the
> source of checkpatch.pl itself, and at first glance it appears that it
> should accept them.  I'll see if I can work out why it's complaining.

I think I've found a bug in checkpatch.pl.  I'll remove the parentheses
from my patches 'cause the error-reportd appear to be bogus, and pass on
my findings.

J.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Jeremy Sowden 
On 2017-12-02, at 10:35:06 +, Jeremy Sowden wrote:
> On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> > On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > > -#define DEFAULT_PIPE_INFO \
> > > -{ \
> > > - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > > - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > > - IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > > - { 0, 0},/* output system in res */ \
> > > - DEFAULT_SHADING_INFO,   /* shading_info */ \
> > > - DEFAULT_GRID_INFO,  /* grid_info */ \
> > > - 0   /* num_invalid_frames */ \
> > > -}
> > > +#define DEFAULT_PIPE_INFO ( \
> >
> > Why does this have a ( now?  That can't compile can it??
>
> It does.

That was a bit terse: the macros expand to compound-literals, so
putting parens around them is no different from:

  #define THREE (3)

It's also superfluous, of course.

> > > + (struct ia_css_pipe_info) { \
> > > + .output_info= 
> > > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > > + .vf_output_info = 
> > > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > > + .raw_output_info= 
> > > IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> > > + .output_system_in_res_info  = { 0, 0 }, \
> > > + .shading_info   = DEFAULT_SHADING_INFO, \
> > > + .grid_info  = DEFAULT_GRID_INFO, \
> > > + .num_invalid_frames = 0 \
> > > + } \
> > > +)
>
> Checkpatch got quite shouty, e.g.:
>
>   ERROR: Macros with complex values should be enclosed in parentheses
>   #826: FILE: 
> drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h:215:
>   +#define DEFAULT_DVS_STAT_PUBLIC_DVS_GLOBAL_CFG \
>   +(struct dvs_stat_public_dvs_global_cfg) { \
>   +   .kappa  = 0, \
>   +   .match_shift= 0, \
>   +   .ybin_mode  = 0, \
>   +}
>
> so I just wrapped all of them.

I've run checkpatch.pl against the unparenthesized patches, and it only
objects to some of the macros.  I've also taken a look at the source of
checkpatch.pl itself, and at first glance it appears that it should
accept them.  I'll see if I can work out why it's complaining.

J.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Dan Carpenter 
On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> -#define DEFAULT_PIPE_INFO \
> -{ \
> - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> - IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> - { 0, 0},/* output system in res */ \
> - DEFAULT_SHADING_INFO,   /* shading_info */ \
> - DEFAULT_GRID_INFO,  /* grid_info */ \
> - 0   /* num_invalid_frames */ \
> -}
> +#define DEFAULT_PIPE_INFO ( \

Why does this have a ( now?  That can't compile can it??

> + (struct ia_css_pipe_info) { \
> + .output_info= 
> {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> + .vf_output_info = 
> {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> + .raw_output_info= 
> IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> + .output_system_in_res_info  = { 0, 0 }, \
> + .shading_info   = DEFAULT_SHADING_INFO, \
> + .grid_info  = DEFAULT_GRID_INFO, \
> + .num_invalid_frames = 0 \
> + } \
> +)

We need to get better compile test coverage on this...  :/  There are
some others as well.

regards,
dan carpenter

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Jeremy Sowden 
On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > -#define DEFAULT_PIPE_INFO \
> > -{ \
> > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > -   IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > -   { 0, 0},/* output system in res */ \
> > -   DEFAULT_SHADING_INFO,   /* shading_info */ \
> > -   DEFAULT_GRID_INFO,  /* grid_info */ \
> > -   0   /* num_invalid_frames */ \
> > -}
> > +#define DEFAULT_PIPE_INFO ( \
>
> Why does this have a ( now?  That can't compile can it??

It does.

> > +   (struct ia_css_pipe_info) { \
> > +   .output_info= 
> > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > +   .vf_output_info = 
> > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > +   .raw_output_info= 
> > IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> > +   .output_system_in_res_info  = { 0, 0 }, \
> > +   .shading_info   = DEFAULT_SHADING_INFO, \
> > +   .grid_info  = DEFAULT_GRID_INFO, \
> > +   .num_invalid_frames = 0 \
> > +   } \
> > +)

Checkpatch got quite shouty, e.g.:

  ERROR: Macros with complex values should be enclosed in parentheses
  #826: FILE: 
drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h:215:
  +#define DEFAULT_DVS_STAT_PUBLIC_DVS_GLOBAL_CFG \
  +(struct dvs_stat_public_dvs_global_cfg) { \
  +   .kappa  = 0, \
  +   .match_shift= 0, \
  +   .ybin_mode  = 0, \
  +}

so I just wrapped all of them.

> We need to get better compile test coverage on this...  :/  There are
> some others as well.

I have run a test-compilation.  Some of the code doesn't get built
because it's #ifdeffed off.  I did try adding -DISP2401 (which enables
most of it), that that just causes unrelated compilation failures.

J.


signature.asc
Description: PGP signature

[PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-01 Thread Jeremy Sowden 
The CSS API uses a lot of nested anonymous structs defined in object
macros to assign default values to its data-structures.  These have been
changed to use compound-literals and designated initializers to make
them more comprehensible and less fragile.

The compound-literals can also be used in assignment, which means we can
get rid of some temporary variables whose only purpose is to be
initialized by one of these anonymous structs and then serve as the
rvalue in an assignment expression.

Signed-off-by: Jeremy Sowden 
---
 .../hive_isp_css_common/input_formatter_global.h   |  31 ++--
 .../pci/atomisp2/css2400/ia_css_frame_public.h |  56 ---
 .../atomisp/pci/atomisp2/css2400/ia_css_pipe.h | 173 +++--
 .../pci/atomisp2/css2400/ia_css_pipe_public.h  | 159 +--
 .../atomisp/pci/atomisp2/css2400/ia_css_types.h|  98 ++--
 .../isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h | 149 +-
 .../kernels/sdis/common/ia_css_sdis_common_types.h |  96 +---
 .../isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c   |   3 +-
 .../runtime/binary/interface/ia_css_binary.h   | 155 +-
 .../atomisp2/css2400/runtime/binary/src/binary.c   |   3 +-
 .../isp_param/interface/ia_css_isp_param_types.h   |  32 ++--
 .../runtime/pipeline/interface/ia_css_pipeline.h   |  35 +++--
 .../css2400/runtime/pipeline/src/pipeline.c|   7 +-
 .../media/atomisp/pci/atomisp2/css2400/sh_css.c|  22 +--
 .../atomisp/pci/atomisp2/css2400/sh_css_legacy.h   |  21 +--
 .../atomisp/pci/atomisp2/css2400/sh_css_metrics.h  |  36 ++---
 16 files changed, 613 insertions(+), 463 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
index 5654d911db65..5e19b5bd56e6 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
@@ -107,21 +107,22 @@ struct input_formatter_cfg_s {
uint32_tblock_no_reqs;
 };
 
-#define DEFAULT_IF_CONFIG \
-{ \
-   0,  /* start_line */\
-   0,  /* start_column */\
-   0,  /* left_padding */\
-   0,  /* cropped_height */\
-   0,  /* cropped_width */\
-   0,  /* deinterleaving */\
-   0,  /*.buf_vecs */\
-   0,  /* buf_start_index */\
-   0,  /* buf_increment */\
-   0,  /* buf_eol_offset */\
-   false,  /* is_yuv420_format */\
-   false   /* block_no_reqs */\
-}
+#define DEFAULT_IF_CONFIG ( \
+   (struct input_formatter_cfg_s) { \
+   .start_line = 0, \
+   .start_column   = 0, \
+   .left_padding   = 0, \
+   .cropped_height = 0, \
+   .cropped_width  = 0, \
+   .deinterleaving = 0, \
+   .buf_vecs   = 0, \
+   .buf_start_index= 0, \
+   .buf_increment  = 0, \
+   .buf_eol_offset = 0, \
+   .is_yuv420_format   = false, \
+   .block_no_reqs  = false \
+   } \
+)
 
 extern const hrt_address HIVE_IF_SRST_ADDRESS[N_INPUT_FORMATTER_ID];
 extern const hrt_data HIVE_IF_SRST_MASK[N_INPUT_FORMATTER_ID];
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
index 92f2389176b2..56527feeb558 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
@@ -120,17 +120,22 @@ struct ia_css_frame_info {
struct ia_css_crop_info crop_info;
 };
 
-#define IA_CSS_BINARY_DEFAULT_FRAME_INFO \
-{ \
-   {0,  /* width */ \
-0}, /* height */ \
-   0,   /* padded_width */ \
-   IA_CSS_FRAME_FORMAT_NUM, /* format */ \
-   0,   /* raw_bit_depth */ \
-   IA_CSS_BAYER_ORDER_NUM,  /* raw_bayer_order */ \
-   {0,   /*start col */ \
-0},   /*start line */ \
-}
+#define IA_CSS_BINARY_DEFAULT_FRAME_INFO ( \
+   (struct ia_css_frame_info) { \
+   .res= (struct ia_css_resolution) { \
+   .width = 0, \
+   .height = 0 \
+   }, \
+   .padded_width   = 0, \
+   .format = IA_CSS_FRAME_FORMAT_NUM,  \
+   .raw_bit_depth  = 0, \
+