[PATCH] staging: media: atomisp: Add parentheses around macro definitions

[PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Sean Whitton]

Fix checkpatch error
"ERROR: Macros with complex values should be enclosed in parentheses"
at hive_isp_css_include/sp.h:41, hive_isp_css_include/sp.h:42.

Signed-off-by: Sean Whitton <spwhitton@spwhitton.name>
---
 drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

This is my first Linux kernel patch, from Helen Koike's DebConf24 workshop.
Thanks!

diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h b/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
index a7d00c7bb8bc..128109afe842 100644
--- a/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
+++ b/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
@@ -38,8 +38,8 @@
 #define STORAGE_CLASS_SP_C
 #include "sp_public.h"
 #else  /* __INLINE_SP__ */
-#define STORAGE_CLASS_SP_H static inline
-#define STORAGE_CLASS_SP_C static inline
+#define STORAGE_CLASS_SP_H (static inline)
+#define STORAGE_CLASS_SP_C (static inline)
 #include "sp_private.h"
 #endif /* __INLINE_SP__ */
 
-- 
2.45.2

Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Greg Kroah-Hartman]

On Tue, Jul 30, 2024 at 03:23:45PM +0900, Sean Whitton wrote:
> Fix checkpatch error
> "ERROR: Macros with complex values should be enclosed in parentheses"
> at hive_isp_css_include/sp.h:41, hive_isp_css_include/sp.h:42.
> 
> Signed-off-by: Sean Whitton <spwhitton@spwhitton.name>
> ---
>  drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> This is my first Linux kernel patch, from Helen Koike's DebConf24 workshop.
> Thanks!
> 
> diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h b/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
> index a7d00c7bb8bc..128109afe842 100644
> --- a/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
> +++ b/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
> @@ -38,8 +38,8 @@
>  #define STORAGE_CLASS_SP_C
>  #include "sp_public.h"
>  #else  /* __INLINE_SP__ */
> -#define STORAGE_CLASS_SP_H static inline
> -#define STORAGE_CLASS_SP_C static inline
> +#define STORAGE_CLASS_SP_H (static inline)
> +#define STORAGE_CLASS_SP_C (static inline)

This isn't a "complex values", and really should just be removed
entirely and use the correct "static inline" properly.

thanks,

greg k-h

Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Dan Carpenter]

On Tue, Jul 30, 2024 at 03:23:45PM +0900, Sean Whitton wrote:
> Fix checkpatch error
> "ERROR: Macros with complex values should be enclosed in parentheses"
> at hive_isp_css_include/sp.h:41, hive_isp_css_include/sp.h:42.
> 
> Signed-off-by: Sean Whitton <spwhitton@spwhitton.name>
> ---
>  drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> This is my first Linux kernel patch, from Helen Koike's DebConf24 workshop.
> Thanks!
> 
> diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h b/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
> index a7d00c7bb8bc..128109afe842 100644
> --- a/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
> +++ b/drivers/staging/media/atomisp/pci/hive_isp_css_include/sp.h
> @@ -38,8 +38,8 @@
>  #define STORAGE_CLASS_SP_C
>  #include "sp_public.h"
>  #else  /* __INLINE_SP__ */
> -#define STORAGE_CLASS_SP_H static inline
> -#define STORAGE_CLASS_SP_C static inline
> +#define STORAGE_CLASS_SP_H (static inline)
> +#define STORAGE_CLASS_SP_C (static inline)

This must be dead code, otherwise it would break the build.  I'm not
sure what's going on with this header file.  Anyway this patch isn't
correct.

regards,
dan carpenter

Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Sean Whitton]

Hello,

On Tue 30 Jul 2024 at 08:38am +02, Greg Kroah-Hartman wrote:

> This isn't a "complex values", and really should just be removed
> entirely and use the correct "static inline" properly.

I found that there are several headers in
atomisp/pci/hive_isp_css_include which have this pattern of defining an
_INLINE_*_ preprocessor constant, and using it to choose between
'extern' and 'static inline' in each file where the header is included.

I don't know what the author's intention was.  Are you saying that you
think this preprocessor mechanism should just be replaced with
hardcoding 'extern' or 'static inline' in each file which includes one
of these headers?

Thanks!

-- 
Sean Whitton

Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Dan Carpenter]

On Sat, Aug 03, 2024 at 11:28:02AM +0800, Sean Whitton wrote:
> Hello,
> 
> On Tue 30 Jul 2024 at 08:38am +02, Greg Kroah-Hartman wrote:
> 
> > This isn't a "complex values", and really should just be removed
> > entirely and use the correct "static inline" properly.
> 
> I found that there are several headers in
> atomisp/pci/hive_isp_css_include which have this pattern of defining an
> _INLINE_*_ preprocessor constant, and using it to choose between
> 'extern' and 'static inline' in each file where the header is included.
> 
> I don't know what the author's intention was.  Are you saying that you
> think this preprocessor mechanism should just be replaced with
> hardcoding 'extern' or 'static inline' in each file which includes one
> of these headers?

*You* need to figure out what the proper thing is.  Not us.  That's the
difficult part of writing a patch.  Once you know what the correct thing
is, then the rest is just typing.

That business of defining STORAGE_CLASS_SP_C is weird.  Figure out the
authors intention and find a better way to do it.

Figure out why your code compiled as well because putting parentheses
around (static inline) is a syntax error.

regards,
dan carpenter

Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Sean Whitton]

Hello,

On Fri 02 Aug 2024 at 11:28pm -05, Dan Carpenter wrote:

> *You* need to figure out what the proper thing is.  Not us.  That's the
> difficult part of writing a patch.  Once you know what the correct thing
> is, then the rest is just typing.
>
> That business of defining STORAGE_CLASS_SP_C is weird.  Figure out the
> authors intention and find a better way to do it.
>
> Figure out why your code compiled as well because putting parentheses
> around (static inline) is a syntax error.

I asked follow-up questions because it seems like at least partially a
matter of style to say that the business of defining STORAGE_CLASS_SP_C
is weird.  Maybe there is a better approach than what is currently done,
but maybe there isn't.  Maybe the checkpatch warning should just be
suppressed (if that's something that can be done).  I would be grateful
for some additional pointers.

-- 
Sean Whitton

Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Dan Carpenter]

On Sat, Aug 03, 2024 at 01:33:43PM +0800, Sean Whitton wrote:
> Hello,
> 
> On Fri 02 Aug 2024 at 11:28pm -05, Dan Carpenter wrote:
> 
> > *You* need to figure out what the proper thing is.  Not us.  That's the
> > difficult part of writing a patch.  Once you know what the correct thing
> > is, then the rest is just typing.
> >
> > That business of defining STORAGE_CLASS_SP_C is weird.  Figure out the
> > authors intention and find a better way to do it.
> >
> > Figure out why your code compiled as well because putting parentheses
> > around (static inline) is a syntax error.
> 
> I asked follow-up questions because it seems like at least partially a
> matter of style to say that the business of defining STORAGE_CLASS_SP_C
> is weird.

I'm a domain expert when it comes to kernel style.  ;)  Trust me, it's
weird.  There are other places which do it as will but it's not normal.

> Maybe there is a better approach than what is currently done,
> but maybe there isn't.

Correct.  Just because it's weird, doesn't mean it's wrong.  Figure out
why the author did what they did and after that you'll probably be able
to judge if it makes sense.

> Maybe the checkpatch warning should just be
> suppressed (if that's something that can be done).

Yes.  Try to suppress the warning.  You don't need anyone's permission.
I think it will be difficult and I doubt you will succeed.  But you
never know until you try.  Even if you don't succeed, it's a useful
exercise.

> I would be grateful for some additional pointers.
> 

Ok.  Here was your question.

> I don't know what the author's intention was.  Are you saying that you
> think this preprocessor mechanism should just be replaced with
> hardcoding 'extern' or 'static inline' in each file which includes one
> of these headers?

The answer is you need to figure out what the author's intention was.
1) Look through the git log.  2) Try removing it and see if anything
breaks.  3) Do a grep for __INLINE_SP__.  (I deleted some extra hints
here because if I give any more hints then it's just me doing the
project).

Once you know why the macro exists then you can decide it we should do a
sed to replace it.  The sed to get rid of the macro is just an automated
one liner thing.  The difficult part is answering why the macro was
created and do we still need it?

regards,
dan carpenter

Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Sean Whitton]

Hello,

Thanks for the additional pointers.  I'll dig into this soon.

-- 
Sean Whitton