Re: [PATCH 10/53] Input: atmel_mxt_ts - Add memory access interfacevia sysfs

[Mark Brown]

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?

Attachment: signature.asc
Description: Digital signature