[PATCH 1/2] pinctrl: remove unneeded #ifdef around declarations

[PATCH 1/2] pinctrl: remove unneeded #ifdef around declarations

[Masahiro Yamada]

What is the point in surrounding the whole of declarations with
ifdef like this?

  #ifdef CONFIG_FOO
  int foo(void);
  #endif

If CONFIG_FOO is not defined, all callers of foo() will fail
with implicit declaration errors since the top Makefile adds
-Werror-implicit-function-declaration to KBUILD_CFLAGS.

This breaks the build earlier when you are doing something wrong.
That's it.

Anyway, it will fail to link since the definition of foo() is not
compiled.

In summary, these ifdef are unneeded.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 include/linux/pinctrl/pinconf-generic.h | 20 ++++++--------------
 include/linux/pinctrl/pinconf.h         |  4 ----
 include/linux/pinctrl/pinctrl.h         |  4 ----
 include/linux/pinctrl/pinmux.h          |  4 ----
 4 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 6f260c1d3467..b55231993d24 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -11,6 +11,12 @@
 #ifndef __LINUX_PINCTRL_PINCONF_GENERIC_H
 #define __LINUX_PINCTRL_PINCONF_GENERIC_H
 
+#include <linux/device.h>
+#include <linux/pinctrl/machine.h>
+
+struct pinctrl_dev;
+struct pinctrl_map;
+
 /**
  * enum pin_config_param - possible pin configuration parameters
  * @PIN_CONFIG_BIAS_BUS_HOLD: the pin will be set to weakly latch so that it
@@ -155,9 +161,6 @@ static inline unsigned long pinconf_to_config_packed(enum pin_config_param param
 	return PIN_CONF_PACKED(param, argument);
 }
 
-#ifdef CONFIG_GENERIC_PINCONF
-
-#ifdef CONFIG_DEBUG_FS
 #define PCONFDUMP(a, b, c, d) {					\
 	.param = a, .display = b, .format = c, .has_arg = d	\
 	}
@@ -168,14 +171,6 @@ struct pin_config_item {
 	const char * const format;
 	bool has_arg;
 };
-#endif /* CONFIG_DEBUG_FS */
-
-#ifdef CONFIG_OF
-
-#include <linux/device.h>
-#include <linux/pinctrl/machine.h>
-struct pinctrl_dev;
-struct pinctrl_map;
 
 struct pinconf_generic_params {
 	const char * const property;
@@ -220,8 +215,5 @@ static inline int pinconf_generic_dt_node_to_map_all(
 	return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
 			PIN_MAP_TYPE_INVALID);
 }
-#endif
-
-#endif /* CONFIG_GENERIC_PINCONF */
 
 #endif /* __LINUX_PINCTRL_PINCONF_GENERIC_H */
diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
index 514414a5ad01..eb16a1e0bcfa 100644
--- a/include/linux/pinctrl/pinconf.h
+++ b/include/linux/pinctrl/pinconf.h
@@ -11,8 +11,6 @@
 #ifndef __LINUX_PINCTRL_PINCONF_H
 #define __LINUX_PINCTRL_PINCONF_H
 
-#ifdef CONFIG_PINCONF
-
 struct pinctrl_dev;
 struct seq_file;
 
@@ -64,6 +62,4 @@ struct pinconf_ops {
 					    unsigned long config);
 };
 
-#endif
-
 #endif /* __LINUX_PINCTRL_PINCONF_H */
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 34b10d112be6..c6159f041f4e 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -11,8 +11,6 @@
 #ifndef __LINUX_PINCTRL_PINCTRL_H
 #define __LINUX_PINCTRL_PINCTRL_H
 
-#ifdef CONFIG_PINCTRL
-
 #include <linux/radix-tree.h>
 #include <linux/list.h>
 #include <linux/seq_file.h>
@@ -197,6 +195,4 @@ extern const char *pinctrl_dev_get_name(struct pinctrl_dev *pctldev);
 extern const char *pinctrl_dev_get_devname(struct pinctrl_dev *pctldev);
 extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
 
-#endif /* !CONFIG_PINCTRL */
-
 #endif /* __LINUX_PINCTRL_PINCTRL_H */
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
index e873ed97d79e..9a647fa5c8f1 100644
--- a/include/linux/pinctrl/pinmux.h
+++ b/include/linux/pinctrl/pinmux.h
@@ -15,8 +15,6 @@
 #include <linux/seq_file.h>
 #include <linux/pinctrl/pinctrl.h>
 
-#ifdef CONFIG_PINMUX
-
 struct pinctrl_dev;
 
 /**
@@ -84,6 +82,4 @@ struct pinmux_ops {
 	bool strict;
 };
 
-#endif /* CONFIG_PINMUX */
-
 #endif /* __LINUX_PINCTRL_PINMUX_H */
-- 
2.17.1

[PATCH 2/2] pinctrl: make pinconf.h self-contained

[Masahiro Yamada]

This header uses 'bool', but it does not include any header by itself.

So, it could cause unknown type name error, depending on the header
include order, although probably <linux/types.h> has been included by
someone else.

Include <linux/types.h> to make it self-contained.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 include/linux/pinctrl/pinconf.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
index eb16a1e0bcfa..f8a8215e9021 100644
--- a/include/linux/pinctrl/pinconf.h
+++ b/include/linux/pinctrl/pinconf.h
@@ -11,6 +11,8 @@
 #ifndef __LINUX_PINCTRL_PINCONF_H
 #define __LINUX_PINCTRL_PINCONF_H
 
+#include <linux/types.h>
+
 struct pinctrl_dev;
 struct seq_file;
 
-- 
2.17.1

Re: [PATCH 1/2] pinctrl: remove unneeded #ifdef around declarations

[Enrico Weigelt, metux IT consult]

On 13.06.19 03:55, Masahiro Yamada wrote:
> What is the point in surrounding the whole of declarations with
> ifdef like this?
> 
>   #ifdef CONFIG_FOO
>   int foo(void);
>   #endif
> 
> If CONFIG_FOO is not defined, all callers of foo() will fail
> with implicit declaration errors since the top Makefile adds
> -Werror-implicit-function-declaration to KBUILD_CFLAGS.
> 
> This breaks the build earlier when you are doing something wrong.
> That's it.

hmm, in general I like the idea of breaking the build as early as
possible. depending on your available cpu power, a kernel build can
take a while, and it could be a huge waste of time when having to
wait for link stage, just to find out about missing functions.

@linus: what's your oppinion ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 1/2] pinctrl: remove unneeded #ifdef around declarations

[Masahiro Yamada]

On Fri, Jun 14, 2019 at 10:21 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
>
> On 13.06.19 03:55, Masahiro Yamada wrote:
> > What is the point in surrounding the whole of declarations with
> > ifdef like this?
> >
> >   #ifdef CONFIG_FOO
> >   int foo(void);
> >   #endif
> >
> > If CONFIG_FOO is not defined, all callers of foo() will fail
> > with implicit declaration errors since the top Makefile adds
> > -Werror-implicit-function-declaration to KBUILD_CFLAGS.
> >
> > This breaks the build earlier when you are doing something wrong.
> > That's it.
>
> hmm, in general I like the idea of breaking the build as early as
> possible. depending on your available cpu power, a kernel build can
> take a while, and it could be a huge waste of time when having to
> wait for link stage, just to find out about missing functions.
>
> @linus: what's your oppinion ?
>
>
> --mtx
>
> --
> Enrico Weigelt, metux IT consult
> Free software and Linux embedded engineering
> info@metux.net -- +49-151-27565287


My previous clean-up (http://patchwork.ozlabs.org/patch/1112656/)
broke this build.

And, this patch will fix the build issue.

Did you realize the madness of
surrounding the forward declarations with #ifdef ?


All GPIO/pinctrl headers are written in a bad way.

Linus Walleij must realize he was doing *wrong*.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] pinctrl: make pinconf.h self-contained

[Linus Walleij]

On Thu, Jun 13, 2019 at 3:55 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:

> This header uses 'bool', but it does not include any header by itself.
>
> So, it could cause unknown type name error, depending on the header
> include order, although probably <linux/types.h> has been included by
> someone else.
>
> Include <linux/types.h> to make it self-contained.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: remove unneeded #ifdef around declarations

[Linus Walleij]

Hi Masahiro,

thanks for your patch. For some reason I managed to pick up
patch 2 before patch 1. I applied this now with some fuzzing.
(Please check the result.)

On Thu, Jun 13, 2019 at 3:55 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:

> What is the point in surrounding the whole of declarations with
> ifdef like this?

I don't know if it is generally good to have phrases posed as
questions in a commit message, we prefer to have statements
about the change not a polemic dialog.

>   #ifdef CONFIG_FOO
>   int foo(void);
>   #endif
>
> If CONFIG_FOO is not defined, all callers of foo() will fail
> with implicit declaration errors since the top Makefile adds
> -Werror-implicit-function-declaration to KBUILD_CFLAGS.

Maybe this flag was not in the top Makefile when the #ifdefs
where introduced?

> This breaks the build earlier when you are doing something wrong.
> That's it.

Good idea.

> Anyway, it will fail to link since the definition of foo() is not
> compiled.
>
> In summary, these ifdef are unneeded.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Pushing this to the zeroday builders and let's see what happens!

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: remove unneeded #ifdef around declarations

[Masahiro Yamada]

On Tue, Jun 25, 2019 at 5:52 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Masahiro,
>
> thanks for your patch. For some reason I managed to pick up
> patch 2 before patch 1. I applied this now with some fuzzing.
> (Please check the result.)
>
> On Thu, Jun 13, 2019 at 3:55 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>
> > What is the point in surrounding the whole of declarations with
> > ifdef like this?
>
> I don't know if it is generally good to have phrases posed as
> questions in a commit message, we prefer to have statements
> about the change not a polemic dialog.
>
> >   #ifdef CONFIG_FOO
> >   int foo(void);
> >   #endif
> >
> > If CONFIG_FOO is not defined, all callers of foo() will fail
> > with implicit declaration errors since the top Makefile adds
> > -Werror-implicit-function-declaration to KBUILD_CFLAGS.
>
> Maybe this flag was not in the top Makefile when the #ifdefs
> where introduced?
>
> > This breaks the build earlier when you are doing something wrong.
> > That's it.
>
> Good idea.
>
> > Anyway, it will fail to link since the definition of foo() is not
> > compiled.
> >
> > In summary, these ifdef are unneeded.
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>
> Pushing this to the zeroday builders and let's see what happens!


Sorry for my terrible commit message and offending response.

I appended a new commit message below.

If you are OK with rebasing, please consider replacement.

--------------------------->8----------------------------------
pinctrl: remove less important #ifdef around declarations

The whole declarations in these headers are surrounded by #ifdef.

As far as I understood, the motivation of this is probably to break
the build earlier if a driver misses to select or depend on correct
CONFIG options in Kconfig.

Since commit 94bed2a9c4ae ("Add -Werror-implicit-function-declaration")
no one cannot call functions that have not been declared.

So, I see some benefit in doing this in the cost of uglier headers.

In reality, it would not be so easy to catch missed 'select' or
'depends on' because PINCTRL, PINMUX, etc. are already selected by
someone else eventually. So, this kind of error, if any, will be
caught by randconfig bots.

In summary, I am not a big fan of deep #ifdef nesting, and this
does not matter for normal developers. The code readability wins.
--------------------------->8----------------------------------




-- 
Best Regards
Masahiro Yamada