Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1305638 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 2/3] Input: cyttsp - add device tree bindings Oreste Salerno <oreste.salerno@tomtom.com> - 2016-01-10 18:40 +0100
    Re: [PATCH v3 2/3] Input: cyttsp - add device tree bindings Rob Herring <robh@kernel.org> - 2016-01-11 04:10 +0100
      Re: [PATCH v3 2/3] Input: cyttsp - add device tree bindings Oreste Salerno <oreste.salerno@tomtom.com> - 2016-01-11 19:00 +0100
        Re: [PATCH v3 2/3] Input: cyttsp - add device tree bindings Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-01-11 19:50 +0100
          Re: [PATCH v3 2/3] Input: cyttsp - add device tree bindings Oreste Salerno <oreste.salerno@tomtom.com> - 2016-01-11 20:10 +0100
    Re: [PATCH v3 2/3] Input: cyttsp - add device tree bindings Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-01-11 19:40 +0100

#1305638 — [PATCH v3 2/3] Input: cyttsp - add device tree bindings

Signed-off-by: Oreste Salerno <oreste.salerno@tomtom.com>
---
 .../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.
+- 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
+
+[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+[1]: Documentation/devicetree/bindings/gpio/gpio.txt
+
+Example:
+	&i2c1 {
+		/* ... */
+		cyttsp@a {
+			compatible = "cypress,cyttsp-i2c";
+			reg = <0xa>;
+			interrupt-parent = <&msm_gpio>;
+			interrupts = <13 0x2008>;
+			reset-gpios = <&msm_gpio 12 0x00>;
+
+			touchscreen-size-x = <800>;
+			touchscreen-size-y = <480>;
+			bootloader-key = /bits/ 8 <0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08>;
+
+			active-distance = <8>;
+			active-interval-ms = <0>;
+			lowpower-interval-ms = <200>;
+			touch-timeout-ms = <100>;
+		};
+
+		/* ... */
+	};
+
+	&mcspi1 {
+		/* ... */
+		cyttsp@0 {
+			compatible = "cypress,cyttsp-spi";
+			spi-max-frequency = <6000000>;
+			reg = <0>;
+			interrupt-parent = <&msm_gpio>;
+			interrupts = <13 0x2008>;
+			reset-gpios = <&msm_gpio 12 0x00>;
+
+			touchscreen-size-x = <800>;
+			touchscreen-size-y = <480>;
+			bootloader-key = /bits/ 8 <0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08>;
+
+			active-distance = <8>;
+			active-interval-ms = <0>;
+			lowpower-interval-ms = <200>;
+			touch-timeout-ms = <100>;
+		};
+
+		/* ... */
+	};
diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
index 5b74e8b..5dc6bf6 100644
--- a/drivers/input/touchscreen/cyttsp_core.c
+++ b/drivers/input/touchscreen/cyttsp_core.c
@@ -33,6 +33,8 @@
 #include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/gpio/consumer.h>
 
 #include "cyttsp_core.h"
 
@@ -57,6 +59,7 @@
 #define CY_DELAY_DFLT			20 /* ms */
 #define CY_DELAY_MAX			500
 #define CY_ACT_DIST_DFLT		0xF8
+#define CY_ACT_DIST_MASK		0x0F
 #define CY_HNDSHK_BIT			0x80
 /* device mode bits */
 #define CY_OPERATE_MODE			0x00
@@ -528,18 +531,136 @@ static void cyttsp_close(struct input_dev *dev)
 		cyttsp_disable(ts);
 }
 
+#ifdef CONFIG_OF
+static const struct cyttsp_platform_data *cyttsp_parse_dt(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct cyttsp_platform_data *pdata;
+	u32 dt_value;
+	int ret;
+	static const char err_msg[] =
+		"property not provided in the device tree";
+
+	if (!np)
+		return ERR_PTR(-ENOENT);
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->bl_keys = devm_kzalloc(dev, CY_NUM_BL_KEYS, GFP_KERNEL);
+	if (!pdata->bl_keys)
+		return ERR_PTR(-ENOMEM);
+
+	/* Set some default values */
+	pdata->act_dist = CY_ACT_DIST_DFLT;
+	pdata->act_intrvl = CY_ACT_INTRVL_DFLT;
+	pdata->tch_tmout = CY_TCH_TMOUT_DFLT;
+	pdata->lp_intrvl = CY_LP_INTRVL_DFLT;
+	pdata->name = "cyttsp";
+
+	ret = of_property_read_u32(np, "touchscreen-size-x", &pdata->maxx);
+	if (ret) {
+		dev_err(dev, "touchscreen-size-x %s\n", err_msg);
+		return ERR_PTR(ret);
+	}
+
+	ret = of_property_read_u32(np, "touchscreen-size-y", &pdata->maxy);
+	if (ret) {
+		dev_err(dev, "touchscreen-size-y %s\n", err_msg);
+		return ERR_PTR(ret);
+	}
+
+	pdata->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->reset_gpio)) {
+		ret = PTR_ERR(pdata->reset_gpio);
+		dev_err(dev, "error acquiring reset gpio: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	ret = of_property_read_u8_array(np, "bootloader-key",
+			pdata->bl_keys, CY_NUM_BL_KEYS);
+	if (ret) {
+		dev_err(dev, "bootloader-key %s\n", err_msg);
+		return ERR_PTR(ret);
+	}
+
+	if (!of_property_read_u32(np, "active-distance", &dt_value)) {
+		if (dt_value > 15) {
+			dev_err(dev, "active-distance (%u) must be [0-15]\n",
+				dt_value);
+			return ERR_PTR(-EINVAL);
+		}
+		pdata->act_dist &= ~CY_ACT_DIST_MASK;
+		pdata->act_dist |= (u8)dt_value;
+	}
+
+	if (!of_property_read_u32(np, "active-interval-ms", &dt_value)) {
+		if (dt_value > 255) {
+			dev_err(dev, "active-interval-ms (%u) must be [0-255]\n",
+				dt_value);
+			return ERR_PTR(-EINVAL);
+		}
+		pdata->act_intrvl = (u8)dt_value;
+	}
+
+	if (!of_property_read_u32(np, "lowpower-interval-ms", &dt_value)) {
+		if (dt_value > 2550) {
+			dev_err(dev, "lowpower-interval-ms (%u) must be [0-2550]\n",
+				dt_value);
+			return ERR_PTR(-EINVAL);
+		}
+		/* Register value is expressed in 0.01s / bit */
+		pdata->lp_intrvl = (u8)(dt_value/10);
+	}
+
+	if (!of_property_read_u32(np, "touch-timeout-ms", &dt_value)) {
+		if (dt_value > 2550) {
+			dev_err(dev, "touch-timeout-ms (%u) must be [0-2550]\n",
+				dt_value);
+			return ERR_PTR(-EINVAL);
+		}
+		/* Register value is expressed in 0.01s / bit */
+		pdata->tch_tmout = (u8)(dt_value/10);
+	}
+
+	return pdata;
+}
+#else
+static const struct cyttsp_platform_data *cyttsp_parse_dt(struct device *dev)
+{
+	return ERR_PTR(-ENOENT);
+}
+#endif
+
+static const struct cyttsp_platform_data *
+cyttsp_get_platform_data(struct device *dev)
+{
+	const struct cyttsp_platform_data *pdata;
+
+	pdata = dev_get_platdata(dev);
+	if (pdata)
+		return pdata;
+
+	pdata = cyttsp_parse_dt(dev);
+	if (!IS_ERR(pdata) || PTR_ERR(pdata) != -ENOENT)
+		return pdata;
+
+	dev_err(dev, "No platform data specified\n");
+	return ERR_PTR(-EINVAL);
+}
+
 struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
 			    struct device *dev, int irq, size_t xfer_buf_size)
 {
-	const struct cyttsp_platform_data *pdata = dev_get_platdata(dev);
+	const struct cyttsp_platform_data *pdata;
 	struct cyttsp *ts;
 	struct input_dev *input_dev;
 	int error;
 
-	if (!pdata || !pdata->name || irq <= 0) {
-		error = -EINVAL;
-		goto err_out;
-	}
+	pdata = cyttsp_get_platform_data(dev);
+	if (IS_ERR(pdata))
+		return ERR_CAST(pdata);
 
 	ts = kzalloc(sizeof(*ts) + xfer_buf_size, GFP_KERNEL);
 	input_dev = input_allocate_device();
@@ -550,7 +671,7 @@ struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
 
 	ts->dev = dev;
 	ts->input = input_dev;
-	ts->pdata = dev_get_platdata(dev);
+	ts->pdata = pdata;
 	ts->bus_ops = bus_ops;
 	ts->irq = irq;
 
@@ -618,7 +739,6 @@ err_platform_exit:
 err_free_mem:
 	input_free_device(input_dev);
 	kfree(ts);
-err_out:
 	return ERR_PTR(error);
 }
 EXPORT_SYMBOL_GPL(cyttsp_probe);
diff --git a/include/linux/input/cyttsp.h b/include/linux/input/cyttsp.h
index d7c2520..92a9d52 100644
--- a/include/linux/input/cyttsp.h
+++ b/include/linux/input/cyttsp.h
@@ -29,6 +29,8 @@
 #ifndef _CYTTSP_H_
 #define _CYTTSP_H_
 
+#include <linux/gpio/consumer.h>
+
 #define CY_SPI_NAME "cyttsp-spi"
 #define CY_I2C_NAME "cyttsp-i2c"
 /* Active Power state scanning/processing refresh interval */
@@ -51,6 +53,7 @@ struct cyttsp_platform_data {
 	int (*init)(void);
 	void (*exit)(void);
 	char *name;
+	struct gpio_desc *reset_gpio;
 	u8 *bl_keys;
 };
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1305795

On Sun, Jan 10, 2016 at 06:36:08PM +0100, Oreste Salerno wrote:
> Signed-off-by: Oreste Salerno <oreste.salerno@tomtom.com>
> ---
>  .../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? 

> +- 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.

[toc] | [prev] | [next] | [standalone]

#1306579

On Sun, Jan 10, 2016 at 09:01:56PM -0600, Rob Herring wrote:
> On Sun, Jan 10, 2016 at 06:36:08PM +0100, Oreste Salerno wrote:
> > Signed-off-by: Oreste Salerno <oreste.salerno@tomtom.com>
> > ---
> >  .../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().
 
> > +- 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?  

[toc] | [prev] | [next] | [standalone]

#1306601

On Mon, Jan 11, 2016 at 06:54:23PM +0100, Oreste Salerno wrote:
> On Sun, Jan 10, 2016 at 09:01:56PM -0600, Rob Herring wrote:
> > On Sun, Jan 10, 2016 at 06:36:08PM +0100, Oreste Salerno wrote:
> > > Signed-off-by: Oreste Salerno <oreste.salerno@tomtom.com>
> > > ---
> > >  .../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.

>  
> > > +- 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.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1306603

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:
> > On Sun, Jan 10, 2016 at 09:01:56PM -0600, Rob Herring wrote:
> > > On Sun, Jan 10, 2016 at 06:36:08PM +0100, Oreste Salerno wrote:
> > > > Signed-off-by: Oreste Salerno <oreste.salerno@tomtom.com>
> > > > ---
> > > >  .../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.
 
> >  
> > > > +- 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

[toc] | [prev] | [next] | [standalone]

#1306597

Hi Oreste,

On Sun, Jan 10, 2016 at 06:36:08PM +0100, Oreste Salerno wrote:
> Signed-off-by: Oreste Salerno <oreste.salerno@tomtom.com>
> ---
>  .../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)

Stray space before tab after spi-max-frequency.

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

Let's direct users to common touchscreen properties.

> +- 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.
> +- 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
> +
> +[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +[1]: Documentation/devicetree/bindings/gpio/gpio.txt
> +
> +Example:
> +	&i2c1 {
> +		/* ... */
> +		cyttsp@a {
> +			compatible = "cypress,cyttsp-i2c";
> +			reg = <0xa>;
> +			interrupt-parent = <&msm_gpio>;
> +			interrupts = <13 0x2008>;
> +			reset-gpios = <&msm_gpio 12 0x00>;
> +
> +			touchscreen-size-x = <800>;
> +			touchscreen-size-y = <480>;
> +			bootloader-key = /bits/ 8 <0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08>;
> +
> +			active-distance = <8>;
> +			active-interval-ms = <0>;
> +			lowpower-interval-ms = <200>;
> +			touch-timeout-ms = <100>;
> +		};
> +
> +		/* ... */
> +	};
> +
> +	&mcspi1 {
> +		/* ... */
> +		cyttsp@0 {
> +			compatible = "cypress,cyttsp-spi";
> +			spi-max-frequency = <6000000>;
> +			reg = <0>;
> +			interrupt-parent = <&msm_gpio>;
> +			interrupts = <13 0x2008>;
> +			reset-gpios = <&msm_gpio 12 0x00>;
> +
> +			touchscreen-size-x = <800>;
> +			touchscreen-size-y = <480>;
> +			bootloader-key = /bits/ 8 <0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08>;
> +
> +			active-distance = <8>;
> +			active-interval-ms = <0>;
> +			lowpower-interval-ms = <200>;
> +			touch-timeout-ms = <100>;
> +		};
> +
> +		/* ... */
> +	};
> diff --git a/drivers/input/touchscreen/cyttsp_core.c b/drivers/input/touchscreen/cyttsp_core.c
> index 5b74e8b..5dc6bf6 100644
> --- a/drivers/input/touchscreen/cyttsp_core.c
> +++ b/drivers/input/touchscreen/cyttsp_core.c
> @@ -33,6 +33,8 @@
>  #include <linux/gpio.h>
>  #include <linux/interrupt.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include "cyttsp_core.h"
>  
> @@ -57,6 +59,7 @@
>  #define CY_DELAY_DFLT			20 /* ms */
>  #define CY_DELAY_MAX			500
>  #define CY_ACT_DIST_DFLT		0xF8
> +#define CY_ACT_DIST_MASK		0x0F
>  #define CY_HNDSHK_BIT			0x80
>  /* device mode bits */
>  #define CY_OPERATE_MODE			0x00
> @@ -528,18 +531,136 @@ static void cyttsp_close(struct input_dev *dev)
>  		cyttsp_disable(ts);
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct cyttsp_platform_data *cyttsp_parse_dt(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct cyttsp_platform_data *pdata;
> +	u32 dt_value;
> +	int ret;
> +	static const char err_msg[] =
> +		"property not provided in the device tree";
> +
> +	if (!np)
> +		return ERR_PTR(-ENOENT);
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdata->bl_keys = devm_kzalloc(dev, CY_NUM_BL_KEYS, GFP_KERNEL);
> +	if (!pdata->bl_keys)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Set some default values */
> +	pdata->act_dist = CY_ACT_DIST_DFLT;
> +	pdata->act_intrvl = CY_ACT_INTRVL_DFLT;
> +	pdata->tch_tmout = CY_TCH_TMOUT_DFLT;
> +	pdata->lp_intrvl = CY_LP_INTRVL_DFLT;
> +	pdata->name = "cyttsp";
> +
> +	ret = of_property_read_u32(np, "touchscreen-size-x", &pdata->maxx);
> +	if (ret) {
> +		dev_err(dev, "touchscreen-size-x %s\n", err_msg);
> +		return ERR_PTR(ret);
> +	}
> +
> +	ret = of_property_read_u32(np, "touchscreen-size-y", &pdata->maxy);
> +	if (ret) {
> +		dev_err(dev, "touchscreen-size-y %s\n", err_msg);
> +		return ERR_PTR(ret);
> +	}

I'd rather we called touchscreen_parse_properties() in common code when
setting up the input device. It should work for variety of platform
providers (OF, ACPI, etc).

> +
> +	pdata->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(pdata->reset_gpio)) {
> +		ret = PTR_ERR(pdata->reset_gpio);
> +		dev_err(dev, "error acquiring reset gpio: %d\n", ret);
> +		return ERR_PTR(ret);
> +	}

Please call it directly in probe(). Again, it should work fine not only
on OF, but on all other platforms, so 

> +
> +	ret = of_property_read_u8_array(np, "bootloader-key",
> +			pdata->bl_keys, CY_NUM_BL_KEYS);
> +	if (ret) {
> +		dev_err(dev, "bootloader-key %s\n", err_msg);
> +		return ERR_PTR(ret);
> +	}
> +
> +	if (!of_property_read_u32(np, "active-distance", &dt_value)) {
> +		if (dt_value > 15) {
> +			dev_err(dev, "active-distance (%u) must be [0-15]\n",
> +				dt_value);
> +			return ERR_PTR(-EINVAL);
> +		}
> +		pdata->act_dist &= ~CY_ACT_DIST_MASK;
> +		pdata->act_dist |= (u8)dt_value;

I do not think you need to cast.

> +	}
> +
> +	if (!of_property_read_u32(np, "active-interval-ms", &dt_value)) {
> +		if (dt_value > 255) {
> +			dev_err(dev, "active-interval-ms (%u) must be [0-255]\n",
> +				dt_value);
> +			return ERR_PTR(-EINVAL);
> +		}
> +		pdata->act_intrvl = (u8)dt_value;

I do not think you need to cast.

> +	}
> +
> +	if (!of_property_read_u32(np, "lowpower-interval-ms", &dt_value)) {
> +		if (dt_value > 2550) {
> +			dev_err(dev, "lowpower-interval-ms (%u) must be [0-2550]\n",
> +				dt_value);
> +			return ERR_PTR(-EINVAL);
> +		}
> +		/* Register value is expressed in 0.01s / bit */
> +		pdata->lp_intrvl = (u8)(dt_value/10);

Spaces around operations please. Do we need explicit cast here?

> +	}
> +
> +	if (!of_property_read_u32(np, "touch-timeout-ms", &dt_value)) {
> +		if (dt_value > 2550) {
> +			dev_err(dev, "touch-timeout-ms (%u) must be [0-2550]\n",
> +				dt_value);
> +			return ERR_PTR(-EINVAL);
> +		}
> +		/* Register value is expressed in 0.01s / bit */
> +		pdata->tch_tmout = (u8)(dt_value/10);

Spaces around operations please. Do we need explicit cast here?

> +	}
> +
> +	return pdata;
> +}
> +#else
> +static const struct cyttsp_platform_data *cyttsp_parse_dt(struct device *dev)
> +{
> +	return ERR_PTR(-ENOENT);
> +}
> +#endif
> +
> +static const struct cyttsp_platform_data *
> +cyttsp_get_platform_data(struct device *dev)
> +{
> +	const struct cyttsp_platform_data *pdata;
> +
> +	pdata = dev_get_platdata(dev);
> +	if (pdata)
> +		return pdata;
> +
> +	pdata = cyttsp_parse_dt(dev);
> +	if (!IS_ERR(pdata) || PTR_ERR(pdata) != -ENOENT)
> +		return pdata;
> +
> +	dev_err(dev, "No platform data specified\n");
> +	return ERR_PTR(-EINVAL);
> +}
> +
>  struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
>  			    struct device *dev, int irq, size_t xfer_buf_size)
>  {
> -	const struct cyttsp_platform_data *pdata = dev_get_platdata(dev);
> +	const struct cyttsp_platform_data *pdata;
>  	struct cyttsp *ts;
>  	struct input_dev *input_dev;
>  	int error;
>  
> -	if (!pdata || !pdata->name || irq <= 0) {
> -		error = -EINVAL;
> -		goto err_out;
> -	}
> +	pdata = cyttsp_get_platform_data(dev);
> +	if (IS_ERR(pdata))
> +		return ERR_CAST(pdata);
>  
>  	ts = kzalloc(sizeof(*ts) + xfer_buf_size, GFP_KERNEL);
>  	input_dev = input_allocate_device();
> @@ -550,7 +671,7 @@ struct cyttsp *cyttsp_probe(const struct cyttsp_bus_ops *bus_ops,
>  
>  	ts->dev = dev;
>  	ts->input = input_dev;
> -	ts->pdata = dev_get_platdata(dev);
> +	ts->pdata = pdata;
>  	ts->bus_ops = bus_ops;
>  	ts->irq = irq;
>  
> @@ -618,7 +739,6 @@ err_platform_exit:
>  err_free_mem:
>  	input_free_device(input_dev);
>  	kfree(ts);
> -err_out:
>  	return ERR_PTR(error);
>  }
>  EXPORT_SYMBOL_GPL(cyttsp_probe);
> diff --git a/include/linux/input/cyttsp.h b/include/linux/input/cyttsp.h
> index d7c2520..92a9d52 100644
> --- a/include/linux/input/cyttsp.h
> +++ b/include/linux/input/cyttsp.h
> @@ -29,6 +29,8 @@
>  #ifndef _CYTTSP_H_
>  #define _CYTTSP_H_
>  
> +#include <linux/gpio/consumer.h>
> +
>  #define CY_SPI_NAME "cyttsp-spi"
>  #define CY_I2C_NAME "cyttsp-i2c"
>  /* Active Power state scanning/processing refresh interval */
> @@ -51,6 +53,7 @@ struct cyttsp_platform_data {
>  	int (*init)(void);
>  	void (*exit)(void);
>  	char *name;
> +	struct gpio_desc *reset_gpio;

No, we shoudl not be passing gpio descriptor in platform data. For
non-of non-ACPI system gpiod_* framework allows boards to specify lookup
tables so they can use that. Just drop it form here.

>  	u8 *bl_keys;
>  };
>  
> -- 
> 1.9.1
> 

Thanks.

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web