Re: [PATCH v3 2/3] Input: cyttsp - add device tree bindings

On Mon, Jan 11, 2016 at 10:45:50AM -0800, Dmitry Torokhov wrote:

On Mon, Jan 11, 2016 at 06:54:23PM +0100, Oreste Salerno wrote:

quoted

On Sun, Jan 10, 2016 at 09:01:56PM -0600, Rob Herring wrote:

quoted

On Sun, Jan 10, 2016 at 06:36:08PM +0100, Oreste Salerno wrote:

quoted

Signed-off-by: Oreste Salerno <redacted>
---
 .../bindings/input/touchscreen/cyttsp.txt          |  82 +++++++++++++
 drivers/input/touchscreen/cyttsp_core.c            | 134 +++++++++++++++++++--
 include/linux/input/cyttsp.h                       |   3 +
 3 files changed, 212 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cyttsp.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/cyttsp.txt b/Documentation/devicetree/bindings/input/touchscreen/cyttsp.txt
new file mode 100644
index 0000000..2dc5c65
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/cyttsp.txt

@@ -0,0 +1,82 @@
+* Cypress cyttsp touchscreen controller
+
+Required properties:
+- compatible		: must be "cypress,cyttsp-i2c" or "cypress,cyttsp-spi"
+- reg			: Device I2C address or SPI chip select number
+- spi-max-frequency 	: Maximum SPI clocking speed of the device (for cyttsp-spi)
+- interrupt-parent	: the phandle for the gpio controller
+			  (see interrupt binding[0]).
+- interrupts		: (gpio) interrupt to which the chip is connected
+			  (see interrupt binding[0]).
+- reset-gpios		: the reset gpio the chip is connected to
+			  (see GPIO binding[1] for more details).
+- touchscreen-size-x	: horizontal resolution of touchscreen (in pixels)
+- touchscreen-size-y	: vertical resolution of touchscreen (in pixels)
+- bootloader-key	: the 8-byte bootloader key that is required to switch
+			  the chip from bootloader mode (default mode) to
+			  application mode.
+			  This property has to be specified as an array of 8
+			  '/bits/ 8' values.
+
+Optional properties:
+- active-distance	: the distance in pixels beyond which a touch must move
+			  before movement is detected and reported by the device.
+			  Valid values: 0-15.

Wouldn't this be the same as touchscreen-fuzz-(x|y) which is the 
replacement for the deprecated moving-threshold? 

It is somewhat related. But I understand that the touchscreen-fuzz-(x|y) properties
are meant to be used as parameters passed to input_set_abs_params(), which are used
for a SW-based jitter filtering.
The active-distance value instead is used to set a HW register in the chip which
effectively performs the filtering in the HW.
The driver is currently passing 0 as fuzz parameter to input_set_abs_params().

It doe snot have to and it will not if you use
touchscreen_parse_properties(). But you are right, fuzz is reserved for
software de-jittering, and it can be changed at any time from userspace
at input device level, but will not be communicated to the hardware. I
think hardware-based filtering is hardware-specific and I think
having a separate property is fine for it. We can still standardize on
name, units, etc though.

Yes, I looked at touchscreen_parse_properties and I was planning to use it as part
of a future improvement patch. But it makes sense to introduce it in this patch already.
 

quoted

quoted

quoted

+- active-interval-ms	: the minimum period in ms between consecutive
+			  scanning/processing cycles when the chip is in active mode.
+			  Valid values: 0-255.
+- lowpower-interval-ms	: the minimum period in ms between consecutive
+			  scanning/processing cycles when the chip is in low-power mode.
+			  Valid values: 0-2550
+- touch-timeout-ms	: minimum time in ms spent in the active power state while no
+			  touches are detected before entering low-power mode.
+			  Valid values: 0-2550
+

Otherwise, looks fine.

I am still waiting for a review from the driver maintainer (Ferruh Yigit), but I see
that he hasn't been actively reviewing patches for the cyttsp* drivers for a while.
I wonder what would be the process in this case?  

Please address the items from my other email and I should be able to
pick it up.

OK, I'll try to rework the patch soon and post a new version.
Thanks a lot for your review!

Thanks.

-- 
Dmitry