| Message ID | 1503567070-115646-3-git-send-email-preid@electromag.com.au |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Aug 24, 2017 at 05:31:02PM +0800, Phil Reid wrote: > In preparation to adding of_i2c_setup_smbus_alert() move > i2c_setup_smbus_alert() to core module. of_i2c_setup_smbus_alert() > will call i2c_setup_smbus_alert() and this avoid module dependecy issues. I am not very happy with this but don't want to cause another delay. So, I hope we can discuss and fix it incrementally. From what it does, I really think both setup_alert functions belong to i2c-smbus.c. Three possibilities come to my mind (untested, though): a) use try_then_request_module somehow b) add to CONFIG_I2C something like: select I2C_SMBUS if OF c) get rid of i2c-smbus.c entirely and move it all into the core Dunno if a) or b) have been tried in the course of this series already? Kind regards, Wolfram
On 29/10/2017 23:44, Wolfram Sang wrote: > On Thu, Aug 24, 2017 at 05:31:02PM +0800, Phil Reid wrote: >> In preparation to adding of_i2c_setup_smbus_alert() move >> i2c_setup_smbus_alert() to core module. of_i2c_setup_smbus_alert() >> will call i2c_setup_smbus_alert() and this avoid module dependecy issues. > > I am not very happy with this but don't want to cause another delay. So, > I hope we can discuss and fix it incrementally. > > From what it does, I really think both setup_alert functions belong to > i2c-smbus.c. Three possibilities come to my mind (untested, though): > > a) use try_then_request_module somehow > > b) add to CONFIG_I2C something like: > select I2C_SMBUS if OF > > c) get rid of i2c-smbus.c entirely and move it all into the core > > Dunno if a) or b) have been tried in the course of this series already? > G'day Wolfram, Thanks for reviewing. Nope hadn't tried either a) or b). a) is not something I was aware of. Had a brief play and not sure how to make this work. This gives me the same linking problem. A variant of b) was tried by selecting I2C_SMBUS from driver modules having smbalert support, but that caused problems if the i2c core was builtin and the driver was a module. But this looks to work for me. A variant of c) with option to exclude the i2c-smbus code still. Add i2c-smbus to the i2c-core module by: Change Kconfig I2C_SMBUS to a bool add i2c-smbus to i2c-core object. fixup duplicate init_module calls etc. Possibly also rename i2c-smbus to i2c-smbus-alert? Let me know your preference and I'll put something together.
Hey Phil, (CCing Jean for some additional expertise) > > > In preparation to adding of_i2c_setup_smbus_alert() move > > > i2c_setup_smbus_alert() to core module. of_i2c_setup_smbus_alert() > > > will call i2c_setup_smbus_alert() and this avoid module dependecy issues. > > > > I am not very happy with this but don't want to cause another delay. So, > > I hope we can discuss and fix it incrementally. > > > > From what it does, I really think both setup_alert functions belong > > to i2c-smbus.c. Three possibilities come to my mind (untested, > > though): > > > > a) use try_then_request_module somehow > > > > b) add to CONFIG_I2C something like: select I2C_SMBUS if OF > > > > c) get rid of i2c-smbus.c entirely and move it all into the core > > > > Dunno if a) or b) have been tried in the course of this series > > already? > > > > Nope hadn't tried either a) or b). a) is not something I was aware of. > Had a brief play and not sure how to make this work. This gives me the > same linking problem. Which one? I2C core is built-in and the driver is a module? And i2c-smbus is a module then, too? > A variant of b) was tried by selecting I2C_SMBUS from driver modules > having smbalert support, but that caused problems if the i2c core was > builtin and the driver was a module. But this looks to work for me. It should work(tm). Yet, it will cause overhead, because OF is big these days and I2C_SMBUS is still rare. > A variant of c) with option to exclude the i2c-smbus code still. > Add i2c-smbus to the i2c-core module by: > Change Kconfig I2C_SMBUS to a bool > add i2c-smbus to i2c-core object. > fixup duplicate init_module calls etc. I see. From my side, why not. But I'd really like Jean's opinion here on why i2c-smbus.c was a seperate module and not included in the core. Because I split the I2C core into multiple source files recently, we could have a seperate source file for i2c-smbus-alert without ifdeffery but with some Makefile magic. > Possibly also rename i2c-smbus to i2c-smbus-alert? Why the rename? I thought we put all the code into the core? > Let me know your preference and I'll put something together. Thanks for the assistance. I am afraid we need a bit more discussion first, though. All the best, Wolfram
On 7/11/2017 05:36, Wolfram Sang wrote: > Hey Phil, > > (CCing Jean for some additional expertise) > >>>> In preparation to adding of_i2c_setup_smbus_alert() move >>>> i2c_setup_smbus_alert() to core module. of_i2c_setup_smbus_alert() >>>> will call i2c_setup_smbus_alert() and this avoid module dependecy issues. >>> >>> I am not very happy with this but don't want to cause another delay. So, >>> I hope we can discuss and fix it incrementally. >>> >>> From what it does, I really think both setup_alert functions belong >>> to i2c-smbus.c. Three possibilities come to my mind (untested, >>> though): >>> >>> a) use try_then_request_module somehow >>> >>> b) add to CONFIG_I2C something like: select I2C_SMBUS if OF >>> >>> c) get rid of i2c-smbus.c entirely and move it all into the core >>> >>> Dunno if a) or b) have been tried in the course of this series >>> already? >>> >> >> Nope hadn't tried either a) or b). a) is not something I was aware of. >> Had a brief play and not sure how to make this work. This gives me the >> same linking problem. > > Which one? I2C core is built-in and the driver is a module? And > i2c-smbus is a module then, too? Yes. I may be doing something wrong with how to use a) thou.. I do like conditional pulling the alert code in the to core if everyone else is happy with that. > >> A variant of b) was tried by selecting I2C_SMBUS from driver modules >> having smbalert support, but that caused problems if the i2c core was >> builtin and the driver was a module. But this looks to work for me. > > It should work(tm). Yet, it will cause overhead, because OF is big these > days and I2C_SMBUS is still rare. > >> A variant of c) with option to exclude the i2c-smbus code still. >> Add i2c-smbus to the i2c-core module by: >> Change Kconfig I2C_SMBUS to a bool >> add i2c-smbus to i2c-core object. >> fixup duplicate init_module calls etc. > > I see. From my side, why not. But I'd really like Jean's opinion here on > why i2c-smbus.c was a seperate module and not included in the core. > Because I split the I2C core into multiple source files recently, we > could have a seperate source file for i2c-smbus-alert without ifdeffery > but with some Makefile magic. > >> Possibly also rename i2c-smbus to i2c-smbus-alert? > > Why the rename? I thought we put all the code into the core? I was just thinking that all that's in the file now is smbus-alert code. The other smbus (host notify) code was moved to i2c-core So maybe i2c-core-smbus-alert and as you say use makefile to include it. I've tried this and it seems to work for the various combinations. I'll wait a bit for Jean to offer an opinion. > >> Let me know your preference and I'll put something together. > > Thanks for the assistance. I am afraid we need a bit more discussion > first, though.
> I was just thinking that all that's in the file now is smbus-alert code. > The other smbus (host notify) code was moved to i2c-core > So maybe i2c-core-smbus-alert and as you say use makefile to include it. Aaaah, i2c-core-smbus-alert.c. Yes, that probably makes sense. I was wondering about i2c-smbus-alert.c (without -core)... > I've tried this and it seems to work for the various combinations. > I'll wait a bit for Jean to offer an opinion. Well, if you have code already, can you post it as RFC and CC Jean? It usually is easier to talk over code, even if it is not perfect yet...
On 7/11/2017 15:22, Wolfram Sang wrote: > >> I was just thinking that all that's in the file now is smbus-alert code. >> The other smbus (host notify) code was moved to i2c-core >> So maybe i2c-core-smbus-alert and as you say use makefile to include it. > > Aaaah, i2c-core-smbus-alert.c. Yes, that probably makes sense. I was > wondering about i2c-smbus-alert.c (without -core)... > >> I've tried this and it seems to work for the various combinations. >> I'll wait a bit for Jean to offer an opinion. > > Well, if you have code already, can you post it as RFC and CC Jean? > It usually is easier to talk over code, even if it is not perfect yet... > Yes will do. Was hoping to clean it up into some incremental patches and post this week. But it looks like next week now.
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c index 10f00a8..7f3ec02 100644 --- a/drivers/i2c/i2c-core-smbus.c +++ b/drivers/i2c/i2c-core-smbus.c @@ -17,6 +17,7 @@ #include <linux/device.h> #include <linux/err.h> #include <linux/i2c.h> +#include <linux/i2c-smbus.h> #define CREATE_TRACE_POINTS #include <trace/events/smbus.h> @@ -592,3 +593,35 @@ s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client, return i; } EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data_or_emulated); + +/** + * i2c_setup_smbus_alert - Setup SMBus alert support + * @adapter: the target adapter + * @setup: setup data for the SMBus alert handler + * Context: can sleep + * + * Setup handling of the SMBus alert protocol on a given I2C bus segment. + * + * Handling can be done either through our IRQ handler, or by the + * adapter (from its handler, periodic polling, or whatever). + * + * NOTE that if we manage the IRQ, we *MUST* know if it's level or + * edge triggered in order to hand it to the workqueue correctly. + * If triggering the alert seems to wedge the system, you probably + * should have said it's level triggered. + * + * This returns the ara client, which should be saved for later use with + * i2c_handle_smbus_alert() and ultimately i2c_unregister_device(); or NULL + * to indicate an error. + */ +struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter, + struct i2c_smbus_alert_setup *setup) +{ + struct i2c_board_info ara_board_info = { + I2C_BOARD_INFO("smbus_alert", 0x0c), + .platform_data = setup, + }; + + return i2c_new_device(adapter, &ara_board_info); +} +EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert); diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c index d4af270..d0bb035 100644 --- a/drivers/i2c/i2c-smbus.c +++ b/drivers/i2c/i2c-smbus.c @@ -183,38 +183,6 @@ static int smbalert_remove(struct i2c_client *ara) }; /** - * i2c_setup_smbus_alert - Setup SMBus alert support - * @adapter: the target adapter - * @setup: setup data for the SMBus alert handler - * Context: can sleep - * - * Setup handling of the SMBus alert protocol on a given I2C bus segment. - * - * Handling can be done either through our IRQ handler, or by the - * adapter (from its handler, periodic polling, or whatever). - * - * NOTE that if we manage the IRQ, we *MUST* know if it's level or - * edge triggered in order to hand it to the workqueue correctly. - * If triggering the alert seems to wedge the system, you probably - * should have said it's level triggered. - * - * This returns the ara client, which should be saved for later use with - * i2c_handle_smbus_alert() and ultimately i2c_unregister_device(); or NULL - * to indicate an error. - */ -struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter, - struct i2c_smbus_alert_setup *setup) -{ - struct i2c_board_info ara_board_info = { - I2C_BOARD_INFO("smbus_alert", 0x0c), - .platform_data = setup, - }; - - return i2c_new_device(adapter, &ara_board_info); -} -EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert); - -/** * i2c_handle_smbus_alert - Handle an SMBus alert * @ara: the ARA client on the relevant adapter * Context: can't sleep
In preparation to adding of_i2c_setup_smbus_alert() move i2c_setup_smbus_alert() to core module. of_i2c_setup_smbus_alert() will call i2c_setup_smbus_alert() and this avoid module dependecy issues. Signed-off-by: Phil Reid <preid@electromag.com.au> --- drivers/i2c/i2c-core-smbus.c | 33 +++++++++++++++++++++++++++++++++ drivers/i2c/i2c-smbus.c | 32 -------------------------------- 2 files changed, 33 insertions(+), 32 deletions(-)