2016-04-19 12:21:52

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support

From: Patrice Chotard <[email protected]>

This series cleans and fixes some bugs in MFD/GPIO STMPE drivers and prepare
the ground to add new STMPE1600 support.

STMPE1600 datasheet is available here :
http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html

Only STMPE1600 has been tested on STM32 platform. As i have no board with
others STMPE variant(STMPE610/STMPE801/STMPE811/STMPE1601/STMPE1801/STMPE2401
and STMPE2403), i put in CC boards's maintainers which are using others STMPE variant.

If they can kindly check that no regression has been introduce by this series
:

For ARM/FREESCALE IMX / MXC ARM ARCHITECTURE:
_ Shawn Guo <[email protected]>
_ Sascha Hauer <[email protected]>

For ARM/SOCFPGA ARCHITECTURE
_ Dinh Nguyen <[email protected]>

For SPEAR PLATFORM SUPPORT
_ Viresh Kumar <[email protected]>
_ Shiraz Hashim <[email protected]>

For TEGRA ARCHITECTURE SUPPORT
_ Stephen Warren <[email protected]>
_ Thierry Reding <[email protected]>
_ Alexandre Courbot <[email protected]>

For ARM/Ux500 ARM ARCHITECTURE
_ Linus Walleij <[email protected]>

Patrice Chotard (8):
mfd: stmpe: Add STMPE_IDX_SYS_CTRL/2 enum
mfd: stmpe: Add reset support for all STMPE variant
gpio: stmpe: fix edge and rising/falling edge detection
gpio: stmpe: write int status register only when needed
Documentation: dt: add stmpe1600 compatible string to stmpe mfd
mfd: Add STMPE1600 support
gpio: stmpe: Add STMPE1600 support
gpio: stmpe: configure GPIO as output by default

Documentation/devicetree/bindings/mfd/stmpe.txt | 2 +-
drivers/gpio/gpio-stmpe.c | 142 ++++++++++++++++++------
drivers/mfd/stmpe-i2c.c | 2 +
drivers/mfd/stmpe.c | 105 ++++++++++++++----
drivers/mfd/stmpe.h | 30 ++++-
include/linux/mfd/stmpe.h | 3 +
6 files changed, 223 insertions(+), 61 deletions(-)

--
1.9.1


2016-04-19 12:19:32

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 4/8] gpio: stmpe: write int status register only when needed

From: Patrice Chotard <[email protected]>

On STMPE801/1801 datasheets, it's mentionned writing
in interrupt status register has no effect, bits are
cleared when reading.

Signed-off-by: Amelie DELAUNAY <[email protected]>
Signed-off-by: Patrice Chotard <[email protected]>
---
drivers/gpio/gpio-stmpe.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index 225e075..f8c9d22 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -344,12 +344,16 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
stat &= ~(1 << bit);
}

- stmpe_reg_write(stmpe, statmsbreg + i, status[i]);
-
- /* Edge detect register is not present on 801 and 1801 */
- if (stmpe->partnum != STMPE801 || stmpe->partnum != STMPE1801)
+ /*
+ * interrupt status register write has no effect on
+ * 801 and 1801, bits are cleared when read.
+ * Edge detect register is not present on 801 and 1801
+ */
+ if (stmpe->partnum != STMPE801 || stmpe->partnum != STMPE1801) {
+ stmpe_reg_write(stmpe, statmsbreg + i, status[i]);
stmpe_reg_write(stmpe, stmpe->regs[STMPE_IDX_GPEDR_MSB]
+ i, status[i]);
+ }
}

return IRQ_HANDLED;
--
1.9.1

2016-04-19 12:19:33

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 5/8] Documentation: dt: add stmpe1600 compatible string to stmpe mfd

From: Patrice Chotard <[email protected]>

This patch adds a new compatible string for stmpe mfd to support
stmpe1600 variant.

Signed-off-by: Amelie DELAUNAY <[email protected]>
---
Documentation/devicetree/bindings/mfd/stmpe.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
index 3fb68bf..f9065a5 100644
--- a/Documentation/devicetree/bindings/mfd/stmpe.txt
+++ b/Documentation/devicetree/bindings/mfd/stmpe.txt
@@ -4,7 +4,7 @@ STMPE is an MFD device which may expose the following inbuilt devices: gpio,
keypad, touchscreen, adc, pwm, rotator.

Required properties:
- - compatible : "st,stmpe[610|801|811|1601|2401|2403]"
+ - compatible : "st,stmpe[610|801|811|1600|1601|2401|2403]"
- reg : I2C/SPI address of the device

Optional properties:
--
1.9.1

2016-04-19 12:19:36

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 1/8] mfd: stmpe: Add STMPE_IDX_SYS_CTRL/2 enum

From: Patrice Chotard <[email protected]>

As STMPE1801/1601/24xx has a SYS_CTRL register and
STMPE1601/2403 has even a SYS_CTRL2 register, add
STMPE_IDX_SYS_CTRL/2 and update driver code accordingly

This update prepares the ground for not yet supported STMPE1600
which share similar REG_SYS_CTRL register.

Signed-off-by: Patrice Chotard <[email protected]>
---
drivers/mfd/stmpe.c | 21 ++++++++++++++-------
drivers/mfd/stmpe.h | 2 ++
include/linux/mfd/stmpe.h | 2 ++
3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index fb8f9e8..c553b73 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -448,6 +448,8 @@ static const struct mfd_cell stmpe_ts_cell = {

static const u8 stmpe811_regs[] = {
[STMPE_IDX_CHIP_ID] = STMPE811_REG_CHIP_ID,
+ [STMPE_IDX_SYS_CTRL] = STMPE811_REG_SYS_CTRL,
+ [STMPE_IDX_SYS_CTRL2] = STMPE811_REG_SYS_CTRL2,
[STMPE_IDX_ICR_LSB] = STMPE811_REG_INT_CTRL,
[STMPE_IDX_IER_LSB] = STMPE811_REG_INT_EN,
[STMPE_IDX_ISR_MSB] = STMPE811_REG_INT_STA,
@@ -490,7 +492,7 @@ static int stmpe811_enable(struct stmpe *stmpe, unsigned int blocks,
if (blocks & STMPE_BLOCK_TOUCHSCREEN)
mask |= STMPE811_SYS_CTRL2_TSC_OFF;

- return __stmpe_set_bits(stmpe, STMPE811_REG_SYS_CTRL2, mask,
+ return __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL2], mask,
enable ? 0 : mask);
}

@@ -535,6 +537,8 @@ static struct stmpe_variant_info stmpe610 = {

static const u8 stmpe1601_regs[] = {
[STMPE_IDX_CHIP_ID] = STMPE1601_REG_CHIP_ID,
+ [STMPE_IDX_SYS_CTRL] = STMPE1601_REG_SYS_CTRL,
+ [STMPE_IDX_SYS_CTRL2] = STMPE1601_REG_SYS_CTRL2,
[STMPE_IDX_ICR_LSB] = STMPE1601_REG_ICR_LSB,
[STMPE_IDX_IER_LSB] = STMPE1601_REG_IER_LSB,
[STMPE_IDX_ISR_MSB] = STMPE1601_REG_ISR_MSB,
@@ -619,13 +623,13 @@ static int stmpe1601_autosleep(struct stmpe *stmpe,
return timeout;
}

- ret = __stmpe_set_bits(stmpe, STMPE1601_REG_SYS_CTRL2,
+ ret = __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL2],
STMPE1601_AUTOSLEEP_TIMEOUT_MASK,
timeout);
if (ret < 0)
return ret;

- return __stmpe_set_bits(stmpe, STMPE1601_REG_SYS_CTRL2,
+ return __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL2],
STPME1601_AUTOSLEEP_ENABLE,
STPME1601_AUTOSLEEP_ENABLE);
}
@@ -650,7 +654,7 @@ static int stmpe1601_enable(struct stmpe *stmpe, unsigned int blocks,
else
mask &= ~STMPE1601_SYS_CTRL_ENABLE_SPWM;

- return __stmpe_set_bits(stmpe, STMPE1601_REG_SYS_CTRL, mask,
+ return __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL], mask,
enable ? mask : 0);
}

@@ -689,6 +693,7 @@ static struct stmpe_variant_info stmpe1601 = {
*/
static const u8 stmpe1801_regs[] = {
[STMPE_IDX_CHIP_ID] = STMPE1801_REG_CHIP_ID,
+ [STMPE_IDX_SYS_CTRL] = STMPE1801_REG_SYS_CTRL,
[STMPE_IDX_ICR_LSB] = STMPE1801_REG_INT_CTRL_LOW,
[STMPE_IDX_IER_LSB] = STMPE1801_REG_INT_EN_MASK_LOW,
[STMPE_IDX_ISR_LSB] = STMPE1801_REG_INT_STA_LOW,
@@ -735,14 +740,14 @@ static int stmpe1801_reset(struct stmpe *stmpe)
unsigned long timeout;
int ret = 0;

- ret = __stmpe_set_bits(stmpe, STMPE1801_REG_SYS_CTRL,
+ ret = __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL],
STMPE1801_MSK_SYS_CTRL_RESET, STMPE1801_MSK_SYS_CTRL_RESET);
if (ret < 0)
return ret;

timeout = jiffies + msecs_to_jiffies(100);
while (time_before(jiffies, timeout)) {
- ret = __stmpe_reg_read(stmpe, STMPE1801_REG_SYS_CTRL);
+ ret = __stmpe_reg_read(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL]);
if (ret < 0)
return ret;
if (!(ret & STMPE1801_MSK_SYS_CTRL_RESET))
@@ -773,6 +778,8 @@ static struct stmpe_variant_info stmpe1801 = {

static const u8 stmpe24xx_regs[] = {
[STMPE_IDX_CHIP_ID] = STMPE24XX_REG_CHIP_ID,
+ [STMPE_IDX_SYS_CTRL] = STMPE24XX_REG_SYS_CTRL,
+ [STMPE_IDX_SYS_CTRL2] = STMPE24XX_REG_SYS_CTRL2,
[STMPE_IDX_ICR_LSB] = STMPE24XX_REG_ICR_LSB,
[STMPE_IDX_IER_LSB] = STMPE24XX_REG_IER_LSB,
[STMPE_IDX_ISR_MSB] = STMPE24XX_REG_ISR_MSB,
@@ -819,7 +826,7 @@ static int stmpe24xx_enable(struct stmpe *stmpe, unsigned int blocks,
if (blocks & STMPE_BLOCK_KEYPAD)
mask |= STMPE24XX_SYS_CTRL_ENABLE_KPC;

- return __stmpe_set_bits(stmpe, STMPE24XX_REG_SYS_CTRL, mask,
+ return __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL], mask,
enable ? mask : 0);
}

diff --git a/drivers/mfd/stmpe.h b/drivers/mfd/stmpe.h
index 84adb46..406f9f2 100644
--- a/drivers/mfd/stmpe.h
+++ b/drivers/mfd/stmpe.h
@@ -138,6 +138,7 @@ int stmpe_remove(struct stmpe *stmpe);
#define STMPE811_NR_INTERNAL_IRQS 8

#define STMPE811_REG_CHIP_ID 0x00
+#define STMPE811_REG_SYS_CTRL 0x03
#define STMPE811_REG_SYS_CTRL2 0x04
#define STMPE811_REG_SPI_CFG 0x08
#define STMPE811_REG_INT_CTRL 0x09
@@ -264,6 +265,7 @@ int stmpe_remove(struct stmpe *stmpe);
#define STMPE24XX_NR_INTERNAL_IRQS 9

#define STMPE24XX_REG_SYS_CTRL 0x02
+#define STMPE24XX_REG_SYS_CTRL2 0x03
#define STMPE24XX_REG_ICR_LSB 0x11
#define STMPE24XX_REG_IER_LSB 0x13
#define STMPE24XX_REG_ISR_MSB 0x14
diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h
index cb83883..6c6690f 100644
--- a/include/linux/mfd/stmpe.h
+++ b/include/linux/mfd/stmpe.h
@@ -39,6 +39,8 @@ enum stmpe_partnum {
*/
enum {
STMPE_IDX_CHIP_ID,
+ STMPE_IDX_SYS_CTRL,
+ STMPE_IDX_SYS_CTRL2,
STMPE_IDX_ICR_LSB,
STMPE_IDX_IER_LSB,
STMPE_IDX_ISR_LSB,
--
1.9.1

2016-04-19 12:19:34

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 6/8] mfd: Add STMPE1600 support

From: Patrice Chotard <[email protected]>

STMPE1600 is a 16-bit port expander.
Datasheet is available here :
http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html

As STMPE1600's SYS_CTRL register has the same layout as
STMPE801 variant, unify STMPExxx_REG_SYS_CTRL_RESET/INT_EN/INT_HI
bit masks to more generic STMPE_SYS_CTRL_RESET/INT_EN/INT_HI

Signed-off-by: Amelie DELAUNAY <[email protected]>
Signed-off-by: Patrice Chotard <[email protected]>
---
drivers/mfd/stmpe-i2c.c | 2 ++
drivers/mfd/stmpe.c | 61 +++++++++++++++++++++++++++++++++++++++++------
drivers/mfd/stmpe.h | 21 ++++++++++++----
include/linux/mfd/stmpe.h | 1 +
4 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/stmpe-i2c.c b/drivers/mfd/stmpe-i2c.c
index c3f4aab..863c39a 100644
--- a/drivers/mfd/stmpe-i2c.c
+++ b/drivers/mfd/stmpe-i2c.c
@@ -57,6 +57,7 @@ static const struct of_device_id stmpe_of_match[] = {
{ .compatible = "st,stmpe610", .data = (void *)STMPE610, },
{ .compatible = "st,stmpe801", .data = (void *)STMPE801, },
{ .compatible = "st,stmpe811", .data = (void *)STMPE811, },
+ { .compatible = "st,stmpe1600", .data = (void *)STMPE1600, },
{ .compatible = "st,stmpe1601", .data = (void *)STMPE1601, },
{ .compatible = "st,stmpe1801", .data = (void *)STMPE1801, },
{ .compatible = "st,stmpe2401", .data = (void *)STMPE2401, },
@@ -101,6 +102,7 @@ static const struct i2c_device_id stmpe_i2c_id[] = {
{ "stmpe610", STMPE610 },
{ "stmpe801", STMPE801 },
{ "stmpe811", STMPE811 },
+ { "stmpe1600", STMPE1600 },
{ "stmpe1601", STMPE1601 },
{ "stmpe1801", STMPE1801 },
{ "stmpe2401", STMPE2401 },
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index af682d0..53f61b9 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -532,6 +532,51 @@ static struct stmpe_variant_info stmpe610 = {
};

/*
+ * STMPE1600
+ */
+
+static const u8 stmpe1600_regs[] = {
+ [STMPE_IDX_CHIP_ID] = STMPE1600_REG_CHIP_ID,
+ [STMPE_IDX_SYS_CTRL] = STMPE1600_REG_SYS_CTRL,
+ [STMPE_IDX_ICR_LSB] = STMPE1600_REG_SYS_CTRL,
+ [STMPE_IDX_GPMR_LSB] = STMPE1600_REG_GPMR,
+ [STMPE_IDX_GPSR_LSB] = STMPE1600_REG_GPSR,
+ [STMPE_IDX_GPDR_LSB] = STMPE1600_REG_GPDR,
+ [STMPE_IDX_IEGPIOR_LSB] = STMPE1600_REG_IEGPIOR,
+ [STMPE_IDX_ISGPIOR_LSB] = STMPE1600_REG_ISGPIOR,
+};
+
+static struct stmpe_variant_block stmpe1600_blocks[] = {
+ {
+ .cell = &stmpe_gpio_cell,
+ .irq = 0,
+ .block = STMPE_BLOCK_GPIO,
+ },
+};
+
+static int stmpe1600_enable(struct stmpe *stmpe, unsigned int blocks,
+ bool enable)
+{
+ if (blocks & STMPE_BLOCK_GPIO)
+ return 0;
+ else
+ return -EINVAL;
+}
+
+static struct stmpe_variant_info stmpe1600 = {
+ .name = "stmpe1600",
+ .id_val = STMPE1600_ID,
+ .id_mask = 0xffff,
+ .num_gpios = 16,
+ .af_bits = 0,
+ .regs = stmpe1600_regs,
+ .blocks = stmpe1600_blocks,
+ .num_blocks = ARRAY_SIZE(stmpe1600_blocks),
+ .num_irqs = STMPE1600_NR_INTERNAL_IRQS,
+ .enable = stmpe1600_enable,
+};
+
+/*
* STMPE1601
*/

@@ -888,6 +933,7 @@ static struct stmpe_variant_info *stmpe_variant_info[STMPE_NBR_PARTS] = {
[STMPE610] = &stmpe610,
[STMPE801] = &stmpe801,
[STMPE811] = &stmpe811,
+ [STMPE1600] = &stmpe1600,
[STMPE1601] = &stmpe1601,
[STMPE1801] = &stmpe1801,
[STMPE2401] = &stmpe2401,
@@ -914,7 +960,8 @@ static irqreturn_t stmpe_irq(int irq, void *data)
int ret;
int i;

- if (variant->id_val == STMPE801_ID) {
+ if (variant->id_val == STMPE801_ID ||
+ variant->id_val == STMPE1600_ID) {
int base = irq_create_mapping(stmpe->domain, 0);

handle_nested_irq(base);
@@ -1088,13 +1135,13 @@ static int stmpe_chip_init(struct stmpe *stmpe)
return ret;

if (stmpe->irq >= 0) {
- if (id == STMPE801_ID)
- icr = STMPE801_REG_SYS_CTRL_INT_EN;
+ if (id == STMPE801_ID || id == STMPE1600_ID)
+ icr = STMPE_SYS_CTRL_INT_EN;
else
icr = STMPE_ICR_LSB_GIM;

- /* STMPE801 doesn't support Edge interrupts */
- if (id != STMPE801_ID) {
+ /* STMPE801 and STMPE1600 don't support Edge interrupts */
+ if (id != STMPE801_ID && id != STMPE1600_ID) {
if (irq_trigger == IRQF_TRIGGER_FALLING ||
irq_trigger == IRQF_TRIGGER_RISING)
icr |= STMPE_ICR_LSB_EDGE;
@@ -1102,8 +1149,8 @@ static int stmpe_chip_init(struct stmpe *stmpe)

if (irq_trigger == IRQF_TRIGGER_RISING ||
irq_trigger == IRQF_TRIGGER_HIGH) {
- if (id == STMPE801_ID)
- icr |= STMPE801_REG_SYS_CTRL_INT_HI;
+ if (id == STMPE801_ID || id == STMPE1600_ID)
+ icr |= STMPE_SYS_CTRL_INT_HI;
else
icr |= STMPE_ICR_LSB_HIGH;
}
diff --git a/drivers/mfd/stmpe.h b/drivers/mfd/stmpe.h
index 4ae343d..c829caa 100644
--- a/drivers/mfd/stmpe.h
+++ b/drivers/mfd/stmpe.h
@@ -105,6 +105,8 @@ int stmpe_remove(struct stmpe *stmpe);
#define STMPE_ICR_LSB_GIM (1 << 0)

#define STMPE_SYS_CTRL_RESET (1 << 7)
+#define STMPE_SYS_CTRL_INT_EN (1 << 2)
+#define STMPE_SYS_CTRL_INT_HI (1 << 0)

/*
* STMPE801
@@ -121,10 +123,6 @@ int stmpe_remove(struct stmpe *stmpe);
#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
*/
@@ -166,6 +164,21 @@ int stmpe_remove(struct stmpe *stmpe);
#define STMPE811_SYS_CTRL2_TS_OFF (1 << 3)

/*
+ * STMPE1600
+ */
+#define STMPE1600_ID 0x0016
+#define STMPE1600_NR_INTERNAL_IRQS 16
+
+#define STMPE1600_REG_CHIP_ID 0x00
+#define STMPE1600_REG_SYS_CTRL 0x03
+#define STMPE1600_REG_IEGPIOR 0x08
+#define STMPE1600_REG_ISGPIOR 0x0A
+#define STMPE1600_REG_GPMR 0x10
+#define STMPE1600_REG_GPSR 0x12
+#define STMPE1600_REG_GPDR 0x14
+#define STMPE1600_REG_GPPIR 0x16
+
+/*
* STMPE1601
*/

diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h
index 6c6690f..2d94ac7 100644
--- a/include/linux/mfd/stmpe.h
+++ b/include/linux/mfd/stmpe.h
@@ -26,6 +26,7 @@ enum stmpe_partnum {
STMPE610,
STMPE801,
STMPE811,
+ STMPE1600,
STMPE1601,
STMPE1801,
STMPE2401,
--
1.9.1

2016-04-19 12:19:30

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 2/8] mfd: stmpe: Add reset support for all STMPE variant

From: Patrice Chotard <[email protected]>

Reset was only implemented for STMPE1801 variant despite
all variant have a SOFT_RESET bit.

For STMPE2401/2403/801/1601/1801 SOFT_RESET bit is bit 7
of SYS_CTRL register.
For STMPE610/811 (which have the same variant id) SOFT_RESET
bit is bit 1 of SYS_CTRL register.

Signed-off-by: Patrice Chotard <[email protected]>
---
drivers/mfd/stmpe.c | 23 +++++++++++++++--------
drivers/mfd/stmpe.h | 7 +++++--
2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index c553b73..af682d0 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -735,13 +735,22 @@ static int stmpe1801_enable(struct stmpe *stmpe, unsigned int blocks,
enable ? mask : 0);
}

-static int stmpe1801_reset(struct stmpe *stmpe)
+static int stmpe_reset(struct stmpe *stmpe)
{
+ u16 id_val = stmpe->variant->id_val;
unsigned long timeout;
int ret = 0;
+ u8 reset_bit;
+
+ if (id_val == STMPE811_ID)
+ /* STMPE801 and STMPE610 use bit 1 of SYS_CTRL register */
+ reset_bit = STMPE811_SYS_CTRL_RESET;
+ else
+ /* all other STMPE variant use bit 7 of SYS_CTRL register */
+ reset_bit = STMPE_SYS_CTRL_RESET;

ret = __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL],
- STMPE1801_MSK_SYS_CTRL_RESET, STMPE1801_MSK_SYS_CTRL_RESET);
+ reset_bit, reset_bit);
if (ret < 0)
return ret;

@@ -750,7 +759,7 @@ static int stmpe1801_reset(struct stmpe *stmpe)
ret = __stmpe_reg_read(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL]);
if (ret < 0)
return ret;
- if (!(ret & STMPE1801_MSK_SYS_CTRL_RESET))
+ if (!(ret & reset_bit))
return 0;
usleep_range(100, 200);
}
@@ -1074,11 +1083,9 @@ static int stmpe_chip_init(struct stmpe *stmpe)
if (ret)
return ret;

- if (id == STMPE1801_ID) {
- ret = stmpe1801_reset(stmpe);
- if (ret < 0)
- return ret;
- }
+ ret = stmpe_reset(stmpe);
+ if (ret < 0)
+ return ret;

if (stmpe->irq >= 0) {
if (id == STMPE801_ID)
diff --git a/drivers/mfd/stmpe.h b/drivers/mfd/stmpe.h
index 406f9f2..4ae343d 100644
--- a/drivers/mfd/stmpe.h
+++ b/drivers/mfd/stmpe.h
@@ -104,6 +104,8 @@ int stmpe_remove(struct stmpe *stmpe);
#define STMPE_ICR_LSB_EDGE (1 << 1)
#define STMPE_ICR_LSB_GIM (1 << 0)

+#define STMPE_SYS_CTRL_RESET (1 << 7)
+
/*
* STMPE801
*/
@@ -126,6 +128,7 @@ int stmpe_remove(struct stmpe *stmpe);
/*
* STMPE811
*/
+#define STMPE811_ID 0x0811

#define STMPE811_IRQ_TOUCH_DET 0
#define STMPE811_IRQ_FIFO_TH 1
@@ -155,6 +158,8 @@ int stmpe_remove(struct stmpe *stmpe);
#define STMPE811_REG_GPIO_FE 0x16
#define STMPE811_REG_GPIO_AF 0x17

+#define STMPE811_SYS_CTRL_RESET (1 << 1)
+
#define STMPE811_SYS_CTRL2_ADC_OFF (1 << 0)
#define STMPE811_SYS_CTRL2_TSC_OFF (1 << 1)
#define STMPE811_SYS_CTRL2_GPIO_OFF (1 << 2)
@@ -244,8 +249,6 @@ int stmpe_remove(struct stmpe *stmpe);
#define STMPE1801_REG_GPIO_PULL_UP_MID 0x23
#define STMPE1801_REG_GPIO_PULL_UP_HIGH 0x24

-#define STMPE1801_MSK_SYS_CTRL_RESET (1 << 7)
-
#define STMPE1801_MSK_INT_EN_KPC (1 << 1)
#define STMPE1801_MSK_INT_EN_GPIO (1 << 3)

--
1.9.1

2016-04-19 12:19:28

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge detection

From: Patrice Chotard <[email protected]>

By cross-checking STMPE 610/801/811/1601/2401/2403 datasheets,
it appears that edge detection and rising/falling edge detection
is not supported by all STMPE variant:

GPIO GPIO
Edge detection rising/falling
edge detection
610 | X | X |
801 | | |
811 | X | X |
1600 | | |
1601 | X | X |
1801 | | X |
2401 | X | X |
2403 | X | X |

Rework stmpe_dbg_show_one() and stmpe_gpio_irq to correctly
take these cases into account.

Signed-off-by: Patrice Chotard <[email protected]>
---
drivers/gpio/gpio-stmpe.c | 58 +++++++++++++++++++++++++++++++++--------------
1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index 5197edf..225e075 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -231,27 +231,51 @@ static void stmpe_dbg_show_one(struct seq_file *s,
gpio, label ?: "(none)",
val ? "hi" : "lo");
} else {
- u8 edge_det_reg = stmpe->regs[STMPE_IDX_GPEDR_MSB] + num_banks - 1 - (offset / 8);
- u8 rise_reg = stmpe->regs[STMPE_IDX_GPRER_LSB] - (offset / 8);
- u8 fall_reg = stmpe->regs[STMPE_IDX_GPFER_LSB] - (offset / 8);
- u8 irqen_reg = stmpe->regs[STMPE_IDX_IEGPIOR_LSB] - (offset / 8);
+ u8 edge_det_reg;
+ u8 rise_reg;
+ u8 fall_reg;
+ u8 irqen_reg;
bool edge_det;
bool rise;
bool fall;
bool irqen;

- ret = stmpe_reg_read(stmpe, edge_det_reg);
- if (ret < 0)
- return;
- edge_det = !!(ret & mask);
- ret = stmpe_reg_read(stmpe, rise_reg);
- if (ret < 0)
+ switch (stmpe->partnum) {
+ case STMPE610:
+ case STMPE811:
+ case STMPE1601:
+ case STMPE2401:
+ case STMPE2403:
+ edge_det_reg = stmpe->regs[STMPE_IDX_GPEDR_MSB] +
+ num_banks - 1 - (offset / 8);
+ ret = stmpe_reg_read(stmpe, edge_det_reg);
+ if (ret < 0)
+ return;
+ edge_det = !!(ret & mask);
+
+ case STMPE1801:
+ rise_reg = stmpe->regs[STMPE_IDX_GPRER_LSB] -
+ (offset / 8);
+ fall_reg = stmpe->regs[STMPE_IDX_GPFER_LSB] -
+ (offset / 8);
+ ret = stmpe_reg_read(stmpe, rise_reg);
+ if (ret < 0)
+ return;
+ rise = !!(ret & mask);
+ ret = stmpe_reg_read(stmpe, fall_reg);
+ if (ret < 0)
+ return;
+ fall = !!(ret & mask);
+
+ case STMPE801:
+ irqen_reg = stmpe->regs[STMPE_IDX_IEGPIOR_LSB] -
+ (offset / 8);
+ break;
+
+ default:
return;
- rise = !!(ret & mask);
- ret = stmpe_reg_read(stmpe, fall_reg);
- if (ret < 0)
- return;
- fall = !!(ret & mask);
+ }
+
ret = stmpe_reg_read(stmpe, irqen_reg);
if (ret < 0)
return;
@@ -322,8 +346,8 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)

stmpe_reg_write(stmpe, statmsbreg + i, status[i]);

- /* Edge detect register is not present on 801 */
- if (stmpe->partnum != STMPE801)
+ /* Edge detect register is not present on 801 and 1801 */
+ if (stmpe->partnum != STMPE801 || stmpe->partnum != STMPE1801)
stmpe_reg_write(stmpe, stmpe->regs[STMPE_IDX_GPEDR_MSB]
+ i, status[i]);
}
--
1.9.1

2016-04-19 12:22:22

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support

From: Patrice Chotard <[email protected]>

The particularities of this variant are:
- GPIO_XXX_LSB and GPIO_XXX_MSB memory locations are inverted compared
to other variants.
- There is no Edge detection, Rising Edge and Falling Edge registers.
- IRQ flags are cleared when read, no need to write in Status register.

Signed-off-by: Amelie DELAUNAY <[email protected]>
Signed-off-by: Patrice Chotard <[email protected]>
---
drivers/gpio/gpio-stmpe.c | 74 ++++++++++++++++++++++++++++++++++++++---------
1 file changed, 60 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8c9d22..45e5b92 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -40,10 +40,15 @@ static int stmpe_gpio_get(struct gpio_chip *chip, unsigned offset)
{
struct stmpe_gpio *stmpe_gpio = gpiochip_get_data(chip);
struct stmpe *stmpe = stmpe_gpio->stmpe;
- u8 reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8);
+ u8 reg;
u8 mask = 1 << (offset % 8);
int ret;

+ if (stmpe->partnum == STMPE1600)
+ reg = stmpe->regs[STMPE_IDX_GPMR_LSB] + (offset / 8);
+ else
+ reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8);
+
ret = stmpe_reg_read(stmpe, reg);
if (ret < 0)
return ret;
@@ -56,9 +61,14 @@ static void stmpe_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
struct stmpe_gpio *stmpe_gpio = gpiochip_get_data(chip);
struct stmpe *stmpe = stmpe_gpio->stmpe;
int which = val ? STMPE_IDX_GPSR_LSB : STMPE_IDX_GPCR_LSB;
- u8 reg = stmpe->regs[which] - (offset / 8);
+ u8 reg;
u8 mask = 1 << (offset % 8);

+ if (stmpe->partnum == STMPE1600)
+ reg = stmpe->regs[which] + (offset / 8);
+ else
+ reg = stmpe->regs[which] - (offset / 8);
+
/*
* Some variants have single register for gpio set/clear functionality.
* For them we need to write 0 to clear and 1 to set.
@@ -74,9 +84,14 @@ static int stmpe_gpio_direction_output(struct gpio_chip *chip,
{
struct stmpe_gpio *stmpe_gpio = gpiochip_get_data(chip);
struct stmpe *stmpe = stmpe_gpio->stmpe;
- u8 reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
+ u8 reg;
u8 mask = 1 << (offset % 8);

+ if (stmpe->partnum == STMPE1600)
+ reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
+ else
+ reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
+
stmpe_gpio_set(chip, offset, val);

return stmpe_set_bits(stmpe, reg, mask, mask);
@@ -87,9 +102,14 @@ static int stmpe_gpio_direction_input(struct gpio_chip *chip,
{
struct stmpe_gpio *stmpe_gpio = gpiochip_get_data(chip);
struct stmpe *stmpe = stmpe_gpio->stmpe;
- u8 reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
+ u8 reg;
u8 mask = 1 << (offset % 8);

+ if (stmpe->partnum == STMPE1600)
+ reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
+ else
+ reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
+
return stmpe_set_bits(stmpe, reg, mask, 0);
}

@@ -126,8 +146,9 @@ 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)
+ /* STMPE801 and STMPE 1600 don't have RE and FE registers */
+ if (stmpe_gpio->stmpe->partnum == STMPE801 ||
+ stmpe_gpio->stmpe->partnum == STMPE1600)
return 0;

if (type & IRQ_TYPE_EDGE_RISING)
@@ -165,9 +186,10 @@ 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))
+ /* STMPE801 and STMPE1600 don't have RE and FE registers */
+ if ((stmpe->partnum == STMPE801 ||
+ stmpe->partnum == STMPE1600) &&
+ (i != REG_IE))
continue;

for (j = 0; j < num_banks; j++) {
@@ -178,7 +200,14 @@ static void stmpe_gpio_irq_sync_unlock(struct irq_data *d)
continue;

stmpe_gpio->oldregs[i][j] = new;
- stmpe_reg_write(stmpe, stmpe->regs[regmap[i]] - j, new);
+ if (stmpe->partnum == STMPE1600)
+ stmpe_reg_write(stmpe,
+ stmpe->regs[regmap[i]] + j,
+ new);
+ else
+ stmpe_reg_write(stmpe,
+ stmpe->regs[regmap[i]] - j,
+ new);
}
}

@@ -216,11 +245,16 @@ static void stmpe_dbg_show_one(struct seq_file *s,
const char *label = gpiochip_is_requested(gc, offset);
int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
bool val = !!stmpe_gpio_get(gc, offset);
- u8 dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
+ u8 dir_reg;
u8 mask = 1 << (offset % 8);
int ret;
u8 dir;

+ if (stmpe->partnum == STMPE1600)
+ dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
+ else
+ dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
+
ret = stmpe_reg_read(stmpe, dir_reg);
if (ret < 0)
return;
@@ -272,6 +306,11 @@ static void stmpe_dbg_show_one(struct seq_file *s,
(offset / 8);
break;

+ case STMPE1600:
+ irqen_reg = stmpe->regs[STMPE_IDX_IEGPIOR_LSB] +
+ (offset / 8);
+ break;
+
default:
return;
}
@@ -321,12 +360,18 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
int ret;
int i;

+ if (stmpe->partnum == STMPE1600)
+ statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_LSB];
+ else
+ statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_MSB];
+
ret = stmpe_block_read(stmpe, statmsbreg, num_banks, status);
if (ret < 0)
return IRQ_NONE;

for (i = 0; i < num_banks; i++) {
- int bank = num_banks - i - 1;
+ int bank = (stmpe_gpio->stmpe->partnum == STMPE1600) ? i :
+ num_banks - i - 1;
unsigned int enabled = stmpe_gpio->regs[REG_IE][bank];
unsigned int stat = status[i];

@@ -346,10 +391,11 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)

/*
* interrupt status register write has no effect on
- * 801 and 1801, bits are cleared when read.
+ * 801/1801/1600, bits are cleared when read.
* Edge detect register is not present on 801 and 1801
*/
- if (stmpe->partnum != STMPE801 || stmpe->partnum != STMPE1801) {
+ if (stmpe->partnum != STMPE801 || stmpe->partnum != STMPE1600 ||
+ stmpe->partnum != STMPE1801) {
stmpe_reg_write(stmpe, statmsbreg + i, status[i]);
stmpe_reg_write(stmpe, stmpe->regs[STMPE_IDX_GPEDR_MSB]
+ i, status[i]);
--
1.9.1

2016-04-19 12:22:21

by Patrice CHOTARD

[permalink] [raw]
Subject: [PATCH 8/8] gpio: stmpe: configure GPIO as output by default

From: Patrice Chotard <[email protected]>

Configures all GPIOs as output, in order to minimize power
consumption when GPIOs are unused.

Signed-off-by: Amelie DELAUNAY <[email protected]>
Signed-off-by: Patrice Chotard <[email protected]>
---
drivers/gpio/gpio-stmpe.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index 45e5b92..80c6ae6 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -410,7 +410,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev)
struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
struct device_node *np = pdev->dev.of_node;
struct stmpe_gpio *stmpe_gpio;
- int ret;
+ int ret, i;
int irq = 0;

irq = platform_get_irq(pdev, 0);
@@ -475,6 +475,10 @@ static int stmpe_gpio_probe(struct platform_device *pdev)
NULL);
}

+ /* To minimize power consumption, configure unused GPIOs as outputs */
+ for (i = 0; i < stmpe_gpio->chip.ngpio; i++)
+ stmpe_gpio_direction_output(&stmpe_gpio->chip, i, 0);
+
platform_set_drvdata(pdev, stmpe_gpio);

return 0;
--
1.9.1

2016-04-19 12:39:53

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge detection

Hi,

[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on v4.6-rc4 next-20160419]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url: https://github.com/0day-ci/linux/commits/patrice-chotard-st-com/STMPE-fixes-rework-and-add-STMPE1600-support/20160419-202526
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: x86_64-randconfig-x010-201616 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

drivers/gpio/gpio-stmpe.c: In function 'stmpe_dbg_show':
>> drivers/gpio/gpio-stmpe.c:284:3: warning: 'fall' may be used uninitialized in this function [-Wmaybe-uninitialized]
seq_printf(s, " gpio-%-3d (%-20.20s) in %s %s %s%s%s",
^
drivers/gpio/gpio-stmpe.c:240:8: note: 'fall' was declared here
bool fall;
^
>> drivers/gpio/gpio-stmpe.c:284:3: warning: 'rise' may be used uninitialized in this function [-Wmaybe-uninitialized]
seq_printf(s, " gpio-%-3d (%-20.20s) in %s %s %s%s%s",
^
drivers/gpio/gpio-stmpe.c:239:8: note: 'rise' was declared here
bool rise;
^
>> drivers/gpio/gpio-stmpe.c:284:3: warning: 'edge_det' may be used uninitialized in this function [-Wmaybe-uninitialized]
seq_printf(s, " gpio-%-3d (%-20.20s) in %s %s %s%s%s",
^
drivers/gpio/gpio-stmpe.c:238:8: note: 'edge_det' was declared here
bool edge_det;
^

vim +/fall +284 drivers/gpio/gpio-stmpe.c

98190e59 Patrice Chotard 2016-04-19 234 u8 edge_det_reg;
98190e59 Patrice Chotard 2016-04-19 235 u8 rise_reg;
98190e59 Patrice Chotard 2016-04-19 236 u8 fall_reg;
98190e59 Patrice Chotard 2016-04-19 237 u8 irqen_reg;
27ec8a9c Linus Walleij 2014-10-02 238 bool edge_det;
27ec8a9c Linus Walleij 2014-10-02 239 bool rise;
27ec8a9c Linus Walleij 2014-10-02 @240 bool fall;
27ec8a9c Linus Walleij 2014-10-02 241 bool irqen;
27ec8a9c Linus Walleij 2014-10-02 242
98190e59 Patrice Chotard 2016-04-19 243 switch (stmpe->partnum) {
98190e59 Patrice Chotard 2016-04-19 244 case STMPE610:
98190e59 Patrice Chotard 2016-04-19 245 case STMPE811:
98190e59 Patrice Chotard 2016-04-19 246 case STMPE1601:
98190e59 Patrice Chotard 2016-04-19 247 case STMPE2401:
98190e59 Patrice Chotard 2016-04-19 248 case STMPE2403:
98190e59 Patrice Chotard 2016-04-19 249 edge_det_reg = stmpe->regs[STMPE_IDX_GPEDR_MSB] +
98190e59 Patrice Chotard 2016-04-19 250 num_banks - 1 - (offset / 8);
27ec8a9c Linus Walleij 2014-10-02 251 ret = stmpe_reg_read(stmpe, edge_det_reg);
27ec8a9c Linus Walleij 2014-10-02 252 if (ret < 0)
27ec8a9c Linus Walleij 2014-10-02 253 return;
27ec8a9c Linus Walleij 2014-10-02 254 edge_det = !!(ret & mask);
98190e59 Patrice Chotard 2016-04-19 255
98190e59 Patrice Chotard 2016-04-19 256 case STMPE1801:
98190e59 Patrice Chotard 2016-04-19 257 rise_reg = stmpe->regs[STMPE_IDX_GPRER_LSB] -
98190e59 Patrice Chotard 2016-04-19 258 (offset / 8);
98190e59 Patrice Chotard 2016-04-19 259 fall_reg = stmpe->regs[STMPE_IDX_GPFER_LSB] -
98190e59 Patrice Chotard 2016-04-19 260 (offset / 8);
27ec8a9c Linus Walleij 2014-10-02 261 ret = stmpe_reg_read(stmpe, rise_reg);
27ec8a9c Linus Walleij 2014-10-02 262 if (ret < 0)
27ec8a9c Linus Walleij 2014-10-02 263 return;
27ec8a9c Linus Walleij 2014-10-02 264 rise = !!(ret & mask);
27ec8a9c Linus Walleij 2014-10-02 265 ret = stmpe_reg_read(stmpe, fall_reg);
27ec8a9c Linus Walleij 2014-10-02 266 if (ret < 0)
27ec8a9c Linus Walleij 2014-10-02 267 return;
27ec8a9c Linus Walleij 2014-10-02 268 fall = !!(ret & mask);
98190e59 Patrice Chotard 2016-04-19 269
98190e59 Patrice Chotard 2016-04-19 270 case STMPE801:
98190e59 Patrice Chotard 2016-04-19 271 irqen_reg = stmpe->regs[STMPE_IDX_IEGPIOR_LSB] -
98190e59 Patrice Chotard 2016-04-19 272 (offset / 8);
98190e59 Patrice Chotard 2016-04-19 273 break;
98190e59 Patrice Chotard 2016-04-19 274
98190e59 Patrice Chotard 2016-04-19 275 default:
98190e59 Patrice Chotard 2016-04-19 276 return;
98190e59 Patrice Chotard 2016-04-19 277 }
98190e59 Patrice Chotard 2016-04-19 278
27ec8a9c Linus Walleij 2014-10-02 279 ret = stmpe_reg_read(stmpe, irqen_reg);
27ec8a9c Linus Walleij 2014-10-02 280 if (ret < 0)
27ec8a9c Linus Walleij 2014-10-02 281 return;
27ec8a9c Linus Walleij 2014-10-02 282 irqen = !!(ret & mask);
27ec8a9c Linus Walleij 2014-10-02 283
27ec8a9c Linus Walleij 2014-10-02 @284 seq_printf(s, " gpio-%-3d (%-20.20s) in %s %s %s%s%s",
27ec8a9c Linus Walleij 2014-10-02 285 gpio, label ?: "(none)",
27ec8a9c Linus Walleij 2014-10-02 286 val ? "hi" : "lo",
27ec8a9c Linus Walleij 2014-10-02 287 edge_det ? "edge-asserted" : "edge-inactive",

:::::: The code at line 284 was first introduced by commit
:::::: 27ec8a9cb504e9995c123dc74e0cca0cba81d07f gpio: stmpe: add verbose debug code

:::::: TO: Linus Walleij <[email protected]>
:::::: CC: Linus Walleij <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation


Attachments:
(No filename) (5.45 kB)
.config.gz (23.17 kB)
Download all attachments

2016-04-19 12:41:32

by Thierry Reding

[permalink] [raw]
Subject: Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support

On Tue, Apr 19, 2016 at 02:18:29PM +0200, [email protected] wrote:
> From: Patrice Chotard <[email protected]>
>
> This series cleans and fixes some bugs in MFD/GPIO STMPE drivers and prepare
> the ground to add new STMPE1600 support.
>
> STMPE1600 datasheet is available here :
> http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
> i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html
>
> Only STMPE1600 has been tested on STM32 platform. As i have no board with
> others STMPE variant(STMPE610/STMPE801/STMPE811/STMPE1601/STMPE1801/STMPE2401
> and STMPE2403), i put in CC boards's maintainers which are using others STMPE variant.
>
> If they can kindly check that no regression has been introduce by this series
> :
>
> For ARM/FREESCALE IMX / MXC ARM ARCHITECTURE:
> _ Shawn Guo <[email protected]>
> _ Sascha Hauer <[email protected]>
>
> For ARM/SOCFPGA ARCHITECTURE
> _ Dinh Nguyen <[email protected]>
>
> For SPEAR PLATFORM SUPPORT
> _ Viresh Kumar <[email protected]>
> _ Shiraz Hashim <[email protected]>
>
> For TEGRA ARCHITECTURE SUPPORT
> _ Stephen Warren <[email protected]>
> _ Thierry Reding <[email protected]>
> _ Alexandre Courbot <[email protected]>

Adding Marcel Ziswiler, who's better suited at judging whether or not
this has any impact on Apalis/Colibri.

Marcel, in case you don't have these in your inbox you can find them on
linux-gpio's patchwork:

https://patchwork.ozlabs.org/project/linux-gpio/list/

Thierry


Attachments:
(No filename) (1.51 kB)
signature.asc (819.00 B)
Download all attachments

2016-04-19 15:53:19

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support

On 04/19/2016 06:18 AM, [email protected] wrote:
> From: Patrice Chotard <[email protected]>
>
> This series cleans and fixes some bugs in MFD/GPIO STMPE drivers and prepare
> the ground to add new STMPE1600 support.
>
> STMPE1600 datasheet is available here :
> http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
> i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html
>
> Only STMPE1600 has been tested on STM32 platform. As i have no board with
> others STMPE variant(STMPE610/STMPE801/STMPE811/STMPE1601/STMPE1801/STMPE2401
> and STMPE2403), i put in CC boards's maintainers which are using others STMPE variant.
>
> If they can kindly check that no regression has been introduce by this series

> For TEGRA ARCHITECTURE SUPPORT
> _ Stephen Warren <[email protected]>
> _ Thierry Reding <[email protected]>
> _ Alexandre Courbot <[email protected]>

I don't know what STMPE is, and I don't believe it is used on Tegra;
what makes you think it is?

2016-04-20 07:40:57

by Patrice CHOTARD

[permalink] [raw]
Subject: Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support



On 04/19/2016 05:53 PM, Stephen Warren wrote:
> On 04/19/2016 06:18 AM, [email protected] wrote:
>> From: Patrice Chotard <[email protected]>
>>
>> This series cleans and fixes some bugs in MFD/GPIO STMPE drivers and
>> prepare
>> the ground to add new STMPE1600 support.
>>
>> STMPE1600 datasheet is available here :
>> http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
>>
>> i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html
>>
>> Only STMPE1600 has been tested on STM32 platform. As i have no board
>> with
>> others STMPE
>> variant(STMPE610/STMPE801/STMPE811/STMPE1601/STMPE1801/STMPE2401
>> and STMPE2403), i put in CC boards's maintainers which are using
>> others STMPE variant.
>>
>> If they can kindly check that no regression has been introduce by
>> this series
>
>> For TEGRA ARCHITECTURE SUPPORT
>> _ Stephen Warren <[email protected]>
>> _ Thierry Reding <[email protected]>
>> _ Alexandre Courbot <[email protected]>
>
> I don't know what STMPE is, and I don't believe it is used on Tegra;
> what makes you think it is?

Hi Stephen

STMPE family is GPIO expander, and for some of them, it includes others
fonctionnality:
_STMPE811 and STMPE610: touchscreen controller
_ STMPE1601, STMPE2401 and STMPE2403: keypad and PWM controller
_ STMPE1801: keypad controller

For more informations, some datasheets are available here:
http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/i-o-expanders-and-level-translators/i-o-expanders.html?querycriteria=productId=SC1027

drivers/mfd/stmpe.c and drivers/gpio/gpio-stmpe.c are drivers which
support all STMPE variant.
I put you in copy as STMPE811 is used on tegra30-apalis and
tegra30-colibri platforms.

Thanks

Patrice

2016-04-20 14:25:14

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support

On Tue, Apr 19, 2016 at 5:53 PM, Stephen Warren <[email protected]> wrote:
> On 04/19/2016 06:18 AM, [email protected] wrote:

>> For TEGRA ARCHITECTURE SUPPORT
>> _ Stephen Warren <[email protected]>
>> _ Thierry Reding <[email protected]>
>> _ Alexandre Courbot <[email protected]>
>
> I don't know what STMPE is,

ST Microelectronics Multi-Purpose Expander.
Some GPIO and keypad and touchscreen, PWM and what
not.

> and I don't believe it is used on Tegra; what
> makes you think it is?

Probably this:

$ git grep stmpe arch/arm/boot/dts/

arch/arm/boot/dts/tegra30-apalis.dtsi: stmpe811@41 {
arch/arm/boot/dts/tegra30-apalis.dtsi: compatible =
"st,stmpe811";
arch/arm/boot/dts/tegra30-apalis.dtsi: stmpe_touchscreen {
arch/arm/boot/dts/tegra30-apalis.dtsi:
compatible = "st,stmpe-ts";
arch/arm/boot/dts/tegra30-colibri.dtsi: stmpe811@41 {
arch/arm/boot/dts/tegra30-colibri.dtsi: compatible =
"st,stmpe811";
arch/arm/boot/dts/tegra30-colibri.dtsi: stmpe_touchscreen {
arch/arm/boot/dts/tegra30-colibri.dtsi:
compatible = "st,stmpe-ts";

Yours,
Linus Walleij

2016-04-20 14:32:40

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 1/8] mfd: stmpe: Add STMPE_IDX_SYS_CTRL/2 enum

On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:

> From: Patrice Chotard <[email protected]>
>
> As STMPE1801/1601/24xx has a SYS_CTRL register and
> STMPE1601/2403 has even a SYS_CTRL2 register, add
> STMPE_IDX_SYS_CTRL/2 and update driver code accordingly
>
> This update prepares the ground for not yet supported STMPE1600
> which share similar REG_SYS_CTRL register.
>
> Signed-off-by: Patrice Chotard <[email protected]>

Looks reasonable:
Acked-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2016-04-20 14:34:30

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 2/8] mfd: stmpe: Add reset support for all STMPE variant

On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:

> From: Patrice Chotard <[email protected]>
>
> Reset was only implemented for STMPE1801 variant despite
> all variant have a SOFT_RESET bit.
>
> For STMPE2401/2403/801/1601/1801 SOFT_RESET bit is bit 7
> of SYS_CTRL register.
> For STMPE610/811 (which have the same variant id) SOFT_RESET
> bit is bit 1 of SYS_CTRL register.
>
> Signed-off-by: Patrice Chotard <[email protected]>

Acked-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2016-04-20 14:37:32

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge detection

On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:

> From: Patrice Chotard <[email protected]>
>
> By cross-checking STMPE 610/801/811/1601/2401/2403 datasheets,
> it appears that edge detection and rising/falling edge detection
> is not supported by all STMPE variant:
>
> GPIO GPIO
> Edge detection rising/falling
> edge detection
> 610 | X | X |
> 801 | | |
> 811 | X | X |
> 1600 | | |
> 1601 | X | X |
> 1801 | | X |
> 2401 | X | X |
> 2403 | X | X |
>
> Rework stmpe_dbg_show_one() and stmpe_gpio_irq to correctly
> take these cases into account.
>
> Signed-off-by: Patrice Chotard <[email protected]>

Very nice.
Reviewed-by: Linus Walleij <[email protected]>

I expect this to go into the MFD tree with the rest, I guess
Lee will cook me an immutable branch for the whole thing
once he's happy with it.

Yours,
Linus Walleij

2016-04-20 14:38:40

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 4/8] gpio: stmpe: write int status register only when needed

On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:

> From: Patrice Chotard <[email protected]>
>
> On STMPE801/1801 datasheets, it's mentionned writing
> in interrupt status register has no effect, bits are
> cleared when reading.
>
> Signed-off-by: Amelie DELAUNAY <[email protected]>
> Signed-off-by: Patrice Chotard <[email protected]>

Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2016-04-20 14:39:08

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 5/8] Documentation: dt: add stmpe1600 compatible string to stmpe mfd

On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:

> From: Patrice Chotard <[email protected]>
>
> This patch adds a new compatible string for stmpe mfd to support
> stmpe1600 variant.
>
> Signed-off-by: Amelie DELAUNAY <[email protected]>

Acked-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2016-04-20 14:43:33

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 6/8] mfd: Add STMPE1600 support

On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:

> From: Patrice Chotard <[email protected]>
>
> STMPE1600 is a 16-bit port expander.
> Datasheet is available here :
> http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
> i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html
>
> As STMPE1600's SYS_CTRL register has the same layout as
> STMPE801 variant, unify STMPExxx_REG_SYS_CTRL_RESET/INT_EN/INT_HI
> bit masks to more generic STMPE_SYS_CTRL_RESET/INT_EN/INT_HI
>
> Signed-off-by: Amelie DELAUNAY <[email protected]>
> Signed-off-by: Patrice Chotard <[email protected]>

(...)
> #define STMPE_SYS_CTRL_RESET (1 << 7)
> +#define STMPE_SYS_CTRL_INT_EN (1 << 2)
> +#define STMPE_SYS_CTRL_INT_HI (1 << 0)
>
> /*
> * STMPE801
> @@ -121,10 +123,6 @@ int stmpe_remove(struct stmpe *stmpe);
> #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)

This looks like unrelated cleanups?
Should this be in the reset enablement patch?
(It's OK with me though.)

With that fixed:
Acked-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2016-04-20 14:53:49

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support

On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:

> From: Patrice Chotard <[email protected]>
>
> The particularities of this variant are:
> - GPIO_XXX_LSB and GPIO_XXX_MSB memory locations are inverted compared
> to other variants.
> - There is no Edge detection, Rising Edge and Falling Edge registers.
> - IRQ flags are cleared when read, no need to write in Status register.
>
> Signed-off-by: Amelie DELAUNAY <[email protected]>
> Signed-off-by: Patrice Chotard <[email protected]>

> - u8 reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8);
> + u8 reg;
> u8 mask = 1 << (offset % 8);
> int ret;
>
> + if (stmpe->partnum == STMPE1600)
> + reg = stmpe->regs[STMPE_IDX_GPMR_LSB] + (offset / 8);
> + else
> + reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8);

This construct is a bit hard to grasp.

Can we think of something more intuitive? Maybe using more
code lines but easier to understand.

Subtracting the offset is just totally unintuitive in the first place,
the STMPE1600 arrangement is much more intuitive.

I would prefer if we address the LSB+MSB register explicitly
instead of adding or subtracting 1 to the LSB register to get
to the MSB register.

> + if (stmpe->partnum == STMPE1600)
> + reg = stmpe->regs[which] + (offset / 8);
> + else
> + reg = stmpe->regs[which] - (offset / 8);

Same.

> + if (stmpe->partnum == STMPE1600)
> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
> + else
> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);

Same.

> + if (stmpe->partnum == STMPE1600)
> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
> + else
> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);

Same.

> + stmpe_reg_write(stmpe,
> + stmpe->regs[regmap[i]] + j,
> + new);
> + else
> + stmpe_reg_write(stmpe,
> + stmpe->regs[regmap[i]] - j,
> + new);

This is also unintuitively backwards.

> + if (stmpe->partnum == STMPE1600)
> + dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
> + else
> + dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);

Same.

> + if (stmpe->partnum == STMPE1600)
> + statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_LSB];
> + else
> + statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_MSB];

And this kind of points at the problem.

Can we write this in some way that make it super-clear which register
we're using and why?

Yours,
Linus Walleij

2016-04-20 14:56:54

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 8/8] gpio: stmpe: configure GPIO as output by default

On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:

> From: Patrice Chotard <[email protected]>
>
> Configures all GPIOs as output, in order to minimize power
> consumption when GPIOs are unused.
>
> Signed-off-by: Amelie DELAUNAY <[email protected]>
> Signed-off-by: Patrice Chotard <[email protected]>

Not only do you set them all to outout but also:

> + /* To minimize power consumption, configure unused GPIOs as outputs */
> + for (i = 0; i < stmpe_gpio->chip.ngpio; i++)
> + stmpe_gpio_direction_output(&stmpe_gpio->chip, i, 0);

You are driving them all low.

Now GPIO is general purpose: what if they are connected to a
line with a pull-up resistor?

That is not saving power, instead consuming more than if you
were setting them all to 1.

I am afraid this is wrong.

What you need is to be able to define in the DT (or similar)
a set of initial values for the GPIO lines, and set them to
0 for this design.

Such bindings have been discussed but no conclusion
or merged patch has emerged. Please help out in driving
a standard for this!

Yours,
Linus Walleij

2016-04-20 16:03:08

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support

On 04/20/2016 01:40 AM, Patrice Chotard wrote:
> On 04/19/2016 05:53 PM, Stephen Warren wrote:
>> On 04/19/2016 06:18 AM, [email protected] wrote:
>>> From: Patrice Chotard <[email protected]>
>>>
>>> This series cleans and fixes some bugs in MFD/GPIO STMPE drivers and
>>> prepare
>>> the ground to add new STMPE1600 support.
>>>
>>> STMPE1600 datasheet is available here :
>>> http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
>>>
>>> i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html
>>>
>>> Only STMPE1600 has been tested on STM32 platform. As i have no board
>>> with
>>> others STMPE
>>> variant(STMPE610/STMPE801/STMPE811/STMPE1601/STMPE1801/STMPE2401
>>> and STMPE2403), i put in CC boards's maintainers which are using
>>> others STMPE variant.
>>>
>>> If they can kindly check that no regression has been introduce by
>>> this series
>>
>>> For TEGRA ARCHITECTURE SUPPORT
>>> _ Stephen Warren <[email protected]>
>>> _ Thierry Reding <[email protected]>
>>> _ Alexandre Courbot <[email protected]>
>>
>> I don't know what STMPE is, and I don't believe it is used on Tegra;
>> what makes you think it is?
...
> I put you in copy as STMPE811 is used on tegra30-apalis and
> tegra30-colibri platforms.

Ah. You'd best contact the individual board owners, since those are
3rd-party Tegra boards and I don't believe anyone at NVIDIA has them to
test with etc. I added likely candidates to Cc and dropped all the
individuals unrelated to Tegra to keep the CC list low.

2016-04-20 17:50:28

by Marcel Ziswiler

[permalink] [raw]
Subject: Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support


On Apr 19, 2016 14:41, Thierry Reding <[email protected]> wrote:
>...
> Adding Marcel Ziswiler, who's better suited a

On Tue, Apr 19, 2016 at 02:18:29PM +0200, [email protected] wrote:
> From: Patrice Chotard <[email protected]>
>
> This series cleans and fixes some bugs in MFD/GPIO STMPE drivers and prepare
> the ground to add new STMPE1600 support.
>
> STMPE1600 datasheet is available here :
> http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
> i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html
>
> Only STMPE1600 has been tested on STM32 platform. As i have no board with
> others STMPE variant(STMPE610/STMPE801/STMPE811/STMPE1601/STMPE1801/STMPE2401
> and STMPE2403), i put in CC boards's maintainers which are using others STMPE variant.
>
> If they can kindly check that no regression has been introduce by this series
> :
>
> For ARM/FREESCALE IMX / MXC ARM ARCHITECTURE:
> _ Shawn Guo <[email protected]>
> _ Sascha Hauer <[email protected]>
>
> For ARM/SOCFPGA ARCHITECTURE
> _ Dinh Nguyen <[email protected]>
>
> For SPEAR PLATFORM SUPPORT
> _ Viresh Kumar <[email protected]>
> _ Shiraz Hashim <[email protected]>
>
> For TEGRA ARCHITECTURE SUPPORT
> _ Stephen Warren <[email protected]>
> _ Thierry Reding <[email protected]>
> _ Alexandre Courbot <[email protected]>

Adding Marcel Ziswiler, who's better suited at judging whether or not
this has any impact on Apalis/Colibri.

Marcel, in case you don't have these in your inbox you can find them on
linux-gpio's patchwork:

https://patchwork.ozlabs.org/project/linux-gpio/list/

Thierry

2016-04-21 03:11:25

by Marcel Ziswiler

[permalink] [raw]
Subject: Re: [PATCH 0/8] STMPE fixes/rework and add STMPE1600 support

On Wed, 2016-04-20 at 10:02 -0600, Stephen Warren wrote:
> On 04/20/2016 01:40 AM, Patrice Chotard wrote:
> >
> > On 04/19/2016 05:53 PM, Stephen Warren wrote:
> > >
> > > On 04/19/2016 06:18 AM, [email protected] wrote:
> > > >
> > > > From: Patrice Chotard <[email protected]>
> > > >
> > > > This series cleans and fixes some bugs in MFD/GPIO STMPE
> > > > drivers and
> > > > prepare
> > > >   the ground to add new STMPE1600 support.
> > > >
> > > > STMPE1600 datasheet is available here :
> > > > http://www2.st.com/content/st_com/en/products/interfaces-and-tr
> > > > ansceivers/
> > > >
> > > > i-o-expanders-and-level-translators/i-o-
> > > > expanders/stmpe1600.html
> > > >
> > > > Only STMPE1600 has been tested on STM32 platform. As i have no
> > > > board
> > > > with
> > > > others STMPE
> > > > variant(STMPE610/STMPE801/STMPE811/STMPE1601/STMPE1801/STMPE240
> > > > 1
> > > > and STMPE2403), i put in CC boards's maintainers which are
> > > > using
> > > > others STMPE variant.
> > > >
> > > > If they can kindly check that no regression has been introduce
> > > > by
> > > > this series
> > > >
> > > > For TEGRA ARCHITECTURE SUPPORT
> > > >     _ Stephen Warren <[email protected]>
> > > >     _ Thierry Reding <[email protected]>
> > > >     _ Alexandre Courbot <[email protected]>
> > > I don't know what STMPE is, and I don't believe it is used on
> > > Tegra;
> > > what makes you think it is?
> ...
> >
> > I put you in copy as STMPE811 is used on tegra30-apalis and
> > tegra30-colibri platforms.
> Ah. You'd best contact the individual board owners, since those are 
> 3rd-party Tegra boards and I don't believe anyone at NVIDIA has them
> to 
> test with etc. I added likely candidates to Cc and dropped all the 
> individuals unrelated to Tegra to keep the CC list low.

I gave the whole series a spin on Apalis T30 2GB V1.1A featuring a
STMPE811 and the EDT VGA touch panel connected to it still works
perfectly running LXDE on top of the modesetting X driver.
Unfortunately the 4.6.0-rc4-next-20160420 I used for testing is broken
beyond easy quick repair on i.MX 6 so I was unable to validate it on
there, sorry.


Tested-by: Marcel Ziswiler <[email protected]>

2016-04-21 13:49:37

by Patrice CHOTARD

[permalink] [raw]
Subject: Re: [PATCH 3/8] gpio: stmpe: fix edge and rising/falling edge detection



On 04/20/2016 04:37 PM, Linus Walleij wrote:
> On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:
>
>> From: Patrice Chotard <[email protected]>
>>
>> By cross-checking STMPE 610/801/811/1601/2401/2403 datasheets,
>> it appears that edge detection and rising/falling edge detection
>> is not supported by all STMPE variant:
>>
>> GPIO GPIO
>> Edge detection rising/falling
>> edge detection
>> 610 | X | X |
>> 801 | | |
>> 811 | X | X |
>> 1600 | | |
>> 1601 | X | X |
>> 1801 | | X |
>> 2401 | X | X |
>> 2403 | X | X |
>>
>> Rework stmpe_dbg_show_one() and stmpe_gpio_irq to correctly
>> take these cases into account.
>>
>> Signed-off-by: Patrice Chotard <[email protected]>
> Very nice.
> Reviewed-by: Linus Walleij <[email protected]>
>
> I expect this to go into the MFD tree with the rest, I guess
> Lee will cook me an immutable branch for the whole thing
> once he's happy with it.
>
> Yours,
> Linus Walleij

Hi Linus

I will send a v2 as warnings as been detected by kbuild test robot

Thanks

Patrice

2016-04-21 13:52:20

by Patrice CHOTARD

[permalink] [raw]
Subject: Re: [PATCH 6/8] mfd: Add STMPE1600 support



On 04/20/2016 04:43 PM, Linus Walleij wrote:
> On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:
>
>> From: Patrice Chotard <[email protected]>
>>
>> STMPE1600 is a 16-bit port expander.
>> Datasheet is available here :
>> http://www2.st.com/content/st_com/en/products/interfaces-and-transceivers/
>> i-o-expanders-and-level-translators/i-o-expanders/stmpe1600.html
>>
>> As STMPE1600's SYS_CTRL register has the same layout as
>> STMPE801 variant, unify STMPExxx_REG_SYS_CTRL_RESET/INT_EN/INT_HI
>> bit masks to more generic STMPE_SYS_CTRL_RESET/INT_EN/INT_HI
>>
>> Signed-off-by: Amelie DELAUNAY <[email protected]>
>> Signed-off-by: Patrice Chotard <[email protected]>
> (...)
>> #define STMPE_SYS_CTRL_RESET (1 << 7)
>> +#define STMPE_SYS_CTRL_INT_EN (1 << 2)
>> +#define STMPE_SYS_CTRL_INT_HI (1 << 0)
>>
>> /*
>> * STMPE801
>> @@ -121,10 +123,6 @@ int stmpe_remove(struct stmpe *stmpe);
>> #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)
> This looks like unrelated cleanups?
> Should this be in the reset enablement patch?
> (It's OK with me though.)

Not completely unrelated as STMPE801 and STMPE1600 use the same SYS_CTRL
register layout.
But nevertheless i will put this fix in a separate patch.

Thanks

Patrice

>
> With that fixed:
> Acked-by: Linus Walleij <[email protected]>
>
> Yours,
> Linus Walleij
Hi


2016-04-22 07:18:19

by Patrice CHOTARD

[permalink] [raw]
Subject: Re: [PATCH 7/8] gpio: stmpe: Add STMPE1600 support



On 04/20/2016 04:53 PM, Linus Walleij wrote:
> On Tue, Apr 19, 2016 at 2:18 PM, <[email protected]> wrote:
>
>> From: Patrice Chotard <[email protected]>
>>
>> The particularities of this variant are:
>> - GPIO_XXX_LSB and GPIO_XXX_MSB memory locations are inverted compared
>> to other variants.
>> - There is no Edge detection, Rising Edge and Falling Edge registers.
>> - IRQ flags are cleared when read, no need to write in Status register.
>>
>> Signed-off-by: Amelie DELAUNAY <[email protected]>
>> Signed-off-by: Patrice Chotard <[email protected]>
>> - u8 reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8);
>> + u8 reg;
>> u8 mask = 1 << (offset % 8);
>> int ret;
>>
>> + if (stmpe->partnum == STMPE1600)
>> + reg = stmpe->regs[STMPE_IDX_GPMR_LSB] + (offset / 8);
>> + else
>> + reg = stmpe->regs[STMPE_IDX_GPMR_LSB] - (offset / 8);
> This construct is a bit hard to grasp.
>
> Can we think of something more intuitive? Maybe using more
> code lines but easier to understand.
>
> Subtracting the offset is just totally unintuitive in the first place,
> the STMPE1600 arrangement is much more intuitive.
>
> I would prefer if we address the LSB+MSB register explicitly
> instead of adding or subtracting 1 to the LSB register to get
> to the MSB register.
>
>> + if (stmpe->partnum == STMPE1600)
>> + reg = stmpe->regs[which] + (offset / 8);
>> + else
>> + reg = stmpe->regs[which] - (offset / 8);
> Same.
>
>> + if (stmpe->partnum == STMPE1600)
>> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
>> + else
>> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
> Same.
>
>> + if (stmpe->partnum == STMPE1600)
>> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
>> + else
>> + reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
> Same.
>
>> + stmpe_reg_write(stmpe,
>> + stmpe->regs[regmap[i]] + j,
>> + new);
>> + else
>> + stmpe_reg_write(stmpe,
>> + stmpe->regs[regmap[i]] - j,
>> + new);
> This is also unintuitively backwards.
>
>> + if (stmpe->partnum == STMPE1600)
>> + dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] + (offset / 8);
>> + else
>> + dir_reg = stmpe->regs[STMPE_IDX_GPDR_LSB] - (offset / 8);
> Same.
>
>> + if (stmpe->partnum == STMPE1600)
>> + statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_LSB];
>> + else
>> + statmsbreg = stmpe->regs[STMPE_IDX_ISGPIOR_MSB];
> And this kind of points at the problem.
>
> Can we write this in some way that make it super-clear which register
> we're using and why?

Ok i will rework all these points

Thanks

Patrice

>
> Yours,
> Linus Walleij

2016-04-26 08:18:52

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 2/8] mfd: stmpe: Add reset support for all STMPE variant

On Tue, 19 Apr 2016, [email protected] wrote:

> From: Patrice Chotard <[email protected]>
>
> Reset was only implemented for STMPE1801 variant despite
> all variant have a SOFT_RESET bit.
>
> For STMPE2401/2403/801/1601/1801 SOFT_RESET bit is bit 7
> of SYS_CTRL register.
> For STMPE610/811 (which have the same variant id) SOFT_RESET
> bit is bit 1 of SYS_CTRL register.
>
> Signed-off-by: Patrice Chotard <[email protected]>
> ---
> drivers/mfd/stmpe.c | 23 +++++++++++++++--------
> drivers/mfd/stmpe.h | 7 +++++--
> 2 files changed, 20 insertions(+), 10 deletions(-)

When you fix and resubmit, please add my:

Acked-by: Lee Jones <[email protected]>

... for my own personal reference.

> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index c553b73..af682d0 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -735,13 +735,22 @@ static int stmpe1801_enable(struct stmpe *stmpe, unsigned int blocks,
> enable ? mask : 0);
> }
>
> -static int stmpe1801_reset(struct stmpe *stmpe)
> +static int stmpe_reset(struct stmpe *stmpe)
> {
> + u16 id_val = stmpe->variant->id_val;
> unsigned long timeout;
> int ret = 0;
> + u8 reset_bit;
> +
> + if (id_val == STMPE811_ID)
> + /* STMPE801 and STMPE610 use bit 1 of SYS_CTRL register */
> + reset_bit = STMPE811_SYS_CTRL_RESET;
> + else
> + /* all other STMPE variant use bit 7 of SYS_CTRL register */
> + reset_bit = STMPE_SYS_CTRL_RESET;
>
> ret = __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL],
> - STMPE1801_MSK_SYS_CTRL_RESET, STMPE1801_MSK_SYS_CTRL_RESET);
> + reset_bit, reset_bit);
> if (ret < 0)
> return ret;
>
> @@ -750,7 +759,7 @@ static int stmpe1801_reset(struct stmpe *stmpe)
> ret = __stmpe_reg_read(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL]);
> if (ret < 0)
> return ret;
> - if (!(ret & STMPE1801_MSK_SYS_CTRL_RESET))
> + if (!(ret & reset_bit))
> return 0;
> usleep_range(100, 200);
> }
> @@ -1074,11 +1083,9 @@ static int stmpe_chip_init(struct stmpe *stmpe)
> if (ret)
> return ret;
>
> - if (id == STMPE1801_ID) {
> - ret = stmpe1801_reset(stmpe);
> - if (ret < 0)
> - return ret;
> - }
> + ret = stmpe_reset(stmpe);
> + if (ret < 0)
> + return ret;
>
> if (stmpe->irq >= 0) {
> if (id == STMPE801_ID)
> diff --git a/drivers/mfd/stmpe.h b/drivers/mfd/stmpe.h
> index 406f9f2..4ae343d 100644
> --- a/drivers/mfd/stmpe.h
> +++ b/drivers/mfd/stmpe.h
> @@ -104,6 +104,8 @@ int stmpe_remove(struct stmpe *stmpe);
> #define STMPE_ICR_LSB_EDGE (1 << 1)
> #define STMPE_ICR_LSB_GIM (1 << 0)
>
> +#define STMPE_SYS_CTRL_RESET (1 << 7)
> +
> /*
> * STMPE801
> */
> @@ -126,6 +128,7 @@ int stmpe_remove(struct stmpe *stmpe);
> /*
> * STMPE811
> */
> +#define STMPE811_ID 0x0811
>
> #define STMPE811_IRQ_TOUCH_DET 0
> #define STMPE811_IRQ_FIFO_TH 1
> @@ -155,6 +158,8 @@ int stmpe_remove(struct stmpe *stmpe);
> #define STMPE811_REG_GPIO_FE 0x16
> #define STMPE811_REG_GPIO_AF 0x17
>
> +#define STMPE811_SYS_CTRL_RESET (1 << 1)
> +
> #define STMPE811_SYS_CTRL2_ADC_OFF (1 << 0)
> #define STMPE811_SYS_CTRL2_TSC_OFF (1 << 1)
> #define STMPE811_SYS_CTRL2_GPIO_OFF (1 << 2)
> @@ -244,8 +249,6 @@ int stmpe_remove(struct stmpe *stmpe);
> #define STMPE1801_REG_GPIO_PULL_UP_MID 0x23
> #define STMPE1801_REG_GPIO_PULL_UP_HIGH 0x24
>
> -#define STMPE1801_MSK_SYS_CTRL_RESET (1 << 7)
> -
> #define STMPE1801_MSK_INT_EN_KPC (1 << 1)
> #define STMPE1801_MSK_INT_EN_GPIO (1 << 3)
>

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2016-04-26 08:19:18

by Lee Jones

[permalink] [raw]
Subject: Re: [PATCH 1/8] mfd: stmpe: Add STMPE_IDX_SYS_CTRL/2 enum

On Tue, 19 Apr 2016, [email protected] wrote:

> From: Patrice Chotard <[email protected]>
>
> As STMPE1801/1601/24xx has a SYS_CTRL register and
> STMPE1601/2403 has even a SYS_CTRL2 register, add
> STMPE_IDX_SYS_CTRL/2 and update driver code accordingly
>
> This update prepares the ground for not yet supported STMPE1600
> which share similar REG_SYS_CTRL register.
>
> Signed-off-by: Patrice Chotard <[email protected]>
> ---
> drivers/mfd/stmpe.c | 21 ++++++++++++++-------
> drivers/mfd/stmpe.h | 2 ++
> include/linux/mfd/stmpe.h | 2 ++
> 3 files changed, 18 insertions(+), 7 deletions(-)

When you fix and resubmit, please add my:

Acked-by: Lee Jones <[email protected]>

... for my own personal reference.

> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index fb8f9e8..c553b73 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -448,6 +448,8 @@ static const struct mfd_cell stmpe_ts_cell = {
>
> static const u8 stmpe811_regs[] = {
> [STMPE_IDX_CHIP_ID] = STMPE811_REG_CHIP_ID,
> + [STMPE_IDX_SYS_CTRL] = STMPE811_REG_SYS_CTRL,
> + [STMPE_IDX_SYS_CTRL2] = STMPE811_REG_SYS_CTRL2,
> [STMPE_IDX_ICR_LSB] = STMPE811_REG_INT_CTRL,
> [STMPE_IDX_IER_LSB] = STMPE811_REG_INT_EN,
> [STMPE_IDX_ISR_MSB] = STMPE811_REG_INT_STA,
> @@ -490,7 +492,7 @@ static int stmpe811_enable(struct stmpe *stmpe, unsigned int blocks,
> if (blocks & STMPE_BLOCK_TOUCHSCREEN)
> mask |= STMPE811_SYS_CTRL2_TSC_OFF;
>
> - return __stmpe_set_bits(stmpe, STMPE811_REG_SYS_CTRL2, mask,
> + return __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL2], mask,
> enable ? 0 : mask);
> }
>
> @@ -535,6 +537,8 @@ static struct stmpe_variant_info stmpe610 = {
>
> static const u8 stmpe1601_regs[] = {
> [STMPE_IDX_CHIP_ID] = STMPE1601_REG_CHIP_ID,
> + [STMPE_IDX_SYS_CTRL] = STMPE1601_REG_SYS_CTRL,
> + [STMPE_IDX_SYS_CTRL2] = STMPE1601_REG_SYS_CTRL2,
> [STMPE_IDX_ICR_LSB] = STMPE1601_REG_ICR_LSB,
> [STMPE_IDX_IER_LSB] = STMPE1601_REG_IER_LSB,
> [STMPE_IDX_ISR_MSB] = STMPE1601_REG_ISR_MSB,
> @@ -619,13 +623,13 @@ static int stmpe1601_autosleep(struct stmpe *stmpe,
> return timeout;
> }
>
> - ret = __stmpe_set_bits(stmpe, STMPE1601_REG_SYS_CTRL2,
> + ret = __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL2],
> STMPE1601_AUTOSLEEP_TIMEOUT_MASK,
> timeout);
> if (ret < 0)
> return ret;
>
> - return __stmpe_set_bits(stmpe, STMPE1601_REG_SYS_CTRL2,
> + return __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL2],
> STPME1601_AUTOSLEEP_ENABLE,
> STPME1601_AUTOSLEEP_ENABLE);
> }
> @@ -650,7 +654,7 @@ static int stmpe1601_enable(struct stmpe *stmpe, unsigned int blocks,
> else
> mask &= ~STMPE1601_SYS_CTRL_ENABLE_SPWM;
>
> - return __stmpe_set_bits(stmpe, STMPE1601_REG_SYS_CTRL, mask,
> + return __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL], mask,
> enable ? mask : 0);
> }
>
> @@ -689,6 +693,7 @@ static struct stmpe_variant_info stmpe1601 = {
> */
> static const u8 stmpe1801_regs[] = {
> [STMPE_IDX_CHIP_ID] = STMPE1801_REG_CHIP_ID,
> + [STMPE_IDX_SYS_CTRL] = STMPE1801_REG_SYS_CTRL,
> [STMPE_IDX_ICR_LSB] = STMPE1801_REG_INT_CTRL_LOW,
> [STMPE_IDX_IER_LSB] = STMPE1801_REG_INT_EN_MASK_LOW,
> [STMPE_IDX_ISR_LSB] = STMPE1801_REG_INT_STA_LOW,
> @@ -735,14 +740,14 @@ static int stmpe1801_reset(struct stmpe *stmpe)
> unsigned long timeout;
> int ret = 0;
>
> - ret = __stmpe_set_bits(stmpe, STMPE1801_REG_SYS_CTRL,
> + ret = __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL],
> STMPE1801_MSK_SYS_CTRL_RESET, STMPE1801_MSK_SYS_CTRL_RESET);
> if (ret < 0)
> return ret;
>
> timeout = jiffies + msecs_to_jiffies(100);
> while (time_before(jiffies, timeout)) {
> - ret = __stmpe_reg_read(stmpe, STMPE1801_REG_SYS_CTRL);
> + ret = __stmpe_reg_read(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL]);
> if (ret < 0)
> return ret;
> if (!(ret & STMPE1801_MSK_SYS_CTRL_RESET))
> @@ -773,6 +778,8 @@ static struct stmpe_variant_info stmpe1801 = {
>
> static const u8 stmpe24xx_regs[] = {
> [STMPE_IDX_CHIP_ID] = STMPE24XX_REG_CHIP_ID,
> + [STMPE_IDX_SYS_CTRL] = STMPE24XX_REG_SYS_CTRL,
> + [STMPE_IDX_SYS_CTRL2] = STMPE24XX_REG_SYS_CTRL2,
> [STMPE_IDX_ICR_LSB] = STMPE24XX_REG_ICR_LSB,
> [STMPE_IDX_IER_LSB] = STMPE24XX_REG_IER_LSB,
> [STMPE_IDX_ISR_MSB] = STMPE24XX_REG_ISR_MSB,
> @@ -819,7 +826,7 @@ static int stmpe24xx_enable(struct stmpe *stmpe, unsigned int blocks,
> if (blocks & STMPE_BLOCK_KEYPAD)
> mask |= STMPE24XX_SYS_CTRL_ENABLE_KPC;
>
> - return __stmpe_set_bits(stmpe, STMPE24XX_REG_SYS_CTRL, mask,
> + return __stmpe_set_bits(stmpe, stmpe->regs[STMPE_IDX_SYS_CTRL], mask,
> enable ? mask : 0);
> }
>
> diff --git a/drivers/mfd/stmpe.h b/drivers/mfd/stmpe.h
> index 84adb46..406f9f2 100644
> --- a/drivers/mfd/stmpe.h
> +++ b/drivers/mfd/stmpe.h
> @@ -138,6 +138,7 @@ int stmpe_remove(struct stmpe *stmpe);
> #define STMPE811_NR_INTERNAL_IRQS 8
>
> #define STMPE811_REG_CHIP_ID 0x00
> +#define STMPE811_REG_SYS_CTRL 0x03
> #define STMPE811_REG_SYS_CTRL2 0x04
> #define STMPE811_REG_SPI_CFG 0x08
> #define STMPE811_REG_INT_CTRL 0x09
> @@ -264,6 +265,7 @@ int stmpe_remove(struct stmpe *stmpe);
> #define STMPE24XX_NR_INTERNAL_IRQS 9
>
> #define STMPE24XX_REG_SYS_CTRL 0x02
> +#define STMPE24XX_REG_SYS_CTRL2 0x03
> #define STMPE24XX_REG_ICR_LSB 0x11
> #define STMPE24XX_REG_IER_LSB 0x13
> #define STMPE24XX_REG_ISR_MSB 0x14
> diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h
> index cb83883..6c6690f 100644
> --- a/include/linux/mfd/stmpe.h
> +++ b/include/linux/mfd/stmpe.h
> @@ -39,6 +39,8 @@ enum stmpe_partnum {
> */
> enum {
> STMPE_IDX_CHIP_ID,
> + STMPE_IDX_SYS_CTRL,
> + STMPE_IDX_SYS_CTRL2,
> STMPE_IDX_ICR_LSB,
> STMPE_IDX_IER_LSB,
> STMPE_IDX_ISR_LSB,

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog