diff mbox

[10/53] Input: atmel_mxt_ts - Add memory access interface via sysfs

Message ID 1370453866-16534-11-git-send-email-nick.dyer@itdev.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Nick Dyer June 5, 2013, 5:37 p.m. UTC
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(+)

Comments

Dmitry Torokhov June 5, 2013, 5:57 p.m. UTC | #1
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.
Nick Dyer June 5, 2013, 6:45 p.m. UTC | #2
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
Dmitry Torokhov June 5, 2013, 7:04 p.m. UTC | #3
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.
Nick Dyer June 5, 2013, 8:31 p.m. UTC | #4
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
Dmitry Torokhov June 5, 2013, 9:07 p.m. UTC | #5
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.
Nick Dyer June 5, 2013, 9:36 p.m. UTC | #6
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
Mark Brown June 6, 2013, 9:48 a.m. UTC | #7
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.
Mark Brown June 6, 2013, 10:03 a.m. UTC | #8
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?
Nick Dyer June 6, 2013, 10:31 a.m. UTC | #9
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
Nick Dyer June 6, 2013, 10:40 a.m. UTC | #10
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
Mark Brown June 6, 2013, 10:46 a.m. UTC | #11
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.
Mark Brown June 6, 2013, 10:55 a.m. UTC | #12
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.
Nick Dyer June 6, 2013, 11 a.m. UTC | #13
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
Mark Brown June 6, 2013, 11:14 a.m. UTC | #14
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.
Nick Dyer June 6, 2013, 11:17 a.m. UTC | #15
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
Nick Dyer June 6, 2013, 11:34 a.m. UTC | #16
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
Mark Brown June 6, 2013, 1:16 p.m. UTC | #17
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.
Mark Brown June 6, 2013, 1:21 p.m. UTC | #18
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.
Nick Dyer June 6, 2013, 4:13 p.m. UTC | #19
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
Mark Brown June 6, 2013, 4:46 p.m. UTC | #20
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.
Greg Kroah-Hartman June 6, 2013, 6:01 p.m. UTC | #21
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
Nick Dyer June 7, 2013, 2:47 p.m. UTC | #22
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
Nick Dyer June 7, 2013, 3:12 p.m. UTC | #23
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
Mark Brown June 7, 2013, 3:41 p.m. UTC | #24
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.
Nick Dyer June 7, 2013, 4:11 p.m. UTC | #25
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
Mark Brown June 7, 2013, 4:47 p.m. UTC | #26
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?
Nick Dyer June 11, 2013, 10:39 a.m. UTC | #27
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
Mark Brown June 11, 2013, 11:47 a.m. UTC | #28
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?
Nick Dyer June 11, 2013, 12:16 p.m. UTC | #29
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
Mark Brown June 19, 2013, 6:59 p.m. UTC | #30
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.
Nick Dyer June 21, 2013, 4:16 p.m. UTC | #31
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
Mark Brown July 2, 2013, 10:11 a.m. UTC | #32
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.
Nick Dyer July 11, 2013, 7:41 a.m. UTC | #33
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
Mark Brown July 11, 2013, 10:30 a.m. UTC | #34
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 mbox

Patch

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);