Re: TPM: STMicroelectronics st33 driver SPI

[Ashley Lai <ashley@ashleylai.com>]

Please see my comments below.  Did you get a chance to run the trousers 
testsuite?

git://trousers.git.sourceforge.net/gitroot/trousers/testsuite

+static u8 spi_read8_reg(struct tpm_chip *tpm, u8 tpm_register,
+                   u8 *tpm_data, u16 tpm_size)
+{
+       ...
+
+       /* header + status byte + size of the data + status byte */
+       value = spi_sync_transfer(dev, &xfer, 1);
+
+       if (tpm_size > 0 && value == 0) {
+               nbr_dummy_bytes = 2;
Why use 2 for the dummy bytes?  Some comments here would be 
helpful.  Should we use a #define for 2?

...

+static unsigned long wait_for_serirq_timeout(struct tpm_chip *chip,
+        bool condition, unsigned long timeout)
+{
+       long status = 0;
+       struct spi_device *client;
+       struct st33zp24_platform_data *pin_infos;
+
+       client = (struct spi_device __force *)chip->vendor.iobase;
+       pin_infos = client->dev.platform_data;
+
+       status = wait_for_completion_interruptible_timeout(
+                               &pin_infos->irq_detection, timeout);
+       if (status > 0)
+               enable_irq(gpio_to_irq(pin_infos->io_serirq));
+       gpio_direction_input(pin_infos->io_serirq);
+
+       if (!status)
+               return -EBUSY;
This function returns -EBUSY but the return function type is unsigned.

...

+static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+       u32 size = 0, burstcnt, len;
+       long status = 0;
+
+       while (size < count &&
+              wait_for_stat(chip,
+                            TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+                            chip->vendor.timeout_c,
+                            &chip->vendor.read_queue)
+                                               == 0) {
+               burstcnt = get_burstcount(chip);
+               len = min_t(int, burstcnt, count - size);

What if get_burstcount returns -EBUSY?  Should burstcnt be checked
and handled here in case of error?

+               status = spi_read8_reg(chip, TPM_DATA_FIFO, buf + size, 
len);
+               if (status < 0)
+                       return status;
+
+
Extra empty line here.
+               size += len;

See above comment.  If len is -EBUSY, there might be an issue here.

+       }
+return size;

...

+static int tpm_stm_spi_send(struct tpm_chip *chip, unsigned char *buf,
+                           size_t len)
+{
...
+	for (i = 0; i < len - 1;) {
+		burstcnt = get_burstcount(chip);
Again burstcnt needs to be checked to make sure it is > 0.

+		size = min_t(int, len - i - 1, burstcnt);
+		ret = spi_write8_reg(chip, TPM_DATA_FIFO, buf, size);
+		if (ret < 0)
+			goto out_err;
+		i += size;
+	}
...
+       /* write last byte */
+       spi_write8_reg(chip, TPM_DATA_FIFO, buf + len - 1, 1);
Should we check for the return code here?  A few lines below it checks 
for the return value.
...
+
+
+       /* go and do it */
+       data = TPM_STS_GO;
+       ret = spi_write8_reg(chip, TPM_STS, &data, 1);

Regards,
--Ashley Lai