[PATCH 0/3] Add DT support for tps6507x touchscreen

[PATCH 0/3] Add DT support for tps6507x touchscreen

[yegorslists]

From: Yegor Yefremov <yegorslists@googlemail.com>

This is an attempt to revive DT support for TI tps6507x that was started
in 2013 [1], [2].

Current changes:
 - remove tsc node
 - add /bits/ 16 to for 16-bit values
 - rework bindings description, i.e. all properties are optional

Histor of original changes:

Changes since V3:
 - Rebased on top of Dmitry's changes
 - Removed error handling for optional DT properties

Changes since V2:
 - Updated tps6507x documentation.
 - Removed unnecessary code.

Changes since V1:
 - Updated tps6507x documentation.
 - Updated commit message.
 - return proper error value in absence platform and DT data
   for touchscreen.

[1] https://www.spinics.net/lists/devicetree/msg09388.html
[2] https://patchwork.kernel.org/patch/2324441/

Yegor Yefremov (3):
  tps6507x-ts: update to devm_* API
  tps6507x-ts: add DT support
  tps6507x-ts: add DT bindings description

 Documentation/devicetree/bindings/mfd/tps6507x.txt | 30 +++++++-
 drivers/input/touchscreen/tps6507x-ts.c            | 79 ++++++++++++----------
 2 files changed, 73 insertions(+), 36 deletions(-)

-- 
2.1.4

[PATCH 1/3] tps6507x-ts: update to devm_* API

[yegorslists]

From: Yegor Yefremov <yegorslists@googlemail.com>

Update the code to use devm_* API so that driver
core will manage resources.

Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
---
 drivers/input/touchscreen/tps6507x-ts.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
index a340bfc..5bf1ec6 100644
--- a/drivers/input/touchscreen/tps6507x-ts.c
+++ b/drivers/input/touchscreen/tps6507x-ts.c
@@ -226,7 +226,7 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 	 */
 	init_data = tps_board->tps6507x_ts_init_data;
 
-	tsc = kzalloc(sizeof(struct tps6507x_ts), GFP_KERNEL);
+	tsc = devm_kzalloc(&pdev->dev, sizeof(struct tps6507x_ts), GFP_KERNEL);
 	if (!tsc) {
 		dev_err(tps6507x_dev->dev, "failed to allocate driver data\n");
 		return -ENOMEM;
@@ -244,7 +244,7 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 	if (!poll_dev) {
 		dev_err(tsc->dev, "Failed to allocate polled input device.\n");
 		error = -ENOMEM;
-		goto err_free_mem;
+		goto err;
 	}
 
 	tsc->poll_dev = poll_dev;
@@ -286,8 +286,7 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 
 err_free_polled_dev:
 	input_free_polled_device(poll_dev);
-err_free_mem:
-	kfree(tsc);
+err:
 	return error;
 }
 
@@ -299,8 +298,6 @@ static int tps6507x_ts_remove(struct platform_device *pdev)
 	input_unregister_polled_device(poll_dev);
 	input_free_polled_device(poll_dev);
 
-	kfree(tsc);
-
 	return 0;
 }
 
-- 
2.1.4

[PATCH 2/3] tps6507x-ts: add DT support

[yegorslists]

From: Yegor Yefremov <yegorslists@googlemail.com>

This patch adds support for DT to tps6507x-ts driver.

Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
---
 drivers/input/touchscreen/tps6507x-ts.c | 70 ++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
index 5bf1ec6..2d61220 100644
--- a/drivers/input/touchscreen/tps6507x-ts.c
+++ b/drivers/input/touchscreen/tps6507x-ts.c
@@ -22,6 +22,8 @@
 #include <linux/mfd/tps6507x.h>
 #include <linux/input/tps6507x-ts.h>
 #include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
 #define TPS_DEFAULT_MIN_PRESSURE 0x30
@@ -199,33 +201,52 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
 	tps6507x_adc_standby(tsc);
 }
 
+static void tsc_init_data(struct tps6507x_ts *tsc,
+		struct input_dev *input_dev)
+{
+	struct device_node *node = tsc->dev->of_node;
+	const struct tps6507x_board *tps_board =
+			dev_get_platdata(tsc->dev);
+	const struct touchscreen_init_data *init_data = NULL;
+
+	if (tps_board)
+		/*
+		 * init_data points to array of touchscreen_init structure
+		 * coming from the board-evm file.
+		 */
+		init_data = tps_board->tps6507x_ts_init_data;
+
+	if (node == NULL && init_data == NULL) {
+		tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
+		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
+	} else if (init_data) {
+		tsc->poll_dev->poll_interval = init_data->poll_period;
+		tsc->min_pressure = init_data->min_pressure;
+		input_dev->id.vendor = init_data->vendor;
+		input_dev->id.product = init_data->product;
+		input_dev->id.version = init_data->version;
+	} else if (node) {
+		of_property_read_u32(node, "ti,poll-period",
+				&tsc->poll_dev->poll_interval);
+		of_property_read_u16(node, "ti,min-pressure",
+				&tsc->min_pressure);
+		of_property_read_u16(node, "ti,vendor",
+				&input_dev->id.vendor);
+		of_property_read_u16(node, "ti,product",
+				&input_dev->id.product);
+		of_property_read_u16(node, "ti,version",
+				&input_dev->id.version);
+	}
+}
+
 static int tps6507x_ts_probe(struct platform_device *pdev)
 {
 	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
-	const struct tps6507x_board *tps_board;
-	const struct touchscreen_init_data *init_data;
 	struct tps6507x_ts *tsc;
 	struct input_polled_dev *poll_dev;
 	struct input_dev *input_dev;
 	int error;
 
-	/*
-	 * tps_board points to pmic related constants
-	 * coming from the board-evm file.
-	 */
-	tps_board = dev_get_platdata(tps6507x_dev->dev);
-	if (!tps_board) {
-		dev_err(tps6507x_dev->dev,
-			"Could not find tps6507x platform data\n");
-		return -ENODEV;
-	}
-
-	/*
-	 * init_data points to array of regulator_init structures
-	 * coming from the board-evm file.
-	 */
-	init_data = tps_board->tps6507x_ts_init_data;
-
 	tsc = devm_kzalloc(&pdev->dev, sizeof(struct tps6507x_ts), GFP_KERNEL);
 	if (!tsc) {
 		dev_err(tps6507x_dev->dev, "failed to allocate driver data\n");
@@ -234,8 +255,6 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 
 	tsc->mfd = tps6507x_dev;
 	tsc->dev = tps6507x_dev->dev;
-	tsc->min_pressure = init_data ?
-			init_data->min_pressure : TPS_DEFAULT_MIN_PRESSURE;
 
 	snprintf(tsc->phys, sizeof(tsc->phys),
 		 "%s/input0", dev_name(tsc->dev));
@@ -251,8 +270,6 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 
 	poll_dev->private = tsc;
 	poll_dev->poll = tps6507x_ts_poll;
-	poll_dev->poll_interval = init_data ?
-			init_data->poll_period : TSC_DEFAULT_POLL_PERIOD;
 
 	input_dev = poll_dev->input;
 	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
@@ -266,11 +283,8 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 	input_dev->phys = tsc->phys;
 	input_dev->dev.parent = tsc->dev;
 	input_dev->id.bustype = BUS_I2C;
-	if (init_data) {
-		input_dev->id.vendor = init_data->vendor;
-		input_dev->id.product = init_data->product;
-		input_dev->id.version = init_data->version;
-	}
+
+	tsc_init_data(tsc, input_dev);
 
 	error = tps6507x_adc_standby(tsc);
 	if (error)
-- 
2.1.4

[PATCH 3/3] tps6507x-ts: add DT bindings description

[yegorslists]

From: Yegor Yefremov <yegorslists@googlemail.com>

Provide description for following properties:

- ti,poll-period
- ti,min-pressure
- ti,vendor
- ti,product
- ti,version

Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
---
 Documentation/devicetree/bindings/mfd/tps6507x.txt | 30 ++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
index 8fffa3c..eb654ef 100644
--- a/Documentation/devicetree/bindings/mfd/tps6507x.txt
+++ b/Documentation/devicetree/bindings/mfd/tps6507x.txt
@@ -1,4 +1,8 @@
-TPS6507x Power Management Integrated Circuit
+TPS6507x Multifunctional Device.
+
+Features provided by TPS6507x:
+        1. Power Management Integrated Circuit.
+        2. Touch-Screen.
 
 Required properties:
 - compatible: "ti,tps6507x"
@@ -30,6 +34,23 @@ Regulator Optional properties:
 			1: If defdcdc pin of DCDC2/DCDC3 is driven HIGH.
   If this property is not defined, it defaults to 0 (not enabled).
 
+Touchscreen Optional properties:
+- ti,poll-period: Time at which touch input is getting sampled in ms.
+		  Default value: 30 ms.
+- ti,min-pressure: Minimum pressure value to trigger touch.
+		   Default value: 0x30.
+		   This property has to be a '/bits/ 16' value
+
+Entries below describe the input_id structure [1] for the touch device:
+- ti,vendor: Touchscreen vendor id.
+	     This property has to be a '/bits/ 16' value
+- ti,product: Touchscreen product id.
+	      This property has to be a '/bits/ 16' value
+- ti,version: Touchscreen version id.
+	      This property has to be a '/bits/ 16' value
+
+[1] include/uapi/linux/input.h
+
 Example:
 
 	pmu: tps6507x@48 {
@@ -40,6 +61,12 @@ Example:
 		vindcdc3-supply = <...>;
 		vinldo1_2-supply = <...>;
 
+		ti,poll-period = <30>;
+		ti,min-pressure = /bits/ 16 <0x30>;
+		ti,vendor = /bits/ 16 <0>;
+		ti,product = /bits/ 16 <65070>;
+		ti,version = /bits/ 16 <0x100>;
+
 		regulators {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -87,5 +114,4 @@ Example:
 				regulator-boot-on;
 			};
 		};
-
 	};
-- 
2.1.4

Re: [PATCH 2/3] tps6507x-ts: add DT support

[Dmitry Torokhov]

Hi Yegor,

On Fri, Feb 24, 2017 at 04:42:25PM +0100, yegorslists@googlemail.com wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
> 
> This patch adds support for DT to tps6507x-ts driver.
> 
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
>  drivers/input/touchscreen/tps6507x-ts.c | 70 ++++++++++++++++++++-------------
>  1 file changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> index 5bf1ec6..2d61220 100644
> --- a/drivers/input/touchscreen/tps6507x-ts.c
> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> @@ -22,6 +22,8 @@
>  #include <linux/mfd/tps6507x.h>
>  #include <linux/input/tps6507x-ts.h>
>  #include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  
>  #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
>  #define TPS_DEFAULT_MIN_PRESSURE 0x30
> @@ -199,33 +201,52 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
>  	tps6507x_adc_standby(tsc);
>  }
>  
> +static void tsc_init_data(struct tps6507x_ts *tsc,
> +		struct input_dev *input_dev)
> +{
> +	struct device_node *node = tsc->dev->of_node;
> +	const struct tps6507x_board *tps_board =
> +			dev_get_platdata(tsc->dev);
> +	const struct touchscreen_init_data *init_data = NULL;
> +
> +	if (tps_board)
> +		/*
> +		 * init_data points to array of touchscreen_init structure
> +		 * coming from the board-evm file.
> +		 */
> +		init_data = tps_board->tps6507x_ts_init_data;
> +
> +	if (node == NULL && init_data == NULL) {
> +		tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
> +		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;

I think you want to always init with defaults, then override with
property data.

> +	} else if (init_data) {
> +		tsc->poll_dev->poll_interval = init_data->poll_period;
> +		tsc->min_pressure = init_data->min_pressure;
> +		input_dev->id.vendor = init_data->vendor;
> +		input_dev->id.product = init_data->product;
> +		input_dev->id.version = init_data->version;
> +	} else if (node) {
> +		of_property_read_u32(node, "ti,poll-period",
> +				&tsc->poll_dev->poll_interval);
> +		of_property_read_u16(node, "ti,min-pressure",
> +				&tsc->min_pressure);

Anything but u32 is better avoided in DT as it requires special
annotation which is easily forgotten.

I also would prefer if we switched to generic device properties and
retired the static init data.


> +		of_property_read_u16(node, "ti,vendor",
> +				&input_dev->id.vendor);
> +		of_property_read_u16(node, "ti,product",
> +				&input_dev->id.product);

Would we not know vendor and product from the compatible string?

> +		of_property_read_u16(node, "ti,version",
> +				&input_dev->id.version);

I wonder if anyone cares about this.

> +	}
> +}
> +
>  static int tps6507x_ts_probe(struct platform_device *pdev)
>  {
>  	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
> -	const struct tps6507x_board *tps_board;
> -	const struct touchscreen_init_data *init_data;
>  	struct tps6507x_ts *tsc;
>  	struct input_polled_dev *poll_dev;
>  	struct input_dev *input_dev;
>  	int error;
>  
> -	/*
> -	 * tps_board points to pmic related constants
> -	 * coming from the board-evm file.
> -	 */
> -	tps_board = dev_get_platdata(tps6507x_dev->dev);
> -	if (!tps_board) {
> -		dev_err(tps6507x_dev->dev,
> -			"Could not find tps6507x platform data\n");
> -		return -ENODEV;
> -	}
> -
> -	/*
> -	 * init_data points to array of regulator_init structures
> -	 * coming from the board-evm file.
> -	 */
> -	init_data = tps_board->tps6507x_ts_init_data;
> -
>  	tsc = devm_kzalloc(&pdev->dev, sizeof(struct tps6507x_ts), GFP_KERNEL);
>  	if (!tsc) {
>  		dev_err(tps6507x_dev->dev, "failed to allocate driver data\n");
> @@ -234,8 +255,6 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>  
>  	tsc->mfd = tps6507x_dev;
>  	tsc->dev = tps6507x_dev->dev;
> -	tsc->min_pressure = init_data ?
> -			init_data->min_pressure : TPS_DEFAULT_MIN_PRESSURE;
>  
>  	snprintf(tsc->phys, sizeof(tsc->phys),
>  		 "%s/input0", dev_name(tsc->dev));
> @@ -251,8 +270,6 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>  
>  	poll_dev->private = tsc;
>  	poll_dev->poll = tps6507x_ts_poll;
> -	poll_dev->poll_interval = init_data ?
> -			init_data->poll_period : TSC_DEFAULT_POLL_PERIOD;
>  
>  	input_dev = poll_dev->input;
>  	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> @@ -266,11 +283,8 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>  	input_dev->phys = tsc->phys;
>  	input_dev->dev.parent = tsc->dev;
>  	input_dev->id.bustype = BUS_I2C;
> -	if (init_data) {
> -		input_dev->id.vendor = init_data->vendor;
> -		input_dev->id.product = init_data->product;
> -		input_dev->id.version = init_data->version;
> -	}
> +
> +	tsc_init_data(tsc, input_dev);
>  
>  	error = tps6507x_adc_standby(tsc);
>  	if (error)
> -- 
> 2.1.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] tps6507x-ts: add DT support

[Yegor Yefremov]

Hi Dmitry,

On Sat, Feb 25, 2017 at 8:08 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Yegor,
>
> On Fri, Feb 24, 2017 at 04:42:25PM +0100, yegorslists@googlemail.com wrote:
>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>
>> This patch adds support for DT to tps6507x-ts driver.
>>
>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> ---
>>  drivers/input/touchscreen/tps6507x-ts.c | 70 ++++++++++++++++++++-------------
>>  1 file changed, 42 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
>> index 5bf1ec6..2d61220 100644
>> --- a/drivers/input/touchscreen/tps6507x-ts.c
>> +++ b/drivers/input/touchscreen/tps6507x-ts.c
>> @@ -22,6 +22,8 @@
>>  #include <linux/mfd/tps6507x.h>
>>  #include <linux/input/tps6507x-ts.h>
>>  #include <linux/delay.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>>
>>  #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
>>  #define TPS_DEFAULT_MIN_PRESSURE 0x30
>> @@ -199,33 +201,52 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
>>       tps6507x_adc_standby(tsc);
>>  }
>>
>> +static void tsc_init_data(struct tps6507x_ts *tsc,
>> +             struct input_dev *input_dev)
>> +{
>> +     struct device_node *node = tsc->dev->of_node;
>> +     const struct tps6507x_board *tps_board =
>> +                     dev_get_platdata(tsc->dev);
>> +     const struct touchscreen_init_data *init_data = NULL;
>> +
>> +     if (tps_board)
>> +             /*
>> +              * init_data points to array of touchscreen_init structure
>> +              * coming from the board-evm file.
>> +              */
>> +             init_data = tps_board->tps6507x_ts_init_data;
>> +
>> +     if (node == NULL && init_data == NULL) {
>> +             tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
>> +             tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
>
> I think you want to always init with defaults, then override with
> property data.

ACK

>> +     } else if (init_data) {
>> +             tsc->poll_dev->poll_interval = init_data->poll_period;
>> +             tsc->min_pressure = init_data->min_pressure;
>> +             input_dev->id.vendor = init_data->vendor;
>> +             input_dev->id.product = init_data->product;
>> +             input_dev->id.version = init_data->version;
>> +     } else if (node) {
>> +             of_property_read_u32(node, "ti,poll-period",
>> +                             &tsc->poll_dev->poll_interval);
>> +             of_property_read_u16(node, "ti,min-pressure",
>> +                             &tsc->min_pressure);
>
> Anything but u32 is better avoided in DT as it requires special
> annotation which is easily forgotten.

Will convert to 32-bit values.

> I also would prefer if we switched to generic device properties and
> retired the static init data.

do you mean converting the driver to DT-only variant?

>> +             of_property_read_u16(node, "ti,vendor",
>> +                             &input_dev->id.vendor);
>> +             of_property_read_u16(node, "ti,product",
>> +                             &input_dev->id.product);
>
> Would we not know vendor and product from the compatible string?
>
>> +             of_property_read_u16(node, "ti,version",
>> +                             &input_dev->id.version);
>
> I wonder if anyone cares about this.

I've looked at other touchscreen drivers and this structure seems to
be filled mostly for the RS232 connected drivers. So I'd suggest just
to leave vendor, product and version at 0 and not provide these values
via DT.  At least tslib and hence Qt is working without these values
set.

@Kevin: so far the only user of this touch driver is da850-evm board.
Can we drop the values for the id structure? Btw. what else is needed
to remove arch/arm/mach-davinci/board-da850-evm.c completely?

Yegor

>> +     }
>> +}
>> +
>>  static int tps6507x_ts_probe(struct platform_device *pdev)
>>  {
>>       struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
>> -     const struct tps6507x_board *tps_board;
>> -     const struct touchscreen_init_data *init_data;
>>       struct tps6507x_ts *tsc;
>>       struct input_polled_dev *poll_dev;
>>       struct input_dev *input_dev;
>>       int error;
>>
>> -     /*
>> -      * tps_board points to pmic related constants
>> -      * coming from the board-evm file.
>> -      */
>> -     tps_board = dev_get_platdata(tps6507x_dev->dev);
>> -     if (!tps_board) {
>> -             dev_err(tps6507x_dev->dev,
>> -                     "Could not find tps6507x platform data\n");
>> -             return -ENODEV;
>> -     }
>> -
>> -     /*
>> -      * init_data points to array of regulator_init structures
>> -      * coming from the board-evm file.
>> -      */
>> -     init_data = tps_board->tps6507x_ts_init_data;
>> -
>>       tsc = devm_kzalloc(&pdev->dev, sizeof(struct tps6507x_ts), GFP_KERNEL);
>>       if (!tsc) {
>>               dev_err(tps6507x_dev->dev, "failed to allocate driver data\n");
>> @@ -234,8 +255,6 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>>
>>       tsc->mfd = tps6507x_dev;
>>       tsc->dev = tps6507x_dev->dev;
>> -     tsc->min_pressure = init_data ?
>> -                     init_data->min_pressure : TPS_DEFAULT_MIN_PRESSURE;
>>
>>       snprintf(tsc->phys, sizeof(tsc->phys),
>>                "%s/input0", dev_name(tsc->dev));
>> @@ -251,8 +270,6 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>>
>>       poll_dev->private = tsc;
>>       poll_dev->poll = tps6507x_ts_poll;
>> -     poll_dev->poll_interval = init_data ?
>> -                     init_data->poll_period : TSC_DEFAULT_POLL_PERIOD;
>>
>>       input_dev = poll_dev->input;
>>       input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>> @@ -266,11 +283,8 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>>       input_dev->phys = tsc->phys;
>>       input_dev->dev.parent = tsc->dev;
>>       input_dev->id.bustype = BUS_I2C;
>> -     if (init_data) {
>> -             input_dev->id.vendor = init_data->vendor;
>> -             input_dev->id.product = init_data->product;
>> -             input_dev->id.version = init_data->version;
>> -     }
>> +
>> +     tsc_init_data(tsc, input_dev);
>>
>>       error = tps6507x_adc_standby(tsc);
>>       if (error)
>> --
>> 2.1.4
>>
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 2/3] tps6507x-ts: add DT support

[Kevin Hilman]

+ Sekhar for the board-da850-evm questions.

Yegor Yefremov <yegorslists@googlemail.com> writes:

> Hi Dmitry,
>
> On Sat, Feb 25, 2017 at 8:08 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> Hi Yegor,
>>
>> On Fri, Feb 24, 2017 at 04:42:25PM +0100, yegorslists@googlemail.com wrote:
>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>>
>>> This patch adds support for DT to tps6507x-ts driver.
>>>
>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>>> ---
>>>  drivers/input/touchscreen/tps6507x-ts.c | 70 ++++++++++++++++++++-------------
>>>  1 file changed, 42 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
>>> index 5bf1ec6..2d61220 100644
>>> --- a/drivers/input/touchscreen/tps6507x-ts.c
>>> +++ b/drivers/input/touchscreen/tps6507x-ts.c
>>> @@ -22,6 +22,8 @@
>>>  #include <linux/mfd/tps6507x.h>
>>>  #include <linux/input/tps6507x-ts.h>
>>>  #include <linux/delay.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>>
>>>  #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
>>>  #define TPS_DEFAULT_MIN_PRESSURE 0x30
>>> @@ -199,33 +201,52 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
>>>       tps6507x_adc_standby(tsc);
>>>  }
>>>
>>> +static void tsc_init_data(struct tps6507x_ts *tsc,
>>> +             struct input_dev *input_dev)
>>> +{
>>> +     struct device_node *node = tsc->dev->of_node;
>>> +     const struct tps6507x_board *tps_board =
>>> +                     dev_get_platdata(tsc->dev);
>>> +     const struct touchscreen_init_data *init_data = NULL;
>>> +
>>> +     if (tps_board)
>>> +             /*
>>> +              * init_data points to array of touchscreen_init structure
>>> +              * coming from the board-evm file.
>>> +              */
>>> +             init_data = tps_board->tps6507x_ts_init_data;
>>> +
>>> +     if (node == NULL && init_data == NULL) {
>>> +             tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
>>> +             tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
>>
>> I think you want to always init with defaults, then override with
>> property data.
>
> ACK
>
>>> +     } else if (init_data) {
>>> +             tsc->poll_dev->poll_interval = init_data->poll_period;
>>> +             tsc->min_pressure = init_data->min_pressure;
>>> +             input_dev->id.vendor = init_data->vendor;
>>> +             input_dev->id.product = init_data->product;
>>> +             input_dev->id.version = init_data->version;
>>> +     } else if (node) {
>>> +             of_property_read_u32(node, "ti,poll-period",
>>> +                             &tsc->poll_dev->poll_interval);
>>> +             of_property_read_u16(node, "ti,min-pressure",
>>> +                             &tsc->min_pressure);
>>
>> Anything but u32 is better avoided in DT as it requires special
>> annotation which is easily forgotten.
>
> Will convert to 32-bit values.
>
>> I also would prefer if we switched to generic device properties and
>> retired the static init data.
>
> do you mean converting the driver to DT-only variant?
>
>>> +             of_property_read_u16(node, "ti,vendor",
>>> +                             &input_dev->id.vendor);
>>> +             of_property_read_u16(node, "ti,product",
>>> +                             &input_dev->id.product);
>>
>> Would we not know vendor and product from the compatible string?
>>
>>> +             of_property_read_u16(node, "ti,version",
>>> +                             &input_dev->id.version);
>>
>> I wonder if anyone cares about this.
>
> I've looked at other touchscreen drivers and this structure seems to
> be filled mostly for the RS232 connected drivers. So I'd suggest just
> to leave vendor, product and version at 0 and not provide these values
> via DT.  At least tslib and hence Qt is working without these values
> set.
>
> @Kevin: so far the only user of this touch driver is da850-evm board.
> Can we drop the values for the id structure? Btw. what else is needed
> to remove arch/arm/mach-davinci/board-da850-evm.c completely?
>
> Yegor
>
>>> +     }
>>> +}
>>> +
>>>  static int tps6507x_ts_probe(struct platform_device *pdev)
>>>  {
>>>       struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
>>> -     const struct tps6507x_board *tps_board;
>>> -     const struct touchscreen_init_data *init_data;
>>>       struct tps6507x_ts *tsc;
>>>       struct input_polled_dev *poll_dev;
>>>       struct input_dev *input_dev;
>>>       int error;
>>>
>>> -     /*
>>> -      * tps_board points to pmic related constants
>>> -      * coming from the board-evm file.
>>> -      */
>>> -     tps_board = dev_get_platdata(tps6507x_dev->dev);
>>> -     if (!tps_board) {
>>> -             dev_err(tps6507x_dev->dev,
>>> -                     "Could not find tps6507x platform data\n");
>>> -             return -ENODEV;
>>> -     }
>>> -
>>> -     /*
>>> -      * init_data points to array of regulator_init structures
>>> -      * coming from the board-evm file.
>>> -      */
>>> -     init_data = tps_board->tps6507x_ts_init_data;
>>> -
>>>       tsc = devm_kzalloc(&pdev->dev, sizeof(struct tps6507x_ts), GFP_KERNEL);
>>>       if (!tsc) {
>>>               dev_err(tps6507x_dev->dev, "failed to allocate driver data\n");
>>> @@ -234,8 +255,6 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>>>
>>>       tsc->mfd = tps6507x_dev;
>>>       tsc->dev = tps6507x_dev->dev;
>>> -     tsc->min_pressure = init_data ?
>>> -                     init_data->min_pressure : TPS_DEFAULT_MIN_PRESSURE;
>>>
>>>       snprintf(tsc->phys, sizeof(tsc->phys),
>>>                "%s/input0", dev_name(tsc->dev));
>>> @@ -251,8 +270,6 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>>>
>>>       poll_dev->private = tsc;
>>>       poll_dev->poll = tps6507x_ts_poll;
>>> -     poll_dev->poll_interval = init_data ?
>>> -                     init_data->poll_period : TSC_DEFAULT_POLL_PERIOD;
>>>
>>>       input_dev = poll_dev->input;
>>>       input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>>> @@ -266,11 +283,8 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>>>       input_dev->phys = tsc->phys;
>>>       input_dev->dev.parent = tsc->dev;
>>>       input_dev->id.bustype = BUS_I2C;
>>> -     if (init_data) {
>>> -             input_dev->id.vendor = init_data->vendor;
>>> -             input_dev->id.product = init_data->product;
>>> -             input_dev->id.version = init_data->version;
>>> -     }
>>> +
>>> +     tsc_init_data(tsc, input_dev);
>>>
>>>       error = tps6507x_adc_standby(tsc);
>>>       if (error)
>>> --
>>> 2.1.4
>>>
>>
>> Thanks.
>>
>> --
>> Dmitry

Re: [PATCH 2/3] tps6507x-ts: add DT support

[Sekhar Nori]

On Tuesday 28 February 2017 07:46 AM, Kevin Hilman wrote:
> + Sekhar for the board-da850-evm questions.
> 
> Yegor Yefremov <yegorslists@googlemail.com> writes:
> 
>> Hi Dmitry,
>>
>> On Sat, Feb 25, 2017 at 8:08 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>> Hi Yegor,
>>>
>>> On Fri, Feb 24, 2017 at 04:42:25PM +0100, yegorslists@googlemail.com wrote:
>>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>>>
>>>> This patch adds support for DT to tps6507x-ts driver.
>>>>
>>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>>>> ---
>>>>  drivers/input/touchscreen/tps6507x-ts.c | 70 ++++++++++++++++++++-------------
>>>>  1 file changed, 42 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
>>>> index 5bf1ec6..2d61220 100644
>>>> --- a/drivers/input/touchscreen/tps6507x-ts.c
>>>> +++ b/drivers/input/touchscreen/tps6507x-ts.c
>>>> @@ -22,6 +22,8 @@
>>>>  #include <linux/mfd/tps6507x.h>
>>>>  #include <linux/input/tps6507x-ts.h>
>>>>  #include <linux/delay.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>>
>>>>  #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
>>>>  #define TPS_DEFAULT_MIN_PRESSURE 0x30
>>>> @@ -199,33 +201,52 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
>>>>       tps6507x_adc_standby(tsc);
>>>>  }
>>>>
>>>> +static void tsc_init_data(struct tps6507x_ts *tsc,
>>>> +             struct input_dev *input_dev)
>>>> +{
>>>> +     struct device_node *node = tsc->dev->of_node;
>>>> +     const struct tps6507x_board *tps_board =
>>>> +                     dev_get_platdata(tsc->dev);
>>>> +     const struct touchscreen_init_data *init_data = NULL;
>>>> +
>>>> +     if (tps_board)
>>>> +             /*
>>>> +              * init_data points to array of touchscreen_init structure
>>>> +              * coming from the board-evm file.
>>>> +              */
>>>> +             init_data = tps_board->tps6507x_ts_init_data;
>>>> +
>>>> +     if (node == NULL && init_data == NULL) {
>>>> +             tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
>>>> +             tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
>>>
>>> I think you want to always init with defaults, then override with
>>> property data.
>>
>> ACK
>>
>>>> +     } else if (init_data) {
>>>> +             tsc->poll_dev->poll_interval = init_data->poll_period;
>>>> +             tsc->min_pressure = init_data->min_pressure;
>>>> +             input_dev->id.vendor = init_data->vendor;
>>>> +             input_dev->id.product = init_data->product;
>>>> +             input_dev->id.version = init_data->version;
>>>> +     } else if (node) {
>>>> +             of_property_read_u32(node, "ti,poll-period",
>>>> +                             &tsc->poll_dev->poll_interval);
>>>> +             of_property_read_u16(node, "ti,min-pressure",
>>>> +                             &tsc->min_pressure);
>>>
>>> Anything but u32 is better avoided in DT as it requires special
>>> annotation which is easily forgotten.
>>
>> Will convert to 32-bit values.
>>
>>> I also would prefer if we switched to generic device properties and
>>> retired the static init data.
>>
>> do you mean converting the driver to DT-only variant?
>>
>>>> +             of_property_read_u16(node, "ti,vendor",
>>>> +                             &input_dev->id.vendor);
>>>> +             of_property_read_u16(node, "ti,product",
>>>> +                             &input_dev->id.product);
>>>
>>> Would we not know vendor and product from the compatible string?
>>>
>>>> +             of_property_read_u16(node, "ti,version",
>>>> +                             &input_dev->id.version);
>>>
>>> I wonder if anyone cares about this.
>>
>> I've looked at other touchscreen drivers and this structure seems to
>> be filled mostly for the RS232 connected drivers. So I'd suggest just
>> to leave vendor, product and version at 0 and not provide these values
>> via DT.  At least tslib and hence Qt is working without these values
>> set.
>>
>> @Kevin: so far the only user of this touch driver is da850-evm board.
>> Can we drop the values for the id structure? Btw. what else is needed

It seems like it was mostly added so it can be read through sysfs. But
since it has been exposed to userspace, I am not sure if it can simply
be dropped. May be used hardcoded values for these in the driver based
on compatible string ?

>> to remove arch/arm/mach-davinci/board-da850-evm.c completely?

A lot of work has recently happened that should make this possible in
the near future. There are things like NOR flash, RMII ethernet, cpuidle
support etc which needs work. Ultimately we want a feature compatible
device-tree support to exist for at least one kernel version before the
board file is removed.

Thanks,
Sekhar

Re: [PATCH 2/3] tps6507x-ts: add DT support

[Dmitry Torokhov]

On Tue, Feb 28, 2017 at 03:36:26PM +0530, Sekhar Nori wrote:
> On Tuesday 28 February 2017 07:46 AM, Kevin Hilman wrote:
> > + Sekhar for the board-da850-evm questions.
> > 
> > Yegor Yefremov <yegorslists@googlemail.com> writes:
> > 
> >> Hi Dmitry,
> >>
> >> On Sat, Feb 25, 2017 at 8:08 PM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >>> Hi Yegor,
> >>>
> >>> On Fri, Feb 24, 2017 at 04:42:25PM +0100, yegorslists@googlemail.com wrote:
> >>>> From: Yegor Yefremov <yegorslists@googlemail.com>
> >>>>
> >>>> This patch adds support for DT to tps6507x-ts driver.
> >>>>
> >>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> >>>> ---
> >>>>  drivers/input/touchscreen/tps6507x-ts.c | 70 ++++++++++++++++++++-------------
> >>>>  1 file changed, 42 insertions(+), 28 deletions(-)
> >>>>
> >>>> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> >>>> index 5bf1ec6..2d61220 100644
> >>>> --- a/drivers/input/touchscreen/tps6507x-ts.c
> >>>> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> >>>> @@ -22,6 +22,8 @@
> >>>>  #include <linux/mfd/tps6507x.h>
> >>>>  #include <linux/input/tps6507x-ts.h>
> >>>>  #include <linux/delay.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/of_device.h>
> >>>>
> >>>>  #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
> >>>>  #define TPS_DEFAULT_MIN_PRESSURE 0x30
> >>>> @@ -199,33 +201,52 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
> >>>>       tps6507x_adc_standby(tsc);
> >>>>  }
> >>>>
> >>>> +static void tsc_init_data(struct tps6507x_ts *tsc,
> >>>> +             struct input_dev *input_dev)
> >>>> +{
> >>>> +     struct device_node *node = tsc->dev->of_node;
> >>>> +     const struct tps6507x_board *tps_board =
> >>>> +                     dev_get_platdata(tsc->dev);
> >>>> +     const struct touchscreen_init_data *init_data = NULL;
> >>>> +
> >>>> +     if (tps_board)
> >>>> +             /*
> >>>> +              * init_data points to array of touchscreen_init structure
> >>>> +              * coming from the board-evm file.
> >>>> +              */
> >>>> +             init_data = tps_board->tps6507x_ts_init_data;
> >>>> +
> >>>> +     if (node == NULL && init_data == NULL) {
> >>>> +             tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
> >>>> +             tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
> >>>
> >>> I think you want to always init with defaults, then override with
> >>> property data.
> >>
> >> ACK
> >>
> >>>> +     } else if (init_data) {
> >>>> +             tsc->poll_dev->poll_interval = init_data->poll_period;
> >>>> +             tsc->min_pressure = init_data->min_pressure;
> >>>> +             input_dev->id.vendor = init_data->vendor;
> >>>> +             input_dev->id.product = init_data->product;
> >>>> +             input_dev->id.version = init_data->version;
> >>>> +     } else if (node) {
> >>>> +             of_property_read_u32(node, "ti,poll-period",
> >>>> +                             &tsc->poll_dev->poll_interval);
> >>>> +             of_property_read_u16(node, "ti,min-pressure",
> >>>> +                             &tsc->min_pressure);
> >>>
> >>> Anything but u32 is better avoided in DT as it requires special
> >>> annotation which is easily forgotten.
> >>
> >> Will convert to 32-bit values.
> >>
> >>> I also would prefer if we switched to generic device properties and
> >>> retired the static init data.
> >>
> >> do you mean converting the driver to DT-only variant?

Not to DT only. device_property_read_* API works on DT, ACPI and legacy
boards that can be converted to use property sets (see
device_add_properties() and struct property_entry API).

> >>
> >>>> +             of_property_read_u16(node, "ti,vendor",
> >>>> +                             &input_dev->id.vendor);
> >>>> +             of_property_read_u16(node, "ti,product",
> >>>> +                             &input_dev->id.product);
> >>>
> >>> Would we not know vendor and product from the compatible string?
> >>>
> >>>> +             of_property_read_u16(node, "ti,version",
> >>>> +                             &input_dev->id.version);
> >>>
> >>> I wonder if anyone cares about this.
> >>
> >> I've looked at other touchscreen drivers and this structure seems to
> >> be filled mostly for the RS232 connected drivers. So I'd suggest just
> >> to leave vendor, product and version at 0 and not provide these values
> >> via DT.  At least tslib and hence Qt is working without these values
> >> set.
> >>
> >> @Kevin: so far the only user of this touch driver is da850-evm board.
> >> Can we drop the values for the id structure? Btw. what else is needed
> 
> It seems like it was mostly added so it can be read through sysfs. But
> since it has been exposed to userspace, I am not sure if it can simply
> be dropped. May be used hardcoded values for these in the driver based
> on compatible string ?

That's what I said. We do know the product and vendor. Version could be
dropped.

-- 
Dmitry

Re: [PATCH 1/3] tps6507x-ts: update to devm_* API

[Dmitry Torokhov]

On Fri, Feb 24, 2017 at 04:42:24PM +0100, yegorslists@googlemail.com wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
> 
> Update the code to use devm_* API so that driver
> core will manage resources.
> 
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
>  drivers/input/touchscreen/tps6507x-ts.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> index a340bfc..5bf1ec6 100644
> --- a/drivers/input/touchscreen/tps6507x-ts.c
> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> @@ -226,7 +226,7 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>  	 */
>  	init_data = tps_board->tps6507x_ts_init_data;
>  
> -	tsc = kzalloc(sizeof(struct tps6507x_ts), GFP_KERNEL);
> +	tsc = devm_kzalloc(&pdev->dev, sizeof(struct tps6507x_ts), GFP_KERNEL);

If you do this, why not use devm_input_allocate_polled_device() as well?
Then you can get rid of tps6507x_ts_remove().

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] tps6507x-ts: update to devm_* API

[Yegor Yefremov]

On Tue, Feb 28, 2017 at 7:57 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Feb 24, 2017 at 04:42:24PM +0100, yegorslists@googlemail.com wrote:
>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>
>> Update the code to use devm_* API so that driver
>> core will manage resources.
>>
>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> ---
>>  drivers/input/touchscreen/tps6507x-ts.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
>> index a340bfc..5bf1ec6 100644
>> --- a/drivers/input/touchscreen/tps6507x-ts.c
>> +++ b/drivers/input/touchscreen/tps6507x-ts.c
>> @@ -226,7 +226,7 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>>        */
>>       init_data = tps_board->tps6507x_ts_init_data;
>>
>> -     tsc = kzalloc(sizeof(struct tps6507x_ts), GFP_KERNEL);
>> +     tsc = devm_kzalloc(&pdev->dev, sizeof(struct tps6507x_ts), GFP_KERNEL);
>
> If you do this, why not use devm_input_allocate_polled_device() as well?
> Then you can get rid of tps6507x_ts_remove().

Will do. The less code the better.

Yegor

Re: [PATCH 2/3] tps6507x-ts: add DT support

[Yegor Yefremov]

On Tue, Feb 28, 2017 at 7:41 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Feb 28, 2017 at 03:36:26PM +0530, Sekhar Nori wrote:
>> On Tuesday 28 February 2017 07:46 AM, Kevin Hilman wrote:
>> > + Sekhar for the board-da850-evm questions.
>> >
>> > Yegor Yefremov <yegorslists@googlemail.com> writes:
>> >
>> >> Hi Dmitry,
>> >>
>> >> On Sat, Feb 25, 2017 at 8:08 PM, Dmitry Torokhov
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >>> Hi Yegor,
>> >>>
>> >>> On Fri, Feb 24, 2017 at 04:42:25PM +0100, yegorslists@googlemail.com wrote:
>> >>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>> >>>>
>> >>>> This patch adds support for DT to tps6507x-ts driver.
>> >>>>
>> >>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> >>>> ---
>> >>>>  drivers/input/touchscreen/tps6507x-ts.c | 70 ++++++++++++++++++++-------------
>> >>>>  1 file changed, 42 insertions(+), 28 deletions(-)
>> >>>>
>> >>>> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
>> >>>> index 5bf1ec6..2d61220 100644
>> >>>> --- a/drivers/input/touchscreen/tps6507x-ts.c
>> >>>> +++ b/drivers/input/touchscreen/tps6507x-ts.c
>> >>>> @@ -22,6 +22,8 @@
>> >>>>  #include <linux/mfd/tps6507x.h>
>> >>>>  #include <linux/input/tps6507x-ts.h>
>> >>>>  #include <linux/delay.h>
>> >>>> +#include <linux/of.h>
>> >>>> +#include <linux/of_device.h>
>> >>>>
>> >>>>  #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
>> >>>>  #define TPS_DEFAULT_MIN_PRESSURE 0x30
>> >>>> @@ -199,33 +201,52 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
>> >>>>       tps6507x_adc_standby(tsc);
>> >>>>  }
>> >>>>
>> >>>> +static void tsc_init_data(struct tps6507x_ts *tsc,
>> >>>> +             struct input_dev *input_dev)
>> >>>> +{
>> >>>> +     struct device_node *node = tsc->dev->of_node;
>> >>>> +     const struct tps6507x_board *tps_board =
>> >>>> +                     dev_get_platdata(tsc->dev);
>> >>>> +     const struct touchscreen_init_data *init_data = NULL;
>> >>>> +
>> >>>> +     if (tps_board)
>> >>>> +             /*
>> >>>> +              * init_data points to array of touchscreen_init structure
>> >>>> +              * coming from the board-evm file.
>> >>>> +              */
>> >>>> +             init_data = tps_board->tps6507x_ts_init_data;
>> >>>> +
>> >>>> +     if (node == NULL && init_data == NULL) {
>> >>>> +             tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
>> >>>> +             tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
>> >>>
>> >>> I think you want to always init with defaults, then override with
>> >>> property data.
>> >>
>> >> ACK
>> >>
>> >>>> +     } else if (init_data) {
>> >>>> +             tsc->poll_dev->poll_interval = init_data->poll_period;
>> >>>> +             tsc->min_pressure = init_data->min_pressure;
>> >>>> +             input_dev->id.vendor = init_data->vendor;
>> >>>> +             input_dev->id.product = init_data->product;
>> >>>> +             input_dev->id.version = init_data->version;
>> >>>> +     } else if (node) {
>> >>>> +             of_property_read_u32(node, "ti,poll-period",
>> >>>> +                             &tsc->poll_dev->poll_interval);
>> >>>> +             of_property_read_u16(node, "ti,min-pressure",
>> >>>> +                             &tsc->min_pressure);
>> >>>
>> >>> Anything but u32 is better avoided in DT as it requires special
>> >>> annotation which is easily forgotten.
>> >>
>> >> Will convert to 32-bit values.
>> >>
>> >>> I also would prefer if we switched to generic device properties and
>> >>> retired the static init data.
>> >>
>> >> do you mean converting the driver to DT-only variant?
>
> Not to DT only. device_property_read_* API works on DT, ACPI and legacy
> boards that can be converted to use property sets (see
> device_add_properties() and struct property_entry API).

OK. I have found some examples in the board files.

>> >>
>> >>>> +             of_property_read_u16(node, "ti,vendor",
>> >>>> +                             &input_dev->id.vendor);
>> >>>> +             of_property_read_u16(node, "ti,product",
>> >>>> +                             &input_dev->id.product);
>> >>>
>> >>> Would we not know vendor and product from the compatible string?
>> >>>
>> >>>> +             of_property_read_u16(node, "ti,version",
>> >>>> +                             &input_dev->id.version);
>> >>>
>> >>> I wonder if anyone cares about this.
>> >>
>> >> I've looked at other touchscreen drivers and this structure seems to
>> >> be filled mostly for the RS232 connected drivers. So I'd suggest just
>> >> to leave vendor, product and version at 0 and not provide these values
>> >> via DT.  At least tslib and hence Qt is working without these values
>> >> set.
>> >>
>> >> @Kevin: so far the only user of this touch driver is da850-evm board.
>> >> Can we drop the values for the id structure? Btw. what else is needed
>>
>> It seems like it was mostly added so it can be read through sysfs. But
>> since it has been exposed to userspace, I am not sure if it can simply
>> be dropped. May be used hardcoded values for these in the driver based
>> on compatible string ?
>
> That's what I said. We do know the product and vendor. Version could be
> dropped.

Do you mean this string? compatible = "ti,tps6507x"?

This is our current configuration taken from
arch/arm/mach-davinci/board-da850-evm.c:

        .vendor = 0,            /* /sys/class/input/input?/id/vendor
*/                                               |
ti,version = /bits/ 16 <0x100>;
        .product = 65070,       /* /sys/class/input/input?/id/product
*/                                              |
        .version = 0x100,       /* /sys/class/input/input?/id/version */

We can leave product by 65070, set version to 0, but what to do with
the vendor ID? What is TI's vendor ID? Is this 0x0451 as mentioned in
this forum [1]?

[1] https://e2e.ti.com/support/microcontrollers/msp430/f/166/t/18974

Yegor

Re: [PATCH 3/3] tps6507x-ts: add DT bindings description

[Lee Jones]

On Fri, 24 Feb 2017, yegorslists@googlemail.com wrote:

> From: Yegor Yefremov <yegorslists@googlemail.com>
> 
> Provide description for following properties:
> 
> - ti,poll-period
> - ti,min-pressure
> - ti,vendor
> - ti,product
> - ti,version
> 
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 30 ++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> index 8fffa3c..eb654ef 100644
> --- a/Documentation/devicetree/bindings/mfd/tps6507x.txt
> +++ b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> @@ -1,4 +1,8 @@
> -TPS6507x Power Management Integrated Circuit
> +TPS6507x Multifunctional Device.
> +
> +Features provided by TPS6507x:
> +        1. Power Management Integrated Circuit.
> +        2. Touch-Screen.
>  
>  Required properties:
>  - compatible: "ti,tps6507x"
> @@ -30,6 +34,23 @@ Regulator Optional properties:
>  			1: If defdcdc pin of DCDC2/DCDC3 is driven HIGH.
>    If this property is not defined, it defaults to 0 (not enabled).
>  
> +Touchscreen Optional properties:
> +- ti,poll-period: Time at which touch input is getting sampled in ms.
> +		  Default value: 30 ms.
> +- ti,min-pressure: Minimum pressure value to trigger touch.
> +		   Default value: 0x30.
> +		   This property has to be a '/bits/ 16' value
> +
> +Entries below describe the input_id structure [1] for the touch device:
> +- ti,vendor: Touchscreen vendor id.
> +	     This property has to be a '/bits/ 16' value
> +- ti,product: Touchscreen product id.
> +	      This property has to be a '/bits/ 16' value
> +- ti,version: Touchscreen version id.
> +	      This property has to be a '/bits/ 16' value
> +
> +[1] include/uapi/linux/input.h
> +

I think this should live in the touchscreen binding doc.

>  Example:
>  
>  	pmu: tps6507x@48 {
> @@ -40,6 +61,12 @@ Example:
>  		vindcdc3-supply = <...>;
>  		vinldo1_2-supply = <...>;
>  
> +		ti,poll-period = <30>;
> +		ti,min-pressure = /bits/ 16 <0x30>;
> +		ti,vendor = /bits/ 16 <0>;
> +		ti,product = /bits/ 16 <65070>;
> +		ti,version = /bits/ 16 <0x100>;
> +
>  		regulators {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> @@ -87,5 +114,4 @@ Example:
>  				regulator-boot-on;
>  			};
>  		};
> -
>  	};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog