[PATCH v3 00/15] ST33 I2C TPM driver cleanup

[PATCH v3 00/15] ST33 I2C TPM driver cleanup

[Christophe Ricard]

Hi Peter,

This new patchset version is taking into account as much as possible
Jason Gunthorpe feedbacks. I hope, i am not missing any.

It still brings:
- Some few code clean up from code style up to structure
- Device tree support keeping static platform data configuration support.
- Fixes for irq support.
- Update the GPLv2 license header

This patchset apply on top of James Morris linux-security tree
on top of 594081ee7145cc30a3977cb4e218f81213b63dc5 on next branch
Hope i am targetting to correct tree.

The full patchset got also crosschecked by Jean-Luc here in copy.

Best Regards
Christophe

Christophe Ricard (15):
  tpm/tpm_i2c_stm_st33: Update Kconfig in order to be inline to other
    similar product
  tpm/tpm_i2c_stm_st33: Fix few coding style error reported by
    scripts/checkpatch.pl
  tpm/tpm_i2c_stm_st33: Move tpm registers to tpm_i2c_stm_st33.c
  tpm/tpm_i2c_stm_st33: Add new tpm_stm_dev structure and remove
    tpm_i2c_buffer[0], [1] buffer.
  tpm/tpm_i2c_stm_st33: Remove reference to io_serirq
  tpm/tpm_i2c_stm_st33: Replace err/rc/ret by r for a function return
    code
  tpm/tpm_i2c_stm_st33: Replace tpm_st33_* function with tpm_stm_*
  tpm/tpm_i2c_stm_st33: Add devicetree structure
  tpm/tpm_i2c_stm_st33/dts/st33zp24_i2c: Add DTS Documentation
  tpm/tpm_i2c_stm_st33: Few code cleanup
  tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by
    wait_for_tpm_stat
  tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers
  tpm/tpm_i2c_stm_st33: Fix potential bug in tpm_stm_i2c_send
  tpm/tpm_i2c_stm_st33: Change License header to have up to date address
    information
  tpm/tpm_i2c_stm_st33: Increment driver version to 1.2.1.

 .../devicetree/bindings/security/tpm/st33zp24.txt  |  36 ++
 drivers/char/tpm/Kconfig                           |   4 +-
 drivers/char/tpm/Makefile                          |   2 +-
 drivers/char/tpm/tpm_i2c_stm_st33.c                | 619 +++++++++++----------
 drivers/char/tpm/tpm_i2c_stm_st33.h                |  61 --
 include/linux/platform_data/tpm_i2c_stm_st33.h     |  39 ++
 6 files changed, 392 insertions(+), 369 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/st33zp24.txt
 delete mode 100644 drivers/char/tpm/tpm_i2c_stm_st33.h
 create mode 100644 include/linux/platform_data/tpm_i2c_stm_st33.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 01/15] tpm/tpm_i2c_stm_st33: Update Kconfig in order to be inline to other similar product

[Christophe Ricard]

STMicroelectronics i2c tpm is the only one to have a different tristate
label.

Rename it "TPM Interface Specification 1.2 Interface (I2C - STMicroelectronics)"

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/Kconfig  | 4 ++--
 drivers/char/tpm/Makefile | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index c54cac3..edfb45c 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -100,8 +100,8 @@ config TCG_IBMVTPM
 	  will be accessible from within Linux.  To compile this driver
 	  as a module, choose M here; the module will be called tpm_ibmvtpm.
 
-config TCG_ST33_I2C
-	tristate "STMicroelectronics ST33 I2C TPM"
+config TCG_TIS_I2C_ST33
+	tristate "TPM Interface Specification 1.2 Interface (I2C - STMicroelectronics)"
 	depends on I2C
 	depends on GPIOLIB
 	---help---
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 4d85dd6..7f54dae 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -20,5 +20,5 @@ obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
 obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
 obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
 obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
-obj-$(CONFIG_TCG_ST33_I2C) += tpm_i2c_stm_st33.o
+obj-$(CONFIG_TCG_TIS_I2C_ST33) += tpm_i2c_stm_st33.o
 obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 02/15] tpm/tpm_i2c_stm_st33: Fix few coding style error reported by scripts/checkpatch.pl

[Christophe Ricard]

Fix:
- WARNING: Missing a blank line after declarations
- WARNING: braces {} are not necessary for any arm of this statement

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 4669e37..52d80ef 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -157,6 +157,7 @@ static int read8_reg(struct i2c_client *client, u8 tpm_register,
 static void clear_interruption(struct i2c_client *client)
 {
 	u8 interrupt;
+
 	I2C_READ_DATA(client, TPM_INT_STATUS, &interrupt, 1);
 	I2C_WRITE_DATA(client, TPM_INT_STATUS, &interrupt, 1);
 	I2C_READ_DATA(client, TPM_INT_STATUS, &interrupt, 1);
@@ -233,6 +234,7 @@ static u8 tpm_stm_i2c_status(struct tpm_chip *chip)
 {
 	struct i2c_client *client;
 	u8 data;
+
 	client = (struct i2c_client *)TPM_VPRIV(chip);
 
 	I2C_READ_DATA(client, TPM_STS, &data, 1);
@@ -784,11 +786,11 @@ static int tpm_st33_i2c_pm_suspend(struct device *dev)
 	struct st33zp24_platform_data *pin_infos = dev->platform_data;
 	int ret = 0;
 
-	if (power_mgt) {
+	if (power_mgt)
 		gpio_set_value(pin_infos->io_lpcpd, 0);
-	} else {
+	else
 		ret = tpm_pm_suspend(dev);
-	}
+
 	return ret;
 }				/* tpm_st33_i2c_suspend() */
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 03/15] tpm/tpm_i2c_stm_st33: Move tpm registers to tpm_i2c_stm_st33.c

[Christophe Ricard]

Move tpm registers to tpm_i2c_stm_st33.c.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 17 +++++++++++++++++
 drivers/char/tpm/tpm_i2c_stm_st33.h | 17 -----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 52d80ef..8a8fc10 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -50,6 +50,23 @@
 #include <linux/slab.h>
 
 #include "tpm.h"
+
+#define TPM_ACCESS			0x0
+#define TPM_STS				0x18
+#define TPM_HASH_END			0x20
+#define TPM_DATA_FIFO			0x24
+#define TPM_HASH_DATA			0x24
+#define TPM_HASH_START			0x28
+#define TPM_INTF_CAPABILITY		0x14
+#define TPM_INT_STATUS			0x10
+#define TPM_INT_ENABLE			0x08
+
+#define TPM_DUMMY_BYTE			0xAA
+#define TPM_WRITE_DIRECTION		0x80
+#define TPM_HEADER_SIZE			10
+#define TPM_BUFSIZE			2048
+
+#define LOCALITY0		0
 #include "tpm_i2c_stm_st33.h"
 
 enum stm33zp24_access {
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.h b/drivers/char/tpm/tpm_i2c_stm_st33.h
index 439a432..3041271 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.h
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.h
@@ -30,23 +30,6 @@
 #ifndef __STM_ST33_TPM_I2C_MAIN_H__
 #define __STM_ST33_TPM_I2C_MAIN_H__
 
-#define TPM_ACCESS			(0x0)
-#define TPM_STS				(0x18)
-#define TPM_HASH_END			(0x20)
-#define TPM_DATA_FIFO			(0x24)
-#define TPM_HASH_DATA			(0x24)
-#define TPM_HASH_START			(0x28)
-#define TPM_INTF_CAPABILITY		(0x14)
-#define TPM_INT_STATUS			(0x10)
-#define TPM_INT_ENABLE			(0x08)
-
-#define TPM_DUMMY_BYTE			0xAA
-#define TPM_WRITE_DIRECTION		0x80
-#define TPM_HEADER_SIZE			10
-#define TPM_BUFSIZE			2048
-
-#define LOCALITY0		0
-
 #define TPM_ST33_I2C			"st33zp24_i2c"
 
 struct st33zp24_platform_data {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 04/15] tpm/tpm_i2c_stm_st33: Add new tpm_stm_dev structure and remove tpm_i2c_buffer[0], [1] buffer.

[Christophe Ricard]

In order to clean big buffers in st33zp24_platform_data structure,
replace with tpm_stm_dev for driver internal usage.
As only one buffer is really necessary replace with buf field.

In the mean time move tpm_i2c_stm_st33.h to include/linux/platform_data.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c            | 198 +++++++++++--------------
 drivers/char/tpm/tpm_i2c_stm_st33.h            |  44 ------
 include/linux/platform_data/tpm_i2c_stm_st33.h |  41 +++++
 3 files changed, 130 insertions(+), 153 deletions(-)
 delete mode 100644 drivers/char/tpm/tpm_i2c_stm_st33.h
 create mode 100644 include/linux/platform_data/tpm_i2c_stm_st33.h

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 8a8fc10..07ce463 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -41,7 +41,6 @@
 #include <linux/wait.h>
 #include <linux/string.h>
 #include <linux/interrupt.h>
-#include <linux/spinlock.h>
 #include <linux/sysfs.h>
 #include <linux/gpio.h>
 #include <linux/sched.h>
@@ -49,6 +48,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 
+#include <linux/platform_data/tpm_i2c_stm_st33.h>
 #include "tpm.h"
 
 #define TPM_ACCESS			0x0
@@ -67,7 +67,7 @@
 #define TPM_BUFSIZE			2048
 
 #define LOCALITY0		0
-#include "tpm_i2c_stm_st33.h"
+
 
 enum stm33zp24_access {
 	TPM_ACCESS_VALID = 0x80,
@@ -99,6 +99,15 @@ enum tis_defaults {
 	TIS_LONG_TIMEOUT = 2000,
 };
 
+struct tpm_stm_dev {
+	struct i2c_client *client;
+	struct completion irq_detection;
+	struct tpm_chip *chip;
+	u8 buf[TPM_BUFSIZE + 1];
+	int io_serirq;
+	int io_lpcpd;
+};
+
 /*
  * write8_reg
  * Send byte to the TIS register according to the ST33ZP24 I2C protocol.
@@ -107,17 +116,12 @@ enum tis_defaults {
  * @param: tpm_size, The length of the data
  * @return: Returns negative errno, or else the number of bytes written.
  */
-static int write8_reg(struct i2c_client *client, u8 tpm_register,
+static int write8_reg(struct tpm_stm_dev *tpm_dev, u8 tpm_register,
 		      u8 *tpm_data, u16 tpm_size)
 {
-	struct st33zp24_platform_data *pin_infos;
-
-	pin_infos = client->dev.platform_data;
-
-	pin_infos->tpm_i2c_buffer[0][0] = tpm_register;
-	memcpy(&pin_infos->tpm_i2c_buffer[0][1], tpm_data, tpm_size);
-	return i2c_master_send(client, pin_infos->tpm_i2c_buffer[0],
-				tpm_size + 1);
+	tpm_dev->buf[0] = tpm_register;
+	memcpy(tpm_dev->buf + 1, tpm_data, tpm_size);
+	return i2c_master_send(tpm_dev->client, tpm_dev->buf, tpm_size + 1);
 } /* write8_reg() */
 
 /*
@@ -128,50 +132,50 @@ static int write8_reg(struct i2c_client *client, u8 tpm_register,
  * @param: tpm_size, tpm TPM response size to read.
  * @return: number of byte read successfully: should be one if success.
  */
-static int read8_reg(struct i2c_client *client, u8 tpm_register,
+static int read8_reg(struct tpm_stm_dev *tpm_dev, u8 tpm_register,
 		    u8 *tpm_data, int tpm_size)
 {
 	u8 status = 0;
 	u8 data;
 
 	data = TPM_DUMMY_BYTE;
-	status = write8_reg(client, tpm_register, &data, 1);
+	status = write8_reg(tpm_dev, tpm_register, &data, 1);
 	if (status == 2)
-		status = i2c_master_recv(client, tpm_data, tpm_size);
+		status = i2c_master_recv(tpm_dev->client, tpm_data, tpm_size);
 	return status;
 } /* read8_reg() */
 
 /*
  * I2C_WRITE_DATA
  * Send byte to the TIS register according to the ST33ZP24 I2C protocol.
- * @param: client, the chip description
+ * @param: tpm_dev, the chip description
  * @param: tpm_register, the tpm tis register where the data should be written
  * @param: tpm_data, the tpm_data to write inside the tpm_register
  * @param: tpm_size, The length of the data
  * @return: number of byte written successfully: should be one if success.
  */
-#define I2C_WRITE_DATA(client, tpm_register, tpm_data, tpm_size) \
-	(write8_reg(client, tpm_register | \
+#define I2C_WRITE_DATA(tpm_dev, tpm_register, tpm_data, tpm_size) \
+	(write8_reg(tpm_dev, tpm_register | \
 	TPM_WRITE_DIRECTION, tpm_data, tpm_size))
 
 /*
  * I2C_READ_DATA
  * Recv byte from the TIS register according to the ST33ZP24 I2C protocol.
- * @param: tpm, the chip description
+ * @param: tpm_dev, the chip description
  * @param: tpm_register, the tpm tis register where the data should be read
  * @param: tpm_data, the TPM response
  * @param: tpm_size, tpm TPM response size to read.
  * @return: number of byte read successfully: should be one if success.
  */
-#define I2C_READ_DATA(client, tpm_register, tpm_data, tpm_size) \
-	(read8_reg(client, tpm_register, tpm_data, tpm_size))
+#define I2C_READ_DATA(tpm_dev, tpm_register, tpm_data, tpm_size) \
+	(read8_reg(tpm_dev, tpm_register, tpm_data, tpm_size))
 
 /*
  * clear_interruption
  * clear the TPM interrupt register.
  * @param: tpm, the chip description
  */
-static void clear_interruption(struct i2c_client *client)
+static void clear_interruption(struct tpm_stm_dev *tpm_dev)
 {
 	u8 interrupt;
 
@@ -191,17 +195,16 @@ static long _wait_for_interrupt_serirq_timeout(struct tpm_chip *chip,
 {
 	long status;
 	struct i2c_client *client;
-	struct st33zp24_platform_data *pin_infos;
+	struct tpm_stm_dev *tpm_dev;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
-	pin_infos = client->dev.platform_data;
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
+	client = tpm_dev->client;
 
 	status = wait_for_completion_interruptible_timeout(
-					&pin_infos->irq_detection,
-						timeout);
+					&tpm_dev->irq_detection,
+					timeout);
 	if (status > 0)
-		enable_irq(gpio_to_irq(pin_infos->io_serirq));
-	gpio_direction_input(pin_infos->io_serirq);
+		enable_irq(client->irq);
 
 	return status;
 } /* wait_for_interrupt_serirq_timeout() */
@@ -210,15 +213,15 @@ static int wait_for_serirq_timeout(struct tpm_chip *chip, bool condition,
 				 unsigned long timeout)
 {
 	int status = 2;
-	struct i2c_client *client;
+	struct tpm_stm_dev *tpm_dev;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
 	status = _wait_for_interrupt_serirq_timeout(chip, timeout);
 	if (!status) {
 		status = -EBUSY;
 	} else {
-		clear_interruption(client);
+		clear_interruption(tpm_dev);
 		if (condition)
 			status = 1;
 	}
@@ -231,13 +234,13 @@ static int wait_for_serirq_timeout(struct tpm_chip *chip, bool condition,
  */
 static void tpm_stm_i2c_cancel(struct tpm_chip *chip)
 {
-	struct i2c_client *client;
+	struct tpm_stm_dev *tpm_dev;
 	u8 data;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
 	data = TPM_STS_COMMAND_READY;
-	I2C_WRITE_DATA(client, TPM_STS, &data, 1);
+	I2C_WRITE_DATA(tpm_dev, TPM_STS, &data, 1);
 	if (chip->vendor.irq)
 		wait_for_serirq_timeout(chip, 1, chip->vendor.timeout_a);
 }	/* tpm_stm_i2c_cancel() */
@@ -249,12 +252,12 @@ static void tpm_stm_i2c_cancel(struct tpm_chip *chip)
  */
 static u8 tpm_stm_i2c_status(struct tpm_chip *chip)
 {
-	struct i2c_client *client;
+	struct tpm_stm_dev *tpm_dev;
 	u8 data;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
-	I2C_READ_DATA(client, TPM_STS, &data, 1);
+	I2C_READ_DATA(tpm_dev, TPM_STS, &data, 1);
 	return data;
 }				/* tpm_stm_i2c_status() */
 
@@ -266,13 +269,13 @@ static u8 tpm_stm_i2c_status(struct tpm_chip *chip)
  */
 static int check_locality(struct tpm_chip *chip)
 {
-	struct i2c_client *client;
+	struct tpm_stm_dev *tpm_dev;
 	u8 data;
 	u8 status;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
-	status = I2C_READ_DATA(client, TPM_ACCESS, &data, 1);
+	status = I2C_READ_DATA(tpm_dev, TPM_ACCESS, &data, 1);
 	if (status && (data &
 		(TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
 		(TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID))
@@ -291,16 +294,16 @@ static int request_locality(struct tpm_chip *chip)
 {
 	unsigned long stop;
 	long rc;
-	struct i2c_client *client;
+	struct tpm_stm_dev *tpm_dev;
 	u8 data;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
 	if (check_locality(chip) == chip->vendor.locality)
 		return chip->vendor.locality;
 
 	data = TPM_ACCESS_REQUEST_USE;
-	rc = I2C_WRITE_DATA(client, TPM_ACCESS, &data, 1);
+	rc = I2C_WRITE_DATA(tpm_dev, TPM_ACCESS, &data, 1);
 	if (rc < 0)
 		goto end;
 
@@ -329,13 +332,13 @@ end:
  */
 static void release_locality(struct tpm_chip *chip)
 {
-	struct i2c_client *client;
+	struct tpm_stm_dev *tpm_dev;
 	u8 data;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 	data = TPM_ACCESS_ACTIVE_LOCALITY;
 
-	I2C_WRITE_DATA(client, TPM_ACCESS, &data, 1);
+	I2C_WRITE_DATA(tpm_dev, TPM_ACCESS, &data, 1);
 }
 
 /*
@@ -348,19 +351,20 @@ static int get_burstcount(struct tpm_chip *chip)
 	unsigned long stop;
 	int burstcnt, status;
 	u8 tpm_reg, temp;
+	struct tpm_stm_dev *tpm_dev;
 
-	struct i2c_client *client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
 	stop = jiffies + chip->vendor.timeout_d;
 	do {
 		tpm_reg = TPM_STS + 1;
-		status = I2C_READ_DATA(client, tpm_reg, &temp, 1);
+		status = I2C_READ_DATA(tpm_dev, tpm_reg, &temp, 1);
 		if (status < 0)
 			goto end;
 
 		tpm_reg = tpm_reg + 1;
 		burstcnt = temp;
-		status = I2C_READ_DATA(client, tpm_reg, &temp, 1);
+		status = I2C_READ_DATA(tpm_dev, tpm_reg, &temp, 1);
 		if (status < 0)
 			goto end;
 
@@ -417,9 +421,9 @@ static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
 static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	int size = 0, burstcnt, len;
-	struct i2c_client *client;
+	struct tpm_stm_dev *tpm_dev;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
 	while (size < count &&
 	       wait_for_stat(chip,
@@ -431,7 +435,7 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 		if (burstcnt < 0)
 			return burstcnt;
 		len = min_t(int, burstcnt, count - size);
-		I2C_READ_DATA(client, TPM_DATA_FIFO, buf + size, len);
+		I2C_READ_DATA(tpm_dev, TPM_DATA_FIFO, buf + size, len);
 		size += len;
 	}
 	return size;
@@ -447,14 +451,14 @@ static irqreturn_t tpm_ioserirq_handler(int irq, void *dev_id)
 {
 	struct tpm_chip *chip = dev_id;
 	struct i2c_client *client;
-	struct st33zp24_platform_data *pin_infos;
+	struct tpm_stm_dev *tpm_dev;
 
 	disable_irq_nosync(irq);
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
-	pin_infos = client->dev.platform_data;
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
+	client = tpm_dev->client;
 
-	complete(&pin_infos->irq_detection);
+	complete(&tpm_dev->irq_detection);
 	return IRQ_HANDLED;
 } /* tpm_ioserirq_handler() */
 
@@ -476,13 +480,15 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 	int ret;
 	u8 data;
 	struct i2c_client *client;
+	struct tpm_stm_dev *tpm_dev;
 
 	if (chip == NULL)
 		return -EBUSY;
 	if (len < TPM_HEADER_SIZE)
 		return -EBUSY;
 
-	client = (struct i2c_client *)TPM_VPRIV(chip);
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
+	client = tpm_dev->client;
 
 	client->flags = 0;
 
@@ -506,7 +512,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 		if (burstcnt < 0)
 			return burstcnt;
 		size = min_t(int, len - i - 1, burstcnt);
-		ret = I2C_WRITE_DATA(client, TPM_DATA_FIFO, buf, size);
+		ret = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf, size);
 		if (ret < 0)
 			goto out_err;
 
@@ -519,7 +525,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 		goto out_err;
 	}
 
-	ret = I2C_WRITE_DATA(client, TPM_DATA_FIFO, buf + len - 1, 1);
+	ret = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf + len - 1, 1);
 	if (ret < 0)
 		goto out_err;
 
@@ -530,7 +536,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 	}
 
 	data = TPM_STS_GO;
-	I2C_WRITE_DATA(client, TPM_STS, &data, 1);
+	I2C_WRITE_DATA(tpm_dev, TPM_STS, &data, 1);
 
 	return len;
 out_err:
@@ -624,6 +630,7 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	u8 intmask;
 	struct tpm_chip *chip;
 	struct st33zp24_platform_data *platform_data;
+	struct tpm_stm_dev *tpm_dev;
 
 	if (client == NULL) {
 		pr_info("%s: i2c client is NULL. Device not accessible.\n",
@@ -638,11 +645,12 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		goto end;
 	}
 
-	chip = tpm_register_hardware(&client->dev, &st_i2c_tpm);
-	if (!chip) {
-		dev_info(&client->dev, "fail chip\n");
-		err = -ENODEV;
-		goto end;
+	tpm_dev = devm_kzalloc(&client->dev, sizeof(struct tpm_stm_dev),
+			       GFP_KERNEL);
+	if (!tpm_dev) {
+		dev_info(&client->dev, "cannot allocate memory for tpm data\n");
+		err = -ENOMEM;
+		goto _tpm_clean_answer;
 	}
 
 	platform_data = client->dev.platform_data;
@@ -653,20 +661,14 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		goto _tpm_clean_answer;
 	}
 
-	platform_data->tpm_i2c_buffer[0] =
-	    kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
-	if (platform_data->tpm_i2c_buffer[0] == NULL) {
-		err = -ENOMEM;
-		goto _tpm_clean_answer;
-	}
-	platform_data->tpm_i2c_buffer[1] =
-	    kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
-	if (platform_data->tpm_i2c_buffer[1] == NULL) {
-		err = -ENOMEM;
-		goto _tpm_clean_response1;
+	chip = tpm_register_hardware(&client->dev, &st_i2c_tpm);
+	if (!chip) {
+		dev_info(&client->dev, "fail chip\n");
+		return -ENODEV;
 	}
 
-	TPM_VPRIV(chip) = client;
+	TPM_VPRIV(chip) = tpm_dev;
+	tpm_dev->client = client;
 
 	chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
 	chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
@@ -683,16 +685,16 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	}
 
 	if (interrupts) {
-		init_completion(&platform_data->irq_detection);
+		init_completion(&tpm_dev->irq_detection);
 		if (request_locality(chip) != LOCALITY0) {
 			err = -ENODEV;
-			goto _tpm_clean_response2;
+			goto _tpm_clean_answer;
 		}
 		err = gpio_request(platform_data->io_serirq, "TPM IO_SERIRQ");
 		if (err)
 			goto _gpio_init2;
 
-		clear_interruption(client);
+		clear_interruption(tpm_dev);
 		err = request_irq(gpio_to_irq(platform_data->io_serirq),
 				&tpm_ioserirq_handler,
 				IRQF_TRIGGER_HIGH,
@@ -703,7 +705,7 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			goto _irq_set;
 		}
 
-		err = I2C_READ_DATA(client, TPM_INT_ENABLE, &intmask, 1);
+		err = I2C_READ_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
 		if (err < 0)
 			goto _irq_set;
 
@@ -714,16 +716,16 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			|  TPM_INTF_STS_VALID_INT
 			|  TPM_INTF_DATA_AVAIL_INT;
 
-		err = I2C_WRITE_DATA(client, TPM_INT_ENABLE, &intmask, 1);
+		err = I2C_WRITE_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
 		if (err < 0)
 			goto _irq_set;
 
 		intmask = TPM_GLOBAL_INT_ENABLE;
-		err = I2C_WRITE_DATA(client, (TPM_INT_ENABLE + 3), &intmask, 1);
+		err = I2C_WRITE_DATA(tpm_dev, (TPM_INT_ENABLE + 3), &intmask, 1);
 		if (err < 0)
 			goto _irq_set;
 
-		err = I2C_READ_DATA(client, TPM_INT_STATUS, &intmask, 1);
+		err = I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &intmask, 1);
 		if (err < 0)
 			goto _irq_set;
 
@@ -745,12 +747,6 @@ _gpio_init2:
 _gpio_init1:
 	if (power_mgt)
 		gpio_free(platform_data->io_lpcpd);
-_tpm_clean_response2:
-	kzfree(platform_data->tpm_i2c_buffer[1]);
-	platform_data->tpm_i2c_buffer[1] = NULL;
-_tpm_clean_response1:
-	kzfree(platform_data->tpm_i2c_buffer[0]);
-	platform_data->tpm_i2c_buffer[0] = NULL;
 _tpm_clean_answer:
 	tpm_remove_hardware(chip->dev);
 end:
@@ -766,28 +762,12 @@ end:
  */
 static int tpm_st33_i2c_remove(struct i2c_client *client)
 {
-	struct tpm_chip *chip = (struct tpm_chip *)i2c_get_clientdata(client);
-	struct st33zp24_platform_data *pin_infos =
-		((struct i2c_client *)TPM_VPRIV(chip))->dev.platform_data;
-
-	if (pin_infos != NULL) {
-		free_irq(pin_infos->io_serirq, chip);
-
-		gpio_free(pin_infos->io_serirq);
-		gpio_free(pin_infos->io_lpcpd);
+	struct tpm_chip *chip =
+		(struct tpm_chip *) i2c_get_clientdata(client);
 
+	if (chip)
 		tpm_remove_hardware(chip->dev);
 
-		if (pin_infos->tpm_i2c_buffer[1] != NULL) {
-			kzfree(pin_infos->tpm_i2c_buffer[1]);
-			pin_infos->tpm_i2c_buffer[1] = NULL;
-		}
-		if (pin_infos->tpm_i2c_buffer[0] != NULL) {
-			kzfree(pin_infos->tpm_i2c_buffer[0]);
-			pin_infos->tpm_i2c_buffer[0] = NULL;
-		}
-	}
-
 	return 0;
 }
 
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.h b/drivers/char/tpm/tpm_i2c_stm_st33.h
deleted file mode 100644
index 3041271..0000000
--- a/drivers/char/tpm/tpm_i2c_stm_st33.h
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
- * STMicroelectronics TPM I2C Linux driver for TPM ST33ZP24
- * Copyright (C) 2009, 2010  STMicroelectronics
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- * STMicroelectronics version 1.2.0, Copyright (C) 2010
- * STMicroelectronics comes with ABSOLUTELY NO WARRANTY.
- * This is free software, and you are welcome to redistribute it
- * under certain conditions.
- *
- * @Author: Christophe RICARD tpmsupport-qxv4g6HH51o@public.gmane.org
- *
- * @File: stm_st33_tpm_i2c.h
- *
- * @Date: 09/15/2010
- */
-#ifndef __STM_ST33_TPM_I2C_MAIN_H__
-#define __STM_ST33_TPM_I2C_MAIN_H__
-
-#define TPM_ST33_I2C			"st33zp24_i2c"
-
-struct st33zp24_platform_data {
-	int io_serirq;
-	int io_lpcpd;
-	struct i2c_client *client;
-	u8 *tpm_i2c_buffer[2]; /* 0 Request 1 Response */
-	struct completion irq_detection;
-	struct mutex lock;
-};
-
-#endif /* __STM_ST33_TPM_I2C_MAIN_H__ */
diff --git a/include/linux/platform_data/tpm_i2c_stm_st33.h b/include/linux/platform_data/tpm_i2c_stm_st33.h
new file mode 100644
index 0000000..c5e4b7f
--- /dev/null
+++ b/include/linux/platform_data/tpm_i2c_stm_st33.h
@@ -0,0 +1,41 @@
+/*
+ * STMicroelectronics TPM I2C Linux driver for TPM ST33ZP24
+ * Copyright (C) 2009, 2010  STMicroelectronics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * STMicroelectronics version 1.2.0, Copyright (C) 2010
+ * STMicroelectronics comes with ABSOLUTELY NO WARRANTY.
+ * This is free software, and you are welcome to redistribute it
+ * under certain conditions.
+ *
+ * @Author: Christophe RICARD tpmsupport-qxv4g6HH51o@public.gmane.org
+ *
+ * @File: stm_st33_tpm_i2c.h
+ *
+ * @Date: 09/15/2010
+ */
+#ifndef __STM_ST33_TPM_I2C_MAIN_H__
+#define __STM_ST33_TPM_I2C_MAIN_H__
+
+
+#define TPM_ST33_I2C			"st33zp24_i2c"
+
+struct st33zp24_platform_data {
+	int io_serirq;
+	int io_lpcpd;
+};
+
+#endif /* __STM_ST33_TPM_I2C_MAIN_H__ */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 05/15] tpm/tpm_i2c_stm_st33: Remove reference to io_serirq

[Christophe Ricard]

The serirq gpio pin is used only as interrupt. After driver initialization,
the serirq signal is always used through interrupt and never with gpio
kernel API.

The irq can then be initialized during the platform_data definition within the client->irq pin.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c            | 13 +++----------
 include/linux/platform_data/tpm_i2c_stm_st33.h |  1 -
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 07ce463..71ef1da 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -104,7 +104,6 @@ struct tpm_stm_dev {
 	struct completion irq_detection;
 	struct tpm_chip *chip;
 	u8 buf[TPM_BUFSIZE + 1];
-	int io_serirq;
 	int io_lpcpd;
 };
 
@@ -690,18 +689,15 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			err = -ENODEV;
 			goto _tpm_clean_answer;
 		}
-		err = gpio_request(platform_data->io_serirq, "TPM IO_SERIRQ");
-		if (err)
-			goto _gpio_init2;
 
 		clear_interruption(tpm_dev);
-		err = request_irq(gpio_to_irq(platform_data->io_serirq),
+		err = request_irq(client->irq,
 				&tpm_ioserirq_handler,
 				IRQF_TRIGGER_HIGH,
 				"TPM SERIRQ management", chip);
 		if (err < 0) {
 			dev_err(chip->dev , "TPM SERIRQ signals %d not available\n",
-				gpio_to_irq(platform_data->io_serirq));
+				client->irq);
 			goto _irq_set;
 		}
 
@@ -740,10 +736,7 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	dev_info(chip->dev, "TPM I2C Initialized\n");
 	return 0;
 _irq_set:
-	free_irq(gpio_to_irq(platform_data->io_serirq), (void *)chip);
-_gpio_init2:
-	if (interrupts)
-		gpio_free(platform_data->io_serirq);
+	free_irq(client->irq, (void *)chip);
 _gpio_init1:
 	if (power_mgt)
 		gpio_free(platform_data->io_lpcpd);
diff --git a/include/linux/platform_data/tpm_i2c_stm_st33.h b/include/linux/platform_data/tpm_i2c_stm_st33.h
index c5e4b7f..ec698f2 100644
--- a/include/linux/platform_data/tpm_i2c_stm_st33.h
+++ b/include/linux/platform_data/tpm_i2c_stm_st33.h
@@ -34,7 +34,6 @@
 #define TPM_ST33_I2C			"st33zp24_i2c"
 
 struct st33zp24_platform_data {
-	int io_serirq;
 	int io_lpcpd;
 };
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 06/15] tpm/tpm_i2c_stm_st33: Replace err/rc/ret by r for a function return code

[Christophe Ricard]

Some functions return err, rc or ret for a status code.

Return r instead for all of them.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 97 ++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 49 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 71ef1da..4997eff 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -292,7 +292,7 @@ static int check_locality(struct tpm_chip *chip)
 static int request_locality(struct tpm_chip *chip)
 {
 	unsigned long stop;
-	long rc;
+	long r;
 	struct tpm_stm_dev *tpm_dev;
 	u8 data;
 
@@ -302,15 +302,15 @@ static int request_locality(struct tpm_chip *chip)
 		return chip->vendor.locality;
 
 	data = TPM_ACCESS_REQUEST_USE;
-	rc = I2C_WRITE_DATA(tpm_dev, TPM_ACCESS, &data, 1);
-	if (rc < 0)
+	r = I2C_WRITE_DATA(tpm_dev, TPM_ACCESS, &data, 1);
+	if (r < 0)
 		goto end;
 
 	if (chip->vendor.irq) {
-		rc = wait_for_serirq_timeout(chip, (check_locality
+		r = wait_for_serirq_timeout(chip, (check_locality
 						       (chip) >= 0),
 						      chip->vendor.timeout_a);
-		if (rc > 0)
+		if (r > 0)
 			return chip->vendor.locality;
 	} else {
 		stop = jiffies + chip->vendor.timeout_a;
@@ -320,9 +320,9 @@ static int request_locality(struct tpm_chip *chip)
 			msleep(TPM_TIMEOUT);
 		} while (time_before(jiffies, stop));
 	}
-	rc = -EACCES;
+	r = -EACCES;
 end:
-	return rc;
+	return r;
 } /* request_locality() */
 
 /*
@@ -389,14 +389,14 @@ static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
 			 wait_queue_head_t *queue)
 {
 	unsigned long stop;
-	long rc;
+	long r;
 	u8 status;
 
 	 if (chip->vendor.irq) {
-		rc = wait_for_serirq_timeout(chip, ((tpm_stm_i2c_status
+		r = wait_for_serirq_timeout(chip, ((tpm_stm_i2c_status
 							(chip) & mask) ==
 						       mask), timeout);
-		if (rc > 0)
+		if (r > 0)
 			return 0;
 	} else {
 		stop = jiffies + timeout;
@@ -476,7 +476,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 {
 	u32 status, i, size;
 	int burstcnt = 0;
-	int ret;
+	int r;
 	u8 data;
 	struct i2c_client *client;
 	struct tpm_stm_dev *tpm_dev;
@@ -491,9 +491,9 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 
 	client->flags = 0;
 
-	ret = request_locality(chip);
-	if (ret < 0)
-		return ret;
+	r = request_locality(chip);
+	if (r < 0)
+		return r;
 
 	status = tpm_stm_i2c_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
@@ -501,7 +501,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 		if (wait_for_stat
 		    (chip, TPM_STS_COMMAND_READY, chip->vendor.timeout_b,
 		     &chip->vendor.int_queue) < 0) {
-			ret = -ETIME;
+			r = -ETIME;
 			goto out_err;
 		}
 	}
@@ -511,8 +511,8 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 		if (burstcnt < 0)
 			return burstcnt;
 		size = min_t(int, len - i - 1, burstcnt);
-		ret = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf, size);
-		if (ret < 0)
+		r = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf, size);
+		if (r < 0)
 			goto out_err;
 
 		i += size;
@@ -520,17 +520,17 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 
 	status = tpm_stm_i2c_status(chip);
 	if ((status & TPM_STS_DATA_EXPECT) == 0) {
-		ret = -EIO;
+		r = -EIO;
 		goto out_err;
 	}
 
-	ret = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf + len - 1, 1);
-	if (ret < 0)
+	r = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf + len - 1, 1);
+	if (r < 0)
 		goto out_err;
 
 	status = tpm_stm_i2c_status(chip);
 	if ((status & TPM_STS_DATA_EXPECT) != 0) {
-		ret = -EIO;
+		r = -EIO;
 		goto out_err;
 	}
 
@@ -541,7 +541,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 out_err:
 	tpm_stm_i2c_cancel(chip);
 	release_locality(chip);
-	return ret;
+	return r;
 }
 
 /*
@@ -625,7 +625,7 @@ MODULE_PARM_DESC(power_mgt, "Power Management");
 static int
 tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
-	int err;
+	int r;
 	u8 intmask;
 	struct tpm_chip *chip;
 	struct st33zp24_platform_data *platform_data;
@@ -634,13 +634,13 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (client == NULL) {
 		pr_info("%s: i2c client is NULL. Device not accessible.\n",
 			__func__);
-		err = -ENODEV;
+		r = -ENODEV;
 		goto end;
 	}
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_info(&client->dev, "client not i2c capable\n");
-		err = -ENODEV;
+		r = -ENODEV;
 		goto end;
 	}
 
@@ -648,7 +648,7 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			       GFP_KERNEL);
 	if (!tpm_dev) {
 		dev_info(&client->dev, "cannot allocate memory for tpm data\n");
-		err = -ENOMEM;
+		r = -ENOMEM;
 		goto _tpm_clean_answer;
 	}
 
@@ -656,7 +656,7 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	if (!platform_data) {
 		dev_info(&client->dev, "chip not available\n");
-		err = -ENODEV;
+		r = -ENODEV;
 		goto _tpm_clean_answer;
 	}
 
@@ -677,8 +677,8 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	chip->vendor.locality = LOCALITY0;
 
 	if (power_mgt) {
-		err = gpio_request(platform_data->io_lpcpd, "TPM IO_LPCPD");
-		if (err)
+		r = gpio_request(platform_data->io_lpcpd, "TPM IO_LPCPD");
+		if (r)
 			goto _gpio_init1;
 		gpio_set_value(platform_data->io_lpcpd, 1);
 	}
@@ -686,7 +686,7 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (interrupts) {
 		init_completion(&tpm_dev->irq_detection);
 		if (request_locality(chip) != LOCALITY0) {
-			err = -ENODEV;
+			r = -ENODEV;
 			goto _tpm_clean_answer;
 		}
 
@@ -695,14 +695,14 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 				&tpm_ioserirq_handler,
 				IRQF_TRIGGER_HIGH,
 				"TPM SERIRQ management", chip);
-		if (err < 0) {
+		if (r < 0) {
 			dev_err(chip->dev , "TPM SERIRQ signals %d not available\n",
 				client->irq);
 			goto _irq_set;
 		}
 
-		err = I2C_READ_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
-		if (err < 0)
+		r = I2C_READ_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
+		if (r < 0)
 			goto _irq_set;
 
 		intmask |= TPM_INTF_CMD_READY_INT
@@ -712,17 +712,17 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			|  TPM_INTF_STS_VALID_INT
 			|  TPM_INTF_DATA_AVAIL_INT;
 
-		err = I2C_WRITE_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
-		if (err < 0)
+		r = I2C_WRITE_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
+		if (r < 0)
 			goto _irq_set;
 
 		intmask = TPM_GLOBAL_INT_ENABLE;
-		err = I2C_WRITE_DATA(tpm_dev, (TPM_INT_ENABLE + 3), &intmask, 1);
-		if (err < 0)
+		r = I2C_WRITE_DATA(tpm_dev, (TPM_INT_ENABLE + 3), &intmask, 1);
+		if (r < 0)
 			goto _irq_set;
 
-		err = I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &intmask, 1);
-		if (err < 0)
+		r = I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &intmask, 1);
+		if (r < 0)
 			goto _irq_set;
 
 		chip->vendor.irq = interrupts;
@@ -744,7 +744,7 @@ _tpm_clean_answer:
 	tpm_remove_hardware(chip->dev);
 end:
 	pr_info("TPM I2C initialisation fail\n");
-	return err;
+	return r;
 }
 
 /*
@@ -774,14 +774,13 @@ static int tpm_st33_i2c_remove(struct i2c_client *client)
 static int tpm_st33_i2c_pm_suspend(struct device *dev)
 {
 	struct st33zp24_platform_data *pin_infos = dev->platform_data;
-	int ret = 0;
+	int r = 0;
 
 	if (power_mgt)
 		gpio_set_value(pin_infos->io_lpcpd, 0);
 	else
-		ret = tpm_pm_suspend(dev);
-
-	return ret;
+		r = tpm_pm_suspend(dev);
+	return r;
 }				/* tpm_st33_i2c_suspend() */
 
 /*
@@ -794,20 +793,20 @@ static int tpm_st33_i2c_pm_resume(struct device *dev)
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 	struct st33zp24_platform_data *pin_infos = dev->platform_data;
 
-	int ret = 0;
+	int r = 0;
 
 	if (power_mgt) {
 		gpio_set_value(pin_infos->io_lpcpd, 1);
-		ret = wait_for_serirq_timeout(chip,
+		r = wait_for_serirq_timeout(chip,
 					  (chip->ops->status(chip) &
 					  TPM_STS_VALID) == TPM_STS_VALID,
 					  chip->vendor.timeout_b);
 	} else {
-		ret = tpm_pm_resume(dev);
-		if (!ret)
+		r = tpm_pm_resume(dev);
+		if (!r)
 			tpm_do_selftest(chip);
 	}
-	return ret;
+	return r;
 }				/* tpm_st33_i2c_pm_resume() */
 #endif
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 07/15] tpm/tpm_i2c_stm_st33: Replace tpm_st33_* function with tpm_stm_*

[Christophe Ricard]

For sanity, replace every tpm_st33_* with tpm_stm_*

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 50 ++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 4997eff..fdcdf9b 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -225,7 +225,7 @@ static int wait_for_serirq_timeout(struct tpm_chip *chip, bool condition,
 			status = 1;
 	}
 	return status;
-}
+} /* wait_for_serirq_timeout() */
 
 /*
  * tpm_stm_i2c_cancel, cancel is not implemented.
@@ -242,7 +242,7 @@ static void tpm_stm_i2c_cancel(struct tpm_chip *chip)
 	I2C_WRITE_DATA(tpm_dev, TPM_STS, &data, 1);
 	if (chip->vendor.irq)
 		wait_for_serirq_timeout(chip, 1, chip->vendor.timeout_a);
-}	/* tpm_stm_i2c_cancel() */
+} /* tpm_stm_i2c_cancel() */
 
 /*
  * tpm_stm_spi_status return the TPM_STS register
@@ -258,7 +258,7 @@ static u8 tpm_stm_i2c_status(struct tpm_chip *chip)
 
 	I2C_READ_DATA(tpm_dev, TPM_STS, &data, 1);
 	return data;
-}				/* tpm_stm_i2c_status() */
+} /* tpm_stm_i2c_status() */
 
 
 /*
@@ -592,7 +592,7 @@ out:
 	return size;
 }
 
-static bool tpm_st33_i2c_req_canceled(struct tpm_chip *chip, u8 status)
+static bool tpm_stm_i2c_req_canceled(struct tpm_chip *chip, u8 status)
 {
 	return (status == TPM_STS_COMMAND_READY);
 }
@@ -604,7 +604,7 @@ static const struct tpm_class_ops st_i2c_tpm = {
 	.status = tpm_stm_i2c_status,
 	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
-	.req_canceled = tpm_st33_i2c_req_canceled,
+	.req_canceled = tpm_stm_i2c_req_canceled,
 };
 
 static int interrupts;
@@ -616,14 +616,14 @@ module_param(power_mgt, int, 0444);
 MODULE_PARM_DESC(power_mgt, "Power Management");
 
 /*
- * tpm_st33_i2c_probe initialize the TPM device
+ * tpm_stm_i2c_probe initialize the TPM device
  * @param: client, the i2c_client drescription (TPM I2C description).
  * @param: id, the i2c_device_id struct.
  * @return: 0 in case of success.
  *	 -1 in other case.
  */
 static int
-tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
+tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	int r;
 	u8 intmask;
@@ -748,12 +748,12 @@ end:
 }
 
 /*
- * tpm_st33_i2c_remove remove the TPM device
+ * tpm_stm_i2c_remove remove the TPM device
  * @param: client, the i2c_client drescription (TPM I2C description).
 		clear_bit(0, &chip->is_open);
  * @return: 0 in case of success.
  */
-static int tpm_st33_i2c_remove(struct i2c_client *client)
+static int tpm_stm_i2c_remove(struct i2c_client *client)
 {
 	struct tpm_chip *chip =
 		(struct tpm_chip *) i2c_get_clientdata(client);
@@ -766,12 +766,12 @@ static int tpm_st33_i2c_remove(struct i2c_client *client)
 
 #ifdef CONFIG_PM_SLEEP
 /*
- * tpm_st33_i2c_pm_suspend suspend the TPM device
+ * tpm_stm_i2c_pm_suspend suspend the TPM device
  * @param: client, the i2c_client drescription (TPM I2C description).
  * @param: mesg, the power management message.
  * @return: 0 in case of success.
  */
-static int tpm_st33_i2c_pm_suspend(struct device *dev)
+static int tpm_stm_i2c_pm_suspend(struct device *dev)
 {
 	struct st33zp24_platform_data *pin_infos = dev->platform_data;
 	int r = 0;
@@ -781,14 +781,14 @@ static int tpm_st33_i2c_pm_suspend(struct device *dev)
 	else
 		r = tpm_pm_suspend(dev);
 	return r;
-}				/* tpm_st33_i2c_suspend() */
+} /* tpm_stm_i2c_suspend() */
 
 /*
- * tpm_st33_i2c_pm_resume resume the TPM device
+ * tpm_stm_i2c_pm_resume resume the TPM device
  * @param: client, the i2c_client drescription (TPM I2C description).
  * @return: 0 in case of success.
  */
-static int tpm_st33_i2c_pm_resume(struct device *dev)
+static int tpm_stm_i2c_pm_resume(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 	struct st33zp24_platform_data *pin_infos = dev->platform_data;
@@ -807,28 +807,28 @@ static int tpm_st33_i2c_pm_resume(struct device *dev)
 			tpm_do_selftest(chip);
 	}
 	return r;
-}				/* tpm_st33_i2c_pm_resume() */
+} /* tpm_stm_i2c_pm_resume() */
 #endif
 
-static const struct i2c_device_id tpm_st33_i2c_id[] = {
+static const struct i2c_device_id tpm_stm_i2c_id[] = {
 	{TPM_ST33_I2C, 0},
 	{}
 };
-MODULE_DEVICE_TABLE(i2c, tpm_st33_i2c_id);
-static SIMPLE_DEV_PM_OPS(tpm_st33_i2c_ops, tpm_st33_i2c_pm_suspend,
-	tpm_st33_i2c_pm_resume);
-static struct i2c_driver tpm_st33_i2c_driver = {
+MODULE_DEVICE_TABLE(i2c, tpm_stm_i2c_id);
+static SIMPLE_DEV_PM_OPS(tpm_stm_i2c_ops, tpm_stm_i2c_pm_suspend,
+	tpm_stm_i2c_pm_resume);
+static struct i2c_driver tpm_stm_i2c_driver = {
 	.driver = {
 		   .owner = THIS_MODULE,
 		   .name = TPM_ST33_I2C,
-		   .pm = &tpm_st33_i2c_ops,
+		   .pm = &tpm_stm_i2c_ops,
 		   },
-	.probe = tpm_st33_i2c_probe,
-	.remove = tpm_st33_i2c_remove,
-	.id_table = tpm_st33_i2c_id
+	.probe = tpm_stm_i2c_probe,
+	.remove = tpm_stm_i2c_remove,
+	.id_table = tpm_stm_i2c_id
 };
 
-module_i2c_driver(tpm_st33_i2c_driver);
+module_i2c_driver(tpm_stm_i2c_driver);
 
 MODULE_AUTHOR("Christophe Ricard (tpmsupport-qxv4g6HH51o@public.gmane.org)");
 MODULE_DESCRIPTION("STM TPM I2C ST33 Driver");
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 08/15] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Christophe Ricard]

Add tpm_stm_st33_i2c dts structure keeping backward compatibility
with static platform_data support as well.
In the mean time the code is made much simpler by:
- Moving all gpio_request to devm_gpio_request_one primitive
- Moving request_irq to devm_request_threaded_irq

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 173 +++++++++++++++++++++++++-----------
 1 file changed, 122 insertions(+), 51 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index fdcdf9b..6980708 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -47,6 +47,10 @@
 #include <linux/uaccess.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#ifdef CONFIG_OF
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#endif
 
 #include <linux/platform_data/tpm_i2c_stm_st33.h>
 #include "tpm.h"
@@ -607,13 +611,78 @@ static const struct tpm_class_ops st_i2c_tpm = {
 	.req_canceled = tpm_stm_i2c_req_canceled,
 };
 
-static int interrupts;
-module_param(interrupts, int, 0444);
-MODULE_PARM_DESC(interrupts, "Enable interrupts");
+#ifdef CONFIG_OF
+static int tpm_stm_i2c_of_request_resources(struct tpm_chip *chip)
+{
+	struct device_node *pp;
+	struct tpm_stm_dev *tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
+	struct i2c_client *client = tpm_dev->client;
+
+	int gpio;
+	int r;
+
+	pp = client->dev.of_node;
+	if (!pp)
+		return -ENODEV;
+
+	/* Get GPIO from device tree */
+	gpio = of_get_named_gpio(pp, "lpcpd-gpios", 0);
+	if (gpio < 0) {
+		pr_err("Failed to retrieve lpcpd-gpios from dts.\n");
+		tpm_dev->io_lpcpd = -1;
+		/*
+		 * lpcpd pin is not specified. This is not an issue as
+		 * power management can be also managed by TPM specific
+		 * commands. So leave with a success status code.
+		 */
+		return 0;
+	}
+	/* GPIO request and configuration */
+	r = devm_gpio_request_one(&client->dev, gpio,
+			GPIOF_OUT_INIT_HIGH, "TPM IO LPCPD");
+	if (r) {
+		pr_err("Failed to request lpcpd pin\n");
+		return -ENODEV;
+	}
+	tpm_dev->io_lpcpd = gpio;
+
+	return 0;
+}
+#else
+static int tpm_stm_i2c_of_request_resources(struct i2c_client *client)
+{
+	return -ENODEV;
+}
+#endif
+
+static int tpm_stm_i2c_request_resources(struct i2c_client *client,
+					 struct tpm_chip *chip)
+{
+	struct st33zp24_platform_data *pdata;
+	struct tpm_stm_dev *tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
+	int r;
+
+	pdata = client->dev.platform_data;
+	if (pdata == NULL) {
+		pr_err("No platform data\n");
+		return -EINVAL;
+	}
 
-static int power_mgt = 1;
-module_param(power_mgt, int, 0444);
-MODULE_PARM_DESC(power_mgt, "Power Management");
+	/* store for late use */
+	tpm_dev->io_lpcpd = pdata->io_lpcpd;
+
+	if (gpio_is_valid(pdata->io_lpcpd)) {
+		r = devm_gpio_request_one(&client->dev,
+				pdata->io_lpcpd, GPIOF_OUT_INIT_HIGH,
+				"TPM IO_LPCPD");
+		if (r) {
+			pr_err("%s : reset gpio_request failed\n", __FILE__);
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
 
 /*
  * tpm_stm_i2c_probe initialize the TPM device
@@ -634,30 +703,19 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (client == NULL) {
 		pr_info("%s: i2c client is NULL. Device not accessible.\n",
 			__func__);
-		r = -ENODEV;
-		goto end;
+		return -ENODEV;
 	}
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_info(&client->dev, "client not i2c capable\n");
-		r = -ENODEV;
-		goto end;
+		return -ENODEV;
 	}
 
 	tpm_dev = devm_kzalloc(&client->dev, sizeof(struct tpm_stm_dev),
 			       GFP_KERNEL);
 	if (!tpm_dev) {
 		dev_info(&client->dev, "cannot allocate memory for tpm data\n");
-		r = -ENOMEM;
-		goto _tpm_clean_answer;
-	}
-
-	platform_data = client->dev.platform_data;
-
-	if (!platform_data) {
-		dev_info(&client->dev, "chip not available\n");
-		r = -ENODEV;
-		goto _tpm_clean_answer;
+		return -ENOMEM;
 	}
 
 	chip = tpm_register_hardware(&client->dev, &st_i2c_tpm);
@@ -669,6 +727,25 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	TPM_VPRIV(chip) = tpm_dev;
 	tpm_dev->client = client;
 
+	platform_data = client->dev.platform_data;
+	if (!platform_data && client->dev.of_node) {
+		r = tpm_stm_i2c_of_request_resources(chip);
+		if (r) {
+			pr_err("No platform data\n");
+			goto _tpm_clean_answer;
+		}
+	} else if (platform_data) {
+		r = tpm_stm_i2c_request_resources(client, chip);
+		if (r) {
+			pr_err("Cannot get platform resources\n");
+			goto _tpm_clean_answer;
+		}
+	} else {
+		pr_err("tpm_stm_st33 platform resources not available\n");
+		goto _tpm_clean_answer;
+	}
+
+
 	chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
 	chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
 	chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
@@ -676,14 +753,7 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	chip->vendor.locality = LOCALITY0;
 
-	if (power_mgt) {
-		r = gpio_request(platform_data->io_lpcpd, "TPM IO_LPCPD");
-		if (r)
-			goto _gpio_init1;
-		gpio_set_value(platform_data->io_lpcpd, 1);
-	}
-
-	if (interrupts) {
+	if (client->irq) {
 		init_completion(&tpm_dev->irq_detection);
 		if (request_locality(chip) != LOCALITY0) {
 			r = -ENODEV;
@@ -691,41 +761,38 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		}
 
 		clear_interruption(tpm_dev);
-		err = request_irq(client->irq,
-				&tpm_ioserirq_handler,
-				IRQF_TRIGGER_HIGH,
+		r = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+				tpm_ioserirq_handler,
+				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
 				"TPM SERIRQ management", chip);
 		if (r < 0) {
 			dev_err(chip->dev , "TPM SERIRQ signals %d not available\n",
 				client->irq);
-			goto _irq_set;
+			goto _tpm_clean_answer;
 		}
 
 		r = I2C_READ_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
 		if (r < 0)
-			goto _irq_set;
+			goto _tpm_clean_answer;
 
 		intmask |= TPM_INTF_CMD_READY_INT
-			|  TPM_INTF_FIFO_AVALAIBLE_INT
-			|  TPM_INTF_WAKE_UP_READY_INT
-			|  TPM_INTF_LOCALITY_CHANGE_INT
 			|  TPM_INTF_STS_VALID_INT
 			|  TPM_INTF_DATA_AVAIL_INT;
 
 		r = I2C_WRITE_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
 		if (r < 0)
-			goto _irq_set;
+			goto _tpm_clean_answer;
 
 		intmask = TPM_GLOBAL_INT_ENABLE;
 		r = I2C_WRITE_DATA(tpm_dev, (TPM_INT_ENABLE + 3), &intmask, 1);
 		if (r < 0)
-			goto _irq_set;
+			goto _tpm_clean_answer;
 
 		r = I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &intmask, 1);
 		if (r < 0)
-			goto _irq_set;
+			goto _tpm_clean_answer;
 
-		chip->vendor.irq = interrupts;
+		chip->vendor.irq = client->irq;
 
 		tpm_gen_interrupt(chip);
 	}
@@ -735,14 +802,8 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	dev_info(chip->dev, "TPM I2C Initialized\n");
 	return 0;
-_irq_set:
-	free_irq(client->irq, (void *)chip);
-_gpio_init1:
-	if (power_mgt)
-		gpio_free(platform_data->io_lpcpd);
 _tpm_clean_answer:
 	tpm_remove_hardware(chip->dev);
-end:
 	pr_info("TPM I2C initialisation fail\n");
 	return r;
 }
@@ -776,7 +837,7 @@ static int tpm_stm_i2c_pm_suspend(struct device *dev)
 	struct st33zp24_platform_data *pin_infos = dev->platform_data;
 	int r = 0;
 
-	if (power_mgt)
+	if (gpio_is_valid(pin_infos->io_lpcpd))
 		gpio_set_value(pin_infos->io_lpcpd, 0);
 	else
 		r = tpm_pm_suspend(dev);
@@ -795,7 +856,7 @@ static int tpm_stm_i2c_pm_resume(struct device *dev)
 
 	int r = 0;
 
-	if (power_mgt) {
+	if (gpio_is_valid(pin_infos->io_lpcpd)) {
 		gpio_set_value(pin_infos->io_lpcpd, 1);
 		r = wait_for_serirq_timeout(chip,
 					  (chip->ops->status(chip) &
@@ -814,14 +875,24 @@ static const struct i2c_device_id tpm_stm_i2c_id[] = {
 	{TPM_ST33_I2C, 0},
 	{}
 };
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_st33zp24_i2c_match[] = {
+	{ .compatible = "st,st33zp24_i2c", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_st33zp24_i2c_match);
+#endif
+
 MODULE_DEVICE_TABLE(i2c, tpm_stm_i2c_id);
 static SIMPLE_DEV_PM_OPS(tpm_stm_i2c_ops, tpm_stm_i2c_pm_suspend,
 	tpm_stm_i2c_pm_resume);
 static struct i2c_driver tpm_stm_i2c_driver = {
 	.driver = {
-		   .owner = THIS_MODULE,
-		   .name = TPM_ST33_I2C,
-		   .pm = &tpm_stm_i2c_ops,
+		.owner = THIS_MODULE,
+		.name = TPM_ST33_I2C,
+		.pm = &tpm_stm_i2c_ops,
+		.of_match_table = of_match_ptr(of_st33zp24_i2c_match),
 		   },
 	.probe = tpm_stm_i2c_probe,
 	.remove = tpm_stm_i2c_remove,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 09/15] tpm/tpm_i2c_stm_st33/dts/st33zp24_i2c: Add DTS Documentation

[Christophe Ricard]

st33zp24 tpm can be seen as a trivial i2c device as other i2c tpm.
However several other properties needs to be documented such as lpcpd.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 .../devicetree/bindings/security/tpm/st33zp24.txt  | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/st33zp24.txt

diff --git a/Documentation/devicetree/bindings/security/tpm/st33zp24.txt b/Documentation/devicetree/bindings/security/tpm/st33zp24.txt
new file mode 100644
index 0000000..eb48222
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/st33zp24.txt
@@ -0,0 +1,36 @@
+* STMicroelectronics SAS. ST33ZP24 TPM SoC
+
+Required properties:
+- compatible: Should be "st,st33zp24_i2c".
+- clock-frequency: I²C work frequency.
+- reg: address on the bus
+
+Optional ST33ZP24 Properties:
+- interrupt-parent: phandle for the interrupt gpio controller
+- interrupts: GPIO interrupt to which the chip is connected
+- lpcpd-gpios: Output GPIO pin used for ST33ZP24 power management D1/D2 state.
+If set vps must present when the platform is going into sleep/hibernate mode.
+
+Optional SoC Specific Properties:
+- pinctrl-names: Contains only one value - "default".
+- pintctrl-0: Specifies the pin control groups used for this controller.
+
+Example (for ARM-based BeagleBoard xM with ST33ZP24 on I2C2):
+
+&i2c2 {
+
+        status = "okay";
+
+        st33zp24: st33zp24@13 {
+
+                compatible = "st,st33zp24_i2c";
+
+                reg = <0x013>;
+                clock-frequency = <400000>;
+
+                interrupt-parent = <&gpio5>;
+                interrupts = <7 IRQ_TYPE_LEVEL_HIGH>;
+
+                lpcpd-gpios = <&gpio5 15 GPIO_ACTIVE_HIGH>;
+        };
+};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 10/15] tpm/tpm_i2c_stm_st33: Few code cleanup

[Christophe Ricard]

Cleanup code indentation, braces, test variable when NULL.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 6980708..cc5aef3 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -27,7 +27,7 @@
  *
  * @Synopsis:
  *	09/15/2010:	First shot driver tpm_tis driver for
-			 lpc is used as model.
+ *			 lpc is used as model.
  */
 
 #include <linux/pci.h>
@@ -396,7 +396,7 @@ static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
 	long r;
 	u8 status;
 
-	 if (chip->vendor.irq) {
+	if (chip->vendor.irq) {
 		r = wait_for_serirq_timeout(chip, ((tpm_stm_i2c_status
 							(chip) & mask) ==
 						       mask), timeout);
@@ -432,8 +432,7 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 	       wait_for_stat(chip,
 			     TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 			     chip->vendor.timeout_c,
-			     &chip->vendor.read_queue)
-	       == 0) {
+			     &chip->vendor.read_queue) == 0) {
 		burstcnt = get_burstcount(chip);
 		if (burstcnt < 0)
 			return burstcnt;
@@ -485,7 +484,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 	struct i2c_client *client;
 	struct tpm_stm_dev *tpm_dev;
 
-	if (chip == NULL)
+	if (!chip)
 		return -EBUSY;
 	if (len < TPM_HEADER_SIZE)
 		return -EBUSY;
@@ -562,7 +561,7 @@ static int tpm_stm_i2c_recv(struct tpm_chip *chip, unsigned char *buf,
 	int size = 0;
 	int expected;
 
-	if (chip == NULL)
+	if (!chip)
 		return -EBUSY;
 
 	if (count < TPM_HEADER_SIZE) {
@@ -583,7 +582,7 @@ static int tpm_stm_i2c_recv(struct tpm_chip *chip, unsigned char *buf,
 	}
 
 	size += recv_data(chip, &buf[TPM_HEADER_SIZE],
-					expected - TPM_HEADER_SIZE);
+			expected - TPM_HEADER_SIZE);
 	if (size < expected) {
 		dev_err(chip->dev, "Unable to read remainder of result\n");
 		size = -ETIME;
@@ -663,7 +662,7 @@ static int tpm_stm_i2c_request_resources(struct i2c_client *client,
 	int r;
 
 	pdata = client->dev.platform_data;
-	if (pdata == NULL) {
+	if (!pdata) {
 		pr_err("No platform data\n");
 		return -EINVAL;
 	}
@@ -695,12 +694,12 @@ static int
 tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	int r;
-	u8 intmask;
+	u8 intmask = 0;
 	struct tpm_chip *chip;
 	struct st33zp24_platform_data *platform_data;
 	struct tpm_stm_dev *tpm_dev;
 
-	if (client == NULL) {
+	if (!client) {
 		pr_info("%s: i2c client is NULL. Device not accessible.\n",
 			__func__);
 		return -ENODEV;
@@ -762,8 +761,7 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 		clear_interruption(tpm_dev);
 		r = devm_request_threaded_irq(&client->dev, client->irq, NULL,
-				tpm_ioserirq_handler,
-				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+				tpm_ioserirq_handler, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
 				"TPM SERIRQ management", chip);
 		if (r < 0) {
 			dev_err(chip->dev , "TPM SERIRQ signals %d not available\n",
@@ -811,7 +809,7 @@ _tpm_clean_answer:
 /*
  * tpm_stm_i2c_remove remove the TPM device
  * @param: client, the i2c_client drescription (TPM I2C description).
-		clear_bit(0, &chip->is_open);
+ *		clear_bit(0, &chip->is_open);
  * @return: 0 in case of success.
  */
 static int tpm_stm_i2c_remove(struct i2c_client *client)
@@ -841,6 +839,7 @@ static int tpm_stm_i2c_pm_suspend(struct device *dev)
 		gpio_set_value(pin_infos->io_lpcpd, 0);
 	else
 		r = tpm_pm_suspend(dev);
+
 	return r;
 } /* tpm_stm_i2c_suspend() */
 
@@ -875,6 +874,7 @@ static const struct i2c_device_id tpm_stm_i2c_id[] = {
 	{TPM_ST33_I2C, 0},
 	{}
 };
+MODULE_DEVICE_TABLE(i2c, tpm_stm_i2c_id);
 
 #ifdef CONFIG_OF
 static const struct of_device_id of_st33zp24_i2c_match[] = {
@@ -884,7 +884,6 @@ static const struct of_device_id of_st33zp24_i2c_match[] = {
 MODULE_DEVICE_TABLE(of, of_st33zp24_i2c_match);
 #endif
 
-MODULE_DEVICE_TABLE(i2c, tpm_stm_i2c_id);
 static SIMPLE_DEV_PM_OPS(tpm_stm_i2c_ops, tpm_stm_i2c_pm_suspend,
 	tpm_stm_i2c_pm_resume);
 static struct i2c_driver tpm_stm_i2c_driver = {
@@ -893,7 +892,7 @@ static struct i2c_driver tpm_stm_i2c_driver = {
 		.name = TPM_ST33_I2C,
 		.pm = &tpm_stm_i2c_ops,
 		.of_match_table = of_match_ptr(of_st33zp24_i2c_match),
-		   },
+	},
 	.probe = tpm_stm_i2c_probe,
 	.remove = tpm_stm_i2c_remove,
 	.id_table = tpm_stm_i2c_id
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 11/15] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Christophe Ricard]

The tpm layer already provides a function to wait for a TIS event
wait_for_tpm_stat. Exactly like wait_for_serirq_timeout, it can work in
polling or interrupt mode.
Instead of using a completion struct, we rely on the waitqueue read_queue
and int_queue from chip->vendor field.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 138 +++++++++++++-----------------------
 1 file changed, 48 insertions(+), 90 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index cc5aef3..388bb64 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -39,6 +39,7 @@
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/wait.h>
+#include <linux/freezer.h>
 #include <linux/string.h>
 #include <linux/interrupt.h>
 #include <linux/sysfs.h>
@@ -105,7 +106,6 @@ enum tis_defaults {
 
 struct tpm_stm_dev {
 	struct i2c_client *client;
-	struct completion irq_detection;
 	struct tpm_chip *chip;
 	u8 buf[TPM_BUFSIZE + 1];
 	int io_lpcpd;
@@ -182,56 +182,12 @@ static void clear_interruption(struct tpm_stm_dev *tpm_dev)
 {
 	u8 interrupt;
 
-	I2C_READ_DATA(client, TPM_INT_STATUS, &interrupt, 1);
-	I2C_WRITE_DATA(client, TPM_INT_STATUS, &interrupt, 1);
-	I2C_READ_DATA(client, TPM_INT_STATUS, &interrupt, 1);
+	I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
+	I2C_WRITE_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
+	I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
 } /* clear_interruption() */
 
 /*
- * _wait_for_interrupt_serirq_timeout
- * @param: tpm, the chip description
- * @param: timeout, the timeout of the interrupt
- * @return: the status of the interruption.
- */
-static long _wait_for_interrupt_serirq_timeout(struct tpm_chip *chip,
-						unsigned long timeout)
-{
-	long status;
-	struct i2c_client *client;
-	struct tpm_stm_dev *tpm_dev;
-
-	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
-	client = tpm_dev->client;
-
-	status = wait_for_completion_interruptible_timeout(
-					&tpm_dev->irq_detection,
-					timeout);
-	if (status > 0)
-		enable_irq(client->irq);
-
-	return status;
-} /* wait_for_interrupt_serirq_timeout() */
-
-static int wait_for_serirq_timeout(struct tpm_chip *chip, bool condition,
-				 unsigned long timeout)
-{
-	int status = 2;
-	struct tpm_stm_dev *tpm_dev;
-
-	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
-
-	status = _wait_for_interrupt_serirq_timeout(chip, timeout);
-	if (!status) {
-		status = -EBUSY;
-	} else {
-		clear_interruption(tpm_dev);
-		if (condition)
-			status = 1;
-	}
-	return status;
-} /* wait_for_serirq_timeout() */
-
-/*
  * tpm_stm_i2c_cancel, cancel is not implemented.
  * @param: chip, the tpm_chip description as specified in driver/char/tpm/tpm.h
  */
@@ -244,8 +200,6 @@ static void tpm_stm_i2c_cancel(struct tpm_chip *chip)
 
 	data = TPM_STS_COMMAND_READY;
 	I2C_WRITE_DATA(tpm_dev, TPM_STS, &data, 1);
-	if (chip->vendor.irq)
-		wait_for_serirq_timeout(chip, 1, chip->vendor.timeout_a);
 } /* tpm_stm_i2c_cancel() */
 
 /*
@@ -295,27 +249,37 @@ static int check_locality(struct tpm_chip *chip)
  */
 static int request_locality(struct tpm_chip *chip)
 {
-	unsigned long stop;
+	unsigned long stop, timeout;
 	long r;
 	struct tpm_stm_dev *tpm_dev;
 	u8 data;
 
-	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
-
 	if (check_locality(chip) == chip->vendor.locality)
 		return chip->vendor.locality;
 
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
+
 	data = TPM_ACCESS_REQUEST_USE;
 	r = I2C_WRITE_DATA(tpm_dev, TPM_ACCESS, &data, 1);
 	if (r < 0)
 		goto end;
 
+	stop = jiffies + chip->vendor.timeout_a;
+
 	if (chip->vendor.irq) {
-		r = wait_for_serirq_timeout(chip, (check_locality
-						       (chip) >= 0),
-						      chip->vendor.timeout_a);
+again:
+		timeout = stop - jiffies;
+		if ((long) timeout <= 0)
+			return -1;
+		r = wait_event_interruptible_timeout(chip->vendor.read_queue,
+					check_locality(chip) >= 0,
+					timeout);
 		if (r > 0)
 			return chip->vendor.locality;
+		if (r == -ERESTARTSYS && freezing(current)) {
+			clear_thread_flag(TIF_SIGPENDING);
+			goto again;
+		}
 	} else {
 		stop = jiffies + chip->vendor.timeout_a;
 		do {
@@ -387,31 +351,26 @@ end:
  * @param: mask, the value mask to wait
  * @param: timeout, the timeout
  * @param: queue, the wait queue.
+ * @param: check_cancel, does the command can be cancelled ?
  * @return: the tpm status, 0 if success, -ETIME if timeout is reached.
  */
 static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
-			 wait_queue_head_t *queue)
+			 wait_queue_head_t *queue, bool check_cancel)
 {
-	unsigned long stop;
-	long r;
-	u8 status;
+	int r;
+	struct tpm_stm_dev *tpm_dev;
 
-	if (chip->vendor.irq) {
-		r = wait_for_serirq_timeout(chip, ((tpm_stm_i2c_status
-							(chip) & mask) ==
-						       mask), timeout);
-		if (r > 0)
-			return 0;
-	} else {
-		stop = jiffies + timeout;
-		do {
-			msleep(TPM_TIMEOUT);
-			status = tpm_stm_i2c_status(chip);
-			if ((status & mask) == mask)
-				return 0;
-		} while (time_before(jiffies, stop));
-	}
-	return -ETIME;
+	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
+
+	if (chip->vendor.irq)
+		enable_irq(chip->vendor.irq);
+
+	r = wait_for_tpm_stat(chip, mask, timeout, queue, check_cancel);
+
+	if (chip->vendor.irq)
+		clear_interruption(tpm_dev);
+
+	return r;
 } /* wait_for_stat() */
 
 /*
@@ -432,7 +391,7 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 	       wait_for_stat(chip,
 			     TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 			     chip->vendor.timeout_c,
-			     &chip->vendor.read_queue) == 0) {
+			     &chip->vendor.read_queue, true) == 0) {
 		burstcnt = get_burstcount(chip);
 		if (burstcnt < 0)
 			return burstcnt;
@@ -452,15 +411,10 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 static irqreturn_t tpm_ioserirq_handler(int irq, void *dev_id)
 {
 	struct tpm_chip *chip = dev_id;
-	struct i2c_client *client;
-	struct tpm_stm_dev *tpm_dev;
 
-	disable_irq_nosync(irq);
+	wake_up_interruptible(&chip->vendor.read_queue);
+	disable_irq_nosync(chip->vendor.irq);
 
-	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
-	client = tpm_dev->client;
-
-	complete(&tpm_dev->irq_detection);
 	return IRQ_HANDLED;
 } /* tpm_ioserirq_handler() */
 
@@ -503,7 +457,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 		tpm_stm_i2c_cancel(chip);
 		if (wait_for_stat
 		    (chip, TPM_STS_COMMAND_READY, chip->vendor.timeout_b,
-		     &chip->vendor.int_queue) < 0) {
+		     &chip->vendor.read_queue, false) < 0) {
 			r = -ETIME;
 			goto out_err;
 		}
@@ -753,7 +707,9 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	chip->vendor.locality = LOCALITY0;
 
 	if (client->irq) {
-		init_completion(&tpm_dev->irq_detection);
+		/* INTERRUPT Setup */
+		init_waitqueue_head(&chip->vendor.read_queue);
+
 		if (request_locality(chip) != LOCALITY0) {
 			r = -ENODEV;
 			goto _tpm_clean_answer;
@@ -792,6 +748,8 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 		chip->vendor.irq = client->irq;
 
+		disable_irq_nosync(chip->vendor.irq);
+
 		tpm_gen_interrupt(chip);
 	}
 
@@ -857,10 +815,10 @@ static int tpm_stm_i2c_pm_resume(struct device *dev)
 
 	if (gpio_is_valid(pin_infos->io_lpcpd)) {
 		gpio_set_value(pin_infos->io_lpcpd, 1);
-		r = wait_for_serirq_timeout(chip,
-					  (chip->ops->status(chip) &
-					  TPM_STS_VALID) == TPM_STS_VALID,
-					  chip->vendor.timeout_b);
+		r = wait_for_stat(chip,
+				(chip->ops->status(chip) &
+				TPM_STS_VALID) == TPM_STS_VALID, chip->vendor.timeout_b,
+				&chip->vendor.read_queue, false);
 	} else {
 		r = tpm_pm_resume(dev);
 		if (!r)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 12/15] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

[Christophe Ricard]

Remove useless i2c read on TPM_INT_ENABLE and TPM_INT_STATUS

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 388bb64..ed4176e 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -184,7 +184,6 @@ static void clear_interruption(struct tpm_stm_dev *tpm_dev)
 
 	I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
 	I2C_WRITE_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
-	I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
 } /* clear_interruption() */
 
 /*
@@ -725,10 +724,6 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			goto _tpm_clean_answer;
 		}
 
-		r = I2C_READ_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
-		if (r < 0)
-			goto _tpm_clean_answer;
-
 		intmask |= TPM_INTF_CMD_READY_INT
 			|  TPM_INTF_STS_VALID_INT
 			|  TPM_INTF_DATA_AVAIL_INT;
@@ -742,10 +737,6 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		if (r < 0)
 			goto _tpm_clean_answer;
 
-		r = I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &intmask, 1);
-		if (r < 0)
-			goto _tpm_clean_answer;
-
 		chip->vendor.irq = client->irq;
 
 		disable_irq_nosync(chip->vendor.irq);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 13/15] tpm/tpm_i2c_stm_st33: Fix potential bug in tpm_stm_i2c_send

[Christophe Ricard]

When sending data in tpm_stm_i2c_send, each loop iteration send buf.
Send buf + i instead as the goal of this for loop is to send a number
of byte from buf that fit in burstcnt. Once those byte are sent, we are
supposed to send the next ones.

The driver was working because the burstcount value returns always the maximum size for a TPM
command or response. (0x800 for a command and 0x400 for a response).

Reviewed-By: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index ed4176e..76b958a 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -467,7 +467,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 		if (burstcnt < 0)
 			return burstcnt;
 		size = min_t(int, len - i - 1, burstcnt);
-		r = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf, size);
+		r = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf + i, size);
 		if (r < 0)
 			goto out_err;
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 14/15] tpm/tpm_i2c_stm_st33: Change License header to have up to date address information

[Christophe Ricard]

The Free Software Foundation may have mail address change.
In order to be sure to have up to date mail address give an url to
the license which includes accurate informations.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c            | 5 ++---
 include/linux/platform_data/tpm_i2c_stm_st33.h | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 76b958a..ab366f1 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -12,9 +12,8 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
  *
  * STMicroelectronics version 1.2.0, Copyright (C) 2010
  * STMicroelectronics comes with ABSOLUTELY NO WARRANTY.
diff --git a/include/linux/platform_data/tpm_i2c_stm_st33.h b/include/linux/platform_data/tpm_i2c_stm_st33.h
index ec698f2..85775cf 100644
--- a/include/linux/platform_data/tpm_i2c_stm_st33.h
+++ b/include/linux/platform_data/tpm_i2c_stm_st33.h
@@ -12,9 +12,8 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
  *
  * STMicroelectronics version 1.2.0, Copyright (C) 2010
  * STMicroelectronics comes with ABSOLUTELY NO WARRANTY.
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 15/15] tpm/tpm_i2c_stm_st33: Increment driver version to 1.2.1.

[Christophe Ricard]

Many changes were added to the driver so increment the version.

Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
---
 drivers/char/tpm/tpm_i2c_stm_st33.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index ab366f1..6ed8c05 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -1,6 +1,6 @@
 /*
  * STMicroelectronics TPM I2C Linux driver for TPM ST33ZP24
- * Copyright (C) 2009, 2010  STMicroelectronics
+ * Copyright (C) 2009, 2010, 2014  STMicroelectronics
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -15,7 +15,7 @@
  * You should have received a copy of the GNU General Public License
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  *
- * STMicroelectronics version 1.2.0, Copyright (C) 2010
+ * STMicroelectronics version 1.2.1, Copyright (C) 2014
  * STMicroelectronics comes with ABSOLUTELY NO WARRANTY.
  * This is free software, and you are welcome to redistribute it
  * under certain conditions.
@@ -850,5 +850,5 @@ module_i2c_driver(tpm_stm_i2c_driver);
 
 MODULE_AUTHOR("Christophe Ricard (tpmsupport-qxv4g6HH51o@public.gmane.org)");
 MODULE_DESCRIPTION("STM TPM I2C ST33 Driver");
-MODULE_VERSION("1.2.0");
+MODULE_VERSION("1.2.1");
 MODULE_LICENSE("GPL");
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [tpmdd-devel] [PATCH v3 12/15] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

[Marcin Obara]

Wasn't last read required for flush write?

2014-10-13 22:23 GMT+02:00 Christophe Ricard <christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Remove useless i2c read on TPM_INT_ENABLE and TPM_INT_STATUS
>
> Signed-off-by: Christophe Ricard <christophe-h.ricard-qxv4g6HH51o@public.gmane.org>
> ---
>  drivers/char/tpm/tpm_i2c_stm_st33.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
> index 388bb64..ed4176e 100644
> --- a/drivers/char/tpm/tpm_i2c_stm_st33.c
> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
> @@ -184,7 +184,6 @@ static void clear_interruption(struct tpm_stm_dev *tpm_dev)
>
>         I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
>         I2C_WRITE_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
> -       I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
>  } /* clear_interruption() */
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [tpmdd-devel] [PATCH v3 12/15] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

[Christophe Henri RICARD]

Hi Marcin,

In the current clear_interruption function, reading the TPM_INT_STATUS after a write has no effect (aka: flushing the current request).
I am removing it because the interrupt value retrieved after this read is never used.

The only thing I can think of would to verify that the TPM_INT_STATUS got the expected state and return a status code in case of error.
However, I believe in case of such situation, the TPM will generate other error such as transmission error that will be detected on the other layers.

Best Regards
Christophe

-----Original Message-----
From: marcin.obara@gmail.com [mailto:marcin.obara@gmail.com] On Behalf Of Marcin Obara
Sent: mardi 14 octobre 2014 10:18
To: Christophe Ricard
Cc: peterhuewe@gmx.de; ashley@ashleylai.com; tpmdd@selhorst.net; devicetree@vger.kernel.org; Jean-Luc BLANC; tpmdd-devel; Christophe Henri RICARD
Subject: Re: [tpmdd-devel] [PATCH v3 12/15] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

Wasn't last read required for flush write?

2014-10-13 22:23 GMT+02:00 Christophe Ricard <christophe.ricard@gmail.com>:
> Remove useless i2c read on TPM_INT_ENABLE and TPM_INT_STATUS
>
> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
> ---
>  drivers/char/tpm/tpm_i2c_stm_st33.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
> index 388bb64..ed4176e 100644
> --- a/drivers/char/tpm/tpm_i2c_stm_st33.c
> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
> @@ -184,7 +184,6 @@ static void clear_interruption(struct tpm_stm_dev *tpm_dev)
>
>         I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
>         I2C_WRITE_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
> -       I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
>  } /* clear_interruption() */

Re: [PATCH v3 08/15] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Jason Gunthorpe]

On Mon, Oct 13, 2014 at 10:23:30PM +0200, Christophe Ricard wrote:
> Add tpm_stm_st33_i2c dts structure keeping backward compatibility
> with static platform_data support as well.
> In the mean time the code is made much simpler by:
> - Moving all gpio_request to devm_gpio_request_one primitive

> - Moving request_irq to devm_request_threaded_irq

This should move to patch 11, and it doesn't look like threaded_irq is
necessary anymore?

> +		pr_err("Failed to retrieve lpcpd-gpios from dts.\n");

All these prints should be dev_err, I think, clinet->dev looks like it
must be valid at this point.

This is a general comment actually, all the pr_* prints look like they
have access to a struct device and should be dev_* prints.

> +	/* GPIO request and configuration */
> +	r = devm_gpio_request_one(&client->dev, gpio,
> +			GPIOF_OUT_INIT_HIGH, "TPM IO LPCPD");
> +	if (r) {
> +		pr_err("Failed to request lpcpd pin\n");

Ditto

> +		return -ENODEV;

Return r?

> +	pdata = client->dev.platform_data;
> +	if (pdata == NULL) {
> +		pr_err("No platform data\n");
> +		return -EINVAL;
> +	}

It would be nice if the platform data was optional since it is now
the case that the only content (io_lpcd) is optional.

> +		if (r) {
> +			pr_err("%s : reset gpio_request failed\n", __FILE__);
> +			return -ENODEV;

Ditto

>  	tpm_dev = devm_kzalloc(&client->dev, sizeof(struct tpm_stm_dev),
>  			       GFP_KERNEL);
>  	if (!tpm_dev) {
>  		dev_info(&client->dev, "cannot allocate memory for tpm
>  			       data\n");

The print is redundant, kzalloc failure already prints lots of stuff

> +	platform_data = client->dev.platform_data;
> +	if (!platform_data && client->dev.of_node) {
> +		r = tpm_stm_i2c_of_request_resources(chip);
> +		if (r) {
> +			pr_err("No platform data\n");

If we go down this branch then tpm_stm_i2c_of_request_resources
already printed, so this print is redundant

> +			goto _tpm_clean_answer;
> +		}
> +	} else if (platform_data) {
> +		r = tpm_stm_i2c_request_resources(client, chip);
> +		if (r) {
> +			pr_err("Cannot get platform resources\n");

Ditto

> +			goto _tpm_clean_answer;
> +		}
> +	} else {
> +		pr_err("tpm_stm_st33 platform resources not available\n");
> +		goto _tpm_clean_answer;

Again, would be nice if platform data was optional

>  		intmask |= TPM_INTF_CMD_READY_INT
> -			|  TPM_INTF_FIFO_AVALAIBLE_INT
> -			|  TPM_INTF_WAKE_UP_READY_INT
> -			|  TPM_INTF_LOCALITY_CHANGE_INT
>  			|  TPM_INTF_STS_VALID_INT
>  			|  TPM_INTF_DATA_AVAIL_INT;

This hunk should also go to patch 11, I think..

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 11/15] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Jason Gunthorpe]

On Mon, Oct 13, 2014 at 10:23:33PM +0200, Christophe Ricard wrote:
> The tpm layer already provides a function to wait for a TIS event
> wait_for_tpm_stat. Exactly like wait_for_serirq_timeout, it can work in
> polling or interrupt mode.
> Instead of using a completion struct, we rely on the waitqueue read_queue
> and int_queue from chip->vendor field.


>  static int request_locality(struct tpm_chip *chip)
>  {
[..]

>  	if (chip->vendor.irq) {
> -		r = wait_for_serirq_timeout(chip, (check_locality
> -						       (chip) >= 0),
> -						      chip->vendor.timeout_a);
> +again:
> +		timeout = stop - jiffies;
> +		if ((long) timeout <= 0)
> +			return -1;

I don't see an enable_irq before this sleep:

> +		r = wait_event_interruptible_timeout(chip->vendor.read_queue,
> +					check_locality(chip) >= 0,
> +					timeout);

Can it use wait_for_stat?

>  static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
> -			 wait_queue_head_t *queue)
> +			 wait_queue_head_t *queue, bool check_cancel)
>  {

So, I didn't audit the driver 100%, but this new version has the flow

- enable_irq
- wait for irq
- clear irq

Which is conceptually fine (and efficient), but it requires the rest
of the driver to guarentee that the interrupt is consistent after
every interation with the TPM. Which for this driver I think means one
of two states:
 - No interrupt asserted
 - Interrupt asserted, and the TPM is ready to accept a command
Other states will cause longer command execution times, but not
malfunction.

For instance, it looks to me like request_locality might not maintain
that invariant.

Clearing old pending bits before starting an interaction is certainly
safer, but costs that extra I2C sequence.

> +	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
> +
> +	if (chip->vendor.irq)
> +		enable_irq(chip->vendor.irq);
> +
> +	r = wait_for_tpm_stat(chip, mask, timeout, queue, check_cancel);

This seqence is racy, the reason the nuvoton driver has the counter is
because wake_up_interruptible only wakes sleeping threads, so the
driver has to use the counter to close the race between the enable_irq
and the actual sleep:

		unsigned int cur_intrs = priv->intrs;
		enable_irq(chip->vendor.irq);
		rc = wait_event_interruptible_timeout(*queue,
						      cur_intrs != priv->intrs,
						      timeout);

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/15] ST33 I2C TPM driver cleanup

[Jason Gunthorpe]

On Mon, Oct 13, 2014 at 10:23:22PM +0200, Christophe Ricard wrote:

>   tpm/tpm_i2c_stm_st33: Update Kconfig in order to be inline to other
>     similar product
>   tpm/tpm_i2c_stm_st33: Fix few coding style error reported by
>     scripts/checkpatch.pl
>   tpm/tpm_i2c_stm_st33: Move tpm registers to tpm_i2c_stm_st33.c
>   tpm/tpm_i2c_stm_st33: Add new tpm_stm_dev structure and remove
>     tpm_i2c_buffer[0], [1] buffer.
>   tpm/tpm_i2c_stm_st33: Remove reference to io_serirq
>   tpm/tpm_i2c_stm_st33: Replace err/rc/ret by r for a function return
>     code
>   tpm/tpm_i2c_stm_st33: Replace tpm_st33_* function with tpm_stm_*
>   tpm/tpm_i2c_stm_st33/dts/st33zp24_i2c: Add DTS Documentation
>   tpm/tpm_i2c_stm_st33: Few code cleanup
>   tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers
>   tpm/tpm_i2c_stm_st33: Fix potential bug in tpm_stm_i2c_send
>   tpm/tpm_i2c_stm_st33: Change License header to have up to date address
>     information
>   tpm/tpm_i2c_stm_st33: Increment driver version to 1.2.1.


You can add my

Reviewed-By: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

On the above in the series

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 08/15] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Christophe RICARD]

Hi Jason,

On Tue, 14 Oct 2014 11:32:09 -0600
Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:

> On Mon, Oct 13, 2014 at 10:23:30PM +0200, Christophe Ricard wrote:
> > Add tpm_stm_st33_i2c dts structure keeping backward compatibility
> > with static platform_data support as well.
> > In the mean time the code is made much simpler by:
> > - Moving all gpio_request to devm_gpio_request_one primitive
> 
> > - Moving request_irq to devm_request_threaded_irq
> 
> This should move to patch 11, and it doesn't look like threaded_irq is
> necessary anymore?
> 
> > +		pr_err("Failed to retrieve lpcpd-gpios from
> > dts.\n");
> 
> All these prints should be dev_err, I think, clinet->dev looks like it
> must be valid at this point.
> 
> This is a general comment actually, all the pr_* prints look like they
> have access to a struct device and should be dev_* prints.
> 
Ok. Basically every tpm device drivers use dev_* so... Let's go for it.

> > +	/* GPIO request and configuration */
> > +	r = devm_gpio_request_one(&client->dev, gpio,
> > +			GPIOF_OUT_INIT_HIGH, "TPM IO LPCPD");
> > +	if (r) {
> > +		pr_err("Failed to request lpcpd pin\n");
> 
> Ditto
> 
> > +		return -ENODEV;
> 
> Return r?
Ok

> 
> > +	pdata = client->dev.platform_data;
> > +	if (pdata == NULL) {
> > +		pr_err("No platform data\n");
> > +		return -EINVAL;
> > +	}
> 
> It would be nice if the platform data was optional since it is now
> the case that the only content (io_lpcd) is optional.
>
Yes. You are correct.
 
> > +		if (r) {
> > +			pr_err("%s : reset gpio_request failed\n",
> > __FILE__);
> > +			return -ENODEV;
> 
> Ditto
> 
> >  	tpm_dev = devm_kzalloc(&client->dev, sizeof(struct
> > tpm_stm_dev), GFP_KERNEL);
> >  	if (!tpm_dev) {
> >  		dev_info(&client->dev, "cannot allocate memory for
> > tpm data\n");
> 
> The print is redundant, kzalloc failure already prints lots of stuff
Ok. I will remove this one.

> 
> > +	platform_data = client->dev.platform_data;
> > +	if (!platform_data && client->dev.of_node) {
> > +		r = tpm_stm_i2c_of_request_resources(chip);
> > +		if (r) {
> > +			pr_err("No platform data\n");
> 
> If we go down this branch then tpm_stm_i2c_of_request_resources
> already printed, so this print is redundant
> 
Ok. I will remove them in the probe function and will extends a little
bit in the xxx_request_resources functions.

> > +			goto _tpm_clean_answer;
> > +		}
> > +	} else if (platform_data) {
> > +		r = tpm_stm_i2c_request_resources(client, chip);
> > +		if (r) {
> > +			pr_err("Cannot get platform resources\n");
> 
> Ditto
> 
> > +			goto _tpm_clean_answer;
> > +		}
> > +	} else {
> > +		pr_err("tpm_stm_st33 platform resources not
> > available\n");
> > +		goto _tpm_clean_answer;
> 
> Again, would be nice if platform data was optional
> 
Ok. I will remove this else branch.

> >  		intmask |= TPM_INTF_CMD_READY_INT
> > -			|  TPM_INTF_FIFO_AVALAIBLE_INT
> > -			|  TPM_INTF_WAKE_UP_READY_INT
> > -			|  TPM_INTF_LOCALITY_CHANGE_INT
> >  			|  TPM_INTF_STS_VALID_INT
> >  			|  TPM_INTF_DATA_AVAIL_INT;
> 
> This hunk should also go to patch 11, I think..
>
So basically merging this hunk with patch 11 would mean current
content + irq management improvement. It makes sense.
I will try to include your comments in a future v4.

> Jason

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 11/15] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Christophe RICARD]

On Tue, 14 Oct 2014 12:09:14 -0600
Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:

> On Mon, Oct 13, 2014 at 10:23:33PM +0200, Christophe Ricard wrote:
> > The tpm layer already provides a function to wait for a TIS event
> > wait_for_tpm_stat. Exactly like wait_for_serirq_timeout, it can
> > work in polling or interrupt mode.
> > Instead of using a completion struct, we rely on the waitqueue
> > read_queue and int_queue from chip->vendor field.
> 
> 
> >  static int request_locality(struct tpm_chip *chip)
> >  {
> [..]
> 
> >  	if (chip->vendor.irq) {
> > -		r = wait_for_serirq_timeout(chip, (check_locality
> > -						       (chip) >=
> > 0),
> > -
> > chip->vendor.timeout_a); +again:
> > +		timeout = stop - jiffies;
> > +		if ((long) timeout <= 0)
> > +			return -1;
> 
> I don't see an enable_irq before this sleep:
I missed that one i guess.

> 
> > +		r =
> > wait_event_interruptible_timeout(chip->vendor.read_queue,
> > +					check_locality(chip) >= 0,
> > +					timeout);
> 
> Can it use wait_for_stat?
It actually cannot use wait_for_stat because wait_for_stat is waiting
for a mask condition on the status register. Here we are addressing the
TPM_ACCESS register.

> 
> >  static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned
> > long timeout,
> > -			 wait_queue_head_t *queue)
> > +			 wait_queue_head_t *queue, bool
> > check_cancel) {
> 
> So, I didn't audit the driver 100%, but this new version has the flow
> 
> - enable_irq
> - wait for irq
> - clear irq
> 
> Which is conceptually fine (and efficient), but it requires the rest
> of the driver to guarentee that the interrupt is consistent after
> every interation with the TPM. Which for this driver I think means one
> of two states:
>  - No interrupt asserted
>  - Interrupt asserted, and the TPM is ready to accept a command
> Other states will cause longer command execution times, but not
> malfunction.
> 
> For instance, it looks to me like request_locality might not maintain
> that invariant.
> 
> Clearing old pending bits before starting an interaction is certainly
> safer, but costs that extra I2C sequence.
> 
> > +	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
> > +
> > +	if (chip->vendor.irq)
> > +		enable_irq(chip->vendor.irq);
> > +
> > +	r = wait_for_tpm_stat(chip, mask, timeout, queue,
> > check_cancel);
> 
> This seqence is racy, the reason the nuvoton driver has the counter is
> because wake_up_interruptible only wakes sleeping threads, so the
> driver has to use the counter to close the race between the enable_irq
> and the actual sleep:
> 
> 		unsigned int cur_intrs = priv->intrs;
> 		enable_irq(chip->vendor.irq);
> 		rc = wait_event_interruptible_timeout(*queue,
> 						      cur_intrs !=
> priv->intrs, timeout);
If my understanding is correct the counter prevent the case where the
irq is raised before entering into the wait_event_interruptible_timeout.
wait_for_tpm_stat will check before going into sleep the status
register in order to make sure the condition is not already satisfied
and should fulfill this requirement.

As i could get different kind of interruption i think i cannot avoid an
i2c check here. The other solution would be to activate only the right
interruption in the INT_ENABLE tis register but still with an i2c
transaction.

> 
> Jason

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 11/15] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Jason Gunthorpe]

On Tue, Oct 14, 2014 at 10:44:25PM +0200, Christophe RICARD wrote:

> > > +		r =
> > > wait_event_interruptible_timeout(chip->vendor.read_queue,
> > > +					check_locality(chip) >= 0,
> > > +					timeout);
> > 
> > Can it use wait_for_stat?
> It actually cannot use wait_for_stat because wait_for_stat is waiting
> for a mask condition on the status register. Here we are addressing the
> TPM_ACCESS register.

It would be cleaner if wait_for_stat handled all sleep-for-irq
actions.

> > This seqence is racy, the reason the nuvoton driver has the counter is
> > because wake_up_interruptible only wakes sleeping threads, so the
> > driver has to use the counter to close the race between the enable_irq
> > and the actual sleep:
> > 
> > 		unsigned int cur_intrs = priv->intrs;
> > 		enable_irq(chip->vendor.irq);
> > 		rc = wait_event_interruptible_timeout(*queue,
> > 						      cur_intrs !=
> > priv->intrs, timeout);
> If my understanding is correct the counter prevent the case where the
> irq is raised before entering into the wait_event_interruptible_timeout.
> wait_for_tpm_stat will check before going into sleep the status
> register in order to make sure the condition is not already satisfied
> and should fulfill this requirement.

Yes, your analysis is correct - removing the extra I2C polling would
be the goal of using the counter rather than an I2C read.

> As i could get different kind of interruption i think i cannot avoid an
> i2c check here. The other solution would be to activate only the right
> interruption in the INT_ENABLE tis register but still with an i2c
> transaction.

But there is a problem with the wrong interruption no matter what, the
wait_event_ loop does not ever re-enable_irq() - any generated IRQ
must already be exactly the IRQ that is being waited for. Basically,
the driver must remain synchronized with the chip and it must know
what IRQs can be generated at any point.

When I read the driver I assumed this was already happening, and the
auditing was going to be done to make sure it is the case, hence my
comments on the idle state.

The advantage is clearly that many I2C transactions can be eliminated
by relying on the IRQ line to signal progress.

The alternative is to have the wait_event loops clear the pending bits
and re-enable the IRQ every time they go around, but this is more
transactions.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html