Skip to contentSkip to conversation

stmpe: mfd & gpio updates

PATCH In review Viresh Kumar 5 participants 17 messages Nov 17, 2011 – Dec 19, 2011

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v2 cover stmpe: mfd & gpio updates

Changes since V1:
- V2 of this patchset also includes, earlier sent patches, Acked-by Linus:
mfd/stmpe: separate out controller and interface specific code mfd/stmpe: Add support for SPI interface
- Issues with above patches are also fixed
- MODULE_DESCRIPTION and MODULE_AUTHOR are fixed in stmpe-spi and stmpe-i2c files
- gpio-stmpe patch is update to use stmpe->partnum instead of stmpe->variant->id_val
- Patch "mfd/stmpe: Move struct stmpe_variant_info to linux/mfd/stmpe.h" is dropped as it is no longer required.

Viresh Kumar (5):
mfd/stmpe: separate out controller and interface specific code mfd/stmpe: Add support for SPI interface
mfd/stmpe: Add support for stmpe variant 610 mfd/stmpe: ADD support for stmpe variant 801 gpio/gpio-stmpe: ADD support for stmpe variant 801

 drivers/gpio/gpio-stmpe.c |   31 +++++-
 drivers/mfd/Kconfig       |   19 ++++-
 drivers/mfd/Makefile      |    2 +
 drivers/mfd/stmpe-i2c.c   |  109 ++++++++++++++++++++
 drivers/mfd/stmpe-spi.c   |  150 +++++++++++++++++++++++++++
 drivers/mfd/stmpe.c       |  250 ++++++++++++++++++++++++++-------------------
 drivers/mfd/stmpe.h       |   53 ++++++++++
 include/linux/mfd/stmpe.h |    9 ++-
 8 files changed, 512 insertions(+), 111 deletions(-)
 create mode 100644 drivers/mfd/stmpe-i2c.c
 create mode 100644 drivers/mfd/stmpe-spi.c
Signature · 1 line
1.7.2.2
#1 lore raw

PATCH v2 1/5 mfd/stmpe: separate out controller and interface specific code

Few STMPE controller can have register interface over SPI or I2C. Current implementation only supports I2C and all code is present in a single file stmpe.c. It would be better to separate out I2C interface specific code from controller specific code. Later SPI specific code can be added in a separate file.

This patch separates out I2C and controller specific code into separate files, making stmpe.c independent of I2C.

---
 drivers/mfd/Kconfig       |   11 ++++
 drivers/mfd/Makefile      |    1 +
 drivers/mfd/stmpe-i2c.c   |  107 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/stmpe.c       |  133 +++++++++++++++------------------------------
 drivers/mfd/stmpe.h       |   33 +++++++++++
 include/linux/mfd/stmpe.h |    7 ++-
 6 files changed, 200 insertions(+), 92 deletions(-)
 create mode 100644 drivers/mfd/stmpe-i2c.c
drivers/mfd/Kconfig +11 -0
index a67adcb..2f8561a 100644
@@ -293,6 +293,17 @@ config MFD_STMPE
 		Keypad: stmpe-keypad
 		Touchscreen: stmpe-ts
 
+menu "STMPE Interface Drivers"
+depends on MFD_STMPE
+
+config STMPE_I2C
+	bool "STMPE I2C Inteface"
+	depends on I2C
+	default y
+	help
+	  This is used to enable I2C interface of STMPE
+endmenu
+
 config MFD_TC3589X
 	bool "Support Toshiba TC35892 and variants"
 	depends on I2C=y && GENERIC_HARDIRQS
drivers/mfd/Makefile +1 -0
index c580203..24ee9bb 100644
@@ -16,6 +16,7 @@ obj-$(CONFIG_MFD_DM355EVM_MSP)	+= dm355evm_msp.o
 obj-$(CONFIG_MFD_TI_SSP)	+= ti-ssp.o
 
 obj-$(CONFIG_MFD_STMPE)		+= stmpe.o
+obj-$(CONFIG_STMPE_I2C)		+= stmpe-i2c.o
 obj-$(CONFIG_MFD_TC3589X)	+= tc3589x.o
 obj-$(CONFIG_MFD_T7L66XB)	+= t7l66xb.o tmio_core.o
 obj-$(CONFIG_MFD_TC6387XB)	+= tc6387xb.o tmio_core.o
drivers/mfd/stmpe-i2c.c new file +107 -0

107 lines not on this page · raw message

drivers/mfd/stmpe.c +43 -90

223 lines not on this page · raw message

drivers/mfd/stmpe.h +33 -0
index e4ee3895..18d89a6 100644
@@ -8,6 +8,14 @@
 #ifndef __STMPE_H
 #define __STMPE_H
 
+#include <linux/device.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/stmpe.h>
+#include <linux/printk.h>
+#include <linux/types.h>
+
+extern const struct dev_pm_ops stmpe_dev_pm_ops;
+
 #ifdef STMPE_DUMP_BYTES
 static inline void stmpe_dump_bytes(const char *str, const void *buf,
 				    size_t len)
@@ -67,6 +75,31 @@ struct stmpe_variant_info {
 	int (*enable_autosleep)(struct stmpe *stmpe, int autosleep_timeout);
 };
 
+/**
+ * struct stmpe_client_info - i2c or spi specific routines/info
+ * @data: client specific data
+ * @read_byte: read single byte
+ * @write_byte: write single byte
+ * @read_block: read block or multiple bytes
+ * @write_block: write block or multiple bytes
+ * @init: client init routine, called during probe
+ */
+struct stmpe_client_info {
+	void *data;
+	int irq;
+	void *client;
+	struct device *dev;
+	int (*read_byte)(struct stmpe *stmpe, u8 reg);
+	int (*write_byte)(struct stmpe *stmpe, u8 reg, u8 val);
+	int (*read_block)(struct stmpe *stmpe, u8 reg, u8 len, u8 *values);
+	int (*write_block)(struct stmpe *stmpe, u8 reg, u8 len,
+			const u8 *values);
+	void (*init)(struct stmpe *stmpe);
+};
+
+int stmpe_probe(struct stmpe_client_info *ci, int partnum);
+int stmpe_remove(struct stmpe *stmpe);
+
 #define STMPE_ICR_LSB_HIGH	(1 << 2)
 #define STMPE_ICR_LSB_EDGE	(1 << 1)
 #define STMPE_ICR_LSB_GIM	(1 << 0)
include/linux/mfd/stmpe.h +5 -2
index 270d661..babc6b2 100644
@@ -50,13 +50,15 @@ enum {
 
 
 struct stmpe_variant_info;
+struct stmpe_client_info;
 
 /**
  * struct stmpe - STMPE MFD structure
  * @lock: lock protecting I/O operations
  * @irq_lock: IRQ bus lock
  * @dev: device, mostly for dev_dbg()
- * @i2c: i2c client
+ * @client: client - i2c or spi
+ * @ci: client specific information
  * @partnum: part number
  * @variant: the detected STMPE model number
  * @regs: list of addresses of registers which are at different addresses on
@@ -72,7 +74,8 @@ struct stmpe {
 	struct mutex lock;
 	struct mutex irq_lock;
 	struct device *dev;
-	struct i2c_client *i2c;
+	void *client;
+	struct stmpe_client_info *ci;
 	enum stmpe_partnum partnum;
 	struct stmpe_variant_info *variant;
 	const u8 *regs;
Signature · 1 line
1.7.2.2
#2 lore raw

PATCH v2 2/5 mfd/stmpe: Add support for SPI interface

In reply to Viresh Kumar

Few STMPE controller can have register interface over SPI or I2C. Current implementation only supports I2C.

This patch adds support for SPI interface for accessing STMPE's address space.

---
 drivers/mfd/Kconfig     |    8 ++-
 drivers/mfd/Makefile    |    1 +
 drivers/mfd/stmpe-spi.c |  148 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/stmpe.h     |    1 +
 4 files changed, 157 insertions(+), 1 deletions(-)
 create mode 100644 drivers/mfd/stmpe-spi.c
drivers/mfd/Kconfig +7 -1
index 2f8561a..ebfb296 100644
@@ -272,7 +272,7 @@ config TWL6040_CORE
 
 config MFD_STMPE
 	bool "Support STMicroelectronics STMPE"
-	depends on I2C=y && GENERIC_HARDIRQS
+	depends on (I2C=y || SPI_MASTER=y) && GENERIC_HARDIRQS
 	select MFD_CORE
 	help
 	  Support for the STMPE family of I/O Expanders from
@@ -302,6 +302,12 @@ config STMPE_I2C
 	default y
 	help
 	  This is used to enable I2C interface of STMPE
+
+config STMPE_SPI
+	bool "STMPE SPI Inteface"
+	depends on SPI_MASTER
+	help
+	  This is used to enable SPI interface of STMPE
 endmenu
 
 config MFD_TC3589X
drivers/mfd/Makefile +1 -0
index 24ee9bb..eac1e9f 100644
@@ -17,6 +17,7 @@ obj-$(CONFIG_MFD_TI_SSP)	+= ti-ssp.o
 
 obj-$(CONFIG_MFD_STMPE)		+= stmpe.o
 obj-$(CONFIG_STMPE_I2C)		+= stmpe-i2c.o
+obj-$(CONFIG_STMPE_SPI)		+= stmpe-spi.o
 obj-$(CONFIG_MFD_TC3589X)	+= tc3589x.o
 obj-$(CONFIG_MFD_T7L66XB)	+= t7l66xb.o tmio_core.o
 obj-$(CONFIG_MFD_TC6387XB)	+= tc6387xb.o tmio_core.o
drivers/mfd/stmpe-spi.c new file +148 -0

148 lines not on this page · raw message

drivers/mfd/stmpe.h +1 -0
index 18d89a6..a73f4c1 100644
@@ -120,6 +120,7 @@ int stmpe_remove(struct stmpe *stmpe);
 
 #define STMPE811_REG_CHIP_ID		0x00
 #define STMPE811_REG_SYS_CTRL2		0x04
+#define STMPE811_REG_SPI_CFG		0x08
 #define STMPE811_REG_INT_CTRL		0x09
 #define STMPE811_REG_INT_EN		0x0A
 #define STMPE811_REG_INT_STA		0x0B
Signature · 1 line
1.7.2.2
#3 lore raw

PATCH v2 3/5 mfd/stmpe: Add support for stmpe variant 610

In reply to Viresh Kumar

STMPE610 is very much like STMPE811, except the number of gpio pins, which is 8 in 811 and 6 in 610. This patch adds support for variant 610. STMPE610 will share most of the code with STMPE811.

---
 drivers/mfd/stmpe-i2c.c   |    1 +
 drivers/mfd/stmpe-spi.c   |    1 +
 drivers/mfd/stmpe.c       |   20 ++++++++++++++++++--
 include/linux/mfd/stmpe.h |    1 +
 4 files changed, 21 insertions(+), 2 deletions(-)
drivers/mfd/stmpe-i2c.c +1 -0
index 0a43659..b11d33b 100644
@@ -71,6 +71,7 @@ static int __devexit stmpe_i2c_remove(struct i2c_client *i2c)
 }
 
 static const struct i2c_device_id stmpe_i2c_id[] = {
+	{ "stmpe610", STMPE610 },
 	{ "stmpe811", STMPE811 },
 	{ "stmpe1601", STMPE1601 },
 	{ "stmpe2401", STMPE2401 },
drivers/mfd/stmpe-spi.c +1 -0
index 53efce4..46963a5 100644
@@ -109,6 +109,7 @@ static int __devexit stmpe_spi_remove(struct spi_device *spi)
 }
 
 static const struct spi_device_id stmpe_spi_id[] = {
+	{ "stmpe610", STMPE610 },
 	{ "stmpe811", STMPE811 },
 	{ "stmpe1601", STMPE1601 },
 	{ "stmpe2401", STMPE2401 },
drivers/mfd/stmpe.c +18 -2
index 83bacde..67ff3dc 100644
@@ -321,7 +321,7 @@ static struct mfd_cell stmpe_keypad_cell = {
 };
 
 /*
- * Touchscreen (STMPE811)
+ * Touchscreen (STMPE811 or STMPE610)
  */
 
 static struct resource stmpe_ts_resources[] = {
@@ -346,7 +346,7 @@ static struct mfd_cell stmpe_ts_cell = {
 };
 
 /*
- * STMPE811
+ * STMPE811 or STMPE610
  */
 
 static const u8 stmpe811_regs[] = {
@@ -417,6 +417,21 @@ static struct stmpe_variant_info stmpe811 = {
 	.get_altfunc	= stmpe811_get_altfunc,
 };
 
+/* Similar to 811, except number of gpios */
+static struct stmpe_variant_info stmpe610 = {
+	.name		= "stmpe610",
+	.id_val		= 0x0811,
+	.id_mask	= 0xffff,
+	.num_gpios	= 6,
+	.af_bits	= 1,
+	.regs		= stmpe811_regs,
+	.blocks		= stmpe811_blocks,
+	.num_blocks	= ARRAY_SIZE(stmpe811_blocks),
+	.num_irqs	= STMPE811_NR_INTERNAL_IRQS,
+	.enable		= stmpe811_enable,
+	.get_altfunc	= stmpe811_get_altfunc,
+};
+
 /*
  * STMPE1601
  */
@@ -651,6 +666,7 @@ static struct stmpe_variant_info stmpe2403 = {
 };
 
 static struct stmpe_variant_info *stmpe_variant_info[] = {
+	[STMPE610]	= &stmpe610,
 	[STMPE811]	= &stmpe811,
 	[STMPE1601]	= &stmpe1601,
 	[STMPE2401]	= &stmpe2401,
include/linux/mfd/stmpe.h +1 -0
index babc6b2..342005a 100644
@@ -20,6 +20,7 @@ enum stmpe_block {
 };
 
 enum stmpe_partnum {
+	STMPE610,
 	STMPE811,
 	STMPE1601,
 	STMPE2401,
Signature · 1 line
1.7.2.2
Acked-by on v2 3/5
#4 lore raw

Re: [PATCH V2 3/5] mfd/stmpe: Add support for stmpe variant 610

Quoted 5 lines · Viresh Kumar

STMPE610 is very much like STMPE811, except the number of gpio pins, which is 8 in 811 and 6 in 610. This patch adds support for variant 610. STMPE610 will share most of the code with STMPE811.

Thanks,
Linus Walleij

#5 lore raw

PATCH v2 4/5 mfd/stmpe: ADD support for stmpe variant 801

In reply to Viresh Kumar

STMPE801 is a GPIO expander. Registers for 801 are much different from other variants. This patch adds support for STMPE801 in stmpe mfd driver.

---
 drivers/mfd/stmpe-i2c.c   |    1 +
 drivers/mfd/stmpe-spi.c   |    1 +
 drivers/mfd/stmpe.c       |   97 +++++++++++++++++++++++++++++++++++++++------
 drivers/mfd/stmpe.h       |   19 +++++++++
 include/linux/mfd/stmpe.h |    1 +
 5 files changed, 106 insertions(+), 13 deletions(-)
drivers/mfd/stmpe-i2c.c +1 -0
index b11d33b..373f423 100644
@@ -72,6 +72,7 @@ static int __devexit stmpe_i2c_remove(struct i2c_client *i2c)
 
 static const struct i2c_device_id stmpe_i2c_id[] = {
 	{ "stmpe610", STMPE610 },
+	{ "stmpe801", STMPE801 },
 	{ "stmpe811", STMPE811 },
 	{ "stmpe1601", STMPE1601 },
 	{ "stmpe2401", STMPE2401 },
drivers/mfd/stmpe-spi.c +1 -0
index 46963a5..b58c43c 100644
@@ -110,6 +110,7 @@ static int __devexit stmpe_spi_remove(struct spi_device *spi)
 
 static const struct spi_device_id stmpe_spi_id[] = {
 	{ "stmpe610", STMPE610 },
+	{ "stmpe801", STMPE801 },
 	{ "stmpe811", STMPE811 },
 	{ "stmpe1601", STMPE1601 },
 	{ "stmpe2401", STMPE2401 },
drivers/mfd/stmpe.c +84 -13

151 lines not on this page · raw message

drivers/mfd/stmpe.h +19 -0
index a73f4c1..7b8e13f 100644
@@ -105,6 +105,25 @@ int stmpe_remove(struct stmpe *stmpe);
 #define STMPE_ICR_LSB_GIM	(1 << 0)
 
 /*
+ * STMPE801
+ */
+#define STMPE801_ID			0x0108
+#define STMPE801_NR_INTERNAL_IRQS	1
+
+#define STMPE801_REG_CHIP_ID		0x00
+#define STMPE801_REG_VERSION_ID		0x02
+#define STMPE801_REG_SYS_CTRL		0x04
+#define STMPE801_REG_GPIO_INT_EN	0x08
+#define STMPE801_REG_GPIO_INT_STA	0x09
+#define STMPE801_REG_GPIO_MP_STA	0x10
+#define STMPE801_REG_GPIO_SET_PIN	0x11
+#define STMPE801_REG_GPIO_DIR		0x12
+
+#define STMPE801_REG_SYS_CTRL_RESET	(1 << 7)
+#define STMPE801_REG_SYS_CTRL_INT_EN	(1 << 2)
+#define STMPE801_REG_SYS_CTRL_INT_HI	(1 << 0)
+
+/*
  * STMPE811
  */
 
include/linux/mfd/stmpe.h +1 -0
index 342005a..ca1d7a3 100644
@@ -21,6 +21,7 @@ enum stmpe_block {
 
 enum stmpe_partnum {
 	STMPE610,
+	STMPE801,
 	STMPE811,
 	STMPE1601,
 	STMPE2401,
Signature · 1 line
1.7.2.2
#6 lore raw

PATCH v2 5/5 gpio/gpio-stmpe: ADD support for stmpe variant 801

In reply to Viresh Kumar

STMPE801 is a GPIO expander. GPIO registers for 801 are slightly different from other variants. This patch adds support for STMPE801 in stmpe gpio driver.

---
 drivers/gpio/gpio-stmpe.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)
drivers/gpio/gpio-stmpe.c +28 -3
index 4c980b5..000b019 100644
@@ -65,7 +65,15 @@ static void stmpe_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
 	u8 reg = stmpe->regs[which] - (offset / 8);
 	u8 mask = 1 << (offset % 8);
 
-	stmpe_reg_write(stmpe, reg, mask);
+	/*
+	 * Some variants have single register for gpio set/clear functionality.
+	 * For them we need to write 0 to clear and 1 to set.
+	 */
+	if (!val && (stmpe->regs[STMPE_IDX_GPSR_LSB] ==
+				stmpe->regs[STMPE_IDX_GPCR_LSB]))
+		stmpe_set_bits(stmpe, reg, mask, ~mask);
+	else
+		stmpe_set_bits(stmpe, reg, mask, mask);
 }
 
 static int stmpe_gpio_direction_output(struct gpio_chip *chip,
@@ -125,10 +133,19 @@ static struct gpio_chip template_chip = {
 static int stmpe_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct stmpe_gpio *stmpe_gpio = irq_data_get_irq_chip_data(d);
+	struct stmpe *stmpe = stmpe_gpio->stmpe;
 	int offset = d->irq - stmpe_gpio->irq_base;
 	int regoffset = offset / 8;
 	int mask = 1 << (offset % 8);
 
+	/* STMPE801 doesn't have RE and FE registers */
+	if (stmpe->partnum == STMPE801) {
+		if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
+			return 0;
+		else
+			return -EINVAL;
+	}
+
 	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
 		return -EINVAL;
 
@@ -165,6 +182,11 @@ static void stmpe_gpio_irq_sync_unlock(struct irq_data *d)
 	int i, j;
 
 	for (i = 0; i < CACHE_NR_REGS; i++) {
+		/* STMPE801 doesn't have RE and FE registers */
+		if ((stmpe->partnum == STMPE801) &&
+				(i != REG_IE))
+			continue;
+
 		for (j = 0; j < num_banks; j++) {
 			u8 old = stmpe_gpio->oldregs[i][j];
 			u8 new = stmpe_gpio->regs[i][j];
@@ -241,8 +263,11 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
 		}
 
 		stmpe_reg_write(stmpe, statmsbreg + i, status[i]);
-		stmpe_reg_write(stmpe, stmpe->regs[STMPE_IDX_GPEDR_MSB] + i,
-				status[i]);
+
+		/* Edge detect register is not present on 801 */
+		if (stmpe->partnum != STMPE801)
+			stmpe_reg_write(stmpe, stmpe->regs[STMPE_IDX_GPEDR_MSB]
+					+ i, status[i]);
 	}
 
 	return IRQ_HANDLED;
Signature · 1 line
1.7.2.2
#7 lore raw

Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Quoted 9 lines · Viresh Kumar
drivers/gpio/gpio-stmpe.c +0 -1
index 4c980b5..000b019 100644
@@ -65,7 +65,15 @@ static void stmpe_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
        u8 reg = stmpe->regs[which] - (offset / 8);
        u8 mask = 1 << (offset % 8);
 
-       stmpe_reg_write(stmpe, reg, mask);

drivers/gpio/gpio-stmpe.c static void stmpe_gpio_set(struct gpio_chip *chip, unsigned offset, int val) · line 77

Quoted 10 lines
drivers/gpio/gpio-stmpe.c +9 -0
+       /*
+        * Some variants have single register for gpio set/clear functionality.
+        * For them we need to write 0 to clear and 1 to set.
+        */
+       if (!val && (stmpe->regs[STMPE_IDX_GPSR_LSB] ==
+                               stmpe->regs[STMPE_IDX_GPCR_LSB]))
+               stmpe_set_bits(stmpe, reg, mask, ~mask);
+       else
+               stmpe_set_bits(stmpe, reg, mask, mask);
  }

This code,

(1) for 801, when clearing one GPIO, sets all the others.
(2) for other devices, adds an an unnecessary read (within stmpe_set_bits()), which wasn't there before.

Please rework to something like:

	if (stmpe->regs[...)
		stmpe_set_bits(stmpe, reg, mask, val ? mask : 0);
	else
		stmpe_reg_write(stmpe, reg, mask);
Quoted 5 lines
drivers/gpio/gpio-stmpe.c +0 -0
  static int stmpe_gpio_direction_output(struct gpio_chip *chip,
@@ -125,10 +133,19 @@ static struct gpio_chip template_chip = {
  static int stmpe_gpio_irq_set_type(struct irq_data *d, unsigned int type)
  {

drivers/gpio/gpio-stmpe.c static struct gpio_chip template_chip = { · line 144

Quoted 10 lines
drivers/gpio/gpio-stmpe.c +5 -0
        struct stmpe_gpio *stmpe_gpio = irq_data_get_irq_chip_data(d);
+       struct stmpe *stmpe = stmpe_gpio->stmpe;
        int offset = d->irq - stmpe_gpio->irq_base;
        int regoffset = offset / 8;
        int mask = 1 << (offset % 8);
 
+       /* STMPE801 doesn't have RE and FE registers */
+       if (stmpe->partnum == STMPE801) {
+               if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
+                       return 0;

This looks wrong. From the datasheet I see that it supports edges only, so perhaps you meant to say IRQ_TYPE_EDGE_* above.

In that case please reorganize this to add the return 0 after the existing check which excludes levels (below).

Quoted 6 lines
drivers/gpio/gpio-stmpe.c +4 -0
+               else
+                       return -EINVAL;
+       }
+
        if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
                return -EINVAL;
#8 lore raw

Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Quoted 15 lines · Rabin Vincent

drivers/gpio/gpio-stmpe.c static void stmpe_gpio_set(struct gpio_chip *chip, unsigned offset, int val) · line 70

Quoted 11 lines · Viresh Kumar
drivers/gpio/gpio-stmpe.c +5 -0
index 4c980b5..000b019 100644
@@ -65,7 +65,15 @@ static void stmpe_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
+       if (!val && (stmpe->regs[STMPE_IDX_GPSR_LSB] ==
+                               stmpe->regs[STMPE_IDX_GPCR_LSB]))
+               stmpe_set_bits(stmpe, reg, mask, ~mask);
+       else
+               stmpe_set_bits(stmpe, reg, mask, mask);
 }

This code,

Quoted 1 line

(1) for 801, when clearing one GPIO, sets all the others.

I assumed stmpe_set_bits will only affect bits which are 1 in mask and i was wrong. :(

Quoted 3 lines

(2) for other devices, adds an an unnecessary read (within stmpe_set_bits()), which wasn't there before.

Correct.

Quoted 7 lines

Please rework to something like:

	if (stmpe->regs[...)
		stmpe_set_bits(stmpe, reg, mask, val ? mask : 0);
	else
		stmpe_reg_write(stmpe, reg, mask);

Sure.

Quoted 11 lines
Quoted 11 lines
drivers/gpio/gpio-stmpe.c +1 -0
 static int stmpe_gpio_direction_output(struct gpio_chip *chip,
@@ -125,10 +133,19 @@ static struct gpio_chip template_chip = {
 static int stmpe_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
       struct stmpe_gpio *stmpe_gpio = irq_data_get_irq_chip_data(d);
+       struct stmpe *stmpe = stmpe_gpio->stmpe;
       int offset = d->irq - stmpe_gpio->irq_base;
       int regoffset = offset / 8;
       int mask = 1 << (offset % 8);
 
Quoted 8 lines

drivers/gpio/gpio-stmpe.c static struct gpio_chip template_chip = { · line 144

Quoted 4 lines
drivers/gpio/gpio-stmpe.c +4 -0
+       /* STMPE801 doesn't have RE and FE registers */
+       if (stmpe->partnum == STMPE801) {
+               if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
+                       return 0;

This looks wrong. From the datasheet I see that it supports edges only, so perhaps you meant to say IRQ_TYPE_EDGE_* above.

I meant to say IRQ_TYPE_LEVEL* only, but i was wrong. I didn't read the manual correctly. :(

Quoted 3 lines

In that case please reorganize this to add the return 0 after the existing check which excludes levels (below).

Sure

Signature · 1 line
viresh
#9 lore raw

Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Quoted 6 lines · Viresh Kumar
Quoted 1 line · Rabin Vincent

(1) for 801, when clearing one GPIO, sets all the others.

I assumed stmpe_set_bits will only affect bits which are 1 in mask and i was wrong. :(

That does look like a straight bug in stmpe_set_bits() which ought to be fixed regardless of anything else.

Quoted 6 lines
Quoted 2 lines

(2) for other devices, adds an an unnecessary read (within stmpe_set_bits()), which wasn't there before.

Correct.

Quoted 1 line

Please rework to something like:

Looking at the code in stmpe.c it might be as well to convert to regmap which would if nothing else save a bunch of custom code in the driver and let you use the cache code to eliminate the read. Probably that will help eliminate reads in other places too.

#10 lore raw

Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Quoted 5 lines · Mark Brown
Quoted 2 lines
Quoted 2 lines

I assumed stmpe_set_bits will only affect bits which are 1 in mask and i was wrong. :(

That does look like a straight bug in stmpe_set_bits() which ought to be fixed regardless of anything else.

I think so. I will fix it in a separate patch.

Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

(2) for other devices, adds an an unnecessary read (within stmpe_set_bits()), which wasn't there before.

Correct.

Quoted 1 line
Quoted 1 line

Please rework to something like:

Looking at the code in stmpe.c it might be as well to convert to regmap which would if nothing else save a bunch of custom code in the driver and let you use the cache code to eliminate the read. Probably that will help eliminate reads in other places too.

Ya, i already got this feedback from Rabin. I am a bit busy right now and so requested sameo to accept this patchset as it is. I will update drivers to use regmap as soon as i get some time.

Signature · 1 line
viresh
#11 lore raw

Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Hi Viresh,

Quoted 13 lines · Viresh Kumar
Quoted 5 lines · Mark Brown
Quoted 2 lines
Quoted 2 lines

I assumed stmpe_set_bits will only affect bits which are 1 in mask and i was wrong. :(

That does look like a straight bug in stmpe_set_bits() which ought to be fixed regardless of anything else.

I think so. I will fix it in a separate patch.

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

(2) for other devices, adds an an unnecessary read (within stmpe_set_bits()), which wasn't there before.

Correct.

Quoted 1 line
Quoted 1 line

Please rework to something like:

Quoted 8 lines
Quoted 4 lines

Looking at the code in stmpe.c it might be as well to convert to regmap which would if nothing else save a bunch of custom code in the driver and let you use the cache code to eliminate the read. Probably that will help eliminate reads in other places too.

Ya, i already got this feedback from Rabin. I am a bit busy right now and so requested sameo to accept this patchset as it is. I will update drivers to use regmap as soon as i get some time.

Ok, so I reverted your first 2 patches patchset since it seems it was problematic. Now, I think Rabin's concern on your patch #5 seems valid, and I'd like you to fix it before applying this patchset.

Cheers,
Samuel.

Signature · 2 lines
Intel Open Source Technology Centre
http://oss.intel.com/
#12 lore raw

Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Quoted 3 lines · Samuel Ortiz

Ok, so I reverted your first 2 patches patchset since it seems it was problematic. Now, I think Rabin's concern on your patch #5 seems valid, and I'd like you to fix it before applying this patchset.

Which problem are you talking about? Issues in v2 of patch 5/5 or converting to regmap?

Earlier once are already fixed in V3 of 5/5. And for later one, i already confirmed to do it in a separate patchset.

Signature · 1 line
viresh
#13 lore raw

Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Hi Viresh,

Quoted 9 lines · Viresh Kumar
Quoted 3 lines · Samuel Ortiz

Ok, so I reverted your first 2 patches patchset since it seems it was problematic. Now, I think Rabin's concern on your patch #5 seems valid, and I'd like you to fix it before applying this patchset.

Which problem are you talking about? Issues in v2 of patch 5/5 or converting to regmap?

Earlier once are already fixed in V3 of 5/5. And for later one, i already confirmed to do it in a separate patchset.

I haven't received a V3 for 5/5. Could you please re-send it to me ?

Cheers,
Samuel.

Signature · 2 lines
Intel Open Source Technology Centre
http://oss.intel.com/
#14 lore raw

Re: [PATCH V2 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Quoted 1 line · Samuel Ortiz

I haven't received a V3 for 5/5. Could you please re-send it to me ?

I sent it again. :)

Signature · 1 line
viresh
#15 lore raw

PATCH v3 5/5 gpio/gpio-stmpe: ADD support for stmpe variant 801

In reply to Viresh Kumar

STMPE801 is a GPIO expander. GPIO registers for 801 are slightly different from other variants. This patch adds support for STMPE801 in stmpe gpio driver.

---

Changes since V2:
- stmpe_gpio_set() is updates, so that it doesn't configure other gpio pins by mistake.
- STMPE801 also supports Edge interrupts and not level ints. Code updated for this too.

 drivers/gpio/gpio-stmpe.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)
drivers/gpio/gpio-stmpe.c +22 -3
index 4c980b5..87a68a8 100644
@@ -65,7 +65,14 @@ static void stmpe_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
 	u8 reg = stmpe->regs[which] - (offset / 8);
 	u8 mask = 1 << (offset % 8);
 
-	stmpe_reg_write(stmpe, reg, mask);
+	/*
+	 * Some variants have single register for gpio set/clear functionality.
+	 * For them we need to write 0 to clear and 1 to set.
+	 */
+	if (stmpe->regs[STMPE_IDX_GPSR_LSB] == stmpe->regs[STMPE_IDX_GPCR_LSB])
+		stmpe_set_bits(stmpe, reg, mask, val ? mask : 0);
+	else
+		stmpe_reg_write(stmpe, reg, mask);
 }
 
 static int stmpe_gpio_direction_output(struct gpio_chip *chip,
@@ -132,6 +139,10 @@ static int stmpe_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
 		return -EINVAL;
 
+	/* STMPE801 doesn't have RE and FE registers */
+	if (stmpe_gpio->stmpe->partnum == STMPE801)
+		return 0;
+
 	if (type == IRQ_TYPE_EDGE_RISING)
 		stmpe_gpio->regs[REG_RE][regoffset] |= mask;
 	else
@@ -165,6 +176,11 @@ static void stmpe_gpio_irq_sync_unlock(struct irq_data *d)
 	int i, j;
 
 	for (i = 0; i < CACHE_NR_REGS; i++) {
+		/* STMPE801 doesn't have RE and FE registers */
+		if ((stmpe->partnum == STMPE801) &&
+				(i != REG_IE))
+			continue;
+
 		for (j = 0; j < num_banks; j++) {
 			u8 old = stmpe_gpio->oldregs[i][j];
 			u8 new = stmpe_gpio->regs[i][j];
@@ -241,8 +257,11 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
 		}
 
 		stmpe_reg_write(stmpe, statmsbreg + i, status[i]);
-		stmpe_reg_write(stmpe, stmpe->regs[STMPE_IDX_GPEDR_MSB] + i,
-				status[i]);
+
+		/* Edge detect register is not present on 801 */
+		if (stmpe->partnum != STMPE801)
+			stmpe_reg_write(stmpe, stmpe->regs[STMPE_IDX_GPEDR_MSB]
+					+ i, status[i]);
 	}
 
 	return IRQ_HANDLED;
Signature · 1 line
1.7.2.2
#16 lore raw

Re: [PATVCH V3 Resend 5/5] gpio/gpio-stmpe: ADD support for stmpe variant 801

Hi Viresh,

Quoted 11 lines · Viresh Kumar

STMPE801 is a GPIO expander. GPIO registers for 801 are slightly different from other variants. This patch adds support for STMPE801 in stmpe gpio driver.

---

Hi Samuel,

Please apply this patch instead of V2 5/5.

All 5 patches applied now, thanks.

Cheers,
Samuel.

Signature · 2 lines
Intel Open Source Technology Centre
http://oss.intel.com/