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

[PATCH v2 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].

Changes since V2:
 - use devm_input_allocate_polled_device and remove tps6507x_ts_remove
 - device_property_read_* API to get poll_interval and min_pressure
 - remove id values from DT and make them static

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 | 16 ++++-
 drivers/input/touchscreen/tps6507x-ts.c            | 71 ++++++----------------
 2 files changed, 31 insertions(+), 56 deletions(-)

-- 
2.1.4

[PATCH v2 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 | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
index a340bfc..fbaa2f68 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;
@@ -240,11 +240,10 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 	snprintf(tsc->phys, sizeof(tsc->phys),
 		 "%s/input0", dev_name(tsc->dev));
 
-	poll_dev = input_allocate_polled_device();
+	poll_dev = devm_input_allocate_polled_device(&pdev->dev);
 	if (!poll_dev) {
 		dev_err(tsc->dev, "Failed to allocate polled input device.\n");
-		error = -ENOMEM;
-		goto err_free_mem;
+		return -ENOMEM;
 	}
 
 	tsc->poll_dev = poll_dev;
@@ -274,34 +273,15 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 
 	error = tps6507x_adc_standby(tsc);
 	if (error)
-		goto err_free_polled_dev;
+		return error;
 
 	error = input_register_polled_device(poll_dev);
 	if (error)
-		goto err_free_polled_dev;
+		return error;
 
 	platform_set_drvdata(pdev, tsc);
 
 	return 0;
-
-err_free_polled_dev:
-	input_free_polled_device(poll_dev);
-err_free_mem:
-	kfree(tsc);
-	return error;
-}
-
-static int tps6507x_ts_remove(struct platform_device *pdev)
-{
-	struct tps6507x_ts *tsc = platform_get_drvdata(pdev);
-	struct input_polled_dev *poll_dev = tsc->poll_dev;
-
-	input_unregister_polled_device(poll_dev);
-	input_free_polled_device(poll_dev);
-
-	kfree(tsc);
-
-	return 0;
 }
 
 static struct platform_driver tps6507x_ts_driver = {
@@ -309,7 +289,6 @@ static struct platform_driver tps6507x_ts_driver = {
 		.name = "tps6507x-ts",
 	},
 	.probe = tps6507x_ts_probe,
-	.remove = tps6507x_ts_remove,
 };
 module_platform_driver(tps6507x_ts_driver);
 
-- 
2.1.4

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

[Dmitry Torokhov]

On Thu, Mar 09, 2017 at 03:31:07PM +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>


Applied, thank you. We can continue discussing DT conversion, but this
patch is good as is.

Thanks.

-- 
Dmitry

[PATCH v2 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 | 40 ++++++++++-----------------------
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
index fbaa2f68..502ed0c 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
@@ -202,30 +204,11 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
 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 +217,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));
@@ -250,8 +231,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);
@@ -265,11 +244,16 @@ 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;
-	}
+	input_dev->id.vendor = 0;
+	input_dev->id.product = 65070;
+	input_dev->id.version = 0x100;
+
+	tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
+	tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
+	device_property_read_u32_array(tsc->dev, "ti,poll-period",
+				       &tsc->poll_dev->poll_interval, 1);
+	device_property_read_u16_array(tsc->dev, "ti,min-pressure",
+				       &tsc->min_pressure, 1);
 
 	error = tps6507x_adc_standby(tsc);
 	if (error)
-- 
2.1.4

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

[Yegor Yefremov]

Hi Dmitry, Sekhar,

On Thu, Mar 9, 2017 at 3:31 PM,  <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 | 40 ++++++++++-----------------------
>  1 file changed, 12 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> index fbaa2f68..502ed0c 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
> @@ -202,30 +204,11 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
>  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 +217,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));
> @@ -250,8 +231,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);
> @@ -265,11 +244,16 @@ 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;
> -       }
> +       input_dev->id.vendor = 0;
> +       input_dev->id.product = 65070;
> +       input_dev->id.version = 0x100;

I've left the data as is to preserve current behavior until we decide
on what exact IDs to take.

> +       tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
> +       tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
> +       device_property_read_u32_array(tsc->dev, "ti,poll-period",
> +                                      &tsc->poll_dev->poll_interval, 1);
> +       device_property_read_u16_array(tsc->dev, "ti,min-pressure",
> +                                      &tsc->min_pressure, 1);

This is working for DT, but I have problems with converting
arch/arm/mach-davinci/board-da850-evm.c

tps is a i2c device. How do I obtain struct device *dev from i2c_board?

Yegor

>         error = tps6507x_adc_standby(tsc);
>         if (error)
> --
> 2.1.4
>

[PATCH v2 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

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

diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
index 8fffa3c..8875662 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,12 @@ 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.
+
 Example:
 
 	pmu: tps6507x@48 {
@@ -40,6 +50,9 @@ Example:
 		vindcdc3-supply = <...>;
 		vinldo1_2-supply = <...>;
 
+		ti,poll-period = <30>;
+		ti,min-pressure = <0x30>;
+
 		regulators {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -87,5 +100,4 @@ Example:
 				regulator-boot-on;
 			};
 		};
-
 	};
-- 
2.1.4

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

[Rob Herring]

On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>

This needs to go to DT list.

>
> Provide description for following properties:
>
> - ti,poll-period
> - ti,min-pressure
>
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> index 8fffa3c..8875662 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,12 @@ 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.

Isn't there a standard property for this?

> +- ti,min-pressure: Minimum pressure value to trigger touch.
> +                  Default value: 0x30.
> +
>  Example:
>
>         pmu: tps6507x@48 {
> @@ -40,6 +50,9 @@ Example:
>                 vindcdc3-supply = <...>;
>                 vinldo1_2-supply = <...>;
>
> +               ti,poll-period = <30>;
> +               ti,min-pressure = <0x30>;
> +
>                 regulators {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> @@ -87,5 +100,4 @@ Example:
>                                 regulator-boot-on;
>                         };
>                 };
> -
>         };
> --
> 2.1.4
>

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

[Yegor Yefremov]

On Thu, Mar 9, 2017 at 3:49 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
>> From: Yegor Yefremov <yegorslists@googlemail.com>
>
> This needs to go to DT list.

Will do.

>>
>> Provide description for following properties:
>>
>> - ti,poll-period
>> - ti,min-pressure
>>
>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> ---
>>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
>> index 8fffa3c..8875662 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,12 @@ 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.
>
> Isn't there a standard property for this?

Such a value will be already used in [1]. I've looked at [2], but it
doesn't have such a field.

[1] Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
[2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

>> +- ti,min-pressure: Minimum pressure value to trigger touch.
>> +                  Default value: 0x30.
>> +
>>  Example:
>>
>>         pmu: tps6507x@48 {
>> @@ -40,6 +50,9 @@ Example:
>>                 vindcdc3-supply = <...>;
>>                 vinldo1_2-supply = <...>;
>>
>> +               ti,poll-period = <30>;
>> +               ti,min-pressure = <0x30>;
>> +
>>                 regulators {
>>                         #address-cells = <1>;
>>                         #size-cells = <0>;
>> @@ -87,5 +100,4 @@ Example:
>>                                 regulator-boot-on;
>>                         };
>>                 };
>> -
>>         };
>> --
>> 2.1.4
>>

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

[Dmitry Torokhov]

On Thu, Mar 09, 2017 at 04:18:40PM +0100, Yegor Yefremov wrote:
> On Thu, Mar 9, 2017 at 3:49 PM, Rob Herring <robh@kernel.org> wrote:
> > On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
> >> From: Yegor Yefremov <yegorslists@googlemail.com>
> >
> > This needs to go to DT list.
> 
> Will do.
> 
> >>
> >> Provide description for following properties:
> >>
> >> - ti,poll-period
> >> - ti,min-pressure
> >>
> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> >> ---
> >>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> >> index 8fffa3c..8875662 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,12 @@ 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.
> >
> > Isn't there a standard property for this?
> 
> Such a value will be already used in [1]. I've looked at [2], but it
> doesn't have such a field.
> 
> [1] Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> [2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

I think the bigger question (since we are adding this to DT I assume it
is not longer "toy" driver) can we convert it to a proper
interrupt-driven scheme? Polling for real shipping devices is not
efficient, I do not think we should even try to allow this mode in DTS
bindings (note that tsc2007 use is different: it allows to specify
sampling interval once touch is detected).

Thanks.

-- 
Dmitry

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

[Sekhar Nori]

On Thursday 09 March 2017 11:25 PM, Dmitry Torokhov wrote:
> On Thu, Mar 09, 2017 at 04:18:40PM +0100, Yegor Yefremov wrote:
>> On Thu, Mar 9, 2017 at 3:49 PM, Rob Herring <robh@kernel.org> wrote:
>>> On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
>>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>>
>>> This needs to go to DT list.
>>
>> Will do.
>>
>>>>
>>>> Provide description for following properties:
>>>>
>>>> - ti,poll-period
>>>> - ti,min-pressure
>>>>
>>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
>>>> index 8fffa3c..8875662 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,12 @@ 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.
>>>
>>> Isn't there a standard property for this?
>>
>> Such a value will be already used in [1]. I've looked at [2], but it
>> doesn't have such a field.
>>
>> [1] Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> [2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> 
> I think the bigger question (since we are adding this to DT I assume it
> is not longer "toy" driver) can we convert it to a proper
> interrupt-driven scheme? Polling for real shipping devices is not
> efficient, I do not think we should even try to allow this mode in DTS
> bindings (note that tsc2007 use is different: it allows to specify
> sampling interval once touch is detected).

Unfortunately, the only current user of tps6507x in kernel (da850-evm)
does not have the touch interrupt connected to the processor. So if we
dont support polled mode in when using device-tree, da850-evm.dts wont
be able to use this driver.

Thanks,
Sekhar

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

[Dmitry Torokhov]

On Fri, Mar 10, 2017 at 12:11:08PM +0530, Sekhar Nori wrote:
> On Thursday 09 March 2017 11:25 PM, Dmitry Torokhov wrote:
> > On Thu, Mar 09, 2017 at 04:18:40PM +0100, Yegor Yefremov wrote:
> >> On Thu, Mar 9, 2017 at 3:49 PM, Rob Herring <robh@kernel.org> wrote:
> >>> On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
> >>>> From: Yegor Yefremov <yegorslists@googlemail.com>
> >>>
> >>> This needs to go to DT list.
> >>
> >> Will do.
> >>
> >>>>
> >>>> Provide description for following properties:
> >>>>
> >>>> - ti,poll-period
> >>>> - ti,min-pressure
> >>>>
> >>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
> >>>>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> >>>> index 8fffa3c..8875662 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,12 @@ 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.
> >>>
> >>> Isn't there a standard property for this?
> >>
> >> Such a value will be already used in [1]. I've looked at [2], but it
> >> doesn't have such a field.
> >>
> >> [1] Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> >> [2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> > 
> > I think the bigger question (since we are adding this to DT I assume it
> > is not longer "toy" driver) can we convert it to a proper
> > interrupt-driven scheme? Polling for real shipping devices is not
> > efficient, I do not think we should even try to allow this mode in DTS
> > bindings (note that tsc2007 use is different: it allows to specify
> > sampling interval once touch is detected).
> 
> Unfortunately, the only current user of tps6507x in kernel (da850-evm)
> does not have the touch interrupt connected to the processor. So if we
> dont support polled mode in when using device-tree, da850-evm.dts wont
> be able to use this driver.

Do we care? Right now the driver is unusable for any practical
application I think as it burns CPU cycles by constantly polling the
touch controller.

Yegor, your conversion to device tree: is it done because you have a
real product using this?

Thanks.

-- 
Dmitry

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

[Yegor Yefremov]

On Fri, Mar 10, 2017 at 7:00 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Mar 10, 2017 at 12:11:08PM +0530, Sekhar Nori wrote:
>> On Thursday 09 March 2017 11:25 PM, Dmitry Torokhov wrote:
>> > On Thu, Mar 09, 2017 at 04:18:40PM +0100, Yegor Yefremov wrote:
>> >> On Thu, Mar 9, 2017 at 3:49 PM, Rob Herring <robh@kernel.org> wrote:
>> >>> On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
>> >>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>> >>>
>> >>> This needs to go to DT list.
>> >>
>> >> Will do.
>> >>
>> >>>>
>> >>>> Provide description for following properties:
>> >>>>
>> >>>> - ti,poll-period
>> >>>> - ti,min-pressure
>> >>>>
>> >>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> >>>> ---
>> >>>>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
>> >>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>> >>>>
>> >>>> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
>> >>>> index 8fffa3c..8875662 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,12 @@ 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.
>> >>>
>> >>> Isn't there a standard property for this?
>> >>
>> >> Such a value will be already used in [1]. I've looked at [2], but it
>> >> doesn't have such a field.
>> >>
>> >> [1] Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> >> [2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>> >
>> > I think the bigger question (since we are adding this to DT I assume it
>> > is not longer "toy" driver) can we convert it to a proper
>> > interrupt-driven scheme? Polling for real shipping devices is not
>> > efficient, I do not think we should even try to allow this mode in DTS
>> > bindings (note that tsc2007 use is different: it allows to specify
>> > sampling interval once touch is detected).
>>
>> Unfortunately, the only current user of tps6507x in kernel (da850-evm)
>> does not have the touch interrupt connected to the processor. So if we
>> dont support polled mode in when using device-tree, da850-evm.dts wont
>> be able to use this driver.
>
> Do we care? Right now the driver is unusable for any practical
> application I think as it burns CPU cycles by constantly polling the
> touch controller.
>
> Yegor, your conversion to device tree: is it done because you have a
> real product using this?

Yes. It is VS-860 device http://www.visionsystems.de/produkte/vs-860.html

It even seems to have interrupt pin connected. So we have to support both modes.

Yegor

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

[Dmitry Torokhov]

On Fri, Mar 10, 2017 at 08:54:51PM +0100, Yegor Yefremov wrote:
> On Fri, Mar 10, 2017 at 7:00 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Mar 10, 2017 at 12:11:08PM +0530, Sekhar Nori wrote:
> >> On Thursday 09 March 2017 11:25 PM, Dmitry Torokhov wrote:
> >> > On Thu, Mar 09, 2017 at 04:18:40PM +0100, Yegor Yefremov wrote:
> >> >> On Thu, Mar 9, 2017 at 3:49 PM, Rob Herring <robh@kernel.org> wrote:
> >> >>> On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
> >> >>>> From: Yegor Yefremov <yegorslists@googlemail.com>
> >> >>>
> >> >>> This needs to go to DT list.
> >> >>
> >> >> Will do.
> >> >>
> >> >>>>
> >> >>>> Provide description for following properties:
> >> >>>>
> >> >>>> - ti,poll-period
> >> >>>> - ti,min-pressure
> >> >>>>
> >> >>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> >> >>>> ---
> >> >>>>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
> >> >>>>  1 file changed, 14 insertions(+), 2 deletions(-)
> >> >>>>
> >> >>>> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> >> >>>> index 8fffa3c..8875662 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,12 @@ 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.
> >> >>>
> >> >>> Isn't there a standard property for this?
> >> >>
> >> >> Such a value will be already used in [1]. I've looked at [2], but it
> >> >> doesn't have such a field.
> >> >>
> >> >> [1] Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> >> >> [2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
> >> >
> >> > I think the bigger question (since we are adding this to DT I assume it
> >> > is not longer "toy" driver) can we convert it to a proper
> >> > interrupt-driven scheme? Polling for real shipping devices is not
> >> > efficient, I do not think we should even try to allow this mode in DTS
> >> > bindings (note that tsc2007 use is different: it allows to specify
> >> > sampling interval once touch is detected).
> >>
> >> Unfortunately, the only current user of tps6507x in kernel (da850-evm)
> >> does not have the touch interrupt connected to the processor. So if we
> >> dont support polled mode in when using device-tree, da850-evm.dts wont
> >> be able to use this driver.
> >
> > Do we care? Right now the driver is unusable for any practical
> > application I think as it burns CPU cycles by constantly polling the
> > touch controller.
> >
> > Yegor, your conversion to device tree: is it done because you have a
> > real product using this?
> 
> Yes. It is VS-860 device http://www.visionsystems.de/produkte/vs-860.html
> 
> It even seems to have interrupt pin connected. So we have to support both modes.

Well, I'd rather supported interrupt only. Polling is a toy mode.

Thanks.

-- 
Dmitry

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

[Sekhar Nori]

Hi Dmitry,

On Saturday 11 March 2017 01:38 AM, Dmitry Torokhov wrote:
> On Fri, Mar 10, 2017 at 08:54:51PM +0100, Yegor Yefremov wrote:
>> On Fri, Mar 10, 2017 at 7:00 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>> On Fri, Mar 10, 2017 at 12:11:08PM +0530, Sekhar Nori wrote:
>>>> On Thursday 09 March 2017 11:25 PM, Dmitry Torokhov wrote:
>>>>> On Thu, Mar 09, 2017 at 04:18:40PM +0100, Yegor Yefremov wrote:
>>>>>> On Thu, Mar 9, 2017 at 3:49 PM, Rob Herring <robh@kernel.org> wrote:
>>>>>>> On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
>>>>>>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>>>>>>
>>>>>>> This needs to go to DT list.
>>>>>>
>>>>>> Will do.
>>>>>>
>>>>>>>>
>>>>>>>> Provide description for following properties:
>>>>>>>>
>>>>>>>> - ti,poll-period
>>>>>>>> - ti,min-pressure
>>>>>>>>
>>>>>>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>>>>>>>> ---
>>>>>>>>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
>>>>>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
>>>>>>>> index 8fffa3c..8875662 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,12 @@ 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.
>>>>>>>
>>>>>>> Isn't there a standard property for this?
>>>>>>
>>>>>> Such a value will be already used in [1]. I've looked at [2], but it
>>>>>> doesn't have such a field.
>>>>>>
>>>>>> [1] Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>>>>>> [2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>>>>>
>>>>> I think the bigger question (since we are adding this to DT I assume it
>>>>> is not longer "toy" driver) can we convert it to a proper
>>>>> interrupt-driven scheme? Polling for real shipping devices is not
>>>>> efficient, I do not think we should even try to allow this mode in DTS
>>>>> bindings (note that tsc2007 use is different: it allows to specify
>>>>> sampling interval once touch is detected).
>>>>
>>>> Unfortunately, the only current user of tps6507x in kernel (da850-evm)
>>>> does not have the touch interrupt connected to the processor. So if we
>>>> dont support polled mode in when using device-tree, da850-evm.dts wont
>>>> be able to use this driver.
>>>
>>> Do we care? Right now the driver is unusable for any practical
>>> application I think as it burns CPU cycles by constantly polling the
>>> touch controller.
>>>
>>> Yegor, your conversion to device tree: is it done because you have a
>>> real product using this?
>>
>> Yes. It is VS-860 device http://www.visionsystems.de/produkte/vs-860.html
>>
>> It even seems to have interrupt pin connected. So we have to support both modes.
> 
> Well, I'd rather supported interrupt only. Polling is a toy mode.

On my DA850 EVM, running

$ evtest /dev/input/event0  > /dev/null &

and monitoring cpu load using top while continuously touching the screen
takes the CPU to about 90% idle. This is against 98% idle with no touch.
This running on ARM9 at 300Mhz. The processor can do about 450Mhz.

So, while I agree polling mode is not ideal, its not completely useless
as well. It will help the EVM to move to DT completely. Also, this
should not affect any core infrastructure or even interrupt driven users
of this driver.

Please do reconsider the stance on interrupt-only DT mode.

Thanks,
Sekhar

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

[Rob Herring]

On Thu, Mar 9, 2017 at 11:55 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Mar 09, 2017 at 04:18:40PM +0100, Yegor Yefremov wrote:
>> On Thu, Mar 9, 2017 at 3:49 PM, Rob Herring <robh@kernel.org> wrote:
>> > On Thu, Mar 9, 2017 at 8:31 AM,  <yegorslists@googlemail.com> wrote:
>> >> From: Yegor Yefremov <yegorslists@googlemail.com>
>> >
>> > This needs to go to DT list.
>>
>> Will do.
>>
>> >>
>> >> Provide description for following properties:
>> >>
>> >> - ti,poll-period
>> >> - ti,min-pressure
>> >>
>> >> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> >> ---
>> >>  Documentation/devicetree/bindings/mfd/tps6507x.txt | 16 ++++++++++++++--
>> >>  1 file changed, 14 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
>> >> index 8fffa3c..8875662 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,12 @@ 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.
>> >
>> > Isn't there a standard property for this?
>>
>> Such a value will be already used in [1]. I've looked at [2], but it
>> doesn't have such a field.
>>
>> [1] Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>> [2] Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>
> I think the bigger question (since we are adding this to DT I assume it
> is not longer "toy" driver) can we convert it to a proper
> interrupt-driven scheme? Polling for real shipping devices is not
> efficient, I do not think we should even try to allow this mode in DTS
> bindings (note that tsc2007 use is different: it allows to specify
> sampling interval once touch is detected).

I guess I was assuming that polling period corresponded to sampling
period or rate. The latter makes more sense for a binding and I would
think is fairly common whether the h/w has functionality or the driver
has to implement the sampling rate in s/w.

Rob