Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny wrote: > rmi.h provides public definitions required by the RMI bus implementation and > modules that interact with it. > > debugfs and sysfs attributes are documented in files in > Documentation/ABI/testing. There's two files, one for debugfs and one for > sysfs. > > > Signed-off-by: Christopher Heiny > Cut this white line. > Cc: Dmitry Torokhov > Cc: Linus Walleij > Cc: Naveen Kumar Gaddipati > Cc: Joeri de Gram There is always room for improvement, but you've done a massive job at fixing this driver to a state which is perfectly maintainable in the kernel. Reviewed-by: Linus Walleij Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny che...@synaptics.com wrote: rmi.h provides public definitions required by the RMI bus implementation and modules that interact with it. debugfs and sysfs attributes are documented in files in Documentation/ABI/testing. There's two files, one for debugfs and one for sysfs. Signed-off-by: Christopher Heiny che...@synaptics.com Cut this white line. Cc: Dmitry Torokhov dmitry.torok...@gmail.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com Cc: Joeri de Gram j.de.g...@gmail.com There is always room for improvement, but you've done a massive job at fixing this driver to a state which is perfectly maintainable in the kernel. Reviewed-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 01/06] input/rmi4: Public header and documentation
rmi.h provides public definitions required by the RMI bus implementation and modules that interact with it. debugfs and sysfs attributes are documented in files in Documentation/ABI/testing. There's two files, one for debugfs and one for sysfs. Signed-off-by: Christopher Heiny Cc: Dmitry Torokhov Cc: Linus Walleij Cc: Naveen Kumar Gaddipati Cc: Joeri de Gram --- Documentation/ABI/testing/debugfs-rmi4 | 99 ++ Documentation/ABI/testing/sysfs-rmi4 | 103 ++ include/linux/rmi.h| 596 3 files changed, 798 insertions(+), 0 deletions(-) diff --git a/Documentation/ABI/testing/debugfs-rmi4 b/Documentation/ABI/testing/debugfs-rmi4 new file mode 100644 index 000..ef0739d --- /dev/null +++ b/Documentation/ABI/testing/debugfs-rmi4 @@ -0,0 +1,99 @@ +What: /sys/kernel/debug/rmi/devices +Date: October 2012 +KernelVersion: 3.x +Contact: Christopher Heiny +Description: + + The RMI4 driver implementation exposes a set of informational and control + parameters via debugfs. These parameters are those that typically are only + viewed or adjusted during product development, tuning, and debug. + For parameters that are referenced and/or adjusted during normal operation, + please see sysfs-rmi4 in this directory. + + General debugging parameters for a particular RMI4 sensor are found in + /sys/kernel/debug/rmi/sensorXX/, where XX is a the device's ID as a two + digit number (padded with leading zeros). Function specific parameters + for an RMI4 sensor are found in /sys/kernel/debug/rmi/devices/FYY/, where + XX is a the device's ID as a two digit number (padded with leading zeros) + and YY is the hexdecimal function number (for example, F11 for RMI function + F11). + + For RMI4 functions that support multiple sensor instances (such as F11), + the parameters for individual sensors have .Z appended to them, where Z is + the index of the sensor instance (for example, clip.0, clip.1, clip.2, and + so on). + + Some of the parameters exposed here are described in detail in the + RMI4 Specification, which is found here: +http://www.synaptics.com/sites/default/files/511-000136-01_revD.pdf + For such parameters, we'll reference you to that document, rather than + copying the contents here. + + /sys/kernel/debug/rmi/ + /sensorXX/ + attn_count - (ro) Shows the number of ATTN interrupts handled so far. + comms_debug - (rw) Write 1 to this dump information about register + reads and writes to the console. Write 0 to this to turn + this feature off. WARNING: Imposes a major performance + penalty when turned on. + irq_debug - (rw) Write 1 to this dump information about interrupts + to the console. Write 0 to this to turn this feature off. + WARNIG: Imposes a major performance penalty when turned on. + phys - (ro) Presents information about the physical connection of + this device. It has one line, with the format: + + prot tx_count tx_bytes tx_errors rx_count rx_bytes rx_errors attn + + Where + prot is one of i2c, spi1, or spi2 + tx_count is the number of write operations + tx_bytes is the number of bytes written + tx_errors is the number of write operations that encountered errors + rx_count is the number of read operations + rx_bytes is the total number of bytes read + rx_errors is the number of read operations that encountered errors + + All counts are 64-bit unsigned values, and are set to zero + when the physical layer driver is initialized. + + /sensorXX/F01/ + interrupt_enable - (rw) allows you to read or modify the F01 + interrupt enable mask (the F01_RMI_Ctrl1 register(s)). + + /sensorXX/F11/ + clip.Z - (rw) Controls in-driver coordinate clipping for the 2D + sensor Z. This is a set of four unsigned values in the + range [0..65535], representing the lower bounds on X, the + upper bounds on X, the lower bounds on Y, and the upper + bounds on Y. Coordinates will be clipped to these ranges. + If enabled, clip is the final transformation to be applied + to the coordinates. The default upper and lower bounds for + clip are 0 and 65535 respectively for both axes. + delta_threshold.Z - (rw) Controls the F11 distance thresholds. This + contains two values, corresponding to F11_2D_Ctrl2 and + F11_2D_Ctrl3. Se the spec for more details. + flip.Z - (rw) This parameter is a pair of single binary digits (for +
[RFC PATCH 01/06] input/rmi4: Public header and documentation
rmi.h provides public definitions required by the RMI bus implementation and modules that interact with it. debugfs and sysfs attributes are documented in files in Documentation/ABI/testing. There's two files, one for debugfs and one for sysfs. Signed-off-by: Christopher Heiny che...@synaptics.com Cc: Dmitry Torokhov dmitry.torok...@gmail.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com Cc: Joeri de Gram j.de.g...@gmail.com --- Documentation/ABI/testing/debugfs-rmi4 | 99 ++ Documentation/ABI/testing/sysfs-rmi4 | 103 ++ include/linux/rmi.h| 596 3 files changed, 798 insertions(+), 0 deletions(-) diff --git a/Documentation/ABI/testing/debugfs-rmi4 b/Documentation/ABI/testing/debugfs-rmi4 new file mode 100644 index 000..ef0739d --- /dev/null +++ b/Documentation/ABI/testing/debugfs-rmi4 @@ -0,0 +1,99 @@ +What: /sys/kernel/debug/rmi/devices +Date: October 2012 +KernelVersion: 3.x +Contact: Christopher Heiny che...@synaptics.com +Description: + + The RMI4 driver implementation exposes a set of informational and control + parameters via debugfs. These parameters are those that typically are only + viewed or adjusted during product development, tuning, and debug. + For parameters that are referenced and/or adjusted during normal operation, + please see sysfs-rmi4 in this directory. + + General debugging parameters for a particular RMI4 sensor are found in + /sys/kernel/debug/rmi/sensorXX/, where XX is a the device's ID as a two + digit number (padded with leading zeros). Function specific parameters + for an RMI4 sensor are found in /sys/kernel/debug/rmi/devices/FYY/, where + XX is a the device's ID as a two digit number (padded with leading zeros) + and YY is the hexdecimal function number (for example, F11 for RMI function + F11). + + For RMI4 functions that support multiple sensor instances (such as F11), + the parameters for individual sensors have .Z appended to them, where Z is + the index of the sensor instance (for example, clip.0, clip.1, clip.2, and + so on). + + Some of the parameters exposed here are described in detail in the + RMI4 Specification, which is found here: +http://www.synaptics.com/sites/default/files/511-000136-01_revD.pdf + For such parameters, we'll reference you to that document, rather than + copying the contents here. + + /sys/kernel/debug/rmi/ + /sensorXX/ + attn_count - (ro) Shows the number of ATTN interrupts handled so far. + comms_debug - (rw) Write 1 to this dump information about register + reads and writes to the console. Write 0 to this to turn + this feature off. WARNING: Imposes a major performance + penalty when turned on. + irq_debug - (rw) Write 1 to this dump information about interrupts + to the console. Write 0 to this to turn this feature off. + WARNIG: Imposes a major performance penalty when turned on. + phys - (ro) Presents information about the physical connection of + this device. It has one line, with the format: + + prot tx_count tx_bytes tx_errors rx_count rx_bytes rx_errors attn + + Where + prot is one of i2c, spi1, or spi2 + tx_count is the number of write operations + tx_bytes is the number of bytes written + tx_errors is the number of write operations that encountered errors + rx_count is the number of read operations + rx_bytes is the total number of bytes read + rx_errors is the number of read operations that encountered errors + + All counts are 64-bit unsigned values, and are set to zero + when the physical layer driver is initialized. + + /sensorXX/F01/ + interrupt_enable - (rw) allows you to read or modify the F01 + interrupt enable mask (the F01_RMI_Ctrl1 register(s)). + + /sensorXX/F11/ + clip.Z - (rw) Controls in-driver coordinate clipping for the 2D + sensor Z. This is a set of four unsigned values in the + range [0..65535], representing the lower bounds on X, the + upper bounds on X, the lower bounds on Y, and the upper + bounds on Y. Coordinates will be clipped to these ranges. + If enabled, clip is the final transformation to be applied + to the coordinates. The default upper and lower bounds for + clip are 0 and 65535 respectively for both axes. + delta_threshold.Z - (rw) Controls the F11 distance thresholds. This + contains two values, corresponding to F11_2D_Ctrl2 and +
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Tue, Oct 23, 2012 at 03:10:20PM -0700, Christopher Heiny wrote: > On 10/11/2012 10:16 PM, Mark Brown wrote: > >On Thu, Oct 11, 2012 at 03:56:22AM +, Christopher Heiny wrote: > >Fix your mailer to word wrap within paragraphs. > Sorry - I was on the road and had to use a web interface. It looked > OK during composition. Is this better? Yes, that's great - thanks. > However, a general capability in SPI core is a fine idea for future > work, once we get the RMI4 driver ironed out. Can we agree to defer > that for now? Obviously there's no pressure to implement the framework feature, however it shouldn't be implemented at the driver level. signature.asc Description: Digital signature
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Tue, Oct 23, 2012 at 03:10:20PM -0700, Christopher Heiny wrote: On 10/11/2012 10:16 PM, Mark Brown wrote: On Thu, Oct 11, 2012 at 03:56:22AM +, Christopher Heiny wrote: Fix your mailer to word wrap within paragraphs. Sorry - I was on the road and had to use a web interface. It looked OK during composition. Is this better? Yes, that's great - thanks. However, a general capability in SPI core is a fine idea for future work, once we get the RMI4 driver ironed out. Can we agree to defer that for now? Obviously there's no pressure to implement the framework feature, however it shouldn't be implemented at the driver level. signature.asc Description: Digital signature
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/15/2012 11:26 PM, Mark Brown wrote: On Thu, Oct 11, 2012 at 05:32:59PM +0200, Linus Walleij wrote: On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny wrote: In previous patch submissions, we always used these warning functions. But in the feedback on those patches, we were asked to just make sysfs show/store NULL if the attribute is write/read only. However, during their development process, our customers want to see the warnings if the attributes are accessed incorrectly. So we made these warnings a debug option. Basically my stance is that you should not lower yourself to the level of others not getting the point of your technical solution by making unelegant compromises, what you should do is to bring them up to your level so they understand that your solution is elegant. It seems like what you really want to do is add a debug feature to sysfs which will optionally complain loudly at bad accesses; obviously it's not something that should be there all the time as running then handling an error is a perfectly legitimate thing to do. As with the /CS handling it'd mean it was handled at an appropriate level and could be reused elsewhere (it might also help make it clear to your customers why this is generally bad form). See my reply to Dmitry of a bit ago. These are no longer needed, and we'll be dropping them. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/11/2012 08:32 AM, Linus Walleij wrote:
On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny wrote:
Linus Walleij wrote:
But please use arithmetic operators (I think I said this on the last
review):
dest[0] = src & 0xFF;
dest[1] = src >> 8;
Doing it the above way makes artithmetic look like maths, and it isn't.
Besides it's done this way in most parts of the kernel and we're
familiar with it.
Yes, you mentioned it previously. I'm somewhat paranoid, though, and
don't trust the shift/mask method to work correctly on big-endian
machines. If the shifts can be relied on to behave (I'm guessing the
answer is "yes", since you say this idiom is used widely in the
kernel), then I'll change it.
If the behaviour was not consistent across different endianness
it would not be part of the C language specification...
<< means shift left in the accumulator or whatever you have.
It will work the same no matter how bits are laid out in
memory.
OK, after reviewing the spec I'll accept that. We'll make the change.
+static inline ssize_t rmi_store_error(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ dev_warn(dev,
+"WARNING: Attempt to write %d characters to read-only
attribute %s.", + count, attr->attr.name);
+ return -EPERM;
+}
Here it looks like you're hiding a lot of stuff that should be dev_warn()?
Consider my earlier point about dynamic debug.
In previous patch submissions, we always used these warning functions.
But in the feedback on those patches, we were asked to just make
sysfs show/store NULL if the attribute is write/read only. However,
during their development process, our customers want to see the
warnings if the attributes are accessed incorrectly. So we made
these warnings a debug option.
See Dmitry's comment ...
Basically my stance is that you should not lower yourself to the
level of others not getting the point of your technical solution
by making unelegant compromises, what
you should do is to bring them up to your level so they
understand that your solution is elegant.
Maybe a bit utopist I know...
What's the old saying? "I want to live in Theory. Everything is always
so nice there..." :-)
Anyway, see my reply to Dmitry a bit ago. These are no longer needed,
so we'll drop them.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/11/2012 01:24 AM, Dmitry Torokhov wrote:
On Thu, Oct 11, 2012 at 03:41:41AM +, Christopher Heiny wrote:
>Linus Walleij wrote:
> >On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny
wrote:
> >
> > >+#ifdef CONFIG_RMI4_DEBUG
> > >+/**
> > >+ * Utility routine to handle writes to read-only attributes. Hopefully
> > >+ * this will never happen, but if the user does something stupid, we
> > >don't
> > >+ * want to accept it quietly (which is what can happen if you just put
> > >NULL + * for the attribute's store function).
> > >+ */
> > >+static inline ssize_t rmi_store_error(struct device *dev,
> > >+ struct device_attribute *attr,
> > >+ const char *buf, size_t count)
> > >+{
> > >+ dev_warn(dev,
> > >+"WARNING: Attempt to write %d characters to read-only
> > >attribute %s.", + count, attr->attr.name);
> > >+ return -EPERM;
> > >+}
> >
> >Here it looks like you're hiding a lot of stuff that should be dev_warn()?
> >Consider my earlier point about dynamic debug.
>
>In previous patch submissions, we always used these warning functions.
>But in the feedback on those patches, we were asked to just make sysfs
>show/store NULL if the attribute is write/read only. However, during
>their development process, our customers want to see the warnings if
>the attributes are accessed incorrectly. So we made these warnings a
>debug option.
>
I think it is the case when customer is not always right. Given that
the attributes are created with S_IRUGO mask how will we even get these
methods to fire?
We were able to get those to fire in earlier kernels under some UIs
(such as Android). However, we no longer support those earlier version.
I have checked the behavior on up-to-date kernels and UI versions, and
everyone seems to handle this correctly. That means we can drop these
definitions entirely, so we'll do that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Tuesday, October 23, 2012 03:39:00 PM Christopher Heiny wrote:
> On 10/11/2012 01:20 AM, Dmitry Torokhov wrote:
> > On Fri, Oct 05, 2012 at 09:09:58PM -0700, Christopher Heiny wrote:
> >> +
> >> + int (*write_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
> >> + int len);
> >> + int (*read_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
> >> +int len);
> >> +
> >
> > If you declare your buffer as [const] void * instead of u8 * I think you
> > will be able to get rid of most of your unions.
>
> Good point. We'll explore that. If you see it in the next patch,
> you'll know it worked out.
>
> >> + * Helper fn to convert a byte array representing a 16 bit value in the
> >> RMI + * endian-ness to a 16-bit value in the native processor's specific
> >> endianness. + * We don't use ntohs/htons here because, well, we're not
> >> dealing with + * a pair of 16 bit values. Casting dest to u16* wouldn't
> >> work, because + * that would imply knowing the byte order of u16 in the
> >> first place. The + * same applies for using shifts and masks.
> >> + */
> >> +static inline u16 batohs(u8 *src)
> >> +{
> >> + return src[1] * 0x100 + src[0];
> >> +}
> >> +
> >> +/**
> >> + * Helper function to convert a 16 bit value (in host processor
> >> endianess) to + * a byte array in the RMI endianess for u16s. See above
> >> comment for + * why we dont us htons or something like that.
> >> + */
> >> +static inline void hstoba(u8 *dest, u16 src)
> >> +{
> >> + dest[0] = src % 0x100;
> >> + dest[1] = src / 0x100;
> >> +}
> >
> > These are not used anymore, right?
>
> There are function drivers that we chose not to include that depend on
> these. We'll drop these from rmi.h until we're ready to submit those
> other function drivers.
OK, in this case, like last time we discussed, your function drivers
should use cpu_to_le16() and le16_to_cpu() instead.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/11/2012 01:20 AM, Dmitry Torokhov wrote:
On Fri, Oct 05, 2012 at 09:09:58PM -0700, Christopher Heiny wrote:
+
+ int (*write_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
+ int len);
+ int (*read_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
+ int len);
+
If you declare your buffer as [const] void * instead of u8 * I think you
will be able to get rid of most of your unions.
Good point. We'll explore that. If you see it in the next patch,
you'll know it worked out.
+ * Helper fn to convert a byte array representing a 16 bit value in the RMI
+ * endian-ness to a 16-bit value in the native processor's specific endianness.
+ * We don't use ntohs/htons here because, well, we're not dealing with
+ * a pair of 16 bit values. Casting dest to u16* wouldn't work, because
+ * that would imply knowing the byte order of u16 in the first place. The
+ * same applies for using shifts and masks.
+ */
+static inline u16 batohs(u8 *src)
+{
+ return src[1] * 0x100 + src[0];
+}
+
+/**
+ * Helper function to convert a 16 bit value (in host processor endianess) to
+ * a byte array in the RMI endianess for u16s. See above comment for
+ * why we dont us htons or something like that.
+ */
+static inline void hstoba(u8 *dest, u16 src)
+{
+ dest[0] = src % 0x100;
+ dest[1] = src / 0x100;
+}
These are not used anymore, right?
There are function drivers that we chose not to include that depend on
these. We'll drop these from rmi.h until we're ready to submit those
other function drivers.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/11/2012 10:16 PM, Mark Brown wrote: On Thu, Oct 11, 2012 at 03:56:22AM +, Christopher Heiny wrote: Fix your mailer to word wrap within paragraphs. Sorry - I was on the road and had to use a web interface. It looked OK during composition. Is this better? >If this feature is a deal-breaker, we can take it out. In the absence >of a generic GPIO implementation for CS, though, I'd much rather leave >it in. Once generic GPIO CS arrives, we'll remove it pretty quickly. > Why not just implement this at an appropriate level in the SPI subsystem? One of the great things about Linux is that you can change the core code... At the moment, we're trying to keep our work focused on just the core RMI4 driver. It's evident that we've still got a bit to learn, and I'd be more comfortable keeping our learning experiences confined as they are now. However, a general capability in SPI core is a fine idea for future work, once we get the RMI4 driver ironed out. Can we agree to defer that for now? Thanks, Chris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/11/2012 10:16 PM, Mark Brown wrote: On Thu, Oct 11, 2012 at 03:56:22AM +, Christopher Heiny wrote: Fix your mailer to word wrap within paragraphs. Sorry - I was on the road and had to use a web interface. It looked OK during composition. Is this better? If this feature is a deal-breaker, we can take it out. In the absence of a generic GPIO implementation for CS, though, I'd much rather leave it in. Once generic GPIO CS arrives, we'll remove it pretty quickly. Why not just implement this at an appropriate level in the SPI subsystem? One of the great things about Linux is that you can change the core code... At the moment, we're trying to keep our work focused on just the core RMI4 driver. It's evident that we've still got a bit to learn, and I'd be more comfortable keeping our learning experiences confined as they are now. However, a general capability in SPI core is a fine idea for future work, once we get the RMI4 driver ironed out. Can we agree to defer that for now? Thanks, Chris -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/11/2012 01:20 AM, Dmitry Torokhov wrote:
On Fri, Oct 05, 2012 at 09:09:58PM -0700, Christopher Heiny wrote:
+
+ int (*write_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
+ int len);
+ int (*read_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
+ int len);
+
If you declare your buffer as [const] void * instead of u8 * I think you
will be able to get rid of most of your unions.
Good point. We'll explore that. If you see it in the next patch,
you'll know it worked out.
+ * Helper fn to convert a byte array representing a 16 bit value in the RMI
+ * endian-ness to a 16-bit value in the native processor's specific endianness.
+ * We don't use ntohs/htons here because, well, we're not dealing with
+ * a pair of 16 bit values. Casting dest to u16* wouldn't work, because
+ * that would imply knowing the byte order of u16 in the first place. The
+ * same applies for using shifts and masks.
+ */
+static inline u16 batohs(u8 *src)
+{
+ return src[1] * 0x100 + src[0];
+}
+
+/**
+ * Helper function to convert a 16 bit value (in host processor endianess) to
+ * a byte array in the RMI endianess for u16s. See above comment for
+ * why we dont us htons or something like that.
+ */
+static inline void hstoba(u8 *dest, u16 src)
+{
+ dest[0] = src % 0x100;
+ dest[1] = src / 0x100;
+}
These are not used anymore, right?
There are function drivers that we chose not to include that depend on
these. We'll drop these from rmi.h until we're ready to submit those
other function drivers.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Tuesday, October 23, 2012 03:39:00 PM Christopher Heiny wrote:
On 10/11/2012 01:20 AM, Dmitry Torokhov wrote:
On Fri, Oct 05, 2012 at 09:09:58PM -0700, Christopher Heiny wrote:
+
+ int (*write_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
+ int len);
+ int (*read_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
+int len);
+
If you declare your buffer as [const] void * instead of u8 * I think you
will be able to get rid of most of your unions.
Good point. We'll explore that. If you see it in the next patch,
you'll know it worked out.
+ * Helper fn to convert a byte array representing a 16 bit value in the
RMI + * endian-ness to a 16-bit value in the native processor's specific
endianness. + * We don't use ntohs/htons here because, well, we're not
dealing with + * a pair of 16 bit values. Casting dest to u16* wouldn't
work, because + * that would imply knowing the byte order of u16 in the
first place. The + * same applies for using shifts and masks.
+ */
+static inline u16 batohs(u8 *src)
+{
+ return src[1] * 0x100 + src[0];
+}
+
+/**
+ * Helper function to convert a 16 bit value (in host processor
endianess) to + * a byte array in the RMI endianess for u16s. See above
comment for + * why we dont us htons or something like that.
+ */
+static inline void hstoba(u8 *dest, u16 src)
+{
+ dest[0] = src % 0x100;
+ dest[1] = src / 0x100;
+}
These are not used anymore, right?
There are function drivers that we chose not to include that depend on
these. We'll drop these from rmi.h until we're ready to submit those
other function drivers.
OK, in this case, like last time we discussed, your function drivers
should use cpu_to_le16() and le16_to_cpu() instead.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/11/2012 01:24 AM, Dmitry Torokhov wrote:
On Thu, Oct 11, 2012 at 03:41:41AM +, Christopher Heiny wrote:
Linus Walleij wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heinyche...@synaptics.com
wrote:
+#ifdef CONFIG_RMI4_DEBUG
+/**
+ * Utility routine to handle writes to read-only attributes. Hopefully
+ * this will never happen, but if the user does something stupid, we
don't
+ * want to accept it quietly (which is what can happen if you just put
NULL + * for the attribute's store function).
+ */
+static inline ssize_t rmi_store_error(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ dev_warn(dev,
+WARNING: Attempt to write %d characters to read-only
attribute %s., + count, attr-attr.name);
+ return -EPERM;
+}
Here it looks like you're hiding a lot of stuff that should be dev_warn()?
Consider my earlier point about dynamic debug.
In previous patch submissions, we always used these warning functions.
But in the feedback on those patches, we were asked to just make sysfs
show/store NULL if the attribute is write/read only. However, during
their development process, our customers want to see the warnings if
the attributes are accessed incorrectly. So we made these warnings a
debug option.
I think it is the case when customer is not always right. Given that
the attributes are created with S_IRUGO mask how will we even get these
methods to fire?
We were able to get those to fire in earlier kernels under some UIs
(such as Android). However, we no longer support those earlier version.
I have checked the behavior on up-to-date kernels and UI versions, and
everyone seems to handle this correctly. That means we can drop these
definitions entirely, so we'll do that.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/11/2012 08:32 AM, Linus Walleij wrote:
On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny che...@synaptics.com wrote:
Linus Walleij wrote:
But please use arithmetic operators (I think I said this on the last
review):
dest[0] = src 0xFF;
dest[1] = src 8;
Doing it the above way makes artithmetic look like maths, and it isn't.
Besides it's done this way in most parts of the kernel and we're
familiar with it.
Yes, you mentioned it previously. I'm somewhat paranoid, though, and
don't trust the shift/mask method to work correctly on big-endian
machines. If the shifts can be relied on to behave (I'm guessing the
answer is yes, since you say this idiom is used widely in the
kernel), then I'll change it.
If the behaviour was not consistent across different endianness
it would not be part of the C language specification...
means shift left in the accumulator or whatever you have.
It will work the same no matter how bits are laid out in
memory.
OK, after reviewing the spec I'll accept that. We'll make the change.
+static inline ssize_t rmi_store_error(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ dev_warn(dev,
+WARNING: Attempt to write %d characters to read-only
attribute %s., + count, attr-attr.name);
+ return -EPERM;
+}
Here it looks like you're hiding a lot of stuff that should be dev_warn()?
Consider my earlier point about dynamic debug.
In previous patch submissions, we always used these warning functions.
But in the feedback on those patches, we were asked to just make
sysfs show/store NULL if the attribute is write/read only. However,
during their development process, our customers want to see the
warnings if the attributes are accessed incorrectly. So we made
these warnings a debug option.
See Dmitry's comment ...
Basically my stance is that you should not lower yourself to the
level of others not getting the point of your technical solution
by making unelegant compromises, what
you should do is to bring them up to your level so they
understand that your solution is elegant.
Maybe a bit utopist I know...
What's the old saying? I want to live in Theory. Everything is always
so nice there... :-)
Anyway, see my reply to Dmitry a bit ago. These are no longer needed,
so we'll drop them.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On 10/15/2012 11:26 PM, Mark Brown wrote: On Thu, Oct 11, 2012 at 05:32:59PM +0200, Linus Walleij wrote: On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny che...@synaptics.com wrote: In previous patch submissions, we always used these warning functions. But in the feedback on those patches, we were asked to just make sysfs show/store NULL if the attribute is write/read only. However, during their development process, our customers want to see the warnings if the attributes are accessed incorrectly. So we made these warnings a debug option. Basically my stance is that you should not lower yourself to the level of others not getting the point of your technical solution by making unelegant compromises, what you should do is to bring them up to your level so they understand that your solution is elegant. It seems like what you really want to do is add a debug feature to sysfs which will optionally complain loudly at bad accesses; obviously it's not something that should be there all the time as running then handling an error is a perfectly legitimate thing to do. As with the /CS handling it'd mean it was handled at an appropriate level and could be reused elsewhere (it might also help make it clear to your customers why this is generally bad form). See my reply to Dmitry of a bit ago. These are no longer needed, and we'll be dropping them. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 05:32:59PM +0200, Linus Walleij wrote: > On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny > wrote: > > In previous patch submissions, we always used these warning functions. > > But in the feedback on those patches, we were asked to just make > > sysfs show/store NULL if the attribute is write/read only. However, > > during their development process, our customers want to see the > > warnings if the attributes are accessed incorrectly. So we made > > these warnings a debug option. > Basically my stance is that you should not lower yourself to the > level of others not getting the point of your technical solution > by making unelegant compromises, what > you should do is to bring them up to your level so they > understand that your solution is elegant. It seems like what you really want to do is add a debug feature to sysfs which will optionally complain loudly at bad accesses; obviously it's not something that should be there all the time as running then handling an error is a perfectly legitimate thing to do. As with the /CS handling it'd mean it was handled at an appropriate level and could be reused elsewhere (it might also help make it clear to your customers why this is generally bad form). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 05:32:59PM +0200, Linus Walleij wrote: On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny che...@synaptics.com wrote: In previous patch submissions, we always used these warning functions. But in the feedback on those patches, we were asked to just make sysfs show/store NULL if the attribute is write/read only. However, during their development process, our customers want to see the warnings if the attributes are accessed incorrectly. So we made these warnings a debug option. Basically my stance is that you should not lower yourself to the level of others not getting the point of your technical solution by making unelegant compromises, what you should do is to bring them up to your level so they understand that your solution is elegant. It seems like what you really want to do is add a debug feature to sysfs which will optionally complain loudly at bad accesses; obviously it's not something that should be there all the time as running then handling an error is a perfectly legitimate thing to do. As with the /CS handling it'd mean it was handled at an appropriate level and could be reused elsewhere (it might also help make it clear to your customers why this is generally bad form). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 03:56:22AM +, Christopher Heiny wrote: Fix your mailer to word wrap within paragraphs. > If this feature is a deal-breaker, we can take it out. In the absence > of a generic GPIO implementation for CS, though, I'd much rather leave > it in. Once generic GPIO CS arrives, we'll remove it pretty quickly. Why not just implement this at an appropriate level in the SPI subsystem? One of the great things about Linux is that you can change the core code... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny wrote:
> Linus Walleij wrote:
>> But please use arithmetic operators (I think I said this on the last
>> review):
>>
>> dest[0] = src & 0xFF;
>> dest[1] = src >> 8;
>>
>> Doing it the above way makes artithmetic look like maths, and it isn't.
>> Besides it's done this way in most parts of the kernel and we're
>> familiar with it.
>
> Yes, you mentioned it previously. I'm somewhat paranoid, though, and
> don't trust the shift/mask method to work correctly on big-endian
> machines. If the shifts can be relied on to behave (I'm guessing the
> answer is "yes", since you say this idiom is used widely in the
> kernel), then I'll change it.
If the behaviour was not consistent across different endianness
it would not be part of the C language specification...
<< means shift left in the accumulator or whatever you have.
It will work the same no matter how bits are laid out in
memory.
>> > +static inline ssize_t rmi_store_error(struct device *dev,
>> > + struct device_attribute *attr,
>> > + const char *buf, size_t count)
>> > +{
>> > + dev_warn(dev,
>> > +"WARNING: Attempt to write %d characters to read-only
>> > attribute %s.", + count, attr->attr.name);
>> > + return -EPERM;
>> > +}
>>
>> Here it looks like you're hiding a lot of stuff that should be dev_warn()?
>> Consider my earlier point about dynamic debug.
>
> In previous patch submissions, we always used these warning functions.
> But in the feedback on those patches, we were asked to just make
> sysfs show/store NULL if the attribute is write/read only. However,
> during their development process, our customers want to see the
> warnings if the attributes are accessed incorrectly. So we made
> these warnings a debug option.
See Dmitry's comment ...
Basically my stance is that you should not lower yourself to the
level of others not getting the point of your technical solution
by making unelegant compromises, what
you should do is to bring them up to your level so they
understand that your solution is elegant.
Maybe a bit utopist I know...
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny wrote: > Linus Walleij wrote: >> On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny >> wrote: >> > + chargerinput ... (rw) User space programs can use this to tell >> > the + sensor that the system is plugged into an external >> > power + source (as opposed to running on >> > batteries). This allows + the sensor firmware to make >> > necessary adjustments for the + current capacitence >> > regime. Write 1 to this when the + system is using >> > external power, write 0 to this when the + system is >> > running on batteries. See spec for full details. >> I remember discussing in-kernel notifiers for this. I don't >> really see the point in tunnelling a notification from the drivers/power >> subsystem to the drivers/input subsystem through userspace for >> no good. >> >> It's no blocker though, I don't expect you to fix this as part of >> this driver submission. >> >> Maybe Anton can comment? > > Hmmm. I agree that it'd be good to avoid looping through userspace. But > > I found ways to notfiy the kernel that the charger is plugged/unplugged, > but that's only useful if you're a battery charger device driver. I also > found ways for userspace to get notification of charger events. I > didn't spot any way for in-kernel drivers to get notification of such > events. Perhaps I'm not looking the right places, though - can you > provide a pointer? So my point was that there is indeed no such in-kernel notification mechanism. Maybe something engineered similar to this: http://www.spinics.net/lists/lm-sensors/msg33834.html Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 03:41:41AM +, Christopher Heiny wrote:
> Linus Walleij wrote:
> > On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny
> > wrote:
> >
> > > +#ifdef CONFIG_RMI4_DEBUG
> > > +/**
> > > + * Utility routine to handle writes to read-only attributes. Hopefully
> > > + * this will never happen, but if the user does something stupid, we
> > > don't
> > > + * want to accept it quietly (which is what can happen if you just put
> > > NULL + * for the attribute's store function).
> > > + */
> > > +static inline ssize_t rmi_store_error(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + dev_warn(dev,
> > > +"WARNING: Attempt to write %d characters to read-only
> > > attribute %s.", + count, attr->attr.name);
> > > + return -EPERM;
> > > +}
> >
> > Here it looks like you're hiding a lot of stuff that should be dev_warn()?
> > Consider my earlier point about dynamic debug.
>
> In previous patch submissions, we always used these warning functions.
> But in the feedback on those patches, we were asked to just make sysfs
> show/store NULL if the attribute is write/read only. However, during
> their development process, our customers want to see the warnings if
> the attributes are accessed incorrectly. So we made these warnings a
> debug option.
I think it is the case when customer is not always right. Given that
the attributes are created with S_IRUGO mask how will we even get these
methods to fire?
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Fri, Oct 05, 2012 at 09:09:58PM -0700, Christopher Heiny wrote:
> +
> + int (*write_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
> +int len);
> + int (*read_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
> + int len);
> +
If you declare your buffer as [const] void * instead of u8 * I think you
will be able to get rid of most of your unions.
> +
> +/**
> + * Helper fn to convert a byte array representing a 16 bit value in the RMI
> + * endian-ness to a 16-bit value in the native processor's specific
> endianness.
> + * We don't use ntohs/htons here because, well, we're not dealing with
> + * a pair of 16 bit values. Casting dest to u16* wouldn't work, because
> + * that would imply knowing the byte order of u16 in the first place. The
> + * same applies for using shifts and masks.
> + */
> +static inline u16 batohs(u8 *src)
> +{
> + return src[1] * 0x100 + src[0];
> +}
> +
> +/**
> + * Helper function to convert a 16 bit value (in host processor endianess) to
> + * a byte array in the RMI endianess for u16s. See above comment for
> + * why we dont us htons or something like that.
> + */
> +static inline void hstoba(u8 *dest, u16 src)
> +{
> + dest[0] = src % 0x100;
> + dest[1] = src / 0x100;
> +}
These are not used anymore, right?
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Fri, Oct 05, 2012 at 09:09:58PM -0700, Christopher Heiny wrote:
+
+ int (*write_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
+int len);
+ int (*read_block)(struct rmi_phys_device *phys, u16 addr, u8 *buf,
+ int len);
+
If you declare your buffer as [const] void * instead of u8 * I think you
will be able to get rid of most of your unions.
+
+/**
+ * Helper fn to convert a byte array representing a 16 bit value in the RMI
+ * endian-ness to a 16-bit value in the native processor's specific
endianness.
+ * We don't use ntohs/htons here because, well, we're not dealing with
+ * a pair of 16 bit values. Casting dest to u16* wouldn't work, because
+ * that would imply knowing the byte order of u16 in the first place. The
+ * same applies for using shifts and masks.
+ */
+static inline u16 batohs(u8 *src)
+{
+ return src[1] * 0x100 + src[0];
+}
+
+/**
+ * Helper function to convert a 16 bit value (in host processor endianess) to
+ * a byte array in the RMI endianess for u16s. See above comment for
+ * why we dont us htons or something like that.
+ */
+static inline void hstoba(u8 *dest, u16 src)
+{
+ dest[0] = src % 0x100;
+ dest[1] = src / 0x100;
+}
These are not used anymore, right?
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 03:41:41AM +, Christopher Heiny wrote:
Linus Walleij wrote:
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com
wrote:
+#ifdef CONFIG_RMI4_DEBUG
+/**
+ * Utility routine to handle writes to read-only attributes. Hopefully
+ * this will never happen, but if the user does something stupid, we
don't
+ * want to accept it quietly (which is what can happen if you just put
NULL + * for the attribute's store function).
+ */
+static inline ssize_t rmi_store_error(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ dev_warn(dev,
+WARNING: Attempt to write %d characters to read-only
attribute %s., + count, attr-attr.name);
+ return -EPERM;
+}
Here it looks like you're hiding a lot of stuff that should be dev_warn()?
Consider my earlier point about dynamic debug.
In previous patch submissions, we always used these warning functions.
But in the feedback on those patches, we were asked to just make sysfs
show/store NULL if the attribute is write/read only. However, during
their development process, our customers want to see the warnings if
the attributes are accessed incorrectly. So we made these warnings a
debug option.
I think it is the case when customer is not always right. Given that
the attributes are created with S_IRUGO mask how will we even get these
methods to fire?
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny che...@synaptics.com wrote: Linus Walleij wrote: On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com wrote: + chargerinput ... (rw) User space programs can use this to tell the + sensor that the system is plugged into an external power + source (as opposed to running on batteries). This allows + the sensor firmware to make necessary adjustments for the + current capacitence regime. Write 1 to this when the + system is using external power, write 0 to this when the + system is running on batteries. See spec for full details. I remember discussing in-kernel notifiers for this. I don't really see the point in tunnelling a notification from the drivers/power subsystem to the drivers/input subsystem through userspace for no good. It's no blocker though, I don't expect you to fix this as part of this driver submission. Maybe Anton can comment? Hmmm. I agree that it'd be good to avoid looping through userspace. But I found ways to notfiy the kernel that the charger is plugged/unplugged, but that's only useful if you're a battery charger device driver. I also found ways for userspace to get notification of charger events. I didn't spot any way for in-kernel drivers to get notification of such events. Perhaps I'm not looking the right places, though - can you provide a pointer? So my point was that there is indeed no such in-kernel notification mechanism. Maybe something engineered similar to this: http://www.spinics.net/lists/lm-sensors/msg33834.html Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny che...@synaptics.com wrote:
Linus Walleij wrote:
But please use arithmetic operators (I think I said this on the last
review):
dest[0] = src 0xFF;
dest[1] = src 8;
Doing it the above way makes artithmetic look like maths, and it isn't.
Besides it's done this way in most parts of the kernel and we're
familiar with it.
Yes, you mentioned it previously. I'm somewhat paranoid, though, and
don't trust the shift/mask method to work correctly on big-endian
machines. If the shifts can be relied on to behave (I'm guessing the
answer is yes, since you say this idiom is used widely in the
kernel), then I'll change it.
If the behaviour was not consistent across different endianness
it would not be part of the C language specification...
means shift left in the accumulator or whatever you have.
It will work the same no matter how bits are laid out in
memory.
+static inline ssize_t rmi_store_error(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ dev_warn(dev,
+WARNING: Attempt to write %d characters to read-only
attribute %s., + count, attr-attr.name);
+ return -EPERM;
+}
Here it looks like you're hiding a lot of stuff that should be dev_warn()?
Consider my earlier point about dynamic debug.
In previous patch submissions, we always used these warning functions.
But in the feedback on those patches, we were asked to just make
sysfs show/store NULL if the attribute is write/read only. However,
during their development process, our customers want to see the
warnings if the attributes are accessed incorrectly. So we made
these warnings a debug option.
See Dmitry's comment ...
Basically my stance is that you should not lower yourself to the
level of others not getting the point of your technical solution
by making unelegant compromises, what
you should do is to bring them up to your level so they
understand that your solution is elegant.
Maybe a bit utopist I know...
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Thu, Oct 11, 2012 at 03:56:22AM +, Christopher Heiny wrote: Fix your mailer to word wrap within paragraphs. If this feature is a deal-breaker, we can take it out. In the absence of a generic GPIO implementation for CS, though, I'd much rather leave it in. Once generic GPIO CS arrives, we'll remove it pretty quickly. Why not just implement this at an appropriate level in the SPI subsystem? One of the great things about Linux is that you can change the core code... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC PATCH 01/06] input/rmi4: Public header and documentation
Mark Brown wrote:
> On Tue, Oct 09, 2012 at 09:43:13AM +0200, Linus Walleij wrote:
> > On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny
> > wrote:
> > > + * @cs_assert - For systems where the SPI subsystem does not control
> > > the CS/SSB + * line, or where such control is broken, you can provide a
> > > custom routine to + * handle a GPIO as CS/SSB. This routine will be
> > > called at the beginning and + * end of each SPI transaction. The RMI
> > > SPI implementation will wait + * pre_delay_us after this routine
> > > returns before starting the SPI transfer; + * and post_delay_us after
> > > completion of the SPI transfer(s) before calling it + * with
> > > assert==FALSE.
> >
> > Hm hm, can you describe the case where this happens?
> >
> > Usually we don't avoid fixes for broken drivers by duct-taping
> > solutions into other drivers, instead we fix the SPI driver.
> >
> > I can think of systems where CS is asserted not by using
> > GPIO but by poking some special register for example, which
> > is a valid reason for including this, but working around broken
> > SPI drivers is not a valid reason to include this.
> >
> > (Paging Mark about it.)
>
> Yeah, this seems silly - by this logic we'd have to go round implementing
> manual /CS control in every single SPI client driver which isn't
> terribly sensible. The driver should just assume that the SPI
> controller does what it's told. As you say if there's an issue the
> relevant controller driver should take care of things.
>
> We should also have generic support in the SPI framework for GPIO based
> /CS, there's enough drivers open coding this already either due to
> hardware limitations or to support extra chip selects.
>
> The ability of SPI hardware and driver authors to get /CS right is
> pretty depressing :/
You will get no argument at all from me on that point. I'll even add board
layout engineers to the list ("it wasn't convenient to run CS, so we just used
a different pin. You can just mux it, right?"). Basically this feature exists
to help get prototype systems up and running while the SPI
hardware/driver/layout matures.
If this feature is a deal-breaker, we can take it out. In the absence of a
generic GPIO implementation for CS, though, I'd much rather leave it in. Once
generic GPIO CS arrives, we'll remove it pretty quickly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC PATCH 01/06] input/rmi4: Public header and documentation
Linus Walleij wrote: > On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny > wrote: > > As requested in the feedback from the previous patch, we've documented the > > debugfs and sysfs attributes in files in > > Documentation/ABI/testing. There's two files, one for debugfs and one > > for sysfs. > > This is a massive improvement! Atleast as far as I've read... If you fix the > below remarks I think I'm ready to accept this file, but that's just me and > doesn't say anything about what Dmitry et al will comment on... Thanks! See my comments below. > > (...) > > > + The RMI4 driver implementation exposes a set of informational and > > control + parameters via debugs. These parameters are those that > > typically are only > s/debugs/debugfs > > (...) > > > + comms_debug - (rw) Write 1 to this dump information about > > register + reads and writes to the console. Write 0 to > > this to turn + this feature off. WARNING: Imposes a > > major performance + penalty when turned on. > > + irq_debug - (rw) Write 1 to this dump information about > > interrupts + to the console. Write 0 to this to turn > > this feature off. + WARNIG: Imposes a major performance > > penalty when turned on. > Hm. Usually we control dynamic debug prints by standard kernel > frameworks, can you tell what is wrong with this and why you need > a custom mechanism? See the following: > Documentation/dynamic-debug-howto.txt > http://lwn.net/Articles/434833/ The current arrangement was arrived at after some discussion with customers. Originally we went with the Kconfig based approach you suggested in August. However, the response from our guinea pigs, um, very helpful test customers, was "AAAggh! Too complicated and too static!!" As a result we explored alternatives. The dynamic debug interface was considered, but it is usually disabled in our customer's kernel configurations, even during development. In the end, we arrived at some simple debugfs on/off switches for the more verbose features (like comms_debug and irq_debug, above). If this is a deal-breaker, I can go back to the customers and see if they are willing to consider enabling dynamic debug during prototyping and development. > > (...) > > > +++ b/Documentation/ABI/testing/sysfs-rmi4 > > (...) > > > + chargerinput ... (rw) User space programs can use this to tell > > the + sensor that the system is plugged into an external > > power + source (as opposed to running on > > batteries). This allows + the sensor firmware to make > > necessary adjustments for the + current capacitence > > regime. Write 1 to this when the + system is using > > external power, write 0 to this when the + system is > > running on batteries. See spec for full details. > I remember discussing in-kernel notifiers for this. I don't > really see the point in tunnelling a notification from the drivers/power > subsystem to the drivers/input subsystem through userspace for > no good. > > It's no blocker though, I don't expect you to fix this as part of > this driver submission. > > Maybe Anton can comment? Hmmm. I agree that it'd be good to avoid looping through userspace. But I found ways to notfiy the kernel that the charger is plugged/unplugged, but that's only useful if you're a battery charger device driver. I also found ways for userspace to get notification of charger events. I didn't spot any way for in-kernel drivers to get notification of such events. Perhaps I'm not looking the right places, though - can you provide a pointer? > > (...) > > > + interrupt_enable ... (ro) This represents the current RMI4 > > interrupt + mask (F01_RMI_Ctrl1 registers). See spec > > for full details. > What does the userspace have to do with this stuff? Seems way > too low-level, but whatever. It's primarily used in hardware prototyping and bring up. Perhaps it belongs in debugfs in that case. > > (...) > > > + sleepmode ... (rw) Controls power management on the > > device. Writing + 0 to this parameter puts the device > > into its normal operating + mode. Writing 1 to this > > parameter fully disables touch + sensors and similar > > inputs - no touch data will be reported + from the > > device in this mode. Writing 2 or 3 to this device > > + may or may not have an effect, depending on the > > particular + device - see the product specification for > > your sensor for + details. > > Usually power management is controlled from kernelspace, but no > big deal, maybe userspace knows even more details in some > cases. Well, in some cases userspace does think it knows more :-). This one should
RE: [RFC PATCH 01/06] input/rmi4: Public header and documentation
Linus Walleij wrote: On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com wrote: As requested in the feedback from the previous patch, we've documented the debugfs and sysfs attributes in files in Documentation/ABI/testing. There's two files, one for debugfs and one for sysfs. This is a massive improvement! Atleast as far as I've read... If you fix the below remarks I think I'm ready to accept this file, but that's just me and doesn't say anything about what Dmitry et al will comment on... Thanks! See my comments below. (...) + The RMI4 driver implementation exposes a set of informational and control + parameters via debugs. These parameters are those that typically are only s/debugs/debugfs (...) + comms_debug - (rw) Write 1 to this dump information about register + reads and writes to the console. Write 0 to this to turn + this feature off. WARNING: Imposes a major performance + penalty when turned on. + irq_debug - (rw) Write 1 to this dump information about interrupts + to the console. Write 0 to this to turn this feature off. + WARNIG: Imposes a major performance penalty when turned on. Hm. Usually we control dynamic debug prints by standard kernel frameworks, can you tell what is wrong with this and why you need a custom mechanism? See the following: Documentation/dynamic-debug-howto.txt http://lwn.net/Articles/434833/ The current arrangement was arrived at after some discussion with customers. Originally we went with the Kconfig based approach you suggested in August. However, the response from our guinea pigs, um, very helpful test customers, was AAAggh! Too complicated and too static!! As a result we explored alternatives. The dynamic debug interface was considered, but it is usually disabled in our customer's kernel configurations, even during development. In the end, we arrived at some simple debugfs on/off switches for the more verbose features (like comms_debug and irq_debug, above). If this is a deal-breaker, I can go back to the customers and see if they are willing to consider enabling dynamic debug during prototyping and development. (...) +++ b/Documentation/ABI/testing/sysfs-rmi4 (...) + chargerinput ... (rw) User space programs can use this to tell the + sensor that the system is plugged into an external power + source (as opposed to running on batteries). This allows + the sensor firmware to make necessary adjustments for the + current capacitence regime. Write 1 to this when the + system is using external power, write 0 to this when the + system is running on batteries. See spec for full details. I remember discussing in-kernel notifiers for this. I don't really see the point in tunnelling a notification from the drivers/power subsystem to the drivers/input subsystem through userspace for no good. It's no blocker though, I don't expect you to fix this as part of this driver submission. Maybe Anton can comment? Hmmm. I agree that it'd be good to avoid looping through userspace. But I found ways to notfiy the kernel that the charger is plugged/unplugged, but that's only useful if you're a battery charger device driver. I also found ways for userspace to get notification of charger events. I didn't spot any way for in-kernel drivers to get notification of such events. Perhaps I'm not looking the right places, though - can you provide a pointer? (...) + interrupt_enable ... (ro) This represents the current RMI4 interrupt + mask (F01_RMI_Ctrl1 registers). See spec for full details. What does the userspace have to do with this stuff? Seems way too low-level, but whatever. It's primarily used in hardware prototyping and bring up. Perhaps it belongs in debugfs in that case. (...) + sleepmode ... (rw) Controls power management on the device. Writing + 0 to this parameter puts the device into its normal operating + mode. Writing 1 to this parameter fully disables touch + sensors and similar inputs - no touch data will be reported + from the device in this mode. Writing 2 or 3 to this device + may or may not have an effect, depending on the particular + device - see the product specification for your sensor for + details. Usually power management is controlled from kernelspace, but no big deal, maybe userspace knows even more details in some cases. Well, in some cases userspace does think it knows more :-). This one should definitely stay in sysfs. + unconfigured ... (ro) This is the opposite of the configured
RE: [RFC PATCH 01/06] input/rmi4: Public header and documentation
Mark Brown wrote: On Tue, Oct 09, 2012 at 09:43:13AM +0200, Linus Walleij wrote: On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com wrote: + * @cs_assert - For systems where the SPI subsystem does not control the CS/SSB + * line, or where such control is broken, you can provide a custom routine to + * handle a GPIO as CS/SSB. This routine will be called at the beginning and + * end of each SPI transaction. The RMI SPI implementation will wait + * pre_delay_us after this routine returns before starting the SPI transfer; + * and post_delay_us after completion of the SPI transfer(s) before calling it + * with assert==FALSE. Hm hm, can you describe the case where this happens? Usually we don't avoid fixes for broken drivers by duct-taping solutions into other drivers, instead we fix the SPI driver. I can think of systems where CS is asserted not by using GPIO but by poking some special register for example, which is a valid reason for including this, but working around broken SPI drivers is not a valid reason to include this. (Paging Mark about it.) Yeah, this seems silly - by this logic we'd have to go round implementing manual /CS control in every single SPI client driver which isn't terribly sensible. The driver should just assume that the SPI controller does what it's told. As you say if there's an issue the relevant controller driver should take care of things. We should also have generic support in the SPI framework for GPIO based /CS, there's enough drivers open coding this already either due to hardware limitations or to support extra chip selects. The ability of SPI hardware and driver authors to get /CS right is pretty depressing :/ You will get no argument at all from me on that point. I'll even add board layout engineers to the list (it wasn't convenient to run CS, so we just used a different pin. You can just mux it, right?). Basically this feature exists to help get prototype systems up and running while the SPI hardware/driver/layout matures. If this feature is a deal-breaker, we can take it out. In the absence of a generic GPIO implementation for CS, though, I'd much rather leave it in. Once generic GPIO CS arrives, we'll remove it pretty quickly. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Tue, Oct 09, 2012 at 09:43:13AM +0200, Linus Walleij wrote: > On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny > wrote: > > + * @cs_assert - For systems where the SPI subsystem does not control the > > CS/SSB > > + * line, or where such control is broken, you can provide a custom routine > > to > > + * handle a GPIO as CS/SSB. This routine will be called at the beginning > > and > > + * end of each SPI transaction. The RMI SPI implementation will wait > > + * pre_delay_us after this routine returns before starting the SPI > > transfer; > > + * and post_delay_us after completion of the SPI transfer(s) before > > calling it > > + * with assert==FALSE. > Hm hm, can you describe the case where this happens? > Usually we don't avoid fixes for broken drivers by duct-taping > solutions into other drivers, instead we fix the SPI driver. > I can think of systems where CS is asserted not by using > GPIO but by poking some special register for example, which > is a valid reason for including this, but working around broken > SPI drivers is not a valid reason to include this. > (Paging Mark about it.) Yeah, this seems silly - by this logic we'd have to go round implementing manual /CS control in every single SPI client driver which isn't terribly sensible. The driver should just assume that the SPI controller does what it's told. As you say if there's an issue the relevant controller driver should take care of things. We should also have generic support in the SPI framework for GPIO based /CS, there's enough drivers open coding this already either due to hardware limitations or to support extra chip selects. The ability of SPI hardware and driver authors to get /CS right is pretty depressing :/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny wrote: > As requested in the feedback from the previous patch, we've documented the > debugfs and sysfs attributes in files in Documentation/ABI/testing. There's > two files, one for debugfs and one for sysfs. This is a massive improvement! Atleast as far as I've read... If you fix the below remarks I think I'm ready to accept this file, but that's just me and doesn't say anything about what Dmitry et al will comment on... (...) > + The RMI4 driver implementation exposes a set of informational and control > + parameters via debugs. These parameters are those that typically are only s/debugs/debugfs (...) > + comms_debug - (rw) Write 1 to this dump information about register > + reads and writes to the console. Write 0 to this to turn > + this feature off. WARNING: Imposes a major performance > + penalty when turned on. > + irq_debug - (rw) Write 1 to this dump information about interrupts > + to the console. Write 0 to this to turn this feature off. > + WARNIG: Imposes a major performance penalty when turned on. Hm. Usually we control dynamic debug prints by standard kernel frameworks, can you tell what is wrong with this and why you need a custom mechanism? See the following: Documentation/dynamic-debug-howto.txt http://lwn.net/Articles/434833/ (...) > +++ b/Documentation/ABI/testing/sysfs-rmi4 (...) > + chargerinput ... (rw) User space programs can use this to tell the > + sensor that the system is plugged into an external power > + source (as opposed to running on batteries). This allows > + the sensor firmware to make necessary adjustments for the > + current capacitence regime. Write 1 to this when the > + system is using external power, write 0 to this when the > + system is running on batteries. See spec for full details. I remember discussing in-kernel notifiers for this. I don't really see the point in tunnelling a notification from the drivers/power subsystem to the drivers/input subsystem through userspace for no good. It's no blocker though, I don't expect you to fix this as part of this driver submission. Maybe Anton can comment? (...) > + interrupt_enable ... (ro) This represents the current RMI4 > interrupt > + mask (F01_RMI_Ctrl1 registers). See spec for full details. What does the userspace have to do with this stuff? Seems way too low-level, but whatever. (...) > + sleepmode ... (rw) Controls power management on the device. > Writing > + 0 to this parameter puts the device into its normal > operating > + mode. Writing 1 to this parameter fully disables touch > + sensors and similar inputs - no touch data will be reported > + from the device in this mode. Writing 2 or 3 to this > device > + may or may not have an effect, depending on the particular > + device - see the product specification for your sensor for > + details. Usually power management is controlled from kernelspace, but no big deal, maybe userspace knows even more details in some cases. > + unconfigured ... (ro) This is the opposite of the configured bit, > + described above. So why is it needed? Isn't it implicit from the "configured" property if this is 0 then it's unconfigured? Seems superfluous. (...) > +++ b/include/linux/rmi.h (...) > +#ifdef CONFIG_RMI4_DEBUG > +#include > +#endif Don't include it conditionally, always just include it whether you use it or not. It doesn't hurt, and doesn't cause compile problems. (...) > +/** > + * struct rmi_device_platform_data_spi - provides paramters used in SPI s/paramters/parameters/ > + * communitcations. All Synaptics SPI products support a standard SPI s/communitcations/communications > + * @cs_assert - For systems where the SPI subsystem does not control the > CS/SSB > + * line, or where such control is broken, you can provide a custom routine to > + * handle a GPIO as CS/SSB. This routine will be called at the beginning and > + * end of each SPI transaction. The RMI SPI implementation will wait > + * pre_delay_us after this routine returns before starting the SPI transfer; > + * and post_delay_us after completion of the SPI transfer(s) before calling > it > + * with assert==FALSE. Hm hm, can you describe the case where this happens? Usually we don't avoid fixes for broken drivers by duct-taping solutions into other drivers, instead we fix the SPI driver. I can think of systems where CS is asserted not by using GPIO but by poking some special register for example, which is a valid reason for including this, but working around broken SPI drivers is not a valid reason to include this.
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com wrote: As requested in the feedback from the previous patch, we've documented the debugfs and sysfs attributes in files in Documentation/ABI/testing. There's two files, one for debugfs and one for sysfs. This is a massive improvement! Atleast as far as I've read... If you fix the below remarks I think I'm ready to accept this file, but that's just me and doesn't say anything about what Dmitry et al will comment on... (...) + The RMI4 driver implementation exposes a set of informational and control + parameters via debugs. These parameters are those that typically are only s/debugs/debugfs (...) + comms_debug - (rw) Write 1 to this dump information about register + reads and writes to the console. Write 0 to this to turn + this feature off. WARNING: Imposes a major performance + penalty when turned on. + irq_debug - (rw) Write 1 to this dump information about interrupts + to the console. Write 0 to this to turn this feature off. + WARNIG: Imposes a major performance penalty when turned on. Hm. Usually we control dynamic debug prints by standard kernel frameworks, can you tell what is wrong with this and why you need a custom mechanism? See the following: Documentation/dynamic-debug-howto.txt http://lwn.net/Articles/434833/ (...) +++ b/Documentation/ABI/testing/sysfs-rmi4 (...) + chargerinput ... (rw) User space programs can use this to tell the + sensor that the system is plugged into an external power + source (as opposed to running on batteries). This allows + the sensor firmware to make necessary adjustments for the + current capacitence regime. Write 1 to this when the + system is using external power, write 0 to this when the + system is running on batteries. See spec for full details. I remember discussing in-kernel notifiers for this. I don't really see the point in tunnelling a notification from the drivers/power subsystem to the drivers/input subsystem through userspace for no good. It's no blocker though, I don't expect you to fix this as part of this driver submission. Maybe Anton can comment? (...) + interrupt_enable ... (ro) This represents the current RMI4 interrupt + mask (F01_RMI_Ctrl1 registers). See spec for full details. What does the userspace have to do with this stuff? Seems way too low-level, but whatever. (...) + sleepmode ... (rw) Controls power management on the device. Writing + 0 to this parameter puts the device into its normal operating + mode. Writing 1 to this parameter fully disables touch + sensors and similar inputs - no touch data will be reported + from the device in this mode. Writing 2 or 3 to this device + may or may not have an effect, depending on the particular + device - see the product specification for your sensor for + details. Usually power management is controlled from kernelspace, but no big deal, maybe userspace knows even more details in some cases. + unconfigured ... (ro) This is the opposite of the configured bit, + described above. So why is it needed? Isn't it implicit from the configured property if this is 0 then it's unconfigured? Seems superfluous. (...) +++ b/include/linux/rmi.h (...) +#ifdef CONFIG_RMI4_DEBUG +#include linux/debugfs.h +#endif Don't include it conditionally, always just include it whether you use it or not. It doesn't hurt, and doesn't cause compile problems. (...) +/** + * struct rmi_device_platform_data_spi - provides paramters used in SPI s/paramters/parameters/ + * communitcations. All Synaptics SPI products support a standard SPI s/communitcations/communications + * @cs_assert - For systems where the SPI subsystem does not control the CS/SSB + * line, or where such control is broken, you can provide a custom routine to + * handle a GPIO as CS/SSB. This routine will be called at the beginning and + * end of each SPI transaction. The RMI SPI implementation will wait + * pre_delay_us after this routine returns before starting the SPI transfer; + * and post_delay_us after completion of the SPI transfer(s) before calling it + * with assert==FALSE. Hm hm, can you describe the case where this happens? Usually we don't avoid fixes for broken drivers by duct-taping solutions into other drivers, instead we fix the SPI driver. I can think of systems where CS is asserted not by using GPIO but by poking some special register for example, which is a valid reason for including this, but working around broken SPI drivers is not a valid reason to include this. (Paging Mark about it.)
Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation
On Tue, Oct 09, 2012 at 09:43:13AM +0200, Linus Walleij wrote: On Sat, Oct 6, 2012 at 6:09 AM, Christopher Heiny che...@synaptics.com wrote: + * @cs_assert - For systems where the SPI subsystem does not control the CS/SSB + * line, or where such control is broken, you can provide a custom routine to + * handle a GPIO as CS/SSB. This routine will be called at the beginning and + * end of each SPI transaction. The RMI SPI implementation will wait + * pre_delay_us after this routine returns before starting the SPI transfer; + * and post_delay_us after completion of the SPI transfer(s) before calling it + * with assert==FALSE. Hm hm, can you describe the case where this happens? Usually we don't avoid fixes for broken drivers by duct-taping solutions into other drivers, instead we fix the SPI driver. I can think of systems where CS is asserted not by using GPIO but by poking some special register for example, which is a valid reason for including this, but working around broken SPI drivers is not a valid reason to include this. (Paging Mark about it.) Yeah, this seems silly - by this logic we'd have to go round implementing manual /CS control in every single SPI client driver which isn't terribly sensible. The driver should just assume that the SPI controller does what it's told. As you say if there's an issue the relevant controller driver should take care of things. We should also have generic support in the SPI framework for GPIO based /CS, there's enough drivers open coding this already either due to hardware limitations or to support extra chip selects. The ability of SPI hardware and driver authors to get /CS right is pretty depressing :/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 01/06] input/rmi4: Public header and documentation
As requested in the feedback from the previous patch, we've documented the debugfs and sysfs attributes in files in Documentation/ABI/testing. There's two files, one for debugfs and one for sysfs. Signed-off-by: Christopher Heiny Cc: Dmitry Torokhov Cc: Linus Walleij Cc: Naveen Kumar Gaddipati Cc: Joeri de Gram --- Documentation/ABI/testing/debugfs-rmi4 | 99 + Documentation/ABI/testing/sysfs-rmi4 | 103 + include/linux/rmi.h| 696 3 files changed, 898 insertions(+), 0 deletions(-) diff --git a/Documentation/ABI/testing/debugfs-rmi4 b/Documentation/ABI/testing/debugfs-rmi4 new file mode 100644 index 000..cf1aa2d --- /dev/null +++ b/Documentation/ABI/testing/debugfs-rmi4 @@ -0,0 +1,99 @@ +What: /sys/kernel/debug/rmi/devices +Date: October 2012 +KernelVersion: 3.x +Contact: Christopher Heiny +Description: + + The RMI4 driver implementation exposes a set of informational and control + parameters via debugs. These parameters are those that typically are only + viewed or adjusted during product development, tuning, and debug. + For parameters that are referenced and/or adjusted during normal operation, + please see sysfs-rmi4 in this directory. + + General debugging parameters for a particular RMI4 sensor are found in + /sys/kernel/debug/rmi/sensorXX/, where XX is a the device's ID as a two + digit number (padded with leading zeros). Function specific parameters + for an RMI4 sensor are found in /sys/kernel/debug/rmi/devices/FYY/, where + XX is a the device's ID as a two digit number (padded with leading zeros) + and YY is the hexdecimal function number (for example, F11 for RMI function + F11). + + For RMI4 functions that support multiple sensor instances (such as F11), + the parameters for individual sensors have .Z appended to them, where Z is + the index of the sensor instance (for example, clip.0, clip.1, clip.2, and + so on). + + Some of the parameters exposed here are described in detail in the + RMI4 Specification, which is found here: +http://www.synaptics.com/sites/default/files/511-000136-01_revD.pdf + For such parameters, we'll reference you to that document, rather than + copying the contents here. + + /sys/kernel/debug/rmi/ + /sensorXX/ + attn_count - (ro) Shows the number of ATTN interrupts handled so far. + comms_debug - (rw) Write 1 to this dump information about register + reads and writes to the console. Write 0 to this to turn + this feature off. WARNING: Imposes a major performance + penalty when turned on. + irq_debug - (rw) Write 1 to this dump information about interrupts + to the console. Write 0 to this to turn this feature off. + WARNIG: Imposes a major performance penalty when turned on. + phys - (ro) Presents information about the physical connection of + this device. It has one line, with the format: + + prot tx_count tx_bytes tx_errors rx_count rx_bytes rx_errors attn + + Where + prot is one of i2c, spi1, or spi2 + tx_count is the number of write operations + tx_bytes is the number of bytes written + tx_errors is the number of write operations that encountered errors + rx_count is the number of read operations + rx_bytes is the total number of bytes read + rx_errors is the number of read operations that encountered errors + + All counts are 64-bit unsigned values, and are set to zero + when the physical layer driver is initialized. + + /sensorXX/F01/ + interrupt_enable - (rw) allows you to read or modify the F01 + interrupt enable mask (the F01_RMI_Ctrl1 register(s)). + + /sensorXX/F11/ + clip.Z - (rw) Controls in-driver coordinate clipping for the 2D + sensor Z. This is a set of four unsigned values in the + range [0..65535], representing the lower bounds on X, the + upper bounds on X, the lower bounds on Y, and the upper + bounds on Y. Coordinates will be clipped to these ranges. + If enabled, clip is the final transformation to be applied + to the coordinates. The default upper and lower bounds for + clip are 0 and 65535 respectively for both axes. + delta_threshold.Z - (rw) Controls the F11 distance thresholds. This + contains two values, corresponding to F11_2D_Ctrl2 and + F11_2D_Ctrl3. Se the spec for more details. + flip.Z - (rw) This parameter is a pair of single binary digits (for + example, "0 0" or "1 0"), corresponding to the X and
[RFC PATCH 01/06] input/rmi4: Public header and documentation
As requested in the feedback from the previous patch, we've documented the debugfs and sysfs attributes in files in Documentation/ABI/testing. There's two files, one for debugfs and one for sysfs. Signed-off-by: Christopher Heiny che...@synaptics.com Cc: Dmitry Torokhov dmitry.torok...@gmail.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com Cc: Joeri de Gram j.de.g...@gmail.com --- Documentation/ABI/testing/debugfs-rmi4 | 99 + Documentation/ABI/testing/sysfs-rmi4 | 103 + include/linux/rmi.h| 696 3 files changed, 898 insertions(+), 0 deletions(-) diff --git a/Documentation/ABI/testing/debugfs-rmi4 b/Documentation/ABI/testing/debugfs-rmi4 new file mode 100644 index 000..cf1aa2d --- /dev/null +++ b/Documentation/ABI/testing/debugfs-rmi4 @@ -0,0 +1,99 @@ +What: /sys/kernel/debug/rmi/devices +Date: October 2012 +KernelVersion: 3.x +Contact: Christopher Heiny che...@synaptics.com +Description: + + The RMI4 driver implementation exposes a set of informational and control + parameters via debugs. These parameters are those that typically are only + viewed or adjusted during product development, tuning, and debug. + For parameters that are referenced and/or adjusted during normal operation, + please see sysfs-rmi4 in this directory. + + General debugging parameters for a particular RMI4 sensor are found in + /sys/kernel/debug/rmi/sensorXX/, where XX is a the device's ID as a two + digit number (padded with leading zeros). Function specific parameters + for an RMI4 sensor are found in /sys/kernel/debug/rmi/devices/FYY/, where + XX is a the device's ID as a two digit number (padded with leading zeros) + and YY is the hexdecimal function number (for example, F11 for RMI function + F11). + + For RMI4 functions that support multiple sensor instances (such as F11), + the parameters for individual sensors have .Z appended to them, where Z is + the index of the sensor instance (for example, clip.0, clip.1, clip.2, and + so on). + + Some of the parameters exposed here are described in detail in the + RMI4 Specification, which is found here: +http://www.synaptics.com/sites/default/files/511-000136-01_revD.pdf + For such parameters, we'll reference you to that document, rather than + copying the contents here. + + /sys/kernel/debug/rmi/ + /sensorXX/ + attn_count - (ro) Shows the number of ATTN interrupts handled so far. + comms_debug - (rw) Write 1 to this dump information about register + reads and writes to the console. Write 0 to this to turn + this feature off. WARNING: Imposes a major performance + penalty when turned on. + irq_debug - (rw) Write 1 to this dump information about interrupts + to the console. Write 0 to this to turn this feature off. + WARNIG: Imposes a major performance penalty when turned on. + phys - (ro) Presents information about the physical connection of + this device. It has one line, with the format: + + prot tx_count tx_bytes tx_errors rx_count rx_bytes rx_errors attn + + Where + prot is one of i2c, spi1, or spi2 + tx_count is the number of write operations + tx_bytes is the number of bytes written + tx_errors is the number of write operations that encountered errors + rx_count is the number of read operations + rx_bytes is the total number of bytes read + rx_errors is the number of read operations that encountered errors + + All counts are 64-bit unsigned values, and are set to zero + when the physical layer driver is initialized. + + /sensorXX/F01/ + interrupt_enable - (rw) allows you to read or modify the F01 + interrupt enable mask (the F01_RMI_Ctrl1 register(s)). + + /sensorXX/F11/ + clip.Z - (rw) Controls in-driver coordinate clipping for the 2D + sensor Z. This is a set of four unsigned values in the + range [0..65535], representing the lower bounds on X, the + upper bounds on X, the lower bounds on Y, and the upper + bounds on Y. Coordinates will be clipped to these ranges. + If enabled, clip is the final transformation to be applied + to the coordinates. The default upper and lower bounds for + clip are 0 and 65535 respectively for both axes. + delta_threshold.Z - (rw) Controls the F11 distance thresholds. This + contains two values, corresponding to F11_2D_Ctrl2 and + F11_2D_Ctrl3. Se the spec for more details. +
