| Message ID | 1560796779-17117-1-git-send-email-info@metux.net |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/3] include: linux: i2c: more helpers for declaring i2c drivers | expand |
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.
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
> 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.
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
(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.
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)