[PATCH 00/16] ST33 I2C TPM driver cleanup

[PATCH 00/16] ST33 I2C TPM driver cleanup

[Christophe Ricard]

Hi Peter,

The following patchset brings:
- Some few code clean up from code style up to structure
- Device tree support keeping static platform data configuration support.
- Fix 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 (16):
  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: 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: 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: Change License header to have up to date address
    information
  tpm/tpm_i2c_stm_st33: Add tpm_lock mutex for safe irq management
  tpm/tpm_i2c_stm_st33: Fix potential bug in tpm_stm_i2c_send
  tpm/tpm_i2c_stm_st33: Add delay before release_locality to make sure
    irq are cleared
  tpm: Increment driver version to 1.2.1.

 .../devicetree/bindings/security/tpm/st33zp24.txt  |  35 ++
 drivers/char/tpm/Kconfig                           |  20 +-
 drivers/char/tpm/tpm_i2c_stm_st33.c                | 680 +++++++++++----------
 drivers/char/tpm/tpm_i2c_stm_st33.h                |  61 --
 include/linux/platform_data/tpm_i2c_stm_st33.h     |  40 ++
 5 files changed, 458 insertions(+), 378 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 01/16] 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index c54cac3..21abb0e 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -63,6 +63,16 @@ config TCG_TIS_I2C_NUVOTON
 	  To compile this driver as a module, choose M here; the module
 	  will be called tpm_i2c_nuvoton.
 
+config TCG_ST33_I2C
+	tristate "TPM Interface Specification 1.2 Interface (I2C - STMicroelectronics)"
+	depends on I2C
+	depends on GPIOLIB
+	---help---
+	  If you have a TPM security chip from STMicroelectronics working with
+	  an I2C bus say Yes and it will be accessible from within Linux.
+	  To compile this driver as a module, choose M here; the module will be
+	  called tpm_stm_st33_i2c.
+
 config TCG_NSC
 	tristate "National Semiconductor TPM Interface"
 	depends on X86
@@ -100,16 +110,6 @@ 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"
-	depends on I2C
-	depends on GPIOLIB
-	---help---
-	  If you have a TPM security chip from STMicroelectronics working with
-	  an I2C bus say Yes and it will be accessible from within Linux.
-	  To compile this driver as a module, choose M here; the module will be
-	  called tpm_stm_st33_i2c.
-
 config TCG_XEN
 	tristate "XEN TPM Interface"
 	depends on TCG_TPM && XEN
-- 
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 01/16] tpm/tpm_i2c_stm_st33: Update Kconfig in order to be inline to other similar product

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 10:02:54PM +0200, Christophe Ricard wrote:
> STMicroelectronics i2c tpm is the only one to have a different tristate
> label.
> 
> Rename it "TPM Interface Specification 1.2 Interface (I2C - STMicroelectronics)"

Looks fine, but why move the stanza? Are you sorting on the short
description? (vs config tag?)

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

[PATCH 02/16] 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 03/16] 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 04/16] 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, 131 insertions(+), 152 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..098f881 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -49,6 +49,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 +68,7 @@
 #define TPM_BUFSIZE			2048
 
 #define LOCALITY0		0
-#include "tpm_i2c_stm_st33.h"
+
 
 enum stm33zp24_access {
 	TPM_ACCESS_VALID = 0x80,
@@ -99,6 +100,16 @@ enum tis_defaults {
 	TIS_LONG_TIMEOUT = 2000,
 };
 
+struct tpm_stm_dev {
+	struct i2c_client *client;
+	struct completion irq_detection;
+	struct mutex lock;
+	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 +118,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 +134,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 +197,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 +215,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 +236,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 +254,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 +271,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 +296,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 +334,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 +353,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 +423,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 +437,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 +453,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 +482,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 +514,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 +527,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 +538,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 +632,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 +647,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 +663,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 +687,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 +707,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 +718,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 +749,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 +764,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 05/16] 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 | 103 ++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 52 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 098f881..b20f148 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -295,7 +295,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;
 
@@ -305,15 +305,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;
@@ -323,9 +323,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() */
 
 /*
@@ -392,14 +392,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;
@@ -479,7 +479,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;
@@ -494,9 +494,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) {
@@ -504,7 +504,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;
 		}
 	}
@@ -514,8 +514,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;
@@ -523,17 +523,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;
 	}
 
@@ -544,7 +544,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;
 }
 
 /*
@@ -628,7 +628,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;
@@ -637,13 +637,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;
 	}
 
@@ -651,7 +651,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;
 	}
 
@@ -659,7 +659,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;
 	}
 
@@ -680,8 +680,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);
 	}
@@ -689,26 +689,26 @@ 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;
 		}
-		err = gpio_request(platform_data->io_serirq, "TPM IO_SERIRQ");
-		if (err)
+		r = gpio_request(platform_data->io_serirq, "TPM IO_SERIRQ");
+		if (r)
 			goto _gpio_init2;
 
 		clear_interruption(tpm_dev);
-		err = request_irq(gpio_to_irq(platform_data->io_serirq),
+		r = request_irq(gpio_to_irq(platform_data->io_serirq),
 				&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",
 				gpio_to_irq(platform_data->io_serirq));
 			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
@@ -718,17 +718,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;
@@ -753,7 +753,7 @@ _tpm_clean_answer:
 	tpm_remove_hardware(chip->dev);
 end:
 	pr_info("TPM I2C initialisation fail\n");
-	return err;
+	return r;
 }
 
 /*
@@ -783,14 +783,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() */
 
 /*
@@ -803,20 +802,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 06/16] 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 b20f148..9466422 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -228,7 +228,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.
@@ -245,7 +245,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
@@ -261,7 +261,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() */
 
 
 /*
@@ -595,7 +595,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);
 }
@@ -607,7 +607,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;
@@ -619,14 +619,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;
@@ -757,12 +757,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);
@@ -775,12 +775,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;
@@ -790,14 +790,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;
@@ -816,28 +816,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 07/16] 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 | 198 +++++++++++++++++++++++++++---------
 1 file changed, 152 insertions(+), 46 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 9466422..ac23f0f 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -48,6 +48,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"
@@ -618,6 +622,108 @@ static int power_mgt = 1;
 module_param(power_mgt, int, 0444);
 MODULE_PARM_DESC(power_mgt, "Power Management");
 
+#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");
+		power_mgt = 0;
+		goto _irq_probe;
+	}
+	power_mgt = 1;
+	/* 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 serirq pin\n");
+		return -ENODEV;
+	}
+	tpm_dev->io_lpcpd = gpio;
+
+_irq_probe:
+	/* IRQ */
+	r = irq_of_parse_and_map(pp, 0);
+	if (r < 0) {
+		pr_err("Unable to get irq, error: %d\n", r);
+		interrupts = 0;
+		goto _end;
+	}
+	interrupts = 1;
+	client->irq = r;
+
+	return 0;
+_end:
+	return r;
+}
+#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;
+	int irq;
+
+	pdata = client->dev.platform_data;
+	if (pdata == NULL) {
+		pr_err("No platform data\n");
+		return -EINVAL;
+	}
+
+	/* store for late use */
+	tpm_dev->io_serirq = pdata->io_serirq;
+	tpm_dev->io_lpcpd = pdata->io_lpcpd;
+
+	if (interrupts) {
+		r = devm_gpio_request_one(&client->dev, pdata->io_serirq,
+					GPIOF_IN, "TPM IO_SERIRQ");
+		if (r) {
+			pr_err("%s : gpio_request failed\n", __FILE__);
+			return -ENODEV;
+		}
+
+		/* IRQ */
+		irq = gpio_to_irq(pdata->io_serirq);
+		if (irq < 0) {
+			pr_err("Unable to get irq number for GPIO %d %d\n",
+				pdata->io_serirq, r);
+			return -ENODEV;
+		}
+		client->irq = irq;
+	}
+
+	if (power_mgt) {
+		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
  * @param: client, the i2c_client drescription (TPM I2C description).
@@ -637,30 +743,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);
@@ -672,6 +767,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);
@@ -679,37 +793,27 @@ 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) {
 		init_completion(&tpm_dev->irq_detection);
 		if (request_locality(chip) != LOCALITY0) {
 			r = -ENODEV;
 			goto _tpm_clean_answer;
 		}
-		r = gpio_request(platform_data->io_serirq, "TPM IO_SERIRQ");
-		if (r)
-			goto _gpio_init2;
 
 		clear_interruption(tpm_dev);
-		r = request_irq(gpio_to_irq(platform_data->io_serirq),
-				&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",
-				gpio_to_irq(platform_data->io_serirq));
-			goto _irq_set;
+				client->irq);
+			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
@@ -720,18 +824,18 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 		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);
 	}
@@ -741,17 +845,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(gpio_to_irq(platform_data->io_serirq), (void *)chip);
-_gpio_init2:
-	if (interrupts)
-		gpio_free(platform_data->io_serirq);
-_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;
 }
@@ -823,14 +918,25 @@ 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", },
+	{}
+};
+#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,
+		#ifdef CONFIG_OF
+			.of_match_table = of_match_ptr(of_st33zp24_i2c_match),
+		#endif
 		   },
 	.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

Re: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 10:03:00PM +0200, Christophe Ricard wrote:

> +_irq_probe:
> +	/* IRQ */
> +	r = irq_of_parse_and_map(pp, 0);
> +	if (r < 0) {
> +		pr_err("Unable to get irq, error: %d\n", r);
> +		interrupts = 0;
> +		goto _end;
> +	}
> +	interrupts = 1;
> +	client->irq = r;

client->irq is already set correctly by of_i2c_register_devices - so I
don't think this sequence is necessary?

> +	if (interrupts) {
> +		r = devm_gpio_request_one(&client->dev, pdata->io_serirq,
> +					GPIOF_IN, "TPM IO_SERIRQ");

Similarly, I wonder if pdata->io_serirq is just duplication of
client->irq and that should be set by the creator instead?

> +#ifdef CONFIG_OF
> +static const struct of_device_id of_st33zp24_i2c_match[] = {
> +	{ .compatible = "st,st33zp24_i2c", },
> +	{}
> +};

missing:

MODULE_DEVICE_TABLE(of, of_st33zp24_i2c_match);

> +		#ifdef CONFIG_OF
> +			.of_match_table = of_match_ptr(of_st33zp24_i2c_match),
> +		#endif

The #ifdef is unnecessary, the of_match_ptr macro takes care of that
already.

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: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Christophe RICARD]

Hi Jason,

On 08/10/2014 00:30, Jason Gunthorpe wrote:
> On Tue, Oct 07, 2014 at 10:03:00PM +0200, Christophe Ricard wrote:
>
>> +_irq_probe:
>> +	/* IRQ */
>> +	r = irq_of_parse_and_map(pp, 0);
>> +	if (r < 0) {
>> +		pr_err("Unable to get irq, error: %d\n", r);
>> +		interrupts = 0;
>> +		goto _end;
>> +	}
>> +	interrupts = 1;
>> +	client->irq = r;
> client->irq is already set correctly by of_i2c_register_devices - so I
> don't think this sequence is necessary?
May be you are right. I will crosscheck.
>> +	if (interrupts) {
>> +		r = devm_gpio_request_one(&client->dev, pdata->io_serirq,
>> +					GPIOF_IN, "TPM IO_SERIRQ");
> Similarly, I wonder if pdata->io_serirq is just duplication of
> client->irq and that should be set by the creator instead?
pdata->io_serirq stores the gpio number which will be converted into irq 
number.
pdata->io_serirq is only use by static platform configuration not 
devicetree configuration
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_st33zp24_i2c_match[] = {
>> +	{ .compatible = "st,st33zp24_i2c", },
>> +	{}
>> +};
> missing:
>
> MODULE_DEVICE_TABLE(of, of_st33zp24_i2c_match);
Oops. Good catch :).
>> +		#ifdef CONFIG_OF
>> +			.of_match_table = of_match_ptr(of_st33zp24_i2c_match),
>> +		#endif
> The #ifdef is unnecessary, the of_match_ptr macro takes care of that
> already.
Ok will clean that.
>
> 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: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Jason Gunthorpe]

On Wed, Oct 08, 2014 at 07:47:58AM +0200, Christophe RICARD wrote:
> >>+	if (interrupts) {
> >>+		r = devm_gpio_request_one(&client->dev, pdata->io_serirq,
> >>+					GPIOF_IN, "TPM IO_SERIRQ");
> >Similarly, I wonder if pdata->io_serirq is just duplication of
> >client->irq and that should be set by the creator instead?

> pdata->io_serirq stores the gpio number which will be converted into
> irq number.  pdata->io_serirq is only use by static platform
> configuration not devicetree configuration

Right, but the driver never uses it as a GPIO, so accepting a GPIO is
actually less flexible - a platform may connect the TPM to a dedicated
IRQ pin, for instance.

The creator should just specify the irq in client->irq, however that is
typically done..

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: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Christophe Henri RICARD]

Hi Jason,

I have currently seen both. I agree on the principles as it is simplifying the code a little bit.
I including this clean up in this patch Add devicetree structure for a future v2 submission

Best Regards
Christophe

-----Original Message-----
From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org] 
Sent: mercredi 8 octobre 2014 18:29
To: Christophe RICARD
Cc: peterhuewe-Mmb7MZpHnFY@public.gmane.org; ashley-fm2HMyfA2y6tG0bUXCXiUA@public.gmane.org; tpmdd-yWjUBOtONefk1uMJSBkQmQ@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Christophe Henri RICARD; Jean-Luc BLANC
Subject: Re: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

On Wed, Oct 08, 2014 at 07:47:58AM +0200, Christophe RICARD wrote:
> >>+	if (interrupts) {
> >>+		r = devm_gpio_request_one(&client->dev, pdata->io_serirq,
> >>+					GPIOF_IN, "TPM IO_SERIRQ");
> >Similarly, I wonder if pdata->io_serirq is just duplication of
> >client->irq and that should be set by the creator instead?

> pdata->io_serirq stores the gpio number which will be converted into
> irq number.  pdata->io_serirq is only use by static platform 
> configuration not devicetree configuration

Right, but the driver never uses it as a GPIO, so accepting a GPIO is actually less flexible - a platform may connect the TPM to a dedicated IRQ pin, for instance.

The creator should just specify the irq in client->irq, however that is typically done..

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: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Jason Gunthorpe]

On Wed, Oct 08, 2014 at 10:41:36PM +0200, Christophe Henri RICARD wrote:
> Hi Jason,
> 
> I have currently seen both. I agree on the principles as it is
> simplifying the code a little bit.  I including this clean up in
> this patch Add devicetree structure for a future v2 submission

I was just looking at what the remaining lpcpd GPIO does, and..

1) I think you can probably just make this optional, if it is not
   specified in the DT or through platform data, just don't fiddle with
   it. This makes the driver much friendlier - ie it can be manually
   attached via sysfs for instance.

2) This looks fishy:

static int tpm_st33_i2c_pm_suspend(struct device *dev)
	if (power_mgt) {
		gpio_set_value(pin_infos->io_lpcpd, 0);
	} else {
		ret = tpm_pm_suspend(dev);
	}

   I can't think of a reason why the driver wouldn't need to call
   tpm_pm_suspend to save the internal state? Ditto for restore?

   I'm also suspicious of the power_mgt module option, that should
   probably go away entirely?

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: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Christophe Henri RICARD]

Jason,

For your information, the lpcpd pin is named like this as per PCClientTPMSpecification is also known as TPMSTB on this product.
The TPMSTB pin can be used in order to support D1 and D2 power management state.
I am actually sorry to refer to Microsoft website here ;) : http://msdn.microsoft.com/en-us/library/windows/hardware/ff543162%28v=vs.85%29.aspx.
As mention by our product datasheet:
"In order to reach D1/D2 state, the TPMSTB pin must be connected to a GPIO signal.
In state D1/D2, the contents of the volatile memory is *preserved*."

Also "... the implementation of the LPCPD# pin on a TPM is platform and chipset implementation specific." Cf PCClientTPMSpecification.

I am currently giving the opportunity to an integrator to use this feature. 

I don't mind removing the power_mgt module option but don't you think there might be interest for an integrator to update their TPM power management policy on their platform ?

Best Regards
Christophe
-----Original Message-----
From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org] 
Sent: mercredi 8 octobre 2014 22:51
To: Christophe Henri RICARD
Cc: Christophe RICARD; peterhuewe-Mmb7MZpHnFY@public.gmane.org; ashley-fm2HMyfA2y6tG0bUXCXiUA@public.gmane.org; tpmdd-yWjUBOtONefk1uMJSBkQmQ@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Jean-Luc BLANC
Subject: Re: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

On Wed, Oct 08, 2014 at 10:41:36PM +0200, Christophe Henri RICARD wrote:
> Hi Jason,
> 
> I have currently seen both. I agree on the principles as it is 
> simplifying the code a little bit.  I including this clean up in this 
> patch Add devicetree structure for a future v2 submission

I was just looking at what the remaining lpcpd GPIO does, and..

1) I think you can probably just make this optional, if it is not
   specified in the DT or through platform data, just don't fiddle with
   it. This makes the driver much friendlier - ie it can be manually
   attached via sysfs for instance.

2) This looks fishy:

static int tpm_st33_i2c_pm_suspend(struct device *dev)
	if (power_mgt) {
		gpio_set_value(pin_infos->io_lpcpd, 0);
	} else {
		ret = tpm_pm_suspend(dev);
	}

   I can't think of a reason why the driver wouldn't need to call
   tpm_pm_suspend to save the internal state? Ditto for restore?

   I'm also suspicious of the power_mgt module option, that should
   probably go away entirely?

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: [tpmdd-devel] [PATCH 07/16] tpm/tpm_i2c_stm_st33: Add devicetree structure

[Jason Gunthorpe]

On Wed, Oct 08, 2014 at 11:14:40PM +0200, Christophe Henri RICARD wrote:

> For your information, the lpcpd pin is named like this as per
> PCClientTPMSpecification is also known as TPMSTB on this product.
> The TPMSTB pin can be used in order to support D1 and D2 power
> management state.  I am actually sorry to refer to Microsoft website
> here ;) :

So, having the option to enter D1/D2 through that pin in the driver
seems fine, and it should just be enabled if the driver is told what
that pin is. No need for a module parameter, and no need for the
driver to fail probe if it isn't told how to access that GPIO.

However.. suspend/resume are called from the broader PM framework and
a platform might arrange things so the TPM is put into a deeper sleep
than D1/D2 (ie entirely powered off because the CPU was put into
suspend-to-ram and the peripheral power converter was switched off)

Thus the driver really must save the TPM state at suspend, and if the
volatile state was wiped after resume then it must be restored from
the saved state.

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

[PATCH 08/16] tpm: 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  | 35 ++++++++++++++++++++++
 1 file changed, 35 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..75302c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/st33zp24.txt
@@ -0,0 +1,35 @@
+* STMicroelectronics SAS. ST33ZP24 TPM SoC
+
+Required properties:
+- compatible: Should be "st,st33zp24_i2c".
+- clock-frequency: I²C work frequency.
+- reg: address on the bus
+
+Option 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 enabling/disabling the ST33ZP24
+
+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@1 {
+
+                compatible = "st,st33zp24_i2c";
+
+                reg = <0x01>;
+                clock-frequency = <400000>;
+
+                interrupt-parent = <&gpio5>;
+                interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
+
+                lpcpd-gpios = <&gpio5 29 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 09/16] 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 | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index ac23f0f..cb5a0387 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -399,7 +399,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);
@@ -435,8 +435,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;
@@ -488,7 +487,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;
@@ -565,7 +564,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) {
@@ -586,7 +585,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;
@@ -684,7 +683,7 @@ static int tpm_stm_i2c_request_resources(struct i2c_client *client,
 	int irq;
 
 	pdata = client->dev.platform_data;
-	if (pdata == NULL) {
+	if (!pdata) {
 		pr_err("No platform data\n");
 		return -EINVAL;
 	}
@@ -735,12 +734,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;
@@ -802,8 +801,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",
@@ -884,6 +882,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() */
 
@@ -937,7 +936,7 @@ static struct i2c_driver tpm_stm_i2c_driver = {
 		#ifdef CONFIG_OF
 			.of_match_table = of_match_ptr(of_st33zp24_i2c_match),
 		#endif
-		   },
+	},
 	.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 10/16] 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 | 152 ++++++++++++------------------------
 1 file changed, 48 insertions(+), 104 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index cb5a0387..e99bb78 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/spinlock.h>
@@ -106,7 +107,6 @@ enum tis_defaults {
 
 struct tpm_stm_dev {
 	struct i2c_client *client;
-	struct completion irq_detection;
 	struct mutex lock;
 	struct tpm_chip *chip;
 	u8 buf[TPM_BUFSIZE + 1];
@@ -181,58 +181,16 @@ static int read8_reg(struct tpm_stm_dev *tpm_dev, u8 tpm_register,
  * clear the TPM interrupt register.
  * @param: tpm, the chip description
  */
-static void clear_interruption(struct tpm_stm_dev *tpm_dev)
+static u8 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);
-} /* 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() */
+	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);
 
-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() */
+	return interrupt;
+} /* clear_interruption() */
 
 /*
  * tpm_stm_i2c_cancel, cancel is not implemented.
@@ -247,8 +205,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() */
 
 /*
@@ -298,27 +254,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.int_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 {
@@ -385,39 +351,6 @@ end:
 } /* get_burstcount() */
 
 /*
- * wait_for_stat wait for a TPM_STS value
- * @param: chip, the tpm chip description
- * @param: mask, the value mask to wait
- * @param: timeout, the timeout
- * @param: queue, the wait queue.
- * @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)
-{
-	unsigned long stop;
-	long r;
-	u8 status;
-
-	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;
-} /* wait_for_stat() */
-
-/*
  * recv_data receive data
  * @param: chip, the tpm chip description
  * @param: buf, the buffer where the data are received
@@ -432,10 +365,10 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
 	while (size < count &&
-	       wait_for_stat(chip,
+	       wait_for_tpm_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;
@@ -455,15 +388,23 @@ 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);
+	u8 interrupt;
 
 	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
-	client = tpm_dev->client;
 
-	complete(&tpm_dev->irq_detection);
+	interrupt = clear_interruption(tpm_dev);
+	if (!interrupt)
+		return IRQ_HANDLED;
+
+	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
+		wake_up_interruptible(&chip->vendor.read_queue);
+
+	if (interrupt &
+	    (TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
+	     TPM_INTF_CMD_READY_INT))
+		wake_up_interruptible(&chip->vendor.int_queue);
+
 	return IRQ_HANDLED;
 } /* tpm_ioserirq_handler() */
 
@@ -504,9 +445,9 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 	status = tpm_stm_i2c_status(chip);
 	if ((status & TPM_STS_COMMAND_READY) == 0) {
 		tpm_stm_i2c_cancel(chip);
-		if (wait_for_stat
+		if (wait_for_tpm_stat
 		    (chip, TPM_STS_COMMAND_READY, chip->vendor.timeout_b,
-		     &chip->vendor.int_queue) < 0) {
+		     &chip->vendor.int_queue, false) < 0) {
 			r = -ETIME;
 			goto out_err;
 		}
@@ -793,7 +734,10 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	chip->vendor.locality = LOCALITY0;
 
 	if (interrupts) {
-		init_completion(&tpm_dev->irq_detection);
+		/* INTERRUPT Setup */
+		init_waitqueue_head(&chip->vendor.read_queue);
+		init_waitqueue_head(&chip->vendor.int_queue);
+
 		if (request_locality(chip) != LOCALITY0) {
 			r = -ENODEV;
 			goto _tpm_clean_answer;
@@ -900,10 +844,10 @@ static int tpm_stm_i2c_pm_resume(struct device *dev)
 
 	if (power_mgt) {
 		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_tpm_stat(chip, 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

Re: [tpmdd-devel] [PATCH 10/16] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 10:03:03PM +0200, Christophe Ricard wrote:

> +++ 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>

I'm surprised to see that include here..

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: [tpmdd-devel] [PATCH 10/16] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Christophe RICARD]

  Hi Jason,

The freezer header is here for the freezing(current) function call in 
get_burstcount().

Best Regards
Christophe
On 08/10/2014 00:11, Jason Gunthorpe wrote:
> On Tue, Oct 07, 2014 at 10:03:03PM +0200, Christophe Ricard wrote:
>
>> +++ 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>
> I'm surprised to see that include here..
>
> 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: [tpmdd-devel] [PATCH 10/16] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Jason Gunthorpe]

On Wed, Oct 08, 2014 at 07:38:30AM +0200, Christophe RICARD wrote:
>  Hi Jason,
> 
> The freezer header is here for the freezing(current) function call
> in get_burstcount().

But freezing(current) does not occur in patch 10/16.

Is this include in the right patch in the series?

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: [tpmdd-devel] [PATCH 10/16] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Christophe Henri RICARD]

Hi Jason,

Patch 10/16 includes the following lines:
+		if (r == -ERESTARTSYS && freezing(current)) {
+			clear_thread_flag(TIF_SIGPENDING);
+			goto again;
+		}

Line 149 in this patch. I am surprise you are saying it does not occur.

Best Regards
Christophe

-----Original Message-----
From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org] 
Sent: mercredi 8 octobre 2014 18:26
To: Christophe RICARD
Cc: peterhuewe-Mmb7MZpHnFY@public.gmane.org; ashley-fm2HMyfA2y6tG0bUXCXiUA@public.gmane.org; tpmdd-yWjUBOtONefk1uMJSBkQmQ@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Christophe Henri RICARD; Jean-Luc BLANC
Subject: Re: [tpmdd-devel] [PATCH 10/16] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

On Wed, Oct 08, 2014 at 07:38:30AM +0200, Christophe RICARD wrote:
>  Hi Jason,
> 
> The freezer header is here for the freezing(current) function call in 
> get_burstcount().

But freezing(current) does not occur in patch 10/16.

Is this include in the right patch in the series?

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: [tpmdd-devel] [PATCH 10/16] tpm/tpm_i2c_stm_st33: Replace wait_for_serirq_timeout by wait_for_tpm_stat

[Jason Gunthorpe]

On Wed, Oct 08, 2014 at 10:26:57PM +0200, Christophe Henri RICARD wrote:
> Hi Jason,
> 
> Patch 10/16 includes the following lines:
> +		if (r == -ERESTARTSYS && freezing(current)) {
> +			clear_thread_flag(TIF_SIGPENDING);
> +			goto again;
> +		}
> 
> Line 149 in this patch. I am surprise you are saying it does not occur.

Okay, I see it now, it is in request_locality, not get_burstcount

And I see this code fragment is copied from commit 20b87bbfada, so it
seems reasonable - though I wonder if all wait_event's need similar
protection?

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

[PATCH 11/16] 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 e99bb78..660ff8b 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -187,7 +187,6 @@ static u8 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);
 
 	return interrupt;
 } /* clear_interruption() */
@@ -753,10 +752,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_FIFO_AVALAIBLE_INT
 			|  TPM_INTF_WAKE_UP_READY_INT
@@ -773,10 +768,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;
 
 		tpm_gen_interrupt(chip);
-- 
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 11/16] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 10:03:04PM +0200, Christophe Ricard wrote:
> 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 e99bb78..660ff8b 100644
> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
> @@ -187,7 +187,6 @@ static u8 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);

Hum, I don't have a chip datasheet here, but is this really useless?

It looks like a synchronizing read to me - ie completing the read at
the CPU is enough to know that the TPM itself has processed the prior
write and is known to have lowered the level triggered interrupt..

A read like this is the sort of thing that you'd expect to avoid the
udelay in your 'Add delay before release_locality to make sure irq are
cleared'

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: [tpmdd-devel] [PATCH 11/16] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

[Christophe RICARD]

Hi Jason,

On 08/10/2014 00:09, Jason Gunthorpe wrote:
> On Tue, Oct 07, 2014 at 10:03:04PM +0200, Christophe Ricard wrote:
>> 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 e99bb78..660ff8b 100644
>> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
>> @@ -187,7 +187,6 @@ static u8 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);
> Hum, I don't have a chip datasheet here, but is this really useless?
>
> It looks like a synchronizing read to me - ie completing the read at
> the CPU is enough to know that the TPM itself has processed the prior
> write and is known to have lowered the level triggered interrupt..
>
> A read like this is the sort of thing that you'd expect to avoid the
> udelay in your 'Add delay before release_locality to make sure irq are
> cleared'
I believe this is completely 2 different things. The delay before the
release locality is to make sure that the isr will be service before 
release_locality to guarantee
that any pending interrupt are cleared while the locality is active.
Here i just want to save 2 i2c transaction as only 1 write is enough to 
get the register change as effective.
>
> Jason
Best Regards
Christophe
--
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 11/16] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

[Jason Gunthorpe]

On Wed, Oct 08, 2014 at 07:36:04AM +0200, Christophe RICARD wrote:

> I believe this is completely 2 different things. The delay before the
> release locality is to make sure that the isr will be service before
> release_locality to guarantee
> that any pending interrupt are cleared while the locality is active.
> Here i just want to save 2 i2c transaction as only 1 write is enough
> to get the register change as effective.

I think I follow the interrupt changes a bit better now..

First of all, things are spread out too much, ie patch 10 makes the
actual ISR handler change but patch 13 corrects the locking bug
introduced in patch 10, and patch 7 switches to the threaded irq
required by patch 13 - this should all be in the same patch.

Second, I don't think switching to threaded IRQs and then using I2C
transactions in the handler is a great idea. I think you should follow
the pattern in the nuvoton driver:

The IRQ handler simply records the interrupt occured, triggers a WQ
and disables the IRQ:

static irqreturn_t i2c_nuvoton_int_handler(int dummy, void *dev_id)
{
	struct tpm_chip *chip = dev_id;
	struct priv_data *priv = chip->vendor.priv;

	priv->intrs++;
	wake_up(&chip->vendor.read_queue);
	disable_irq_nosync(chip->vendor.irq);
	return IRQ_HANDLED;
}

The ops explicitly enables the IRQ before sleeping waiting on the
output FIFO:

	if (chip->vendor.irq && queue) {
		s32 rc;
		struct priv_data *priv = chip->vendor.priv;
		unsigned int cur_intrs = priv->intrs;

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

For your chip you might need to ack the IRQ before writing a new
command to the input FIFO.

1) This gets rid of the udelay, the IRQ doesn't do anything so any
   acks can be sequenced properly with the request_locality
2) This gets rid of the locking, the IRQ doesn't attempt to ack, and
   acks can be sequenced before any enable_irq

If your chip is sane like other TPMs the IRQ pin will only be asserted
while there is data pending in the out command FIFO, reading the FIFO
should naturally clear the IRQ and the acking process may be entirely
unnecessary and can be removed. If you have to ack via that weird
read/write sequence then it should always be done before submitting a
new command to the input fifo.

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: [tpmdd-devel] [PATCH 11/16] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

[Christophe Henri RICARD]

Hi Jason,

> If your chip is sane like other TPMs the IRQ pin will *only* be asserted while there is data pending in the out command FIFO, reading the FIFO *should naturally clear the IRQ *and the acking process may be entirely unnecessary and can be removed.
I believe this assessment is wrong according to existing specifications and current implementation and I will explain you why:

Our TPM is managing the TPM TIS registers Interrupt Enable and Interrupt Status.
The TPM register Interrupt Enable support globalIntEnable (bit 31), commandReadyEnable (bit 7), fifoAvailableEnable (bit 6), Wakeup ready enable (bit 5), loc4SoftRelease(bit 3), stsValidIntEnable(bit 1), dataAvailIntEnable(bit 0).
Except fifoAvailableEnable (bit 6), Wakeup ready enable (bit 5), loc4SoftRelease(bit 3) all of them are specified in the TCG TIS specification.

The TPM register Interrupt Status support commandReadyIntOccured(bit 7), fifoAvailableOccured(bit 6), WakeupReadyOccured(bit 5), loc4SoftRelease(bit 3), localityChangeIntOccured(bit 2), stsValidIntOccured(bit 1), dataAvailIntOccured(bit 0).
Except fifoAvailableOccured(bit 6), WakeupReadyOccured(bit 5), loc4SoftRelease(bit 3) all of them are specified in the TCG TIS specification.

When any enabled interrupt is getting active, the serirq line will get active (high state reversed from lpc implementation) but it will not tell which one. It could be any of the interrupt status register.
Still according to the TCG_PCClientTPMInterfaceSpecification_TIS, the only when to clear a pending interrupt is to write a 1 to the corresponding bit in the TPM_INT_STATUS register.

According to which one is triggered, the wait queue to wake up is different it could be chip->vendor.read_queue or chip->vendor.int_queue.
According to other driver implementation:
- chip->vendor.read_queue is used to signal data availability.
- chip->vendor.int_queue is used to signal any other interrupt sts_valid_int, cmd_read_int, locality_change_int.

As a possible clean up, I can see:
- the locality_change_int in the handler may not be manage in my isr as it is not supposed to happen in the OS.
- the interrupt TPM_INTF_LOCALITY_CHANGE_INT, TPM_INTF_FIFO_AVAILABLE_INT, TPM_INTF_WAKE_UP_READY_INT does not need to be activated.

The udelay before the release_locality has be chosen small in order to reduce the impact in case the driver is used in polling mode.

I would point out as well the int_queue initialization in the i2c_nuvoton_probe which is never used in the tpm_i2c_nuvoton.c nor in any tpm core file.

The only point that could be raised is that there is no TPM 1.2 protocol specification for I2C. During our chip implementation, we tried to fit best to existing documentation. 
Therefore I believe claiming this I2C TPM implementation or this one should be taken as reference is not appropriate as per previous statement.

As a conclusion to this, I believe I can add the clean-up pointed out previously and I will try to respin and rework patch 10, 13 and 7.

However, I am not in favor to change to non-threaded irq unless I have a clear and convincing argument to do so.

I will submit a v2 patchset including as much as possible fix/clean according to your feedback soon.

Thank you for your feedback.

Best Regards
Christophe

-----Original Message-----
From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org] 
Sent: mercredi 8 octobre 2014 19:12
To: Christophe RICARD
Cc: peterhuewe-Mmb7MZpHnFY@public.gmane.org; ashley-fm2HMyfA2y6tG0bUXCXiUA@public.gmane.org; tpmdd-yWjUBOtONefk1uMJSBkQmQ@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Christophe Henri RICARD; Jean-Luc BLANC
Subject: Re: [tpmdd-devel] [PATCH 11/16] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

On Wed, Oct 08, 2014 at 07:36:04AM +0200, Christophe RICARD wrote:

> I believe this is completely 2 different things. The delay before the 
> release locality is to make sure that the isr will be service before 
> release_locality to guarantee that any pending interrupt are cleared 
> while the locality is active.
> Here i just want to save 2 i2c transaction as only 1 write is enough 
> to get the register change as effective.

I think I follow the interrupt changes a bit better now..

First of all, things are spread out too much, ie patch 10 makes the actual ISR handler change but patch 13 corrects the locking bug introduced in patch 10, and patch 7 switches to the threaded irq required by patch 13 - this should all be in the same patch.

Second, I don't think switching to threaded IRQs and then using I2C transactions in the handler is a great idea. I think you should follow the pattern in the nuvoton driver:

The IRQ handler simply records the interrupt occured, triggers a WQ and disables the IRQ:

static irqreturn_t i2c_nuvoton_int_handler(int dummy, void *dev_id) {
	struct tpm_chip *chip = dev_id;
	struct priv_data *priv = chip->vendor.priv;

	priv->intrs++;
	wake_up(&chip->vendor.read_queue);
	disable_irq_nosync(chip->vendor.irq);
	return IRQ_HANDLED;
}

The ops explicitly enables the IRQ before sleeping waiting on the output FIFO:

	if (chip->vendor.irq && queue) {
		s32 rc;
		struct priv_data *priv = chip->vendor.priv;
		unsigned int cur_intrs = priv->intrs;

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

For your chip you might need to ack the IRQ before writing a new command to the input FIFO.

1) This gets rid of the udelay, the IRQ doesn't do anything so any
   acks can be sequenced properly with the request_locality
2) This gets rid of the locking, the IRQ doesn't attempt to ack, and
   acks can be sequenced before any enable_irq

If your chip is sane like other TPMs the IRQ pin will only be asserted while there is data pending in the out command FIFO, reading the FIFO should naturally clear the IRQ and the acking process may be entirely unnecessary and can be removed. If you have to ack via that weird read/write sequence then it should always be done before submitting a new command to the input fifo.

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: [tpmdd-devel] [PATCH 11/16] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers

[Jason Gunthorpe]

On Thu, Oct 09, 2014 at 07:35:07PM +0200, Christophe Henri RICARD wrote:
> Hi Jason,
> 
> > If your chip is sane like other TPMs the IRQ pin will *only* be
> > asserted while there is data pending in the out command FIFO,
> > reading the FIFO *should naturally clear the IRQ *and the acking
> > process may be entirely unnecessary and can be removed.

> I believe this assessment is wrong according to existing
> specifications and current implementation and I will explain you
> why:

It does sound like it is not how the ST33 chip operates.

> Our TPM is managing the TPM TIS registers Interrupt Enable and
> Interrupt Status.

The TIS register mapping was intended for environments where register
access is not expensive, and can be done from an ISR. I2C is not such
an environment, which is why other vendors are not using such a strict
mapping of the TIS registers.

That doesn't really change anything, it just means the driver has to
do more I2C transactions to manage this extra chip state.

> According to which one is triggered, the wait queue to wake up is
> different it could be chip->vendor.read_queue or
> chip->vendor.int_queue.

These different queues are not needed, the driver knows what it is
waiting for when it enables the interrupt handler, and can manipulate
the interrupt mask if necessary for special cases. Multiple queues
make more sense when the ISR can cheaply read the status register,
which is not true for I2C.

> I would point out as well the int_queue initialization in the
> i2c_nuvoton_probe which is never used in the tpm_i2c_nuvoton.c nor
> in any tpm core file.

Chip structure members that are not used in the core code are hold
overs from the ancient core design. Drivers ideally should not use
them, favoring their own structures in their driver private
allocation. Someday that will get cleaned up.

Don't get confused that int_queue and read_queue are in the global
chip, they have no special meaning, modern drivers should not use
them at all.

> However, I am not in favor to change to non-threaded irq unless I
> have a clear and convincing argument to do so.

The need for the udelay should be all the convincing required. The
reason for the udelay clearly shows the driver has a synchronization
problem - and sleeping to solve synchronization problem is rarely
correct.

This is especially true since I've already explained how to design so
everything is synchronous and solve the issue.

Again:

In TPM the interrupt is not delivering an asynchronous notification,
everything is synchronous to the driver state, ISRs happen only to
indicate completion of driver initiated actions.

This is why the synchronous flow I suggested is much safer and better,
the driver just can't get the situation where the IRQ cannot safely
run because the main driver thread has changed the TPM state.

To summarize, the flow for an interrupt wait becomes very
simple, ie to wait for command ready:

 - Do write to clear all interrupts
 - do read on command ready
 - if not ready then write to enable only the command ready occured interrupt
 - enable_irq
 - wait for irq w/ timer
 - do read on command ready
 - if not ready and no timeout, do write to clear all interrupts,
   enable_irq, loop.
 - if not ready and timeout, disable irq, return error

All sleepables follow the same synchronous pattern. This makes the
driver compeltely single threaded so no analysis for thread problems
is need. No locking primitives are needed. The inter-locking problem
with request_locality goes away.

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

[PATCH 12/16] 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 660ff8b..ab2a675 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 c5e4b7f..88f9cb1 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 13/16] tpm/tpm_i2c_stm_st33: Add tpm_lock mutex for safe irq management

[Christophe Ricard]

Adding tpm_lock mutex in order to guarantee that a i2c_read_data or a
i2c_write_data will not get interrupted by a threaded interrupt.

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

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index ab2a675..8d32ade 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/freezer.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>
@@ -106,7 +105,7 @@ enum tis_defaults {
 
 struct tpm_stm_dev {
 	struct i2c_client *client;
-	struct mutex lock;
+	struct mutex tpm_lock;
 	struct tpm_chip *chip;
 	u8 buf[TPM_BUFSIZE + 1];
 	int io_serirq;
@@ -147,6 +146,7 @@ static int read8_reg(struct tpm_stm_dev *tpm_dev, u8 tpm_register,
 	status = write8_reg(tpm_dev, tpm_register, &data, 1);
 	if (status == 2)
 		status = i2c_master_recv(tpm_dev->client, tpm_data, tpm_size);
+
 	return status;
 } /* read8_reg() */
 
@@ -163,6 +163,18 @@ static int read8_reg(struct tpm_stm_dev *tpm_dev, u8 tpm_register,
 	(write8_reg(tpm_dev, tpm_register | \
 	TPM_WRITE_DIRECTION, tpm_data, tpm_size))
 
+
+static int i2c_write_data(struct tpm_stm_dev *tpm_dev, u8 tpm_register,
+				u8 *tpm_data, u16 tpm_size)
+{
+	u8 status;
+
+	mutex_lock(&tpm_dev->tpm_lock);
+	status = I2C_WRITE_DATA(tpm_dev, tpm_register, tpm_data, tpm_size);
+	mutex_unlock(&tpm_dev->tpm_lock);
+
+	return status;
+} /* i2c_write_data() */
 /*
  * I2C_READ_DATA
  * Recv byte from the TIS register according to the ST33ZP24 I2C protocol.
@@ -175,6 +187,18 @@ static int read8_reg(struct tpm_stm_dev *tpm_dev, u8 tpm_register,
 #define I2C_READ_DATA(tpm_dev, tpm_register, tpm_data, tpm_size) \
 	(read8_reg(tpm_dev, tpm_register, tpm_data, tpm_size))
 
+static int i2c_read_data(struct tpm_stm_dev *tpm_dev, u8 tpm_register,
+				u8 *tpm_data, u16 tpm_size)
+{
+	u8 status;
+
+	mutex_lock(&tpm_dev->tpm_lock);
+	status = I2C_READ_DATA(tpm_dev, tpm_register, tpm_data, tpm_size);
+	mutex_unlock(&tpm_dev->tpm_lock);
+
+	return status;
+} /* i2c_read_data() */
+
 /*
  * clear_interruption
  * clear the TPM interrupt register.
@@ -184,8 +208,8 @@ static u8 clear_interruption(struct tpm_stm_dev *tpm_dev)
 {
 	u8 interrupt;
 
-	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);
+	i2c_write_data(tpm_dev, TPM_INT_STATUS, &interrupt, 1);
 
 	return interrupt;
 } /* clear_interruption() */
@@ -202,7 +226,7 @@ static void tpm_stm_i2c_cancel(struct tpm_chip *chip)
 	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
 	data = TPM_STS_COMMAND_READY;
-	I2C_WRITE_DATA(tpm_dev, TPM_STS, &data, 1);
+	i2c_write_data(tpm_dev, TPM_STS, &data, 1);
 } /* tpm_stm_i2c_cancel() */
 
 /*
@@ -217,7 +241,7 @@ static u8 tpm_stm_i2c_status(struct tpm_chip *chip)
 
 	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
-	I2C_READ_DATA(tpm_dev, TPM_STS, &data, 1);
+	i2c_read_data(tpm_dev, TPM_STS, &data, 1);
 	return data;
 } /* tpm_stm_i2c_status() */
 
@@ -235,7 +259,7 @@ static int check_locality(struct tpm_chip *chip)
 
 	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
-	status = I2C_READ_DATA(tpm_dev, 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))
@@ -263,7 +287,7 @@ static int request_locality(struct tpm_chip *chip)
 	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 
 	data = TPM_ACCESS_REQUEST_USE;
-	r = I2C_WRITE_DATA(tpm_dev, TPM_ACCESS, &data, 1);
+	r = i2c_write_data(tpm_dev, TPM_ACCESS, &data, 1);
 	if (r < 0)
 		goto end;
 
@@ -308,7 +332,7 @@ static void release_locality(struct tpm_chip *chip)
 	tpm_dev = (struct tpm_stm_dev *)TPM_VPRIV(chip);
 	data = TPM_ACCESS_ACTIVE_LOCALITY;
 
-	I2C_WRITE_DATA(tpm_dev, TPM_ACCESS, &data, 1);
+	i2c_write_data(tpm_dev, TPM_ACCESS, &data, 1);
 }
 
 /*
@@ -328,13 +352,13 @@ static int get_burstcount(struct tpm_chip *chip)
 	stop = jiffies + chip->vendor.timeout_d;
 	do {
 		tpm_reg = TPM_STS + 1;
-		status = I2C_READ_DATA(tpm_dev, 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(tpm_dev, tpm_reg, &temp, 1);
+		status = i2c_read_data(tpm_dev, tpm_reg, &temp, 1);
 		if (status < 0)
 			goto end;
 
@@ -366,12 +390,12 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 	       wait_for_tpm_stat(chip,
 			     TPM_STS_DATA_AVAIL | TPM_STS_VALID,
 			     chip->vendor.timeout_c,
-			     &chip->vendor.read_queue, true) == 0) {
+			     &chip->vendor.read_queue, false) == 0) {
 		burstcnt = get_burstcount(chip);
 		if (burstcnt < 0)
 			return burstcnt;
 		len = min_t(int, burstcnt, count - size);
-		I2C_READ_DATA(tpm_dev, TPM_DATA_FIFO, buf + size, len);
+		i2c_read_data(tpm_dev, TPM_DATA_FIFO, buf + size, len);
 		size += len;
 	}
 	return size;
@@ -456,7 +480,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, size);
 		if (r < 0)
 			goto out_err;
 
@@ -469,7 +493,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 		goto out_err;
 	}
 
-	r = I2C_WRITE_DATA(tpm_dev, TPM_DATA_FIFO, buf + len - 1, 1);
+	r = i2c_write_data(tpm_dev, TPM_DATA_FIFO, buf + len - 1, 1);
 	if (r < 0)
 		goto out_err;
 
@@ -480,7 +504,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 	}
 
 	data = TPM_STS_GO;
-	I2C_WRITE_DATA(tpm_dev, TPM_STS, &data, 1);
+	i2c_write_data(tpm_dev, TPM_STS, &data, 1);
 
 	return len;
 out_err:
@@ -730,6 +754,7 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
 
 	chip->vendor.locality = LOCALITY0;
+	mutex_init(&tpm_dev->tpm_lock);
 
 	if (interrupts) {
 		/* INTERRUPT Setup */
@@ -758,12 +783,12 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			|  TPM_INTF_STS_VALID_INT
 			|  TPM_INTF_DATA_AVAIL_INT;
 
-		r = I2C_WRITE_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
+		r = i2c_write_data(tpm_dev, TPM_INT_ENABLE, &intmask, 1);
 		if (r < 0)
 			goto _tpm_clean_answer;
 
 		intmask = TPM_GLOBAL_INT_ENABLE;
-		r = I2C_WRITE_DATA(tpm_dev, (TPM_INT_ENABLE + 3), &intmask, 1);
+		r = i2c_write_data(tpm_dev, (TPM_INT_ENABLE + 3), &intmask, 1);
 		if (r < 0)
 			goto _tpm_clean_answer;
 
-- 
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 13/16] tpm/tpm_i2c_stm_st33: Add tpm_lock mutex for safe irq management

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 10:03:06PM +0200, Christophe Ricard wrote:
> Adding tpm_lock mutex in order to guarantee that a i2c_read_data or a
> i2c_write_data will not get interrupted by a threaded interrupt.

Can you elaborate on this?

Any call from the TPM core itself through 'ops' is locked already, and
I don't see this driver's IRQ handler doing I2C ops..

What scenario is this protecting against?

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: [tpmdd-devel] [PATCH 13/16] tpm/tpm_i2c_stm_st33: Add tpm_lock mutex for safe irq management

[Christophe RICARD]

Hi Jason,

Here i want to prevent a TPM TIS write command sent when a TPM TIS read 
command is on going on the bus.
Basically:
- a TPM TIS write looks like the following format: <W reg data>
- a TPM TIS read looks like the following format: <W reg dummy><R data>

If an interrupt occur a TPM TIS read might be interrupted by a TPM TIS 
write to clear the pending interrupt giving potentially something like that:
<W reg dummy><W reg data> <R data>
The TPM behavior in this situation is unknown...

Best Regards
Christophe
On 07/10/2014 23:56, Jason Gunthorpe wrote:
> On Tue, Oct 07, 2014 at 10:03:06PM +0200, Christophe Ricard wrote:
>> Adding tpm_lock mutex in order to guarantee that a i2c_read_data or a
>> i2c_write_data will not get interrupted by a threaded interrupt.
> Can you elaborate on this?
>
> Any call from the TPM core itself through 'ops' is locked already, and
> I don't see this driver's IRQ handler doing I2C ops..
>
> What scenario is this protecting against?
>
> 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

[PATCH 14/16] 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.

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 8d32ade..de9f12e 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -480,7 +480,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

Re: [tpmdd-devel] [PATCH 14/16] tpm/tpm_i2c_stm_st33: Fix potential bug in tpm_stm_i2c_send

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 10:03:07PM +0200, Christophe Ricard wrote:
> 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.

So, this driver never really worked? I'm guessing sending a larger
command (take ownership, for example) will exceed the burst count and
just blow up?

This should be marked for stable (please see
Documentation/stable_kernel_rules.txt)

Also, please make it the first patch in your series so it applies
cleanly to older kernels.

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 8d32ade..de9f12e 100644
> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
> @@ -480,7 +480,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;
>  
--
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 14/16] tpm/tpm_i2c_stm_st33: Fix potential bug in tpm_stm_i2c_send

[Christophe RICARD]

Hi Jason,

In fact this driver worked because the TPM burstcnt was always big 
enought to support all TPM commands.
In other term burstcnt > len.

No problem for making making this patch as number 1 in this series.

Best Regards
Christophe
On 08/10/2014 00:04, Jason Gunthorpe wrote:
> On Tue, Oct 07, 2014 at 10:03:07PM +0200, Christophe Ricard wrote:
>> 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.
> So, this driver never really worked? I'm guessing sending a larger
> command (take ownership, for example) will exceed the burst count and
> just blow up?
>
> This should be marked for stable (please see
> Documentation/stable_kernel_rules.txt)
>
> Also, please make it the first patch in your series so it applies
> cleanly to older kernels.
>
> 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 8d32ade..de9f12e 100644
>> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
>> @@ -480,7 +480,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;
>>   

--
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 14/16] tpm/tpm_i2c_stm_st33: Fix potential bug in tpm_stm_i2c_send

[Jason Gunthorpe]

On Wed, Oct 08, 2014 at 09:38:39AM +0200, Christophe Ricard wrote:
>    I would just add an additional comment to my previous message.
>    After reviewing Documentation/stable_kernel_rules.txt, it looks like it
>    is *not* necessary to send this patch marked as stable because of the
>    following rule:
>    "- It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing)."
>    As argued previously this patch is more an algorithm fix luckily never
>    seen with the current ST33 I2C TPM devices.
>    I looks like a "This could be a problem..."
>    Do you agree with this ?

Yes, please clarify your patch description with something like:

'This never happens in practice because the burst count is XXX bytes
and the largest TPM command is YYY bytes.'

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

[PATCH 15/16] tpm/tpm_i2c_stm_st33: Add delay before release_locality to make sure irq are cleared

[Christophe Ricard]

In order to manage irq, locality must be active. As Status Ready interrupt is activated,
when going back into ready state with the cancel function, we need to add a little delay
to make sure the irq is going to be serviced before the release_locality is hit.

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

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index de9f12e..4c78845 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -509,6 +509,7 @@ static int tpm_stm_i2c_send(struct tpm_chip *chip, unsigned char *buf,
 	return len;
 out_err:
 	tpm_stm_i2c_cancel(chip);
+	usleep_range(100, 250);
 	release_locality(chip);
 	return r;
 }
@@ -557,6 +558,7 @@ static int tpm_stm_i2c_recv(struct tpm_chip *chip, unsigned char *buf,
 
 out:
 	chip->ops->cancel(chip);
+	usleep_range(100, 250);
 	release_locality(chip);
 	return size;
 }
-- 
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 15/16] tpm/tpm_i2c_stm_st33: Add delay before release_locality to make sure irq are cleared

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 10:03:08PM +0200, Christophe Ricard wrote:
> In order to manage irq, locality must be active. As Status Ready
> interrupt is activated, when going back into ready state with the
> cancel function, we need to add a little delay to make sure the irq
> is going to be serviced before the release_locality is hit.

Using a delay for this seems pretty sketchy, is this supposed to be
waiting for a level sensitive IRQ to desassert? or a queued IRQ to be
delivered?

What is the harm without the udelay?

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

[PATCH 16/16] tpm: 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 4c78845..b0d9bc2 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.
@@ -907,5 +907,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: [PATCH 00/16] ST33 I2C TPM driver cleanup

[Peter Hüwe]

Hi Christophe,


Am Dienstag, 7. Oktober 2014, 22:02:53 schrieb Christophe Ricard:
> Hi Peter,
> 
> The following patchset brings:
> - Some few code clean up from code style up to structure
> - Device tree support keeping static platform data configuration support.
> - Fix 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


I'll try to have a look at them as soon as possible - but with the ELCE/Linux 
Con comming up next week (TPM minisummit! ;) it's a bit hectic at the moment.

Thanks,
Peter
--
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 00/16] ST33 I2C TPM driver cleanup

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 10:02:53PM +0200, Christophe Ricard wrote:
> Hi Peter,
> 
> The following patchset brings:
> - Some few code clean up from code style up to structure
> - Device tree support keeping static platform data configuration support.
> - Fix irq support.
> - Update the GPLv2 license header

I really don't have much to say about this - it looks like the older
driver was never being used so if you have reviewed and tested it
again in a new system I think that is an improvement..

Can you put this on a github or something? It would be nice to see the
file new .c file rather than looking at patches..
 
>   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.

I'm very happy to see this, this is much better than the old way.

>   tpm/tpm_i2c_stm_st33: Replace err/rc/ret by r for a function return
>     code

This is very churny, and I didn't think 'r' was a kernel convention..

>   tpm/tpm_i2c_stm_st33: Replace tpm_st33_* function with tpm_stm_*

What is the part branding? We still have 'st33' in various places..
 
>   tpm/tpm_i2c_stm_st33: Add devicetree structure
>   tpm: dts: st33zp24_i2c: Add DTS Documentation

Peter: FYI, the protocol for device tree stuff is to wait for an ack
from the device tree maintainers on the documentation patch..

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: [tpmdd-devel] [PATCH 00/16] ST33 I2C TPM driver cleanup

[Christophe RICARD]

Hi Jason,

On 08/10/2014 00:25, Jason Gunthorpe wrote:
> On Tue, Oct 07, 2014 at 10:02:53PM +0200, Christophe Ricard wrote:
>> Hi Peter,
>>
>> The following patchset brings:
>> - Some few code clean up from code style up to structure
>> - Device tree support keeping static platform data configuration support.
>> - Fix irq support.
>> - Update the GPLv2 license header
> I really don't have much to say about this - it looks like the older
> driver was never being used so if you have reviewed and tested it
> again in a new system I think that is an improvement..
>
> Can you put this on a github or something? It would be nice to see the
> file new .c file rather than looking at patches..
I will try to.
>   
>>    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.
> I'm very happy to see this, this is much better than the old way.
Those tpm_i2c_buffer were actually there to workaround a standby/resume 
ops on very old kernel (may be 5 years old).
They are not necessary any more and quite uggly to read ;). I am happy 
as well :).
>>    tpm/tpm_i2c_stm_st33: Replace err/rc/ret by r for a function return
>>      code
> This is very churny, and I didn't think 'r' was a kernel convention..
I am not sure r is really a kernel convention but i believe at least we 
need to have a convention in each driver.
>
>>    tpm/tpm_i2c_stm_st33: Replace tpm_st33_* function with tpm_stm_*
> What is the part branding? We still have 'st33' in various places..
The branding of this chip is st33zp24. Still, i have tried to have a 
convention for function naming.
There might be st33 at some place in platform data stucture for example.
>   
>>    tpm/tpm_i2c_stm_st33: Add devicetree structure
>>    tpm: dts: st33zp24_i2c: Add DTS Documentation
> Peter: FYI, the protocol for device tree stuff is to wait for an ack
> from the device tree maintainers on the documentation patch..
>
> 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