RE: [PATCH 2/4] Input: RMI4 - move sensor driver and F01 handler into the core
Sorry for top posting this - I forgot to email while still at work. I've been poking at this, and think (a) it's possible that I'm being too paranoid, and (b) it looks like it'll work with some of the future stuff. So I'll back off my objections for the moment, and give these changes a try, possibly with some slight modifications. Thanks! Chris From: linux-input-ow...@vger.kernel.org [linux-input-ow...@vger.kernel.org] on behalf of Dmitry Torokhov [dmitry.torok...@gmail.com] Sent: Thursday, November 29, 2012 9:21 AM To: Christopher Heiny Cc: Linus Walleij; Linux Input; Linux Kernel; Allie Xiong; Vivian Ly; Daniel Rosenberg; Alexandra Chin; Joerie de Gram; Wolfram Sang; Mathieu Poirier Subject: Re: [PATCH 2/4] Input: RMI4 - move sensor driver and F01 handler into the core Hi Chris, On Wed, Nov 28, 2012 at 08:54:32PM -0800, Christopher Heiny wrote: > On 11/27/2012 01:21 AM, Dmitry Torokhov wrote: > >There is no point in having the sensor driver and F01 handler separate > >from the RMI core since it is not useful without them and having them > >all together simplifies initialization among other things. > > Hi Dmitry, > > I've been looking at this patch as well as your patch 3/4 changes, > and I'm not sure it's for the better. > > One thing that confuses me is that these appear to go against the > advice we've been getting over the past months to rely more on > standard kernel bus and driver implementations, instead of the > "roll-your-own" implementation we had been using before. > > More importantly, the patches inextricably link the sensor driver > implementation and the F01 driver implementation to the bus > implementation, and means that any given system can have only one > way of managing F01. As you observed, a sensor is pretty much > useless without an F01 handler, but I am reasonably sure that there > will be future systems that have more than one RMI4 sensor in them, > and there is a strong possibility that these sensors may have > different requirements for handling F01. In the near future, then, > these changes will have to be refactored back to something more like > the structure of our 2012/11/16 patch set. > > Additionally, having F01 as a special case means that when we start > implementing things such as support for request_firmware(), there > will have to be a bunch of special case code to deal with F01, since > it's no longer "just another function driver". That seems to go in > exactly the opposite direction of the simplification that you're > trying to achieve. But F01 continues to being "just another function driver" even with my changes. It is still registered as rmi_fucntion_handler and uses standard matching mechanisms to bind to rmi_functions registered by the sensor driver. What I changed is the fact that rmi_f01 is no longer a separate module which could be loaded after loading rmi_bus and it can't be unloaded without unloading rmi_bus. This simplifies things and makes it easier to have rmi core compiled as a module. Also I do not quite follow your idea that devices might have different requirements for handling F01. If that is true then be _can't_ implement "F01" as "another function driver"... But that is orthogonal for the 3/4 change we are discussing here. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/4] Input: RMI4 - move sensor driver and F01 handler into the core
Sorry for top posting this - I forgot to email while still at work. I've been poking at this, and think (a) it's possible that I'm being too paranoid, and (b) it looks like it'll work with some of the future stuff. So I'll back off my objections for the moment, and give these changes a try, possibly with some slight modifications. Thanks! Chris From: linux-input-ow...@vger.kernel.org [linux-input-ow...@vger.kernel.org] on behalf of Dmitry Torokhov [dmitry.torok...@gmail.com] Sent: Thursday, November 29, 2012 9:21 AM To: Christopher Heiny Cc: Linus Walleij; Linux Input; Linux Kernel; Allie Xiong; Vivian Ly; Daniel Rosenberg; Alexandra Chin; Joerie de Gram; Wolfram Sang; Mathieu Poirier Subject: Re: [PATCH 2/4] Input: RMI4 - move sensor driver and F01 handler into the core Hi Chris, On Wed, Nov 28, 2012 at 08:54:32PM -0800, Christopher Heiny wrote: On 11/27/2012 01:21 AM, Dmitry Torokhov wrote: There is no point in having the sensor driver and F01 handler separate from the RMI core since it is not useful without them and having them all together simplifies initialization among other things. Hi Dmitry, I've been looking at this patch as well as your patch 3/4 changes, and I'm not sure it's for the better. One thing that confuses me is that these appear to go against the advice we've been getting over the past months to rely more on standard kernel bus and driver implementations, instead of the roll-your-own implementation we had been using before. More importantly, the patches inextricably link the sensor driver implementation and the F01 driver implementation to the bus implementation, and means that any given system can have only one way of managing F01. As you observed, a sensor is pretty much useless without an F01 handler, but I am reasonably sure that there will be future systems that have more than one RMI4 sensor in them, and there is a strong possibility that these sensors may have different requirements for handling F01. In the near future, then, these changes will have to be refactored back to something more like the structure of our 2012/11/16 patch set. Additionally, having F01 as a special case means that when we start implementing things such as support for request_firmware(), there will have to be a bunch of special case code to deal with F01, since it's no longer just another function driver. That seems to go in exactly the opposite direction of the simplification that you're trying to achieve. But F01 continues to being just another function driver even with my changes. It is still registered as rmi_fucntion_handler and uses standard matching mechanisms to bind to rmi_functions registered by the sensor driver. What I changed is the fact that rmi_f01 is no longer a separate module which could be loaded after loading rmi_bus and it can't be unloaded without unloading rmi_bus. This simplifies things and makes it easier to have rmi core compiled as a module. Also I do not quite follow your idea that devices might have different requirements for handling F01. If that is true then be _can't_ implement F01 as another function driver... But that is orthogonal for the 3/4 change we are discussing here. Thanks. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Input: RMI4 - move sensor driver and F01 handler into the core
Hi Chris, On Wed, Nov 28, 2012 at 08:54:32PM -0800, Christopher Heiny wrote: > On 11/27/2012 01:21 AM, Dmitry Torokhov wrote: > >There is no point in having the sensor driver and F01 handler separate > >from the RMI core since it is not useful without them and having them > >all together simplifies initialization among other things. > > Hi Dmitry, > > I've been looking at this patch as well as your patch 3/4 changes, > and I'm not sure it's for the better. > > One thing that confuses me is that these appear to go against the > advice we've been getting over the past months to rely more on > standard kernel bus and driver implementations, instead of the > "roll-your-own" implementation we had been using before. > > More importantly, the patches inextricably link the sensor driver > implementation and the F01 driver implementation to the bus > implementation, and means that any given system can have only one > way of managing F01. As you observed, a sensor is pretty much > useless without an F01 handler, but I am reasonably sure that there > will be future systems that have more than one RMI4 sensor in them, > and there is a strong possibility that these sensors may have > different requirements for handling F01. In the near future, then, > these changes will have to be refactored back to something more like > the structure of our 2012/11/16 patch set. > > Additionally, having F01 as a special case means that when we start > implementing things such as support for request_firmware(), there > will have to be a bunch of special case code to deal with F01, since > it's no longer "just another function driver". That seems to go in > exactly the opposite direction of the simplification that you're > trying to achieve. But F01 continues to being "just another function driver" even with my changes. It is still registered as rmi_fucntion_handler and uses standard matching mechanisms to bind to rmi_functions registered by the sensor driver. What I changed is the fact that rmi_f01 is no longer a separate module which could be loaded after loading rmi_bus and it can't be unloaded without unloading rmi_bus. This simplifies things and makes it easier to have rmi core compiled as a module. Also I do not quite follow your idea that devices might have different requirements for handling F01. If that is true then be _can't_ implement "F01" as "another function driver"... But that is orthogonal for the 3/4 change we are discussing here. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Input: RMI4 - move sensor driver and F01 handler into the core
Hi Chris, On Wed, Nov 28, 2012 at 08:54:32PM -0800, Christopher Heiny wrote: On 11/27/2012 01:21 AM, Dmitry Torokhov wrote: There is no point in having the sensor driver and F01 handler separate from the RMI core since it is not useful without them and having them all together simplifies initialization among other things. Hi Dmitry, I've been looking at this patch as well as your patch 3/4 changes, and I'm not sure it's for the better. One thing that confuses me is that these appear to go against the advice we've been getting over the past months to rely more on standard kernel bus and driver implementations, instead of the roll-your-own implementation we had been using before. More importantly, the patches inextricably link the sensor driver implementation and the F01 driver implementation to the bus implementation, and means that any given system can have only one way of managing F01. As you observed, a sensor is pretty much useless without an F01 handler, but I am reasonably sure that there will be future systems that have more than one RMI4 sensor in them, and there is a strong possibility that these sensors may have different requirements for handling F01. In the near future, then, these changes will have to be refactored back to something more like the structure of our 2012/11/16 patch set. Additionally, having F01 as a special case means that when we start implementing things such as support for request_firmware(), there will have to be a bunch of special case code to deal with F01, since it's no longer just another function driver. That seems to go in exactly the opposite direction of the simplification that you're trying to achieve. But F01 continues to being just another function driver even with my changes. It is still registered as rmi_fucntion_handler and uses standard matching mechanisms to bind to rmi_functions registered by the sensor driver. What I changed is the fact that rmi_f01 is no longer a separate module which could be loaded after loading rmi_bus and it can't be unloaded without unloading rmi_bus. This simplifies things and makes it easier to have rmi core compiled as a module. Also I do not quite follow your idea that devices might have different requirements for handling F01. If that is true then be _can't_ implement F01 as another function driver... But that is orthogonal for the 3/4 change we are discussing here. Thanks. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] Input: RMI4 - move sensor driver and F01 handler into the core
On 11/27/2012 01:21 AM, Dmitry Torokhov wrote: There is no point in having the sensor driver and F01 handler separate from the RMI core since it is not useful without them and having them all together simplifies initialization among other things. Hi Dmitry, I've been looking at this patch as well as your patch 3/4 changes, and I'm not sure it's for the better. One thing that confuses me is that these appear to go against the advice we've been getting over the past months to rely more on standard kernel bus and driver implementations, instead of the "roll-your-own" implementation we had been using before. More importantly, the patches inextricably link the sensor driver implementation and the F01 driver implementation to the bus implementation, and means that any given system can have only one way of managing F01. As you observed, a sensor is pretty much useless without an F01 handler, but I am reasonably sure that there will be future systems that have more than one RMI4 sensor in them, and there is a strong possibility that these sensors may have different requirements for handling F01. In the near future, then, these changes will have to be refactored back to something more like the structure of our 2012/11/16 patch set. Additionally, having F01 as a special case means that when we start implementing things such as support for request_firmware(), there will have to be a bunch of special case code to deal with F01, since it's no longer "just another function driver". That seems to go in exactly the opposite direction of the simplification that you're trying to achieve. I fully agree that there's a lot of boilerplate that could be (and must be) consolidated and/or eliminated, and that some of the F01 handling during initialization can be simplified. I'm just not sure that this is the right way to go about doing that. If you'd like me to address this inline in the patches, let me know. I just thought it would be better to address design issues up top. Alternatively, I could submit a patch that tries to address your concerns from another approach. Of course, it's entirely possible that I'm full of fertilizer here - I trust that you will let me know in detail if that is the case. :-) Thanks very much, Chris Signed-off-by: Dmitry Torokhov --- drivers/input/rmi4/Kconfig | 23 ++- drivers/input/rmi4/Makefile | 10 +++--- drivers/input/rmi4/rmi_bus.c| 41 - drivers/input/rmi4/rmi_driver.c | 36 ++-- drivers/input/rmi4/rmi_driver.h | 6 ++ drivers/input/rmi4/rmi_f01.c| 22 +++--- 6 files changed, 68 insertions(+), 70 deletions(-) diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig index 41cbbee..d0c7b6e 100644 --- a/drivers/input/rmi4/Kconfig +++ b/drivers/input/rmi4/Kconfig @@ -1,8 +1,8 @@ # # RMI4 configuration # -config RMI4_BUS - bool "Synaptics RMI4 bus support" +config RMI4_CORE + tristate "Synaptics RMI4 bus support" help Say Y here if you want to support the Synaptics RMI4 bus. This is required for all RMI4 device support. @@ -13,7 +13,7 @@ config RMI4_BUS config RMI4_DEBUG bool "RMI4 Debugging" - depends on RMI4_BUS + depends on RMI4_CORE select DEBUG_FS help Say Y here to enable debug feature in the RMI4 driver. @@ -26,8 +26,8 @@ config RMI4_DEBUG If unsure, say N. config RMI4_I2C - bool "RMI4 I2C Support" - depends on RMI4_BUS && I2C + tristate "RMI4 I2C Support" + depends on RMI4_CORE && I2C help Say Y here if you want to support RMI4 devices connected to an I2C bus. @@ -36,20 +36,9 @@ config RMI4_I2C This feature is not currently available as a loadable module. -config RMI4_GENERIC - bool "RMI4 Generic driver" - depends on RMI4_BUS - help - Say Y here if you want to support generic RMI4 devices. - - This is pretty much required if you want to do anything useful with - your RMI device. - - This feature is not currently available as a loadable module. - config RMI4_F11 tristate "RMI4 Function 11 (2D pointing)" - depends on RMI4_BUS && RMI4_GENERIC + depends on RMI4_CORE help Say Y here if you want to add support for RMI4 function 11. diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile index 8882c3d..5c6bad5 100644 --- a/drivers/input/rmi4/Makefile +++ b/drivers/input/rmi4/Makefile @@ -1,8 +1,12 @@ -obj-$(CONFIG_RMI4_BUS) += rmi_bus.o -obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o -obj-$(CONFIG_RMI4_GENERIC) += rmi_driver.o rmi_f01.o +obj-$(CONFIG_RMI4_CORE) += rmi_core.o +rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o + +# Function drivers
Re: [PATCH 2/4] Input: RMI4 - move sensor driver and F01 handler into the core
On 11/27/2012 01:21 AM, Dmitry Torokhov wrote: There is no point in having the sensor driver and F01 handler separate from the RMI core since it is not useful without them and having them all together simplifies initialization among other things. Hi Dmitry, I've been looking at this patch as well as your patch 3/4 changes, and I'm not sure it's for the better. One thing that confuses me is that these appear to go against the advice we've been getting over the past months to rely more on standard kernel bus and driver implementations, instead of the roll-your-own implementation we had been using before. More importantly, the patches inextricably link the sensor driver implementation and the F01 driver implementation to the bus implementation, and means that any given system can have only one way of managing F01. As you observed, a sensor is pretty much useless without an F01 handler, but I am reasonably sure that there will be future systems that have more than one RMI4 sensor in them, and there is a strong possibility that these sensors may have different requirements for handling F01. In the near future, then, these changes will have to be refactored back to something more like the structure of our 2012/11/16 patch set. Additionally, having F01 as a special case means that when we start implementing things such as support for request_firmware(), there will have to be a bunch of special case code to deal with F01, since it's no longer just another function driver. That seems to go in exactly the opposite direction of the simplification that you're trying to achieve. I fully agree that there's a lot of boilerplate that could be (and must be) consolidated and/or eliminated, and that some of the F01 handling during initialization can be simplified. I'm just not sure that this is the right way to go about doing that. If you'd like me to address this inline in the patches, let me know. I just thought it would be better to address design issues up top. Alternatively, I could submit a patch that tries to address your concerns from another approach. Of course, it's entirely possible that I'm full of fertilizer here - I trust that you will let me know in detail if that is the case. :-) Thanks very much, Chris Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com --- drivers/input/rmi4/Kconfig | 23 ++- drivers/input/rmi4/Makefile | 10 +++--- drivers/input/rmi4/rmi_bus.c| 41 - drivers/input/rmi4/rmi_driver.c | 36 ++-- drivers/input/rmi4/rmi_driver.h | 6 ++ drivers/input/rmi4/rmi_f01.c| 22 +++--- 6 files changed, 68 insertions(+), 70 deletions(-) diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig index 41cbbee..d0c7b6e 100644 --- a/drivers/input/rmi4/Kconfig +++ b/drivers/input/rmi4/Kconfig @@ -1,8 +1,8 @@ # # RMI4 configuration # -config RMI4_BUS - bool Synaptics RMI4 bus support +config RMI4_CORE + tristate Synaptics RMI4 bus support help Say Y here if you want to support the Synaptics RMI4 bus. This is required for all RMI4 device support. @@ -13,7 +13,7 @@ config RMI4_BUS config RMI4_DEBUG bool RMI4 Debugging - depends on RMI4_BUS + depends on RMI4_CORE select DEBUG_FS help Say Y here to enable debug feature in the RMI4 driver. @@ -26,8 +26,8 @@ config RMI4_DEBUG If unsure, say N. config RMI4_I2C - bool RMI4 I2C Support - depends on RMI4_BUS I2C + tristate RMI4 I2C Support + depends on RMI4_CORE I2C help Say Y here if you want to support RMI4 devices connected to an I2C bus. @@ -36,20 +36,9 @@ config RMI4_I2C This feature is not currently available as a loadable module. -config RMI4_GENERIC - bool RMI4 Generic driver - depends on RMI4_BUS - help - Say Y here if you want to support generic RMI4 devices. - - This is pretty much required if you want to do anything useful with - your RMI device. - - This feature is not currently available as a loadable module. - config RMI4_F11 tristate RMI4 Function 11 (2D pointing) - depends on RMI4_BUS RMI4_GENERIC + depends on RMI4_CORE help Say Y here if you want to add support for RMI4 function 11. diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile index 8882c3d..5c6bad5 100644 --- a/drivers/input/rmi4/Makefile +++ b/drivers/input/rmi4/Makefile @@ -1,8 +1,12 @@ -obj-$(CONFIG_RMI4_BUS) += rmi_bus.o -obj-$(CONFIG_RMI4_I2C) += rmi_i2c.o -obj-$(CONFIG_RMI4_GENERIC) += rmi_driver.o rmi_f01.o +obj-$(CONFIG_RMI4_CORE) += rmi_core.o +rmi_core-y := rmi_bus.o rmi_driver.o rmi_f01.o + +# Function drivers
