[PATCH V3 0/2] Add DT support for tps6507x touchscreen

[PATCH V3 0/2] Add DT support for tps6507x touchscreen

[Vishwanathrao Badarkhe, Manish]

Patch set adds DT support for tps6507x based touchscreen.
Also, add DT data for tps6507x touchscreen in da850-evm by
providing touchscreen node inside tps6507x mfd device.

This patch series applies on top of linux-next tree and depends on [1].

[1]tps6507x-ts: update to devm_* API
   https://patchwork.kernel.org/patch/2324441/

Tested on da850-evm board.

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.

Vishwanathrao Badarkhe, Manish (2):
  tps6507x-ts: Add DT support
  ARM: davinci: da850: add tps6507x touchscreen DT data

 Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
 arch/arm/boot/dts/da850-evm.dts                    |    9 ++
 drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
 3 files changed, 104 insertions(+), 31 deletions(-)

-- 
1.7.4.1

[PATCH V3 0/2] Add DT support for tps6507x touchscreen

[Vishwanathrao Badarkhe, Manish]

Patch set adds DT support for tps6507x based touchscreen.
Also, add DT data for tps6507x touchscreen in da850-evm by
providing touchscreen node inside tps6507x mfd device.

This patch series applies on top of linux-next tree and depends on [1].

[1]tps6507x-ts: update to devm_* API
   https://patchwork.kernel.org/patch/2324441/

Tested on da850-evm board.

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.

Vishwanathrao Badarkhe, Manish (2):
  tps6507x-ts: Add DT support
  ARM: davinci: da850: add tps6507x touchscreen DT data

 Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
 arch/arm/boot/dts/da850-evm.dts                    |    9 ++
 drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
 3 files changed, 104 insertions(+), 31 deletions(-)

-- 
1.7.4.1

[PATCH V3 1/2] tps6507x-ts: Add DT support

[Vishwanathrao Badarkhe, Manish]

Add device tree based support for TI's tps6507x touchscreen.

Tested on da850-evm.

Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
---
Changes since V2:
 - Removed unnecessary code.
 - Updated Documentation to provide proper names of
   devicetree properties.

Changes since V1:
 - Updated documentation to specify tps6507x as multifunctional
   device.
 - return proper error value in absence of platform and DT
   data for touchscreen.
 - Updated commit message.

:100755 100755 8fffa3c... 65ee2cd... M	Documentation/devicetree/bindings/mfd/tps6507x.txt
:100644 100644 65e0f9a... 89232ee... M	drivers/input/touchscreen/tps6507x-ts.c
 Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
 drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
 2 files changed, 95 insertions(+), 31 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
index 8fffa3c..65ee2cd 100755
--- 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"
@@ -23,6 +27,12 @@ Required properties:
        vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
        vindcdc3-supply  : VDCDC3 input.
        vldo1_2-supply   : VLDO1 and VLDO2 input.
+- tsc: This node specifies touch screen data.
+	ti,poll-period : Time at which touch input is getting sampled in ms.
+	ti,min-pressure: Minimum pressure value to trigger touch.
+	ti,vref	       : voltage reference for ADC.
+		      0: Reference voltage for ADC is disabled.
+		      1: Reference voltage for ADC is enabled.
 
 Regulator Optional properties:
 - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
@@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
+	      in sysclass interface.
+- ti,product: Touchscreen product id to populate
+	      in sysclass interface.
+- ti,version: Touchscreen version id to populate
+	      in sysclass interface.
+
 Example:
 
 	pmu: tps6507x@48 {
@@ -88,4 +106,12 @@ Example:
 			};
 		};
 
+		tsc {
+			ti,poll-period = <30>;
+			ti,min-pressure = <0x30>;
+			ti,vref = <0>;
+			ti,vendor = <0>;
+			ti,product = <65070>;
+			ti,version = <0x100>;
+		};
 	};
diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
index 65e0f9a..89232ee 100644
--- a/drivers/input/touchscreen/tps6507x-ts.c
+++ b/drivers/input/touchscreen/tps6507x-ts.c
@@ -21,6 +21,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
@@ -231,36 +233,76 @@ done:
 	ret = tps6507x_adc_standby(tsc);
 }
 
+static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
+		struct input_dev *input_dev)
+{
+	struct device_node *node = tps6507x_dev->dev->of_node;
+	struct tps6507x_board *tps_board =
+		(struct tps6507x_board *)tps6507x_dev->dev->platform_data;
+	struct touchscreen_init_data *init_data = NULL;
+	int err;
+
+	if (node)
+		node = of_find_node_by_name(node, "tsc");
+	if (tps_board)
+		init_data = tps_board->tps6507x_ts_init_data;
+
+	if (node == NULL || init_data == NULL) {
+		err = -EINVAL;
+		goto error_ret;
+	} else if (init_data) {
+		tps6507x_dev->ts->poll_period = init_data->poll_period;
+		tps6507x_dev->ts->min_pressure = init_data->min_pressure;
+		tps6507x_dev->ts->vref = init_data->vref;
+		input_dev->id.vendor = init_data->vendor;
+		input_dev->id.product = init_data->product;
+		input_dev->id.version = init_data->version;
+	} else if (node) {
+		err = of_property_read_u32(node, "ti,poll-period",
+					(u32 *)&tps6507x_dev->ts->poll_period);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u16(node, "ti,min-pressure",
+					&tps6507x_dev->ts->min_pressure);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u32(node, "ti,vref",
+					&tps6507x_dev->ts->vref);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u16(node, "ti,vendor",
+					&input_dev->id.vendor);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u16(node, "ti,product",
+					&input_dev->id.product);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u16(node, "ti,version",
+					&input_dev->id.version);
+		if (err < 0)
+			goto error_ret;
+	}
+
+	return 0;
+
+error_ret:
+	return err;
+}
+
 static int tps6507x_ts_probe(struct platform_device *pdev)
 {
 	int error;
 	struct tps6507x_ts *tsc;
 	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
-	struct touchscreen_init_data *init_data;
 	struct input_dev *input_dev;
-	struct tps6507x_board *tps_board;
 	int schd;
 
-	/**
-	 * tps_board points to pmic related constants
-	 * coming from the board-evm file.
-	 */
-
-	tps_board = (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
-
-	if (!tps_board) {
-		dev_err(tps6507x_dev->dev,
-			"Could not find tps6507x platform data\n");
-		return -EIO;
-	}
-
-	/**
-	 * 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");
@@ -301,14 +343,10 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&tsc->work, tps6507x_ts_handler);
 
-	if (init_data) {
-		tsc->poll_period = init_data->poll_period;
-		tsc->vref = init_data->vref;
-		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 {
+	error = tsc_init_data(tps6507x_dev, input_dev);
+
+	if (error) {
+		dev_warn(&pdev->dev, "Use default platform data\n");
 		tsc->poll_period = TSC_DEFAULT_POLL_PERIOD;
 		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
 	}
-- 
1.7.4.1

[PATCH V3 1/2] tps6507x-ts: Add DT support

[Vishwanathrao Badarkhe, Manish]

Add device tree based support for TI's tps6507x touchscreen.

Tested on da850-evm.

Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
---
Changes since V2:
 - Removed unnecessary code.
 - Updated Documentation to provide proper names of
   devicetree properties.

Changes since V1:
 - Updated documentation to specify tps6507x as multifunctional
   device.
 - return proper error value in absence of platform and DT
   data for touchscreen.
 - Updated commit message.

:100755 100755 8fffa3c... 65ee2cd... M	Documentation/devicetree/bindings/mfd/tps6507x.txt
:100644 100644 65e0f9a... 89232ee... M	drivers/input/touchscreen/tps6507x-ts.c
 Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
 drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
 2 files changed, 95 insertions(+), 31 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
index 8fffa3c..65ee2cd 100755
--- 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"
@@ -23,6 +27,12 @@ Required properties:
        vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
        vindcdc3-supply  : VDCDC3 input.
        vldo1_2-supply   : VLDO1 and VLDO2 input.
+- tsc: This node specifies touch screen data.
+	ti,poll-period : Time at which touch input is getting sampled in ms.
+	ti,min-pressure: Minimum pressure value to trigger touch.
+	ti,vref	       : voltage reference for ADC.
+		      0: Reference voltage for ADC is disabled.
+		      1: Reference voltage for ADC is enabled.
 
 Regulator Optional properties:
 - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
@@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
+	      in sysclass interface.
+- ti,product: Touchscreen product id to populate
+	      in sysclass interface.
+- ti,version: Touchscreen version id to populate
+	      in sysclass interface.
+
 Example:
 
 	pmu: tps6507x@48 {
@@ -88,4 +106,12 @@ Example:
 			};
 		};
 
+		tsc {
+			ti,poll-period = <30>;
+			ti,min-pressure = <0x30>;
+			ti,vref = <0>;
+			ti,vendor = <0>;
+			ti,product = <65070>;
+			ti,version = <0x100>;
+		};
 	};
diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
index 65e0f9a..89232ee 100644
--- a/drivers/input/touchscreen/tps6507x-ts.c
+++ b/drivers/input/touchscreen/tps6507x-ts.c
@@ -21,6 +21,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
@@ -231,36 +233,76 @@ done:
 	ret = tps6507x_adc_standby(tsc);
 }
 
+static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
+		struct input_dev *input_dev)
+{
+	struct device_node *node = tps6507x_dev->dev->of_node;
+	struct tps6507x_board *tps_board =
+		(struct tps6507x_board *)tps6507x_dev->dev->platform_data;
+	struct touchscreen_init_data *init_data = NULL;
+	int err;
+
+	if (node)
+		node = of_find_node_by_name(node, "tsc");
+	if (tps_board)
+		init_data = tps_board->tps6507x_ts_init_data;
+
+	if (node == NULL || init_data == NULL) {
+		err = -EINVAL;
+		goto error_ret;
+	} else if (init_data) {
+		tps6507x_dev->ts->poll_period = init_data->poll_period;
+		tps6507x_dev->ts->min_pressure = init_data->min_pressure;
+		tps6507x_dev->ts->vref = init_data->vref;
+		input_dev->id.vendor = init_data->vendor;
+		input_dev->id.product = init_data->product;
+		input_dev->id.version = init_data->version;
+	} else if (node) {
+		err = of_property_read_u32(node, "ti,poll-period",
+					(u32 *)&tps6507x_dev->ts->poll_period);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u16(node, "ti,min-pressure",
+					&tps6507x_dev->ts->min_pressure);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u32(node, "ti,vref",
+					&tps6507x_dev->ts->vref);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u16(node, "ti,vendor",
+					&input_dev->id.vendor);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u16(node, "ti,product",
+					&input_dev->id.product);
+		if (err < 0)
+			goto error_ret;
+
+		err = of_property_read_u16(node, "ti,version",
+					&input_dev->id.version);
+		if (err < 0)
+			goto error_ret;
+	}
+
+	return 0;
+
+error_ret:
+	return err;
+}
+
 static int tps6507x_ts_probe(struct platform_device *pdev)
 {
 	int error;
 	struct tps6507x_ts *tsc;
 	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
-	struct touchscreen_init_data *init_data;
 	struct input_dev *input_dev;
-	struct tps6507x_board *tps_board;
 	int schd;
 
-	/**
-	 * tps_board points to pmic related constants
-	 * coming from the board-evm file.
-	 */
-
-	tps_board = (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
-
-	if (!tps_board) {
-		dev_err(tps6507x_dev->dev,
-			"Could not find tps6507x platform data\n");
-		return -EIO;
-	}
-
-	/**
-	 * 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");
@@ -301,14 +343,10 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&tsc->work, tps6507x_ts_handler);
 
-	if (init_data) {
-		tsc->poll_period = init_data->poll_period;
-		tsc->vref = init_data->vref;
-		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 {
+	error = tsc_init_data(tps6507x_dev, input_dev);
+
+	if (error) {
+		dev_warn(&pdev->dev, "Use default platform data\n");
 		tsc->poll_period = TSC_DEFAULT_POLL_PERIOD;
 		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
 	}
-- 
1.7.4.1

[PATCH V3 2/2] ARM: davinci: da850: add tps6507x touchscreen DT data

[Vishwanathrao Badarkhe, Manish]

Add tps6507x touchscreen DT node to da850-evm.
Touchscreen DT data is added as per da850 board file.

Tested on da850-evm.

Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
---
Changes since V2:
 - Updated names of devicetree properties.
Changes since V1:
 - Updated commit message.

:100644 100644 c914357... 1ca1691... M	arch/arm/boot/dts/da850-evm.dts
 arch/arm/boot/dts/da850-evm.dts |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index c914357..1ca1691 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -155,4 +155,13 @@
 			regulator-boot-on;
 		};
 	};
+
+	tsc {
+		ti,poll-period = <30>;
+		ti,min-pressure = <0x30>;
+		ti,vref = <0>;
+		ti,vendor = <0>;
+		ti,product = <65070>;
+		ti,version = <0x100>;
+	};
 };
-- 
1.7.4.1

[PATCH V3 2/2] ARM: davinci: da850: add tps6507x touchscreen DT data

[Vishwanathrao Badarkhe, Manish]

Add tps6507x touchscreen DT node to da850-evm.
Touchscreen DT data is added as per da850 board file.

Tested on da850-evm.

Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
---
Changes since V2:
 - Updated names of devicetree properties.
Changes since V1:
 - Updated commit message.

:100644 100644 c914357... 1ca1691... M	arch/arm/boot/dts/da850-evm.dts
 arch/arm/boot/dts/da850-evm.dts |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index c914357..1ca1691 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -155,4 +155,13 @@
 			regulator-boot-on;
 		};
 	};
+
+	tsc {
+		ti,poll-period = <30>;
+		ti,min-pressure = <0x30>;
+		ti,vref = <0>;
+		ti,vendor = <0>;
+		ti,product = <65070>;
+		ti,version = <0x100>;
+	};
 };
-- 
1.7.4.1

RE: [PATCH V3 0/2] Add DT support for tps6507x touchscreen

[Vishwanathrao Badarkhe, Manish]

Hi Dmitry
Please let me know, are there any review comments on this patch series?

Thanks
Manish Badarkhe

On Tue, May 21, 2013 at 14:24:16, Vishwanathrao Badarkhe, Manish wrote:
> Patch set adds DT support for tps6507x based touchscreen.
> Also, add DT data for tps6507x touchscreen in da850-evm by
> providing touchscreen node inside tps6507x mfd device.
> 
> This patch series applies on top of linux-next tree and depends on [1].
> 
> [1]tps6507x-ts: update to devm_* API
>    https://patchwork.kernel.org/patch/2324441/
> 
> Tested on da850-evm board.
> 
> 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.
> 
> Vishwanathrao Badarkhe, Manish (2):
>   tps6507x-ts: Add DT support
>   ARM: davinci: da850: add tps6507x touchscreen DT data
> 
>  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
>  arch/arm/boot/dts/da850-evm.dts                    |    9 ++
>  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
>  3 files changed, 104 insertions(+), 31 deletions(-)
> 
> -- 
> 1.7.4.1
> 
> 


Regards, 
Manish

RE: [PATCH V3 0/2] Add DT support for tps6507x touchscreen

[Vishwanathrao Badarkhe, Manish]

Hi Dmitry
Please let me know, are there any review comments on this patch series?

Thanks
Manish Badarkhe

On Tue, May 21, 2013 at 14:24:16, Vishwanathrao Badarkhe, Manish wrote:
> Patch set adds DT support for tps6507x based touchscreen.
> Also, add DT data for tps6507x touchscreen in da850-evm by
> providing touchscreen node inside tps6507x mfd device.
> 
> This patch series applies on top of linux-next tree and depends on [1].
> 
> [1]tps6507x-ts: update to devm_* API
>    https://patchwork.kernel.org/patch/2324441/
> 
> Tested on da850-evm board.
> 
> 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.
> 
> Vishwanathrao Badarkhe, Manish (2):
>   tps6507x-ts: Add DT support
>   ARM: davinci: da850: add tps6507x touchscreen DT data
> 
>  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
>  arch/arm/boot/dts/da850-evm.dts                    |    9 ++
>  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
>  3 files changed, 104 insertions(+), 31 deletions(-)
> 
> -- 
> 1.7.4.1
> 
> 


Regards, 
Manish

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Dmitry Torokhov]

Hi Manish,

On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe, Manish wrote:
> Add device tree based support for TI's tps6507x touchscreen.
> 
> Tested on da850-evm.
> 
> Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
> ---
> Changes since V2:
>  - Removed unnecessary code.
>  - Updated Documentation to provide proper names of
>    devicetree properties.
> 
> Changes since V1:
>  - Updated documentation to specify tps6507x as multifunctional
>    device.
>  - return proper error value in absence of platform and DT
>    data for touchscreen.
>  - Updated commit message.
> 
> :100755 100755 8fffa3c... 65ee2cd... M	Documentation/devicetree/bindings/mfd/tps6507x.txt
> :100644 100644 65e0f9a... 89232ee... M	drivers/input/touchscreen/tps6507x-ts.c
>  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
>  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
>  2 files changed, 95 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> index 8fffa3c..65ee2cd 100755
> --- 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"
> @@ -23,6 +27,12 @@ Required properties:
>         vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
>         vindcdc3-supply  : VDCDC3 input.
>         vldo1_2-supply   : VLDO1 and VLDO2 input.
> +- tsc: This node specifies touch screen data.
> +	ti,poll-period : Time at which touch input is getting sampled in ms.
> +	ti,min-pressure: Minimum pressure value to trigger touch.
> +	ti,vref	       : voltage reference for ADC.
> +		      0: Reference voltage for ADC is disabled.
> +		      1: Reference voltage for ADC is enabled.
>  
>  Regulator Optional properties:
>  - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> @@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
> +	      in sysclass interface.
> +- ti,product: Touchscreen product id to populate
> +	      in sysclass interface.
> +- ti,version: Touchscreen version id to populate
> +	      in sysclass interface.
> +
>  Example:
>  
>  	pmu: tps6507x@48 {
> @@ -88,4 +106,12 @@ Example:
>  			};
>  		};
>  
> +		tsc {
> +			ti,poll-period = <30>;
> +			ti,min-pressure = <0x30>;
> +			ti,vref = <0>;
> +			ti,vendor = <0>;
> +			ti,product = <65070>;
> +			ti,version = <0x100>;
> +		};
>  	};
> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> index 65e0f9a..89232ee 100644
> --- a/drivers/input/touchscreen/tps6507x-ts.c
> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> @@ -21,6 +21,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
> @@ -231,36 +233,76 @@ done:
>  	ret = tps6507x_adc_standby(tsc);
>  }
>  
> +static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
> +		struct input_dev *input_dev)
> +{
> +	struct device_node *node = tps6507x_dev->dev->of_node;
> +	struct tps6507x_board *tps_board =
> +		(struct tps6507x_board *)tps6507x_dev->dev->platform_data;


Please make tps_board const pointer and use dev_get_platdata() to fetch
it.

> +	struct touchscreen_init_data *init_data = NULL;
> +	int err;
> +
> +	if (node)
> +		node = of_find_node_by_name(node, "tsc");

Why do you have to locate OF node manually instead of already having it
attached to the device stucture?

> +	if (tps_board)
> +		init_data = tps_board->tps6507x_ts_init_data;
> +
> +	if (node == NULL || init_data == NULL) {
> +		err = -EINVAL;
> +		goto error_ret;
> +	} else if (init_data) {
> +		tps6507x_dev->ts->poll_period = init_data->poll_period;
> +		tps6507x_dev->ts->min_pressure = init_data->min_pressure;
> +		tps6507x_dev->ts->vref = init_data->vref;
> +		input_dev->id.vendor = init_data->vendor;
> +		input_dev->id.product = init_data->product;
> +		input_dev->id.version = init_data->version;
> +	} else if (node) {
> +		err = of_property_read_u32(node, "ti,poll-period",
> +					(u32 *)&tps6507x_dev->ts->poll_period);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,min-pressure",
> +					&tps6507x_dev->ts->min_pressure);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u32(node, "ti,vref",
> +					&tps6507x_dev->ts->vref);

I do not think it is used anywhere, see a patch I just sent out.

> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,vendor",
> +					&input_dev->id.vendor);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,product",
> +					&input_dev->id.product);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,version",
> +					&input_dev->id.version);

You say these 3 properties are optional, so why do you err out if you
fail to read them?

> +		if (err < 0)
> +			goto error_ret;
> +	}
> +
> +	return 0;
> +
> +error_ret:
> +	return err;
> +}
> +
>  static int tps6507x_ts_probe(struct platform_device *pdev)
>  {
>  	int error;
>  	struct tps6507x_ts *tsc;
>  	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
> -	struct touchscreen_init_data *init_data;
>  	struct input_dev *input_dev;
> -	struct tps6507x_board *tps_board;
>  	int schd;
>  
> -	/**
> -	 * tps_board points to pmic related constants
> -	 * coming from the board-evm file.
> -	 */
> -
> -	tps_board = (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> -
> -	if (!tps_board) {
> -		dev_err(tps6507x_dev->dev,
> -			"Could not find tps6507x platform data\n");
> -		return -EIO;
> -	}
> -
> -	/**
> -	 * 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");
> @@ -301,14 +343,10 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&tsc->work, tps6507x_ts_handler);
>  
> -	if (init_data) {
> -		tsc->poll_period = init_data->poll_period;
> -		tsc->vref = init_data->vref;
> -		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 {
> +	error = tsc_init_data(tps6507x_dev, input_dev);
> +
> +	if (error) {
> +		dev_warn(&pdev->dev, "Use default platform data\n");

Why do we have to warn here?

Also, this is not reqlly equivalent transformation of the old code,
which allowed to partially specify init data and would substitute some
of it with defaults.

>  		tsc->poll_period = TSC_DEFAULT_POLL_PERIOD;
>  		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
>  	}
> -- 
> 1.7.4.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Dmitry Torokhov]

Hi Manish,

On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe, Manish wrote:
> Add device tree based support for TI's tps6507x touchscreen.
> 
> Tested on da850-evm.
> 
> Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b-l0cyMroinI0@public.gmane.org>
> ---
> Changes since V2:
>  - Removed unnecessary code.
>  - Updated Documentation to provide proper names of
>    devicetree properties.
> 
> Changes since V1:
>  - Updated documentation to specify tps6507x as multifunctional
>    device.
>  - return proper error value in absence of platform and DT
>    data for touchscreen.
>  - Updated commit message.
> 
> :100755 100755 8fffa3c... 65ee2cd... M	Documentation/devicetree/bindings/mfd/tps6507x.txt
> :100644 100644 65e0f9a... 89232ee... M	drivers/input/touchscreen/tps6507x-ts.c
>  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
>  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
>  2 files changed, 95 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> index 8fffa3c..65ee2cd 100755
> --- 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"
> @@ -23,6 +27,12 @@ Required properties:
>         vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
>         vindcdc3-supply  : VDCDC3 input.
>         vldo1_2-supply   : VLDO1 and VLDO2 input.
> +- tsc: This node specifies touch screen data.
> +	ti,poll-period : Time at which touch input is getting sampled in ms.
> +	ti,min-pressure: Minimum pressure value to trigger touch.
> +	ti,vref	       : voltage reference for ADC.
> +		      0: Reference voltage for ADC is disabled.
> +		      1: Reference voltage for ADC is enabled.
>  
>  Regulator Optional properties:
>  - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> @@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
> +	      in sysclass interface.
> +- ti,product: Touchscreen product id to populate
> +	      in sysclass interface.
> +- ti,version: Touchscreen version id to populate
> +	      in sysclass interface.
> +
>  Example:
>  
>  	pmu: tps6507x@48 {
> @@ -88,4 +106,12 @@ Example:
>  			};
>  		};
>  
> +		tsc {
> +			ti,poll-period = <30>;
> +			ti,min-pressure = <0x30>;
> +			ti,vref = <0>;
> +			ti,vendor = <0>;
> +			ti,product = <65070>;
> +			ti,version = <0x100>;
> +		};
>  	};
> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> index 65e0f9a..89232ee 100644
> --- a/drivers/input/touchscreen/tps6507x-ts.c
> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> @@ -21,6 +21,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
> @@ -231,36 +233,76 @@ done:
>  	ret = tps6507x_adc_standby(tsc);
>  }
>  
> +static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
> +		struct input_dev *input_dev)
> +{
> +	struct device_node *node = tps6507x_dev->dev->of_node;
> +	struct tps6507x_board *tps_board =
> +		(struct tps6507x_board *)tps6507x_dev->dev->platform_data;


Please make tps_board const pointer and use dev_get_platdata() to fetch
it.

> +	struct touchscreen_init_data *init_data = NULL;
> +	int err;
> +
> +	if (node)
> +		node = of_find_node_by_name(node, "tsc");

Why do you have to locate OF node manually instead of already having it
attached to the device stucture?

> +	if (tps_board)
> +		init_data = tps_board->tps6507x_ts_init_data;
> +
> +	if (node == NULL || init_data == NULL) {
> +		err = -EINVAL;
> +		goto error_ret;
> +	} else if (init_data) {
> +		tps6507x_dev->ts->poll_period = init_data->poll_period;
> +		tps6507x_dev->ts->min_pressure = init_data->min_pressure;
> +		tps6507x_dev->ts->vref = init_data->vref;
> +		input_dev->id.vendor = init_data->vendor;
> +		input_dev->id.product = init_data->product;
> +		input_dev->id.version = init_data->version;
> +	} else if (node) {
> +		err = of_property_read_u32(node, "ti,poll-period",
> +					(u32 *)&tps6507x_dev->ts->poll_period);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,min-pressure",
> +					&tps6507x_dev->ts->min_pressure);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u32(node, "ti,vref",
> +					&tps6507x_dev->ts->vref);

I do not think it is used anywhere, see a patch I just sent out.

> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,vendor",
> +					&input_dev->id.vendor);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,product",
> +					&input_dev->id.product);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,version",
> +					&input_dev->id.version);

You say these 3 properties are optional, so why do you err out if you
fail to read them?

> +		if (err < 0)
> +			goto error_ret;
> +	}
> +
> +	return 0;
> +
> +error_ret:
> +	return err;
> +}
> +
>  static int tps6507x_ts_probe(struct platform_device *pdev)
>  {
>  	int error;
>  	struct tps6507x_ts *tsc;
>  	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
> -	struct touchscreen_init_data *init_data;
>  	struct input_dev *input_dev;
> -	struct tps6507x_board *tps_board;
>  	int schd;
>  
> -	/**
> -	 * tps_board points to pmic related constants
> -	 * coming from the board-evm file.
> -	 */
> -
> -	tps_board = (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> -
> -	if (!tps_board) {
> -		dev_err(tps6507x_dev->dev,
> -			"Could not find tps6507x platform data\n");
> -		return -EIO;
> -	}
> -
> -	/**
> -	 * 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");
> @@ -301,14 +343,10 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&tsc->work, tps6507x_ts_handler);
>  
> -	if (init_data) {
> -		tsc->poll_period = init_data->poll_period;
> -		tsc->vref = init_data->vref;
> -		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 {
> +	error = tsc_init_data(tps6507x_dev, input_dev);
> +
> +	if (error) {
> +		dev_warn(&pdev->dev, "Use default platform data\n");

Why do we have to warn here?

Also, this is not reqlly equivalent transformation of the old code,
which allowed to partially specify init data and would substitute some
of it with defaults.

>  		tsc->poll_period = TSC_DEFAULT_POLL_PERIOD;
>  		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
>  	}
> -- 
> 1.7.4.1
> 

Thanks.

-- 
Dmitry

RE: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Vishwanathrao Badarkhe, Manish]

Hi Dmitry,

On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> Hi Manish,
> 
> On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe, Manish wrote:
> > Add device tree based support for TI's tps6507x touchscreen.
> > 
> > Tested on da850-evm.
> > 
> > Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
> > ---
> > Changes since V2:
> >  - Removed unnecessary code.
> >  - Updated Documentation to provide proper names of
> >    devicetree properties.
> > 
> > Changes since V1:
> >  - Updated documentation to specify tps6507x as multifunctional
> >    device.
> >  - return proper error value in absence of platform and DT
> >    data for touchscreen.
> >  - Updated commit message.
> > 
> > :100755 100755 8fffa3c... 65ee2cd... M	Documentation/devicetree/bindings/mfd/tps6507x.txt
> > :100644 100644 65e0f9a... 89232ee... M	drivers/input/touchscreen/tps6507x-ts.c
> >  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
> >  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
> >  2 files changed, 95 insertions(+), 31 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt 
> > b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > index 8fffa3c..65ee2cd 100755
> > --- 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"
> > @@ -23,6 +27,12 @@ Required properties:
> >         vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
> >         vindcdc3-supply  : VDCDC3 input.
> >         vldo1_2-supply   : VLDO1 and VLDO2 input.
> > +- tsc: This node specifies touch screen data.
> > +	ti,poll-period : Time at which touch input is getting sampled in ms.
> > +	ti,min-pressure: Minimum pressure value to trigger touch.
> > +	ti,vref	       : voltage reference for ADC.
> > +		      0: Reference voltage for ADC is disabled.
> > +		      1: Reference voltage for ADC is enabled.
> >  
> >  Regulator Optional properties:
> >  - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> > @@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
> > +	      in sysclass interface.
> > +- ti,product: Touchscreen product id to populate
> > +	      in sysclass interface.
> > +- ti,version: Touchscreen version id to populate
> > +	      in sysclass interface.
> > +
> >  Example:
> >  
> >  	pmu: tps6507x@48 {
> > @@ -88,4 +106,12 @@ Example:
> >  			};
> >  		};
> >  
> > +		tsc {
> > +			ti,poll-period = <30>;
> > +			ti,min-pressure = <0x30>;
> > +			ti,vref = <0>;
> > +			ti,vendor = <0>;
> > +			ti,product = <65070>;
> > +			ti,version = <0x100>;
> > +		};
> >  	};
> > diff --git a/drivers/input/touchscreen/tps6507x-ts.c 
> > b/drivers/input/touchscreen/tps6507x-ts.c
> > index 65e0f9a..89232ee 100644
> > --- a/drivers/input/touchscreen/tps6507x-ts.c
> > +++ b/drivers/input/touchscreen/tps6507x-ts.c
> > @@ -21,6 +21,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 @@ -231,36 +233,76 @@ done:
> >  	ret = tps6507x_adc_standby(tsc);
> >  }
> >  
> > +static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
> > +		struct input_dev *input_dev)
> > +{
> > +	struct device_node *node = tps6507x_dev->dev->of_node;
> > +	struct tps6507x_board *tps_board =
> > +		(struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> 
> 
> Please make tps_board const pointer and use dev_get_platdata() to fetch it.

Ok, I will make "tps_board" as a const pointer and fetch it using dev_get_platdata().

> 
> > +	struct touchscreen_init_data *init_data = NULL;
> > +	int err;
> > +
> > +	if (node)
> > +		node = of_find_node_by_name(node, "tsc");
> 
> Why do you have to locate OF node manually instead of already having it attached to the device stucture?

As TPS6507x is mfd device containing two nodes, regulator and touchscreen.
It is necessary to use "of_find_node_by_name" to find child "tsc" node of 
TPS6507x MFD device.

> 
> > +	if (tps_board)
> > +		init_data = tps_board->tps6507x_ts_init_data;
> > +
> > +	if (node == NULL || init_data == NULL) {
> > +		err = -EINVAL;
> > +		goto error_ret;
> > +	} else if (init_data) {
> > +		tps6507x_dev->ts->poll_period = init_data->poll_period;
> > +		tps6507x_dev->ts->min_pressure = init_data->min_pressure;
> > +		tps6507x_dev->ts->vref = init_data->vref;
> > +		input_dev->id.vendor = init_data->vendor;
> > +		input_dev->id.product = init_data->product;
> > +		input_dev->id.version = init_data->version;
> > +	} else if (node) {
> > +		err = of_property_read_u32(node, "ti,poll-period",
> > +					(u32 *)&tps6507x_dev->ts->poll_period);
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u16(node, "ti,min-pressure",
> > +					&tps6507x_dev->ts->min_pressure);
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u32(node, "ti,vref",
> > +					&tps6507x_dev->ts->vref);
> 
> I do not think it is used anywhere, see a patch I just sent out.

I have seen your patch. I will remove this.

> 
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u16(node, "ti,vendor",
> > +					&input_dev->id.vendor);
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u16(node, "ti,product",
> > +					&input_dev->id.product);
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u16(node, "ti,version",
> > +					&input_dev->id.version);
> 
> You say these 3 properties are optional, so why do you err out if you fail to read them?

Correct, there is no need to return error in case of these optional 
Properties. I will return success even read fails in case of these optional
properties.

> 
> > +		if (err < 0)
> > +			goto error_ret;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_ret:
> > +	return err;
> > +}
> > +
> >  static int tps6507x_ts_probe(struct platform_device *pdev)  {
> >  	int error;
> >  	struct tps6507x_ts *tsc;
> >  	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
> > -	struct touchscreen_init_data *init_data;
> >  	struct input_dev *input_dev;
> > -	struct tps6507x_board *tps_board;
> >  	int schd;
> >  
> > -	/**
> > -	 * tps_board points to pmic related constants
> > -	 * coming from the board-evm file.
> > -	 */
> > -
> > -	tps_board = (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> > -
> > -	if (!tps_board) {
> > -		dev_err(tps6507x_dev->dev,
> > -			"Could not find tps6507x platform data\n");
> > -		return -EIO;
> > -	}
> > -
> > -	/**
> > -	 * 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"); @@ 
> > -301,14 +343,10 @@ static int tps6507x_ts_probe(struct platform_device 
> > *pdev)
> >  
> >  	INIT_DELAYED_WORK(&tsc->work, tps6507x_ts_handler);
> >  
> > -	if (init_data) {
> > -		tsc->poll_period = init_data->poll_period;
> > -		tsc->vref = init_data->vref;
> > -		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 {
> > +	error = tsc_init_data(tps6507x_dev, input_dev);
> > +
> > +	if (error) {
> > +		dev_warn(&pdev->dev, "Use default platform data\n");
> 
> Why do we have to warn here?
> 
> Also, this is not reqlly equivalent transformation of the old code, which allowed to partially specify init data and would substitute some of it with defaults.

As per your current patch submission "[PATCH3/3] Input: tps6507x-ts - convert to polled 
input device infrastructure"
Now, we can partially specify init data and substitute some of it with defaults.
Hence, I will remove warning as well as error handling over here.

It's better for me to test your patch series, and then apply my patches on top of it. 
I will test your series and then will send next version of my patches incorporating 
your review comments.

> 
> >  		tsc->poll_period = TSC_DEFAULT_POLL_PERIOD;
> >  		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
> >  	}
> > --
> > 1.7.4.1
> > 
> 
> Thanks.
> 
> -- 
> Dmitry
> 


Regards, 
Manish

RE: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Vishwanathrao Badarkhe, Manish]

Hi Dmitry,

On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> Hi Manish,
> 
> On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe, Manish wrote:
> > Add device tree based support for TI's tps6507x touchscreen.
> > 
> > Tested on da850-evm.
> > 
> > Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
> > ---
> > Changes since V2:
> >  - Removed unnecessary code.
> >  - Updated Documentation to provide proper names of
> >    devicetree properties.
> > 
> > Changes since V1:
> >  - Updated documentation to specify tps6507x as multifunctional
> >    device.
> >  - return proper error value in absence of platform and DT
> >    data for touchscreen.
> >  - Updated commit message.
> > 
> > :100755 100755 8fffa3c... 65ee2cd... M	Documentation/devicetree/bindings/mfd/tps6507x.txt
> > :100644 100644 65e0f9a... 89232ee... M	drivers/input/touchscreen/tps6507x-ts.c
> >  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
> >  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
> >  2 files changed, 95 insertions(+), 31 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt 
> > b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> > index 8fffa3c..65ee2cd 100755
> > --- 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"
> > @@ -23,6 +27,12 @@ Required properties:
> >         vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
> >         vindcdc3-supply  : VDCDC3 input.
> >         vldo1_2-supply   : VLDO1 and VLDO2 input.
> > +- tsc: This node specifies touch screen data.
> > +	ti,poll-period : Time at which touch input is getting sampled in ms.
> > +	ti,min-pressure: Minimum pressure value to trigger touch.
> > +	ti,vref	       : voltage reference for ADC.
> > +		      0: Reference voltage for ADC is disabled.
> > +		      1: Reference voltage for ADC is enabled.
> >  
> >  Regulator Optional properties:
> >  - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> > @@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
> > +	      in sysclass interface.
> > +- ti,product: Touchscreen product id to populate
> > +	      in sysclass interface.
> > +- ti,version: Touchscreen version id to populate
> > +	      in sysclass interface.
> > +
> >  Example:
> >  
> >  	pmu: tps6507x@48 {
> > @@ -88,4 +106,12 @@ Example:
> >  			};
> >  		};
> >  
> > +		tsc {
> > +			ti,poll-period = <30>;
> > +			ti,min-pressure = <0x30>;
> > +			ti,vref = <0>;
> > +			ti,vendor = <0>;
> > +			ti,product = <65070>;
> > +			ti,version = <0x100>;
> > +		};
> >  	};
> > diff --git a/drivers/input/touchscreen/tps6507x-ts.c 
> > b/drivers/input/touchscreen/tps6507x-ts.c
> > index 65e0f9a..89232ee 100644
> > --- a/drivers/input/touchscreen/tps6507x-ts.c
> > +++ b/drivers/input/touchscreen/tps6507x-ts.c
> > @@ -21,6 +21,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 @@ -231,36 +233,76 @@ done:
> >  	ret = tps6507x_adc_standby(tsc);
> >  }
> >  
> > +static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
> > +		struct input_dev *input_dev)
> > +{
> > +	struct device_node *node = tps6507x_dev->dev->of_node;
> > +	struct tps6507x_board *tps_board =
> > +		(struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> 
> 
> Please make tps_board const pointer and use dev_get_platdata() to fetch it.

Ok, I will make "tps_board" as a const pointer and fetch it using dev_get_platdata().

> 
> > +	struct touchscreen_init_data *init_data = NULL;
> > +	int err;
> > +
> > +	if (node)
> > +		node = of_find_node_by_name(node, "tsc");
> 
> Why do you have to locate OF node manually instead of already having it attached to the device stucture?

As TPS6507x is mfd device containing two nodes, regulator and touchscreen.
It is necessary to use "of_find_node_by_name" to find child "tsc" node of 
TPS6507x MFD device.

> 
> > +	if (tps_board)
> > +		init_data = tps_board->tps6507x_ts_init_data;
> > +
> > +	if (node == NULL || init_data == NULL) {
> > +		err = -EINVAL;
> > +		goto error_ret;
> > +	} else if (init_data) {
> > +		tps6507x_dev->ts->poll_period = init_data->poll_period;
> > +		tps6507x_dev->ts->min_pressure = init_data->min_pressure;
> > +		tps6507x_dev->ts->vref = init_data->vref;
> > +		input_dev->id.vendor = init_data->vendor;
> > +		input_dev->id.product = init_data->product;
> > +		input_dev->id.version = init_data->version;
> > +	} else if (node) {
> > +		err = of_property_read_u32(node, "ti,poll-period",
> > +					(u32 *)&tps6507x_dev->ts->poll_period);
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u16(node, "ti,min-pressure",
> > +					&tps6507x_dev->ts->min_pressure);
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u32(node, "ti,vref",
> > +					&tps6507x_dev->ts->vref);
> 
> I do not think it is used anywhere, see a patch I just sent out.

I have seen your patch. I will remove this.

> 
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u16(node, "ti,vendor",
> > +					&input_dev->id.vendor);
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u16(node, "ti,product",
> > +					&input_dev->id.product);
> > +		if (err < 0)
> > +			goto error_ret;
> > +
> > +		err = of_property_read_u16(node, "ti,version",
> > +					&input_dev->id.version);
> 
> You say these 3 properties are optional, so why do you err out if you fail to read them?

Correct, there is no need to return error in case of these optional 
Properties. I will return success even read fails in case of these optional
properties.

> 
> > +		if (err < 0)
> > +			goto error_ret;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_ret:
> > +	return err;
> > +}
> > +
> >  static int tps6507x_ts_probe(struct platform_device *pdev)  {
> >  	int error;
> >  	struct tps6507x_ts *tsc;
> >  	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
> > -	struct touchscreen_init_data *init_data;
> >  	struct input_dev *input_dev;
> > -	struct tps6507x_board *tps_board;
> >  	int schd;
> >  
> > -	/**
> > -	 * tps_board points to pmic related constants
> > -	 * coming from the board-evm file.
> > -	 */
> > -
> > -	tps_board = (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> > -
> > -	if (!tps_board) {
> > -		dev_err(tps6507x_dev->dev,
> > -			"Could not find tps6507x platform data\n");
> > -		return -EIO;
> > -	}
> > -
> > -	/**
> > -	 * 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"); @@ 
> > -301,14 +343,10 @@ static int tps6507x_ts_probe(struct platform_device 
> > *pdev)
> >  
> >  	INIT_DELAYED_WORK(&tsc->work, tps6507x_ts_handler);
> >  
> > -	if (init_data) {
> > -		tsc->poll_period = init_data->poll_period;
> > -		tsc->vref = init_data->vref;
> > -		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 {
> > +	error = tsc_init_data(tps6507x_dev, input_dev);
> > +
> > +	if (error) {
> > +		dev_warn(&pdev->dev, "Use default platform data\n");
> 
> Why do we have to warn here?
> 
> Also, this is not reqlly equivalent transformation of the old code, which allowed to partially specify init data and would substitute some of it with defaults.

As per your current patch submission "[PATCH3/3] Input: tps6507x-ts - convert to polled 
input device infrastructure"
Now, we can partially specify init data and substitute some of it with defaults.
Hence, I will remove warning as well as error handling over here.

It's better for me to test your patch series, and then apply my patches on top of it. 
I will test your series and then will send next version of my patches incorporating 
your review comments.

> 
> >  		tsc->poll_period = TSC_DEFAULT_POLL_PERIOD;
> >  		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
> >  	}
> > --
> > 1.7.4.1
> > 
> 
> Thanks.
> 
> -- 
> Dmitry
> 


Regards, 
Manish

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Dmitry Torokhov]

Manish,

On Mon, Jun 10, 2013 at 10:23:16AM +0000, Vishwanathrao Badarkhe, Manish wrote:
> Hi Dmitry,
> 
> On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> > Hi Manish,
> > 
> > On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe, Manish wrote:
> > 
> > > +	struct touchscreen_init_data *init_data = NULL;
> > > +	int err;
> > > +
> > > +	if (node)
> > > +		node = of_find_node_by_name(node, "tsc");
> > 
> > Why do you have to locate OF node manually instead of already having it attached to the device stucture?
> 
> As TPS6507x is mfd device containing two nodes, regulator and touchscreen.
> It is necessary to use "of_find_node_by_name" to find child "tsc" node of 
> TPS6507x MFD device.

I understand that TPS6507x is a MFD device, However, I still do not
understand why you do not attach OF data to the child platform device
representing touchscreen when you create it.

Thanks.

-- 
Dmitry

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Dmitry Torokhov]

Manish,

On Mon, Jun 10, 2013 at 10:23:16AM +0000, Vishwanathrao Badarkhe, Manish wrote:
> Hi Dmitry,
> 
> On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> > Hi Manish,
> > 
> > On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe, Manish wrote:
> > 
> > > +	struct touchscreen_init_data *init_data = NULL;
> > > +	int err;
> > > +
> > > +	if (node)
> > > +		node = of_find_node_by_name(node, "tsc");
> > 
> > Why do you have to locate OF node manually instead of already having it attached to the device stucture?
> 
> As TPS6507x is mfd device containing two nodes, regulator and touchscreen.
> It is necessary to use "of_find_node_by_name" to find child "tsc" node of 
> TPS6507x MFD device.

I understand that TPS6507x is a MFD device, However, I still do not
understand why you do not attach OF data to the child platform device
representing touchscreen when you create it.

Thanks.

-- 
Dmitry

RE: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Vishwanathrao Badarkhe, Manish]

Hi Dmitry,

On Mon, Jun 10, 2013 at 23:33:11, Dmitry Torokhov wrote:
> Manish,
> 
> On Mon, Jun 10, 2013 at 10:23:16AM +0000, Vishwanathrao Badarkhe, Manish wrote:
> > Hi Dmitry,
> > 
> > On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> > > Hi Manish,
> > > 
> > > On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe, Manish wrote:
> > > 
> > > > +	struct touchscreen_init_data *init_data = NULL;
> > > > +	int err;
> > > > +
> > > > +	if (node)
> > > > +		node = of_find_node_by_name(node, "tsc");
> > > 
> > > Why do you have to locate OF node manually instead of already having it attached to the device stucture?
> > 
> > As TPS6507x is mfd device containing two nodes, regulator and touchscreen.
> > It is necessary to use "of_find_node_by_name" to find child "tsc" node 
> > of TPS6507x MFD device.
> 
> I understand that TPS6507x is a MFD device, However, I still do not understand why you do not attach OF data to the child platform device representing touch screen when you create it.

I gone through most of MFD devices in mainline kernel where "of_find_node_by_name" 
function is used in order to populate child node properties. 
See below example of max8925 MFD device. 

Max8025 MFD device has following modules
1. Touch screen
2. Charger
3. Backlight
4. regulator

In DT case, device node for max8925 MFD device is in "arch/arm/boot/dts/mmp2-brownstone.dts" 
file having child nodes as regulator, backlight and charger.
Respective drivers (regulator, backlight and charger) of max8925 MFD device used 
"of_find_node_by_name" function in order to populate child node properties.

Here, also same case for TPS6507x MFD device

TPS6507x MFD device has two childs:
1. regulator
2. touch screen.

Regulator driver for TPS6507x is already in mainline and using function 
"of_find_node_by_name" to populate device tree properties for regulators.
On similar lines I used "of_find_node_by_name" to populate device
tree properties for touch screen.

Still, I agreed that it is possible to attach OF data to child platform device
but it requires changes in MFD driver of TPS6507x.

Please let me know your opinion about this.


Thanks, 
Manish

RE: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Vishwanathrao Badarkhe, Manish]

Hi Dmitry,

On Mon, Jun 10, 2013 at 23:33:11, Dmitry Torokhov wrote:
> Manish,
> 
> On Mon, Jun 10, 2013 at 10:23:16AM +0000, Vishwanathrao Badarkhe, Manish wrote:
> > Hi Dmitry,
> > 
> > On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> > > Hi Manish,
> > > 
> > > On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe, Manish wrote:
> > > 
> > > > +	struct touchscreen_init_data *init_data = NULL;
> > > > +	int err;
> > > > +
> > > > +	if (node)
> > > > +		node = of_find_node_by_name(node, "tsc");
> > > 
> > > Why do you have to locate OF node manually instead of already having it attached to the device stucture?
> > 
> > As TPS6507x is mfd device containing two nodes, regulator and touchscreen.
> > It is necessary to use "of_find_node_by_name" to find child "tsc" node 
> > of TPS6507x MFD device.
> 
> I understand that TPS6507x is a MFD device, However, I still do not understand why you do not attach OF data to the child platform device representing touch screen when you create it.

I gone through most of MFD devices in mainline kernel where "of_find_node_by_name" 
function is used in order to populate child node properties. 
See below example of max8925 MFD device. 

Max8025 MFD device has following modules
1. Touch screen
2. Charger
3. Backlight
4. regulator

In DT case, device node for max8925 MFD device is in "arch/arm/boot/dts/mmp2-brownstone.dts" 
file having child nodes as regulator, backlight and charger.
Respective drivers (regulator, backlight and charger) of max8925 MFD device used 
"of_find_node_by_name" function in order to populate child node properties.

Here, also same case for TPS6507x MFD device

TPS6507x MFD device has two childs:
1. regulator
2. touch screen.

Regulator driver for TPS6507x is already in mainline and using function 
"of_find_node_by_name" to populate device tree properties for regulators.
On similar lines I used "of_find_node_by_name" to populate device
tree properties for touch screen.

Still, I agreed that it is possible to attach OF data to child platform device
but it requires changes in MFD driver of TPS6507x.

Please let me know your opinion about this.


Thanks, 
Manish

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Grant Likely]

On Tue, 21 May 2013 14:24:17 +0530, "Vishwanathrao Badarkhe, Manish" <manishv.b@ti.com> wrote:
> Add device tree based support for TI's tps6507x touchscreen.
> 
> Tested on da850-evm.
> 
> Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
> ---
> Changes since V2:
>  - Removed unnecessary code.
>  - Updated Documentation to provide proper names of
>    devicetree properties.
> 
> Changes since V1:
>  - Updated documentation to specify tps6507x as multifunctional
>    device.
>  - return proper error value in absence of platform and DT
>    data for touchscreen.
>  - Updated commit message.
> 
> :100755 100755 8fffa3c... 65ee2cd... M	Documentation/devicetree/bindings/mfd/tps6507x.txt
> :100644 100644 65e0f9a... 89232ee... M	drivers/input/touchscreen/tps6507x-ts.c
>  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-

Binding looks fine to me.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

g.

>  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
>  2 files changed, 95 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> index 8fffa3c..65ee2cd 100755
> --- 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"
> @@ -23,6 +27,12 @@ Required properties:
>         vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
>         vindcdc3-supply  : VDCDC3 input.
>         vldo1_2-supply   : VLDO1 and VLDO2 input.
> +- tsc: This node specifies touch screen data.
> +	ti,poll-period : Time at which touch input is getting sampled in ms.
> +	ti,min-pressure: Minimum pressure value to trigger touch.
> +	ti,vref	       : voltage reference for ADC.
> +		      0: Reference voltage for ADC is disabled.
> +		      1: Reference voltage for ADC is enabled.
>  
>  Regulator Optional properties:
>  - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> @@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
> +	      in sysclass interface.
> +- ti,product: Touchscreen product id to populate
> +	      in sysclass interface.
> +- ti,version: Touchscreen version id to populate
> +	      in sysclass interface.
> +
>  Example:
>  
>  	pmu: tps6507x@48 {
> @@ -88,4 +106,12 @@ Example:
>  			};
>  		};
>  
> +		tsc {
> +			ti,poll-period = <30>;
> +			ti,min-pressure = <0x30>;
> +			ti,vref = <0>;
> +			ti,vendor = <0>;
> +			ti,product = <65070>;
> +			ti,version = <0x100>;
> +		};
>  	};
> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> index 65e0f9a..89232ee 100644
> --- a/drivers/input/touchscreen/tps6507x-ts.c
> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> @@ -21,6 +21,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
> @@ -231,36 +233,76 @@ done:
>  	ret = tps6507x_adc_standby(tsc);
>  }
>  
> +static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
> +		struct input_dev *input_dev)
> +{
> +	struct device_node *node = tps6507x_dev->dev->of_node;
> +	struct tps6507x_board *tps_board =
> +		(struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> +	struct touchscreen_init_data *init_data = NULL;
> +	int err;
> +
> +	if (node)
> +		node = of_find_node_by_name(node, "tsc");
> +	if (tps_board)
> +		init_data = tps_board->tps6507x_ts_init_data;
> +
> +	if (node == NULL || init_data == NULL) {
> +		err = -EINVAL;
> +		goto error_ret;
> +	} else if (init_data) {
> +		tps6507x_dev->ts->poll_period = init_data->poll_period;
> +		tps6507x_dev->ts->min_pressure = init_data->min_pressure;
> +		tps6507x_dev->ts->vref = init_data->vref;
> +		input_dev->id.vendor = init_data->vendor;
> +		input_dev->id.product = init_data->product;
> +		input_dev->id.version = init_data->version;
> +	} else if (node) {
> +		err = of_property_read_u32(node, "ti,poll-period",
> +					(u32 *)&tps6507x_dev->ts->poll_period);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,min-pressure",
> +					&tps6507x_dev->ts->min_pressure);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u32(node, "ti,vref",
> +					&tps6507x_dev->ts->vref);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,vendor",
> +					&input_dev->id.vendor);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,product",
> +					&input_dev->id.product);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,version",
> +					&input_dev->id.version);
> +		if (err < 0)
> +			goto error_ret;
> +	}
> +
> +	return 0;
> +
> +error_ret:
> +	return err;
> +}
> +
>  static int tps6507x_ts_probe(struct platform_device *pdev)
>  {
>  	int error;
>  	struct tps6507x_ts *tsc;
>  	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
> -	struct touchscreen_init_data *init_data;
>  	struct input_dev *input_dev;
> -	struct tps6507x_board *tps_board;
>  	int schd;
>  
> -	/**
> -	 * tps_board points to pmic related constants
> -	 * coming from the board-evm file.
> -	 */
> -
> -	tps_board = (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> -
> -	if (!tps_board) {
> -		dev_err(tps6507x_dev->dev,
> -			"Could not find tps6507x platform data\n");
> -		return -EIO;
> -	}
> -
> -	/**
> -	 * 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");
> @@ -301,14 +343,10 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&tsc->work, tps6507x_ts_handler);
>  
> -	if (init_data) {
> -		tsc->poll_period = init_data->poll_period;
> -		tsc->vref = init_data->vref;
> -		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 {
> +	error = tsc_init_data(tps6507x_dev, input_dev);
> +
> +	if (error) {
> +		dev_warn(&pdev->dev, "Use default platform data\n");
>  		tsc->poll_period = TSC_DEFAULT_POLL_PERIOD;
>  		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
>  	}
> -- 
> 1.7.4.1
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Grant Likely]

On Tue, 21 May 2013 14:24:17 +0530, "Vishwanathrao Badarkhe, Manish" <manishv.b@ti.com> wrote:
> Add device tree based support for TI's tps6507x touchscreen.
> 
> Tested on da850-evm.
> 
> Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
> ---
> Changes since V2:
>  - Removed unnecessary code.
>  - Updated Documentation to provide proper names of
>    devicetree properties.
> 
> Changes since V1:
>  - Updated documentation to specify tps6507x as multifunctional
>    device.
>  - return proper error value in absence of platform and DT
>    data for touchscreen.
>  - Updated commit message.
> 
> :100755 100755 8fffa3c... 65ee2cd... M	Documentation/devicetree/bindings/mfd/tps6507x.txt
> :100644 100644 65e0f9a... 89232ee... M	drivers/input/touchscreen/tps6507x-ts.c
>  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-

Binding looks fine to me.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

g.

>  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
>  2 files changed, 95 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> index 8fffa3c..65ee2cd 100755
> --- 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"
> @@ -23,6 +27,12 @@ Required properties:
>         vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
>         vindcdc3-supply  : VDCDC3 input.
>         vldo1_2-supply   : VLDO1 and VLDO2 input.
> +- tsc: This node specifies touch screen data.
> +	ti,poll-period : Time at which touch input is getting sampled in ms.
> +	ti,min-pressure: Minimum pressure value to trigger touch.
> +	ti,vref	       : voltage reference for ADC.
> +		      0: Reference voltage for ADC is disabled.
> +		      1: Reference voltage for ADC is enabled.
>  
>  Regulator Optional properties:
>  - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> @@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
> +	      in sysclass interface.
> +- ti,product: Touchscreen product id to populate
> +	      in sysclass interface.
> +- ti,version: Touchscreen version id to populate
> +	      in sysclass interface.
> +
>  Example:
>  
>  	pmu: tps6507x@48 {
> @@ -88,4 +106,12 @@ Example:
>  			};
>  		};
>  
> +		tsc {
> +			ti,poll-period = <30>;
> +			ti,min-pressure = <0x30>;
> +			ti,vref = <0>;
> +			ti,vendor = <0>;
> +			ti,product = <65070>;
> +			ti,version = <0x100>;
> +		};
>  	};
> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> index 65e0f9a..89232ee 100644
> --- a/drivers/input/touchscreen/tps6507x-ts.c
> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> @@ -21,6 +21,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
> @@ -231,36 +233,76 @@ done:
>  	ret = tps6507x_adc_standby(tsc);
>  }
>  
> +static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
> +		struct input_dev *input_dev)
> +{
> +	struct device_node *node = tps6507x_dev->dev->of_node;
> +	struct tps6507x_board *tps_board =
> +		(struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> +	struct touchscreen_init_data *init_data = NULL;
> +	int err;
> +
> +	if (node)
> +		node = of_find_node_by_name(node, "tsc");
> +	if (tps_board)
> +		init_data = tps_board->tps6507x_ts_init_data;
> +
> +	if (node == NULL || init_data == NULL) {
> +		err = -EINVAL;
> +		goto error_ret;
> +	} else if (init_data) {
> +		tps6507x_dev->ts->poll_period = init_data->poll_period;
> +		tps6507x_dev->ts->min_pressure = init_data->min_pressure;
> +		tps6507x_dev->ts->vref = init_data->vref;
> +		input_dev->id.vendor = init_data->vendor;
> +		input_dev->id.product = init_data->product;
> +		input_dev->id.version = init_data->version;
> +	} else if (node) {
> +		err = of_property_read_u32(node, "ti,poll-period",
> +					(u32 *)&tps6507x_dev->ts->poll_period);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,min-pressure",
> +					&tps6507x_dev->ts->min_pressure);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u32(node, "ti,vref",
> +					&tps6507x_dev->ts->vref);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,vendor",
> +					&input_dev->id.vendor);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,product",
> +					&input_dev->id.product);
> +		if (err < 0)
> +			goto error_ret;
> +
> +		err = of_property_read_u16(node, "ti,version",
> +					&input_dev->id.version);
> +		if (err < 0)
> +			goto error_ret;
> +	}
> +
> +	return 0;
> +
> +error_ret:
> +	return err;
> +}
> +
>  static int tps6507x_ts_probe(struct platform_device *pdev)
>  {
>  	int error;
>  	struct tps6507x_ts *tsc;
>  	struct tps6507x_dev *tps6507x_dev = dev_get_drvdata(pdev->dev.parent);
> -	struct touchscreen_init_data *init_data;
>  	struct input_dev *input_dev;
> -	struct tps6507x_board *tps_board;
>  	int schd;
>  
> -	/**
> -	 * tps_board points to pmic related constants
> -	 * coming from the board-evm file.
> -	 */
> -
> -	tps_board = (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> -
> -	if (!tps_board) {
> -		dev_err(tps6507x_dev->dev,
> -			"Could not find tps6507x platform data\n");
> -		return -EIO;
> -	}
> -
> -	/**
> -	 * 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");
> @@ -301,14 +343,10 @@ static int tps6507x_ts_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&tsc->work, tps6507x_ts_handler);
>  
> -	if (init_data) {
> -		tsc->poll_period = init_data->poll_period;
> -		tsc->vref = init_data->vref;
> -		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 {
> +	error = tsc_init_data(tps6507x_dev, input_dev);
> +
> +	if (error) {
> +		dev_warn(&pdev->dev, "Use default platform data\n");
>  		tsc->poll_period = TSC_DEFAULT_POLL_PERIOD;
>  		tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
>  	}
> -- 
> 1.7.4.1
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Manish Badarkhe]

[-- Attachment #1.1: Type: text/plain, Size: 2465 bytes --]

Hi Dmitry

On Wed, Jun 12, 2013 at 12:23 PM, Vishwanathrao Badarkhe, Manish <
manishv.b-l0cyMroinI0@public.gmane.org> wrote:
> Hi Dmitry,
>
> On Mon, Jun 10, 2013 at 23:33:11, Dmitry Torokhov wrote:
>> Manish,
>>
>> On Mon, Jun 10, 2013 at 10:23:16AM +0000, Vishwanathrao Badarkhe, Manish
wrote:
>> > Hi Dmitry,
>> >
>> > On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
>> > > Hi Manish,
>> > >
>> > > On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe,
Manish wrote:
>> > >
>> > > > +       struct touchscreen_init_data *init_data = NULL;
>> > > > +       int err;
>> > > > +
>> > > > +       if (node)
>> > > > +               node = of_find_node_by_name(node, "tsc");
>> > >
>> > > Why do you have to locate OF node manually instead of already having
it attached to the device stucture?
>> >
>> > As TPS6507x is mfd device containing two nodes, regulator and
touchscreen.
>> > It is necessary to use "of_find_node_by_name" to find child "tsc" node
>> > of TPS6507x MFD device.
>>
>> I understand that TPS6507x is a MFD device, However, I still do not
understand why you do not attach OF data to the child platform device
representing touch screen when you create it.
>
> I gone through most of MFD devices in mainline kernel where
"of_find_node_by_name"
> function is used in order to populate child node properties.
> See below example of max8925 MFD device.
>
> Max8025 MFD device has following modules
> 1. Touch screen
> 2. Charger
> 3. Backlight
> 4. regulator
>
> In DT case, device node for max8925 MFD device is in
"arch/arm/boot/dts/mmp2-brownstone.dts"
> file having child nodes as regulator, backlight and charger.
> Respective drivers (regulator, backlight and charger) of max8925 MFD
device used
> "of_find_node_by_name" function in order to populate child node
properties.
>
> Here, also same case for TPS6507x MFD device
>
> TPS6507x MFD device has two childs:
> 1. regulator
> 2. touch screen.
>
> Regulator driver for TPS6507x is already in mainline and using function
> "of_find_node_by_name" to populate device tree properties for regulators.
> On similar lines I used "of_find_node_by_name" to populate device
> tree properties for touch screen.
>
> Still, I agreed that it is possible to attach OF data to child platform
device
> but it requires changes in MFD driver of TPS6507x.
>
> Please let me know your opinion about this.

Please let me know so accordingly I will make changes in code.

Regards
Manish Badarkhe

[-- Attachment #1.2: Type: text/html, Size: 3177 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Manish Badarkhe]

Hi Dmitry

On Mon, Jul 1, 2013 at 4:40 PM, Manish Badarkhe
<badarkhe.manish@gmail.com> wrote:
>
> Hi Dmitry
>
>
> On Wed, Jun 12, 2013 at 12:23 PM, Vishwanathrao Badarkhe, Manish
> <manishv.b@ti.com> wrote:
> > Hi Dmitry,
> >
> > On Mon, Jun 10, 2013 at 23:33:11, Dmitry Torokhov wrote:
> >> Manish,
> >>
> >> On Mon, Jun 10, 2013 at 10:23:16AM +0000, Vishwanathrao Badarkhe,
> >> Manish wrote:
> >> > Hi Dmitry,
> >> >
> >> > On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> >> > > Hi Manish,
> >> > >
> >> > > On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe,
> >> > > Manish wrote:
> >> > >
> >> > > > +       struct touchscreen_init_data *init_data = NULL;
> >> > > > +       int err;
> >> > > > +
> >> > > > +       if (node)
> >> > > > +               node = of_find_node_by_name(node, "tsc");
> >> > >
> >> > > Why do you have to locate OF node manually instead of already
> >> > > having it attached to the device stucture?
> >> >
> >> > As TPS6507x is mfd device containing two nodes, regulator and
> >> > touchscreen.
> >> > It is necessary to use "of_find_node_by_name" to find child "tsc"
> >> > node
> >> > of TPS6507x MFD device.
> >>
> >> I understand that TPS6507x is a MFD device, However, I still do not
> >> understand why you do not attach OF data to the child platform device
> >> representing touch screen when you create it.
> >
> > I gone through most of MFD devices in mainline kernel where
> > "of_find_node_by_name"
> > function is used in order to populate child node properties.
> > See below example of max8925 MFD device.
> >
> > Max8025 MFD device has following modules
> > 1. Touch screen
> > 2. Charger
> > 3. Backlight
> > 4. regulator
> >
> > In DT case, device node for max8925 MFD device is in
> > "arch/arm/boot/dts/mmp2-brownstone.dts"
> > file having child nodes as regulator, backlight and charger.
> > Respective drivers (regulator, backlight and charger) of max8925 MFD
> > device used
> > "of_find_node_by_name" function in order to populate child node
> > properties.
> >
> > Here, also same case for TPS6507x MFD device
> >
> > TPS6507x MFD device has two childs:
> > 1. regulator
> > 2. touch screen.
> >
> > Regulator driver for TPS6507x is already in mainline and using function
> > "of_find_node_by_name" to populate device tree properties for
> > regulators.
> > On similar lines I used "of_find_node_by_name" to populate device
> > tree properties for touch screen.
> >
> > Still, I agreed that it is possible to attach OF data to child platform
> > device
> > but it requires changes in MFD driver of TPS6507x.
> >
> > Please let me know your opinion about this.
>
> Please let me know so accordingly I will make changes in code.
>

Please provide your comments so accordingly will make changes in code.

Regards
Manish Badarkhe

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Manish Badarkhe]

Hi Dmitry

On Mon, Jul 1, 2013 at 4:40 PM, Manish Badarkhe
<badarkhe.manish@gmail.com> wrote:
>
> Hi Dmitry
>
>
> On Wed, Jun 12, 2013 at 12:23 PM, Vishwanathrao Badarkhe, Manish
> <manishv.b@ti.com> wrote:
> > Hi Dmitry,
> >
> > On Mon, Jun 10, 2013 at 23:33:11, Dmitry Torokhov wrote:
> >> Manish,
> >>
> >> On Mon, Jun 10, 2013 at 10:23:16AM +0000, Vishwanathrao Badarkhe,
> >> Manish wrote:
> >> > Hi Dmitry,
> >> >
> >> > On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> >> > > Hi Manish,
> >> > >
> >> > > On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe,
> >> > > Manish wrote:
> >> > >
> >> > > > +       struct touchscreen_init_data *init_data = NULL;
> >> > > > +       int err;
> >> > > > +
> >> > > > +       if (node)
> >> > > > +               node = of_find_node_by_name(node, "tsc");
> >> > >
> >> > > Why do you have to locate OF node manually instead of already
> >> > > having it attached to the device stucture?
> >> >
> >> > As TPS6507x is mfd device containing two nodes, regulator and
> >> > touchscreen.
> >> > It is necessary to use "of_find_node_by_name" to find child "tsc"
> >> > node
> >> > of TPS6507x MFD device.
> >>
> >> I understand that TPS6507x is a MFD device, However, I still do not
> >> understand why you do not attach OF data to the child platform device
> >> representing touch screen when you create it.
> >
> > I gone through most of MFD devices in mainline kernel where
> > "of_find_node_by_name"
> > function is used in order to populate child node properties.
> > See below example of max8925 MFD device.
> >
> > Max8025 MFD device has following modules
> > 1. Touch screen
> > 2. Charger
> > 3. Backlight
> > 4. regulator
> >
> > In DT case, device node for max8925 MFD device is in
> > "arch/arm/boot/dts/mmp2-brownstone.dts"
> > file having child nodes as regulator, backlight and charger.
> > Respective drivers (regulator, backlight and charger) of max8925 MFD
> > device used
> > "of_find_node_by_name" function in order to populate child node
> > properties.
> >
> > Here, also same case for TPS6507x MFD device
> >
> > TPS6507x MFD device has two childs:
> > 1. regulator
> > 2. touch screen.
> >
> > Regulator driver for TPS6507x is already in mainline and using function
> > "of_find_node_by_name" to populate device tree properties for
> > regulators.
> > On similar lines I used "of_find_node_by_name" to populate device
> > tree properties for touch screen.
> >
> > Still, I agreed that it is possible to attach OF data to child platform
> > device
> > but it requires changes in MFD driver of TPS6507x.
> >
> > Please let me know your opinion about this.
>
> Please let me know so accordingly I will make changes in code.
>

Please provide your comments so accordingly will make changes in code.

Regards
Manish Badarkhe

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Manish Badarkhe]

[-- Attachment #1.1: Type: text/plain, Size: 3266 bytes --]

Hi Dmitry,

On Mon, Jul 29, 2013 at 11:47 AM, Manish Badarkhe <badarkhe.manish-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> wrote:

> Hi Dmitry
>
> On Mon, Jul 1, 2013 at 4:40 PM, Manish Badarkhe
> <badarkhe.manish-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > Hi Dmitry
> >
> >
> > On Wed, Jun 12, 2013 at 12:23 PM, Vishwanathrao Badarkhe, Manish
> > <manishv.b-l0cyMroinI0@public.gmane.org> wrote:
> > > Hi Dmitry,
> > >
> > > On Mon, Jun 10, 2013 at 23:33:11, Dmitry Torokhov wrote:
> > >> Manish,
> > >>
> > >> On Mon, Jun 10, 2013 at 10:23:16AM +0000, Vishwanathrao Badarkhe,
> > >> Manish wrote:
> > >> > Hi Dmitry,
> > >> >
> > >> > On Mon, Jun 10, 2013 at 11:34:42, Dmitry Torokhov wrote:
> > >> > > Hi Manish,
> > >> > >
> > >> > > On Tue, May 21, 2013 at 02:24:17PM +0530, Vishwanathrao Badarkhe,
> > >> > > Manish wrote:
> > >> > >
> > >> > > > +       struct touchscreen_init_data *init_data = NULL;
> > >> > > > +       int err;
> > >> > > > +
> > >> > > > +       if (node)
> > >> > > > +               node = of_find_node_by_name(node, "tsc");
> > >> > >
> > >> > > Why do you have to locate OF node manually instead of already
> > >> > > having it attached to the device stucture?
> > >> >
> > >> > As TPS6507x is mfd device containing two nodes, regulator and
> > >> > touchscreen.
> > >> > It is necessary to use "of_find_node_by_name" to find child "tsc"
> > >> > node
> > >> > of TPS6507x MFD device.
> > >>
> > >> I understand that TPS6507x is a MFD device, However, I still do not
> > >> understand why you do not attach OF data to the child platform device
> > >> representing touch screen when you create it.
> > >
> > > I gone through most of MFD devices in mainline kernel where
> > > "of_find_node_by_name"
> > > function is used in order to populate child node properties.
> > > See below example of max8925 MFD device.
> > >
> > > Max8025 MFD device has following modules
> > > 1. Touch screen
> > > 2. Charger
> > > 3. Backlight
> > > 4. regulator
> > >
> > > In DT case, device node for max8925 MFD device is in
> > > "arch/arm/boot/dts/mmp2-brownstone.dts"
> > > file having child nodes as regulator, backlight and charger.
> > > Respective drivers (regulator, backlight and charger) of max8925 MFD
> > > device used
> > > "of_find_node_by_name" function in order to populate child node
> > > properties.
> > >
> > > Here, also same case for TPS6507x MFD device
> > >
> > > TPS6507x MFD device has two childs:
> > > 1. regulator
> > > 2. touch screen.
> > >
> > > Regulator driver for TPS6507x is already in mainline and using function
> > > "of_find_node_by_name" to populate device tree properties for
> > > regulators.
> > > On similar lines I used "of_find_node_by_name" to populate device
> > > tree properties for touch screen.
> > >
> > > Still, I agreed that it is possible to attach OF data to child platform
> > > device
> > > but it requires changes in MFD driver of TPS6507x.
> > >
> > > Please let me know your opinion about this.
> >
> > Please let me know so accordingly I will make changes in code.
> >
>
> Please provide your comments so accordingly will make changes in code.
>
>    Please let me know your comments so I will do necessary modifications
   and post this series again.

Regards
Manish Badarkhe

[-- Attachment #1.2: Type: text/html, Size: 4853 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Prabhakar Lad]

Hi Manish,

Thanks for the patch.

On Tue, May 21, 2013 at 2:24 PM, Vishwanathrao Badarkhe, Manish
<manishv.b@ti.com> wrote:
> Add device tree based support for TI's tps6507x touchscreen.
>
> Tested on da850-evm.
>
> Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
> ---
> Changes since V2:
>  - Removed unnecessary code.
>  - Updated Documentation to provide proper names of
>    devicetree properties.
>
> Changes since V1:
>  - Updated documentation to specify tps6507x as multifunctional
>    device.
>  - return proper error value in absence of platform and DT
>    data for touchscreen.
>  - Updated commit message.
>
> :100755 100755 8fffa3c... 65ee2cd... M  Documentation/devicetree/bindings/mfd/tps6507x.txt
> :100644 100644 65e0f9a... 89232ee... M  drivers/input/touchscreen/tps6507x-ts.c
>  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
>  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
>  2 files changed, 95 insertions(+), 31 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
> index 8fffa3c..65ee2cd 100755
> --- 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"
> @@ -23,6 +27,12 @@ Required properties:
>         vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
>         vindcdc3-supply  : VDCDC3 input.
>         vldo1_2-supply   : VLDO1 and VLDO2 input.
> +- tsc: This node specifies touch screen data.
> +       ti,poll-period : Time at which touch input is getting sampled in ms.
> +       ti,min-pressure: Minimum pressure value to trigger touch.
> +       ti,vref        : voltage reference for ADC.
> +                     0: Reference voltage for ADC is disabled.
> +                     1: Reference voltage for ADC is enabled.
>
>  Regulator Optional properties:
>  - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
> @@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
> +             in sysclass interface.
> +- ti,product: Touchscreen product id to populate
> +             in sysclass interface.
> +- ti,version: Touchscreen version id to populate
> +             in sysclass interface.
> +
>  Example:
>
>         pmu: tps6507x@48 {
> @@ -88,4 +106,12 @@ Example:
>                         };
>                 };
>
> +               tsc {
> +                       ti,poll-period = <30>;
> +                       ti,min-pressure = <0x30>;
> +                       ti,vref = <0>;
> +                       ti,vendor = <0>;
> +                       ti,product = <65070>;
> +                       ti,version = <0x100>;
> +               };
>         };
> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> index 65e0f9a..89232ee 100644
> --- a/drivers/input/touchscreen/tps6507x-ts.c
> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> @@ -21,6 +21,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
> @@ -231,36 +233,76 @@ done:
>         ret = tps6507x_adc_standby(tsc);
>  }
>
> +static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
> +               struct input_dev *input_dev)
> +{
> +       struct device_node *node = tps6507x_dev->dev->of_node;
> +       struct tps6507x_board *tps_board =
> +               (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
> +       struct touchscreen_init_data *init_data = NULL;
> +       int err;
> +
> +       if (node)
> +               node = of_find_node_by_name(node, "tsc");
> +       if (tps_board)
> +               init_data = tps_board->tps6507x_ts_init_data;
> +
> +       if (node == NULL || init_data == NULL) {
> +               err = -EINVAL;
> +               goto error_ret;
> +       } else if (init_data) {
> +               tps6507x_dev->ts->poll_period = init_data->poll_period;
> +               tps6507x_dev->ts->min_pressure = init_data->min_pressure;
> +               tps6507x_dev->ts->vref = init_data->vref;
> +               input_dev->id.vendor = init_data->vendor;
> +               input_dev->id.product = init_data->product;
> +               input_dev->id.version = init_data->version;
> +       } else if (node) {
> +               err = of_property_read_u32(node, "ti,poll-period",
> +                                       (u32 *)&tps6507x_dev->ts->poll_period);
> +               if (err < 0)
> +                       goto error_ret;
> +
> +               err = of_property_read_u16(node, "ti,min-pressure",
> +                                       &tps6507x_dev->ts->min_pressure);
> +               if (err < 0)
> +                       goto error_ret;
> +
> +               err = of_property_read_u32(node, "ti,vref",
> +                                       &tps6507x_dev->ts->vref);
> +               if (err < 0)
> +                       goto error_ret;
> +
> +               err = of_property_read_u16(node, "ti,vendor",
> +                                       &input_dev->id.vendor);
> +               if (err < 0)
> +                       goto error_ret;
> +
> +               err = of_property_read_u16(node, "ti,product",
> +                                       &input_dev->id.product);
> +               if (err < 0)
> +                       goto error_ret;
> +
> +               err = of_property_read_u16(node, "ti,version",
> +                                       &input_dev->id.version);
> +               if (err < 0)
> +                       goto error_ret;

All the three above properties as per the documentation  above
are optional, but the code here makes them required property.
May be what you can do is if property is not defined make it as
zero as per documentation.

Regards,
--Prabhakar Lad

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Manish Badarkhe]

Hi Prabhakar,

On Tue, Aug 20, 2013 at 11:44 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> Hi Manish,
>
> Thanks for the patch.
>
> On Tue, May 21, 2013 at 2:24 PM, Vishwanathrao Badarkhe, Manish
> <manishv.b@ti.com> wrote:
>> Add device tree based support for TI's tps6507x touchscreen.
>>
>> Tested on da850-evm.
>>
>> Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
>> ---
>> Changes since V2:
>>  - Removed unnecessary code.
>>  - Updated Documentation to provide proper names of
>>    devicetree properties.
>>
>> Changes since V1:
>>  - Updated documentation to specify tps6507x as multifunctional
>>    device.
>>  - return proper error value in absence of platform and DT
>>    data for touchscreen.
>>  - Updated commit message.
>>
>> :100755 100755 8fffa3c... 65ee2cd... M  Documentation/devicetree/bindings/mfd/tps6507x.txt
>> :100644 100644 65e0f9a... 89232ee... M  drivers/input/touchscreen/tps6507x-ts.c
>>  Documentation/devicetree/bindings/mfd/tps6507x.txt |   28 ++++++-
>>  drivers/input/touchscreen/tps6507x-ts.c            |   98 ++++++++++++++------
>>  2 files changed, 95 insertions(+), 31 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/tps6507x.txt b/Documentation/devicetree/bindings/mfd/tps6507x.txt
>> index 8fffa3c..65ee2cd 100755
>> --- 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"
>> @@ -23,6 +27,12 @@ Required properties:
>>         vindcdc1_2-supply: VDCDC1 and VDCDC2 input.
>>         vindcdc3-supply  : VDCDC3 input.
>>         vldo1_2-supply   : VLDO1 and VLDO2 input.
>> +- tsc: This node specifies touch screen data.
>> +       ti,poll-period : Time at which touch input is getting sampled in ms.
>> +       ti,min-pressure: Minimum pressure value to trigger touch.
>> +       ti,vref        : voltage reference for ADC.
>> +                     0: Reference voltage for ADC is disabled.
>> +                     1: Reference voltage for ADC is enabled.
>>
>>  Regulator Optional properties:
>>  - defdcdc_default: It's property of DCDC2 and DCDC3 regulators.
>> @@ -30,6 +40,14 @@ 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,vendor : Touchscreen vendor id to populate
>> +             in sysclass interface.
>> +- ti,product: Touchscreen product id to populate
>> +             in sysclass interface.
>> +- ti,version: Touchscreen version id to populate
>> +             in sysclass interface.
>> +
>>  Example:
>>
>>         pmu: tps6507x@48 {
>> @@ -88,4 +106,12 @@ Example:
>>                         };
>>                 };
>>
>> +               tsc {
>> +                       ti,poll-period = <30>;
>> +                       ti,min-pressure = <0x30>;
>> +                       ti,vref = <0>;
>> +                       ti,vendor = <0>;
>> +                       ti,product = <65070>;
>> +                       ti,version = <0x100>;
>> +               };
>>         };
>> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
>> index 65e0f9a..89232ee 100644
>> --- a/drivers/input/touchscreen/tps6507x-ts.c
>> +++ b/drivers/input/touchscreen/tps6507x-ts.c
>> @@ -21,6 +21,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
>> @@ -231,36 +233,76 @@ done:
>>         ret = tps6507x_adc_standby(tsc);
>>  }
>>
>> +static int tsc_init_data(struct tps6507x_dev *tps6507x_dev,
>> +               struct input_dev *input_dev)
>> +{
>> +       struct device_node *node = tps6507x_dev->dev->of_node;
>> +       struct tps6507x_board *tps_board =
>> +               (struct tps6507x_board *)tps6507x_dev->dev->platform_data;
>> +       struct touchscreen_init_data *init_data = NULL;
>> +       int err;
>> +
>> +       if (node)
>> +               node = of_find_node_by_name(node, "tsc");
>> +       if (tps_board)
>> +               init_data = tps_board->tps6507x_ts_init_data;
>> +
>> +       if (node == NULL || init_data == NULL) {
>> +               err = -EINVAL;
>> +               goto error_ret;
>> +       } else if (init_data) {
>> +               tps6507x_dev->ts->poll_period = init_data->poll_period;
>> +               tps6507x_dev->ts->min_pressure = init_data->min_pressure;
>> +               tps6507x_dev->ts->vref = init_data->vref;
>> +               input_dev->id.vendor = init_data->vendor;
>> +               input_dev->id.product = init_data->product;
>> +               input_dev->id.version = init_data->version;
>> +       } else if (node) {
>> +               err = of_property_read_u32(node, "ti,poll-period",
>> +                                       (u32 *)&tps6507x_dev->ts->poll_period);
>> +               if (err < 0)
>> +                       goto error_ret;
>> +
>> +               err = of_property_read_u16(node, "ti,min-pressure",
>> +                                       &tps6507x_dev->ts->min_pressure);
>> +               if (err < 0)
>> +                       goto error_ret;
>> +
>> +               err = of_property_read_u32(node, "ti,vref",
>> +                                       &tps6507x_dev->ts->vref);
>> +               if (err < 0)
>> +                       goto error_ret;
>> +
>> +               err = of_property_read_u16(node, "ti,vendor",
>> +                                       &input_dev->id.vendor);
>> +               if (err < 0)
>> +                       goto error_ret;
>> +
>> +               err = of_property_read_u16(node, "ti,product",
>> +                                       &input_dev->id.product);
>> +               if (err < 0)
>> +                       goto error_ret;
>> +
>> +               err = of_property_read_u16(node, "ti,version",
>> +                                       &input_dev->id.version);
>> +               if (err < 0)
>> +                       goto error_ret;
>
> All the three above properties as per the documentation  above
> are optional, but the code here makes them required property.
> May be what you can do is if property is not defined make it as
> zero as per documentation.

I already taken care of this as per Dmitry's comment but not yet
posted patch series as
there is discussion going on populating child node's properties.


Regards
Manish Badarkhe

Re: [PATCH V3 1/2] tps6507x-ts: Add DT support

[Dmitry Torokhov]

Hi Manish,

On Tue, Aug 20, 2013 at 12:25:32PM +0530, Manish Badarkhe wrote:
> Hi Prabhakar,
> 
> On Tue, Aug 20, 2013 at 11:44 AM, Prabhakar Lad
> >
> > All the three above properties as per the documentation  above
> > are optional, but the code here makes them required property.
> > May be what you can do is if property is not defined make it as
> > zero as per documentation.
> 
> I already taken care of this as per Dmitry's comment but not yet
> posted patch series as
> there is discussion going on populating child node's properties.

Please post the series using find node properties (as they were). We can
discuss if MFD devices should be automatically attaching parents OF data
to child devices they are creating later.

Thanks.

-- 
Dmitry