[PATCH 1/2] DT: Add vendor prefix for Emerging Display Technologies

[PATCH 1/2] DT: Add vendor prefix for Emerging Display Technologies

[Lothar Waßmann]

Signed-off-by: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
---
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index b458760..49774c3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -29,6 +29,7 @@ dallas	Maxim Integrated Products (formerly Dallas Semiconductor)
 davicom	DAVICOM Semiconductor, Inc.
 denx	Denx Software Engineering
 dmo	Data Modul AG
+edt	Emerging Display Technologies
 emmicro	EM Microelectronic
 epson	Seiko Epson Corp.
 est	ESTeem Wireless Modems
-- 
1.7.2.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] Input: edt-ft5x06: Add DT support

[Lothar Waßmann]

This patch allows the edt-ft5x06 multitouch panel driver to be
configured via DT.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 .../bindings/input/touchscreen/edt-ft5x06.txt      |   31 ++++
 drivers/input/touchscreen/edt-ft5x06.c             |  145 +++++++++++++++----
 2 files changed, 145 insertions(+), 31 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
new file mode 100644
index 0000000..629dbdd
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -0,0 +1,31 @@
+* EDT FT5x06 Multiple Touch Controller
+
+Required properties:
+- compatible: must be "edt,ft5x06"
+- reg: i2c slave address
+- interrupt-parent: the phandle for the interrupt controller
+- interrupts: touch controller interrupt
+
+Optional properties:
+- reset-gpios:  the gpio pin to be used for resetting the controller
+- wakeup-gpios: the gpio pin to be used for waking up the controller
+
+  The following properties provide default values for the
+  corresponding parameters configurable via sysfs
+  (see Documentation/input/edt-ft5x06.txt)
+- threshold: allows setting the "click"-threshold in the range from 20 to 80.
+- gain: sensitivity (0..31) (lower value -> higher sensitivity)
+- offset: edge compensation (0..31)
+- report_rate: report rate (3..14)
+
+Example:
+
+	edt_ft5x06@38 {
+		compatible = "edt,ft5x06";
+		reg = <0x38>;
+		interrupt-parent = <&gpio2>;
+		interrupts = <5 0>;
+		wakeup-gpios = <&gpio1 9 0>;
+		reset-gpios = <&gpio2 6 1>;
+		wakeup-gpios = <&gpio4 9 0>;
+	};
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index af0d68b..02dce42 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -33,6 +33,7 @@
 #include <linux/debugfs.h>
 #include <linux/slab.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/input/mt.h>
 #include <linux/input/edt-ft5x06.h>
 
@@ -65,6 +66,10 @@ struct edt_ft5x06_ts_data {
 	u16 num_x;
 	u16 num_y;
 
+	int reset_pin;
+	int irq_pin;
+	int wake_pin;
+
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *debug_dir;
 	u8 *raw_buffer;
@@ -617,24 +622,37 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
 
 
 static int edt_ft5x06_ts_reset(struct i2c_client *client,
-					 int reset_pin)
+			struct edt_ft5x06_ts_data *tsdata)
 {
 	int error;
 
-	if (gpio_is_valid(reset_pin)) {
+	if (gpio_is_valid(tsdata->wake_pin)) {
+		error = gpio_request_one(tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
+					 "edt-ft5x06 wake");
+		if (error) {
+			dev_err(&client->dev,
+				"Failed to request GPIO %d as wake pin, error %d\n",
+				tsdata->wake_pin, error);
+			return error;
+		}
+
+		mdelay(5);
+		gpio_set_value(tsdata->wake_pin, 1);
+	}
+	if (gpio_is_valid(tsdata->reset_pin)) {
 		/* this pulls reset down, enabling the low active reset */
-		error = gpio_request_one(reset_pin, GPIOF_OUT_INIT_LOW,
+		error = gpio_request_one(tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
 					 "edt-ft5x06 reset");
 		if (error) {
 			dev_err(&client->dev,
 				"Failed to request GPIO %d as reset pin, error %d\n",
-				reset_pin, error);
+				tsdata->reset_pin, error);
 			return error;
 		}
 
-		mdelay(50);
-		gpio_set_value(reset_pin, 1);
-		mdelay(100);
+		mdelay(5);
+		gpio_set_value(tsdata->reset_pin, 1);
+		mdelay(300);
 	}
 
 	return 0;
@@ -674,6 +692,21 @@ static int edt_ft5x06_ts_identify(struct i2c_client *client,
 	    pdata->name <= edt_ft5x06_attr_##name.limit_high)		\
 		edt_ft5x06_register_write(tsdata, reg, pdata->name)
 
+#define EDT_GET_PROP(name, reg) {					\
+	const u32 *prop = of_get_property(np, #name, NULL);		\
+	if (prop)							\
+		edt_ft5x06_register_write(tsdata, reg, be32_to_cpu(*prop)); \
+}
+
+static void edt_ft5x06_ts_get_dt_defaults(struct device_node *np,
+					struct edt_ft5x06_ts_data *tsdata)
+{
+	EDT_GET_PROP(threshold, WORK_REGISTER_THRESHOLD);
+	EDT_GET_PROP(gain, WORK_REGISTER_GAIN);
+	EDT_GET_PROP(offset, WORK_REGISTER_OFFSET);
+	EDT_GET_PROP(report_rate, WORK_REGISTER_REPORT_RATE);
+}
+
 static void
 edt_ft5x06_ts_get_defaults(struct edt_ft5x06_ts_data *tsdata,
 			   const struct edt_ft5x06_platform_data *pdata)
@@ -701,6 +734,39 @@ edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
 	tsdata->num_y = edt_ft5x06_register_read(tsdata, WORK_REGISTER_NUM_Y);
 }
 
+#ifdef CONFIG_OF
+static int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
+				struct edt_ft5x06_ts_data *tsdata)
+{
+	int ret;
+	struct device_node *np = dev->of_node;
+	enum of_gpio_flags gpio_flags;
+
+	if (!np)
+		return -ENODEV;
+
+	/*
+	 * irq_pin is not needed for DT setup.
+	 * irq is associated via 'interrupts' property in DT
+	 */
+	tsdata->irq_pin = -EINVAL;
+
+	ret = of_get_named_gpio_flags(np, "reset-gpios", 0, &gpio_flags);
+	tsdata->reset_pin = ret;
+
+	ret = of_get_named_gpio_flags(np, "wake-gpios", 0, &gpio_flags);
+	tsdata->wake_pin = ret;
+
+	return 0;
+}
+#else
+static inline int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
+					struct edt_ft5x06_i2c_ts_data *tsdata)
+{
+	return -ENODEV;
+}
+#endif
+
 static int edt_ft5x06_ts_probe(struct i2c_client *client,
 					 const struct i2c_device_id *id)
 {
@@ -713,30 +779,43 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 
 	dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
 
+	tsdata = devm_kzalloc(&client->dev, sizeof(*tsdata), GFP_KERNEL);
+	if (!tsdata) {
+		dev_err(&client->dev, "failed to allocate driver data.\n");
+		return -ENOMEM;
+	}
+
 	if (!pdata) {
-		dev_err(&client->dev, "no platform data?\n");
-		return -EINVAL;
+		error = edt_ft5x06_i2c_ts_probe_dt(&client->dev, tsdata);
+		if (error) {
+			dev_err(&client->dev,
+				"DT probe failed and no platform data present\n");
+			return error;
+		}
+	} else {
+		tsdata->reset_pin = pdata->reset_pin;
+		tsdata->irq_pin = pdata->irq_pin;
+		tsdata->wake_pin = -EINVAL;
 	}
 
-	error = edt_ft5x06_ts_reset(client, pdata->reset_pin);
+	error = edt_ft5x06_ts_reset(client, tsdata);
 	if (error)
 		return error;
 
-	if (gpio_is_valid(pdata->irq_pin)) {
-		error = gpio_request_one(pdata->irq_pin,
+	if (gpio_is_valid(tsdata->irq_pin)) {
+		error = gpio_request_one(tsdata->irq_pin,
 					 GPIOF_IN, "edt-ft5x06 irq");
 		if (error) {
 			dev_err(&client->dev,
 				"Failed to request GPIO %d, error %d\n",
-				pdata->irq_pin, error);
+				tsdata->irq_pin, error);
 			return error;
 		}
 	}
 
-	tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
 	input = input_allocate_device();
-	if (!tsdata || !input) {
-		dev_err(&client->dev, "failed to allocate driver data.\n");
+	if (!input) {
+		dev_err(&client->dev, "failed to allocate input device.\n");
 		error = -ENOMEM;
 		goto err_free_mem;
 	}
@@ -752,7 +831,11 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 		goto err_free_mem;
 	}
 
-	edt_ft5x06_ts_get_defaults(tsdata, pdata);
+	if (!pdata)
+		edt_ft5x06_ts_get_dt_defaults(client->dev.of_node, tsdata);
+	else
+		edt_ft5x06_ts_get_defaults(tsdata, pdata);
+
 	edt_ft5x06_ts_get_parameters(tsdata);
 
 	dev_dbg(&client->dev,
@@ -802,8 +885,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 	device_init_wakeup(&client->dev, 1);
 
 	dev_dbg(&client->dev,
-		"EDT FT5x06 initialized: IRQ pin %d, Reset pin %d.\n",
-		pdata->irq_pin, pdata->reset_pin);
+		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
+		client->irq, tsdata->wake_pin, tsdata->reset_pin);
 
 	return 0;
 
@@ -813,18 +896,18 @@ err_free_irq:
 	free_irq(client->irq, tsdata);
 err_free_mem:
 	input_free_device(input);
-	kfree(tsdata);
-
-	if (gpio_is_valid(pdata->irq_pin))
-		gpio_free(pdata->irq_pin);
+	if (gpio_is_valid(tsdata->irq_pin))
+		gpio_free(tsdata->irq_pin);
+	if (gpio_is_valid(tsdata->reset_pin))
+		gpio_free(tsdata->reset_pin);
+	if (gpio_is_valid(tsdata->wake_pin))
+		gpio_free(tsdata->wake_pin);
 
 	return error;
 }
 
 static int edt_ft5x06_ts_remove(struct i2c_client *client)
 {
-	const struct edt_ft5x06_platform_data *pdata =
-						dev_get_platdata(&client->dev);
 	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
 
 	edt_ft5x06_ts_teardown_debugfs(tsdata);
@@ -833,12 +916,12 @@ static int edt_ft5x06_ts_remove(struct i2c_client *client)
 	free_irq(client->irq, tsdata);
 	input_unregister_device(tsdata->input);
 
-	if (gpio_is_valid(pdata->irq_pin))
-		gpio_free(pdata->irq_pin);
-	if (gpio_is_valid(pdata->reset_pin))
-		gpio_free(pdata->reset_pin);
-
-	kfree(tsdata);
+	if (gpio_is_valid(tsdata->irq_pin))
+		gpio_free(tsdata->irq_pin);
+	if (gpio_is_valid(tsdata->reset_pin))
+		gpio_free(tsdata->reset_pin);
+	if (gpio_is_valid(tsdata->wake_pin))
+		gpio_free(tsdata->wake_pin);
 
 	return 0;
 }
-- 
1.7.2.5

Re: [PATCH 2/2] Input: edt-ft5x06: Add DT support

[Mark Rutland]

On Mon, Jan 13, 2014 at 10:17:04AM +0000, Lothar Waßmann wrote:
> This patch allows the edt-ft5x06 multitouch panel driver to be
> configured via DT.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  .../bindings/input/touchscreen/edt-ft5x06.txt      |   31 ++++
>  drivers/input/touchscreen/edt-ft5x06.c             |  145 +++++++++++++++----
>  2 files changed, 145 insertions(+), 31 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> new file mode 100644
> index 0000000..629dbdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -0,0 +1,31 @@
> +* EDT FT5x06 Multiple Touch Controller
> +
> +Required properties:
> +- compatible: must be "edt,ft5x06"
> +- reg: i2c slave address
> +- interrupt-parent: the phandle for the interrupt controller
> +- interrupts: touch controller interrupt
> +
> +Optional properties:
> +- reset-gpios:  the gpio pin to be used for resetting the controller
> +- wakeup-gpios: the gpio pin to be used for waking up the controller
> +
> +  The following properties provide default values for the
> +  corresponding parameters configurable via sysfs
> +  (see Documentation/input/edt-ft5x06.txt)

The sysfs interface shouldn't matter for the binding, it's a Linux
detail. There's no reason for it to be mentioned in the binding.

> +- threshold: allows setting the "click"-threshold in the range from 20 to 80.
> +- gain: sensitivity (0..31) (lower value -> higher sensitivity)
> +- offset: edge compensation (0..31)
> +- report_rate: report rate (3..14)

s/_/-/ on property names please. Also, it may make sense to prefix these
as they're rather generic sounding names.

Could you elaborate on these a litle please? What units are each of
these in? Why does it make sense to have them in the dt?

> +
> +Example:
> +
> +	edt_ft5x06@38 {
> +		compatible = "edt,ft5x06";
> +		reg = <0x38>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <5 0>;
> +		wakeup-gpios = <&gpio1 9 0>;
> +		reset-gpios = <&gpio2 6 1>;
> +		wakeup-gpios = <&gpio4 9 0>;
> +	};

One of those wakeup-gpios properties should go.

[...]

> +#define EDT_GET_PROP(name, reg) {					\
> +	const u32 *prop = of_get_property(np, #name, NULL);		\
> +	if (prop)							\
> +		edt_ft5x06_register_write(tsdata, reg, be32_to_cpu(*prop)); \
> +}

Use of_property_read_u32, which does endianness conversion and property
length checking (which you're not doing). Sparse _will_ complain here
because you've not maintained the __be32 annotation.

> +static void edt_ft5x06_ts_get_dt_defaults(struct device_node *np,
> +					struct edt_ft5x06_ts_data *tsdata)
> +{
> +	EDT_GET_PROP(threshold, WORK_REGISTER_THRESHOLD);
> +	EDT_GET_PROP(gain, WORK_REGISTER_GAIN);
> +	EDT_GET_PROP(offset, WORK_REGISTER_OFFSET);
> +	EDT_GET_PROP(report_rate, WORK_REGISTER_REPORT_RATE);

These should probably have sanity checks given you've described valid
ranges in the documentation.

[...]

> +static int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
> +				struct edt_ft5x06_ts_data *tsdata)
> +{
> +	int ret;
> +	struct device_node *np = dev->of_node;
> +	enum of_gpio_flags gpio_flags;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	/*
> +	 * irq_pin is not needed for DT setup.
> +	 * irq is associated via 'interrupts' property in DT
> +	 */
> +	tsdata->irq_pin = -EINVAL;
> +
> +	ret = of_get_named_gpio_flags(np, "reset-gpios", 0, &gpio_flags);
> +	tsdata->reset_pin = ret;
> +
> +	ret = of_get_named_gpio_flags(np, "wake-gpios", 0, &gpio_flags);
> +	tsdata->wake_pin = ret;

Shouldn't that be "wakeup-gpios"?

Do you not need the value of flags? I'm not that familiar with the GPIO
subsystem, and it feels odd to rtequest the flags and throw them away.

Thanks,
Mark.

Re: [PATCH 2/2] Input: edt-ft5x06: Add DT support

[Jingoo Han]

On Monday, January 13, 2014 7:17 PM, Lothar Waßmann wrote:
> 
> This patch allows the edt-ft5x06 multitouch panel driver to be
> configured via DT.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>

(+cc Simon Budig)

This 'edt-ft5x06' driver was made by Simon Budig.
Please, add him to CC list.

Also, I added some comments. :-)

> ---
>  .../bindings/input/touchscreen/edt-ft5x06.txt      |   31 ++++
>  drivers/input/touchscreen/edt-ft5x06.c             |  145 +++++++++++++++----
>  2 files changed, 145 insertions(+), 31 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> new file mode 100644
> index 0000000..629dbdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> @@ -0,0 +1,31 @@
> +* EDT FT5x06 Multiple Touch Controller
> +
> +Required properties:
> +- compatible: must be "edt,ft5x06"
> +- reg: i2c slave address
> +- interrupt-parent: the phandle for the interrupt controller
> +- interrupts: touch controller interrupt
> +
> +Optional properties:
> +- reset-gpios:  the gpio pin to be used for resetting the controller
> +- wakeup-gpios: the gpio pin to be used for waking up the controller
> +
> +  The following properties provide default values for the
> +  corresponding parameters configurable via sysfs
> +  (see Documentation/input/edt-ft5x06.txt)
> +- threshold: allows setting the "click"-threshold in the range from 20 to 80.
> +- gain: sensitivity (0..31) (lower value -> higher sensitivity)
> +- offset: edge compensation (0..31)
> +- report_rate: report rate (3..14)

s/report_rate/report-rate

> +
> +Example:
> +
> +	edt_ft5x06@38 {
> +		compatible = "edt,ft5x06";
> +		reg = <0x38>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <5 0>;
> +		wakeup-gpios = <&gpio1 9 0>;
> +		reset-gpios = <&gpio2 6 1>;
> +		wakeup-gpios = <&gpio4 9 0>;
> +	};
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index af0d68b..02dce42 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -33,6 +33,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/slab.h>
>  #include <linux/gpio.h>
> +#include <linux/of_gpio.h>
>  #include <linux/input/mt.h>
>  #include <linux/input/edt-ft5x06.h>
> 
> @@ -65,6 +66,10 @@ struct edt_ft5x06_ts_data {
>  	u16 num_x;
>  	u16 num_y;
> 
> +	int reset_pin;
> +	int irq_pin;
> +	int wake_pin;
> +
>  #if defined(CONFIG_DEBUG_FS)
>  	struct dentry *debug_dir;
>  	u8 *raw_buffer;
> @@ -617,24 +622,37 @@ edt_ft5x06_ts_teardown_debugfs(struct edt_ft5x06_ts_data *tsdata)
> 
> 
>  static int edt_ft5x06_ts_reset(struct i2c_client *client,
> -					 int reset_pin)
> +			struct edt_ft5x06_ts_data *tsdata)
>  {
>  	int error;
> 
> -	if (gpio_is_valid(reset_pin)) {
> +	if (gpio_is_valid(tsdata->wake_pin)) {
> +		error = gpio_request_one(tsdata->wake_pin, GPIOF_OUT_INIT_LOW,
> +					 "edt-ft5x06 wake");

Please use devm_gpio_request_one(), because it makes the code
simpler.

> +		if (error) {
> +			dev_err(&client->dev,
> +				"Failed to request GPIO %d as wake pin, error %d\n",
> +				tsdata->wake_pin, error);
> +			return error;
> +		}
> +
> +		mdelay(5);
> +		gpio_set_value(tsdata->wake_pin, 1);
> +	}
> +	if (gpio_is_valid(tsdata->reset_pin)) {
>  		/* this pulls reset down, enabling the low active reset */
> -		error = gpio_request_one(reset_pin, GPIOF_OUT_INIT_LOW,
> +		error = gpio_request_one(tsdata->reset_pin, GPIOF_OUT_INIT_LOW,
>  					 "edt-ft5x06 reset");

Please use devm_gpio_request_one() too.

>  		if (error) {
>  			dev_err(&client->dev,
>  				"Failed to request GPIO %d as reset pin, error %d\n",
> -				reset_pin, error);
> +				tsdata->reset_pin, error);
>  			return error;
>  		}
> 
> -		mdelay(50);
> -		gpio_set_value(reset_pin, 1);
> -		mdelay(100);
> +		mdelay(5);
> +		gpio_set_value(tsdata->reset_pin, 1);
> +		mdelay(300);
>  	}
> 
>  	return 0;
> @@ -674,6 +692,21 @@ static int edt_ft5x06_ts_identify(struct i2c_client *client,
>  	    pdata->name <= edt_ft5x06_attr_##name.limit_high)		\
>  		edt_ft5x06_register_write(tsdata, reg, pdata->name)
> 
> +#define EDT_GET_PROP(name, reg) {					\
> +	const u32 *prop = of_get_property(np, #name, NULL);		\
> +	if (prop)							\
> +		edt_ft5x06_register_write(tsdata, reg, be32_to_cpu(*prop)); \
> +}
> +
> +static void edt_ft5x06_ts_get_dt_defaults(struct device_node *np,
> +					struct edt_ft5x06_ts_data *tsdata)
> +{
> +	EDT_GET_PROP(threshold, WORK_REGISTER_THRESHOLD);
> +	EDT_GET_PROP(gain, WORK_REGISTER_GAIN);
> +	EDT_GET_PROP(offset, WORK_REGISTER_OFFSET);
> +	EDT_GET_PROP(report_rate, WORK_REGISTER_REPORT_RATE);
> +}
> +
>  static void
>  edt_ft5x06_ts_get_defaults(struct edt_ft5x06_ts_data *tsdata,
>  			   const struct edt_ft5x06_platform_data *pdata)
> @@ -701,6 +734,39 @@ edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
>  	tsdata->num_y = edt_ft5x06_register_read(tsdata, WORK_REGISTER_NUM_Y);
>  }
> 
> +#ifdef CONFIG_OF
> +static int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
> +				struct edt_ft5x06_ts_data *tsdata)
> +{
> +	int ret;
> +	struct device_node *np = dev->of_node;
> +	enum of_gpio_flags gpio_flags;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	/*
> +	 * irq_pin is not needed for DT setup.
> +	 * irq is associated via 'interrupts' property in DT
> +	 */
> +	tsdata->irq_pin = -EINVAL;
> +
> +	ret = of_get_named_gpio_flags(np, "reset-gpios", 0, &gpio_flags);
> +	tsdata->reset_pin = ret;

This code looks awkward.
There are two ways, please choose one of them.

1. Check the return value

	ret = of_get_named_gpio_flags(np, "reset-gpios", 0, &gpio_flags);
	if (ret < 0) {
		dev_err(dev, "reset-gpios pin is not provided.\n");
		return ret;
	}
	tsdata->reset_pin = ret;

2. No check the return value, passing it directly

	tsdata->reset_pin = of_get_named_gpio_flags(np, "reset-gpios", 0, &gpio_flags);


> +
> +	ret = of_get_named_gpio_flags(np, "wake-gpios", 0, &gpio_flags);

According to the Documentation 'edt-ft5x06.txt',
'wakeup-gpios' is used, instead of 'wake-gpios'.
What is the right name?

> +	tsdata->wake_pin = ret;
> +
> +	return 0;
> +}
> +#else
> +static inline int edt_ft5x06_i2c_ts_probe_dt(struct device *dev,
> +					struct edt_ft5x06_i2c_ts_data *tsdata)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  					 const struct i2c_device_id *id)
>  {
> @@ -713,30 +779,43 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> 
>  	dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
> 
> +	tsdata = devm_kzalloc(&client->dev, sizeof(*tsdata), GFP_KERNEL);
> +	if (!tsdata) {
> +		dev_err(&client->dev, "failed to allocate driver data.\n");
> +		return -ENOMEM;
> +	}
> +
>  	if (!pdata) {
> -		dev_err(&client->dev, "no platform data?\n");
> -		return -EINVAL;
> +		error = edt_ft5x06_i2c_ts_probe_dt(&client->dev, tsdata);
> +		if (error) {
> +			dev_err(&client->dev,
> +				"DT probe failed and no platform data present\n");
> +			return error;
> +		}
> +	} else {
> +		tsdata->reset_pin = pdata->reset_pin;
> +		tsdata->irq_pin = pdata->irq_pin;
> +		tsdata->wake_pin = -EINVAL;
>  	}
> 
> -	error = edt_ft5x06_ts_reset(client, pdata->reset_pin);
> +	error = edt_ft5x06_ts_reset(client, tsdata);
>  	if (error)
>  		return error;
> 
> -	if (gpio_is_valid(pdata->irq_pin)) {
> -		error = gpio_request_one(pdata->irq_pin,
> +	if (gpio_is_valid(tsdata->irq_pin)) {
> +		error = gpio_request_one(tsdata->irq_pin,
>  					 GPIOF_IN, "edt-ft5x06 irq");

Please use devm_gpio_request_one() too.

>  		if (error) {
>  			dev_err(&client->dev,
>  				"Failed to request GPIO %d, error %d\n",
> -				pdata->irq_pin, error);
> +				tsdata->irq_pin, error);
>  			return error;
>  		}
>  	}
> 
> -	tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
>  	input = input_allocate_device();
> -	if (!tsdata || !input) {
> -		dev_err(&client->dev, "failed to allocate driver data.\n");
> +	if (!input) {
> +		dev_err(&client->dev, "failed to allocate input device.\n");
>  		error = -ENOMEM;
>  		goto err_free_mem;
>  	}
> @@ -752,7 +831,11 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  		goto err_free_mem;
>  	}
> 
> -	edt_ft5x06_ts_get_defaults(tsdata, pdata);
> +	if (!pdata)
> +		edt_ft5x06_ts_get_dt_defaults(client->dev.of_node, tsdata);
> +	else
> +		edt_ft5x06_ts_get_defaults(tsdata, pdata);
> +
>  	edt_ft5x06_ts_get_parameters(tsdata);
> 
>  	dev_dbg(&client->dev,
> @@ -802,8 +885,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  	device_init_wakeup(&client->dev, 1);
> 
>  	dev_dbg(&client->dev,
> -		"EDT FT5x06 initialized: IRQ pin %d, Reset pin %d.\n",
> -		pdata->irq_pin, pdata->reset_pin);
> +		"EDT FT5x06 initialized: IRQ %d, WAKE pin %d, Reset pin %d.\n",
> +		client->irq, tsdata->wake_pin, tsdata->reset_pin);
> 
>  	return 0;
> 
> @@ -813,18 +896,18 @@ err_free_irq:
>  	free_irq(client->irq, tsdata);
>  err_free_mem:
>  	input_free_device(input);
> -	kfree(tsdata);
> -
> -	if (gpio_is_valid(pdata->irq_pin))
> -		gpio_free(pdata->irq_pin);
> +	if (gpio_is_valid(tsdata->irq_pin))
> +		gpio_free(tsdata->irq_pin);
> +	if (gpio_is_valid(tsdata->reset_pin))
> +		gpio_free(tsdata->reset_pin);
> +	if (gpio_is_valid(tsdata->wake_pin))
> +		gpio_free(tsdata->wake_pin);

If you use devm_gpio_request_one(), these gpio_free()s are not
necessary.

> 
>  	return error;
>  }
> 
>  static int edt_ft5x06_ts_remove(struct i2c_client *client)
>  {
> -	const struct edt_ft5x06_platform_data *pdata =
> -						dev_get_platdata(&client->dev);
>  	struct edt_ft5x06_ts_data *tsdata = i2c_get_clientdata(client);
> 
>  	edt_ft5x06_ts_teardown_debugfs(tsdata);
> @@ -833,12 +916,12 @@ static int edt_ft5x06_ts_remove(struct i2c_client *client)
>  	free_irq(client->irq, tsdata);
>  	input_unregister_device(tsdata->input);
> 
> -	if (gpio_is_valid(pdata->irq_pin))
> -		gpio_free(pdata->irq_pin);
> -	if (gpio_is_valid(pdata->reset_pin))
> -		gpio_free(pdata->reset_pin);
> -
> -	kfree(tsdata);
> +	if (gpio_is_valid(tsdata->irq_pin))
> +		gpio_free(tsdata->irq_pin);
> +	if (gpio_is_valid(tsdata->reset_pin))
> +		gpio_free(tsdata->reset_pin);
> +	if (gpio_is_valid(tsdata->wake_pin))
> +		gpio_free(tsdata->wake_pin);

If you use devm_gpio_request_one(), these gpio_free()s are not
necessary.

Best regards,
Jingoo Han

Re: [PATCH 2/2] Input: edt-ft5x06: Add DT support

[Lothar Waßmann]

Hi,

Mark Rutland wrote:
> On Mon, Jan 13, 2014 at 10:17:04AM +0000, Lothar Waßmann wrote:
> > This patch allows the edt-ft5x06 multitouch panel driver to be
> > configured via DT.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  .../bindings/input/touchscreen/edt-ft5x06.txt      |   31 ++++
> >  drivers/input/touchscreen/edt-ft5x06.c             |  145 +++++++++++++++----
> >  2 files changed, 145 insertions(+), 31 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > new file mode 100644
> > index 0000000..629dbdd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
> > @@ -0,0 +1,31 @@
> > +* EDT FT5x06 Multiple Touch Controller
> > +
[...]
> > +- threshold: allows setting the "click"-threshold in the range from 20 to 80.
> > +- gain: sensitivity (0..31) (lower value -> higher sensitivity)
> > +- offset: edge compensation (0..31)
> > +- report_rate: report rate (3..14)
> 
> s/_/-/ on property names please. Also, it may make sense to prefix these
> as they're rather generic sounding names.
> 
> Could you elaborate on these a litle please? What units are each of
> these in? Why does it make sense to have them in the dt?
> 
I just converted them from being passed via platform_data. I have no
idea what they actually control and what units they use. I could not
even find a manual where they are documented.
Maybe Simon Budig can help out here as the original driver author.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Input: edt-ft5x06: Add DT support

[Simon Budig]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

On 16/01/14 08:49, Lothar Waßmann wrote:
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
>>> @@ -0,0 +1,31 @@
>>> +* EDT FT5x06 Multiple Touch Controller
>>> +
> [...]
>>> +- threshold: allows setting the "click"-threshold in the range from 20 to 80.
>>> +- gain: sensitivity (0..31) (lower value -> higher sensitivity)
>>> +- offset: edge compensation (0..31)
>>> +- report_rate: report rate (3..14)
>>
>> s/_/-/ on property names please. Also, it may make sense to prefix these
>> as they're rather generic sounding names.
>>
>> Could you elaborate on these a litle please? What units are each of
>> these in? Why does it make sense to have them in the dt?
>>
> I just converted them from being passed via platform_data. I have no
> idea what they actually control and what units they use. I could not
> even find a manual where they are documented.
> Maybe Simon Budig can help out here as the original driver author.

The units are not specified in the datasheets available to me. I suspect
that these are some sort of counter values related to the cap sensing.

The defaults differ for the different size glasses, so I really suspect
these are basically just numbers.

The only somewhat reasonable unit is available for the report-rate: it
is specified as about <value> * 10 touch reports per second.

Bye,
        Simon

-- 
       Simon Budig                        kernel concepts GmbH
       simon.budig@kernelconcepts.de      Sieghuetter Hauptweg 48
       +49-271-771091-17                  D-57072 Siegen



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]