Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Adrian Ratiu <adrian.ratiu@collabora.com>
To:     Helen Koike <helen.koike@collabora.com>,
        Adrian Ratiu <adrian.ratiu@collabora.com>,
        linux-integrity@vger.kernel.org
Cc:     Peter Huewe <peterhuewe@gmx.de>,
        Jarkko Sakkinen <jarkko@kernel.org>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Duncan Laurie <dlaurie@chromium.org>,
        Stephen Boyd <swboyd@chromium.org>, kernel@collabora.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] char: tpm: add i2c driver for cr50
In-Reply-To: <b0dba892-5e5b-a625-c072-a5d1eb95ba33@collabora.com>
References: <20201120172345.4040187-1-adrian.ratiu@collabora.com>
 <b0dba892-5e5b-a625-c072-a5d1eb95ba33@collabora.com>
Date:   Mon, 23 Nov 2020 19:41:23 +0200
Message-ID: <87blfoj7i4.fsf@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed
Precedence: bulk

On Fri, 20 Nov 2020, Helen Koike <helen.koike@collabora.com> 
wrote:
> Hello Adrian, 
> 
> I just spotted small things (nothing major), please see below. 

Hi Helen,

I've addressed all the points you raised and left some minor 
comments below, but I will wait a little longer before posting v3 
so others will also have a chance to review. Thank you very much!

> 
> On 11/20/20 2:23 PM, Adrian Ratiu wrote: 
>> From: "dlaurie@chromium.org" <dlaurie@chromium.org>  Add TPM 
>> 2.0 compatible I2C interface for chips with cr50 firmware. 
>> The firmware running on the currently supported H1 MCU requires 
>> a special driver to handle its specific protocol, and this 
>> makes it unsuitable to use tpm_tis_core_* and instead it must 
>> implement the underlying TPM protocol similar to the other I2C 
>> TPM drivers.   - All 4 byes of status register must be 
>> read/written at once. 
> 
> s/byes/bytes 
> 
>> - FIFO and burst count is limited to 63 and must be drained by 
>> AP.  - Provides an interrupt to indicate when read response 
>> data is ready and when the TPM is finished processing write 
>> data.   This driver is based on the existing infineon I2C TPM 
>> driver, which most closely matches the cr50 i2c protocol 
>> behavior.   Cc: Helen Koike <helen.koike@collabora.com> 
>> Signed-off-by: Duncan Laurie <dlaurie@chromium.org> 
>> [swboyd@chromium.org: Depend on i2c even if it's a module, 
>> replace boilier plate with SPDX tag, drop asm/byteorder.h 
>> include, simplify return from probe] Signed-off-by: Stephen 
>> Boyd <swboyd@chromium.org> Signed-off-by: Fabien Lahoudere 
>> <fabien.lahoudere@collabora.com> Signed-off-by: Adrian Ratiu 
>> <adrian.ratiu@collabora.com> --- Changes in v2: 
>>   - Various small fixes all over (reorder includes, 
>>   MAX_BUFSIZE, comments, etc) - Reworked return values of 
>>   i2c_wait_tpm_ready() to fix timeout mis-handling 
>> so ret == 0 now means success, the wait period jiffies is 
>> ignored because that number is meaningless and return a proper 
>> timeout error in case jiffies == 0. 
>>   - Make i2c default to 1 message per transfer (requested by 
>>   Helen) - Move -EIO error reporting to transfer function to 
>>   cleanup transfer() itself 
>> and its R/W callers 
>>   - Remove magic value hardcodings and introduce enum 
>>   force_release. 
>>  v1 posted at https://lkml.org/lkml/2020/2/25/349  Applies on 
>> next-20201120, tested on Chromebook EVE.  --- 
>>  drivers/char/tpm/Kconfig            |  10 + 
>>  drivers/char/tpm/Makefile           |   2 + 
>>  drivers/char/tpm/tpm_tis_i2c_cr50.c | 768 
>>  ++++++++++++++++++++++++++++ 3 files changed, 780 
>>  insertions(+) create mode 100644 
>>  drivers/char/tpm/tpm_tis_i2c_cr50.c 
>>  diff --git a/drivers/char/tpm/Kconfig 
>> b/drivers/char/tpm/Kconfig index a18c314da211..4308f9ca7a43 
>> 100644 --- a/drivers/char/tpm/Kconfig +++ 
>> b/drivers/char/tpm/Kconfig @@ -86,6 +86,16 @@ config 
>> TCG_TIS_SYNQUACER 
>>  	  To compile this driver as a module, choose  M here; the 
>>  module will be called tpm_tis_synquacer.  
>> +config TCG_TIS_I2C_CR50 +	tristate "TPM Interface 
>> Specification 2.0 Interface (I2C - CR50)" +	depends on I2C + 
>> select TCG_CR50 +	help +	  This is a driver for the Google 
>> cr50 I2C TPM interface which is a +	  custom microcontroller 
>> and requires a custom i2c protocol interface +	  to 
>> handle the limitations of the hardware.  To compile this driver 
>> +	  as a module, choose M here; the module will be called 
>> tcg_tis_i2c_cr50.  + 
>>  config TCG_TIS_I2C_ATMEL tristate "TPM Interface Specification 
>>  1.2 Interface (I2C - Atmel)" depends on I2C 
>> diff --git a/drivers/char/tpm/Makefile 
>> b/drivers/char/tpm/Makefile index 84db4fb3a9c9..66d39ea6bd10 
>> 100644 --- a/drivers/char/tpm/Makefile +++ 
>> b/drivers/char/tpm/Makefile @@ -27,6 +27,8 @@ 
>> obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o 
>>  tpm_tis_spi-y := tpm_tis_spi_main.o 
>>  tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o  
>> +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o + 
>>  obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o 
>>  obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o 
>>  obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o 
>> diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c 
>> b/drivers/char/tpm/tpm_tis_i2c_cr50.c new file mode 100644 
>> index 000000000000..37555dafdca0 --- /dev/null +++ 
>> b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -0,0 +1,768 @@ +// 
>> SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2016 Google 
>> Inc.  + * + * Based on Linux Kernel TPM driver by + * Peter 
>> Huewe <peter.huewe@infineon.com> + * Copyright (C) 2011 
>> Infineon Technologies + */ + +/* + * cr50 is a firmware for H1 
>> secure modules that requires special + * handling for the I2C 
>> interface.  + * + * - Use an interrupt for transaction status 
>> instead of hardcoded delays + * - Must use write+wait+read read 
>> protocol + * - All 4 bytes of status register must be 
>> read/written at once + * - Burst count max is 63 bytes, and 
>> burst count behaves + *   slightly differently than other I2C 
>> TPMs + * - When reading from FIFO the full burstcnt must be 
>> read + *   instead of just reading header and determining the 
>> remainder + */ + +#include <linux/acpi.h> +#include 
>> <linux/completion.h> +#include <linux/i2c.h> +#include 
>> <linux/interrupt.h> +#include <linux/module.h> +#include 
>> <linux/pm.h> +#include <linux/slab.h> +#include <linux/wait.h> 
>> + +#include "tpm_tis_core.h" + +#define CR50_MAX_BUFSIZE	64 
>> +#define CR50_TIMEOUT_SHORT_MS	2	/* Short timeout 
>> during transactions */ +#define CR50_TIMEOUT_NOIRQ_MS	20 
>> /* Timeout for TPM ready without IRQ */ +#define 
>> CR50_I2C_DID_VID	0x00281ae0L +#define CR50_I2C_MAX_RETRIES 
>> 3	/* Max retries due to I2C errors */ +#define 
>> CR50_I2C_RETRY_DELAY_LO	55	/* Min usecs between 
>> retries on I2C */ +#define CR50_I2C_RETRY_DELAY_HI	65	/* 
>> Max usecs between retries on I2C */ + +#define 
>> TPM_I2C_ACCESS(l)	(0x0000 | ((l) << 4)) +#define 
>> TPM_I2C_STS(l)		(0x0001 | ((l) << 4)) +#define 
>> TPM_I2C_DATA_FIFO(l)	(0x0005 | ((l) << 4)) +#define 
>> TPM_I2C_DID_VID(l)	(0x0006 | ((l) << 4)) + +struct priv_data 
>> { +	int irq; +	int locality; +	struct completion 
>> tpm_ready; +	u8 buf[CR50_MAX_BUFSIZE]; +}; + +enum 
>> force_release { +	CR50_NO_FORCE = 0x0, +	CR50_FORCE = 0x1, 
>> +}; 
> 
> No need for hexadecimal

Indeed, just a bad habbit of mine, will drop the numbers entirely 
as the values are always the same.
 
> 
>> + +/* + * The cr50 interrupt handler just signals waiting 
>> threads that the + * interrupt was asserted.  It does not do 
>> any processing triggered + * by interrupts but is instead used 
>> to avoid fixed delays.  + * + * @dummy: unuesed parameter 
> 
> s/unuesed/unused 
>

Really should enable my spellchecker :) Thanks. All mispellings 
you pointed out are fixed now.
 
>> + * @dev_id: TPM chip information + */ +static irqreturn_t 
>> cr50_i2c_int_handler(int dummy, void *dev_id) +{ +	struct 
>> tpm_chip *chip = dev_id; +	struct priv_data *priv = 
>> dev_get_drvdata(&chip->dev); + + 
>> complete(&priv->tpm_ready); + +	return IRQ_HANDLED; +} + 
>> +/* + * Wait for completion interrupt if available, otherwise 
>> use a fixed + * delay for the TPM to be ready.  + * + * @chip: 
>> TPM chip information + * + * Returns 0 for success, negative 
>> number for timeout + */ +static int 
>> cr50_i2c_wait_tpm_ready(struct tpm_chip *chip) +{ +	struct 
>> priv_data *priv = dev_get_drvdata(&chip->dev); + +	/* Use a 
>> safe fixed delay if interrupt is not supported */ +	if 
>> (priv->irq <= 0) { +		msleep(CR50_TIMEOUT_NOIRQ_MS); + 
>> return 0; +	} + +	/* Wait for interrupt to indicate TPM is 
>> ready to respond */ +	if 
>> (!wait_for_completion_timeout(&priv->tpm_ready, + 
>> msecs_to_jiffies(chip->timeout_a))) { + 
>> dev_warn(&chip->dev, "Timeout waiting for TPM ready\n"); + 
>> return -ETIMEDOUT; +	} + +	return 0; +} + +/* + * 
>> cr50_i2c_enable_tpm_irq - enable TPM irq + * + * @chip: TPM 
>> chip information + */ +static void 
>> cr50_i2c_enable_tpm_irq(struct tpm_chip *chip) +{ +	struct 
>> priv_data *priv = dev_get_drvdata(&chip->dev); + +	if 
>> (priv->irq > 0) { + 
>> reinit_completion(&priv->tpm_ready); + 
>> enable_irq(priv->irq); +	} +} + +/* + * 
>> cr50_i2c_disable_tpm_irq - disable TPM irq + * + * @chip: TPM 
>> chip information + */ +static void 
>> cr50_i2c_disable_tpm_irq(struct tpm_chip *chip) +{ +	struct 
>> priv_data *priv = dev_get_drvdata(&chip->dev); + +	if 
>> (priv->irq > 0) +		disable_irq(priv->irq); +} + +/* + 
>> * cr50_i2c_transfer_message - transfer a message over i2c + * + 
>> * @dev: device information + * @adapter: i2c adapter + * @msg: 
>> message to transfer + * + * Call unlocked i2c transfer routine 
>> with the provided parameters and retry + * in case of bus 
>> errors. Returns 0 on success, otherwise negative errno.  + */ 
>> +static int cr50_i2c_transfer_message(struct device *dev, + 
>> struct i2c_adapter *adapter, + 
>> struct i2c_msg *msg) +{ +	int rc = 0; 
> 
> No need to init to zero. 
> 
>> +	unsigned int try; + +	for (try = 0; try < 
>> CR50_I2C_MAX_RETRIES; try++) { +		rc = 
>> __i2c_transfer(adapter, msg, 1); +		if (rc == 1) + 
>> return 0; /* Successfully transferred the message */ + 
>> if (try) +			dev_warn(dev, "i2c transfer failed 
>> (attempt %d/%d): %d\n", +				 try + 1, 
>> CR50_I2C_MAX_RETRIES, rc); + 
>> usleep_range(CR50_I2C_RETRY_DELAY_LO, CR50_I2C_RETRY_DELAY_HI); 
>> +	} + +	return -EIO; /* No i2c message transferred */ +} + 
>> +/* + * cr50_i2c_read() - read from TPM register + * + * @chip: 
>> TPM chip information + * @addr: register address to read from + 
>> * @buffer: provided by caller + * @len: number of bytes to read 
>> + * + * 1) send register address byte 'addr' to the TPM + * 2) 
>> wait for TPM to indicate it is ready + * 3) read 'len' bytes of 
>> TPM response into the provided 'buffer' + * + * Returns 
>> negative number for error, 0 for success.  + */ +static int 
>> cr50_i2c_read(struct tpm_chip *chip, u8 addr, u8 *buffer, 
>> size_t len) +{ +	struct i2c_client *client = 
>> to_i2c_client(chip->dev.parent); +	struct i2c_msg msg1 = { 
> 
> Maybe rename to msg_reg_addr (I think it is more meaningful then 
> numbers). 
> 
>> +		.addr = client->addr, +		.len = 1, + 
>> .buf = &addr +	}; +	struct i2c_msg msg2 = { 
> 
> Maybe rename to msg_response (I think it is more meaningful then 
> numbers). 
> 
>> +		.addr = client->addr, +		.flags = I2C_M_RD, 
>> +		.len = len, +		.buf = buffer +	}; + 
>> int rc; + +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); + 
>> +	/* Prepare for completion interrupt */ + 
>> cr50_i2c_enable_tpm_irq(chip); + +	/* Send the register 
>> address byte to the TPM */ +	rc = 
>> cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1); 
>> +	if (rc < 0) +		goto out; + +	/* Wait for TPM to 
>> be ready with response data */ +	rc = 
>> cr50_i2c_wait_tpm_ready(chip); +	if (rc < 0) + 
>> goto out; + +	/* Read response data from the TPM */ +	rc 
>> = cr50_i2c_transfer_message(&chip->dev, client->adapter, 
>> &msg2); + +out: +	cr50_i2c_disable_tpm_irq(chip); + 
>> i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); + +	if 
>> (rc < 0) +		return rc; + +	return 0; +} + +/* + * 
>> cr50_i2c_write() - write to TPM register + * + * @chip: TPM 
>> chip information + * @addr: register address to write to + * 
>> @buffer: data to write + * @len: number of bytes to write + * + 
>> * 1) prepend the provided address to the provided data + * 2) 
>> send the address+data to the TPM + * 3) wait for TPM to 
>> indicate it is done writing + * + * Returns negative number for 
>> error, 0 for success.  + */ +static int cr50_i2c_write(struct 
>> tpm_chip *chip, u8 addr, u8 *buffer, + 
>> size_t len) +{ +	struct priv_data *priv = 
>> dev_get_drvdata(&chip->dev); +	struct i2c_client *client 
>> = to_i2c_client(chip->dev.parent); +	struct i2c_msg msg1 = { 
> 
> Maybe rename to just msg. 
>

Done done and done :)
 
>> +		.addr = client->addr, +		.len = len + 1, + 
>> .buf = priv->buf +	}; +	int rc; + +	if (len > 
>> CR50_MAX_BUFSIZE - 1) +		return -EINVAL; + + 
>> i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT); + +	/* Prepend 
>> the 'register address' to the buffer */ +	priv->buf[0] = 
>> addr; +	memcpy(priv->buf + 1, buffer, len); 
> 
> Shouldn't this copy be done before the lock? 
>

Yes, we can move the copy before the lock because it's always a 
good idea to minimize critical sections. 

>> + +	/* Prepare for completion interrupt */ + 
>> cr50_i2c_enable_tpm_irq(chip); + +	/* Send write request 
>> buffer with address */ +	rc = 
>> cr50_i2c_transfer_message(&chip->dev, client->adapter, &msg1); 
>> +	if (rc < 0) +		goto out; + +	/* Wait for TPM to 
>> be ready, ignore timeout */ + 
>> cr50_i2c_wait_tpm_ready(chip); + +out: + 
>> cr50_i2c_disable_tpm_irq(chip); + 
>> i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT); + +	if 
>> (rc < 0) +		return rc; + +	return 0; +} + +/* + * 
>> cr50_check_locality - verify TPM locality + * + * @chip: TPM 
>> chip information + * + * Returns negative number for error, 0 
>> for success.  + */ +static int cr50_check_locality(struct 
>> tpm_chip *chip) +{ +	u8 mask = TPM_ACCESS_VALID | 
>> TPM_ACCESS_ACTIVE_LOCALITY; +	u8 buf; +	int rc; + 
>> +	rc = cr50_i2c_read(chip, TPM_I2C_ACCESS(0), &buf, 
>> sizeof(buf)); +	if (rc < 0) +		return rc; + +	if 
>> ((buf & mask) == mask) +		return 0; + +	return 
>> -EIO; +} + +/* + * cr50_release_locality - release TPM locality 
>> + * + * @chip: TPM chip information + * @force: flag to force 
>> release if set + */ +static void cr50_release_locality(struct 
>> tpm_chip *chip, +				  enum 
>> force_release force) +{ +	struct priv_data *priv = 
>> dev_get_drvdata(&chip->dev); +	u8 mask = TPM_ACCESS_VALID 
>> | TPM_ACCESS_REQUEST_PENDING; +	u8 addr = 
>> TPM_I2C_ACCESS(priv->locality); +	u8 buf; + +	if 
>> (cr50_i2c_read(chip, addr, &buf, sizeof(buf)) < 0) + 
>> return; + +	if (force || (buf & mask) == mask) { + 
>> buf = TPM_ACCESS_ACTIVE_LOCALITY; + 
>> cr50_i2c_write(chip, addr, &buf, sizeof(buf)); +	} + + 
>> priv->locality = 0; 
> 
> I'm probally missing something, I don't see priv->locality being 
> set to another value other then 0 in the code. 
> 
> Shoul it be set in request_locality() or in 
> cr50_release_locality() ? 
>

Oh I thought I deleted that but turns out it was missed while 
addressing your initial review. Oops. Thanks for the dilligence.

In a nutshell this chip always uses locality 0 and I think that 
private variable is only a reminiscence from the infineon code 
this was based upon, so we should drop it as we don't need to 
track a constant value.
 
>> +} + +/* + * request_locality - request TPM locality + * + * 
>> @chip: TPM chip information + */ +static int 
>> request_locality(struct tpm_chip *chip) 
> 
> All the other function have c50_ prefix. 
> 
>> +{ +	struct priv_data *priv = dev_get_drvdata(&chip->dev); +	u8 
>> buf = TPM_ACCESS_REQUEST_USE; +	unsigned long stop; + 
>> int rc; + +	if (!cr50_check_locality(chip)) + 
>> return 0; + +	rc = cr50_i2c_write(chip, 
>> TPM_I2C_ACCESS(0), &buf, sizeof(buf)); +	if (rc < 0) + 
>> return rc; + +	stop = jiffies + chip->timeout_a; +	do 
>> { +		if (!cr50_check_locality(chip)) { + 
>> priv->locality = 0; +			return 0; + 
>> } +		msleep(CR50_TIMEOUT_SHORT_MS); +	} while 
>> (time_before(jiffies, stop)); + +	return -ETIMEDOUT; +} + 
>> +/* + * cr50 requires all 4 bytes of status register to be read 
>> + * + * @chip: TPM chip information + * + * Returns TPM status 
>> + */ +static u8 cr50_i2c_tis_status(struct tpm_chip *chip) +{ + 
>> struct priv_data *priv = dev_get_drvdata(&chip->dev); +	u8 
>> buf[4]; + +	if (cr50_i2c_read(chip, 
>> TPM_I2C_STS(priv->locality), +			  buf, 
>> sizeof(buf)) < 0) +		return 0; +	return buf[0]; +} 
>> + +/* + * cr50 requires all 4 bytes of status register to be 
>> written + * + * @chip: TPM chip information + */ +static void 
>> cr50_i2c_tis_ready(struct tpm_chip *chip) +{ +	struct 
>> priv_data *priv = dev_get_drvdata(&chip->dev); +	u8 buf[4] 
>> = { TPM_STS_COMMAND_READY }; + +	cr50_i2c_write(chip, 
>> TPM_I2C_STS(priv->locality), buf, sizeof(buf)); + 
>> msleep(CR50_TIMEOUT_SHORT_MS); +} + +/* + * cr50 uses bytes 3:2 
>> of status register for burst count and + * all 4 bytes must be 
>> read + * + * @chip: TPM chip information + * @mask: status mask 
>> + * @burst: return value for burst + * @status: return value 
>> for statis 
> 
> s/statis/status 
> 
>> + * + * Returns negative number for error, 0 for success.  + */ 
>> +static int cr50_i2c_wait_burststs(struct tpm_chip *chip, u8 
>> mask, 
> 
> Maybe rename to cr50_i2c_get_burst_and_status(). 
> 
>> +				  size_t *burst, int *status) 
> 
> Should status be unsigned? 
>

Yes, done.
 
>> +{ +	struct priv_data *priv = dev_get_drvdata(&chip->dev); + 
>> unsigned long stop; +	u8 buf[4]; + +	/* wait for 
>> burstcount */ +	stop = jiffies + chip->timeout_b; +	do 
>> { +		if (cr50_i2c_read(chip, 
>> TPM_I2C_STS(priv->locality), + 
>> buf, sizeof(buf)) < 0) { + 
>> msleep(CR50_TIMEOUT_SHORT_MS); +			continue; 
> 
> Should "status" be set to zero in the begining of the function? 
> If the code hits this "continue" until timeout status remain 
> unchanged. 
>

Yes, nice find.

>> +		}
>> +
>> +		*status = *buf;
>> +		*burst = le16_to_cpup((__le16 *)(buf + 1));
>> +
>> +		if ((*status & mask) == mask &&
>> +		    *burst > 0 && *burst <= CR50_MAX_BUFSIZE - 1)
>> +			return 0;
>> +
>> +		msleep(CR50_TIMEOUT_SHORT_MS);
>> +	} while (time_before(jiffies, stop));
>> +
>> +	dev_err(&chip->dev, "Timeout reading burst and status\n");
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +/*
>> + * cr50_i2c_tis_recv - TPM reception callback
>> + *
>> + * @chip: TPM chip information
>> + * @buf: reception buffer
>> + * @buf_len: buffer length to read
>> + *
>> + * Returns negative number for error, number of bytes read for success.
>> + */
>> +static int cr50_i2c_tis_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len)
>> +{
>> +	struct priv_data *priv = dev_get_drvdata(&chip->dev);
>> +	int status, rc;
>> +	size_t burstcnt, cur, len, expected;
>> +	u8 addr = TPM_I2C_DATA_FIFO(priv->locality);
>> +	u8 mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL;
>> +
>> +	if (buf_len < TPM_HEADER_SIZE)
>> +		return -EINVAL;
>> +
>> +	rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
>> +	if (rc < 0)
>> +		goto out_err;
>> +
>> +	if (burstcnt > buf_len || burstcnt < TPM_HEADER_SIZE) {
>> +		dev_err(&chip->dev,
>> +			"Unexpected burstcnt: %zu (max=%zu, min=%d)\n",
>> +			burstcnt, buf_len, TPM_HEADER_SIZE);
>> +		rc = -EIO;
>> +		goto out_err;
>> +	}
>> +
>> +	/* Read first chunk of burstcnt bytes */
>> +	rc = cr50_i2c_read(chip, addr, buf, burstcnt);
>> +	if (rc < 0) {
>> +		dev_err(&chip->dev, "Read of first chunk failed\n");
>> +		goto out_err;
>> +	}
>> +
>> +	/* Determine expected data in the return buffer */
>> +	expected = be32_to_cpup((__be32 *)(buf + 2));
>> +	if (expected > buf_len) {
>> +		dev_err(&chip->dev, "Buffer too small to receive i2c data\n");
>> +		goto out_err;
>> +	}
>> +
>> +	/* Now read the rest of the data */
>> +	cur = burstcnt;
>> +	while (cur < expected) {
>> +		/* Read updated burst count and check status */
>> +		rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
>> +		if (rc < 0)
>> +			goto out_err;
>> +
>> +		len = min_t(size_t, burstcnt, expected - cur);
>> +		rc = cr50_i2c_read(chip, addr, buf + cur, len);
>> +		if (rc < 0) {
>> +			dev_err(&chip->dev, "Read failed\n");
>> +			goto out_err;
>> +		}
>> +
>> +		cur += len;
>> +	}
>> +
>> +	/* Ensure TPM is done reading data */
>> +	rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
>> +	if (rc < 0)
>> +		goto out_err;
>> +	if (status & TPM_STS_DATA_AVAIL) {
>> +		dev_err(&chip->dev, "Data still available\n");
>> +		rc = -EIO;
>> +		goto out_err;
>> +	}
>> +
>> +	cr50_release_locality(chip, CR50_NO_FORCE);
>> +	return cur;
>> +
>> +out_err:
>> +	/* Abort current transaction if still pending */
>> +	if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
>> +		cr50_i2c_tis_ready(chip);
>> +
>> +	cr50_release_locality(chip, CR50_NO_FORCE);
>> +	return rc;
>> +}
>> +
>> +/*
>> + * cr50_i2c_tis_send - TPM emission callback
>> + *
>> + * @chip: TPM chip information
>> + * @buf: buffer to send
>> + * @len: buffer length
>> + *
>> + * Returns negative number for error, 0 for success.
>> + */
>> +static int cr50_i2c_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>> +{
>> +	struct priv_data *priv = dev_get_drvdata(&chip->dev);
>> +	int rc, status;
>> +	size_t burstcnt, limit, sent = 0;
>> +	u8 tpm_go[4] = { TPM_STS_GO };
>> +	unsigned long stop;
>> +
>> +	rc = request_locality(chip);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	/* Wait until TPM is ready for a command */
>> +	stop = jiffies + chip->timeout_b;
>> +	while (!(cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) {
>> +		if (time_after(jiffies, stop)) {
>> +			rc = -ETIMEDOUT;
>> +			goto out_err;
>> +		}
>> +
>> +		cr50_i2c_tis_ready(chip);
>> +	}
>> +
>> +	while (len > 0) {
>> +		u8 mask = TPM_STS_VALID;
>> +
>> +		/* Wait for data if this is not the first chunk */
>> +		if (sent > 0)
>> +			mask |= TPM_STS_DATA_EXPECT;
>> +
>> +		/* Read burst count and check status */
>> +		rc = cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status);
>> +		if (rc < 0)
>> +			goto out_err;
>> +
>> +		/*
>> +		 * Use burstcnt - 1 to account for the address byte
>> +		 * that is inserted by cr50_i2c_write()
>> +		 */
>> +		limit = min_t(size_t, burstcnt - 1, len);
>> +		rc = cr50_i2c_write(chip, TPM_I2C_DATA_FIFO(priv->locality),
>> +				    &buf[sent], limit);
>> +		if (rc < 0) {
>> +			dev_err(&chip->dev, "Write failed\n");
>> +			goto out_err;
>> +		}
>> +
>> +		sent += limit;
>> +		len -= limit;
>> +	}
>> +
>> +	/* Ensure TPM is not expecting more data */
>> +	rc = cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status);
>> +	if (rc < 0)
>> +		goto out_err;
>> +	if (status & TPM_STS_DATA_EXPECT) {
>> +		dev_err(&chip->dev, "Data still expected\n");
>> +		rc = -EIO;
>> +		goto out_err;
>> +	}
>> +
>> +	/* Start the TPM command */
>> +	rc = cr50_i2c_write(chip, TPM_I2C_STS(priv->locality), tpm_go,
>> +			    sizeof(tpm_go));
>> +	if (rc < 0) {
>> +		dev_err(&chip->dev, "Start command failed\n");
>> +		goto out_err;
>> +	}
>> +	return 0;
>> +
>> +out_err:
>> +	/* Abort current transaction if still pending */
>> +	if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)
>> +		cr50_i2c_tis_ready(chip);
>> +
>> +	cr50_release_locality(chip, CR50_NO_FORCE);
>> +	return rc;
>> +}
>> +
>> +/*
>> + * cr50_i2c_req_canceled - callback to notify a request cancel
>> + *
>> + * @chip: TPM chip information
>> + * @status: status given by the cancel callback
>> + *
>> + * Return if command is ready or not
>> + */
>> +static bool cr50_i2c_req_canceled(struct tpm_chip *chip, u8 status)
>> +{
>> +	return (status == TPM_STS_COMMAND_READY);
>> +}
>> +
>> +static const struct tpm_class_ops cr50_i2c = {
>> +	.flags = TPM_OPS_AUTO_STARTUP,
>> +	.status = &cr50_i2c_tis_status,
>> +	.recv = &cr50_i2c_tis_recv,
>> +	.send = &cr50_i2c_tis_send,
>> +	.cancel = &cr50_i2c_tis_ready,
>> +	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> +	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
>> +	.req_canceled = &cr50_i2c_req_canceled,
>> +};
>> +
>> +static const struct i2c_device_id cr50_i2c_table[] = {
>> +	{"cr50_i2c", 0},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, cr50_i2c_table);
>> +
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id cr50_i2c_acpi_id[] = {
>> +	{ "GOOG0005", 0 },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, cr50_i2c_acpi_id);
>> +#endif
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_cr50_i2c_match[] = {
>> +	{ .compatible = "google,cr50", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, of_cr50_i2c_match);
>> +#endif
>> +
>> +/*
>> + * cr50_i2c_probe - driver prbe function
>
> s/prbe/probe
>
>> + *
>> + * @client: i2x client information
>> + * @id: i2c device id
>> + *
>> + * Returns negative number for error, 0 for success.
>> + */
>> +static int cr50_i2c_probe(struct i2c_client *client,
>> +			  const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct tpm_chip *chip;
>> +	struct priv_data *priv;
>> +	u8 buf[4];
>> +	u32 vendor;
>> +	int rc;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>> +		return -ENODEV;
>> +
>> +	chip = tpmm_chip_alloc(dev, &cr50_i2c);
>> +	if (IS_ERR(chip))
>> +		return PTR_ERR(chip);
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	/* cr50 is a TPM 2.0 chip */
>> +	chip->flags |= TPM_CHIP_FLAG_TPM2;
>> +	chip->flags |= TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED;
>> +
>> +	/* Default timeouts */
>> +	chip->timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
>> +	chip->timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
>> +	chip->timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
>> +	chip->timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
>> +
>> +	dev_set_drvdata(&chip->dev, priv);
>> +	init_completion(&priv->tpm_ready);
>> +
>> +	if (client->irq > 0) {
>> +		rc = devm_request_irq(dev, client->irq, cr50_i2c_int_handler,
>> +				      IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +				      dev->driver->name, chip);
>> +		if (rc < 0) {
>> +			dev_err(dev, "Failed to probe IRQ %d\n", client->irq);
>> +			return rc;
>> +		}
>> +
>> +		disable_irq(client->irq);
>> +		priv->irq = client->irq;
>> +	} else {
>> +		dev_warn(dev, "No IRQ, will use %ums delay for TPM ready\n",
>> +			 CR50_TIMEOUT_NOIRQ_MS);
>> +	}
>> +
>> +	rc = request_locality(chip);
>> +	if (rc < 0) {
>> +		dev_err(dev, "Could not request locality\n");
>> +		return rc;
>> +	}
>> +
>> +	/* Read four bytes from DID_VID register */
>> +	rc = cr50_i2c_read(chip, TPM_I2C_DID_VID(0), buf, sizeof(buf));
>> +	if (rc < 0) {
>> +		dev_err(dev, "Could not read vendor id\n");
>> +		cr50_release_locality(chip, CR50_FORCE);
>> +		return rc;
>> +	}
>> +
>> +	vendor = le32_to_cpup((__le32 *)buf);
>> +	if (vendor != CR50_I2C_DID_VID) {
>> +		dev_err(dev, "Vendor ID did not match! ID was %08x\n", vendor);
>> +		cr50_release_locality(chip, CR50_FORCE);
>> +		return -ENODEV;
>> +	}
>> +
>> +	dev_info(dev, "cr50 TPM 2.0 (i2c 0x%02x irq %d id 0x%x)\n",
>> +		 client->addr, client->irq, vendor >> 16);
>> +
>> +	return tpm_chip_register(chip);
>> +}
>> +
>> +/*
>> + * cr50_i2c_probe - driver prbe function
>
> s/probe/remove
> s/prbr/remove
>
>
> Thanks,
> Helen
>
>> + *
>> + * @client: i2x client information
>> + *
>> + * Returns 0
>> + */
>> +static int cr50_i2c_remove(struct i2c_client *client)
>> +{
>> +	struct tpm_chip *chip = i2c_get_clientdata(client);
>> +
>> +	tpm_chip_unregister(chip);
>> +	cr50_release_locality(chip, CR50_FORCE);
>> +
>> +	return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(cr50_i2c_pm, tpm_pm_suspend, tpm_pm_resume);
>> +
>> +static struct i2c_driver cr50_i2c_driver = {
>> +	.id_table = cr50_i2c_table,
>> +	.probe = cr50_i2c_probe,
>> +	.remove = cr50_i2c_remove,
>> +	.driver = {
>> +		.name = "cr50_i2c",
>> +		.pm = &cr50_i2c_pm,
>> +		.acpi_match_table = ACPI_PTR(cr50_i2c_acpi_id),
>> +		.of_match_table = of_match_ptr(of_cr50_i2c_match),
>> +	},
>> +};
>> +
>> +module_i2c_driver(cr50_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("cr50 TPM I2C Driver");
>> +MODULE_LICENSE("GPL");
>>