[PATCH] allow gpiolib to be a module

[PATCH] allow gpiolib to be a module

[Jan Beulich]

Without ARCH_REQUIRE_GPIOLIB there's no reason to force this code, when
enabled, to always be built into the kernel, which requires only minor
Makefile and source code adjustments.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 drivers/gpio/Kconfig       |    2 +-
 drivers/gpio/Makefile      |    3 ++-
 drivers/gpio/gpiolib.c     |   15 +++++++++++++++
 include/asm-generic/gpio.h |    3 ++-
 4 files changed, 20 insertions(+), 3 deletions(-)

--- 3.6-rc5/drivers/gpio/Kconfig
+++ 3.6-rc5-gpiolib-module/drivers/gpio/Kconfig
@@ -33,7 +33,7 @@ config ARCH_REQUIRE_GPIOLIB
 
 
 menuconfig GPIOLIB
-	bool "GPIO Support"
+	tristate "GPIO Support"
 	depends on ARCH_WANT_OPTIONAL_GPIOLIB || ARCH_REQUIRE_GPIOLIB
 	select GENERIC_GPIO
 	help
--- 3.6-rc5/drivers/gpio/Makefile
+++ 3.6-rc5-gpiolib-module/drivers/gpio/Makefile
@@ -2,7 +2,8 @@
 
 ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
 
-obj-$(CONFIG_GPIOLIB)		+= gpiolib.o devres.o
+gpio-lib-y			:= gpiolib.o devres.o
+obj-$(CONFIG_GPIOLIB)		+= gpio-lib.o
 obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
 
 # Device drivers. Generally keep list sorted alphabetically
--- 3.6-rc5/drivers/gpio/gpiolib.c
+++ 3.6-rc5-gpiolib-module/drivers/gpio/gpiolib.c
@@ -1003,7 +1003,9 @@ static int __init gpiolib_sysfs_init(voi
 
 	return status;
 }
+#ifndef MODULE
 postcore_initcall(gpiolib_sysfs_init);
+#endif
 
 #else
 static inline int gpiochip_export(struct gpio_chip *chip)
@@ -1832,6 +1834,19 @@ static int __init gpiolib_debugfs_init(v
 				NULL, NULL, &gpiolib_operations);
 	return 0;
 }
+#ifndef MODULE
 subsys_initcall(gpiolib_debugfs_init);
+#endif
 
 #endif	/* DEBUG_FS */
+
+#ifdef MODULE
+int __init gpiolib_init(void)
+{
+	return gpiolib_sysfs_init() ?: gpiolib_debugfs_init();
+}
+module_init(gpiolib_init);
+
+MODULE_DESCRIPTION("GPIO library");
+MODULE_LICENSE("GPL");
+#endif
--- 3.6-rc5/include/asm-generic/gpio.h
+++ 3.6-rc5-gpiolib-module/include/asm-generic/gpio.h
@@ -4,9 +4,10 @@
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/kconfig.h>
 #include <linux/of.h>
 
-#ifdef CONFIG_GPIOLIB
+#if IS_ENABLED(CONFIG_GPIOLIB)
 
 #include <linux/compiler.h>
 

Re: [PATCH] allow gpiolib to be a module

[Ryan Mallon]

On 10/09/12 22:16, Jan Beulich wrote:
> Without ARCH_REQUIRE_GPIOLIB there's no reason to force this code, when
> enabled, to always be built into the kernel, which requires only minor
> Makefile and source code adjustments.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> ---

<snip>

> +#ifdef MODULE
> +int __init gpiolib_init(void)

Should be static.

> +{
> +	return gpiolib_sysfs_init() ?: gpiolib_debugfs_init();

I thought this was going to call gpiolib_sysfs_init() twice until I
looked at gcc's documentation. Maybe the less obtuse, and far more common:

  int err;

  err = gpiolib_sysfs_init();
  if (err)
          return err;

  return gpiolib_debugfs_init();

?

> +}
> +module_init(gpiolib_init);

~Ryan

Re: [PATCH] allow gpiolib to be a module

[Jan Beulich]

>>> On 11.09.12 at 08:17, Ryan Mallon <rmallon@gmail.com> wrote:
> On 10/09/12 22:16, Jan Beulich wrote:
>> +#ifdef MODULE
>> +int __init gpiolib_init(void)
> 
> Should be static.

Oh, yes, of course.

>> +{
>> +	return gpiolib_sysfs_init() ?: gpiolib_debugfs_init();
> 
> I thought this was going to call gpiolib_sysfs_init() twice until I
> looked at gcc's documentation. Maybe the less obtuse, and far more common:
> 
>   int err;
> 
>   err = gpiolib_sysfs_init();
>   if (err)
>           return err;
> 
>   return gpiolib_debugfs_init();

That construct is being used in many other places throughout
the kernel (including in gpiolib itself), so I don't see why it can't
be used here - the more that it is precisely available to have a
way to avoid the double evaluation.

Jan

Re: [PATCH] allow gpiolib to be a module

[Linus Walleij]

On Mon, Sep 10, 2012 at 2:16 PM, Jan Beulich <JBeulich@suse.com> wrote:

> Without ARCH_REQUIRE_GPIOLIB there's no reason to force this code, when
> enabled, to always be built into the kernel, which requires only minor
> Makefile and source code adjustments.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I *really* want Grant's opinion on this patch before merging
it...

> +++ 3.6-rc5-gpiolib-module/drivers/gpio/gpiolib.c
> @@ -1003,7 +1003,9 @@ static int __init gpiolib_sysfs_init(voi
>
>         return status;
>  }
> +#ifndef MODULE
>  postcore_initcall(gpiolib_sysfs_init);
> +#endif

#ifdefs are ugly but allright I don't know if there are any better
ways to do this.

(...)

> +#ifndef MODULE
>  subsys_initcall(gpiolib_debugfs_init);
> +#endif
>
>  #endif /* DEBUG_FS */
> +
> +#ifdef MODULE
> +int __init gpiolib_init(void)
> +{
> +       return gpiolib_sysfs_init() ?: gpiolib_debugfs_init();

I can't parse this, sorry the gpio subsystem maintainer is too bad coder.
What about something more readable?

int ret;

ret = gpiolib_sysfs_init();
if (ret)
   return ret;
return gpiolib_debugfs_init();

I know it doesn't look as cool but it's easier for me to understand.

There is a bug too: I don't think this compiles if you compile as
a module but disable debugfs. Try it out.

> +}
> +module_init(gpiolib_init);
> +
> +MODULE_DESCRIPTION("GPIO library");
> +MODULE_LICENSE("GPL");

These two things don't really need to be inside an #ifdef
right?

> +#endif

Yours,
Linus Walleij

Re: [PATCH] allow gpiolib to be a module

[Jan Beulich]

>>> On 11.09.12 at 19:38, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Sep 10, 2012 at 2:16 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> +#ifndef MODULE
>>  subsys_initcall(gpiolib_debugfs_init);
>> +#endif
>>
>>  #endif /* DEBUG_FS */
>> +
>> +#ifdef MODULE
>> +int __init gpiolib_init(void)
>> +{
>> +       return gpiolib_sysfs_init() ?: gpiolib_debugfs_init();
> 
> I can't parse this, sorry the gpio subsystem maintainer is too bad coder.
> What about something more readable?
> 
> int ret;
> 
> ret = gpiolib_sysfs_init();
> if (ret)
>    return ret;
> return gpiolib_debugfs_init();
> 
> I know it doesn't look as cool but it's easier for me to understand.

Okay, since you're the second one to complain despite there
being other uses of the construct in the same source file, I'll
replace it, ...

> There is a bug too: I don't think this compiles if you compile as
> a module but disable debugfs. Try it out.

... the more to get this one fixed. Will re-submit.

>> +}
>> +module_init(gpiolib_init);
>> +
>> +MODULE_DESCRIPTION("GPIO library");
>> +MODULE_LICENSE("GPL");
> 
> These two things don't really need to be inside an #ifdef
> right?

They don't need to be, but since we need a conditional now
anyway, it seemed reasonable to include these in there too.

Jan

Re: [PATCH] allow gpiolib to be a module

[Linus Walleij]

On Wed, Sep 12, 2012 at 9:43 AM, Jan Beulich <JBeulich@suse.com> wrote:

>> I can't parse this, sorry the gpio subsystem maintainer is too bad coder.
>> What about something more readable?
>>
>> int ret;
>>
>> ret = gpiolib_sysfs_init();
>> if (ret)
>>    return ret;
>> return gpiolib_debugfs_init();
>>
>> I know it doesn't look as cool but it's easier for me to understand.
>
> Okay, since you're the second one to complain despite there
> being other uses of the construct in the same source file, I'll
> replace it, ...

Sorry about that, but two wrongs does not make one right.
I'd be happy if you patch the other constructs too...

Yours,
Linus Walleij

Re: [PATCH] allow gpiolib to be a module

[Randy Dunlap]

On 09/12/2012 12:43 AM, Jan Beulich wrote:

>>>> On 11.09.12 at 19:38, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Sep 10, 2012 at 2:16 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>> +#ifndef MODULE
>>>  subsys_initcall(gpiolib_debugfs_init);
>>> +#endif
>>>
>>>  #endif /* DEBUG_FS */
>>> +
>>> +#ifdef MODULE
>>> +int __init gpiolib_init(void)
>>> +{
>>> +       return gpiolib_sysfs_init() ?: gpiolib_debugfs_init();
>>
>> I can't parse this, sorry the gpio subsystem maintainer is too bad coder.
>> What about something more readable?
>>
>> int ret;
>>
>> ret = gpiolib_sysfs_init();
>> if (ret)
>>    return ret;
>> return gpiolib_debugfs_init();
>>
>> I know it doesn't look as cool but it's easier for me to understand.
> 
> Okay, since you're the second one to complain despite there
> being other uses of the construct in the same source file, I'll
> replace it, ...


Does C just use the value generated from the left side of a ?:
expression for the middle (empty) expression or does it call the
function again (which would usually be bad)?


-- 
~Randy

Re: [PATCH] allow gpiolib to be a module

[Jan Beulich]

>>> On 12.09.12 at 19:41, Randy Dunlap <rdunlap@xenotime.net> wrote:
> On 09/12/2012 12:43 AM, Jan Beulich wrote:
>>>>> On 11.09.12 at 19:38, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Mon, Sep 10, 2012 at 2:16 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> +       return gpiolib_sysfs_init() ?: gpiolib_debugfs_init();
>>>
>>> I can't parse this, sorry the gpio subsystem maintainer is too bad coder.
>>> What about something more readable?
>>>
>>> int ret;
>>>
>>> ret = gpiolib_sysfs_init();
>>> if (ret)
>>>    return ret;
>>> return gpiolib_debugfs_init();
>>>
>>> I know it doesn't look as cool but it's easier for me to understand.
>> 
>> Okay, since you're the second one to complain despite there
>> being other uses of the construct in the same source file, I'll
>> replace it, ...
> 
> 
> Does C just use the value generated from the left side of a ?:
> expression for the middle (empty) expression or does it call the
> function again (which would usually be bad)?

It's the purpose of omitting the middle expression to avoid the
second evaluation of that expression (and obviously the compiler
can't generally do this on its own when seeing the same
expression used twice).

Jan