am335x: touch & adc patches

am335x: touch & adc patches

[Sebastian Andrzej Siewior]

Hi,

this is basicall a repost of the recent series. It dependent on a few
patches which were not merged yet but I assuemd otherwise.
This series now includes a reworked version of them and contains:
- pending touchscreen patche (patches 1-3, 5)
- support device tree and removal of platform device structres. The am335x
  platform is DT so there is no need for it.
- small clean ups here and there.

Sebastian

[PATCH 01/19] input: touchscreen: am335x: Step enable bits made configurable

[Sebastian Andrzej Siewior]

From: "Patil, Rachna" <rachna@ti.com>

Current code has hard coded value written to
step enable bits. Now the bits are updated based
on how many steps are needed to be configured got
from platform data.

The user needs to take care not to exceed
the count more than 16. While using ADC and TSC
one should take care to set this parameter correctly.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   10 ++++++++--
 include/linux/mfd/ti_am335x_tscadc.h      |    1 -
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 51e7b87..da652e0 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -39,6 +39,7 @@ struct titsc {
 	unsigned int		irq;
 	unsigned int		wires;
 	unsigned int		x_plate_resistance;
+	unsigned int		enable_bits;
 	bool			pen_down;
 	int			steps_to_configure;
 };
@@ -57,6 +58,7 @@ static void titsc_writel(struct titsc *tsc, unsigned int reg,
 static void titsc_step_config(struct titsc *ts_dev)
 {
 	unsigned int	config;
+	unsigned int	stepenable = 0;
 	int i, total_steps;
 
 	/* Configure the Step registers */
@@ -128,7 +130,11 @@ static void titsc_step_config(struct titsc *ts_dev)
 	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 2),
 			STEPCONFIG_OPENDLY);
 
-	titsc_writel(ts_dev, REG_SE, STPENB_STEPENB_TC);
+	for (i = 0; i <= (total_steps + 2); i++)
+		stepenable |= 1 << i;
+	ts_dev->enable_bits = stepenable;
+
+	titsc_writel(ts_dev, REG_SE, ts_dev->enable_bits);
 }
 
 static void titsc_read_coordinates(struct titsc *ts_dev,
@@ -250,7 +256,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 
 	titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
 
-	titsc_writel(ts_dev, REG_SE, STPENB_STEPENB_TC);
+	titsc_writel(ts_dev, REG_SE, ts_dev->enable_bits);
 	return IRQ_HANDLED;
 }
 
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index c79ad5d..23e4f33 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -47,7 +47,6 @@
 #define STEPENB_MASK		(0x1FFFF << 0)
 #define STEPENB(val)		((val) << 0)
 #define STPENB_STEPENB		STEPENB(0x1FFFF)
-#define STPENB_STEPENB_TC	STEPENB(0x1FFF)
 
 /* IRQ enable */
 #define IRQENB_HW_PEN		BIT(0)
-- 
1.7.10.4

[PATCH 02/19] input: touchscreen: am335x: Order of TSC wires, made configurable

[Sebastian Andrzej Siewior]

From: "Patil, Rachna" <rachna@ti.com>

The current driver expected touchscreen input
wires(XP,XN,YP,YN) to be connected in a particular order.
Making changes to accept this as platform data.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |  156 ++++++++++++++++++++++++++---
 include/linux/input/ti_am335x_tsc.h       |   12 +++
 include/linux/mfd/ti_am335x_tscadc.h      |   10 +-
 3 files changed, 159 insertions(+), 19 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index da652e0..0c460f9 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -33,6 +33,17 @@
 #define SEQ_SETTLE		275
 #define MAX_12BIT		((1 << 12) - 1)
 
+/*
+ * Refer to function regbit_map() to
+ * map the values in the matrix.
+ */
+static int config[4][4] = {
+		{1,	0,	1,	0},
+		{2,	3,	2,	3},
+		{4,	5,	4,	5},
+		{0,	6,	0,	6}
+};
+
 struct titsc {
 	struct input_dev	*input;
 	struct ti_tscadc_dev	*mfd_tscadc;
@@ -42,6 +53,9 @@ struct titsc {
 	unsigned int		enable_bits;
 	bool			pen_down;
 	int			steps_to_configure;
+	int			config_inp[20];
+	int			bit_xp, bit_xn, bit_yp, bit_yn;
+	int			inp_xp, inp_xn, inp_yp, inp_yn;
 };
 
 static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
@@ -55,6 +69,107 @@ static void titsc_writel(struct titsc *tsc, unsigned int reg,
 	writel(val, tsc->mfd_tscadc->tscadc_base + reg);
 }
 
+/*
+ * Each of the analog lines are mapped
+ * with one or two register bits,
+ * which can be either pulled high/low
+ * depending on the value to be read.
+ */
+static int regbit_map(int val)
+{
+	int map_bits = 0;
+
+	switch (val) {
+	case 1:
+		map_bits = XPP;
+		break;
+	case 2:
+		map_bits = XNP;
+		break;
+	case 3:
+		map_bits = XNN;
+		break;
+	case 4:
+		map_bits = YPP;
+		break;
+	case 5:
+		map_bits = YPN;
+		break;
+	case 6:
+		map_bits = YNN;
+		break;
+	}
+
+	return map_bits;
+}
+
+static int titsc_config_wires(struct titsc *ts_dev)
+{
+	int		analog_line[10], wire_order[10];
+	int		i, temp_bits, err;
+
+	for (i = 0; i < 4; i++) {
+		/*
+		 * Get the order in which TSC wires are attached
+		 * w.r.t. each of the analog input lines on the EVM.
+		 */
+		analog_line[i] = ts_dev->config_inp[i] & 0xF0;
+		analog_line[i] = analog_line[i] >> 4;
+
+		wire_order[i] = ts_dev->config_inp[i] & 0x0F;
+	}
+
+	for (i = 0; i < 4; i++) {
+		switch (wire_order[i]) {
+		case 0:
+			temp_bits = config[analog_line[i]][0];
+			if (temp_bits == 0) {
+				err = -EINVAL;
+				goto ret;
+			} else {
+				ts_dev->bit_xp = regbit_map(temp_bits);
+				ts_dev->inp_xp = analog_line[i];
+				break;
+			}
+		case 1:
+			temp_bits = config[analog_line[i]][1];
+			if (temp_bits == 0) {
+				err = -EINVAL;
+				goto ret;
+			} else {
+				ts_dev->bit_xn = regbit_map(temp_bits);
+				ts_dev->inp_xn = analog_line[i];
+				break;
+			}
+		case 2:
+			temp_bits = config[analog_line[i]][2];
+			if (temp_bits == 0) {
+				err = -EINVAL;
+				goto ret;
+			} else {
+				ts_dev->bit_yp = regbit_map(temp_bits);
+				ts_dev->inp_yp = analog_line[i];
+				break;
+			}
+		case 3:
+			temp_bits = config[analog_line[i]][3];
+			if (temp_bits == 0) {
+				err = -EINVAL;
+				goto ret;
+			} else {
+				ts_dev->bit_yn = regbit_map(temp_bits);
+				ts_dev->inp_yn = analog_line[i];
+				break;
+			}
+		}
+	}
+
+	return 0;
+
+ret:
+	return err;
+}
+
 static void titsc_step_config(struct titsc *ts_dev)
 {
 	unsigned int	config;
@@ -65,18 +180,18 @@ static void titsc_step_config(struct titsc *ts_dev)
 	total_steps = 2 * ts_dev->steps_to_configure;
 
 	config = STEPCONFIG_MODE_HWSYNC |
-			STEPCONFIG_AVG_16 | STEPCONFIG_XPP;
+			STEPCONFIG_AVG_16 | ts_dev->bit_xp;
 	switch (ts_dev->wires) {
 	case 4:
-		config |= STEPCONFIG_INP_AN2 | STEPCONFIG_XNN;
+		config |= STEPCONFIG_INP(ts_dev->inp_yp) | ts_dev->bit_xn;
 		break;
 	case 5:
-		config |= STEPCONFIG_YNN |
-				STEPCONFIG_INP_AN4 | STEPCONFIG_XNN |
-				STEPCONFIG_YPP;
+		config |= ts_dev->bit_yn |
+				STEPCONFIG_INP_AN4 | ts_dev->bit_xn |
+				ts_dev->bit_yp;
 		break;
 	case 8:
-		config |= STEPCONFIG_INP_AN2 | STEPCONFIG_XNN;
+		config |= STEPCONFIG_INP(ts_dev->inp_yp) | ts_dev->bit_xn;
 		break;
 	}
 
@@ -87,18 +202,18 @@ static void titsc_step_config(struct titsc *ts_dev)
 
 	config = 0;
 	config = STEPCONFIG_MODE_HWSYNC |
-			STEPCONFIG_AVG_16 | STEPCONFIG_YNN |
+			STEPCONFIG_AVG_16 | ts_dev->bit_yn |
 			STEPCONFIG_INM_ADCREFM | STEPCONFIG_FIFO1;
 	switch (ts_dev->wires) {
 	case 4:
-		config |= STEPCONFIG_YPP;
+		config |= ts_dev->bit_yp | STEPCONFIG_INP(ts_dev->inp_xp);
 		break;
 	case 5:
-		config |= STEPCONFIG_XPP | STEPCONFIG_INP_AN4 |
-				STEPCONFIG_XNP | STEPCONFIG_YPN;
+		config |= ts_dev->bit_xp | STEPCONFIG_INP_AN4 |
+				ts_dev->bit_xn | ts_dev->bit_yp;
 		break;
 	case 8:
-		config |= STEPCONFIG_YPP;
+		config |= ts_dev->bit_yp | STEPCONFIG_INP(ts_dev->inp_xp);
 		break;
 	}
 
@@ -109,9 +224,9 @@ static void titsc_step_config(struct titsc *ts_dev)
 
 	config = 0;
 	/* Charge step configuration */
-	config = STEPCONFIG_XPP | STEPCONFIG_YNN |
+	config = ts_dev->bit_xp | ts_dev->bit_yn |
 			STEPCHARGE_RFP_XPUL | STEPCHARGE_RFM_XNUR |
-			STEPCHARGE_INM_AN1 | STEPCHARGE_INP_AN1;
+			STEPCHARGE_INM_AN1 | STEPCHARGE_INP(ts_dev->inp_yp);
 
 	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
 	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
@@ -119,13 +234,14 @@ static void titsc_step_config(struct titsc *ts_dev)
 	config = 0;
 	/* Configure to calculate pressure */
 	config = STEPCONFIG_MODE_HWSYNC |
-			STEPCONFIG_AVG_16 | STEPCONFIG_YPP |
-			STEPCONFIG_XNN | STEPCONFIG_INM_ADCREFM;
+			STEPCONFIG_AVG_16 | ts_dev->bit_yp |
+			ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
+			STEPCONFIG_INP(ts_dev->inp_xp);
 	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 1), config);
 	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 1),
 			STEPCONFIG_OPENDLY);
 
-	config |= STEPCONFIG_INP_AN3 | STEPCONFIG_FIFO1;
+	config |= STEPCONFIG_INP(ts_dev->inp_yn) | STEPCONFIG_FIFO1;
 	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 2), config);
 	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 2),
 			STEPCONFIG_OPENDLY);
@@ -295,6 +411,8 @@ static int titsc_probe(struct platform_device *pdev)
 	ts_dev->wires = pdata->tsc_init->wires;
 	ts_dev->x_plate_resistance = pdata->tsc_init->x_plate_resistance;
 	ts_dev->steps_to_configure = pdata->tsc_init->steps_to_configure;
+	memcpy(ts_dev->config_inp, pdata->tsc_init->wire_config,
+			sizeof(pdata->tsc_init->wire_config));
 
 	err = request_irq(ts_dev->irq, titsc_irq,
 			  0, pdev->dev.driver->name, ts_dev);
@@ -304,6 +422,11 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	titsc_writel(ts_dev, REG_IRQENABLE, IRQENB_FIFO0THRES);
+	err = titsc_config_wires(ts_dev);
+	if (err) {
+		dev_err(&pdev->dev, "wrong i/p wire configuration\n");
+		goto err_free_irq;
+	}
 	titsc_step_config(ts_dev);
 	titsc_writel(ts_dev, REG_FIFO0THR, ts_dev->steps_to_configure);
 
@@ -373,6 +496,7 @@ static int titsc_resume(struct device *dev)
 				0x00);
 		titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
 	}
+	titsc_config_wires(ts_dev);
 	titsc_step_config(ts_dev);
 	titsc_writel(ts_dev, REG_FIFO0THR,
 			ts_dev->steps_to_configure);
diff --git a/include/linux/input/ti_am335x_tsc.h b/include/linux/input/ti_am335x_tsc.h
index 49269a2..6a66b4d 100644
--- a/include/linux/input/ti_am335x_tsc.h
+++ b/include/linux/input/ti_am335x_tsc.h
@@ -12,12 +12,24 @@
  *			A step configured to read a single
  *			co-ordinate value, can be applied
  *			more number of times for better results.
+ * @wire_config:	Different EVM's could have a different order
+ *			for connecting wires on touchscreen.
+ *			We need to provide an 8 bit number where in
+ *			the 1st four bits represent the analog lines
+ *			and the next 4 bits represent positive/
+ *			negative terminal on that input line.
+ *			Notations to represent the input lines and
+ *			terminals resoectively is as follows:
+ *			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
+ *			XP  = 0, XN = 1, YP = 2, YN = 3.
+ *
  */
 
 struct tsc_data {
 	int wires;
 	int x_plate_resistance;
 	int steps_to_configure;
+	int wire_config[10];
 };
 
 #endif
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 23e4f33..9624fea 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -72,8 +72,6 @@
 #define STEPCONFIG_INM_ADCREFM	STEPCONFIG_INM(8)
 #define STEPCONFIG_INP_MASK	(0xF << 19)
 #define STEPCONFIG_INP(val)	((val) << 19)
-#define STEPCONFIG_INP_AN2	STEPCONFIG_INP(2)
-#define STEPCONFIG_INP_AN3	STEPCONFIG_INP(3)
 #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
 #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
 #define STEPCONFIG_FIFO1	BIT(26)
@@ -95,7 +93,6 @@
 #define STEPCHARGE_INM_AN1	STEPCHARGE_INM(1)
 #define STEPCHARGE_INP_MASK	(0xF << 19)
 #define STEPCHARGE_INP(val)	((val) << 19)
-#define STEPCHARGE_INP_AN1	STEPCHARGE_INP(1)
 #define STEPCHARGE_RFM_MASK	(3 << 23)
 #define STEPCHARGE_RFM(val)	((val) << 23)
 #define STEPCHARGE_RFM_XNUR	STEPCHARGE_RFM(1)
@@ -117,6 +114,13 @@
 #define CNTRLREG_8WIRE		CNTRLREG_AFE_CTRL(3)
 #define CNTRLREG_TSCENB		BIT(7)
 
+#define XPP			STEPCONFIG_XPP
+#define XNP			STEPCONFIG_XNP
+#define XNN			STEPCONFIG_XNN
+#define YPP			STEPCONFIG_YPP
+#define YPN			STEPCONFIG_YPN
+#define YNN			STEPCONFIG_YNN
+
 #define ADC_CLK			3000000
 #define	MAX_CLK_DIV		7
 #define TOTAL_STEPS		16
-- 
1.7.10.4

[PATCH 03/19] input: touchscreen: am335x: remove unwanted fifo flush

[Sebastian Andrzej Siewior]

From: "Patil, Rachna" <rachna@ti.com>

When touchscreen and ADC are used together, this
unwanted fifo flush leads to loss of ADC data.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 0c460f9..064d2b2 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -308,8 +308,6 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int x = 0, y = 0;
 	unsigned int z1, z2, z;
 	unsigned int fsm;
-	unsigned int fifo1count, fifo0count;
-	int i;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
 	if (status & IRQENB_FIFO0THRES) {
@@ -318,14 +316,6 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 		z1 = titsc_readl(ts_dev, REG_FIFO0) & 0xfff;
 		z2 = titsc_readl(ts_dev, REG_FIFO1) & 0xfff;
 
-		fifo1count = titsc_readl(ts_dev, REG_FIFO1CNT);
-		for (i = 0; i < fifo1count; i++)
-			titsc_readl(ts_dev, REG_FIFO1);
-
-		fifo0count = titsc_readl(ts_dev, REG_FIFO0CNT);
-		for (i = 0; i < fifo0count; i++)
-			titsc_readl(ts_dev, REG_FIFO0);
-
 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
 			/*
 			 * Calculate pressure using formula
-- 
1.7.10.4

[PATCH 04/19] input: touchscreen: am335x: Add DT support

[Sebastian Andrzej Siewior]

From: "Patil, Rachna" <rachna@ti.com>

Add DT support for client touchscreen driver

[ panto@antoniou-consulting.com : use of_get_child_by_name
	instead of of_find_node_by_name ]

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
[ bigeasy: shift the code to the left ]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   87 ++++++++++++++++++++++++-----
 1 file changed, 74 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 064d2b2..be5cb67 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -26,6 +26,8 @@
 #include <linux/io.h>
 #include <linux/input/ti_am335x_tsc.h>
 #include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 
@@ -366,6 +368,67 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
+					struct titsc *ts_dev)
+{
+	struct device_node *node = tscadc_dev->dev->of_node;
+	int err, i;
+	u32 val32, wires_conf[4];
+
+	if (!node)
+		return -EINVAL;
+
+	node = of_get_child_by_name(node, "tsc");
+	if (!node)
+		return -EINVAL;
+	err = of_property_read_u32(node, "ti,wires", &val32);
+	if (err < 0)
+		goto error_ret;
+	ts_dev->wires = val32;
+
+	err = of_property_read_u32(node,
+			"ti,x-plate-resistance", &val32);
+	if (err < 0)
+		goto error_ret;
+	ts_dev->x_plate_resistance = val32;
+
+	err = of_property_read_u32(node,
+			"ti,steps-to-configure", &val32);
+	if (err < 0)
+		goto error_ret;
+	ts_dev->steps_to_configure = val32;
+
+	err = of_property_read_u32_array(node, "ti,wire-config",
+			wires_conf, ARRAY_SIZE(wires_conf));
+	if (err < 0)
+		goto error_ret;
+
+	for (i = 0; i < ARRAY_SIZE(wires_conf); i++)
+		ts_dev->config_inp[i] = wires_conf[i];
+	return 0;
+
+error_ret:
+	return err;
+}
+
+static int titsc_parse_pdata(struct ti_tscadc_dev *tscadc_dev,
+					struct titsc *ts_dev)
+{
+	struct mfd_tscadc_board	*pdata = tscadc_dev->dev->platform_data;
+
+	if (!pdata)
+		return -EINVAL;
+
+	ts_dev->wires = pdata->tsc_init->wires;
+	ts_dev->x_plate_resistance =
+		pdata->tsc_init->x_plate_resistance;
+	ts_dev->steps_to_configure =
+		pdata->tsc_init->steps_to_configure;
+	memcpy(ts_dev->config_inp, pdata->tsc_init->wire_config,
+		sizeof(pdata->tsc_init->wire_config));
+	return 0;
+}
+
 /*
  * The functions for inserting/removing driver as a module.
  */
@@ -375,16 +438,8 @@ static int titsc_probe(struct platform_device *pdev)
 	struct titsc *ts_dev;
 	struct input_dev *input_dev;
 	struct ti_tscadc_dev *tscadc_dev = pdev->dev.platform_data;
-	struct mfd_tscadc_board	*pdata;
 	int err;
 
-	pdata = tscadc_dev->dev->platform_data;
-
-	if (!pdata) {
-		dev_err(&pdev->dev, "Could not find platform data\n");
-		return -EINVAL;
-	}
-
 	/* Allocate memory for device */
 	ts_dev = kzalloc(sizeof(struct titsc), GFP_KERNEL);
 	input_dev = input_allocate_device();
@@ -398,11 +453,17 @@ static int titsc_probe(struct platform_device *pdev)
 	ts_dev->mfd_tscadc = tscadc_dev;
 	ts_dev->input = input_dev;
 	ts_dev->irq = tscadc_dev->irq;
-	ts_dev->wires = pdata->tsc_init->wires;
-	ts_dev->x_plate_resistance = pdata->tsc_init->x_plate_resistance;
-	ts_dev->steps_to_configure = pdata->tsc_init->steps_to_configure;
-	memcpy(ts_dev->config_inp, pdata->tsc_init->wire_config,
-			sizeof(pdata->tsc_init->wire_config));
+
+	if (tscadc_dev->dev->platform_data)
+		err = titsc_parse_pdata(tscadc_dev, ts_dev);
+	else
+		err = titsc_parse_dt(tscadc_dev, ts_dev);
+
+	if (err) {
+		dev_err(&pdev->dev, "Could not find platform data\n");
+		err = -EINVAL;
+		goto err_free_mem;
+	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
 			  0, pdev->dev.driver->name, ts_dev);
-- 
1.7.10.4

[PATCH 05/19] input: ti_am335x_tsc: Add variance filters

[Sebastian Andrzej Siewior]

From: "Patil, Rachna" <rachna@ti.com>

Only fine tuning variance present in tslib utility
does not help in removing all the wanted ADC noise.

This logic of filtering is necessary to get this
touchscreen to work finely.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index be5cb67..b7d3c23 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -34,6 +34,8 @@
 #define ADCFSM_STEPID		0x10
 #define SEQ_SETTLE		275
 #define MAX_12BIT		((1 << 12) - 1)
+#define TSCADC_DELTA_X		15
+#define TSCADC_DELTA_Y		15
 
 /*
  * Refer to function regbit_map() to
@@ -53,6 +55,8 @@ struct titsc {
 	unsigned int		wires;
 	unsigned int		x_plate_resistance;
 	unsigned int		enable_bits;
+	unsigned int		bckup_x;
+	unsigned int		bckup_y;
 	bool			pen_down;
 	int			steps_to_configure;
 	int			config_inp[20];
@@ -310,11 +314,18 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int x = 0, y = 0;
 	unsigned int z1, z2, z;
 	unsigned int fsm;
+	unsigned int diffx = 0, diffy = 0;
+	int i;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
 	if (status & IRQENB_FIFO0THRES) {
 		titsc_read_coordinates(ts_dev, &x, &y);
 
+		diffx = abs(x - (ts_dev->bckup_x));
+		diffy = abs(y - (ts_dev->bckup_y));
+		ts_dev->bckup_x = x;
+		ts_dev->bckup_y = y;
+
 		z1 = titsc_readl(ts_dev, REG_FIFO0) & 0xfff;
 		z2 = titsc_readl(ts_dev, REG_FIFO1) & 0xfff;
 
@@ -330,7 +341,8 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 			z /= z1;
 			z = (z + 2047) >> 12;
 
-			if (z <= MAX_12BIT) {
+			if ((diffx < TSCADC_DELTA_X) &&
+			(diffy < TSCADC_DELTA_Y) && (z <= MAX_12BIT)) {
 				input_report_abs(input_dev, ABS_X, x);
 				input_report_abs(input_dev, ABS_Y, y);
 				input_report_abs(input_dev, ABS_PRESSURE, z);
@@ -353,6 +365,8 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 		fsm = titsc_readl(ts_dev, REG_ADCFSM);
 		if (fsm == ADCFSM_STEPID) {
 			ts_dev->pen_down = false;
+			ts_dev->bckup_x = 0;
+			ts_dev->bckup_y = 0;
 			input_report_key(input_dev, BTN_TOUCH, 0);
 			input_report_abs(input_dev, ABS_PRESSURE, 0);
 			input_sync(input_dev);
-- 
1.7.10.4

Re: [PATCH 05/19] input: ti_am335x_tsc: Add variance filters

[Dmitry Torokhov]

Hi,

On Mon, May 27, 2013 at 09:11:52PM +0200, Sebastian Andrzej Siewior wrote:
> From: "Patil, Rachna" <rachna@ti.com>
> 
> Only fine tuning variance present in tslib utility
> does not help in removing all the wanted ADC noise.
> 
> This logic of filtering is necessary to get this
> touchscreen to work finely.

So why do not we enhancing tslib so that it is useful for more
touchscreen?

FWIW I have been asking this question from the very beginning of this
driver being submitted.

-- 
Dmitry

Re: [PATCH 05/19] input: ti_am335x_tsc: Add variance filters

[Sebastian Andrzej Siewior]

* Dmitry Torokhov | 2013-06-04 09:43:49 [-0700]:

>Hi,
Hi Dmitry,

>On Mon, May 27, 2013 at 09:11:52PM +0200, Sebastian Andrzej Siewior wrote:
>> From: "Patil, Rachna" <rachna@ti.com>
>> 
>> Only fine tuning variance present in tslib utility
>> does not help in removing all the wanted ADC noise.
>> 
>> This logic of filtering is necessary to get this
>> touchscreen to work finely.
>
>So why do not we enhancing tslib so that it is useful for more
>touchscreen?

I am reworking the series and dropped this patch. I think it got added
because the driver currently mixes up sometimes some of the Y & Z
coordinates. So I check how it behaves and if required I try to push to
tslib if still required. I doubt someone is using this directly on the
eventX node.

>FWIW I have been asking this question from the very beginning of this
>driver being submitted.

I'm sorry. I'm currently trying to get the three pieces of the driver
(mfd, touch, iio) into shape. If there is anything please let me know.

Sebastian

[PATCH 06/19] input/ti_am33x_tsc: remove platform_data support

[Sebastian Andrzej Siewior]

This patch removes access to platform data mfd_tscadc_board because the
platform is DT only.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index b7d3c23..b467196 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -24,7 +24,6 @@
 #include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
-#include <linux/input/ti_am335x_tsc.h>
 #include <linux/delay.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -425,24 +424,6 @@ static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
 	return err;
 }
 
-static int titsc_parse_pdata(struct ti_tscadc_dev *tscadc_dev,
-					struct titsc *ts_dev)
-{
-	struct mfd_tscadc_board	*pdata = tscadc_dev->dev->platform_data;
-
-	if (!pdata)
-		return -EINVAL;
-
-	ts_dev->wires = pdata->tsc_init->wires;
-	ts_dev->x_plate_resistance =
-		pdata->tsc_init->x_plate_resistance;
-	ts_dev->steps_to_configure =
-		pdata->tsc_init->steps_to_configure;
-	memcpy(ts_dev->config_inp, pdata->tsc_init->wire_config,
-		sizeof(pdata->tsc_init->wire_config));
-	return 0;
-}
-
 /*
  * The functions for inserting/removing driver as a module.
  */
@@ -468,13 +449,9 @@ static int titsc_probe(struct platform_device *pdev)
 	ts_dev->input = input_dev;
 	ts_dev->irq = tscadc_dev->irq;
 
-	if (tscadc_dev->dev->platform_data)
-		err = titsc_parse_pdata(tscadc_dev, ts_dev);
-	else
-		err = titsc_parse_dt(tscadc_dev, ts_dev);
-
+	err = titsc_parse_dt(tscadc_dev, ts_dev);
 	if (err) {
-		dev_err(&pdev->dev, "Could not find platform data\n");
+		dev_err(&pdev->dev, "Could not find valid DT data.\n");
 		err = -EINVAL;
 		goto err_free_mem;
 	}
-- 
1.7.10.4

Re: [PATCH 06/19] input/ti_am33x_tsc: remove platform_data support

[Dmitry Torokhov]

On Mon, May 27, 2013 at 09:11:53PM +0200, Sebastian Andrzej Siewior wrote:
> This patch removes access to platform data mfd_tscadc_board because the
> platform is DT only.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |   27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index b7d3c23..b467196 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -24,7 +24,6 @@
>  #include <linux/clk.h>
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
> -#include <linux/input/ti_am335x_tsc.h>
>  #include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -425,24 +424,6 @@ static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
>  	return err;
>  }
>  
> -static int titsc_parse_pdata(struct ti_tscadc_dev *tscadc_dev,
> -					struct titsc *ts_dev)
> -{
> -	struct mfd_tscadc_board	*pdata = tscadc_dev->dev->platform_data;
> -
> -	if (!pdata)
> -		return -EINVAL;
> -
> -	ts_dev->wires = pdata->tsc_init->wires;
> -	ts_dev->x_plate_resistance =
> -		pdata->tsc_init->x_plate_resistance;
> -	ts_dev->steps_to_configure =
> -		pdata->tsc_init->steps_to_configure;
> -	memcpy(ts_dev->config_inp, pdata->tsc_init->wire_config,
> -		sizeof(pdata->tsc_init->wire_config));
> -	return 0;
> -}
> -
>  /*
>   * The functions for inserting/removing driver as a module.
>   */
> @@ -468,13 +449,9 @@ static int titsc_probe(struct platform_device *pdev)
>  	ts_dev->input = input_dev;
>  	ts_dev->irq = tscadc_dev->irq;
>  
> -	if (tscadc_dev->dev->platform_data)
> -		err = titsc_parse_pdata(tscadc_dev, ts_dev);
> -	else
> -		err = titsc_parse_dt(tscadc_dev, ts_dev);
> -
> +	err = titsc_parse_dt(tscadc_dev, ts_dev);
>  	if (err) {
> -		dev_err(&pdev->dev, "Could not find platform data\n");
> +		dev_err(&pdev->dev, "Could not find valid DT data.\n");
>  		err = -EINVAL;
>  		goto err_free_mem;
>  	}
> -- 
> 1.7.10.4
> 

-- 
Dmitry

[PATCH 07/19] iio: adc: am335x: Add DT support

[Sebastian Andrzej Siewior]

From: "Patil, Rachna" <rachna@ti.com>

Add DT support for client ADC driver.

[ panto@antoniou-consulting.com : use of_get_child_by_name
	instead of of_find_node_by_name ]

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
[bigeasy: shift the code to the left]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c |   23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 5f9a7e7..e657709 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -22,6 +22,8 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/iio/iio.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 #include <linux/platform_data/ti_am335x_adc.h>
@@ -141,11 +143,12 @@ static int tiadc_probe(struct platform_device *pdev)
 	struct iio_dev		*indio_dev;
 	struct tiadc_device	*adc_dev;
 	struct ti_tscadc_dev	*tscadc_dev = pdev->dev.platform_data;
-	struct mfd_tscadc_board	*pdata;
+	struct mfd_tscadc_board	*pdata = tscadc_dev->dev->platform_data;
+	struct device_node	*node = tscadc_dev->dev->of_node;
 	int			err;
+	u32			val32;
 
-	pdata = tscadc_dev->dev->platform_data;
-	if (!pdata || !pdata->adc_init) {
+	if (!pdata && !node) {
 		dev_err(&pdev->dev, "Could not find platform data\n");
 		return -EINVAL;
 	}
@@ -159,7 +162,19 @@ static int tiadc_probe(struct platform_device *pdev)
 	adc_dev = iio_priv(indio_dev);
 
 	adc_dev->mfd_tscadc = tscadc_dev;
-	adc_dev->channels = pdata->adc_init->adc_channels;
+
+	if (pdata)
+		adc_dev->channels = pdata->adc_init->adc_channels;
+	else {
+		node = of_get_child_by_name(node, "adc");
+		if (!node)
+			return  -EINVAL;
+		err = of_property_read_u32(node,
+				"ti,adc-channels", &val32);
+		if (err < 0)
+			goto err_free_device;
+		adc_dev->channels = val32;
+	}
 
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->name = dev_name(&pdev->dev);
-- 
1.7.10.4

Re: [PATCH 07/19] iio: adc: am335x: Add DT support

[Jonathan Cameron]

On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
> From: "Patil, Rachna" <rachna@ti.com>
> 
> Add DT support for client ADC driver.
> 
> [ panto@antoniou-consulting.com : use of_get_child_by_name
> 	instead of of_find_node_by_name ]
> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Patil, Rachna <rachna@ti.com>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> [bigeasy: shift the code to the left]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>

I would take this directly but then I'd imagine that will cause havoc
when the later patches are applied and touch both iio and mfd.


> ---
>  drivers/iio/adc/ti_am335x_adc.c |   23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 5f9a7e7..e657709 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -22,6 +22,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/iio/iio.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  
>  #include <linux/mfd/ti_am335x_tscadc.h>
>  #include <linux/platform_data/ti_am335x_adc.h>
> @@ -141,11 +143,12 @@ static int tiadc_probe(struct platform_device *pdev)
>  	struct iio_dev		*indio_dev;
>  	struct tiadc_device	*adc_dev;
>  	struct ti_tscadc_dev	*tscadc_dev = pdev->dev.platform_data;
> -	struct mfd_tscadc_board	*pdata;
> +	struct mfd_tscadc_board	*pdata = tscadc_dev->dev->platform_data;
> +	struct device_node	*node = tscadc_dev->dev->of_node;
>  	int			err;
> +	u32			val32;
>  
> -	pdata = tscadc_dev->dev->platform_data;
> -	if (!pdata || !pdata->adc_init) {
> +	if (!pdata && !node) {
>  		dev_err(&pdev->dev, "Could not find platform data\n");
>  		return -EINVAL;
>  	}
> @@ -159,7 +162,19 @@ static int tiadc_probe(struct platform_device *pdev)
>  	adc_dev = iio_priv(indio_dev);
>  
>  	adc_dev->mfd_tscadc = tscadc_dev;
> -	adc_dev->channels = pdata->adc_init->adc_channels;
> +
> +	if (pdata)
> +		adc_dev->channels = pdata->adc_init->adc_channels;
> +	else {
> +		node = of_get_child_by_name(node, "adc");
> +		if (!node)
> +			return  -EINVAL;
> +		err = of_property_read_u32(node,
> +				"ti,adc-channels", &val32);
> +		if (err < 0)
> +			goto err_free_device;
> +		adc_dev->channels = val32;
> +	}
>  
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->name = dev_name(&pdev->dev);
> 

Re: [PATCH 07/19] iio: adc: am335x: Add DT support

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 17:58:54 [+0100]:

>On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
>> From: "Patil, Rachna" <rachna@ti.com>
>> 
>> Add DT support for client ADC driver.
>> 
>> [ panto@antoniou-consulting.com : use of_get_child_by_name
>> 	instead of of_find_node_by_name ]
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> Signed-off-by: Patil, Rachna <rachna@ti.com>
>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>> [bigeasy: shift the code to the left]
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>Acked-by: Jonathan Cameron <jic23@kernel.org>
>
>I would take this directly but then I'd imagine that will cause havoc
>when the later patches are applied and touch both iio and mfd.

Yeah, I have no idea what to do here since it would be best to grab the
series in one go. The fact that I hear nothing from input/mfd does not
make it easier :)

Sebastian

Re: [PATCH 07/19] iio: adc: am335x: Add DT support

[Dmitry Torokhov]

On Tue, Jun 04, 2013 at 12:12:02PM +0200, Sebastian Andrzej Siewior wrote:
> * Jonathan Cameron | 2013-06-02 17:58:54 [+0100]:
> 
> >On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
> >> From: "Patil, Rachna" <rachna@ti.com>
> >> 
> >> Add DT support for client ADC driver.
> >> 
> >> [ panto@antoniou-consulting.com : use of_get_child_by_name
> >> 	instead of of_find_node_by_name ]
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> >> Signed-off-by: Patil, Rachna <rachna@ti.com>
> >> Signed-off-by: Felipe Balbi <balbi@ti.com>
> >> [bigeasy: shift the code to the left]
> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >Acked-by: Jonathan Cameron <jic23@kernel.org>
> >
> >I would take this directly but then I'd imagine that will cause havoc
> >when the later patches are applied and touch both iio and mfd.
> 
> Yeah, I have no idea what to do here since it would be best to grab the
> series in one go. The fact that I hear nothing from input/mfd does not
> make it easier :)

It looks like it will have to go through MFD tree.

Thanks.

-- 
Dmitry

[PATCH 08/19] iio/ti_am335x_adc: remove platform_data support

[Sebastian Andrzej Siewior]

This patch removes access to platform data mfd_tscadc_board because the
platform is DT only.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c |   26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index e657709..d821d88 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -26,7 +26,6 @@
 #include <linux/of_device.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
-#include <linux/platform_data/ti_am335x_adc.h>
 
 struct tiadc_device {
 	struct ti_tscadc_dev *mfd_tscadc;
@@ -143,13 +142,12 @@ static int tiadc_probe(struct platform_device *pdev)
 	struct iio_dev		*indio_dev;
 	struct tiadc_device	*adc_dev;
 	struct ti_tscadc_dev	*tscadc_dev = pdev->dev.platform_data;
-	struct mfd_tscadc_board	*pdata = tscadc_dev->dev->platform_data;
 	struct device_node	*node = tscadc_dev->dev->of_node;
 	int			err;
 	u32			val32;
 
-	if (!pdata && !node) {
-		dev_err(&pdev->dev, "Could not find platform data\n");
+	if (!node) {
+		dev_err(&pdev->dev, "Could not find valid DT data.\n");
 		return -EINVAL;
 	}
 
@@ -163,18 +161,14 @@ static int tiadc_probe(struct platform_device *pdev)
 
 	adc_dev->mfd_tscadc = tscadc_dev;
 
-	if (pdata)
-		adc_dev->channels = pdata->adc_init->adc_channels;
-	else {
-		node = of_get_child_by_name(node, "adc");
-		if (!node)
-			return  -EINVAL;
-		err = of_property_read_u32(node,
-				"ti,adc-channels", &val32);
-		if (err < 0)
-			goto err_free_device;
-		adc_dev->channels = val32;
-	}
+	node = of_get_child_by_name(node, "adc");
+	if (!node)
+		return  -EINVAL;
+	err = of_property_read_u32(node,
+			"ti,adc-channels", &val32);
+	if (err < 0)
+		goto err_free_device;
+	adc_dev->channels = val32;
 
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->name = dev_name(&pdev->dev);
-- 
1.7.10.4

Re: [PATCH 08/19] iio/ti_am335x_adc: remove platform_data support

[Jonathan Cameron]

On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
> This patch removes access to platform data mfd_tscadc_board because the
> platform is DT only.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/iio/adc/ti_am335x_adc.c |   26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index e657709..d821d88 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -26,7 +26,6 @@
>  #include <linux/of_device.h>
>  
>  #include <linux/mfd/ti_am335x_tscadc.h>
> -#include <linux/platform_data/ti_am335x_adc.h>
>  
>  struct tiadc_device {
>  	struct ti_tscadc_dev *mfd_tscadc;
> @@ -143,13 +142,12 @@ static int tiadc_probe(struct platform_device *pdev)
>  	struct iio_dev		*indio_dev;
>  	struct tiadc_device	*adc_dev;
>  	struct ti_tscadc_dev	*tscadc_dev = pdev->dev.platform_data;
> -	struct mfd_tscadc_board	*pdata = tscadc_dev->dev->platform_data;
>  	struct device_node	*node = tscadc_dev->dev->of_node;
>  	int			err;
>  	u32			val32;
>  
> -	if (!pdata && !node) {
> -		dev_err(&pdev->dev, "Could not find platform data\n");
> +	if (!node) {
> +		dev_err(&pdev->dev, "Could not find valid DT data.\n");
>  		return -EINVAL;
>  	}
>  
> @@ -163,18 +161,14 @@ static int tiadc_probe(struct platform_device *pdev)
>  
>  	adc_dev->mfd_tscadc = tscadc_dev;
>  
> -	if (pdata)
> -		adc_dev->channels = pdata->adc_init->adc_channels;
> -	else {
> -		node = of_get_child_by_name(node, "adc");
> -		if (!node)
> -			return  -EINVAL;
> -		err = of_property_read_u32(node,
> -				"ti,adc-channels", &val32);
> -		if (err < 0)
> -			goto err_free_device;
> -		adc_dev->channels = val32;
> -	}
> +	node = of_get_child_by_name(node, "adc");
> +	if (!node)
> +		return  -EINVAL;
> +	err = of_property_read_u32(node,
> +			"ti,adc-channels", &val32);
> +	if (err < 0)
> +		goto err_free_device;
> +	adc_dev->channels = val32;
>  
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->name = dev_name(&pdev->dev);
> 

[PATCH 09/19] mfd: ti_am335x_tscadc: Add DT support

[Sebastian Andrzej Siewior]

From: "Patil, Rachna" <rachna@ti.com>

Make changes to add DT support in the MFD core driver.

[ panto@antoniou-consulting.com : use of_get_child_by_name
	instead of of_find_node_by_name ]

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mfd/ti_am335x_tscadc.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index e9f3fb5..7a4edc0 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -22,6 +22,8 @@
 #include <linux/regmap.h>
 #include <linux/mfd/core.h>
 #include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 #include <linux/input/ti_am335x_tsc.h>
@@ -64,20 +66,31 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	struct resource		*res;
 	struct clk		*clk;
 	struct mfd_tscadc_board	*pdata = pdev->dev.platform_data;
+	struct device_node	*node = pdev->dev.of_node;
 	struct mfd_cell		*cell;
 	int			err, ctrl;
 	int			clk_value, clock_rate;
-	int			tsc_wires, adc_channels = 0, total_channels;
+	int			tsc_wires = 0, adc_channels = 0, total_channels;
 
-	if (!pdata) {
+	if (!pdata && !pdev->dev.of_node) {
 		dev_err(&pdev->dev, "Could not find platform data\n");
 		return -EINVAL;
 	}
 
-	if (pdata->adc_init)
-		adc_channels = pdata->adc_init->adc_channels;
+	if (pdev->dev.platform_data) {
+		if (pdata->tsc_init)
+			tsc_wires = pdata->tsc_init->wires;
+
+		if (pdata->adc_init)
+			adc_channels = pdata->adc_init->adc_channels;
+	} else {
+		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
+		of_property_read_u32(node, "ti,wires", &tsc_wires);
+
+		node = of_get_child_by_name(pdev->dev.of_node, "adc");
+		of_property_read_u32(node, "ti,adc-channels", &adc_channels);
+	}
 
-	tsc_wires = pdata->tsc_init->wires;
 	total_channels = tsc_wires + adc_channels;
 
 	if (total_channels > 8) {
@@ -256,11 +269,17 @@ static const struct dev_pm_ops tscadc_pm_ops = {
 #define TSCADC_PM_OPS NULL
 #endif
 
+static const struct of_device_id ti_tscadc_dt_ids[] = {
+	{ .compatible = "ti,ti-tscadc", },
+	{ }
+};
+
 static struct platform_driver ti_tscadc_driver = {
 	.driver = {
 		.name   = "ti_tscadc",
 		.owner	= THIS_MODULE,
 		.pm	= TSCADC_PM_OPS,
+		.of_match_table = of_match_ptr(ti_tscadc_dt_ids),
 	},
 	.probe	= ti_tscadc_probe,
 	.remove	= ti_tscadc_remove,
-- 
1.7.10.4

[PATCH 10/19] mfd/ti_am335x_tscadc: remove platform_data support

[Sebastian Andrzej Siewior]

This patch removes access to platform data mfd_tscadc_board because the
platform is DT only.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mfd/ti_am335x_tscadc.c |   23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 7a4edc0..5b324e5 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -26,8 +26,6 @@
 #include <linux/of_device.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
-#include <linux/input/ti_am335x_tsc.h>
-#include <linux/platform_data/ti_am335x_adc.h>
 
 static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg)
 {
@@ -65,31 +63,22 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	struct ti_tscadc_dev	*tscadc;
 	struct resource		*res;
 	struct clk		*clk;
-	struct mfd_tscadc_board	*pdata = pdev->dev.platform_data;
 	struct device_node	*node = pdev->dev.of_node;
 	struct mfd_cell		*cell;
 	int			err, ctrl;
 	int			clk_value, clock_rate;
 	int			tsc_wires = 0, adc_channels = 0, total_channels;
 
-	if (!pdata && !pdev->dev.of_node) {
-		dev_err(&pdev->dev, "Could not find platform data\n");
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "Could not find valid DT data.\n");
 		return -EINVAL;
 	}
 
-	if (pdev->dev.platform_data) {
-		if (pdata->tsc_init)
-			tsc_wires = pdata->tsc_init->wires;
+	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
+	of_property_read_u32(node, "ti,wires", &tsc_wires);
 
-		if (pdata->adc_init)
-			adc_channels = pdata->adc_init->adc_channels;
-	} else {
-		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
-		of_property_read_u32(node, "ti,wires", &tsc_wires);
-
-		node = of_get_child_by_name(pdev->dev.of_node, "adc");
-		of_property_read_u32(node, "ti,adc-channels", &adc_channels);
-	}
+	node = of_get_child_by_name(pdev->dev.of_node, "adc");
+	of_property_read_u32(node, "ti,adc-channels", &adc_channels);
 
 	total_channels = tsc_wires + adc_channels;
 
-- 
1.7.10.4

[PATCH 11/19] iio & mfd: ti_tscadc: Update with IIO map interface & deal with partial activation

[Sebastian Andrzej Siewior]

From: Pantelis Antoniou <panto@antoniou-consulting.com>

Add an IIO map interface that consumers can use.

While we're here fix the mfd device in the case
where a subdevice might not be activated.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c      |   53 +++++++++++++++++++++++++++++-----
 drivers/mfd/ti_am335x_tscadc.c       |   29 ++++++++++++-------
 include/linux/mfd/ti_am335x_tscadc.h |    8 ++---
 3 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index d821d88..cceff09 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -20,16 +20,19 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
-#include <linux/io.h>
 #include <linux/iio/iio.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 
 struct tiadc_device {
 	struct ti_tscadc_dev *mfd_tscadc;
 	int channels;
+	char *buf;
+	struct iio_map *map;
 };
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -76,25 +79,59 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
 {
 	struct iio_chan_spec *chan_array;
-	int i;
-
-	indio_dev->num_channels = channels;
-	chan_array = kcalloc(indio_dev->num_channels,
-			sizeof(struct iio_chan_spec), GFP_KERNEL);
+	struct iio_chan_spec *chan;
+	char *s;
+	int i, len, size, ret;
 
+	size = indio_dev->num_channels * (sizeof(struct iio_chan_spec) + 6);
+	chan_array = kzalloc(size, GFP_KERNEL);
 	if (chan_array == NULL)
 		return -ENOMEM;
 
-	for (i = 0; i < (indio_dev->num_channels); i++) {
-		struct iio_chan_spec *chan = chan_array + i;
+	/* buffer space is after the array */
+	s = (char *)(chan_array + indio_dev->num_channels);
+	chan = chan_array;
+	for (i = 0; i < indio_dev->num_channels; i++, chan++, s += len + 1) {
+
+		len = sprintf(s, "AIN%d", i);
+
 		chan->type = IIO_VOLTAGE;
 		chan->indexed = 1;
 		chan->channel = i;
 		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+		chan->datasheet_name = s;
+		chan->scan_type.sign = 'u';
+		chan->scan_type.realbits = 12;
+		chan->scan_type.storagebits = 32;
+		chan->scan_type.shift = 0;
 	}
 
 	indio_dev->channels = chan_array;
 
+	size = (indio_dev->num_channels + 1) * sizeof(struct iio_map);
+	adc_dev->map = kzalloc(size, GFP_KERNEL);
+	if (adc_dev->map == NULL) {
+		kfree(chan_array);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		adc_dev->map[i].adc_channel_label =
+			chan_array[i].datasheet_name;
+		adc_dev->map[i].consumer_dev_name = "any";
+		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
+	}
+	adc_dev->map[i].adc_channel_label = NULL;
+	adc_dev->map[i].consumer_dev_name = NULL;
+	adc_dev->map[i].consumer_channel = NULL;
+
+	ret = iio_map_array_register(indio_dev, adc_dev->map);
+	if (ret != 0) {
+		kfree(adc_dev->map);
+		kfree(chan_array);
+		return -ENOMEM;
+	}
+
 	return indio_dev->num_channels;
 }
 
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 5b324e5..bd127bd 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -175,26 +175,35 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	ctrl |= CNTRLREG_TSCSSENB;
 	tscadc_writel(tscadc, REG_CTRL, ctrl);
 
+	tscadc->used_cells = 0;
+	tscadc->tsc_cell = -1;
+	tscadc->adc_cell = -1;
+
 	/* TSC Cell */
-	cell = &tscadc->cells[TSC_CELL];
-	cell->name = "tsc";
-	cell->platform_data = tscadc;
-	cell->pdata_size = sizeof(*tscadc);
+	if (tsc_wires > 0) {
+		tscadc->tsc_cell = tscadc->used_cells;
+		cell = &tscadc->cells[tscadc->used_cells++];
+		cell->name = "tsc";
+		cell->platform_data = tscadc;
+		cell->pdata_size = sizeof(*tscadc);
+	}
 
 	/* ADC Cell */
-	cell = &tscadc->cells[ADC_CELL];
-	cell->name = "tiadc";
-	cell->platform_data = tscadc;
-	cell->pdata_size = sizeof(*tscadc);
+	if (adc_channels > 0) {
+		tscadc->adc_cell = tscadc->used_cells;
+		cell = &tscadc->cells[tscadc->used_cells++];
+		cell->name = "tiadc";
+		cell->platform_data = tscadc;
+		cell->pdata_size = sizeof(*tscadc);
+	}
 
 	err = mfd_add_devices(&pdev->dev, pdev->id, tscadc->cells,
-			TSCADC_CELLS, NULL, 0, NULL);
+			tscadc->used_cells, NULL, 0, NULL);
 	if (err < 0)
 		goto err_disable_clk;
 
 	device_init_wakeup(&pdev->dev, true);
 	platform_set_drvdata(pdev, tscadc);
-
 	return 0;
 
 err_disable_clk:
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 9624fea..50a245f 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -128,11 +128,6 @@
 
 #define TSCADC_CELLS		2
 
-enum tscadc_cells {
-	TSC_CELL,
-	ADC_CELL,
-};
-
 struct mfd_tscadc_board {
 	struct tsc_data *tsc_init;
 	struct adc_data *adc_init;
@@ -143,6 +138,9 @@ struct ti_tscadc_dev {
 	struct regmap *regmap_tscadc;
 	void __iomem *tscadc_base;
 	int irq;
+	int used_cells;	/* 0-2 */
+	int tsc_cell;	/* -1 if not used */
+	int adc_cell;	/* -1 if not used */
 	struct mfd_cell cells[TSCADC_CELLS];
 
 	/* tsc device */
-- 
1.7.10.4

Re: [PATCH 11/19] iio & mfd: ti_tscadc: Update with IIO map interface & deal with partial activation

[Jonathan Cameron]

On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
> From: Pantelis Antoniou <panto@antoniou-consulting.com>
>
> Add an IIO map interface that consumers can use.
>
> While we're here fix the mfd device in the case
> where a subdevice might not be activated.
Any time I read 'while we're here' it rings alarm bells.
Two issues -> Two patches please.

The level of reworking here is rather high. Probably better
to do the structural changes in one patch then follow with the
new functionality in another.

Other comments inline.
>
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/iio/adc/ti_am335x_adc.c      |   53 +++++++++++++++++++++++++++++-----
>  drivers/mfd/ti_am335x_tscadc.c       |   29 ++++++++++++-------
>  include/linux/mfd/ti_am335x_tscadc.h |    8 ++---
>  3 files changed, 67 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index d821d88..cceff09 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -20,16 +20,19 @@
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/platform_device.h>
> -#include <linux/io.h>
why? Not immediately obvious this has anything to do with the rest of this patch.
>  #include <linux/iio/iio.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
>
>  #include <linux/mfd/ti_am335x_tscadc.h>
>
>  struct tiadc_device {
>  	struct ti_tscadc_dev *mfd_tscadc;
>  	int channels;
> +	char *buf;
> +	struct iio_map *map;
>  };
>
>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -76,25 +79,59 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>  {
>  	struct iio_chan_spec *chan_array;

> -	int i;
> -
> -	indio_dev->num_channels = channels;
> -	chan_array = kcalloc(indio_dev->num_channels,
> -			sizeof(struct iio_chan_spec), GFP_KERNEL);
> +	struct iio_chan_spec *chan;
> +	char *s;
> +	int i, len, size, ret;
>

This is less than beautiful... It'll cost you marginally more to
allocate a separate array for the strings, but will be a lot nicer
to read.  If we are dealing with a relatively small maximum number
then just do it as a static const char * array of all possible
strings.

> +	size = indio_dev->num_channels * (sizeof(struct iio_chan_spec) + 6);
> +	chan_array = kzalloc(size, GFP_KERNEL);
>  	if (chan_array == NULL)
>  		return -ENOMEM;
>
> -	for (i = 0; i < (indio_dev->num_channels); i++) {
> -		struct iio_chan_spec *chan = chan_array + i;
> +	/* buffer space is after the array */
> +	s = (char *)(chan_array + indio_dev->num_channels);
> +	chan = chan_array;
> +	for (i = 0; i < indio_dev->num_channels; i++, chan++, s += len + 1) {
> +
> +		len = sprintf(s, "AIN%d", i);
> +
>  		chan->type = IIO_VOLTAGE;
>  		chan->indexed = 1;
>  		chan->channel = i;
>  		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +		chan->datasheet_name = s;
> +		chan->scan_type.sign = 'u';
> +		chan->scan_type.realbits = 12;
> +		chan->scan_type.storagebits = 32;
> +		chan->scan_type.shift = 0;
>  	}
>
>  	indio_dev->channels = chan_array;
>
> +	size = (indio_dev->num_channels + 1) * sizeof(struct iio_map);
> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
> +	if (adc_dev->map == NULL) {
> +		kfree(chan_array);
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		adc_dev->map[i].adc_channel_label =
> +			chan_array[i].datasheet_name;
> +		adc_dev->map[i].consumer_dev_name = "any";
> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
> +	}
> +	adc_dev->map[i].adc_channel_label = NULL;
> +	adc_dev->map[i].consumer_dev_name = NULL;
> +	adc_dev->map[i].consumer_channel = NULL;
> +
> +	ret = iio_map_array_register(indio_dev, adc_dev->map);
> +	if (ret != 0) {
> +		kfree(adc_dev->map);
> +		kfree(chan_array);
> +		return -ENOMEM;
> +	}
> +
>  	return indio_dev->num_channels;
>  }
>
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index 5b324e5..bd127bd 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -175,26 +175,35 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	ctrl |= CNTRLREG_TSCSSENB;
>  	tscadc_writel(tscadc, REG_CTRL, ctrl);
>
> +	tscadc->used_cells = 0;
> +	tscadc->tsc_cell = -1;
> +	tscadc->adc_cell = -1;
> +
>  	/* TSC Cell */
> -	cell = &tscadc->cells[TSC_CELL];
> -	cell->name = "tsc";
> -	cell->platform_data = tscadc;
> -	cell->pdata_size = sizeof(*tscadc);
> +	if (tsc_wires > 0) {
> +		tscadc->tsc_cell = tscadc->used_cells;
> +		cell = &tscadc->cells[tscadc->used_cells++];
> +		cell->name = "tsc";
> +		cell->platform_data = tscadc;
> +		cell->pdata_size = sizeof(*tscadc);
> +	}
>
>  	/* ADC Cell */
> -	cell = &tscadc->cells[ADC_CELL];
> -	cell->name = "tiadc";
> -	cell->platform_data = tscadc;
> -	cell->pdata_size = sizeof(*tscadc);
> +	if (adc_channels > 0) {
> +		tscadc->adc_cell = tscadc->used_cells;
> +		cell = &tscadc->cells[tscadc->used_cells++];
> +		cell->name = "tiadc";
> +		cell->platform_data = tscadc;
> +		cell->pdata_size = sizeof(*tscadc);
> +	}
>
>  	err = mfd_add_devices(&pdev->dev, pdev->id, tscadc->cells,
> -			TSCADC_CELLS, NULL, 0, NULL);
> +			tscadc->used_cells, NULL, 0, NULL);
>  	if (err < 0)
>  		goto err_disable_clk;
>
>  	device_init_wakeup(&pdev->dev, true);
>  	platform_set_drvdata(pdev, tscadc);
> -
>  	return 0;
>
>  err_disable_clk:
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index 9624fea..50a245f 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -128,11 +128,6 @@
>
>  #define TSCADC_CELLS		2
>
> -enum tscadc_cells {
> -	TSC_CELL,
> -	ADC_CELL,
> -};
> -
>  struct mfd_tscadc_board {
>  	struct tsc_data *tsc_init;
>  	struct adc_data *adc_init;
> @@ -143,6 +138,9 @@ struct ti_tscadc_dev {
>  	struct regmap *regmap_tscadc;
>  	void __iomem *tscadc_base;
>  	int irq;
> +	int used_cells;	/* 0-2 */
> +	int tsc_cell;	/* -1 if not used */
> +	int adc_cell;	/* -1 if not used */
>  	struct mfd_cell cells[TSCADC_CELLS];
>
>  	/* tsc device */
>

Re: [PATCH 11/19] iio & mfd: ti_tscadc: Update with IIO map interface & deal with partial activation

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 18:40:27 [+0100]:

>On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
>> From: Pantelis Antoniou <panto@antoniou-consulting.com>
>>
>> Add an IIO map interface that consumers can use.
>>
>> While we're here fix the mfd device in the case
>> where a subdevice might not be activated.
>Any time I read 'while we're here' it rings alarm bells.
>Two issues -> Two patches please.
>
>The level of reworking here is rather high. Probably better
>to do the structural changes in one patch then follow with the
>new functionality in another.

Okay, will try.

>Other comments inline.
>>
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> ---
>>  drivers/iio/adc/ti_am335x_adc.c      |   53 +++++++++++++++++++++++++++++-----
>>  drivers/mfd/ti_am335x_tscadc.c       |   29 ++++++++++++-------
>>  include/linux/mfd/ti_am335x_tscadc.h |    8 ++---
>>  3 files changed, 67 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index d821d88..cceff09 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -20,16 +20,19 @@
>>  #include <linux/slab.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/platform_device.h>
>> -#include <linux/io.h>
>why? Not immediately obvious this has anything to do with the rest of this patch.

Yeah, a patch later or two it is added again :P Will drop this nonsense.

>>  #include <linux/iio/iio.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/iio/driver.h>
>>
>>  #include <linux/mfd/ti_am335x_tscadc.h>
>>
>>  struct tiadc_device {
>>  	struct ti_tscadc_dev *mfd_tscadc;
>>  	int channels;
>> +	char *buf;
>> +	struct iio_map *map;
>>  };
>>
>>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
>> @@ -76,25 +79,59 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>>  static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>>  {
>>  	struct iio_chan_spec *chan_array;
>
>> -	int i;
>> -
>> -	indio_dev->num_channels = channels;
>> -	chan_array = kcalloc(indio_dev->num_channels,
>> -			sizeof(struct iio_chan_spec), GFP_KERNEL);
>> +	struct iio_chan_spec *chan;
>> +	char *s;
>> +	int i, len, size, ret;
>>
>
>This is less than beautiful... It'll cost you marginally more to
>allocate a separate array for the strings, but will be a lot nicer
>to read.  If we are dealing with a relatively small maximum number
>then just do it as a static const char * array of all possible
>strings.

The EVM board takes 4 and the maximum is 8. This is 20 bytes on EVM and
40 for the max. This probably does not hurt in data segment, will
change.

>
>> +	size = indio_dev->num_channels * (sizeof(struct iio_chan_spec) + 6);
>> +	chan_array = kzalloc(size, GFP_KERNEL);
>>  	if (chan_array == NULL)
>>  		return -ENOMEM;

Sebastian

[PATCH 12/19] iio & mfd & input: ti_tscadc: Match mfd sub devices to regmap interface

[Sebastian Andrzej Siewior]

From: Pantelis Antoniou <panto@antoniou-consulting.com>

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c           |   26 +++++++++++++++++---------
 drivers/input/touchscreen/ti_am335x_tsc.c |   13 ++++++++++---
 drivers/mfd/ti_am335x_tscadc.c            |    1 +
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index cceff09..72ffe89 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -25,7 +25,9 @@
 #include <linux/of_device.h>
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
+#include <linux/regmap.h>
 
+#include <linux/io.h>
 #include <linux/mfd/ti_am335x_tscadc.h>
 
 struct tiadc_device {
@@ -37,13 +39,17 @@ struct tiadc_device {
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
 {
-	return readl(adc->mfd_tscadc->tscadc_base + reg);
+	unsigned int val;
+
+	val = (unsigned int)-1;
+	regmap_read(adc->mfd_tscadc->regmap_tscadc, reg, &val);
+	return val;
 }
 
 static void tiadc_writel(struct tiadc_device *adc, unsigned int reg,
 					unsigned int val)
 {
-	writel(val, adc->mfd_tscadc->tscadc_base + reg);
+	regmap_write(adc->mfd_tscadc->regmap_tscadc, reg, val);
 }
 
 static void tiadc_step_config(struct tiadc_device *adc_dev)
@@ -76,22 +82,24 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
 }
 
-static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
+static int tiadc_channel_init(struct iio_dev *indio_dev,
+		struct tiadc_device *adc_dev)
 {
 	struct iio_chan_spec *chan_array;
 	struct iio_chan_spec *chan;
 	char *s;
 	int i, len, size, ret;
+	int channels = adc_dev->channels;
 
-	size = indio_dev->num_channels * (sizeof(struct iio_chan_spec) + 6);
+	size = channels * (sizeof(struct iio_chan_spec) + 6);
 	chan_array = kzalloc(size, GFP_KERNEL);
 	if (chan_array == NULL)
 		return -ENOMEM;
 
 	/* buffer space is after the array */
-	s = (char *)(chan_array + indio_dev->num_channels);
+	s = (char *)(chan_array + channels);
 	chan = chan_array;
-	for (i = 0; i < indio_dev->num_channels; i++, chan++, s += len + 1) {
+	for (i = 0; i < channels; i++, chan++, s += len + 1) {
 
 		len = sprintf(s, "AIN%d", i);
 
@@ -107,8 +115,9 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
 	}
 
 	indio_dev->channels = chan_array;
+	indio_dev->num_channels = channels;
 
-	size = (indio_dev->num_channels + 1) * sizeof(struct iio_map);
+	size = (channels + 1) * sizeof(struct iio_map);
 	adc_dev->map = kzalloc(size, GFP_KERNEL);
 	if (adc_dev->map == NULL) {
 		kfree(chan_array);
@@ -214,7 +223,7 @@ static int tiadc_probe(struct platform_device *pdev)
 
 	tiadc_step_config(adc_dev);
 
-	err = tiadc_channel_init(indio_dev, adc_dev->channels);
+	err = tiadc_channel_init(indio_dev, adc_dev);
 	if (err < 0)
 		goto err_free_device;
 
@@ -223,7 +232,6 @@ static int tiadc_probe(struct platform_device *pdev)
 		goto err_free_channels;
 
 	platform_set_drvdata(pdev, indio_dev);
-
 	return 0;
 
 err_free_channels:
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index b467196..0dbf3df 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -27,6 +27,7 @@
 #include <linux/delay.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/regmap.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 
@@ -65,13 +66,17 @@ struct titsc {
 
 static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
 {
-	return readl(ts->mfd_tscadc->tscadc_base + reg);
+	unsigned int val;
+
+	val = (unsigned int)-1;
+	regmap_read(ts->mfd_tscadc->regmap_tscadc, reg, &val);
+	return val;
 }
 
 static void titsc_writel(struct titsc *tsc, unsigned int reg,
 					unsigned int val)
 {
-	writel(val, tsc->mfd_tscadc->tscadc_base + reg);
+	regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val);
 }
 
 /*
@@ -484,8 +489,10 @@ static int titsc_probe(struct platform_device *pdev)
 
 	/* register to the input system */
 	err = input_register_device(input_dev);
-	if (err)
+	if (err) {
+		dev_err(&pdev->dev, "Failed to register input device\n");
 		goto err_free_irq;
+	}
 
 	platform_set_drvdata(pdev, ts_dev);
 	return 0;
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index bd127bd..a27401a 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg)
 {
 	unsigned int val;
 
+	val = (unsigned int)-1;
 	regmap_read(tsadc->regmap_tscadc, reg, &val);
 	return val;
 }
-- 
1.7.10.4

Re: [PATCH 12/19] iio & mfd & input: ti_tscadc: Match mfd sub devices to regmap interface

[Jonathan Cameron]

On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
> From: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
Some confusing splitting between this and the previous patch that definitely wants
to be cleaned up.

> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/iio/adc/ti_am335x_adc.c           |   26 +++++++++++++++++---------
>  drivers/input/touchscreen/ti_am335x_tsc.c |   13 ++++++++++---
>  drivers/mfd/ti_am335x_tscadc.c            |    1 +
>  3 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index cceff09..72ffe89 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -25,7 +25,9 @@
>  #include <linux/of_device.h>
>  #include <linux/iio/machine.h>
>  #include <linux/iio/driver.h>
> +#include <linux/regmap.h>
>  
> +#include <linux/io.h>
That only went away in the previous patch...

>  #include <linux/mfd/ti_am335x_tscadc.h>
>  
>  struct tiadc_device {
> @@ -37,13 +39,17 @@ struct tiadc_device {
>  
>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
>  {
> -	return readl(adc->mfd_tscadc->tscadc_base + reg);
> +	unsigned int val;
> +
> +	val = (unsigned int)-1;
> +	regmap_read(adc->mfd_tscadc->regmap_tscadc, reg, &val);
> +	return val;
>  }
>  
>  static void tiadc_writel(struct tiadc_device *adc, unsigned int reg,
>  					unsigned int val)
>  {
> -	writel(val, adc->mfd_tscadc->tscadc_base + reg);
> +	regmap_write(adc->mfd_tscadc->regmap_tscadc, reg, val);
>  }
>  
>  static void tiadc_step_config(struct tiadc_device *adc_dev)
> @@ -76,22 +82,24 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>  }
>  
> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
> +static int tiadc_channel_init(struct iio_dev *indio_dev,
> +		struct tiadc_device *adc_dev)
>  {
>  	struct iio_chan_spec *chan_array;
>  	struct iio_chan_spec *chan;
>  	char *s;
>  	int i, len, size, ret;
> +	int channels = adc_dev->channels;
This is an unconnected bit of cleanup.  If you want it put it in the previous patch
which only just introduced what you are changing.
>  
> -	size = indio_dev->num_channels * (sizeof(struct iio_chan_spec) + 6);
> +	size = channels * (sizeof(struct iio_chan_spec) + 6);
>  	chan_array = kzalloc(size, GFP_KERNEL);
>  	if (chan_array == NULL)
>  		return -ENOMEM;
>  
>  	/* buffer space is after the array */
> -	s = (char *)(chan_array + indio_dev->num_channels);
> +	s = (char *)(chan_array + channels);
>  	chan = chan_array;
> -	for (i = 0; i < indio_dev->num_channels; i++, chan++, s += len + 1) {
> +	for (i = 0; i < channels; i++, chan++, s += len + 1) {
>  
>  		len = sprintf(s, "AIN%d", i);
>  
> @@ -107,8 +115,9 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>  	}
>  
>  	indio_dev->channels = chan_array;
> +	indio_dev->num_channels = channels;
>  
> -	size = (indio_dev->num_channels + 1) * sizeof(struct iio_map);
> +	size = (channels + 1) * sizeof(struct iio_map);
>  	adc_dev->map = kzalloc(size, GFP_KERNEL);
>  	if (adc_dev->map == NULL) {
>  		kfree(chan_array);
> @@ -214,7 +223,7 @@ static int tiadc_probe(struct platform_device *pdev)
>  
>  	tiadc_step_config(adc_dev);
>  
> -	err = tiadc_channel_init(indio_dev, adc_dev->channels);
> +	err = tiadc_channel_init(indio_dev, adc_dev);
>  	if (err < 0)
>  		goto err_free_device;
>  
> @@ -223,7 +232,6 @@ static int tiadc_probe(struct platform_device *pdev)
>  		goto err_free_channels;
>  
>  	platform_set_drvdata(pdev, indio_dev);
> -
>  	return 0;
>  
>  err_free_channels:
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index b467196..0dbf3df 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -27,6 +27,7 @@
>  #include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/regmap.h>
>  
>  #include <linux/mfd/ti_am335x_tscadc.h>
>  
> @@ -65,13 +66,17 @@ struct titsc {
>  
>  static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
>  {
> -	return readl(ts->mfd_tscadc->tscadc_base + reg);
> +	unsigned int val;
> +
> +	val = (unsigned int)-1;
> +	regmap_read(ts->mfd_tscadc->regmap_tscadc, reg, &val);
> +	return val;
>  }
>  
>  static void titsc_writel(struct titsc *tsc, unsigned int reg,
>  					unsigned int val)
>  {
> -	writel(val, tsc->mfd_tscadc->tscadc_base + reg);
> +	regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val);
>  }
>  
>  /*
> @@ -484,8 +489,10 @@ static int titsc_probe(struct platform_device *pdev)
>  
>  	/* register to the input system */
>  	err = input_register_device(input_dev);
> -	if (err)
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to register input device\n");
>  		goto err_free_irq;
> +	}
>  
>  	platform_set_drvdata(pdev, ts_dev);
>  	return 0;
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index bd127bd..a27401a 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg)
>  {
>  	unsigned int val;
>
????  What is this doing here? It's not doing the move to regmap but rather setting a default value.
> +	val = (unsigned int)-1;
>  	regmap_read(tsadc->regmap_tscadc, reg, &val);
>  	return val;
>  }
> 

Re: [PATCH 12/19] iio & mfd & input: ti_tscadc: Match mfd sub devices to regmap interface

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 18:46:29 [+0100]:

>On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
>> From: Pantelis Antoniou <panto@antoniou-consulting.com>
>> 
>Some confusing splitting between this and the previous patch that definitely wants
>to be cleaned up.
>
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index cceff09..72ffe89 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -76,22 +82,24 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>>  }
>>  
>> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>> +static int tiadc_channel_init(struct iio_dev *indio_dev,
>> +		struct tiadc_device *adc_dev)
>>  {
>>  	struct iio_chan_spec *chan_array;
>>  	struct iio_chan_spec *chan;
>>  	char *s;
>>  	int i, len, size, ret;
>> +	int channels = adc_dev->channels;
>This is an unconnected bit of cleanup.  If you want it put it in the previous patch
>which only just introduced what you are changing.

It will be probably best.

>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
>> index bd127bd..a27401a 100644
>> --- a/drivers/mfd/ti_am335x_tscadc.c
>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg)
>>  {
>>  	unsigned int val;
>>
>????  What is this doing here? It's not doing the move to regmap but rather setting a default value.

No lo sé :) I saw that and was wondering myself a little and planned the
romval of this default for later. Now I think I do this earlier.

>> +	val = (unsigned int)-1;
>>  	regmap_read(tsadc->regmap_tscadc, reg, &val);
>>  	return val;
>>  }

Sebastian

Re: [PATCH 12/19] iio & mfd & input: ti_tscadc: Match mfd sub devices to regmap interface

[Pantelis Antoniou]

Hi there,

On Jun 4, 2013, at 1:23 PM, Sebastian Andrzej Siewior wrote:

> * Jonathan Cameron | 2013-06-02 18:46:29 [+0100]:
> 
>> On 05/27/2013 08:11 PM, Sebastian Andrzej Siewior wrote:
>>> From: Pantelis Antoniou <panto@antoniou-consulting.com>
>>> 
>> Some confusing splitting between this and the previous patch that definitely wants
>> to be cleaned up.
>> 
>>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>>> index cceff09..72ffe89 100644
>>> --- a/drivers/iio/adc/ti_am335x_adc.c
>>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>>> @@ -76,22 +82,24 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>>> 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>>> }
>>> 
>>> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>>> +static int tiadc_channel_init(struct iio_dev *indio_dev,
>>> +		struct tiadc_device *adc_dev)
>>> {
>>> 	struct iio_chan_spec *chan_array;
>>> 	struct iio_chan_spec *chan;
>>> 	char *s;
>>> 	int i, len, size, ret;
>>> +	int channels = adc_dev->channels;
>> This is an unconnected bit of cleanup.  If you want it put it in the previous patch
>> which only just introduced what you are changing.
> 
> It will be probably best.
> 
>>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
>>> index bd127bd..a27401a 100644
>>> --- a/drivers/mfd/ti_am335x_tscadc.c
>>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>>> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg)
>>> {
>>> 	unsigned int val;
>>> 
>> ????  What is this doing here? It's not doing the move to regmap but rather setting a default value.
> 
> No lo sé :) I saw that and was wondering myself a little and planned the
> romval of this default for later. Now I think I do this earlier.
> 

The tscadc_read function doesn't have a failure mode; doesn't return an error in case of a regmap_read
fail. Rather that change all the callers, we make sure we don't ever return an uninitialized value 
in case of an error (when val should be unchanged).


>>> +	val = (unsigned int)-1;
>>> 	regmap_read(tsadc->regmap_tscadc, reg, &val);
>>> 	return val;
>>> }
> 
> Sebastian

Re: [PATCH 12/19] iio & mfd & input: ti_tscadc: Match mfd sub devices to regmap interface

[Sebastian Andrzej Siewior]

* Pantelis Antoniou | 2013-06-04 13:25:28 [+0300]:

>>>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
>>>> index bd127bd..a27401a 100644
>>>> --- a/drivers/mfd/ti_am335x_tscadc.c
>>>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>>>> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg)
>>>> {
>>>> 	unsigned int val;
>>>> 
>>> ????  What is this doing here? It's not doing the move to regmap but rather setting a default value.
>> 
>> No lo sé :) I saw that and was wondering myself a little and planned the
>> romval of this default for later. Now I think I do this earlier.
>> 
>
>The tscadc_read function doesn't have a failure mode; doesn't return an error in case of a regmap_read
>fail. Rather that change all the callers, we make sure we don't ever return an uninitialized value 
>in case of an error (when val should be unchanged).

One stupid question: Why did you start using regmap in the first place?

>>>> +	val = (unsigned int)-1;
>>>> 	regmap_read(tsadc->regmap_tscadc, reg, &val);
>>>> 	return val;
>>>> }

Sebastian

Re: [PATCH 12/19] iio & mfd & input: ti_tscadc: Match mfd sub devices to regmap interface

[Pantelis Antoniou]

What make you think I am the original author?

I just fixed whatever I had to fix to get it working.

Namely that the multi function device was using regmap in one place and
not in another, which had the nice effect of the regmap cache getting out of sync.

Regards

-- Pantelis

On Jun 4, 2013, at 1:52 PM, Sebastian Andrzej Siewior wrote:

> * Pantelis Antoniou | 2013-06-04 13:25:28 [+0300]:
> 
>>>>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
>>>>> index bd127bd..a27401a 100644
>>>>> --- a/drivers/mfd/ti_am335x_tscadc.c
>>>>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>>>>> @@ -31,6 +31,7 @@ static unsigned int tscadc_readl(struct ti_tscadc_dev *tsadc, unsigned int reg)
>>>>> {
>>>>> 	unsigned int val;
>>>>> 
>>>> ????  What is this doing here? It's not doing the move to regmap but rather setting a default value.
>>> 
>>> No lo sé :) I saw that and was wondering myself a little and planned the
>>> romval of this default for later. Now I think I do this earlier.
>>> 
>> 
>> The tscadc_read function doesn't have a failure mode; doesn't return an error in case of a regmap_read
>> fail. Rather that change all the callers, we make sure we don't ever return an uninitialized value 
>> in case of an error (when val should be unchanged).
> 
> One stupid question: Why did you start using regmap in the first place?
> 
>>>>> +	val = (unsigned int)-1;
>>>>> 	regmap_read(tsadc->regmap_tscadc, reg, &val);
>>>>> 	return val;
>>>>> }
> 
> Sebastian

Re: [PATCH 12/19] iio & mfd & input: ti_tscadc: Match mfd sub devices to regmap interface

[Sebastian Andrzej Siewior]

* Pantelis Antoniou | 2013-06-04 13:54:48 [+0300]:

>What make you think I am the original author?
Nothing, you just made _this_ patch.

>I just fixed whatever I had to fix to get it working.
Fair enough.

>Namely that the multi function device was using regmap in one place and
>not in another, which had the nice effect of the regmap cache getting out of sync.

Yeah, that is a pain. Currently I think why we do need regmap at all
here and I think about getting rid of it. Most of the values read here
need no caching at all.

>Regards
>
>-- Pantelis

Sebastian

Re: [PATCH 12/19] iio & mfd & input: ti_tscadc: Match mfd sub devices to regmap interface

[Pantelis Antoniou]

On Jun 4, 2013, at 2:05 PM, Sebastian Andrzej Siewior wrote:

> * Pantelis Antoniou | 2013-06-04 13:54:48 [+0300]:
> 
>> What make you think I am the original author?
> Nothing, you just made _this_ patch.
> 

I'm the author of that patch that fixes it, I'm not the author of the original driver.

>> I just fixed whatever I had to fix to get it working.
> Fair enough.
> 
>> Namely that the multi function device was using regmap in one place and
>> not in another, which had the nice effect of the regmap cache getting out of sync.
> 
> Yeah, that is a pain. Currently I think why we do need regmap at all
> here and I think about getting rid of it. Most of the values read here
> need no caching at all.
> 

You need to talk to the original driver author then.

>> Regards
>> 
>> -- Pantelis
> 
> Sebastian

Regards

-- Pantelis

[PATCH 13/19] arm: dts: am33xx: add TSC/ADC mfd device support

[Sebastian Andrzej Siewior]

From: "Patil, Rachna" <rachna@ti.com>

Add support for core multifunctional device along
with its clients touchscreen and ADC.

[ panto@antoniou-consulting.com : make sure status is
	set to 'disabled' in dtsi file. ]

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
[bigeasy: add 'status = "okay"']
Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 arch/arm/boot/dts/am335x-evm.dts |   14 ++++++++++++++
 arch/arm/boot/dts/am33xx.dtsi    |    9 +++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 0423298..be6a5b2 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -244,3 +244,17 @@
 &cpsw_emac1 {
 	phy_id = <&davinci_mdio>, <1>;
 };
+
+&tscadc {
+	status = "okay";
+	tsc {
+		ti,wires = <4>;
+		ti,x-plate-resistance = <200>;
+		ti,steps-to-configure = <5>;
+		ti,wire-config = <0x00 0x11 0x22 0x33>;
+	};
+
+	adc {
+		ti,adc-channels = <4>;
+	};
+};
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 1460d9b..e3dcff0 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -404,6 +404,15 @@
 			ti,hwmods = "wkup_m3";
 		};
 
+		tscadc: tscadc@44e0d000 {
+			compatible = "ti,ti-tscadc";
+			reg = <0x44e0d000 0x1000>;
+			interrupt-parent = <&intc>;
+			interrupts = <16>;
+			ti,hwmods = "adc_tsc";
+			status = "disabled";
+		};
+
 		gpmc: gpmc@50000000 {
 			compatible = "ti,am3352-gpmc";
 			ti,hwmods = "gpmc";
-- 
1.7.10.4

[PATCH 14/19] Documentation/DT bindings: add info for TI TSC ADC

[Sebastian Andrzej Siewior]

The platform data structs are killed here because there are no users in
tree (nobody sets the structs up, the RO usage is beeing removed) and the
documentation about the parameters is saved here.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 .../bindings/input/touchscreen/ti-tsc-adc.txt      |   38 ++++++++++++++++++++
 include/linux/input/ti_am335x_tsc.h                |   35 ------------------
 include/linux/mfd/ti_am335x_tscadc.h               |    5 ---
 include/linux/platform_data/ti_am335x_adc.h        |   14 --------
 4 files changed, 38 insertions(+), 54 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
 delete mode 100644 include/linux/input/ti_am335x_tsc.h
 delete mode 100644 include/linux/platform_data/ti_am335x_adc.h

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
new file mode 100644
index 0000000..e533e9d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -0,0 +1,38 @@
+* TI - TSC ADC (Touschscreen and analog digital converter)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Required properties:
+- child "tsc"
+	ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
+		  support on the platform.
+	ti,x-plate-resistance: X plate resistance
+	ti,steps-to-configure: The sequencer supports a total of 16
+			       programmable steps. A step configured to read a
+			       single co-ordinate value. Can be applied more
+			       number of times for better results.
+	ti,wire-config: Different boards could have a different order for
+			connecting wires on touchscreen. We need to provide an
+			8 bit number where in the 1st four bits represent the
+			analog lines and the next 4 bits represent positive/
+			negative terminal on that input line. Notations to
+			represent the input lines and terminals resoectively
+			is as follows:
+			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
+			XP  = 0, XN = 1, YP = 2, YN = 3.
+- child "adc"
+	ti,adc-channels: Number of analog inputs available for ADC
+
+Example:
+	tscadc: tscadc@44e0d000 {
+		compatible = "ti,ti-tscadc";
+		tsc {
+			ti,wires = <4>;
+			ti,x-plate-resistance = <200>;
+			ti,steps-to-configure = <5>;
+			ti,wire-config = <0x00 0x11 0x22 0x33>;
+		};
+
+		adc {
+			ti,adc-channels = <4>;
+		};
+	}
diff --git a/include/linux/input/ti_am335x_tsc.h b/include/linux/input/ti_am335x_tsc.h
deleted file mode 100644
index 6a66b4d..0000000
--- a/include/linux/input/ti_am335x_tsc.h
+++ /dev/null
@@ -1,35 +0,0 @@
-#ifndef __LINUX_TI_AM335X_TSC_H
-#define __LINUX_TI_AM335X_TSC_H
-
-/**
- * struct tsc_data	Touchscreen wire configuration
- * @wires:		Wires refer to application modes
- *			i.e. 4/5/8 wire touchscreen support
- *			on the platform.
- * @x_plate_resistance:	X plate resistance.
- * @steps_to_configure:	The sequencer supports a total of
- *			16 programmable steps.
- *			A step configured to read a single
- *			co-ordinate value, can be applied
- *			more number of times for better results.
- * @wire_config:	Different EVM's could have a different order
- *			for connecting wires on touchscreen.
- *			We need to provide an 8 bit number where in
- *			the 1st four bits represent the analog lines
- *			and the next 4 bits represent positive/
- *			negative terminal on that input line.
- *			Notations to represent the input lines and
- *			terminals resoectively is as follows:
- *			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
- *			XP  = 0, XN = 1, YP = 2, YN = 3.
- *
- */
-
-struct tsc_data {
-	int wires;
-	int x_plate_resistance;
-	int steps_to_configure;
-	int wire_config[10];
-};
-
-#endif
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 50a245f..c985262 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -128,11 +128,6 @@
 
 #define TSCADC_CELLS		2
 
-struct mfd_tscadc_board {
-	struct tsc_data *tsc_init;
-	struct adc_data *adc_init;
-};
-
 struct ti_tscadc_dev {
 	struct device *dev;
 	struct regmap *regmap_tscadc;
diff --git a/include/linux/platform_data/ti_am335x_adc.h b/include/linux/platform_data/ti_am335x_adc.h
deleted file mode 100644
index e41d583..0000000
--- a/include/linux/platform_data/ti_am335x_adc.h
+++ /dev/null
@@ -1,14 +0,0 @@
-#ifndef __LINUX_TI_AM335X_ADC_H
-#define __LINUX_TI_AM335X_ADC_H
-
-/**
- * struct adc_data	ADC Input information
- * @adc_channels:	Number of analog inputs
- *			available for ADC.
- */
-
-struct adc_data {
-	unsigned int adc_channels;
-};
-
-#endif
-- 
1.7.10.4

Re: [PATCH 14/19] Documentation/DT bindings: add info for TI TSC ADC

[Jonathan Cameron]

On 05/27/2013 08:12 PM, Sebastian Andrzej Siewior wrote:
> The platform data structs are killed here because there are no users in
> tree (nobody sets the structs up, the RO usage is beeing removed) and the
> documentation about the parameters is saved here.
The platform data removal has nothing to do with the rest of the patch.
Please review the whole series to cleanup the patch divisions.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   38 ++++++++++++++++++++
>  include/linux/input/ti_am335x_tsc.h                |   35 ------------------
>  include/linux/mfd/ti_am335x_tscadc.h               |    5 ---
>  include/linux/platform_data/ti_am335x_adc.h        |   14 --------
>  4 files changed, 38 insertions(+), 54 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>  delete mode 100644 include/linux/input/ti_am335x_tsc.h
>  delete mode 100644 include/linux/platform_data/ti_am335x_adc.h
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> new file mode 100644
> index 0000000..e533e9d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -0,0 +1,38 @@
> +* TI - TSC ADC (Touschscreen and analog digital converter)
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Required properties:
> +- child "tsc"
> +	ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
> +		  support on the platform.
> +	ti,x-plate-resistance: X plate resistance
> +	ti,steps-to-configure: The sequencer supports a total of 16
> +			       programmable steps. A step configured to read a
> +			       single co-ordinate value. Can be applied more
> +			       number of times for better results.
> +	ti,wire-config: Different boards could have a different order for
> +			connecting wires on touchscreen. We need to provide an
> +			8 bit number where in the 1st four bits represent the
> +			analog lines and the next 4 bits represent positive/
> +			negative terminal on that input line. Notations to
> +			represent the input lines and terminals resoectively
> +			is as follows:
> +			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
> +			XP  = 0, XN = 1, YP = 2, YN = 3.
> +- child "adc"
> +	ti,adc-channels: Number of analog inputs available for ADC
> +
> +Example:
> +	tscadc: tscadc@44e0d000 {
> +		compatible = "ti,ti-tscadc";
> +		tsc {
> +			ti,wires = <4>;
> +			ti,x-plate-resistance = <200>;
> +			ti,steps-to-configure = <5>;
> +			ti,wire-config = <0x00 0x11 0x22 0x33>;
> +		};
> +
> +		adc {
> +			ti,adc-channels = <4>;
> +		};
> +	}
> diff --git a/include/linux/input/ti_am335x_tsc.h b/include/linux/input/ti_am335x_tsc.h
> deleted file mode 100644
> index 6a66b4d..0000000
> --- a/include/linux/input/ti_am335x_tsc.h
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -#ifndef __LINUX_TI_AM335X_TSC_H
> -#define __LINUX_TI_AM335X_TSC_H
> -
> -/**
> - * struct tsc_data	Touchscreen wire configuration
> - * @wires:		Wires refer to application modes
> - *			i.e. 4/5/8 wire touchscreen support
> - *			on the platform.
> - * @x_plate_resistance:	X plate resistance.
> - * @steps_to_configure:	The sequencer supports a total of
> - *			16 programmable steps.
> - *			A step configured to read a single
> - *			co-ordinate value, can be applied
> - *			more number of times for better results.
> - * @wire_config:	Different EVM's could have a different order
> - *			for connecting wires on touchscreen.
> - *			We need to provide an 8 bit number where in
> - *			the 1st four bits represent the analog lines
> - *			and the next 4 bits represent positive/
> - *			negative terminal on that input line.
> - *			Notations to represent the input lines and
> - *			terminals resoectively is as follows:
> - *			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
> - *			XP  = 0, XN = 1, YP = 2, YN = 3.
> - *
> - */
> -
> -struct tsc_data {
> -	int wires;
> -	int x_plate_resistance;
> -	int steps_to_configure;
> -	int wire_config[10];
> -};
> -
> -#endif
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index 50a245f..c985262 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -128,11 +128,6 @@
>  
>  #define TSCADC_CELLS		2
>  
> -struct mfd_tscadc_board {
> -	struct tsc_data *tsc_init;
> -	struct adc_data *adc_init;
> -};
> -
>  struct ti_tscadc_dev {
>  	struct device *dev;
>  	struct regmap *regmap_tscadc;
> diff --git a/include/linux/platform_data/ti_am335x_adc.h b/include/linux/platform_data/ti_am335x_adc.h
> deleted file mode 100644
> index e41d583..0000000
> --- a/include/linux/platform_data/ti_am335x_adc.h
> +++ /dev/null
> @@ -1,14 +0,0 @@
> -#ifndef __LINUX_TI_AM335X_ADC_H
> -#define __LINUX_TI_AM335X_ADC_H
> -
> -/**
> - * struct adc_data	ADC Input information
> - * @adc_channels:	Number of analog inputs
> - *			available for ADC.
> - */
> -
> -struct adc_data {
> -	unsigned int adc_channels;
> -};
> -
> -#endif
> 

Re: [PATCH 14/19] Documentation/DT bindings: add info for TI TSC ADC

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 18:48:24 [+0100]:

>On 05/27/2013 08:12 PM, Sebastian Andrzej Siewior wrote:
>> The platform data structs are killed here because there are no users in
>> tree (nobody sets the structs up, the RO usage is beeing removed) and the
>> documentation about the parameters is saved here.
>The platform data removal has nothing to do with the rest of the patch.
>Please review the whole series to cleanup the patch divisions.

Well, I removed the platform data and moved the documentation .dts but
okay, I will add this along with the DT part.

Sebastian

[PATCH 15/19] input/ti_am335x_tsc: tiny cleanup

[Sebastian Andrzej Siewior]

Things that can be done to done without using make up:
- reduce config_inp from 20 elements to 4
  The loop goes 0…3 so elements 4…19 remain unused.
- put the shift for analog_line into one line, since config_inp is u32 we
  don't need to worry about sign extension,
- check if he DT values are 0…3 as expected
- replace "err = -EINVAL; goto err" with "return -EINVAL;" as there is no
  cleanup and this is less code
- pull out "config[analog_line[i]][0…3];" from the switch case and use
  magic to the 0…3 correct.
- since we removed so much lines, spent a few to get
  "config[an_line][wi_order];" done.
- get rid of "val32, wires_conf" and assign the values directly. This is
  just init code but we can still save a few cycles.
- remove titsc_config_wires() from resume path. ->bit_yn & friends are only
  written once and not changed so as long as we assume that our DDR will
  have no biflips we can skip that.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |  124 ++++++++++++-----------------
 1 file changed, 51 insertions(+), 73 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 0dbf3df..96accaa 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -59,7 +59,7 @@ struct titsc {
 	unsigned int		bckup_y;
 	bool			pen_down;
 	int			steps_to_configure;
-	int			config_inp[20];
+	u32			config_inp[4];
 	int			bit_xp, bit_xn, bit_yp, bit_yn;
 	int			inp_xp, inp_xn, inp_yp, inp_yn;
 };
@@ -115,69 +115,54 @@ static int regbit_map(int val)
 
 static int titsc_config_wires(struct titsc *ts_dev)
 {
-	int		analog_line[10], wire_order[10];
-	int		i, temp_bits, err;
+	u32 analog_line[4];
+	u32 wire_order[4];
+	int i, temp_bits;
 
 	for (i = 0; i < 4; i++) {
 		/*
 		 * Get the order in which TSC wires are attached
 		 * w.r.t. each of the analog input lines on the EVM.
 		 */
-		analog_line[i] = ts_dev->config_inp[i] & 0xF0;
-		analog_line[i] = analog_line[i] >> 4;
-
+		analog_line[i] = (ts_dev->config_inp[i] & 0xF0) >> 4;
 		wire_order[i] = ts_dev->config_inp[i] & 0x0F;
+		if (WARN_ON(analog_line[i] > 4))
+				return -EINVAL;
+		if (WARN_ON(wire_order[i] > 4))
+				return -EINVAL;
 	}
 
 	for (i = 0; i < 4; i++) {
+		int an_line;
+		int wi_order;
+
+		an_line = analog_line[i];
+		wi_order = wire_order[i];
+		temp_bits = config[an_line][wi_order];
+		if (temp_bits == 0)
+			return -EINVAL;
 		switch (wire_order[i]) {
 		case 0:
-			temp_bits = config[analog_line[i]][0];
-			if (temp_bits == 0) {
-				err = -EINVAL;
-				goto ret;
-			} else {
-				ts_dev->bit_xp = regbit_map(temp_bits);
-				ts_dev->inp_xp = analog_line[i];
-				break;
-			}
+			ts_dev->bit_xp = regbit_map(temp_bits);
+			ts_dev->inp_xp = analog_line[i];
+			break;
+
 		case 1:
-			temp_bits = config[analog_line[i]][1];
-			if (temp_bits == 0) {
-				err = -EINVAL;
-				goto ret;
-			} else {
-				ts_dev->bit_xn = regbit_map(temp_bits);
-				ts_dev->inp_xn = analog_line[i];
-				break;
-			}
+			ts_dev->bit_xn = regbit_map(temp_bits);
+			ts_dev->inp_xn = analog_line[i];
+			break;
+
 		case 2:
-			temp_bits = config[analog_line[i]][2];
-			if (temp_bits == 0) {
-				err = -EINVAL;
-				goto ret;
-			} else {
-				ts_dev->bit_yp = regbit_map(temp_bits);
-				ts_dev->inp_yp = analog_line[i];
-				break;
-			}
+			ts_dev->bit_yp = regbit_map(temp_bits);
+			ts_dev->inp_yp = analog_line[i];
+			break;
 		case 3:
-			temp_bits = config[analog_line[i]][3];
-			if (temp_bits == 0) {
-				err = -EINVAL;
-				goto ret;
-			} else {
-				ts_dev->bit_yn = regbit_map(temp_bits);
-				ts_dev->inp_yn = analog_line[i];
-				break;
-			}
+			ts_dev->bit_yn = regbit_map(temp_bits);
+			ts_dev->inp_yn = analog_line[i];
+			break;
 		}
 	}
-
 	return 0;
-
-ret:
-	return err;
 }
 
 static void titsc_step_config(struct titsc *ts_dev)
@@ -319,7 +304,6 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int z1, z2, z;
 	unsigned int fsm;
 	unsigned int diffx = 0, diffy = 0;
-	int i;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
 	if (status & IRQENB_FIFO0THRES) {
@@ -387,11 +371,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 }
 
 static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
-					struct titsc *ts_dev)
+		struct titsc *ts_dev)
 {
 	struct device_node *node = tscadc_dev->dev->of_node;
-	int err, i;
-	u32 val32, wires_conf[4];
+	int err;
 
 	if (!node)
 		return -EINVAL;
@@ -399,34 +382,30 @@ static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
 	node = of_get_child_by_name(node, "tsc");
 	if (!node)
 		return -EINVAL;
-	err = of_property_read_u32(node, "ti,wires", &val32);
+	err = of_property_read_u32(node, "ti,wires", &ts_dev->wires);
 	if (err < 0)
-		goto error_ret;
-	ts_dev->wires = val32;
-
-	err = of_property_read_u32(node,
-			"ti,x-plate-resistance", &val32);
-	if (err < 0)
-		goto error_ret;
-	ts_dev->x_plate_resistance = val32;
+		return err;
+	switch (ts_dev->wires) {
+	case 4:
+	case 5:
+	case 8:
+		break;
+	default:
+		return -EINVAL;
+	}
 
-	err = of_property_read_u32(node,
-			"ti,steps-to-configure", &val32);
+	err = of_property_read_u32(node, "ti,x-plate-resistance",
+			&ts_dev->x_plate_resistance);
 	if (err < 0)
-		goto error_ret;
-	ts_dev->steps_to_configure = val32;
+		return err;
 
-	err = of_property_read_u32_array(node, "ti,wire-config",
-			wires_conf, ARRAY_SIZE(wires_conf));
+	err = of_property_read_u32(node, "ti,steps-to-configure",
+			&ts_dev->steps_to_configure);
 	if (err < 0)
-		goto error_ret;
+		return err;
 
-	for (i = 0; i < ARRAY_SIZE(wires_conf); i++)
-		ts_dev->config_inp[i] = wires_conf[i];
-	return 0;
-
-error_ret:
-	return err;
+	return of_property_read_u32_array(node, "ti,wire-config",
+			ts_dev->config_inp, ARRAY_SIZE(ts_dev->config_inp));
 }
 
 /*
@@ -545,7 +524,6 @@ static int titsc_resume(struct device *dev)
 				0x00);
 		titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
 	}
-	titsc_config_wires(ts_dev);
 	titsc_step_config(ts_dev);
 	titsc_writel(ts_dev, REG_FIFO0THR,
 			ts_dev->steps_to_configure);
-- 
1.7.10.4

Re: [PATCH 15/19] input/ti_am335x_tsc: tiny cleanup

[Jonathan Cameron]

On 05/27/2013 08:12 PM, Sebastian Andrzej Siewior wrote:
> Things that can be done to done without using make up:
That causes some amusing images of code with lipstick ;)  'wake up' perhaps?
> - reduce config_inp from 20 elements to 4
>   The loop goes 0…3 so elements 4…19 remain unused.
> - put the shift for analog_line into one line, since config_inp is u32 we
>   don't need to worry about sign extension,
> - check if he DT values are 0…3 as expected
> - replace "err = -EINVAL; goto err" with "return -EINVAL;" as there is no
>   cleanup and this is less code
> - pull out "config[analog_line[i]][0…3];" from the switch case and use
>   magic to the 0…3 correct.
> - since we removed so much lines, spent a few to get
>   "config[an_line][wi_order];" done.
> - get rid of "val32, wires_conf" and assign the values directly. This is
>   just init code but we can still save a few cycles.
> - remove titsc_config_wires() from resume path. ->bit_yn & friends are only
>   written once and not changed so as long as we assume that our DDR will
>   have no biflips we can skip that.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |  124 ++++++++++++-----------------
>  1 file changed, 51 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 0dbf3df..96accaa 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -59,7 +59,7 @@ struct titsc {
>  	unsigned int		bckup_y;
>  	bool			pen_down;
>  	int			steps_to_configure;
> -	int			config_inp[20];
> +	u32			config_inp[4];
>  	int			bit_xp, bit_xn, bit_yp, bit_yn;
>  	int			inp_xp, inp_xn, inp_yp, inp_yn;
>  };
> @@ -115,69 +115,54 @@ static int regbit_map(int val)
>  
>  static int titsc_config_wires(struct titsc *ts_dev)
>  {
> -	int		analog_line[10], wire_order[10];
> -	int		i, temp_bits, err;
> +	u32 analog_line[4];
> +	u32 wire_order[4];
> +	int i, temp_bits;
>  
>  	for (i = 0; i < 4; i++) {
>  		/*
>  		 * Get the order in which TSC wires are attached
>  		 * w.r.t. each of the analog input lines on the EVM.
>  		 */
> -		analog_line[i] = ts_dev->config_inp[i] & 0xF0;
> -		analog_line[i] = analog_line[i] >> 4;
> -
> +		analog_line[i] = (ts_dev->config_inp[i] & 0xF0) >> 4;
>  		wire_order[i] = ts_dev->config_inp[i] & 0x0F;
> +		if (WARN_ON(analog_line[i] > 4))
> +				return -EINVAL;
> +		if (WARN_ON(wire_order[i] > 4))
> +				return -EINVAL;
>  	}
>  
>  	for (i = 0; i < 4; i++) {
> +		int an_line;
> +		int wi_order;
> +
> +		an_line = analog_line[i];
> +		wi_order = wire_order[i];
> +		temp_bits = config[an_line][wi_order];
> +		if (temp_bits == 0)
> +			return -EINVAL;
>  		switch (wire_order[i]) {
>  		case 0:
> -			temp_bits = config[analog_line[i]][0];
> -			if (temp_bits == 0) {
> -				err = -EINVAL;
> -				goto ret;
> -			} else {
> -				ts_dev->bit_xp = regbit_map(temp_bits);
> -				ts_dev->inp_xp = analog_line[i];
> -				break;
> -			}
> +			ts_dev->bit_xp = regbit_map(temp_bits);
> +			ts_dev->inp_xp = analog_line[i];
> +			break;
> +
>  		case 1:
> -			temp_bits = config[analog_line[i]][1];
> -			if (temp_bits == 0) {
> -				err = -EINVAL;
> -				goto ret;
> -			} else {
> -				ts_dev->bit_xn = regbit_map(temp_bits);
> -				ts_dev->inp_xn = analog_line[i];
> -				break;
> -			}
> +			ts_dev->bit_xn = regbit_map(temp_bits);
> +			ts_dev->inp_xn = analog_line[i];
> +			break;
> +
>  		case 2:
> -			temp_bits = config[analog_line[i]][2];
> -			if (temp_bits == 0) {
> -				err = -EINVAL;
> -				goto ret;
> -			} else {
> -				ts_dev->bit_yp = regbit_map(temp_bits);
> -				ts_dev->inp_yp = analog_line[i];
> -				break;
> -			}
> +			ts_dev->bit_yp = regbit_map(temp_bits);
> +			ts_dev->inp_yp = analog_line[i];
> +			break;
>  		case 3:
> -			temp_bits = config[analog_line[i]][3];
> -			if (temp_bits == 0) {
> -				err = -EINVAL;
> -				goto ret;
> -			} else {
> -				ts_dev->bit_yn = regbit_map(temp_bits);
> -				ts_dev->inp_yn = analog_line[i];
> -				break;
> -			}
> +			ts_dev->bit_yn = regbit_map(temp_bits);
> +			ts_dev->inp_yn = analog_line[i];
> +			break;
>  		}
>  	}
> -
>  	return 0;
> -
> -ret:
> -	return err;
>  }
>  
>  static void titsc_step_config(struct titsc *ts_dev)
> @@ -319,7 +304,6 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  	unsigned int z1, z2, z;
>  	unsigned int fsm;
>  	unsigned int diffx = 0, diffy = 0;
> -	int i;
>  
>  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
>  	if (status & IRQENB_FIFO0THRES) {
> @@ -387,11 +371,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  }
>  
>  static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
> -					struct titsc *ts_dev)
> +		struct titsc *ts_dev)
>  {
>  	struct device_node *node = tscadc_dev->dev->of_node;
> -	int err, i;
> -	u32 val32, wires_conf[4];
> +	int err;
>  
>  	if (!node)
>  		return -EINVAL;
> @@ -399,34 +382,30 @@ static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
>  	node = of_get_child_by_name(node, "tsc");
>  	if (!node)
>  		return -EINVAL;
> -	err = of_property_read_u32(node, "ti,wires", &val32);
> +	err = of_property_read_u32(node, "ti,wires", &ts_dev->wires);
>  	if (err < 0)
> -		goto error_ret;
> -	ts_dev->wires = val32;
> -
> -	err = of_property_read_u32(node,
> -			"ti,x-plate-resistance", &val32);
> -	if (err < 0)
> -		goto error_ret;
> -	ts_dev->x_plate_resistance = val32;
> +		return err;
> +	switch (ts_dev->wires) {
> +	case 4:
> +	case 5:
> +	case 8:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
> -	err = of_property_read_u32(node,
> -			"ti,steps-to-configure", &val32);
> +	err = of_property_read_u32(node, "ti,x-plate-resistance",
> +			&ts_dev->x_plate_resistance);
>  	if (err < 0)
> -		goto error_ret;
> -	ts_dev->steps_to_configure = val32;
> +		return err;
>  
> -	err = of_property_read_u32_array(node, "ti,wire-config",
> -			wires_conf, ARRAY_SIZE(wires_conf));
> +	err = of_property_read_u32(node, "ti,steps-to-configure",
> +			&ts_dev->steps_to_configure);
>  	if (err < 0)
> -		goto error_ret;
> +		return err;
>  
> -	for (i = 0; i < ARRAY_SIZE(wires_conf); i++)
> -		ts_dev->config_inp[i] = wires_conf[i];
> -	return 0;
> -
> -error_ret:
> -	return err;
> +	return of_property_read_u32_array(node, "ti,wire-config",
> +			ts_dev->config_inp, ARRAY_SIZE(ts_dev->config_inp));
>  }
>  
>  /*
> @@ -545,7 +524,6 @@ static int titsc_resume(struct device *dev)
>  				0x00);
>  		titsc_writel(ts_dev, REG_IRQCLR, IRQENB_HW_PEN);
>  	}
> -	titsc_config_wires(ts_dev);
>  	titsc_step_config(ts_dev);
>  	titsc_writel(ts_dev, REG_FIFO0THR,
>  			ts_dev->steps_to_configure);
> 

Re: [PATCH 15/19] input/ti_am335x_tsc: tiny cleanup

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 18:49:20 [+0100]:

>On 05/27/2013 08:12 PM, Sebastian Andrzej Siewior wrote:
>> Things that can be done to done without using make up:
>That causes some amusing images of code with lipstick ;)  'wake up' perhaps?

Well, make up is usually used to cover up for the bad looks. In that
case, the code passed checkpatch but looks ugly as hell so it had to use
some kind of make to get through the review process :) This patch tries
to clean that code so the make up is not required any more.

Sebastian

Re: [PATCH 15/19] input/ti_am335x_tsc: tiny cleanup

[Dmitry Torokhov]

On Mon, May 27, 2013 at 09:12:02PM +0200, Sebastian Andrzej Siewior wrote:
> Things that can be done to done without using make up:
> - reduce config_inp from 20 elements to 4
>   The loop goes 0…3 so elements 4…19 remain unused.
> - put the shift for analog_line into one line, since config_inp is u32 we
>   don't need to worry about sign extension,
> - check if he DT values are 0…3 as expected
> - replace "err = -EINVAL; goto err" with "return -EINVAL;" as there is no
>   cleanup and this is less code
> - pull out "config[analog_line[i]][0…3];" from the switch case and use
>   magic to the 0…3 correct.
> - since we removed so much lines, spent a few to get
>   "config[an_line][wi_order];" done.
> - get rid of "val32, wires_conf" and assign the values directly. This is
>   just init code but we can still save a few cycles.
> - remove titsc_config_wires() from resume path. ->bit_yn & friends are only
>   written once and not changed so as long as we assume that our DDR will
>   have no biflips we can skip that.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> +		analog_line[i] = (ts_dev->config_inp[i] & 0xF0) >> 4;
>  		wire_order[i] = ts_dev->config_inp[i] & 0x0F;
> +		if (WARN_ON(analog_line[i] > 4))
> +				return -EINVAL;
> +		if (WARN_ON(wire_order[i] > 4))
> +				return -EINVAL;

Formatting is still a bit off here...

Thanks,

-- 
Dmitry

[PATCH 16/19] mfd / input: ti_am335x_tsc: rename device from tsc to TI-tsc

[Sebastian Andrzej Siewior]

tsc is a very generic name. This patch adds a TI prefix to make it a
little less generic.
While here add an alias so the driver is loaded on demand.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |    3 ++-
 drivers/mfd/ti_am335x_tscadc.c            |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 96accaa..2921163 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -543,11 +543,12 @@ static struct platform_driver ti_tsc_driver = {
 	.probe	= titsc_probe,
 	.remove	= titsc_remove,
 	.driver	= {
-		.name   = "tsc",
+		.name   = "TI-tsc",
 		.owner	= THIS_MODULE,
 		.pm	= TITSC_PM_OPS,
 	},
 };
+MODULE_ALIAS("platform:TI-tsc");
 module_platform_driver(ti_tsc_driver);
 
 MODULE_DESCRIPTION("TI touchscreen controller driver");
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index a27401a..c25fe00 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -184,7 +184,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	if (tsc_wires > 0) {
 		tscadc->tsc_cell = tscadc->used_cells;
 		cell = &tscadc->cells[tscadc->used_cells++];
-		cell->name = "tsc";
+		cell->name = "TI-tsc";
 		cell->platform_data = tscadc;
 		cell->pdata_size = sizeof(*tscadc);
 	}
-- 
1.7.10.4

Re: [PATCH 16/19] mfd / input: ti_am335x_tsc: rename device from tsc to TI-tsc

[Dmitry Torokhov]

On Mon, May 27, 2013 at 09:12:03PM +0200, Sebastian Andrzej Siewior wrote:
> tsc is a very generic name. This patch adds a TI prefix to make it a
> little less generic.
> While here add an alias so the driver is loaded on demand.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |    3 ++-
>  drivers/mfd/ti_am335x_tscadc.c            |    2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 96accaa..2921163 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -543,11 +543,12 @@ static struct platform_driver ti_tsc_driver = {
>  	.probe	= titsc_probe,
>  	.remove	= titsc_remove,
>  	.driver	= {
> -		.name   = "tsc",
> +		.name   = "TI-tsc",

Still quite generic... Why not call it "TI-am335x-tsc"?

Thanks.

-- 
Dmitry

Re: [PATCH 16/19] mfd / input: ti_am335x_tsc: rename device from tsc to TI-tsc

[Sebastian Andrzej Siewior]

* Dmitry Torokhov | 2013-06-04 09:50:35 [-0700]:

>> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
>> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>> @@ -543,11 +543,12 @@ static struct platform_driver ti_tsc_driver = {
>>  	.probe	= titsc_probe,
>>  	.remove	= titsc_remove,
>>  	.driver	= {
>> -		.name   = "tsc",
>> +		.name   = "TI-tsc",
>
>Still quite generic... Why not call it "TI-am335x-tsc"?
TI-am335x-tsc it is. In that case I make a similar change to adc node.

>Thanks.

Sebastian

[PATCH 17/19] mfd / iio: ti_am335x_adc: rename device from tiadc to TI-adc

[Sebastian Andrzej Siewior]

TI-adc reads a little better compared to tiadc.
While here add an alias so the driver is loaded on demand.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c |    4 ++--
 drivers/mfd/ti_am335x_tscadc.c  |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 72ffe89..d0317fc 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -299,14 +299,14 @@ static const struct dev_pm_ops tiadc_pm_ops = {
 
 static struct platform_driver tiadc_driver = {
 	.driver = {
-		.name   = "tiadc",
+		.name   = "TI-adc",
 		.owner	= THIS_MODULE,
 		.pm	= TIADC_PM_OPS,
 	},
 	.probe	= tiadc_probe,
 	.remove	= tiadc_remove,
 };
-
+MODULE_ALIAS("platform:TI-adc");
 module_platform_driver(tiadc_driver);
 
 MODULE_DESCRIPTION("TI ADC controller driver");
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index c25fe00..014df14 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -193,7 +193,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	if (adc_channels > 0) {
 		tscadc->adc_cell = tscadc->used_cells;
 		cell = &tscadc->cells[tscadc->used_cells++];
-		cell->name = "tiadc";
+		cell->name = "TI-adc";
 		cell->platform_data = tscadc;
 		cell->pdata_size = sizeof(*tscadc);
 	}
-- 
1.7.10.4

Re: [PATCH 17/19] mfd / iio: ti_am335x_adc: rename device from tiadc to TI-adc

[Jonathan Cameron]

On 05/27/2013 08:12 PM, Sebastian Andrzej Siewior wrote:
> TI-adc reads a little better compared to tiadc.
> While here add an alias so the driver is loaded on demand.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>

but you'll be wanting a few more acks on a name change...
> ---
>  drivers/iio/adc/ti_am335x_adc.c |    4 ++--
>  drivers/mfd/ti_am335x_tscadc.c  |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 72ffe89..d0317fc 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -299,14 +299,14 @@ static const struct dev_pm_ops tiadc_pm_ops = {
>  
>  static struct platform_driver tiadc_driver = {
>  	.driver = {
> -		.name   = "tiadc",
> +		.name   = "TI-adc",
>  		.owner	= THIS_MODULE,
>  		.pm	= TIADC_PM_OPS,
>  	},
>  	.probe	= tiadc_probe,
>  	.remove	= tiadc_remove,
>  };
> -
> +MODULE_ALIAS("platform:TI-adc");
>  module_platform_driver(tiadc_driver);
>  
>  MODULE_DESCRIPTION("TI ADC controller driver");
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index c25fe00..014df14 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -193,7 +193,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	if (adc_channels > 0) {
>  		tscadc->adc_cell = tscadc->used_cells;
>  		cell = &tscadc->cells[tscadc->used_cells++];
> -		cell->name = "tiadc";
> +		cell->name = "TI-adc";
>  		cell->platform_data = tscadc;
>  		cell->pdata_size = sizeof(*tscadc);
>  	}
> 

Re: [PATCH 17/19] mfd / iio: ti_am335x_adc: rename device from tiadc to TI-adc

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 18:50:19 [+0100]:

>On 05/27/2013 08:12 PM, Sebastian Andrzej Siewior wrote:
>> TI-adc reads a little better compared to tiadc.
>> While here add an alias so the driver is loaded on demand.
>> 
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>Acked-by: Jonathan Cameron <jic23@kernel.org>
>
>but you'll be wanting a few more acks on a name change...

It is a name change of a driver in tree (two to be exact) which can not
be used as of now and has no users. I can't imagine why someone should
complain.

Thanks for the review.

Sebatian

[PATCH 18/19] mfd/ti_am335x_tscadc: add a module alias for modprobe

[Sebastian Andrzej Siewior]

This ensures that the module is loaded on demand once hardware is found.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mfd/ti_am335x_tscadc.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 014df14..5e4076f 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -272,6 +272,7 @@ static const struct of_device_id ti_tscadc_dt_ids[] = {
 	{ .compatible = "ti,ti-tscadc", },
 	{ }
 };
+MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);
 
 static struct platform_driver ti_tscadc_driver = {
 	.driver = {
-- 
1.7.10.4

[PATCH 19/19] mfd/ti_am335x_tscadc: add private lock/unlock function for regmap read/write

[Sebastian Andrzej Siewior]

Without this, devm_regmap_init_mmio() creates & uses a simple
spin_lock() and this should be enough. Within the probe function the
registers are read and written in process context. Later they are
accessed from the ISR and lockdep complains because now the lock is
taken suddenly with IRQs enabled. Currently I don't see any other way to
keep lockdep quiet than doing this.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mfd/ti_am335x_tscadc.c       |   17 +++++++++++++++++
 include/linux/mfd/ti_am335x_tscadc.h |    2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 5e4076f..04ed630 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -42,11 +42,27 @@ static void tscadc_writel(struct ti_tscadc_dev *tsadc, unsigned int reg,
 	regmap_write(tsadc->regmap_tscadc, reg, val);
 }
 
+static void tscadc_lock_spinlock(void *__map)
+{
+	struct ti_tscadc_dev *tsadc = __map;
+
+	spin_lock_irqsave(&tsadc->reg_map_lock, tsadc->reg_map_flags);
+}
+
+static void tscadc_unlock_spinlock(void *__map)
+{
+	struct ti_tscadc_dev *tsadc = __map;
+
+	spin_unlock_irqrestore(&tsadc->reg_map_lock, tsadc->reg_map_flags);
+}
+
 static const struct regmap_config tscadc_regmap_config = {
 	.name = "ti_tscadc",
 	.reg_bits = 32,
 	.reg_stride = 4,
 	.val_bits = 32,
+	.lock = tscadc_lock_spinlock,
+	.unlock = tscadc_unlock_spinlock,
 };
 
 static void tscadc_idle_config(struct ti_tscadc_dev *config)
@@ -110,6 +126,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	} else
 		tscadc->irq = err;
 
+	spin_lock_init(&tscadc->reg_map_lock);
 	res = devm_request_mem_region(&pdev->dev,
 			res->start, resource_size(res), pdev->name);
 	if (!res) {
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index c985262..284e482 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -129,6 +129,8 @@
 #define TSCADC_CELLS		2
 
 struct ti_tscadc_dev {
+	spinlock_t reg_map_lock;
+	unsigned long reg_map_flags;
 	struct device *dev;
 	struct regmap *regmap_tscadc;
 	void __iomem *tscadc_base;
-- 
1.7.10.4

Re: [PATCH 19/19 v2] mfd/ti_am335x_tscadc: add private lock/unlock function for regmap read/write

[Sebastian Andrzej Siewior]

Without this, devm_regmap_init_mmio() creates & uses a simple
spin_lock() and this should be enough. Within the probe function the
registers are read and written in process context. Later they are
accessed from the ISR and lockdep complains because now the lock is
taken suddenly with IRQs enabled. Currently I don't see any other way to
keep lockdep quiet than doing this.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

v1..v2: pass a ti_tscadc_dev via lock arg

 drivers/mfd/ti_am335x_tscadc.c       |   21 ++++++++++++++++++++-
 include/linux/mfd/ti_am335x_tscadc.h |    2 ++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 5e4076f..bd982e5 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -42,11 +42,27 @@ static void tscadc_writel(struct ti_tscadc_dev *tsadc, unsigned int reg,
 	regmap_write(tsadc->regmap_tscadc, reg, val);
 }
 
+static void tscadc_lock_spinlock(void *__map)
+{
+	struct ti_tscadc_dev *tsadc = __map;
+
+	spin_lock_irqsave(&tsadc->reg_map_lock, tsadc->reg_map_flags);
+}
+
+static void tscadc_unlock_spinlock(void *__map)
+{
+	struct ti_tscadc_dev *tsadc = __map;
+
+	spin_unlock_irqrestore(&tsadc->reg_map_lock, tsadc->reg_map_flags);
+}
+
 static const struct regmap_config tscadc_regmap_config = {
 	.name = "ti_tscadc",
 	.reg_bits = 32,
 	.reg_stride = 4,
 	.val_bits = 32,
+	.lock = tscadc_lock_spinlock,
+	.unlock = tscadc_unlock_spinlock,
 };
 
 static void tscadc_idle_config(struct ti_tscadc_dev *config)
@@ -69,6 +85,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	int			err, ctrl;
 	int			clk_value, clock_rate;
 	int			tsc_wires = 0, adc_channels = 0, total_channels;
+	struct regmap_config	tscadc_regmap_cfg = tscadc_regmap_config;
 
 	if (!pdev->dev.of_node) {
 		dev_err(&pdev->dev, "Could not find valid DT data.\n");
@@ -110,6 +127,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	} else
 		tscadc->irq = err;
 
+	spin_lock_init(&tscadc->reg_map_lock);
 	res = devm_request_mem_region(&pdev->dev,
 			res->start, resource_size(res), pdev->name);
 	if (!res) {
@@ -124,8 +142,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	tscadc_regmap_cfg.lock_arg = tscadc;
 	tscadc->regmap_tscadc = devm_regmap_init_mmio(&pdev->dev,
-			tscadc->tscadc_base, &tscadc_regmap_config);
+			tscadc->tscadc_base, &tscadc_regmap_cfg);
 	if (IS_ERR(tscadc->regmap_tscadc)) {
 		dev_err(&pdev->dev, "regmap init failed\n");
 		err = PTR_ERR(tscadc->regmap_tscadc);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index c985262..284e482 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -129,6 +129,8 @@
 #define TSCADC_CELLS		2
 
 struct ti_tscadc_dev {
+	spinlock_t reg_map_lock;
+	unsigned long reg_map_flags;
 	struct device *dev;
 	struct regmap *regmap_tscadc;
 	void __iomem *tscadc_base;
-- 
1.7.10.4

Re: [PATCH 19/19 v2] mfd/ti_am335x_tscadc: add private lock/unlock function for regmap read/write

[Mark Brown]

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

On Wed, May 29, 2013 at 10:46:42AM +0200, Sebastian Andrzej Siewior wrote:
> Without this, devm_regmap_init_mmio() creates & uses a simple
> spin_lock() and this should be enough. Within the probe function the
> registers are read and written in process context. Later they are
> accessed from the ISR and lockdep complains because now the lock is
> taken suddenly with IRQs enabled. Currently I don't see any other way to
> keep lockdep quiet than doing this.

This is not a good place to make this change, there's nothing specific
to the device here and in actual fact there's already a change in the
works from Lars-Peter Clausen converting the spinlock to always use
spin_lock_irqsave() so it should be resolved soon.  The thread was on
lkml in the past few days.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 19/19 v2] mfd/ti_am335x_tscadc: add private lock/unlock function for regmap read/write

[Lars-Peter Clausen]

On 05/29/2013 01:12 PM, Mark Brown wrote:
> On Wed, May 29, 2013 at 10:46:42AM +0200, Sebastian Andrzej Siewior wrote:
>> Without this, devm_regmap_init_mmio() creates & uses a simple
>> spin_lock() and this should be enough. Within the probe function the
>> registers are read and written in process context. Later they are
>> accessed from the ISR and lockdep complains because now the lock is
>> taken suddenly with IRQs enabled. Currently I don't see any other way to
>> keep lockdep quiet than doing this.
> 
> This is not a good place to make this change, there's nothing specific
> to the device here and in actual fact there's already a change in the
> works from Lars-Peter Clausen converting the spinlock to always use
> spin_lock_irqsave() so it should be resolved soon.  The thread was on
> lkml in the past few days.

Yes, this patch shouldn't be necessary in next/master anymore. For the
record the patch can be found here: https://patchwork.kernel.org/patch/2609721/

- Lars

Re: [PATCH 19/19 v2] mfd/ti_am335x_tscadc: add private lock/unlock function for regmap read/write

[Sebastian Andrzej Siewior]

On 05/29/2013 01:25 PM, Lars-Peter Clausen wrote:
>> This is not a good place to make this change, there's nothing specific
>> to the device here and in actual fact there's already a change in the
>> works from Lars-Peter Clausen converting the spinlock to always use
>> spin_lock_irqsave() so it should be resolved soon.  The thread was on
>> lkml in the past few days.
> 
> Yes, this patch shouldn't be necessary in next/master anymore. For the
> record the patch can be found here: https://patchwork.kernel.org/patch/2609721/

Ah good good good. So this one can dropped. Thank you.

> - Lars

Sebastian