[PATCH 1/3] include: linux: i2c: more helpers for declaring i2c drivers

[PATCH 1/3] include: linux: i2c: more helpers for declaring i2c drivers

[Enrico Weigelt, metux IT consult]

From: Enrico Weigelt <info@metux.net>

Add more helper macros for trivial driver init cases, similar to the
already existing module_i2c_driver()+friends - now for those which
are initialized at other stages (eg. by subsys_initcall()).

This helps to further reduce driver init boilerplate.

Signed-off-by: Enrico Weigelt <info@metux.net>
---
 include/linux/i2c.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 1308126..fee59bd 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -920,6 +920,23 @@ static inline u8 i2c_8bit_addr_from_msg(const struct i2c_msg *msg)
 #define builtin_i2c_driver(__i2c_driver) \
 	builtin_driver(__i2c_driver, i2c_add_driver)
 
+/* subsys_i2c_driver() - Helper macro for drivers that don't do
+ * anything special in module init/exit.  This eliminates a lot of
+ * boilerplate.  Each module may only use this macro once, and
+ * calling it replaces subsys_initcall() and module_exit()
+ */
+#define subsys_i2c_driver(__i2c_driver) \
+static int __init __i2c_driver##_init(void) \
+{ \
+       return i2c_add_driver(&(__i2c_driver)); \
+} \
+subsys_initcall(__i2c_driver##_init); \
+static void __exit __i2c_driver##_exit(void) \
+{ \
+       i2c_del_driver(&(__i2c_driver)); \
+} \
+module_exit(__i2c_driver##_exit);
+
 #endif /* I2C */
 
 #if IS_ENABLED(CONFIG_OF)
-- 
1.9.1

[PATCH 2/3] drivers: gpio: pca953x: use subsys_i2c_driver()

[Enrico Weigelt, metux IT consult]

From: Enrico Weigelt <info@metux.net>

Reduce driver init boilerplate by using the new
subsys_i2c_driver() macro.

Signed-off-by: Enrico Weigelt <info@metux.net>
---
 drivers/gpio/gpio-pca953x.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index cfe827c..532762d 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1202,20 +1202,10 @@ static int pca953x_resume(struct device *dev)
 	.id_table	= pca953x_id,
 };
 
-static int __init pca953x_init(void)
-{
-	return i2c_add_driver(&pca953x_driver);
-}
 /* register after i2c postcore initcall and before
  * subsys initcalls that may rely on these GPIOs
  */
-subsys_initcall(pca953x_init);
-
-static void __exit pca953x_exit(void)
-{
-	i2c_del_driver(&pca953x_driver);
-}
-module_exit(pca953x_exit);
+subsys_i2c_driver(pca953x_driver);
 
 MODULE_AUTHOR("eric miao <eric.miao@marvell.com>");
 MODULE_DESCRIPTION("GPIO expander driver for PCA953x");
-- 
1.9.1

[PATCH 3/3] drivers: gpio: pcf857x: use subsys_i2c_driver()

[Enrico Weigelt, metux IT consult]

From: Enrico Weigelt <info@metux.net>

Reduce driver init boilerplate by using the new
subsys_i2c_driver() macro.

Signed-off-by: Enrico Weigelt <info@metux.net>
---
 drivers/gpio/gpio-pcf857x.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
index 14fb8f6..554663e 100644
--- a/drivers/gpio/gpio-pcf857x.c
+++ b/drivers/gpio/gpio-pcf857x.c
@@ -430,20 +430,10 @@ static void pcf857x_shutdown(struct i2c_client *client)
 	.id_table = pcf857x_id,
 };
 
-static int __init pcf857x_init(void)
-{
-	return i2c_add_driver(&pcf857x_driver);
-}
 /* register after i2c postcore initcall and before
  * subsys initcalls that may rely on these GPIOs
  */
-subsys_initcall(pcf857x_init);
-
-static void __exit pcf857x_exit(void)
-{
-	i2c_del_driver(&pcf857x_driver);
-}
-module_exit(pcf857x_exit);
+subsys_i2c_driver(pcf857x_driver);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("David Brownell");
-- 
1.9.1

Re: [PATCH 1/3] include: linux: i2c: more helpers for declaring i2c drivers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

On Mon, Jun 17, 2019 at 08:39:37PM +0200, Enrico Weigelt, metux IT consult wrote:
> From: Enrico Weigelt <info@metux.net>
> 
> Add more helper macros for trivial driver init cases, similar to the
> already existing module_i2c_driver()+friends - now for those which
> are initialized at other stages (eg. by subsys_initcall()).
> 
> This helps to further reduce driver init boilerplate.

Uh, no! Using subsys_initcall is an old fashioned hack to work around
boot time dependencies. Unless there are very strong arguments, I
usually do not accept them anymore. So, any simplification of that sends
out the wrong message.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] include: linux: i2c: more helpers for declaring i2c drivers

[Enrico Weigelt, metux IT consult]

On 21.06.19 23:17, Wolfram Sang wrote:
> On Mon, Jun 17, 2019 at 08:39:37PM +0200, Enrico Weigelt, metux IT consult wrote:
>> From: Enrico Weigelt <info@metux.net>
>>
>> Add more helper macros for trivial driver init cases, similar to the
>> already existing module_i2c_driver()+friends - now for those which
>> are initialized at other stages (eg. by subsys_initcall()).
>>
>> This helps to further reduce driver init boilerplate.
> 
> Uh, no! Using subsys_initcall is an old fashioned hack to work around
> boot time dependencies. Unless there are very strong arguments, I
> usually do not accept them anymore. So, any simplification of that sends
> out the wrong message.

Okay, what's the correct initialization method then ?
Just convert it to already existing module_i2c_driver() ?


--mtx

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

Re: [PATCH 1/3] include: linux: i2c: more helpers for declaring i2c drivers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]


> Okay, what's the correct initialization method then ?
> Just convert it to already existing module_i2c_driver() ?

"module_platform_driver" you mean? That's tricky because it can
introduce regressions easily. I had one situation where one wanted
subsys_init and one wanted module_init.

The correct solution is to fix the boot dependency in the affected I2C
client drivers. That definately needs HW and thorough testing.

It may also need something better than the current deferred probe. Big
topic.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] include: linux: i2c: more helpers for declaring i2c drivers

[Enrico Weigelt, metux IT consult]

On 24.06.19 10:44, Wolfram Sang wrote:

> The correct solution is to fix the boot dependency in the affected I2C
> client drivers. That definately needs HW and thorough testing.
> 
> It may also need something better than the current deferred probe. Big
> topic.

So, then the current approach of using subsys_initcall() can't be
changed easily, right now. But planned for the future (or at least
not introducing new caes).

But: how does that conflict w/ just moving the existing redundant
pieces into a helper macro ? The logic stays the same - just using
a shorter notation. (assuming my patch isn't buggy ;-)).

I can add a remark in the function documentation that this shall
only by used in rare cases, and maybe something like "that's just
legacy  - introducing  new caller is most certainly wrong" ;-)


--mtx

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

Re: [PATCH 1/3] include: linux: i2c: more helpers for declaring i2c drivers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]


(Found this mail in the offline draft folder of another laptop)

> So, then the current approach of using subsys_initcall() can't be
> changed easily, right now. But planned for the future (or at least
> not introducing new caes).

Yes.

> But: how does that conflict w/ just moving the existing redundant
> pieces into a helper macro ? The logic stays the same - just using
> a shorter notation. (assuming my patch isn't buggy ;-)).

It is not conflicting. My thinking is that such helpers, in general,
scale better and are less error prone. But there is nothing to scale
here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]