| Message ID | 1370453866-16534-11-git-send-email-nick.dyer@itdev.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Nick, On Wednesday, June 05, 2013 06:37:03 PM Nick Dyer wrote: > Atmel maXTouch chips can be addressed via an "Object Based Protocol" which > defines how i2c registers are mapped to different functions within the > chips. This interface exposes the register map and allows user-space > utilities to inspect and alter object configuration, and to view diagnostic > data, while the device is running. I think if the driver was to use regmap then this fucntionality will already be there as regmap exports registers via debugfs. Thanks.
Dmitry Torokhov wrote: > On Wednesday, June 05, 2013 06:37:03 PM Nick Dyer wrote: >> Atmel maXTouch chips can be addressed via an "Object Based Protocol" which >> defines how i2c registers are mapped to different functions within the >> chips. This interface exposes the register map and allows user-space >> utilities to inspect and alter object configuration, and to view diagnostic >> data, while the device is running. > > I think if the driver was to use regmap then this fucntionality will already > be there as regmap exports registers via debugfs. Yes, this was suggested in the past and I spent some time looking into it. We have made a deliberate choice to implement this via sysfs rather than debugfs since it needs to work on devices that don't have debugfs enabled. In addition, there are some quirks about the way in which we have to read/write registers which means regmap isn't a good fit. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, June 05, 2013 07:45:04 PM Nick Dyer wrote: > Dmitry Torokhov wrote: > > On Wednesday, June 05, 2013 06:37:03 PM Nick Dyer wrote: > >> Atmel maXTouch chips can be addressed via an "Object Based Protocol" > >> which > >> defines how i2c registers are mapped to different functions within the > >> chips. This interface exposes the register map and allows user-space > >> utilities to inspect and alter object configuration, and to view > >> diagnostic > >> data, while the device is running. > > > > I think if the driver was to use regmap then this fucntionality will > > already be there as regmap exports registers via debugfs. > > Yes, this was suggested in the past and I spent some time looking into it. > > We have made a deliberate choice to implement this via sysfs rather than > debugfs since it needs to work on devices that don't have debugfs enabled. Then they will have to enable it. Re-implementing something because someone might not enable needed subsystem is not a good idea. Let's say somebody disabled I2C - will you write your own implementation because of that? Of course not, you just say that for given functionality it is a prerequisite. > > In addition, there are some quirks about the way in which we have to > read/write registers which means regmap isn't a good fit. Could you please elaborate more on this? Thanks.
Dmitry Torokhov wrote: >> We have made a deliberate choice to implement this via sysfs rather than >> debugfs since it needs to work on devices that don't have debugfs enabled. > > Then they will have to enable it. Re-implementing something because > someone might not enable needed subsystem is not a good idea. Let's > say somebody disabled I2C - will you write your own implementation because > of that? Of course not, you just say that for given functionality it > is a prerequisite. debugfs is a debug filesystem. This interface is useful for purposes which are not debug. I have to be pragmatic: I don't see debugfs enabled on most shipping Android devices, and however much I tell them to enable debugfs doesn't seem to hold much weight. It's partly path dependence - it was implemented like this because regmap wasn't in mainline at the point when I wrote it. Having a dependency on regmap would now be a API break complicating support of customers using older kernels than mainline. I would also have to update a bunch of software and documentation and people to know about the two different APIs. The existing implementation already appears in shipping devices, so it is well tested. >> In addition, there are some quirks about the way in which we have to >> read/write registers which means regmap isn't a good fit. > > Could you please elaborate more on this? - the mxt chip caches the I2C read pointer, so you can get a performance optimisation by not sending the address on every read/write (I haven't implemented this yet but plan to) - the address pointer can wrap around when you read the T5 message processor, which would confuse regmap - we require I2C retries in some cases due to way the chip handles sleep states - I can't see how to map the object protocol (used on mxt chips) into the way regmap treats register ranges I can look into porting on top of regmap. But it seems a pity to pepper regmap with atmel_mxt_ts quirks just to save on three small functions in the driver. Thanks for your time to look at this. N -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 05, 2013 at 09:31:39PM +0100, Nick Dyer wrote: > Dmitry Torokhov wrote: > >> We have made a deliberate choice to implement this via sysfs rather than > >> debugfs since it needs to work on devices that don't have debugfs enabled. > > > > Then they will have to enable it. Re-implementing something because > > someone might not enable needed subsystem is not a good idea. Let's > > say somebody disabled I2C - will you write your own implementation because > > of that? Of course not, you just say that for given functionality it > > is a prerequisite. > > debugfs is a debug filesystem. This interface is useful for purposes which > are not debug. What other purposes does it serve? I'd expect you need it during new board bringup. > I have to be pragmatic: I don't see debugfs enabled on most > shipping Android devices, and however much I tell them to enable debugfs > doesn't seem to hold much weight. You do not need to have debugfs enabled on shipping kernels, just the ones you use for integration work. > > It's partly path dependence - it was implemented like this because regmap > wasn't in mainline at the point when I wrote it. Having a dependency on > regmap would now be a API break complicating support of customers using > older kernels than mainline. I would also have to update a bunch of > software and documentation and people to know about the two different APIs. > The existing implementation already appears in shipping devices, so it is > well tested. This was never a good argument for introducing an interface into the kernel. > > >> In addition, there are some quirks about the way in which we have to > >> read/write registers which means regmap isn't a good fit. > > > > Could you please elaborate more on this? > > - the mxt chip caches the I2C read pointer, so you can get a performance > optimisation by not sending the address on every read/write (I haven't > implemented this yet but plan to) > - the address pointer can wrap around when you read the T5 message > processor, which would confuse regmap > - we require I2C retries in some cases due to way the chip handles sleep states > - I can't see how to map the object protocol (used on mxt chips) into the > way regmap treats register ranges > > I can look into porting on top of regmap. But it seems a pity to pepper > regmap with atmel_mxt_ts quirks just to save on three small functions in > the driver. This is not about saving 3 functions but rather the fact that individual register access is desired by many parties and instead of each driver implementing it's own solution (via a char device, sysfs, debugfs, etc) we should try to standardize on common userspace interface. Thanks.
Dmitry Torokhov wrote: >> debugfs is a debug filesystem. This interface is useful for purposes which >> are not debug. > > What other purposes does it serve? I'd expect you need it during new > board bringup. Yes, during board bringup it's extremely useful. We have implemented numerous open and closed source utilities that can use this interface. Run-time examples would be adjusting noise suppression or touch suppression parameters based on something going on in the app layer (eg having different parameters during unlock screen), or tuning report rates based on application requirements, ot to inspect debug data if the touch sensor is faulty. You might say, well we should implement an kernel driver interface for these requirements, but they will vary hugely between different products. We are trying to keep the driver as generic as possible and push product-specific complexity to user space. Hence exposing the register map and implementing user-space libraries to deal with this kind of customisation. >> I have to be pragmatic: I don't see debugfs enabled on most >> shipping Android devices, and however much I tell them to enable debugfs >> doesn't seem to hold much weight. > > You do not need to have debugfs enabled on shipping kernels, just the > ones you use for integration work. I disagree. See above. >> It's partly path dependence - it was implemented like this because regmap >> wasn't in mainline at the point when I wrote it. Having a dependency on >> regmap would now be a API break complicating support of customers using >> older kernels than mainline. I would also have to update a bunch of >> software and documentation and people to know about the two different APIs. >> The existing implementation already appears in shipping devices, so it is >> well tested. > > This was never a good argument for introducing an interface into the > kernel. Yes, I know. Just pointing out that making changes to this does result in a significant cost. >> I can look into porting on top of regmap. But it seems a pity to pepper >> regmap with atmel_mxt_ts quirks just to save on three small functions in >> the driver. > > This is not about saving 3 functions but rather the fact that individual > register access is desired by many parties and instead of each driver > implementing it's own solution (via a char device, sysfs, debugfs, etc) > we should try to standardize on common userspace interface. I agree that a common interface is desirable. If regmap met the requirements I would certainly use it instead. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 05, 2013 at 02:07:15PM -0700, Dmitry Torokhov wrote: > On Wed, Jun 05, 2013 at 09:31:39PM +0100, Nick Dyer wrote: > > It's partly path dependence - it was implemented like this because regmap > > wasn't in mainline at the point when I wrote it. Having a dependency on > > regmap would now be a API break complicating support of customers using > > older kernels than mainline. I would also have to update a bunch of > This was never a good argument for introducing an interface into the > kernel. Indeed, and regmap is very easy to backport - it demands little from the rest of the kernel and most of that is bus specific so you can pretty much just copy it into an older kernel.
On Wed, Jun 05, 2013 at 10:36:45PM +0100, Nick Dyer wrote: > Dmitry Torokhov wrote: > > What other purposes does it serve? I'd expect you need it during new > > board bringup. > Run-time examples would be adjusting noise suppression or touch suppression > parameters based on something going on in the app layer (eg having > different parameters during unlock screen), or tuning report rates based on > application requirements, ot to inspect debug data if the touch sensor is > faulty. You might say, well we should implement an kernel driver interface > for these requirements, but they will vary hugely between different > products. We are trying to keep the driver as generic as possible and push If this interface varies dramatically between products then that sounds like a badly designed interface. Obviously the way the interface is being used would be likely to vary between products but what you're talking about sounds like parameter get/set stuff which sounds pretty generic to me. What userspace chooses to do with the parameters is of course another story. > product-specific complexity to user space. Hence exposing the register map > and implementing user-space libraries to deal with this kind of customisation. This sounds like a bad design decision for Linux, it's just asking for fragility if userspace can go randomly poking round the entire register map of the device with nothing coordinating with the driver code. If you expose the paramters to userspace wouldn't that address the issue?
Mark Brown wrote: > On Wed, Jun 05, 2013 at 10:36:45PM +0100, Nick Dyer wrote: >> Run-time examples would be adjusting noise suppression or touch suppression >> parameters based on something going on in the app layer (eg having >> different parameters during unlock screen), or tuning report rates based on >> application requirements, ot to inspect debug data if the touch sensor is >> faulty. You might say, well we should implement an kernel driver interface >> for these requirements, but they will vary hugely between different >> products. We are trying to keep the driver as generic as possible and push > > If this interface varies dramatically between products then that sounds > like a badly designed interface. Obviously the way the interface is > being used would be likely to vary between products but what you're > talking about sounds like parameter get/set stuff which sounds pretty > generic to me. What userspace chooses to do with the parameters is of > course another story. The underlying design is called Object Protocol by Atmel, and defines a method where the the i2c address space starts with a table defining the types and locations of various objects. Each object contains many parameters, most of them are simple get/set as you describe, but many objects have considerably more dynamic behaviours and interactions. The Object Protocol itself is generic, but the objects themselves are different on different maXTouch chips and firmware versions. The objects may change in size, or be added/removed. The protocol is designed to be flexible and allow rapid firmware development without having to continuously update tools. Having to define every parameter in each object (there are thousands) in the kernel driver would be impractically technically, since it would result in a huge, and constantly updating API, which would be always out-of-date, and impossible to support. Also, I'm afraid it would also be impractical legally, since it would breach the NDA terms that Atmel require on these parameter definitions. >> product-specific complexity to user space. Hence exposing the register map >> and implementing user-space libraries to deal with this kind of customisation. > > This sounds like a bad design decision for Linux, it's just asking for > fragility if userspace can go randomly poking round the entire register > map of the device with nothing coordinating with the driver code. It works very well in practice. This same abstraction is used across maXTouch products on many platforms to provide tool support. I agree that its use should be restricted to system programs. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown wrote: > On Wed, Jun 05, 2013 at 02:07:15PM -0700, Dmitry Torokhov wrote: >> On Wed, Jun 05, 2013 at 09:31:39PM +0100, Nick Dyer wrote: > >>> It's partly path dependence - it was implemented like this because regmap >>> wasn't in mainline at the point when I wrote it. Having a dependency on >>> regmap would now be a API break complicating support of customers using >>> older kernels than mainline. I would also have to update a bunch of > >> This was never a good argument for introducing an interface into the >> kernel. > > Indeed, and regmap is very easy to backport - it demands little from the > rest of the kernel and most of that is bus specific so you can pretty > much just copy it into an older kernel. That is good news. It would still be an extra thing to add to the docs, but not a blocking point. I am more worried about the address pointer handling and the I2C retries. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 06, 2013 at 11:40:50AM +0100, Nick Dyer wrote:
> I am more worried about the address pointer handling and the I2C retries.
The retries can just be done further up the stack? All regmap is doing
with I/O errors is punting them straight back up to the caller so the
caller can retry just as well using regmap as it can using the raw I/O
protocol.
Without seeing the address thing it's hard to comment.
On Thu, Jun 06, 2013 at 11:31:57AM +0100, Nick Dyer wrote: > Having to define every parameter in each object (there are thousands) in > the kernel driver would be impractically technically, since it would result > in a huge, and constantly updating API, which would be always out-of-date, > and impossible to support. > Also, I'm afraid it would also be impractical legally, since it would > breach the NDA terms that Atmel require on these parameter definitions. If that's a big problem just put the data table in a section of the firmware (or a separate file that gets requested as a firmware). Or have the firmware be able to enumerate itself when asked. > >> product-specific complexity to user space. Hence exposing the register map > >> and implementing user-space libraries to deal with this kind of customisation. > > This sounds like a bad design decision for Linux, it's just asking for > > fragility if userspace can go randomly poking round the entire register > > map of the device with nothing coordinating with the driver code. > It works very well in practice. This same abstraction is used across > maXTouch products on many platforms to provide tool support. I agree that > its use should be restricted to system programs. It works well so long as people use the supplied binaries in the way that's been proscribed. As soon as people start upgrading kernels, customising things out of your expected flow (because they can or they're on a tight deadline) things will get more fragile. This sort of stuff is really common, the approaches I'm describing are fairly standard solutions.
Mark Brown wrote: > On Thu, Jun 06, 2013 at 11:40:50AM +0100, Nick Dyer wrote: > >> I am more worried about the address pointer handling and the I2C retries. > > The retries can just be done further up the stack? All regmap is doing > with I/O errors is punting them straight back up to the caller so the > caller can retry just as well using regmap as it can using the raw I/O > protocol. It would have to be put into users of the debugfs interface as well. There's quite tight timing required to make it work properly (see patch [40/53]). > Without seeing the address thing it's hard to comment. Patch [36/53]. If the T5 message processor is from address 100-110, you can do a read of 50 bytes starting at address 100, and it will return 10 messages, but anything in regmap that tries to do bounds checking would get confused, I think. Also, we would like to implement address pointer caching. maXTouch allows us to skip the address part of the i2c transaction if the address pointer in the chip hasn't changed. This speeds up interrupt handler slightly. But it requires extra housekeeping at a low level to remember what the address pointer was on the previous transaction to know whether to send it or not. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 06, 2013 at 12:00:54PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > The retries can just be done further up the stack? All regmap is doing > > with I/O errors is punting them straight back up to the caller so the > > caller can retry just as well using regmap as it can using the raw I/O > > protocol. > It would have to be put into users of the debugfs interface as well. > There's quite tight timing required to make it work properly (see patch > [40/53]). This is yet another reason for implementing the protocol properly instead of trying to bodge around the kernel. It really seems like the biggest problem here is the decision to try to bodge the entire thing into userspace with no kernel support. > > Without seeing the address thing it's hard to comment. > Patch [36/53]. If the T5 message processor is from address 100-110, you can > do a read of 50 bytes starting at address 100, and it will return 10 > messages, but anything in regmap that tries to do bounds checking would get > confused, I think. That's just not going to be supported, sorry. You can implement custom locks and access the device directly where you need to do stuff like that while still using regmap for actual registers though. > Also, we would like to implement address pointer caching. maXTouch allows > us to skip the address part of the i2c transaction if the address pointer > in the chip hasn't changed. This speeds up interrupt handler slightly. But > it requires extra housekeeping at a low level to remember what the address > pointer was on the previous transaction to know whether to send it or not. That sounded like what you were talking about, it's pretty common and is sane enough for reads.
Mark Brown wrote: > On Thu, Jun 06, 2013 at 11:31:57AM +0100, Nick Dyer wrote: >> Having to define every parameter in each object (there are thousands) in >> the kernel driver would be impractically technically, since it would result >> in a huge, and constantly updating API, which would be always out-of-date, >> and impossible to support. > >> Also, I'm afraid it would also be impractical legally, since it would >> breach the NDA terms that Atmel require on these parameter definitions. > > If that's a big problem just put the data table in a section of the > firmware (or a separate file that gets requested as a firmware). Or > have the firmware be able to enumerate itself when asked. That still would breach the NDA, I'm afraid. And there's hundreds of existing versions of maXTouch chips already out there which don't have the infrastructure in place to do what you describe. >> It works very well in practice. This same abstraction is used across >> maXTouch products on many platforms to provide tool support. I agree that >> its use should be restricted to system programs. > > It works well so long as people use the supplied binaries in the way > that's been proscribed. As soon as people start upgrading kernels, > customising things out of your expected flow (because they can or > they're on a tight deadline) things will get more fragile. This sort of > stuff is really common, the approaches I'm describing are fairly > standard solutions. If we expose every single parameter as a get/set as you suggest, we haven't added anything that stops a binary of the wrong version doing something stupid. We've just added a complex API to the same underlying thing, which gains nothing. In practice, where there is a risk of the user mucking up their configuration, the open-source user-space tools that we have released provide an easy way to back up and restore the configuration in use, and the kernel driver provides a way to load a known good configuration on probe. The same configuration formats and tools are used across maXTouch products on many platforms. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown wrote: > On Thu, Jun 06, 2013 at 12:00:54PM +0100, Nick Dyer wrote: >> Mark Brown wrote: > >>> The retries can just be done further up the stack? All regmap is doing >>> with I/O errors is punting them straight back up to the caller so the >>> caller can retry just as well using regmap as it can using the raw I/O >>> protocol. > >> It would have to be put into users of the debugfs interface as well. >> There's quite tight timing required to make it work properly (see patch >> [40/53]). > > This is yet another reason for implementing the protocol properly > instead of trying to bodge around the kernel. It really seems like > the biggest problem here is the decision to try to bodge the entire > thing into userspace with no kernel support. With the interface I am proposing it is handled properly, in the kernel driver. From an Atmel perspective, Linux is just another platform and we want to use our existing investment in tools and documentation to manage & debug chips embedded in Linux based devices. So providing a bridge using a relatively simple API between the tools and the kernel driver is the correct decision. I can't provide a 3D graph of live touch data in the kernel driver, for instance. >>> Without seeing the address thing it's hard to comment. > >> Patch [36/53]. If the T5 message processor is from address 100-110, you can >> do a read of 50 bytes starting at address 100, and it will return 10 >> messages, but anything in regmap that tries to do bounds checking would get >> confused, I think. > > That's just not going to be supported, sorry. You can implement custom > locks and access the device directly where you need to do stuff like > that while still using regmap for actual registers though. OK, fair enough. >> Also, we would like to implement address pointer caching. maXTouch allows >> us to skip the address part of the i2c transaction if the address pointer >> in the chip hasn't changed. This speeds up interrupt handler slightly. But >> it requires extra housekeeping at a low level to remember what the address >> pointer was on the previous transaction to know whether to send it or not. > > That sounded like what you were talking about, it's pretty common and is > sane enough for reads. The address pointer is shared between reads and writes on maXTouch, but I guess that's not a huge problem. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 06, 2013 at 12:17:56PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > If that's a big problem just put the data table in a section of the > > firmware (or a separate file that gets requested as a firmware). Or > > have the firmware be able to enumerate itself when asked. > That still would breach the NDA, I'm afraid. And there's hundreds of > existing versions of maXTouch chips already out there which don't have the > infrastructure in place to do what you describe. I'm sorry, this just doesn't seem plausible. The data is going to be on the running system and accessed via the kernel - as soon as people start using this back door they're going to be revealing what they're accessing and the information is going to be present in the binary blobs. You're only revealing that the parameters are present, not what they mean, and if it's a big concern then do something like strip down the data file that gets shipped in production to just the controls that are being accessed. Again, the fact that you have shipped this stuff doesn't make much difference here - you really should work with upstream on interfaces like this sooner rather than later otherwise you're going to have to cope with pushback. > If we expose every single parameter as a get/set as you suggest, we haven't > added anything that stops a binary of the wrong version doing something > stupid. We've just added a complex API to the same underlying thing, which > gains nothing. So this goes back to what I was saying before about the interface being badly designed - if the API you have to present is really this complex then you've got a big problem in kernel or out of kernel. > In practice, where there is a risk of the user mucking up their > configuration, the open-source user-space tools that we have released > provide an easy way to back up and restore the configuration in use, and > the kernel driver provides a way to load a known good configuration on > probe. The same configuration formats and tools are used across maXTouch > products on many platforms. So you're saying that this is all nice and consistent. If that's the case then there should be no problem putting at least the core bit that does the actual physical interaction with the device into the kernel.
On Thu, Jun 06, 2013 at 12:34:57PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > This is yet another reason for implementing the protocol properly > > instead of trying to bodge around the kernel. It really seems like > > the biggest problem here is the decision to try to bodge the entire > > thing into userspace with no kernel support. > With the interface I am proposing it is handled properly, in the kernel driver. You're proposing doing this using direct register I/O... > From an Atmel perspective, Linux is just another platform and we want to > use our existing investment in tools and documentation to manage & debug > chips embedded in Linux based devices. So providing a bridge using a Broadly speaking the kernel upstream just don't care about this. > > That sounded like what you were talking about, it's pretty common and is > > sane enough for reads. > The address pointer is shared between reads and writes on maXTouch, but I > guess that's not a huge problem. That's totally normal.
Mark Brown wrote: > I'm sorry, this just doesn't seem plausible. The data is going to be on > the running system and accessed via the kernel - as soon as people start > using this back door they're going to be revealing what they're > accessing and the information is going to be present in the binary blobs. > You're only revealing that the parameters are present, not what they > mean, and if it's a big concern then do something like strip down the > data file that gets shipped in production to just the controls that are > being accessed. I am not a legal expert, but I have described what you suggest to people who are more expert in the NDA terms and they say I cannot implement a solution which exposes the names of all the parameters. In any case, although having a data file which maps all the parameters is a good idea, they don't exist at present time, so it's really a moot point in terms of reaching a usable solution. Without the register names all you really have is the object protocol that we started with, you would end up accessing /sys/bus/i2c/drivers/atmel_mxt_ts/1-0020/objectX/registerX rather than parsing the object table once when your program started, then performing a read/write to offset X. And we wouldn't have won anything, because the user could still write to the register that turns off reporting (for example) by mistake with both methods. And I would then have to write a user-space program that stuck it all back together and simulated the register access interface so our existing tools would work properly, and it would probably introduce a bunch of new bugs. > Again, the fact that you have shipped this stuff doesn't make much > difference here - you really should work with upstream on interfaces > like this sooner rather than later otherwise you're going to have to > cope with pushback. I appreciate your feedback. In the end the only way to find what will be accepted is to post patches and try to justify the technical decisions made. You may dislike the way that these chips have been implemented, but in the end I can't rip it all up and redesign, I'm only the driver guy and I have to try and improve things in an incremental fashion from where they currently are. The other patches in this series are much more important. If the answer is that I need to remove this particular change from the series and rework the driver on top of regmap (or design something new) to meet this requirement, then that is disappointing. But so be it, we want to be working upstream so we want to solve this problem upstream. >> If we expose every single parameter as a get/set as you suggest, we haven't >> added anything that stops a binary of the wrong version doing something >> stupid. We've just added a complex API to the same underlying thing, which >> gains nothing. > > So this goes back to what I was saying before about the interface being > badly designed - if the API you have to present is really this complex > then you've got a big problem in kernel or out of kernel. Touch controllers are inherently complex system with a lot of configurable parameters. The fact that it's complex and changes quickly doesn't make it badly designed per se. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 06, 2013 at 05:13:32PM +0100, Nick Dyer wrote: > I am not a legal expert, but I have described what you suggest to people > who are more expert in the NDA terms and they say I cannot implement a > solution which exposes the names of all the parameters. In any case, Who said anything about names? I'd assume the userspace stuff is eventually talking to the device based on numbers of some kind and I'd expect that this would be what ends up in the API. > Without the register names all you really have is the object protocol that > we started with, you would end up accessing > /sys/bus/i2c/drivers/atmel_mxt_ts/1-0020/objectX/registerX > rather than parsing the object table once when your program started, then > performing a read/write to offset X. Well, assuming sysfs makes sense for this. It's not immediately clear to me that it does. > And we wouldn't have won anything, because the user could still write to > the register that turns off reporting (for example) by mistake with both > methods. You'd not have access to the entire device register map which seems like a win and people would be able to integrate sensibly with things like the overall device power management (which might help with whatever is causing you to have to cope with failing I/O) more smoothly. > > So this goes back to what I was saying before about the interface being > > badly designed - if the API you have to present is really this complex > > then you've got a big problem in kernel or out of kernel. > Touch controllers are inherently complex system with a lot of configurable > parameters. The fact that it's complex and changes quickly doesn't make it > badly designed per se. Having lots of configuration and having the parameters change regularly doesn't immediately mean that it has to be complex - the requirement is very common, touchscreens aren't too remarkable here. The usual thing is for the kernel to understand how to transfer data back and forth but not the content of the data.
On Wed, Jun 05, 2013 at 06:37:03PM +0100, Nick Dyer wrote: > Atmel maXTouch chips can be addressed via an "Object Based Protocol" which > defines how i2c registers are mapped to different functions within the > chips. This interface exposes the register map and allows user-space > utilities to inspect and alter object configuration, and to view diagnostic > data, while the device is running. > > Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk> > Acked-by: Benson Leung <bleung@chromium.org> > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 82 ++++++++++++++++++++++++++++++ > 1 file changed, 82 insertions(+) Whenever you add/remove/modify sysfs files, you also need a Documentation/ABI/ update as well. Please include that. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Greg KH wrote: > On Wed, Jun 05, 2013 at 06:37:03PM +0100, Nick Dyer wrote: >> Atmel maXTouch chips can be addressed via an "Object Based Protocol" which >> defines how i2c registers are mapped to different functions within the >> chips. This interface exposes the register map and allows user-space >> utilities to inspect and alter object configuration, and to view diagnostic >> data, while the device is running. >> >> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk> >> Acked-by: Benson Leung <bleung@chromium.org> >> --- >> drivers/input/touchscreen/atmel_mxt_ts.c | 82 ++++++++++++++++++++++++++++++ >> 1 file changed, 82 insertions(+) > > Whenever you add/remove/modify sysfs files, you also need a > Documentation/ABI/ update as well. Please include that. Will do. I think I should also document the existing sysfs files which are missing docs. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown wrote: > On Thu, Jun 06, 2013 at 05:13:32PM +0100, Nick Dyer wrote: > >> I am not a legal expert, but I have described what you suggest to people >> who are more expert in the NDA terms and they say I cannot implement a >> solution which exposes the names of all the parameters. In any case, > > Who said anything about names? I'd assume the userspace stuff is > eventually talking to the device based on numbers of some kind and I'd > expect that this would be what ends up in the API. OK. But if user-space is talking to the device based on register numbers, a binary attribute seems like the correct way to present that - isn't that what they're designed for? >> And we wouldn't have won anything, because the user could still write to >> the register that turns off reporting (for example) by mistake with both >> methods. > > You'd not have access to the entire device register map which seems like > a win Not really. The chip itself will enforce which registers are read-only (by ignoring writes) or write-only (by returning zeros if read). Encoding all this information in the driver would just make it more brittle in the face of touch controller firmware updates. It would be possible for the driver to intermediate for some of the registers that it cares about. For example, if we change the screen width then the driver could reinitialise the input device. But I can't see that it makes sense if you are changing several settings in a row for the input device to be reinitialised several times. It is far less buggy to provide a single way of tearing down everything and reinitialising (which could be simply triggered from user space) than to encode all of the dependencies (which is bound to introduce bugs). It's not laziness, it's trying to push policy into user space. >> Touch controllers are inherently complex system with a lot of configurable >> parameters. The fact that it's complex and changes quickly doesn't make it >> badly designed per se. > > Having lots of configuration and having the parameters change regularly > doesn't immediately mean that it has to be complex - the requirement is > very common, touchscreens aren't too remarkable here. The usual thing > is for the kernel to understand how to transfer data back and forth but > not the content of the data. Sure, that's what I'm trying to accomplish. It's just that as far as I can see it makes more sense to use the established protocol that these chips have implemented rather than trying to bodge something else over the top or crowbar it into a different model that will cause abstraction problems. We have thought about this problem at great length, and discussed it on these lists, and with other kernel engineers at customers, etc. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 07, 2013 at 04:12:25PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > Who said anything about names? I'd assume the userspace stuff is > > eventually talking to the device based on numbers of some kind and I'd > > expect that this would be what ends up in the API. > OK. But if user-space is talking to the device based on register numbers, a > binary attribute seems like the correct way to present that - isn't that > what they're designed for? I thought there was this protocol you're concerned aboout, not raw registers? Presenting the actual data in binary form seems sane, how one gets to the data is the issue. > >> And we wouldn't have won anything, because the user could still write to > >> the register that turns off reporting (for example) by mistake with both > >> methods. > > You'd not have access to the entire device register map which seems like > > a win > Not really. > The chip itself will enforce which registers are read-only (by ignoring > writes) or write-only (by returning zeros if read). Encoding all this > information in the driver would just make it more brittle in the face of > touch controller firmware updates. So not only do you interact with the firmware via this protocol but the actual hardware register map is unstable and there's nothing in the device that the driver itself actually interacts with, all it does is ferry these messages from the application layer to the device? Given the number of other patches here that doesn't seem to be the case... > It would be possible for the driver to intermediate for some of the > registers that it cares about. For example, if we change the screen width > then the driver could reinitialise the input device. But I can't see that > it makes sense if you are changing several settings in a row for the input > device to be reinitialised several times. It is far less buggy to provide a > single way of tearing down everything and reinitialising (which could be > simply triggered from user space) than to encode all of the dependencies > (which is bound to introduce bugs). I am having an extremely hard time connecting anything you're talking about here with the discussion at hand I'm afraid... > > Having lots of configuration and having the parameters change regularly > > doesn't immediately mean that it has to be complex - the requirement is > > very common, touchscreens aren't too remarkable here. The usual thing > > is for the kernel to understand how to transfer data back and forth but > > not the content of the data. > Sure, that's what I'm trying to accomplish. It's just that as far as I can > see it makes more sense to use the established protocol that these chips > have implemented rather than trying to bodge something else over the top or > crowbar it into a different model that will cause abstraction problems. We > have thought about this problem at great length, and discussed it on these > lists, and with other kernel engineers at customers, etc. Nobody is talking about changing the protocol for interacting with the device. The discussion here is about the ABI the driver offers to the application layer.
Mark Brown wrote: >> OK. But if user-space is talking to the device based on register numbers, a >> binary attribute seems like the correct way to present that - isn't that >> what they're designed for? > > I thought there was this protocol you're concerned aboout, not raw > registers? Presenting the actual data in binary form seems sane, how > one gets to the data is the issue. OK, so we seem to have gone round in a circle here. Initially I understood you to say that providing a binary read/write attribute for access to the configuration data was not acceptable. >> The chip itself will enforce which registers are read-only (by ignoring >> writes) or write-only (by returning zeros if read). Encoding all this >> information in the driver would just make it more brittle in the face of >> touch controller firmware updates. > > So not only do you interact with the firmware via this protocol but the > actual hardware register map is unstable The register map is fixed at firmware compile time. The driver contains code which parses the object table and figures out the correct register offsets which are used on the particular chip that it is talking to. The user space tools that we have written contain an equivalent parser. Is it the duplication of this code that is your concern? > and there's nothing in the > device that the driver itself actually interacts with, all it does is > ferry these messages from the application layer to the device? Given > the number of other patches here that doesn't seem to be the case... The driver does interact with a subset of the registers. It's main job is reading a certain "object" (set of registers) when triggered by interrupt, which contain the touch reports which are passed to user space. There are other registers that the driver uses, eg screen parameters, power control, telling the device to reset. Are you saying that your concern is that user space shouldn't be able to directly access these registers, for example to trigger a reset? In which case, how should user space reset the chip if required? >> It would be possible for the driver to intermediate for some of the >> registers that it cares about. For example, if we change the screen width >> then the driver could reinitialise the input device. But I can't see that >> it makes sense if you are changing several settings in a row for the input >> device to be reinitialised several times. It is far less buggy to provide a >> single way of tearing down everything and reinitialising (which could be >> simply triggered from user space) than to encode all of the dependencies >> (which is bound to introduce bugs). > > I am having an extremely hard time connecting anything you're talking > about here with the discussion at hand I'm afraid... I'm trying to provide the background to this design as you've requested, I apologise if you find it confusing. > Nobody is talking about changing the protocol for interacting with the > device. The discussion here is about the ABI the driver offers to the > application layer. OK. I think that the ABI offered to the application layer should also be object protocol, implemented over a binary attribute, which is what the patch under discussion does. Is the problem that I need to provide better documentation of object protocol? -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 07, 2013 at 05:11:28PM +0100, Nick Dyer wrote: > Mark Brown wrote: > > I thought there was this protocol you're concerned aboout, not raw > > registers? Presenting the actual data in binary form seems sane, how > > one gets to the data is the issue. > OK, so we seem to have gone round in a circle here. Initially I understood > you to say that providing a binary read/write attribute for access to the > configuration data was not acceptable. What was being proposed was just mapping the register map straight into userspace and implementing the entire protocol in userspace. Having the unparsed attributes visible themselves is relatively normal. > >> The chip itself will enforce which registers are read-only (by ignoring > >> writes) or write-only (by returning zeros if read). Encoding all this > >> information in the driver would just make it more brittle in the face of > >> touch controller firmware updates. > > So not only do you interact with the firmware via this protocol but the > > actual hardware register map is unstable > The register map is fixed at firmware compile time. The driver contains > code which parses the object table and figures out the correct register > offsets which are used on the particular chip that it is talking to. > The user space tools that we have written contain an equivalent parser. Is > it the duplication of this code that is your concern? I don't think you're using the usual definition of "register map" here. You seem to be switching between talking about this object model the device has and device registers - perhaps the objects are also registers sometimes? > Are you saying that your concern is that user space shouldn't be able to > directly access these registers, for example to trigger a reset? In which > case, how should user space reset the chip if required? If the application layer can reset the device underneath the driver that doesn't seem awesome; similarly if the application layer is having to worry about the device getting powered off underneath it this doesn't seem great either. > >> It would be possible for the driver to intermediate for some of the > >> registers that it cares about. For example, if we change the screen width > >> then the driver could reinitialise the input device. But I can't see that > >> it makes sense if you are changing several settings in a row for the input > >> device to be reinitialised several times. It is far less buggy to provide a > >> single way of tearing down everything and reinitialising (which could be > >> simply triggered from user space) than to encode all of the dependencies > >> (which is bound to introduce bugs). > > I am having an extremely hard time connecting anything you're talking > > about here with the discussion at hand I'm afraid... > I'm trying to provide the background to this design as you've requested, I > apologise if you find it confusing. You appear to be assuming a great deal of familiarity with the specifics of this device here. Where does all thi stuff about reinitialsing the device come from? What are these dependencies and what does all this have to do with setting parameters? > > Nobody is talking about changing the protocol for interacting with the > > device. The discussion here is about the ABI the driver offers to the > > application layer. > OK. I think that the ABI offered to the application layer should also be > object protocol, implemented over a binary attribute, which is what the > patch under discussion does. Is the problem that I need to provide better > documentation of object protocol? As Greg says you do need to document any new sysfs ABIs you're adding anyway. However if this is some stateful protocol you're implementing then does it really map onto sysfs well, sysfs attributes are normally more just data values? Won't the driver end up getting into a fight with the magic userspace stuff if the driver has no idea what the magic userspace is doing? How would suspend and resume work?
Mark Brown wrote: > I don't think you're using the usual definition of "register map" here. > You seem to be switching between talking about this object model the > device has and device registers - perhaps the objects are also registers > sometimes? Yes, in Atmel Object Protocol "instances" of "objects" do each have an assigned register range (they do also correspond to modules of code in the firmware). This document is a better description of it than I can give: http://www.atmel.com/Images/Atmel-9626-AT42-QTouch-BSW-AT421085-Object-Protocol-Guide_Datasheet.pdf That's a rather old chip now, but the same system is used throughout maXTouch series chips, and also on some non-touch chips now. > You appear to be assuming a great deal of familiarity with the specifics > of this device here. Where does all thi stuff about reinitialsing the > device come from? What are these dependencies and what does all this > have to do with setting parameters? I think you're uncomfortable with the idea of giving full access by default. I'm not sure how I can convince you that that is OK by design, I can only assert it. Essentially, the device is designed (and tested) on the assumption that touch processing can be going on whilst various parameters are changed in a live fashion. If poking around in the register map caused it to lock up or behave oddly then that would be considered a firmware bug. In general it will fail gracefully - for example if you write a configuration that is invalid it will just stop and emit a CFGERR message. In my previous messages in this thread, I tried to answer some of the particular cases in which you might expect the driver to have to cope with the configuration changing "under it". It's difficult to be exhaustive without having to impart a huge amount of domain knowledge first. In the patch under discussion, the driver would have to trap writes to particular registers and take appropriate action, or provide a mechanism to be told that it needs to reinitialise itself. In practice (it's been widely tested outside of mainline) I haven't found a case where doing this has been essential, although there's nothing about this patch that stops us adding them. The absolute worst thing that I can think of is that you can try to beat the interrupt handler to reading the "message processor" registers, which would possibly leave touches stuck on screen. But even that operation is useful in debugging interrupt line issues. >> OK. I think that the ABI offered to the application layer should also be >> object protocol, implemented over a binary attribute, which is what the >> patch under discussion does. Is the problem that I need to provide better >> documentation of object protocol? > > As Greg says you do need to document any new sysfs ABIs you're adding > anyway. However if this is some stateful protocol you're implementing > then does it really map onto sysfs well, sysfs attributes are normally > more just data values? Perhaps it is a little unusual. There was an older ioctl-based implementation, but I think that would be rejected for different reasons. > Won't the driver end up getting into a fight with the magic userspace > stuff if the driver has no idea what the magic userspace is doing? How > would suspend and resume work? On suspend the driver puts chip into "deep sleep" where touch acquisition is halted and minimal power consumed. But it will still come out of its sleep state temporarily to service I2C comms if necessary (although one particular family requires that I2C retry for this case). -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 11, 2013 at 11:39:37AM +0100, Nick Dyer wrote: > Mark Brown wrote: > > I don't think you're using the usual definition of "register map" here. > > You seem to be switching between talking about this object model the > > device has and device registers - perhaps the objects are also registers > > sometimes? > Yes, in Atmel Object Protocol "instances" of "objects" do each have an > assigned register range (they do also correspond to modules of code in the > firmware). OK, so you're talking about the objects which happen to be implemented in terms of the register map here. I think this is confusing to me because you are moving between the abstraction levels. > Essentially, the device is designed (and tested) on the assumption that > touch processing can be going on whilst various parameters are changed in a > live fashion. If poking around in the register map caused it to lock up or > behave oddly then that would be considered a firmware bug. In general it > will fail gracefully - for example if you write a configuration that is > invalid it will just stop and emit a CFGERR message. That's not really it, it's the fact that this is being done with no abstraction - exposing the entire device register map directly to application layer so the application can totally ignore what's going on in the kernel doesn't seem like awesome design. > The absolute worst thing that I can think of is that you can try to beat > the interrupt handler to reading the "message processor" registers, which > would possibly leave touches stuck on screen. But even that operation is > useful in debugging interrupt line issues. Well, there's also the potential issues with standard application layer code either not being able to do things that the hardware supports or getting into a fight with the magic custom stuff. > > Won't the driver end up getting into a fight with the magic userspace > > stuff if the driver has no idea what the magic userspace is doing? How > > would suspend and resume work? > On suspend the driver puts chip into "deep sleep" where touch acquisition > is halted and minimal power consumed. But it will still come out of its > sleep state temporarily to service I2C comms if necessary (although one > particular family requires that I2C retry for this case). So these errors are just part of waking the chip up - in that case shouldn't the driver be waking the chip up automatically?
Mark Brown wrote: >> Essentially, the device is designed (and tested) on the assumption that >> touch processing can be going on whilst various parameters are changed in a >> live fashion. If poking around in the register map caused it to lock up or >> behave oddly then that would be considered a firmware bug. In general it >> will fail gracefully - for example if you write a configuration that is >> invalid it will just stop and emit a CFGERR message. > > That's not really it, it's the fact that this is being done with no > abstraction - exposing the entire device register map directly to > application layer so the application can totally ignore what's going on > in the kernel doesn't seem like awesome design. Without being able to name all of the registers (which would require a large amount of architecture to keep up-to-date and would probably turn into an unmaintainable mess), you can only split up the register map into separate parts. You'd end up with binary attributes like this (refer to the document I linked for explanation): info_block object_table t5 t6 t9instance1 t9instance2 etc Is that a nicer design from your point of view? I don't personally think that is really gains anything functional other than making the API more complex and increasing the number of read/write operations. I guess you would stop bugs in user space code from accidentally writing into the wrong object. >> The absolute worst thing that I can think of is that you can try to beat >> the interrupt handler to reading the "message processor" registers, which >> would possibly leave touches stuck on screen. But even that operation is >> useful in debugging interrupt line issues. > > Well, there's also the potential issues with standard application layer > code either not being able to do things that the hardware supports or > getting into a fight with the magic custom stuff. I think it's better to present a clean API cut at the right level. If you look at the drivers in various Android trees for these maXTouch chips, there's an awful lot of phone specific code which is not very general and it would be impossible to mainline without having a 20,000 line driver full of #ifdefs. Surely it's better for that to go into a userspace daemon if possible? >> On suspend the driver puts chip into "deep sleep" where touch acquisition >> is halted and minimal power consumed. But it will still come out of its >> sleep state temporarily to service I2C comms if necessary (although one >> particular family requires that I2C retry for this case). > > So these errors are just part of waking the chip up - in that case > shouldn't the driver be waking the chip up automatically? In the reference design for that particular model of chip (mXT1386), there is a WAKEUP pin cross-wired to an I2C line. The only way of getting it to wake up when it is asleep is by trying to perform an I2C operation, which will fail, and then retrying before it times out and goes back to sleep again. There isn't any other way of waking it up. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 11, 2013 at 01:16:31PM +0100, Nick Dyer wrote: > Without being able to name all of the registers (which would require a > large amount of architecture to keep up-to-date and would probably turn > into an unmaintainable mess), you can only split up the register map into > separate parts. You'd end up with binary attributes like this (refer to the > document I linked for explanation): ... > Is that a nicer design from your point of view? I don't personally think > that is really gains anything functional other than making the API more > complex and increasing the number of read/write operations. I guess you Yes, to be honest. I'd hope it wouldn't be increasing the number of read/write operations... > would stop bugs in user space code from accidentally writing into the wrong > object. That seems like a useful thing, and it does allow the driver to relatively easily understand what any of the attributes is doing and take it over if it needs to. > > Well, there's also the potential issues with standard application layer > > code either not being able to do things that the hardware supports or > > getting into a fight with the magic custom stuff. > I think it's better to present a clean API cut at the right level. If you > look at the drivers in various Android trees for these maXTouch chips, > there's an awful lot of phone specific code which is not very general and > it would be impossible to mainline without having a 20,000 line driver full > of #ifdefs. Surely it's better for that to go into a userspace daemon if > possible? Yes, having some of the stuff that understands the contents of the controls in userspace is sensible. However the kernel does offer an API for controlling devices it supports and it seems reasonable to expect that to work too - callibration and whatnot is a bit different but core functionality should just work from the kernel. > >> On suspend the driver puts chip into "deep sleep" where touch acquisition > >> is halted and minimal power consumed. But it will still come out of its > >> sleep state temporarily to service I2C comms if necessary (although one > >> particular family requires that I2C retry for this case). > > So these errors are just part of waking the chip up - in that case > > shouldn't the driver be waking the chip up automatically? > In the reference design for that particular model of chip (mXT1386), there > is a WAKEUP pin cross-wired to an I2C line. The only way of getting it to > wake up when it is asleep is by trying to perform an I2C operation, which > will fail, and then retrying before it times out and goes back to sleep > again. There isn't any other way of waking it up. That still sounds like something the driver can handle (for example, by eating the first error silently if it knows the chip is powered down) and of course a system integrator may choose not to copy the reference design in this respect, it does seem a bit odd after all.
Mark Brown wrote: >> Is that a nicer design from your point of view? I don't personally think >> that is really gains anything functional other than making the API more >> complex and increasing the number of read/write operations. I guess you > > Yes, to be honest. I'd hope it wouldn't be increasing the number of > read/write operations... For some operations it does. For example updating the whole chip config (which is a common thing to want to do), it would turn a couple of write operations into ~20 on recent chips. >> would stop bugs in user space code from accidentally writing into the wrong >> object. > > That seems like a useful thing, and it does allow the driver to > relatively easily understand what any of the attributes is doing and > take it over if it needs to. I guess you might save a tiny amount of lookup code. >>> Well, there's also the potential issues with standard application layer >>> code either not being able to do things that the hardware supports or >>> getting into a fight with the magic custom stuff. > >> I think it's better to present a clean API cut at the right level. If you >> look at the drivers in various Android trees for these maXTouch chips, >> there's an awful lot of phone specific code which is not very general and >> it would be impossible to mainline without having a 20,000 line driver full >> of #ifdefs. Surely it's better for that to go into a userspace daemon if >> possible? > > Yes, having some of the stuff that understands the contents of the > controls in userspace is sensible. However the kernel does offer an API > for controlling devices it supports and it seems reasonable to expect > that to work too - callibration and whatnot is a bit different but core > functionality should just work from the kernel. I completely agree - I just don't think that the API under discussion really forces a choice over any of this. Anyway, I will pull this patch (and the other sysfs interface one) from the series, and try to come up with something along the lines that we have discussed. >> In the reference design for that particular model of chip (mXT1386), there >> is a WAKEUP pin cross-wired to an I2C line. The only way of getting it to >> wake up when it is asleep is by trying to perform an I2C operation, which >> will fail, and then retrying before it times out and goes back to sleep >> again. There isn't any other way of waking it up. > > That still sounds like something the driver can handle (for example, by > eating the first error silently if it knows the chip is powered down) We've tried to implement this idea of tracking the chip power state in the driver and only eating the first error silently when necessary. But there are various entertaining corner cases (for example, it may or may not be in sleep on probe, how do you deal with intermittent i2c glitch). It would end up either being very brittle or an extremely complex mechanism involving tracking state and timers, the result of which is only to suppress a dmesg debug output saying "i2c retry", and to fail very slightly earlier in the normal i2c failure case. The normal fast path through this code is exactly the same. > and of course a system integrator may choose not to copy the reference > design in this respect, it does seem a bit odd after all. You're being a bit optimistic there. Examples of devices that require this are Samsung Galaxy Tab 10.1, Asus Transformer TF101. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 21, 2013 at 09:16:16AM -0700, Nick Dyer wrote: > Mark Brown wrote: > > Yes, to be honest. I'd hope it wouldn't be increasing the number of > > read/write operations... > For some operations it does. For example updating the whole chip config > (which is a common thing to want to do), it would turn a couple of write > operations into ~20 on recent chips. Is that really happening on peformance critical paths other than initial power up (which could be handled more neatly anyway). > > That still sounds like something the driver can handle (for example, by > > eating the first error silently if it knows the chip is powered down) > We've tried to implement this idea of tracking the chip power state in > the driver and only eating the first error silently when necessary. But > there are various entertaining corner cases (for example, it may or may > not be in sleep on probe, how do you deal with intermittent i2c glitch). It > would end up either being very brittle or an extremely complex mechanism > involving tracking state and timers, the result of which is only to > suppress a dmesg debug output saying "i2c retry", and to fail very slightly > earlier in the normal i2c failure case. The normal fast path through > this code is exactly the same. It seems very suspicous that this device has all these problems but others don't... > > and of course a system integrator may choose not to copy the reference > > design in this respect, it does seem a bit odd after all. > You're being a bit optimistic there. Examples of devices that require > this are Samsung Galaxy Tab 10.1, Asus Transformer TF101. If absoluely nobody has used the separate wakeup pin then the hardware designers are wasting a pin there... my point isn't that nobody would use the reference design it's that some boards will have the separate signal.
Mark Brown wrote: > On Fri, Jun 21, 2013 at 09:16:16AM -0700, Nick Dyer wrote: >> Mark Brown wrote: >>> Yes, to be honest. I'd hope it wouldn't be increasing the number of >>> read/write operations... > >> For some operations it does. For example updating the whole chip config >> (which is a common thing to want to do), it would turn a couple of write >> operations into ~20 on recent chips. > > Is that really happening on peformance critical paths other than initial > power up (which could be handled more neatly anyway). Well, you're right that we could probably add more API for performance critical stuff. But that wasn't your original question. >>> and of course a system integrator may choose not to copy the reference >>> design in this respect, it does seem a bit odd after all. > >> You're being a bit optimistic there. Examples of devices that require >> this are Samsung Galaxy Tab 10.1, Asus Transformer TF101. > > If absoluely nobody has used the separate wakeup pin then the hardware > designers are wasting a pin there... my point isn't that nobody would > use the reference design it's that some boards will have the separate > signal. That's entirely hypothetical, and you're wasting our time until you can actually point to such hardware, happy to write patches to support that mode of operation as well if you do. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 11, 2013 at 08:41:41AM +0100, Nick Dyer wrote: > Mark Brown wrote: > > On Fri, Jun 21, 2013 at 09:16:16AM -0700, Nick Dyer wrote: > >> For some operations it does. For example updating the whole chip config > >> (which is a common thing to want to do), it would turn a couple of write > >> operations into ~20 on recent chips. > > Is that really happening on peformance critical paths other than initial > > power up (which could be handled more neatly anyway). > Well, you're right that we could probably add more API for performance > critical stuff. But that wasn't your original question. You probably don't need another API here - for example, if the driver understands that the device is powered off and knows enugh about what the device is doing to understand that it doesn't need to be running then it could for example cache what is being written then flush in a more optimal fashion. > > If absoluely nobody has used the separate wakeup pin then the hardware > > designers are wasting a pin there... my point isn't that nobody would > > use the reference design it's that some boards will have the separate > > signal. > That's entirely hypothetical, and you're wasting our time until you can > actually point to such hardware, happy to write patches to support that > mode of operation as well if you do. See above - it's not just about the possibility that someone might have done a better job with the hardware design, better power management is just a good idea in general.
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 11ee78a..b4bc16f 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -52,6 +52,8 @@ #define MXT_OBJECT_SIZE 6 +#define MXT_MAX_BLOCK_WRITE 256 + /* Object types */ #define MXT_DEBUG_DIAGNOSTIC_T37 37 #define MXT_GEN_MESSAGE_T5 5 @@ -260,6 +262,8 @@ struct mxt_data { unsigned int max_x; unsigned int max_y; bool in_bootloader; + u16 mem_size; + struct bin_attribute mem_access_attr; /* Cached parameters from object table */ u8 T6_reportid; @@ -863,6 +867,7 @@ static int mxt_get_object_table(struct mxt_data *data) int error; int i; u8 reportid; + u16 end_address; table_size = data->info.object_num * sizeof(struct mxt_object); error = __mxt_read_reg(client, MXT_OBJECT_START, table_size, @@ -872,6 +877,7 @@ static int mxt_get_object_table(struct mxt_data *data) /* Valid Report IDs start counting from 1 */ reportid = 1; + data->mem_size = 0; for (i = 0; i < data->info.object_num; i++) { struct mxt_object *object = data->object_table + i; u8 min_id, max_id; @@ -907,6 +913,12 @@ static int mxt_get_object_table(struct mxt_data *data) data->T19_reportid = min_id; break; } + + end_address = object->start_address + + mxt_obj_size(object) * mxt_obj_instances(object) - 1; + + if (end_address >= data->mem_size) + data->mem_size = end_address + 1; } return 0; @@ -1198,6 +1210,56 @@ static ssize_t mxt_update_fw_store(struct device *dev, return count; } +static int mxt_check_mem_access_params(struct mxt_data *data, loff_t off, + size_t *count) +{ + if (off >= data->mem_size) + return -EIO; + + if (off + *count > data->mem_size) + *count = data->mem_size - off; + + if (*count > MXT_MAX_BLOCK_WRITE) + *count = MXT_MAX_BLOCK_WRITE; + + return 0; +} + +static ssize_t mxt_mem_access_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct mxt_data *data = dev_get_drvdata(dev); + int ret = 0; + + ret = mxt_check_mem_access_params(data, off, &count); + if (ret < 0) + return ret; + + if (count > 0) + ret = __mxt_read_reg(data->client, off, count, buf); + + return ret == 0 ? count : ret; +} + +static ssize_t mxt_mem_access_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, loff_t off, + size_t count) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct mxt_data *data = dev_get_drvdata(dev); + int ret = 0; + + ret = mxt_check_mem_access_params(data, off, &count); + if (ret < 0) + return ret; + + if (count > 0) + ret = __mxt_write_reg(data->client, off, count, buf); + + return ret == 0 ? count : 0; +} + static DEVICE_ATTR(fw_version, S_IRUGO, mxt_fw_version_show, NULL); static DEVICE_ATTR(hw_version, S_IRUGO, mxt_hw_version_show, NULL); static DEVICE_ATTR(object, S_IRUGO, mxt_object_show, NULL); @@ -1359,8 +1421,24 @@ static int mxt_probe(struct i2c_client *client, if (error) goto err_unregister_device; + sysfs_bin_attr_init(&data->mem_access_attr); + data->mem_access_attr.attr.name = "mem_access"; + data->mem_access_attr.attr.mode = S_IRUGO | S_IWUSR; + data->mem_access_attr.read = mxt_mem_access_read; + data->mem_access_attr.write = mxt_mem_access_write; + data->mem_access_attr.size = data->mem_size; + + if (sysfs_create_bin_file(&client->dev.kobj, + &data->mem_access_attr) < 0) { + dev_err(&client->dev, "Failed to create %s\n", + data->mem_access_attr.attr.name); + goto err_remove_sysfs_group; + } + return 0; +err_remove_sysfs_group: + sysfs_remove_group(&client->dev.kobj, &mxt_attr_group); err_unregister_device: input_unregister_device(input_dev); input_dev = NULL; @@ -1378,6 +1456,10 @@ static int mxt_remove(struct i2c_client *client) { struct mxt_data *data = i2c_get_clientdata(client); + if (data->mem_access_attr.attr.name) + sysfs_remove_bin_file(&client->dev.kobj, + &data->mem_access_attr); + sysfs_remove_group(&client->dev.kobj, &mxt_attr_group); free_irq(data->irq, data); input_unregister_device(data->input_dev);