| Message ID | 20240730062348.46205-2-spwhitton@spwhitton.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | staging: media: atomisp: Add parentheses around macro definitions | expand |
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
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
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!
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
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.
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
Hello, Thanks for the additional pointers. I'll dig into this soon.
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__ */
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!