[1/3] include: linux: i2c: more helpers for declaring i2c drivers
diff mbox series

Message ID 1560796779-17117-1-git-send-email-info@metux.net
State New
Headers show
Series
  • [1/3] include: linux: i2c: more helpers for declaring i2c drivers
Related show

Commit Message

Enrico Weigelt, metux IT consult June 17, 2019, 6:39 p.m. UTC
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(+)

Comments

Wolfram Sang June 21, 2019, 9:17 p.m. UTC | #1
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.
Enrico Weigelt, metux IT consult June 24, 2019, 5:44 a.m. UTC | #2
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
Wolfram Sang June 24, 2019, 8:44 a.m. UTC | #3
> 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.
Enrico Weigelt, metux IT consult June 24, 2019, 8:59 a.m. UTC | #4
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
Wolfram Sang Aug. 16, 2019, 7:56 p.m. UTC | #5
(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.

Patch
diff mbox series

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)