* [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol @ 2017-01-10 12:16 Arnd Bergmann 2017-01-11 0:39 ` Andrew Duggan 2017-01-13 6:22 ` Dmitry Torokhov 0 siblings, 2 replies; 16+ messages in thread From: Arnd Bergmann @ 2017-01-10 12:16 UTC (permalink / raw) To: Dmitry Torokhov Cc: Arnd Bergmann, Andrew Duggan, Christopher Heiny, Nick Dyer, linux-input, linux-kernel If CONFIG_INPUT=m, we get a build error for the rmi4-f03 driver, added in linux-4.10: drivers/input/built-in.o: In function `rmi_f03_attention': rmi_f03.c:(.text+0xcfe0): undefined reference to `serio_interrupt' rmi_f03.c:(.text+0xd055): undefined reference to `serio_interrupt' drivers/input/built-in.o: In function `rmi_f03_remove': rmi_f03.c:(.text+0xd115): undefined reference to `serio_unregister_port' drivers/input/built-in.o: In function `rmi_f03_probe': rmi_f03.c:(.text+0xd209): undefined reference to `__serio_register_port' If we make the driver itself a 'tristate' instead of 'bool' symbol, Kconfig ensures that it can only be a loadable module in this case, which avoids the problem. Fixes: c5e8848fc98e ("Input: synaptics-rmi4 - add support for F03") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/input/rmi4/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig index 30cc627a4f45..ad945b25722c 100644 --- a/drivers/input/rmi4/Kconfig +++ b/drivers/input/rmi4/Kconfig @@ -40,7 +40,7 @@ config RMI4_SMB called rmi_smbus. config RMI4_F03 - bool "RMI4 Function 03 (PS2 Guest)" + tristate "RMI4 Function 03 (PS2 Guest)" depends on RMI4_CORE && SERIO help Say Y here if you want to add support for RMI4 function 03. -- 2.9.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-10 12:16 [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol Arnd Bergmann @ 2017-01-11 0:39 ` Andrew Duggan 2017-01-11 15:39 ` Arnd Bergmann 2017-01-13 6:22 ` Dmitry Torokhov 1 sibling, 1 reply; 16+ messages in thread From: Andrew Duggan @ 2017-01-11 0:39 UTC (permalink / raw) To: Arnd Bergmann, Dmitry Torokhov Cc: Christopher Heiny, Nick Dyer, linux-input, linux-kernel, Benjamin Tissoires, Lyude Paul On 01/10/2017 04:16 AM, Arnd Bergmann wrote: > If CONFIG_INPUT=m, we get a build error for the rmi4-f03 driver, > added in linux-4.10: > > drivers/input/built-in.o: In function `rmi_f03_attention': > rmi_f03.c:(.text+0xcfe0): undefined reference to `serio_interrupt' > rmi_f03.c:(.text+0xd055): undefined reference to `serio_interrupt' > drivers/input/built-in.o: In function `rmi_f03_remove': > rmi_f03.c:(.text+0xd115): undefined reference to `serio_unregister_port' > drivers/input/built-in.o: In function `rmi_f03_probe': > rmi_f03.c:(.text+0xd209): undefined reference to `__serio_register_port' > > If we make the driver itself a 'tristate' instead of 'bool' symbol, > Kconfig ensures that it can only be a loadable module in this case, > which avoids the problem. Unfortunately, the RMI4 driver does not support building the function drivers as modules. If F03 is built as a module it will not be loaded by the core. If we want f03 to be part of a module then rmi_core needs to be built as a module. We should remove the module macros currently in rmi_f03.c. I was able to get a similar build error by setting CONFIG_RMI_CORE=y and CONFIG_SERIO=m. Was CONFIG_RMI_CORE=y set when you encountered this error? If so I think we should figure out a way to have Kconfig set CONFIG_RMI_CORE=m if serio is built as a module. Andrew > Fixes: c5e8848fc98e ("Input: synaptics-rmi4 - add support for F03") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/input/rmi4/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig > index 30cc627a4f45..ad945b25722c 100644 > --- a/drivers/input/rmi4/Kconfig > +++ b/drivers/input/rmi4/Kconfig > @@ -40,7 +40,7 @@ config RMI4_SMB > called rmi_smbus. > > config RMI4_F03 > - bool "RMI4 Function 03 (PS2 Guest)" > + tristate "RMI4 Function 03 (PS2 Guest)" > depends on RMI4_CORE && SERIO > help > Say Y here if you want to add support for RMI4 function 03. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-11 0:39 ` Andrew Duggan @ 2017-01-11 15:39 ` Arnd Bergmann 2017-01-11 16:28 ` Benjamin Tissoires 0 siblings, 1 reply; 16+ messages in thread From: Arnd Bergmann @ 2017-01-11 15:39 UTC (permalink / raw) To: Andrew Duggan Cc: Dmitry Torokhov, Christopher Heiny, Nick Dyer, linux-input, linux-kernel, Benjamin Tissoires, Lyude Paul On Tuesday, January 10, 2017 4:39:43 PM CET Andrew Duggan wrote: > On 01/10/2017 04:16 AM, Arnd Bergmann wrote: > > If CONFIG_INPUT=m, we get a build error for the rmi4-f03 driver, > > added in linux-4.10: > > > > drivers/input/built-in.o: In function `rmi_f03_attention': > > rmi_f03.c:(.text+0xcfe0): undefined reference to `serio_interrupt' > > rmi_f03.c:(.text+0xd055): undefined reference to `serio_interrupt' > > drivers/input/built-in.o: In function `rmi_f03_remove': > > rmi_f03.c:(.text+0xd115): undefined reference to `serio_unregister_port' > > drivers/input/built-in.o: In function `rmi_f03_probe': > > rmi_f03.c:(.text+0xd209): undefined reference to `__serio_register_port' > > > > If we make the driver itself a 'tristate' instead of 'bool' symbol, > > Kconfig ensures that it can only be a loadable module in this case, > > which avoids the problem. > > Unfortunately, the RMI4 driver does not support building the function > drivers as modules. If F03 is built as a module it will not be loaded by > the core. If we want f03 to be part of a module then rmi_core needs to > be built as a module. We should remove the module macros currently in > rmi_f03.c. > > I was able to get a similar build error by setting CONFIG_RMI_CORE=y and > CONFIG_SERIO=m. Was CONFIG_RMI_CORE=y set when you encountered this > error? If so I think we should figure out a way to have Kconfig set > CONFIG_RMI_CORE=m if serio is built as a module. Ok, I see what you mean now in static struct rmi_function_handler *fn_handlers[] = { &rmi_f01_handler, #ifdef CONFIG_RMI4_F03 &rmi_f03_handler, #endif #ifdef CONFIG_RMI4_F11 &rmi_f11_handler, #endif ... }; I think we can actually make this more modular and more like other drivers work: If each of the sub-drivers gets changed to call rmi_register_function_handler() on its own handler structure, having some drivers as modules would just work. It looks like the rmi_bus.c file was written to do it that way, but for some reason the references to those drivers are all in the same file. Arnd ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-11 15:39 ` Arnd Bergmann @ 2017-01-11 16:28 ` Benjamin Tissoires 2017-01-11 16:54 ` Arnd Bergmann 0 siblings, 1 reply; 16+ messages in thread From: Benjamin Tissoires @ 2017-01-11 16:28 UTC (permalink / raw) To: Arnd Bergmann Cc: Andrew Duggan, Dmitry Torokhov, Christopher Heiny, Nick Dyer, linux-input, linux-kernel, Lyude Paul On Jan 11 2017 or thereabouts, Arnd Bergmann wrote: > On Tuesday, January 10, 2017 4:39:43 PM CET Andrew Duggan wrote: > > On 01/10/2017 04:16 AM, Arnd Bergmann wrote: > > > If CONFIG_INPUT=m, we get a build error for the rmi4-f03 driver, > > > added in linux-4.10: > > > > > > drivers/input/built-in.o: In function `rmi_f03_attention': > > > rmi_f03.c:(.text+0xcfe0): undefined reference to `serio_interrupt' > > > rmi_f03.c:(.text+0xd055): undefined reference to `serio_interrupt' > > > drivers/input/built-in.o: In function `rmi_f03_remove': > > > rmi_f03.c:(.text+0xd115): undefined reference to `serio_unregister_port' > > > drivers/input/built-in.o: In function `rmi_f03_probe': > > > rmi_f03.c:(.text+0xd209): undefined reference to `__serio_register_port' > > > > > > If we make the driver itself a 'tristate' instead of 'bool' symbol, > > > Kconfig ensures that it can only be a loadable module in this case, > > > which avoids the problem. > > > > Unfortunately, the RMI4 driver does not support building the function > > drivers as modules. If F03 is built as a module it will not be loaded by > > the core. If we want f03 to be part of a module then rmi_core needs to > > be built as a module. We should remove the module macros currently in > > rmi_f03.c. > > > > I was able to get a similar build error by setting CONFIG_RMI_CORE=y and > > CONFIG_SERIO=m. Was CONFIG_RMI_CORE=y set when you encountered this > > error? If so I think we should figure out a way to have Kconfig set > > CONFIG_RMI_CORE=m if serio is built as a module. > > > Ok, I see what you mean now in > > static struct rmi_function_handler *fn_handlers[] = { > &rmi_f01_handler, > #ifdef CONFIG_RMI4_F03 > &rmi_f03_handler, > #endif > #ifdef CONFIG_RMI4_F11 > &rmi_f11_handler, > #endif > ... > }; > > I think we can actually make this more modular and more like other > drivers work: > > If each of the sub-drivers gets changed to call > rmi_register_function_handler() on its own handler structure, > having some drivers as modules would just work. > > It looks like the rmi_bus.c file was written to do it that way, > but for some reason the references to those drivers are all > in the same file. > Yep, it was initially written that way, and IIRC there was some issues depending on how the drivers were compiled. For example, if rmi4_core is Y and some functions are m, you can't load the device initially, so you send a -EPROBE_DEFER, but how can you be sure that the function will ever be loaded? Given that we need to have all the functions loaded during probe, we decided to switch to a monolithic rmi4_core driver that has everything it needs inside. I think having the dependency on SERIO=m implying RMI4_CORE=m should be doable and is probably the best solution with the current code. Cheers, Benjamin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-11 16:28 ` Benjamin Tissoires @ 2017-01-11 16:54 ` Arnd Bergmann 2017-01-11 17:48 ` Benjamin Tissoires 0 siblings, 1 reply; 16+ messages in thread From: Arnd Bergmann @ 2017-01-11 16:54 UTC (permalink / raw) To: Benjamin Tissoires Cc: Andrew Duggan, Dmitry Torokhov, Christopher Heiny, Nick Dyer, linux-input, linux-kernel, Lyude Paul On Wednesday, January 11, 2017 5:28:28 PM CET Benjamin Tissoires wrote: > Yep, it was initially written that way, and IIRC there was some issues > depending on how the drivers were compiled. For example, if rmi4_core is > Y and some functions are m, you can't load the device initially, so you > send a -EPROBE_DEFER, but how can you be sure that the function will > ever be loaded? I'm not sure if I understand your problem correctly, but normally the way it's done is that the bus driver notifies user space that a new device has appeared on the bus, and udev looks for the right driver for the device, using a MODULE_DEVICE_TABLE list. Once the driver gets loaded, it binds to the device. > Given that we need to have all the functions loaded during probe, we > decided to switch to a monolithic rmi4_core driver that has everything > it needs inside. If everything is in one module, you can probably get rid of at least part of the bus abstraction as well and just call the functions directly. Looking through the driver some more, I also find the 'rmi_driver rmi_physical_driver' concept very odd, you seem to have a device on the bus that is actually just another representation of the parent device and that creates another set of devices for the functions. Either I misunderstand what this is for, or you have a candidate for cleanup there and once you remove it (by calling rmi_driver_probe() instead of rmi_register_transport_device() to oversimplify the idea), the actual probing for the function drivers becomes much easier to do right. Arnd ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-11 16:54 ` Arnd Bergmann @ 2017-01-11 17:48 ` Benjamin Tissoires 2017-01-11 19:27 ` Christopher Heiny 0 siblings, 1 reply; 16+ messages in thread From: Benjamin Tissoires @ 2017-01-11 17:48 UTC (permalink / raw) To: Arnd Bergmann Cc: Andrew Duggan, Dmitry Torokhov, Christopher Heiny, Nick Dyer, linux-input, linux-kernel, Lyude Paul On Jan 11 2017 or thereabouts, Arnd Bergmann wrote: > On Wednesday, January 11, 2017 5:28:28 PM CET Benjamin Tissoires wrote: > > Yep, it was initially written that way, and IIRC there was some issues > > depending on how the drivers were compiled. For example, if rmi4_core is > > Y and some functions are m, you can't load the device initially, so you > > send a -EPROBE_DEFER, but how can you be sure that the function will > > ever be loaded? > > I'm not sure if I understand your problem correctly, but normally > the way it's done is that the bus driver notifies user space that > a new device has appeared on the bus, and udev looks for the right > driver for the device, using a MODULE_DEVICE_TABLE list. Once the > driver gets loaded, it binds to the device. > I agree, but we never managed to make it properly working for RMI4. See https://lkml.org/lkml/2015/11/5/726 where we decided to switch to a static list of functions. Maybe we did not try hard enough, but we kept the current bus/functions_as_drivers to be able to switch back to the modular option, > > Given that we need to have all the functions loaded during probe, we > > decided to switch to a monolithic rmi4_core driver that has everything > > it needs inside. > > If everything is in one module, you can probably get rid of at > least part of the bus abstraction as well and just call the functions > directly. Agree, though that means we won't be able to switch back. In the current form it's overly engineered. > > Looking through the driver some more, I also find the > 'rmi_driver rmi_physical_driver' concept very odd, you seem to > have a device on the bus that is actually just another representation > of the parent device and that creates another set of devices for > the functions. Either I misunderstand what this is for, or you have I think you have this right. > a candidate for cleanup there and once you remove it (by calling > rmi_driver_probe() instead of rmi_register_transport_device() > to oversimplify the idea), the actual probing for the function > drivers becomes much easier to do right. > Agree, that would simplify the code a lot. I just don't know how important it is for other users of RMI4 to have a modular solution or if the monolithic approach is a consensus now. The modular solution is currently disabled, but we can "always" switch back with a small effort. My opinion on this matter is that there is no need for the modular functions, but others might have a different opinion. Cheers, Benjamin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-11 17:48 ` Benjamin Tissoires @ 2017-01-11 19:27 ` Christopher Heiny 2017-01-13 0:42 ` Andrew Duggan 0 siblings, 1 reply; 16+ messages in thread From: Christopher Heiny @ 2017-01-11 19:27 UTC (permalink / raw) To: Benjamin Tissoires, Arnd Bergmann Cc: Andrew Duggan, Dmitry Torokhov, Nick Dyer, linux-input, linux-kernel, Lyude Paul On Wed, 2017-01-11 at 18:48 +0100, Benjamin Tissoires wrote: > On Jan 11 2017 or thereabouts, Arnd Bergmann wrote: > > > > On Wednesday, January 11, 2017 5:28:28 PM CET Benjamin Tissoires > > wrote: > > > > > > Yep, it was initially written that way, and IIRC there was some > > > issues > > > depending on how the drivers were compiled. For example, if > > > rmi4_core is > > > Y and some functions are m, you can't load the device initially, > > > so you > > > send a -EPROBE_DEFER, but how can you be sure that the function > > > will > > > ever be loaded? > > > > I'm not sure if I understand your problem correctly, but normally > > the way it's done is that the bus driver notifies user space that > > a new device has appeared on the bus, and udev looks for the right > > driver for the device, using a MODULE_DEVICE_TABLE list. Once the > > driver gets loaded, it binds to the device. > > > > I agree, but we never managed to make it properly working for RMI4. > See > https://lkml.org/lkml/2015/11/5/726 where we decided to switch to a > static list of functions. Maybe we did not try hard enough, but we > kept > the current bus/functions_as_drivers to be able to switch back to the > modular option, Actually, long, long ago (well before I got yanked off the RMI4 driver project for a 'short term emergency' two and a half years ago - fortunately Andrew was more than able to take it over) it worked that way. If you had the bus, a physical driver, and the sensor driver running, you could build the functions as modules and attach them via udev or insert them later. We had this working on the bench at one point, but fairly early in the submission process seem to have just assumed it would keep working and stopped regression testing on that feature. There have been an whole lot of changes since then, and somewhere along the line functions-as- loadable-modules stopped working. Since we weren't testing it anymore, it wasn't caught. > > > > > > > > > Given that we need to have all the functions loaded during probe, > > > we > > > decided to switch to a monolithic rmi4_core driver that has > > > everything > > > it needs inside. > > > > If everything is in one module, you can probably get rid of at > > least part of the bus abstraction as well and just call the > > functions > > directly. > > Agree, though that means we won't be able to switch back. In the > current > form it's overly engineered. Well, I'm not sure I agree with that. :-) More on that later in this email. > > > > > > > Looking through the driver some more, I also find the > > 'rmi_driver rmi_physical_driver' concept very odd, you seem to > > have a device on the bus that is actually just another > > representation > > of the parent device and that creates another set of devices for > > the functions. Either I misunderstand what this is for, or you have > > I think you have this right. > > > > > a candidate for cleanup there and once you remove it (by calling > > rmi_driver_probe() instead of rmi_register_transport_device() > > to oversimplify the idea), the actual probing for the function > > drivers becomes much easier to do right. > > > > Agree, that would simplify the code a lot. I just don't know how > important it is for other users of RMI4 to have a modular solution or > if > the monolithic approach is a consensus now. The modular solution is > currently disabled, but we can "always" switch back with a small > effort. > > My opinion on this matter is that there is no need for the modular > functions, but others might have a different opinion. The primary impetus for the original modular function design was to allow phone/tablet/etc manufacturers to write their own handlers for some functions without having to write the entire driver or hack up an existing monolithic driver. This simplifies engineering for the device manufacturer a lot. We also hoped that other peripheral manufacturers would pick up RMI4 for other sorts of sensors and devices - it is an open standard after all. However, this didn't happen, possibly due to the complete lack of promotion of RMI4 as a standard. Modular functions were also intended to make it easier for community OSes like AOKP or Paranoid Android to get up and running on products where in order to fulfill their GPL obligations, the manufacturer simply dumped a pile of sources, which might not actually be working. I'll grant that this could also be achieved with a monolithic driver. Several years ago, there also appeared to be a major trend toward having two or more sensors in a given product, which was a lot easier to manage with modular functions. Although lots of prototypes were built, very few went to production - maybe even none. And finally, I wanted to enable a platform that was easy for Raspberry Pi type tinkering with touch sensing functions - modular functions isolated the parts of the RMI4 system, so the tinkerer need only learn the parts they wanted to putter with. Anyway, I still think modular functions have important benefits as outlined above, and thus the driver is not over-engineered - it simply (or complicatedly :-) ) has the bug that modular functions don't currently work. However, since I'm not very active on this project right now, and not likely to be in the near future, if you folks decide you want to take the driver structure in a different direction, I can live with that. Cheers, Chris ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-11 19:27 ` Christopher Heiny @ 2017-01-13 0:42 ` Andrew Duggan 2017-01-13 21:14 ` Arnd Bergmann 0 siblings, 1 reply; 16+ messages in thread From: Andrew Duggan @ 2017-01-13 0:42 UTC (permalink / raw) To: Christopher Heiny, Benjamin Tissoires, Arnd Bergmann Cc: Dmitry Torokhov, Nick Dyer, linux-input, linux-kernel, Lyude Paul On 01/11/2017 11:27 AM, Christopher Heiny wrote: > On Wed, 2017-01-11 at 18:48 +0100, Benjamin Tissoires wrote: >> On Jan 11 2017 or thereabouts, Arnd Bergmann wrote: >>> On Wednesday, January 11, 2017 5:28:28 PM CET Benjamin Tissoires >>> wrote: >>>> Yep, it was initially written that way, and IIRC there was some >>>> issues >>>> depending on how the drivers were compiled. For example, if >>>> rmi4_core is >>>> Y and some functions are m, you can't load the device initially, >>>> so you >>>> send a -EPROBE_DEFER, but how can you be sure that the function >>>> will >>>> ever be loaded? >>> I'm not sure if I understand your problem correctly, but normally >>> the way it's done is that the bus driver notifies user space that >>> a new device has appeared on the bus, and udev looks for the right >>> driver for the device, using a MODULE_DEVICE_TABLE list. Once the >>> driver gets loaded, it binds to the device. >>> >> I agree, but we never managed to make it properly working for RMI4. >> See >> https://lkml.org/lkml/2015/11/5/726 where we decided to switch to a >> static list of functions. Maybe we did not try hard enough, but we >> kept >> the current bus/functions_as_drivers to be able to switch back to the >> modular option, > Actually, long, long ago (well before I got yanked off the RMI4 driver > project for a 'short term emergency' two and a half years ago - > fortunately Andrew was more than able to take it over) it worked that > way. If you had the bus, a physical driver, and the sensor driver > running, you could build the functions as modules and attach them via > udev or insert them later. > > We had this working on the bench at one point, but fairly early in the > submission process seem to have just assumed it would keep working and > stopped regression testing on that feature. There have been an whole > lot of changes since then, and somewhere along the line functions-as- > loadable-modules stopped working. Since we weren't testing it anymore, > it wasn't caught. > We made the decision to disable support for function drivers as modules after running into a few technical challenges which happened during initialization. The priority was to get the driver upstreamed so we put off getting function drivers working as modules until there was a compelling reason to do so. But, we left the structure mostly intact if we decided to do so. Here are the messages which describe what we discovered at the time: https://lkml.org/lkml/2015/11/10/92 https://lkml.org/lkml/2015/11/12/652 I'm basically coming to the same conclusion I had back then. Getting loadable modules for function drivers to work would add complexity for not a lot of benefit based on the current state of the driver and how it is currently being used. I'm also not sure we will reach the critical mass of function drivers where there is a significant benefit of not having to load them all. >>>> Given that we need to have all the functions loaded during probe, >>>> we >>>> decided to switch to a monolithic rmi4_core driver that has >>>> everything >>>> it needs inside. >>> If everything is in one module, you can probably get rid of at >>> least part of the bus abstraction as well and just call the >>> functions >>> directly. >> Agree, though that means we won't be able to switch back. In the >> current >> form it's overly engineered. > Well, I'm not sure I agree with that. :-) More on that later in this > email. > >>> >>> Looking through the driver some more, I also find the >>> 'rmi_driver rmi_physical_driver' concept very odd, you seem to >>> have a device on the bus that is actually just another >>> representation >>> of the parent device and that creates another set of devices for >>> the functions. Either I misunderstand what this is for, or you have >> I think you have this right. >> >>> a candidate for cleanup there and once you remove it (by calling >>> rmi_driver_probe() instead of rmi_register_transport_device() >>> to oversimplify the idea), the actual probing for the function >>> drivers becomes much easier to do right. >>> >> Agree, that would simplify the code a lot. I just don't know how >> important it is for other users of RMI4 to have a modular solution or >> if >> the monolithic approach is a consensus now. The modular solution is >> currently disabled, but we can "always" switch back with a small >> effort. >> >> My opinion on this matter is that there is no need for the modular >> functions, but others might have a different opinion. Just to clarify, this is proposing that the rmi_physical device be removed and we just calling the equivalent functions from the context of the transport? Basically, what Bjorn suggested here last year: https://lkml.org/lkml/2016/4/21/781 Andrew > The primary impetus for the original modular function design was to > allow phone/tablet/etc manufacturers to write their own handlers for > some functions without having to write the entire driver or hack up an > existing monolithic driver. This simplifies engineering for the device > manufacturer a lot. > > We also hoped that other peripheral manufacturers would pick up RMI4 > for other sorts of sensors and devices - it is an open standard after > all. However, this didn't happen, possibly due to the complete lack of > promotion of RMI4 as a standard. > > Modular functions were also intended to make it easier for community > OSes like AOKP or Paranoid Android to get up and running on products > where in order to fulfill their GPL obligations, the manufacturer > simply dumped a pile of sources, which might not actually be working. > I'll grant that this could also be achieved with a monolithic driver. > > Several years ago, there also appeared to be a major trend toward > having two or more sensors in a given product, which was a lot easier > to manage with modular functions. Although lots of prototypes were > built, very few went to production - maybe even none. > > And finally, I wanted to enable a platform that was easy for Raspberry > Pi type tinkering with touch sensing functions - modular functions > isolated the parts of the RMI4 system, so the tinkerer need only learn > the parts they wanted to putter with. > > Anyway, I still think modular functions have important benefits as > outlined above, and thus the driver is not over-engineered - it simply > (or complicatedly :-) ) has the bug that modular functions don't > currently work. > > However, since I'm not very active on this project right now, and not > likely to be in the near future, if you folks decide you want to take > the driver structure in a different direction, I can live with that. > > Cheers, > Chris ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-13 0:42 ` Andrew Duggan @ 2017-01-13 21:14 ` Arnd Bergmann 0 siblings, 0 replies; 16+ messages in thread From: Arnd Bergmann @ 2017-01-13 21:14 UTC (permalink / raw) To: Andrew Duggan Cc: Christopher Heiny, Benjamin Tissoires, Dmitry Torokhov, Nick Dyer, linux-input, linux-kernel, Lyude Paul On Thursday, January 12, 2017 4:42:55 PM CET Andrew Duggan wrote: > On 01/11/2017 11:27 AM, Christopher Heiny wrote: > > On Wed, 2017-01-11 at 18:48 +0100, Benjamin Tissoires wrote: > > Actually, long, long ago (well before I got yanked off the RMI4 driver > > project for a 'short term emergency' two and a half years ago - > > fortunately Andrew was more than able to take it over) it worked that > > way. If you had the bus, a physical driver, and the sensor driver > > running, you could build the functions as modules and attach them via > > udev or insert them later. > > > > We had this working on the bench at one point, but fairly early in the > > submission process seem to have just assumed it would keep working and > > stopped regression testing on that feature. There have been an whole > > lot of changes since then, and somewhere along the line functions-as- > > loadable-modules stopped working. Since we weren't testing it anymore, > > it wasn't caught. > > > > We made the decision to disable support for function drivers as modules > after running into a few technical challenges which happened during > initialization. The priority was to get the driver upstreamed so we put > off getting function drivers working as modules until there was a > compelling reason to do so. But, we left the structure mostly intact if > we decided to do so. Here are the messages which describe what we > discovered at the time: > > https://lkml.org/lkml/2015/11/10/92 > https://lkml.org/lkml/2015/11/12/652 > > I'm basically coming to the same conclusion I had back then. Getting > loadable modules for function drivers to work would add complexity for > not a lot of benefit based on the current state of the driver and how it > is currently being used. I'm also not sure we will reach the critical > mass of function drivers where there is a significant benefit of not > having to load them all. I think the main benefit of splitting out the function drivers would be simpler handling of the Kconfig dependencies. Each function driver can simply 'select' the core so that gets built-in whenever one of the functions is, and the other functions can have external dependencies like the bus drivers do. > >>> a candidate for cleanup there and once you remove it (by calling > >>> rmi_driver_probe() instead of rmi_register_transport_device() > >>> to oversimplify the idea), the actual probing for the function > >>> drivers becomes much easier to do right. > >>> > >> Agree, that would simplify the code a lot. I just don't know how > >> important it is for other users of RMI4 to have a modular solution or > >> if > >> the monolithic approach is a consensus now. The modular solution is > >> currently disabled, but we can "always" switch back with a small > >> effort. > >> > >> My opinion on this matter is that there is no need for the modular > >> functions, but others might have a different opinion. > > Just to clarify, this is proposing that the rmi_physical device be > removed and we just calling the equivalent functions from the context of > the transport? Basically, what Bjorn suggested here last year: > > https://lkml.org/lkml/2016/4/21/781 Right. I think that should simplify the core enough that separating it from the function drivers is straightforward. I still don't get the part about blocking on user space as mentioned in https://lkml.org/lkml/2015/11/10/92. There should not be any requirement for the probe() function to wait for the child device to get used, the probe just needs to add the child to the bus as any other bus driver does, and from there it gets used once the driver is loaded (or never, if the driver doesn't exist). Arnd ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-10 12:16 [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol Arnd Bergmann 2017-01-11 0:39 ` Andrew Duggan @ 2017-01-13 6:22 ` Dmitry Torokhov 2017-01-13 21:06 ` Arnd Bergmann 1 sibling, 1 reply; 16+ messages in thread From: Dmitry Torokhov @ 2017-01-13 6:22 UTC (permalink / raw) To: Arnd Bergmann Cc: Andrew Duggan, Christopher Heiny, Nick Dyer, linux-input, linux-kernel Hi Arnd, On Tue, Jan 10, 2017 at 01:16:58PM +0100, Arnd Bergmann wrote: > If CONFIG_INPUT=m, we get a build error for the rmi4-f03 driver, > added in linux-4.10: > > drivers/input/built-in.o: In function `rmi_f03_attention': > rmi_f03.c:(.text+0xcfe0): undefined reference to `serio_interrupt' > rmi_f03.c:(.text+0xd055): undefined reference to `serio_interrupt' > drivers/input/built-in.o: In function `rmi_f03_remove': > rmi_f03.c:(.text+0xd115): undefined reference to `serio_unregister_port' > drivers/input/built-in.o: In function `rmi_f03_probe': > rmi_f03.c:(.text+0xd209): undefined reference to `__serio_register_port' > > If we make the driver itself a 'tristate' instead of 'bool' symbol, > Kconfig ensures that it can only be a loadable module in this case, > which avoids the problem. > > Fixes: c5e8848fc98e ("Input: synaptics-rmi4 - add support for F03") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/input/rmi4/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig > index 30cc627a4f45..ad945b25722c 100644 > --- a/drivers/input/rmi4/Kconfig > +++ b/drivers/input/rmi4/Kconfig > @@ -40,7 +40,7 @@ config RMI4_SMB > called rmi_smbus. > > config RMI4_F03 > - bool "RMI4 Function 03 (PS2 Guest)" > + tristate "RMI4 Function 03 (PS2 Guest)" > depends on RMI4_CORE && SERIO > help > Say Y here if you want to add support for RMI4 function 03. > -- > 2.9.0 > As it was explained townthread we can't [currently] make functions modules, in the meantime I have d7ddad0acc4add42567f7879b116a0b9eea31860 that should fix this issue (and I just sent pull request for it). Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-13 6:22 ` Dmitry Torokhov @ 2017-01-13 21:06 ` Arnd Bergmann 2017-01-13 21:15 ` Dmitry Torokhov 0 siblings, 1 reply; 16+ messages in thread From: Arnd Bergmann @ 2017-01-13 21:06 UTC (permalink / raw) To: Dmitry Torokhov Cc: Andrew Duggan, Christopher Heiny, Nick Dyer, linux-input, linux-kernel On Thursday, January 12, 2017 10:22:03 PM CET Dmitry Torokhov wrote: > As it was explained townthread we can't [currently] make functions > modules, in the meantime I have d7ddad0acc4add42567f7879b116a0b9eea31860 > that should fix this issue (and I just sent pull request for it). On today's linux-next (which includes d7ddad0acc4ad), I was still getting this warning : warning: (HID_RMI) selects RMI4_F03 which has unmet direct dependencies (!UML && INPUT && RMI4_CORE && (SERIO=y || RMI4_CORE=SERIO)) This is my fixup, though I'm not too happy with that version. Signed-off-by: Arnd Bergmann <arnd@arndb.de> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 1aeb80e52424..3927259a5d5d 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -785,7 +785,8 @@ config HID_SUNPLUS config HID_RMI tristate "Synaptics RMI4 device support" depends on HID - select RMI4_CORE + depends on SERIO && RMI4_CORE + depends on SERIO=y || RMI4_CORE=SERIO select RMI4_F03 select RMI4_F11 select RMI4_F12 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-13 21:06 ` Arnd Bergmann @ 2017-01-13 21:15 ` Dmitry Torokhov 2017-01-13 21:34 ` Arnd Bergmann 0 siblings, 1 reply; 16+ messages in thread From: Dmitry Torokhov @ 2017-01-13 21:15 UTC (permalink / raw) To: Arnd Bergmann Cc: Andrew Duggan, Christopher Heiny, Nick Dyer, linux-input, linux-kernel On January 13, 2017 1:06:12 PM PST, Arnd Bergmann <arnd@arndb.de> wrote: >On Thursday, January 12, 2017 10:22:03 PM CET Dmitry Torokhov wrote: >> As it was explained townthread we can't [currently] make functions >> modules, in the meantime I have >d7ddad0acc4add42567f7879b116a0b9eea31860 >> that should fix this issue (and I just sent pull request for it). > >On today's linux-next (which includes d7ddad0acc4ad), I was still >getting this warning : > >warning: (HID_RMI) selects RMI4_F03 which has unmet direct dependencies >(!UML && INPUT && RMI4_CORE && (SERIO=y || RMI4_CORE=SERIO)) Ah, yes, that's new hid RMI code.. > >This is my fixup, though I'm not too happy with that version. > >Signed-off-by: Arnd Bergmann <arnd@arndb.de> > >diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >index 1aeb80e52424..3927259a5d5d 100644 >--- a/drivers/hid/Kconfig >+++ b/drivers/hid/Kconfig >@@ -785,7 +785,8 @@ config HID_SUNPLUS > config HID_RMI > tristate "Synaptics RMI4 device support" > depends on HID >- select RMI4_CORE >+ depends on SERIO && RMI4_CORE >+ depends on SERIO=y || RMI4_CORE=SERIO Shouldn't this be simply select SERIO # needed for F03 ? > select RMI4_F03 > select RMI4_F11 > select RMI4_F12 Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-13 21:15 ` Dmitry Torokhov @ 2017-01-13 21:34 ` Arnd Bergmann 2017-01-13 21:42 ` Dmitry Torokhov 0 siblings, 1 reply; 16+ messages in thread From: Arnd Bergmann @ 2017-01-13 21:34 UTC (permalink / raw) To: Dmitry Torokhov Cc: Andrew Duggan, Christopher Heiny, Nick Dyer, linux-input, Linux Kernel Mailing List On Fri, Jan 13, 2017 at 10:15 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > >>This is my fixup, though I'm not too happy with that version. >> >>Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> >>diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >>index 1aeb80e52424..3927259a5d5d 100644 >>--- a/drivers/hid/Kconfig >>+++ b/drivers/hid/Kconfig >>@@ -785,7 +785,8 @@ config HID_SUNPLUS >> config HID_RMI >> tristate "Synaptics RMI4 device support" >> depends on HID >>- select RMI4_CORE >>+ depends on SERIO && RMI4_CORE >>+ depends on SERIO=y || RMI4_CORE=SERIO > > Shouldn't this be simply > > select SERIO # needed for F03 Ah, I guess this would work too. I didn't consider it because SERIO is a user-visible symbol and we generally try not to 'select' them but instead use 'depends on. However, SERIO is already used with 'select' all over the place, so adding another select is actually safer than adding a dependency (which could cause dependency loops here). Actually the best solution is probably to have 'select SERIO' in RMI4, like config RMI4_F03_SERIO tristate depends on RMI4_CORE depends on RMI4_F03 default RMI4_CORE select SERIO As that avoids the 'depends on SERIO=y || RMI4_CORE=SERIO' statement that is different from the other SERIO users, it keeps it all in one place, and it doesn't prevent you from seeing the RMI4_F03 symbol when SERIO=m. Arnd ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-13 21:34 ` Arnd Bergmann @ 2017-01-13 21:42 ` Dmitry Torokhov 2017-01-14 12:09 ` Arnd Bergmann 0 siblings, 1 reply; 16+ messages in thread From: Dmitry Torokhov @ 2017-01-13 21:42 UTC (permalink / raw) To: Arnd Bergmann Cc: Andrew Duggan, Christopher Heiny, Nick Dyer, linux-input, Linux Kernel Mailing List On Fri, Jan 13, 2017 at 10:34:32PM +0100, Arnd Bergmann wrote: > On Fri, Jan 13, 2017 at 10:15 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > >>This is my fixup, though I'm not too happy with that version. > >> > >>Signed-off-by: Arnd Bergmann <arnd@arndb.de> > >> > >>diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > >>index 1aeb80e52424..3927259a5d5d 100644 > >>--- a/drivers/hid/Kconfig > >>+++ b/drivers/hid/Kconfig > >>@@ -785,7 +785,8 @@ config HID_SUNPLUS > >> config HID_RMI > >> tristate "Synaptics RMI4 device support" > >> depends on HID > >>- select RMI4_CORE > >>+ depends on SERIO && RMI4_CORE > >>+ depends on SERIO=y || RMI4_CORE=SERIO > > > > Shouldn't this be simply > > > > select SERIO # needed for F03 > > Ah, I guess this would work too. I didn't consider it because SERIO is > a user-visible symbol and we generally try not to 'select' them but instead > use 'depends on. > > However, SERIO is already used with 'select' all over the place, so adding > another select is actually safer than adding a dependency (which could > cause dependency loops here). > > Actually the best solution is probably to have 'select SERIO' in RMI4, like > > config RMI4_F03_SERIO > tristate > depends on RMI4_CORE > depends on RMI4_F03 > default RMI4_CORE > select SERIO > > As that avoids the 'depends on SERIO=y || RMI4_CORE=SERIO' statement that > is different from the other SERIO users, it keeps it all in one place, > and it doesn't > prevent you from seeing the RMI4_F03 symbol when SERIO=m. Hmm, if this works and resilient with user changing symbols after they've been auto-selected then I like it. How can we run it through multitude of randconfigs? Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-13 21:42 ` Dmitry Torokhov @ 2017-01-14 12:09 ` Arnd Bergmann 2017-01-15 23:39 ` Dmitry Torokhov 0 siblings, 1 reply; 16+ messages in thread From: Arnd Bergmann @ 2017-01-14 12:09 UTC (permalink / raw) To: Dmitry Torokhov Cc: Christopher Heiny, Nick Dyer, Linux Kernel Mailing List, linux-input, Andrew Duggan On Jan 13, 2017 10:42 PM, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote: > On Fri, Jan 13, 2017 at 10:34:32PM +0100, Arnd Bergmann wrote: > > config RMI4_F03_SERIO > > tristate > > depends on RMI4_CORE > > depends on RMI4_F03 > > default RMI4_CORE > > select SERIO > > > > As that avoids the 'depends on SERIO=y || RMI4_CORE=SERIO' statement that > > is different from the other SERIO users, it keeps it all in one place, > > and it doesn't > > prevent you from seeing the RMI4_F03 symbol when SERIO=m. > Hmm, if this works and resilient with user changing symbols after > they've been auto-selected then I like it. How can we run it through > multitude of randconfigs? I've successfully run it over night on a few hundred randconfig builds without problems now, so I'm pretty confident it works. The hidden option will ensure the configuration is always valid even when the user changes it, the only thing that can be unexpected is the same as every 'select': when you enable this option, SERIO will get turned on, and when you disable it again after leaving 'menuconfig', it stays on. Arnd ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol 2017-01-14 12:09 ` Arnd Bergmann @ 2017-01-15 23:39 ` Dmitry Torokhov 0 siblings, 0 replies; 16+ messages in thread From: Dmitry Torokhov @ 2017-01-15 23:39 UTC (permalink / raw) To: Arnd Bergmann Cc: Christopher Heiny, Nick Dyer, Linux Kernel Mailing List, linux-input, Andrew Duggan On Sat, Jan 14, 2017 at 01:09:57PM +0100, Arnd Bergmann wrote: > On Jan 13, 2017 10:42 PM, "Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote: > > On Fri, Jan 13, 2017 at 10:34:32PM +0100, Arnd Bergmann wrote: > > > config RMI4_F03_SERIO > > > tristate > > > depends on RMI4_CORE > > > depends on RMI4_F03 > > > default RMI4_CORE > > > select SERIO > > > > > > As that avoids the 'depends on SERIO=y || RMI4_CORE=SERIO' statement that > > > is different from the other SERIO users, it keeps it all in one place, > > > and it doesn't > > > prevent you from seeing the RMI4_F03 symbol when SERIO=m. > > Hmm, if this works and resilient with user changing symbols after > > they've been auto-selected then I like it. How can we run it through > > multitude of randconfigs? > > I've successfully run it over night on a few hundred randconfig builds without > problems now, so I'm pretty confident it works. > > The hidden option will ensure the configuration is always valid even when > the user changes it, the only thing that can be unexpected is the same as > every 'select': when you enable this option, SERIO will get turned on, and > when you disable it again after leaving 'menuconfig', it stays on. Great. Could you please send me real patch and I'll get it to Linus? Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-01-15 23:39 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-10 12:16 [PATCH] Input: synaptics-rmi4 - make F03 a tristate symbol Arnd Bergmann 2017-01-11 0:39 ` Andrew Duggan 2017-01-11 15:39 ` Arnd Bergmann 2017-01-11 16:28 ` Benjamin Tissoires 2017-01-11 16:54 ` Arnd Bergmann 2017-01-11 17:48 ` Benjamin Tissoires 2017-01-11 19:27 ` Christopher Heiny 2017-01-13 0:42 ` Andrew Duggan 2017-01-13 21:14 ` Arnd Bergmann 2017-01-13 6:22 ` Dmitry Torokhov 2017-01-13 21:06 ` Arnd Bergmann 2017-01-13 21:15 ` Dmitry Torokhov 2017-01-13 21:34 ` Arnd Bergmann 2017-01-13 21:42 ` Dmitry Torokhov 2017-01-14 12:09 ` Arnd Bergmann 2017-01-15 23:39 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).