[PATCH 0/4] [input] tsc2005 cleanup

[PATCH 0/4] [input] tsc2005 cleanup

[Sebastian Reichel]

Hi,

The following patches simplify and cleanup the tsc2005 touchscreen driver.
Apart from making the driver more readable, the direct usage of SPI has been
reduced to one function, so that tsc2004 support may be addable now (see [0]).

The driver could be further simplified by making the vio regulator also
mandatory for boardcode and switching boardcode from set_reset callback
to GPIOD API. Since that would result in a patch touching the ARM and
the input subsystem, I suggest to wait until the Nokia N900 boardcode
is gone.

[0] http://permalink.gmane.org/gmane.linux.kernel.input/37513

-- Sebastian

Sebastian Reichel (4):
  Input: tsc2005 - improve readability of register defines
  Input: tsc2005 - convert to regmap
  Input: tsc2005 - simplify drvdata acquisition
  Input: tsc2005 - convert to gpiod

 drivers/input/touchscreen/tsc2005.c | 235 ++++++++++++++----------------------
 1 file changed, 88 insertions(+), 147 deletions(-)

-- 
2.1.4

[PATCH 1/4] Input: tsc2005 - improve readability of register defines

[Sebastian Reichel]

Improve defines for first control byte by removing 0x00
prefix (the defines are for 8 bit values and not for 16
bit values) and expose register structure by exposing
the shift.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/input/touchscreen/tsc2005.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
index d8c025b..87038b2 100644
--- a/drivers/input/touchscreen/tsc2005.c
+++ b/drivers/input/touchscreen/tsc2005.c
@@ -61,16 +61,24 @@
 #define TSC2005_CMD_12BIT		0x04
 
 /* control byte 0 */
-#define TSC2005_REG_READ		0x0001
-#define TSC2005_REG_PND0		0x0002
-#define TSC2005_REG_X			0x0000
-#define TSC2005_REG_Y			0x0008
-#define TSC2005_REG_Z1			0x0010
-#define TSC2005_REG_Z2			0x0018
-#define TSC2005_REG_TEMP_HIGH		0x0050
-#define TSC2005_REG_CFR0		0x0060
-#define TSC2005_REG_CFR1		0x0068
-#define TSC2005_REG_CFR2		0x0070
+#define TSC2005_REG_READ		0x01 /* R/W access */
+#define TSC2005_REG_PND0		0x02 /* Power Not Down Control */
+#define TSC2005_REG_X			(0x0 << 3)
+#define TSC2005_REG_Y			(0x1 << 3)
+#define TSC2005_REG_Z1			(0x2 << 3)
+#define TSC2005_REG_Z2			(0x3 << 3)
+#define TSC2005_REG_AUX			(0x4 << 3)
+#define TSC2005_REG_TEMP1		(0x5 << 3)
+#define TSC2005_REG_TEMP2		(0x6 << 3)
+#define TSC2005_REG_STATUS		(0x7 << 3)
+#define TSC2005_REG_AUX_HIGH		(0x8 << 3)
+#define TSC2005_REG_AUX_LOW		(0x9 << 3)
+#define TSC2005_REG_TEMP_HIGH		(0xA << 3)
+#define TSC2005_REG_TEMP_LOW		(0xB << 3)
+#define TSC2005_REG_CFR0		(0xC << 3)
+#define TSC2005_REG_CFR1		(0xD << 3)
+#define TSC2005_REG_CFR2		(0xE << 3)
+#define TSC2005_REG_CONV_FUNC		(0xF << 3)
 
 /* configuration register 0 */
 #define TSC2005_CFR0_PRECHARGE_276US	0x0040
-- 
2.1.4

[PATCH 2/4] Input: tsc2005 - convert to regmap

[Sebastian Reichel]

Convert driver, so that it uses regmap instead of directly
using spi_transfer for all register accesses.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/input/touchscreen/tsc2005.c | 170 ++++++++++++------------------------
 1 file changed, 58 insertions(+), 112 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
index 87038b2..0aec45a 100644
--- a/drivers/input/touchscreen/tsc2005.c
+++ b/drivers/input/touchscreen/tsc2005.c
@@ -34,6 +34,7 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/tsc2005.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
 
 /*
  * The touchscreen interface operates as follows:
@@ -120,20 +121,36 @@
 #define TSC2005_SPI_MAX_SPEED_HZ	10000000
 #define TSC2005_PENUP_TIME_MS		40
 
-struct tsc2005_spi_rd {
-	struct spi_transfer	spi_xfer;
-	u32			spi_tx;
-	u32			spi_rx;
+static const struct regmap_range tsc2005_writable_ranges[] = {
+	regmap_reg_range(TSC2005_REG_AUX_HIGH, TSC2005_REG_CFR2),
 };
 
+static const struct regmap_access_table tsc2005_writable_table = {
+	.yes_ranges = tsc2005_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(tsc2005_writable_ranges),
+};
+
+static struct regmap_config tsc2005_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.reg_stride = 0x08,
+	.max_register = 0x78,
+	.read_flag_mask = TSC2005_REG_READ,
+	.wr_table = &tsc2005_writable_table,
+	.use_single_rw = true,
+};
+
+struct tsc2005_data {
+	u16 x;
+	u16 y;
+	u16 z1;
+	u16 z2;
+} __packed;
+#define TSC2005_DATA_REGS 4
+
 struct tsc2005 {
 	struct spi_device	*spi;
-
-	struct spi_message      spi_read_msg;
-	struct tsc2005_spi_rd	spi_x;
-	struct tsc2005_spi_rd	spi_y;
-	struct tsc2005_spi_rd	spi_z1;
-	struct tsc2005_spi_rd	spi_z2;
+	struct regmap		*regmap;
 
 	struct input_dev	*idev;
 	char			phys[32];
@@ -190,62 +207,6 @@ static int tsc2005_cmd(struct tsc2005 *ts, u8 cmd)
 	return 0;
 }
 
-static int tsc2005_write(struct tsc2005 *ts, u8 reg, u16 value)
-{
-	u32 tx = ((reg | TSC2005_REG_PND0) << 16) | value;
-	struct spi_transfer xfer = {
-		.tx_buf		= &tx,
-		.len		= 4,
-		.bits_per_word	= 24,
-	};
-	struct spi_message msg;
-	int error;
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-
-	error = spi_sync(ts->spi, &msg);
-	if (error) {
-		dev_err(&ts->spi->dev,
-			"%s: failed, register: %x, value: %x, error: %d\n",
-			__func__, reg, value, error);
-		return error;
-	}
-
-	return 0;
-}
-
-static void tsc2005_setup_read(struct tsc2005_spi_rd *rd, u8 reg, bool last)
-{
-	memset(rd, 0, sizeof(*rd));
-
-	rd->spi_tx		   = (reg | TSC2005_REG_READ) << 16;
-	rd->spi_xfer.tx_buf	   = &rd->spi_tx;
-	rd->spi_xfer.rx_buf	   = &rd->spi_rx;
-	rd->spi_xfer.len	   = 4;
-	rd->spi_xfer.bits_per_word = 24;
-	rd->spi_xfer.cs_change	   = !last;
-}
-
-static int tsc2005_read(struct tsc2005 *ts, u8 reg, u16 *value)
-{
-	struct tsc2005_spi_rd spi_rd;
-	struct spi_message msg;
-	int error;
-
-	tsc2005_setup_read(&spi_rd, reg, true);
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&spi_rd.spi_xfer, &msg);
-
-	error = spi_sync(ts->spi, &msg);
-	if (error)
-		return error;
-
-	*value = spi_rd.spi_rx;
-	return 0;
-}
-
 static void tsc2005_update_pen_state(struct tsc2005 *ts,
 				     int x, int y, int pressure)
 {
@@ -274,26 +235,23 @@ static irqreturn_t tsc2005_irq_thread(int irq, void *_ts)
 	struct tsc2005 *ts = _ts;
 	unsigned long flags;
 	unsigned int pressure;
-	u32 x, y;
-	u32 z1, z2;
+	struct tsc2005_data tsdata;
 	int error;
 
 	/* read the coordinates */
-	error = spi_sync(ts->spi, &ts->spi_read_msg);
+	error = regmap_bulk_read(ts->regmap, TSC2005_REG_X, &tsdata,
+				 TSC2005_DATA_REGS);
 	if (unlikely(error))
 		goto out;
 
-	x = ts->spi_x.spi_rx;
-	y = ts->spi_y.spi_rx;
-	z1 = ts->spi_z1.spi_rx;
-	z2 = ts->spi_z2.spi_rx;
-
 	/* validate position */
-	if (unlikely(x > MAX_12BIT || y > MAX_12BIT))
+	if (unlikely(tsdata.x > MAX_12BIT || tsdata.y > MAX_12BIT))
 		goto out;
 
 	/* Skip reading if the pressure components are out of range */
-	if (unlikely(z1 == 0 || z2 > MAX_12BIT || z1 >= z2))
+	if (unlikely(tsdata.z1 == 0 || tsdata.z2 > MAX_12BIT))
+		goto out;
+	if (unlikely(tsdata.z1 >= tsdata.z2))
 		goto out;
 
        /*
@@ -301,8 +259,8 @@ static irqreturn_t tsc2005_irq_thread(int irq, void *_ts)
 	* the value before pen-up - that implies SPI fed us stale data
 	*/
 	if (!ts->pen_down &&
-	    ts->in_x == x && ts->in_y == y &&
-	    ts->in_z1 == z1 && ts->in_z2 == z2) {
+	    ts->in_x == tsdata.x && ts->in_y == tsdata.y &&
+	    ts->in_z1 == tsdata.z1 && ts->in_z2 == tsdata.z2) {
 		goto out;
 	}
 
@@ -310,20 +268,20 @@ static irqreturn_t tsc2005_irq_thread(int irq, void *_ts)
 	 * At this point we are happy we have a valid and useful reading.
 	 * Remember it for later comparisons. We may now begin downsampling.
 	 */
-	ts->in_x = x;
-	ts->in_y = y;
-	ts->in_z1 = z1;
-	ts->in_z2 = z2;
+	ts->in_x = tsdata.x;
+	ts->in_y = tsdata.y;
+	ts->in_z1 = tsdata.z1;
+	ts->in_z2 = tsdata.z2;
 
 	/* Compute touch pressure resistance using equation #1 */
-	pressure = x * (z2 - z1) / z1;
+	pressure = tsdata.x * (tsdata.z2 - tsdata.z1) / tsdata.z1;
 	pressure = pressure * ts->x_plate_ohm / 4096;
 	if (unlikely(pressure > MAX_12BIT))
 		goto out;
 
 	spin_lock_irqsave(&ts->lock, flags);
 
-	tsc2005_update_pen_state(ts, x, y, pressure);
+	tsc2005_update_pen_state(ts, tsdata.x, tsdata.y, pressure);
 	mod_timer(&ts->penup_timer,
 		  jiffies + msecs_to_jiffies(TSC2005_PENUP_TIME_MS));
 
@@ -346,9 +304,9 @@ static void tsc2005_penup_timer(unsigned long data)
 
 static void tsc2005_start_scan(struct tsc2005 *ts)
 {
-	tsc2005_write(ts, TSC2005_REG_CFR0, TSC2005_CFR0_INITVALUE);
-	tsc2005_write(ts, TSC2005_REG_CFR1, TSC2005_CFR1_INITVALUE);
-	tsc2005_write(ts, TSC2005_REG_CFR2, TSC2005_CFR2_INITVALUE);
+	regmap_write(ts->regmap, TSC2005_REG_CFR0, TSC2005_CFR0_INITVALUE);
+	regmap_write(ts->regmap, TSC2005_REG_CFR1, TSC2005_CFR1_INITVALUE);
+	regmap_write(ts->regmap, TSC2005_REG_CFR2, TSC2005_CFR2_INITVALUE);
 	tsc2005_cmd(ts, TSC2005_CMD_NORMAL);
 }
 
@@ -398,9 +356,9 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 {
 	struct spi_device *spi = to_spi_device(dev);
 	struct tsc2005 *ts = spi_get_drvdata(spi);
-	u16 temp_high;
-	u16 temp_high_orig;
-	u16 temp_high_test;
+	unsigned int temp_high;
+	unsigned int temp_high_orig;
+	unsigned int temp_high_test;
 	bool success = true;
 	int error;
 
@@ -411,7 +369,7 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 	 */
 	__tsc2005_disable(ts);
 
-	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high_orig);
+	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high_orig);
 	if (error) {
 		dev_warn(dev, "selftest failed: read error %d\n", error);
 		success = false;
@@ -420,14 +378,14 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 
 	temp_high_test = (temp_high_orig - 1) & MAX_12BIT;
 
-	error = tsc2005_write(ts, TSC2005_REG_TEMP_HIGH, temp_high_test);
+	error = regmap_write(ts->regmap, TSC2005_REG_TEMP_HIGH, temp_high_test);
 	if (error) {
 		dev_warn(dev, "selftest failed: write error %d\n", error);
 		success = false;
 		goto out;
 	}
 
-	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high);
+	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high);
 	if (error) {
 		dev_warn(dev, "selftest failed: read error %d after write\n",
 			 error);
@@ -450,7 +408,7 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 		goto out;
 
 	/* test that the reset really happened */
-	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high);
+	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high);
 	if (error) {
 		dev_warn(dev, "selftest failed: read error %d after reset\n",
 			 error);
@@ -503,7 +461,7 @@ static void tsc2005_esd_work(struct work_struct *work)
 {
 	struct tsc2005 *ts = container_of(work, struct tsc2005, esd_work.work);
 	int error;
-	u16 r;
+	unsigned int r;
 
 	if (!mutex_trylock(&ts->mutex)) {
 		/*
@@ -519,7 +477,7 @@ static void tsc2005_esd_work(struct work_struct *work)
 		goto out;
 
 	/* We should be able to read register without disabling interrupts. */
-	error = tsc2005_read(ts, TSC2005_REG_CFR0, &r);
+	error = regmap_read(ts->regmap, TSC2005_REG_CFR0, &r);
 	if (!error &&
 	    !((r ^ TSC2005_CFR0_INITVALUE) & TSC2005_CFR0_RW_MASK)) {
 		goto out;
@@ -583,20 +541,6 @@ static void tsc2005_close(struct input_dev *input)
 	mutex_unlock(&ts->mutex);
 }
 
-static void tsc2005_setup_spi_xfer(struct tsc2005 *ts)
-{
-	tsc2005_setup_read(&ts->spi_x, TSC2005_REG_X, false);
-	tsc2005_setup_read(&ts->spi_y, TSC2005_REG_Y, false);
-	tsc2005_setup_read(&ts->spi_z1, TSC2005_REG_Z1, false);
-	tsc2005_setup_read(&ts->spi_z2, TSC2005_REG_Z2, true);
-
-	spi_message_init(&ts->spi_read_msg);
-	spi_message_add_tail(&ts->spi_x.spi_xfer, &ts->spi_read_msg);
-	spi_message_add_tail(&ts->spi_y.spi_xfer, &ts->spi_read_msg);
-	spi_message_add_tail(&ts->spi_z1.spi_xfer, &ts->spi_read_msg);
-	spi_message_add_tail(&ts->spi_z2.spi_xfer, &ts->spi_read_msg);
-}
-
 static int tsc2005_probe(struct spi_device *spi)
 {
 	const struct tsc2005_platform_data *pdata = dev_get_platdata(&spi->dev);
@@ -661,6 +605,10 @@ static int tsc2005_probe(struct spi_device *spi)
 	ts->spi = spi;
 	ts->idev = input_dev;
 
+	ts->regmap = devm_regmap_init_spi(spi, &tsc2005_regmap_config);
+	if (IS_ERR(ts->regmap))
+		return PTR_ERR(ts->regmap);
+
 	ts->x_plate_ohm = x_plate_ohm;
 	ts->esd_timeout = esd_timeout;
 
@@ -700,8 +648,6 @@ static int tsc2005_probe(struct spi_device *spi)
 
 	INIT_DELAYED_WORK(&ts->esd_work, tsc2005_esd_work);
 
-	tsc2005_setup_spi_xfer(ts);
-
 	snprintf(ts->phys, sizeof(ts->phys),
 		 "%s/input-ts", dev_name(&spi->dev));
 
-- 
2.1.4

Re: [PATCH 2/4] Input: tsc2005 - convert to regmap

[Dmitry Torokhov]

Hi Sebastian,

On Wed, Jul 15, 2015 at 02:13:26PM +0200, Sebastian Reichel wrote:

>  	/* read the coordinates */
> -	error = spi_sync(ts->spi, &ts->spi_read_msg);
> +	error = regmap_bulk_read(ts->regmap, TSC2005_REG_X, &tsdata,
> +				 TSC2005_DATA_REGS);
>  	if (unlikely(error))
>  		goto out;
>  
> -	x = ts->spi_x.spi_rx;
> -	y = ts->spi_y.spi_rx;
> -	z1 = ts->spi_z1.spi_rx;
> -	z2 = ts->spi_z2.spi_rx;
> -
>  	/* validate position */
> -	if (unlikely(x > MAX_12BIT || y > MAX_12BIT))
> +	if (unlikely(tsdata.x > MAX_12BIT || tsdata.y > MAX_12BIT))
>  		goto out;
>  
>  	/* Skip reading if the pressure components are out of range */
> -	if (unlikely(z1 == 0 || z2 > MAX_12BIT || z1 >= z2))
> +	if (unlikely(tsdata.z1 == 0 || tsdata.z2 > MAX_12BIT))
> +		goto out;
> +	if (unlikely(tsdata.z1 >= tsdata.z2))
>  		goto out;

I guess that means that tsc2005 is (and was) endian-broken. We can't use
the data off the wire directly...  So I'll drop bucnh of the changes in
this function so we can convert to CPU endianness before using.

I also got:

  CC [M]  drivers/input/touchscreen/tsc2005.o
  Building modules, stage 2.
Kernel: arch/x86/boot/bzImage is ready  (#1383)
  MODPOST 1403 modules
ERROR: "devm_regmap_init_spi" [drivers/input/touchscreen/tsc2005.ko]
undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

I guess we need "select REGMAP_SPI".

Thanks.

-- 
Dmitry


Input: tsc2005 - convert to regmap

From: Sebastian Reichel <sre@kernel.org>

Convert driver, so that it uses regmap instead of directly using
spi_transfer for all register accesses.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/Kconfig   |    9 +-
 drivers/input/touchscreen/tsc2005.c |  149 ++++++++++++-----------------------
 2 files changed, 55 insertions(+), 103 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 9b3da52..c1acbbc 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -915,10 +915,11 @@ config TOUCHSCREEN_TSC_SERIO
 	  module will be called tsc40.
 
 config TOUCHSCREEN_TSC2005
-        tristate "TSC2005 based touchscreens"
-        depends on SPI_MASTER
-        help
-          Say Y here if you have a TSC2005 based touchscreen.
+	tristate "TSC2005 based touchscreens"
+	depends on SPI_MASTER
+	select REGMAP_SPI
+	help
+	  Say Y here if you have a TSC2005 based touchscreen.
 
 	  If unsure, say N.
 
diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
index 2a27a1f..a04248e 100644
--- a/drivers/input/touchscreen/tsc2005.c
+++ b/drivers/input/touchscreen/tsc2005.c
@@ -34,6 +34,7 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/tsc2005.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
 
 /*
  * The touchscreen interface operates as follows:
@@ -120,20 +121,36 @@
 #define TSC2005_SPI_MAX_SPEED_HZ	10000000
 #define TSC2005_PENUP_TIME_MS		40
 
-struct tsc2005_spi_rd {
-	struct spi_transfer	spi_xfer;
-	u32			spi_tx;
-	u32			spi_rx;
+static const struct regmap_range tsc2005_writable_ranges[] = {
+	regmap_reg_range(TSC2005_REG_AUX_HIGH, TSC2005_REG_CFR2),
 };
 
+static const struct regmap_access_table tsc2005_writable_table = {
+	.yes_ranges = tsc2005_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(tsc2005_writable_ranges),
+};
+
+static struct regmap_config tsc2005_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.reg_stride = 0x08,
+	.max_register = 0x78,
+	.read_flag_mask = TSC2005_REG_READ,
+	.wr_table = &tsc2005_writable_table,
+	.use_single_rw = true,
+};
+
+struct tsc2005_data {
+	u16 x;
+	u16 y;
+	u16 z1;
+	u16 z2;
+} __packed;
+#define TSC2005_DATA_REGS 4
+
 struct tsc2005 {
 	struct spi_device	*spi;
-
-	struct spi_message      spi_read_msg;
-	struct tsc2005_spi_rd	spi_x;
-	struct tsc2005_spi_rd	spi_y;
-	struct tsc2005_spi_rd	spi_z1;
-	struct tsc2005_spi_rd	spi_z2;
+	struct regmap		*regmap;
 
 	struct input_dev	*idev;
 	char			phys[32];
@@ -190,62 +207,6 @@ static int tsc2005_cmd(struct tsc2005 *ts, u8 cmd)
 	return 0;
 }
 
-static int tsc2005_write(struct tsc2005 *ts, u8 reg, u16 value)
-{
-	u32 tx = ((reg | TSC2005_REG_PND0) << 16) | value;
-	struct spi_transfer xfer = {
-		.tx_buf		= &tx,
-		.len		= 4,
-		.bits_per_word	= 24,
-	};
-	struct spi_message msg;
-	int error;
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfer, &msg);
-
-	error = spi_sync(ts->spi, &msg);
-	if (error) {
-		dev_err(&ts->spi->dev,
-			"%s: failed, register: %x, value: %x, error: %d\n",
-			__func__, reg, value, error);
-		return error;
-	}
-
-	return 0;
-}
-
-static void tsc2005_setup_read(struct tsc2005_spi_rd *rd, u8 reg, bool last)
-{
-	memset(rd, 0, sizeof(*rd));
-
-	rd->spi_tx		   = (reg | TSC2005_REG_READ) << 16;
-	rd->spi_xfer.tx_buf	   = &rd->spi_tx;
-	rd->spi_xfer.rx_buf	   = &rd->spi_rx;
-	rd->spi_xfer.len	   = 4;
-	rd->spi_xfer.bits_per_word = 24;
-	rd->spi_xfer.cs_change	   = !last;
-}
-
-static int tsc2005_read(struct tsc2005 *ts, u8 reg, u16 *value)
-{
-	struct tsc2005_spi_rd spi_rd;
-	struct spi_message msg;
-	int error;
-
-	tsc2005_setup_read(&spi_rd, reg, true);
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&spi_rd.spi_xfer, &msg);
-
-	error = spi_sync(ts->spi, &msg);
-	if (error)
-		return error;
-
-	*value = spi_rd.spi_rx;
-	return 0;
-}
-
 static void tsc2005_update_pen_state(struct tsc2005 *ts,
 				     int x, int y, int pressure)
 {
@@ -276,17 +237,19 @@ static irqreturn_t tsc2005_irq_thread(int irq, void *_ts)
 	unsigned int pressure;
 	u32 x, y;
 	u32 z1, z2;
+	struct tsc2005_data tsdata;
 	int error;
 
 	/* read the coordinates */
-	error = spi_sync(ts->spi, &ts->spi_read_msg);
+	error = regmap_bulk_read(ts->regmap, TSC2005_REG_X, &tsdata,
+				 TSC2005_DATA_REGS);
 	if (unlikely(error))
 		goto out;
 
-	x = ts->spi_x.spi_rx;
-	y = ts->spi_y.spi_rx;
-	z1 = ts->spi_z1.spi_rx;
-	z2 = ts->spi_z2.spi_rx;
+	x = tsdata.x;
+	y = tsdata.y;
+	z1 = tsdata.z1;
+	z2 = tsdata.z2;
 
 	/* validate position */
 	if (unlikely(x > MAX_12BIT || y > MAX_12BIT))
@@ -346,9 +309,9 @@ static void tsc2005_penup_timer(unsigned long data)
 
 static void tsc2005_start_scan(struct tsc2005 *ts)
 {
-	tsc2005_write(ts, TSC2005_REG_CFR0, TSC2005_CFR0_INITVALUE);
-	tsc2005_write(ts, TSC2005_REG_CFR1, TSC2005_CFR1_INITVALUE);
-	tsc2005_write(ts, TSC2005_REG_CFR2, TSC2005_CFR2_INITVALUE);
+	regmap_write(ts->regmap, TSC2005_REG_CFR0, TSC2005_CFR0_INITVALUE);
+	regmap_write(ts->regmap, TSC2005_REG_CFR1, TSC2005_CFR1_INITVALUE);
+	regmap_write(ts->regmap, TSC2005_REG_CFR2, TSC2005_CFR2_INITVALUE);
 	tsc2005_cmd(ts, TSC2005_CMD_NORMAL);
 }
 
@@ -398,9 +361,9 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 {
 	struct spi_device *spi = to_spi_device(dev);
 	struct tsc2005 *ts = spi_get_drvdata(spi);
-	u16 temp_high;
-	u16 temp_high_orig;
-	u16 temp_high_test;
+	unsigned int temp_high;
+	unsigned int temp_high_orig;
+	unsigned int temp_high_test;
 	bool success = true;
 	int error;
 
@@ -411,7 +374,7 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 	 */
 	__tsc2005_disable(ts);
 
-	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high_orig);
+	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high_orig);
 	if (error) {
 		dev_warn(dev, "selftest failed: read error %d\n", error);
 		success = false;
@@ -420,14 +383,14 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 
 	temp_high_test = (temp_high_orig - 1) & MAX_12BIT;
 
-	error = tsc2005_write(ts, TSC2005_REG_TEMP_HIGH, temp_high_test);
+	error = regmap_write(ts->regmap, TSC2005_REG_TEMP_HIGH, temp_high_test);
 	if (error) {
 		dev_warn(dev, "selftest failed: write error %d\n", error);
 		success = false;
 		goto out;
 	}
 
-	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high);
+	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high);
 	if (error) {
 		dev_warn(dev, "selftest failed: read error %d after write\n",
 			 error);
@@ -450,7 +413,7 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 		goto out;
 
 	/* test that the reset really happened */
-	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high);
+	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high);
 	if (error) {
 		dev_warn(dev, "selftest failed: read error %d after reset\n",
 			 error);
@@ -503,7 +466,7 @@ static void tsc2005_esd_work(struct work_struct *work)
 {
 	struct tsc2005 *ts = container_of(work, struct tsc2005, esd_work.work);
 	int error;
-	u16 r;
+	unsigned int r;
 
 	if (!mutex_trylock(&ts->mutex)) {
 		/*
@@ -519,7 +482,7 @@ static void tsc2005_esd_work(struct work_struct *work)
 		goto out;
 
 	/* We should be able to read register without disabling interrupts. */
-	error = tsc2005_read(ts, TSC2005_REG_CFR0, &r);
+	error = regmap_read(ts->regmap, TSC2005_REG_CFR0, &r);
 	if (!error &&
 	    !((r ^ TSC2005_CFR0_INITVALUE) & TSC2005_CFR0_RW_MASK)) {
 		goto out;
@@ -583,20 +546,6 @@ static void tsc2005_close(struct input_dev *input)
 	mutex_unlock(&ts->mutex);
 }
 
-static void tsc2005_setup_spi_xfer(struct tsc2005 *ts)
-{
-	tsc2005_setup_read(&ts->spi_x, TSC2005_REG_X, false);
-	tsc2005_setup_read(&ts->spi_y, TSC2005_REG_Y, false);
-	tsc2005_setup_read(&ts->spi_z1, TSC2005_REG_Z1, false);
-	tsc2005_setup_read(&ts->spi_z2, TSC2005_REG_Z2, true);
-
-	spi_message_init(&ts->spi_read_msg);
-	spi_message_add_tail(&ts->spi_x.spi_xfer, &ts->spi_read_msg);
-	spi_message_add_tail(&ts->spi_y.spi_xfer, &ts->spi_read_msg);
-	spi_message_add_tail(&ts->spi_z1.spi_xfer, &ts->spi_read_msg);
-	spi_message_add_tail(&ts->spi_z2.spi_xfer, &ts->spi_read_msg);
-}
-
 static int tsc2005_probe(struct spi_device *spi)
 {
 	const struct tsc2005_platform_data *pdata = dev_get_platdata(&spi->dev);
@@ -661,6 +610,10 @@ static int tsc2005_probe(struct spi_device *spi)
 	ts->spi = spi;
 	ts->idev = input_dev;
 
+	ts->regmap = devm_regmap_init_spi(spi, &tsc2005_regmap_config);
+	if (IS_ERR(ts->regmap))
+		return PTR_ERR(ts->regmap);
+
 	ts->x_plate_ohm = x_plate_ohm;
 	ts->esd_timeout = esd_timeout;
 
@@ -700,8 +653,6 @@ static int tsc2005_probe(struct spi_device *spi)
 
 	INIT_DELAYED_WORK(&ts->esd_work, tsc2005_esd_work);
 
-	tsc2005_setup_spi_xfer(ts);
-
 	snprintf(ts->phys, sizeof(ts->phys),
 		 "%s/input-ts", dev_name(&spi->dev));
 

Re: [PATCH 2/4] Input: tsc2005 - convert to regmap

[Dmitry Torokhov]

On Wed, Jul 15, 2015 at 02:29:55PM -0700, Dmitry Torokhov wrote:
> Hi Sebastian,
> 
> On Wed, Jul 15, 2015 at 02:13:26PM +0200, Sebastian Reichel wrote:
> 
> >  	/* read the coordinates */
> > -	error = spi_sync(ts->spi, &ts->spi_read_msg);
> > +	error = regmap_bulk_read(ts->regmap, TSC2005_REG_X, &tsdata,
> > +				 TSC2005_DATA_REGS);
> >  	if (unlikely(error))
> >  		goto out;
> >  
> > -	x = ts->spi_x.spi_rx;
> > -	y = ts->spi_y.spi_rx;
> > -	z1 = ts->spi_z1.spi_rx;
> > -	z2 = ts->spi_z2.spi_rx;
> > -
> >  	/* validate position */
> > -	if (unlikely(x > MAX_12BIT || y > MAX_12BIT))
> > +	if (unlikely(tsdata.x > MAX_12BIT || tsdata.y > MAX_12BIT))
> >  		goto out;
> >  
> >  	/* Skip reading if the pressure components are out of range */
> > -	if (unlikely(z1 == 0 || z2 > MAX_12BIT || z1 >= z2))
> > +	if (unlikely(tsdata.z1 == 0 || tsdata.z2 > MAX_12BIT))
> > +		goto out;
> > +	if (unlikely(tsdata.z1 >= tsdata.z2))
> >  		goto out;
> 
> I guess that means that tsc2005 is (and was) endian-broken. We can't use
> the data off the wire directly...  So I'll drop bucnh of the changes in
> this function so we can convert to CPU endianness before using.

Ah, no, SPI transfers do convert to/from CPU endianness...

> 
> I also got:
> 
>   CC [M]  drivers/input/touchscreen/tsc2005.o
>   Building modules, stage 2.
> Kernel: arch/x86/boot/bzImage is ready  (#1383)
>   MODPOST 1403 modules
> ERROR: "devm_regmap_init_spi" [drivers/input/touchscreen/tsc2005.ko]
> undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> 
> I guess we need "select REGMAP_SPI".
> 
> Thanks.
> 
> -- 
> Dmitry
> 
> 
> Input: tsc2005 - convert to regmap
> 
> From: Sebastian Reichel <sre@kernel.org>
> 
> Convert driver, so that it uses regmap instead of directly using
> spi_transfer for all register accesses.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/Kconfig   |    9 +-
>  drivers/input/touchscreen/tsc2005.c |  149 ++++++++++++-----------------------
>  2 files changed, 55 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 9b3da52..c1acbbc 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -915,10 +915,11 @@ config TOUCHSCREEN_TSC_SERIO
>  	  module will be called tsc40.
>  
>  config TOUCHSCREEN_TSC2005
> -        tristate "TSC2005 based touchscreens"
> -        depends on SPI_MASTER
> -        help
> -          Say Y here if you have a TSC2005 based touchscreen.
> +	tristate "TSC2005 based touchscreens"
> +	depends on SPI_MASTER
> +	select REGMAP_SPI
> +	help
> +	  Say Y here if you have a TSC2005 based touchscreen.
>  
>  	  If unsure, say N.
>  
> diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
> index 2a27a1f..a04248e 100644
> --- a/drivers/input/touchscreen/tsc2005.c
> +++ b/drivers/input/touchscreen/tsc2005.c
> @@ -34,6 +34,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spi/tsc2005.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
>  
>  /*
>   * The touchscreen interface operates as follows:
> @@ -120,20 +121,36 @@
>  #define TSC2005_SPI_MAX_SPEED_HZ	10000000
>  #define TSC2005_PENUP_TIME_MS		40
>  
> -struct tsc2005_spi_rd {
> -	struct spi_transfer	spi_xfer;
> -	u32			spi_tx;
> -	u32			spi_rx;
> +static const struct regmap_range tsc2005_writable_ranges[] = {
> +	regmap_reg_range(TSC2005_REG_AUX_HIGH, TSC2005_REG_CFR2),
>  };
>  
> +static const struct regmap_access_table tsc2005_writable_table = {
> +	.yes_ranges = tsc2005_writable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(tsc2005_writable_ranges),
> +};
> +
> +static struct regmap_config tsc2005_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.reg_stride = 0x08,
> +	.max_register = 0x78,
> +	.read_flag_mask = TSC2005_REG_READ,
> +	.wr_table = &tsc2005_writable_table,
> +	.use_single_rw = true,
> +};
> +
> +struct tsc2005_data {
> +	u16 x;
> +	u16 y;
> +	u16 z1;
> +	u16 z2;
> +} __packed;
> +#define TSC2005_DATA_REGS 4
> +
>  struct tsc2005 {
>  	struct spi_device	*spi;
> -
> -	struct spi_message      spi_read_msg;
> -	struct tsc2005_spi_rd	spi_x;
> -	struct tsc2005_spi_rd	spi_y;
> -	struct tsc2005_spi_rd	spi_z1;
> -	struct tsc2005_spi_rd	spi_z2;
> +	struct regmap		*regmap;
>  
>  	struct input_dev	*idev;
>  	char			phys[32];
> @@ -190,62 +207,6 @@ static int tsc2005_cmd(struct tsc2005 *ts, u8 cmd)
>  	return 0;
>  }
>  
> -static int tsc2005_write(struct tsc2005 *ts, u8 reg, u16 value)
> -{
> -	u32 tx = ((reg | TSC2005_REG_PND0) << 16) | value;
> -	struct spi_transfer xfer = {
> -		.tx_buf		= &tx,
> -		.len		= 4,
> -		.bits_per_word	= 24,
> -	};
> -	struct spi_message msg;
> -	int error;
> -
> -	spi_message_init(&msg);
> -	spi_message_add_tail(&xfer, &msg);
> -
> -	error = spi_sync(ts->spi, &msg);
> -	if (error) {
> -		dev_err(&ts->spi->dev,
> -			"%s: failed, register: %x, value: %x, error: %d\n",
> -			__func__, reg, value, error);
> -		return error;
> -	}
> -
> -	return 0;
> -}
> -
> -static void tsc2005_setup_read(struct tsc2005_spi_rd *rd, u8 reg, bool last)
> -{
> -	memset(rd, 0, sizeof(*rd));
> -
> -	rd->spi_tx		   = (reg | TSC2005_REG_READ) << 16;
> -	rd->spi_xfer.tx_buf	   = &rd->spi_tx;
> -	rd->spi_xfer.rx_buf	   = &rd->spi_rx;
> -	rd->spi_xfer.len	   = 4;
> -	rd->spi_xfer.bits_per_word = 24;
> -	rd->spi_xfer.cs_change	   = !last;
> -}
> -
> -static int tsc2005_read(struct tsc2005 *ts, u8 reg, u16 *value)
> -{
> -	struct tsc2005_spi_rd spi_rd;
> -	struct spi_message msg;
> -	int error;
> -
> -	tsc2005_setup_read(&spi_rd, reg, true);
> -
> -	spi_message_init(&msg);
> -	spi_message_add_tail(&spi_rd.spi_xfer, &msg);
> -
> -	error = spi_sync(ts->spi, &msg);
> -	if (error)
> -		return error;
> -
> -	*value = spi_rd.spi_rx;
> -	return 0;
> -}
> -
>  static void tsc2005_update_pen_state(struct tsc2005 *ts,
>  				     int x, int y, int pressure)
>  {
> @@ -276,17 +237,19 @@ static irqreturn_t tsc2005_irq_thread(int irq, void *_ts)
>  	unsigned int pressure;
>  	u32 x, y;
>  	u32 z1, z2;
> +	struct tsc2005_data tsdata;
>  	int error;
>  
>  	/* read the coordinates */
> -	error = spi_sync(ts->spi, &ts->spi_read_msg);
> +	error = regmap_bulk_read(ts->regmap, TSC2005_REG_X, &tsdata,
> +				 TSC2005_DATA_REGS);
>  	if (unlikely(error))
>  		goto out;
>  
> -	x = ts->spi_x.spi_rx;
> -	y = ts->spi_y.spi_rx;
> -	z1 = ts->spi_z1.spi_rx;
> -	z2 = ts->spi_z2.spi_rx;
> +	x = tsdata.x;
> +	y = tsdata.y;
> +	z1 = tsdata.z1;
> +	z2 = tsdata.z2;
>  
>  	/* validate position */
>  	if (unlikely(x > MAX_12BIT || y > MAX_12BIT))
> @@ -346,9 +309,9 @@ static void tsc2005_penup_timer(unsigned long data)
>  
>  static void tsc2005_start_scan(struct tsc2005 *ts)
>  {
> -	tsc2005_write(ts, TSC2005_REG_CFR0, TSC2005_CFR0_INITVALUE);
> -	tsc2005_write(ts, TSC2005_REG_CFR1, TSC2005_CFR1_INITVALUE);
> -	tsc2005_write(ts, TSC2005_REG_CFR2, TSC2005_CFR2_INITVALUE);
> +	regmap_write(ts->regmap, TSC2005_REG_CFR0, TSC2005_CFR0_INITVALUE);
> +	regmap_write(ts->regmap, TSC2005_REG_CFR1, TSC2005_CFR1_INITVALUE);
> +	regmap_write(ts->regmap, TSC2005_REG_CFR2, TSC2005_CFR2_INITVALUE);
>  	tsc2005_cmd(ts, TSC2005_CMD_NORMAL);
>  }
>  
> @@ -398,9 +361,9 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
>  {
>  	struct spi_device *spi = to_spi_device(dev);
>  	struct tsc2005 *ts = spi_get_drvdata(spi);
> -	u16 temp_high;
> -	u16 temp_high_orig;
> -	u16 temp_high_test;
> +	unsigned int temp_high;
> +	unsigned int temp_high_orig;
> +	unsigned int temp_high_test;
>  	bool success = true;
>  	int error;
>  
> @@ -411,7 +374,7 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
>  	 */
>  	__tsc2005_disable(ts);
>  
> -	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high_orig);
> +	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high_orig);
>  	if (error) {
>  		dev_warn(dev, "selftest failed: read error %d\n", error);
>  		success = false;
> @@ -420,14 +383,14 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
>  
>  	temp_high_test = (temp_high_orig - 1) & MAX_12BIT;
>  
> -	error = tsc2005_write(ts, TSC2005_REG_TEMP_HIGH, temp_high_test);
> +	error = regmap_write(ts->regmap, TSC2005_REG_TEMP_HIGH, temp_high_test);
>  	if (error) {
>  		dev_warn(dev, "selftest failed: write error %d\n", error);
>  		success = false;
>  		goto out;
>  	}
>  
> -	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high);
> +	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high);
>  	if (error) {
>  		dev_warn(dev, "selftest failed: read error %d after write\n",
>  			 error);
> @@ -450,7 +413,7 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
>  		goto out;
>  
>  	/* test that the reset really happened */
> -	error = tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high);
> +	error = regmap_read(ts->regmap, TSC2005_REG_TEMP_HIGH, &temp_high);
>  	if (error) {
>  		dev_warn(dev, "selftest failed: read error %d after reset\n",
>  			 error);
> @@ -503,7 +466,7 @@ static void tsc2005_esd_work(struct work_struct *work)
>  {
>  	struct tsc2005 *ts = container_of(work, struct tsc2005, esd_work.work);
>  	int error;
> -	u16 r;
> +	unsigned int r;
>  
>  	if (!mutex_trylock(&ts->mutex)) {
>  		/*
> @@ -519,7 +482,7 @@ static void tsc2005_esd_work(struct work_struct *work)
>  		goto out;
>  
>  	/* We should be able to read register without disabling interrupts. */
> -	error = tsc2005_read(ts, TSC2005_REG_CFR0, &r);
> +	error = regmap_read(ts->regmap, TSC2005_REG_CFR0, &r);
>  	if (!error &&
>  	    !((r ^ TSC2005_CFR0_INITVALUE) & TSC2005_CFR0_RW_MASK)) {
>  		goto out;
> @@ -583,20 +546,6 @@ static void tsc2005_close(struct input_dev *input)
>  	mutex_unlock(&ts->mutex);
>  }
>  
> -static void tsc2005_setup_spi_xfer(struct tsc2005 *ts)
> -{
> -	tsc2005_setup_read(&ts->spi_x, TSC2005_REG_X, false);
> -	tsc2005_setup_read(&ts->spi_y, TSC2005_REG_Y, false);
> -	tsc2005_setup_read(&ts->spi_z1, TSC2005_REG_Z1, false);
> -	tsc2005_setup_read(&ts->spi_z2, TSC2005_REG_Z2, true);
> -
> -	spi_message_init(&ts->spi_read_msg);
> -	spi_message_add_tail(&ts->spi_x.spi_xfer, &ts->spi_read_msg);
> -	spi_message_add_tail(&ts->spi_y.spi_xfer, &ts->spi_read_msg);
> -	spi_message_add_tail(&ts->spi_z1.spi_xfer, &ts->spi_read_msg);
> -	spi_message_add_tail(&ts->spi_z2.spi_xfer, &ts->spi_read_msg);
> -}
> -
>  static int tsc2005_probe(struct spi_device *spi)
>  {
>  	const struct tsc2005_platform_data *pdata = dev_get_platdata(&spi->dev);
> @@ -661,6 +610,10 @@ static int tsc2005_probe(struct spi_device *spi)
>  	ts->spi = spi;
>  	ts->idev = input_dev;
>  
> +	ts->regmap = devm_regmap_init_spi(spi, &tsc2005_regmap_config);
> +	if (IS_ERR(ts->regmap))
> +		return PTR_ERR(ts->regmap);
> +
>  	ts->x_plate_ohm = x_plate_ohm;
>  	ts->esd_timeout = esd_timeout;
>  
> @@ -700,8 +653,6 @@ static int tsc2005_probe(struct spi_device *spi)
>  
>  	INIT_DELAYED_WORK(&ts->esd_work, tsc2005_esd_work);
>  
> -	tsc2005_setup_spi_xfer(ts);
> -
>  	snprintf(ts->phys, sizeof(ts->phys),
>  		 "%s/input-ts", dev_name(&spi->dev));
>  

-- 
Dmitry

Re: [PATCH 2/4] Input: tsc2005 - convert to regmap

[Sebastian Reichel]

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

Hi Dmitry,

On Wed, Jul 15, 2015 at 02:29:55PM -0700, Dmitry Torokhov wrote:
> I also got:
> 
>   CC [M]  drivers/input/touchscreen/tsc2005.o
>   Building modules, stage 2.
> Kernel: arch/x86/boot/bzImage is ready  (#1383)
>   MODPOST 1403 modules
> ERROR: "devm_regmap_init_spi" [drivers/input/touchscreen/tsc2005.ko]
> undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> 
> I guess we need "select REGMAP_SPI".

Right. I will add this in PATCHv2.

-- Sebastian

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

Re: [PATCH 2/4] Input: tsc2005 - convert to regmap

[Dmitry Torokhov]

On Wed, Jul 15, 2015 at 02:13:26PM +0200, Sebastian Reichel wrote:
> -static int tsc2005_write(struct tsc2005 *ts, u8 reg, u16 value)
> -{
> -	u32 tx = ((reg | TSC2005_REG_PND0) << 16) | value;
> -	struct spi_transfer xfer = {
> -		.tx_buf		= &tx,
> -		.len		= 4,
> -		.bits_per_word	= 24,
> -	};

I wonder why the original code used 24 bit-sized-words for transfers...

-- 
Dmitry

[PATCH 3/4] Input: tsc2005 - simplify drvdata acquisition

[Sebastian Reichel]

Do not convert device to spi_device just for getting
the driver data, since spi_get_drvdata() just calls
dev_get_drvdata().

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/input/touchscreen/tsc2005.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
index 0aec45a..83508d8 100644
--- a/drivers/input/touchscreen/tsc2005.c
+++ b/drivers/input/touchscreen/tsc2005.c
@@ -354,8 +354,7 @@ static ssize_t tsc2005_selftest_show(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)
 {
-	struct spi_device *spi = to_spi_device(dev);
-	struct tsc2005 *ts = spi_get_drvdata(spi);
+	struct tsc2005 *ts = dev_get_drvdata(dev);
 	unsigned int temp_high;
 	unsigned int temp_high_orig;
 	unsigned int temp_high_test;
@@ -440,8 +439,7 @@ static umode_t tsc2005_attr_is_visible(struct kobject *kobj,
 				      struct attribute *attr, int n)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
-	struct spi_device *spi = to_spi_device(dev);
-	struct tsc2005 *ts = spi_get_drvdata(spi);
+	struct tsc2005 *ts = dev_get_drvdata(dev);
 	umode_t mode = attr->mode;
 
 	if (attr == &dev_attr_selftest.attr) {
@@ -729,8 +727,7 @@ static int tsc2005_remove(struct spi_device *spi)
 
 static int __maybe_unused tsc2005_suspend(struct device *dev)
 {
-	struct spi_device *spi = to_spi_device(dev);
-	struct tsc2005 *ts = spi_get_drvdata(spi);
+	struct tsc2005 *ts = dev_get_drvdata(dev);
 
 	mutex_lock(&ts->mutex);
 
@@ -746,8 +743,7 @@ static int __maybe_unused tsc2005_suspend(struct device *dev)
 
 static int __maybe_unused tsc2005_resume(struct device *dev)
 {
-	struct spi_device *spi = to_spi_device(dev);
-	struct tsc2005 *ts = spi_get_drvdata(spi);
+	struct tsc2005 *ts = dev_get_drvdata(dev);
 
 	mutex_lock(&ts->mutex);
 
-- 
2.1.4

Re: [PATCH 3/4] Input: tsc2005 - simplify drvdata acquisition

[Dmitry Torokhov]

On Wed, Jul 15, 2015 at 02:13:27PM +0200, Sebastian Reichel wrote:
> Do not convert device to spi_device just for getting
> the driver data, since spi_get_drvdata() just calls
> dev_get_drvdata().

Even though at the moment they all share the same data I consider them
logically different and so would prefer not to substityte one for
another.

Thanks.

-- 
Dmitry

Re: [PATCH 3/4] Input: tsc2005 - simplify drvdata acquisition

[Sebastian Reichel]

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

Hi,

On Wed, Jul 15, 2015 at 02:31:07PM -0700, Dmitry Torokhov wrote:
> On Wed, Jul 15, 2015 at 02:13:27PM +0200, Sebastian Reichel wrote:
> > Do not convert device to spi_device just for getting
> > the driver data, since spi_get_drvdata() just calls
> > dev_get_drvdata().
> 
> Even though at the moment they all share the same data I consider them
> logically different and so would prefer not to substityte one for
> another.

I guess your problem is with mixing spi_*_drvdata and dev_*_drvdata
calls? In that case I will also change spi_set_drvdata to
dev_set_drvdata, so that spi_*_drvdata is not used at all.

This will still reduce lines of code and flatten the way for tsc2004
support (which is i2c based).

-- Sebastian

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

Re: [PATCH 3/4] Input: tsc2005 - simplify drvdata acquisition

[Dmitry Torokhov]

On Thu, Jul 16, 2015 at 12:02:21AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Jul 15, 2015 at 02:31:07PM -0700, Dmitry Torokhov wrote:
> > On Wed, Jul 15, 2015 at 02:13:27PM +0200, Sebastian Reichel wrote:
> > > Do not convert device to spi_device just for getting
> > > the driver data, since spi_get_drvdata() just calls
> > > dev_get_drvdata().
> > 
> > Even though at the moment they all share the same data I consider them
> > logically different and so would prefer not to substityte one for
> > another.
> 
> I guess your problem is with mixing spi_*_drvdata and dev_*_drvdata
> calls? In that case I will also change spi_set_drvdata to
> dev_set_drvdata, so that spi_*_drvdata is not used at all.
> 
> This will still reduce lines of code and flatten the way for tsc2004
> support (which is i2c based).

OK, that will work.

Thanks.

-- 
Dmitry

[PATCH 4/4] Input: tsc2005 - convert to gpiod

[Sebastian Reichel]

Convert driver to descriptor based GPIO API. Also
fix the after-probe reset GPIO state, so that the
device is not kept in reset state.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/input/touchscreen/tsc2005.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
index 83508d8..cb08dc8 100644
--- a/drivers/input/touchscreen/tsc2005.c
+++ b/drivers/input/touchscreen/tsc2005.c
@@ -30,11 +30,11 @@
 #include <linux/delay.h>
 #include <linux/pm.h>
 #include <linux/of.h>
-#include <linux/of_gpio.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/tsc2005.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
 
 /*
  * The touchscreen interface operates as follows:
@@ -179,7 +179,7 @@ struct tsc2005 {
 
 	struct regulator	*vio;
 
-	int			reset_gpio;
+	struct gpio_desc	*reset_gpio;
 	void			(*set_reset)(bool enable);
 };
 
@@ -317,8 +317,8 @@ static void tsc2005_stop_scan(struct tsc2005 *ts)
 
 static void tsc2005_set_reset(struct tsc2005 *ts, bool enable)
 {
-	if (ts->reset_gpio >= 0)
-		gpio_set_value(ts->reset_gpio, enable);
+	if (ts->reset_gpio)
+		gpiod_set_value_cansleep(ts->reset_gpio, enable);
 	else if (ts->set_reset)
 		ts->set_reset(enable);
 }
@@ -611,19 +611,11 @@ static int tsc2005_probe(struct spi_device *spi)
 	ts->esd_timeout = esd_timeout;
 
 	if (np) {
-		ts->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
-		if (ts->reset_gpio == -EPROBE_DEFER)
-			return ts->reset_gpio;
-		if (ts->reset_gpio < 0) {
+		ts->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
+						GPIOD_OUT_HIGH);
+		if (IS_ERR(ts->reset_gpio)) {
+			error = PTR_ERR(ts->reset_gpio);
 			dev_err(&spi->dev, "error acquiring reset gpio: %d\n",
-				ts->reset_gpio);
-			return ts->reset_gpio;
-		}
-
-		error = devm_gpio_request_one(&spi->dev, ts->reset_gpio, 0,
-					      "reset-gpios");
-		if (error) {
-			dev_err(&spi->dev, "error requesting reset gpio: %d\n",
 				error);
 			return error;
 		}
@@ -635,7 +627,6 @@ static int tsc2005_probe(struct spi_device *spi)
 			return error;
 		}
 	} else {
-		ts->reset_gpio = -1;
 		ts->set_reset = pdata->set_reset;
 	}
 
-- 
2.1.4

Re: [PATCH 4/4] Input: tsc2005 - convert to gpiod

[Dmitry Torokhov]

On Wed, Jul 15, 2015 at 02:13:28PM +0200, Sebastian Reichel wrote:
> Convert driver to descriptor based GPIO API. Also
> fix the after-probe reset GPIO state, so that the
> device is not kept in reset state.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  drivers/input/touchscreen/tsc2005.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
> index 83508d8..cb08dc8 100644
> --- a/drivers/input/touchscreen/tsc2005.c
> +++ b/drivers/input/touchscreen/tsc2005.c
> @@ -30,11 +30,11 @@
>  #include <linux/delay.h>
>  #include <linux/pm.h>
>  #include <linux/of.h>
> -#include <linux/of_gpio.h>
>  #include <linux/spi/spi.h>
>  #include <linux/spi/tsc2005.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/regmap.h>
> +#include <linux/gpio/consumer.h>
>  
>  /*
>   * The touchscreen interface operates as follows:
> @@ -179,7 +179,7 @@ struct tsc2005 {
>  
>  	struct regulator	*vio;
>  
> -	int			reset_gpio;
> +	struct gpio_desc	*reset_gpio;
>  	void			(*set_reset)(bool enable);
>  };
>  
> @@ -317,8 +317,8 @@ static void tsc2005_stop_scan(struct tsc2005 *ts)
>  
>  static void tsc2005_set_reset(struct tsc2005 *ts, bool enable)
>  {
> -	if (ts->reset_gpio >= 0)
> -		gpio_set_value(ts->reset_gpio, enable);
> +	if (ts->reset_gpio)
> +		gpiod_set_value_cansleep(ts->reset_gpio, enable);
>  	else if (ts->set_reset)
>  		ts->set_reset(enable);
>  }
> @@ -611,19 +611,11 @@ static int tsc2005_probe(struct spi_device *spi)
>  	ts->esd_timeout = esd_timeout;
>  
>  	if (np) {
> -		ts->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
> -		if (ts->reset_gpio == -EPROBE_DEFER)
> -			return ts->reset_gpio;
> -		if (ts->reset_gpio < 0) {
> +		ts->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
> +						GPIOD_OUT_HIGH);

I think we should treat the gpio as optional and try to get the
descriptor event on non-OF boards.

> +		if (IS_ERR(ts->reset_gpio)) {
> +			error = PTR_ERR(ts->reset_gpio);
>  			dev_err(&spi->dev, "error acquiring reset gpio: %d\n",
> -				ts->reset_gpio);
> -			return ts->reset_gpio;
> -		}
> -
> -		error = devm_gpio_request_one(&spi->dev, ts->reset_gpio, 0,
> -					      "reset-gpios");
> -		if (error) {
> -			dev_err(&spi->dev, "error requesting reset gpio: %d\n",
>  				error);
>  			return error;
>  		}
> @@ -635,7 +627,6 @@ static int tsc2005_probe(struct spi_device *spi)
>  			return error;
>  		}
>  	} else {
> -		ts->reset_gpio = -1;
>  		ts->set_reset = pdata->set_reset;
>  	}
>  
> -- 
> 2.1.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH 4/4] Input: tsc2005 - convert to gpiod

[Sebastian Reichel]

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

Hi,

On Wed, Jul 15, 2015 at 02:34:04PM -0700, Dmitry Torokhov wrote:
> [...]
> >  	if (np) {
> > -		ts->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
> > -		if (ts->reset_gpio == -EPROBE_DEFER)
> > -			return ts->reset_gpio;
> > -		if (ts->reset_gpio < 0) {
> > +		ts->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
> > +						GPIOD_OUT_HIGH);
> 
> I think we should treat the gpio as optional and try to get the
> descriptor event on non-OF boards.

As I wrote in the cover letter, I suggest to change this once the
Nokia N900 board code has been removed. At that point changing the
board code API is much easier, since it won't affect multiple trees.

> [...]

-- Sebastian

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

Re: [PATCH 4/4] Input: tsc2005 - convert to gpiod

[Dmitry Torokhov]

On Thu, Jul 16, 2015 at 12:09:41AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Jul 15, 2015 at 02:34:04PM -0700, Dmitry Torokhov wrote:
> > [...]
> > >  	if (np) {
> > > -		ts->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
> > > -		if (ts->reset_gpio == -EPROBE_DEFER)
> > > -			return ts->reset_gpio;
> > > -		if (ts->reset_gpio < 0) {
> > > +		ts->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
> > > +						GPIOD_OUT_HIGH);
> > 
> > I think we should treat the gpio as optional and try to get the
> > descriptor event on non-OF boards.
> 
> As I wrote in the cover letter, I suggest to change this once the
> Nokia N900 board code has been removed. At that point changing the
> board code API is much easier, since it won't affect multiple trees.

I do not see why it has be wait for Nokia board code. Just make it
devm_gpiod_get_optional() and call it unconditionally and fall back onto
custom reset function (if one is supplied).

Thanks.

-- 
Dmitry

Re: [PATCH 4/4] Input: tsc2005 - convert to gpiod

[Sebastian Reichel]

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

On Wed, Jul 15, 2015 at 05:25:32PM -0700, Dmitry Torokhov wrote:
> On Thu, Jul 16, 2015 at 12:09:41AM +0200, Sebastian Reichel wrote:
> > On Wed, Jul 15, 2015 at 02:34:04PM -0700, Dmitry Torokhov wrote:
> > > [...]
> > > >  	if (np) {
> > > > -		ts->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
> > > > -		if (ts->reset_gpio == -EPROBE_DEFER)
> > > > -			return ts->reset_gpio;
> > > > -		if (ts->reset_gpio < 0) {
> > > > +		ts->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
> > > > +						GPIOD_OUT_HIGH);
> > > 
> > > I think we should treat the gpio as optional and try to get the
> > > descriptor event on non-OF boards.
> > 
> > As I wrote in the cover letter, I suggest to change this once the
> > Nokia N900 board code has been removed. At that point changing the
> > board code API is much easier, since it won't affect multiple trees.
> 
> I do not see why it has be wait for Nokia board code. Just make it
> devm_gpiod_get_optional() and call it unconditionally and fall back onto
> custom reset function (if one is supplied).

Right. I guess the same could be done for vio regulator. I will
add this change in the next version of the patchset.

-- Sebastian

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