Re: [RFC PATCH 2/2] tpm: Add tpm_tis_i2c backend for tpm_tis_core

[Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>]

On Thu, Jul 18, 2019 at 07:03:55PM +0200, Alexander Steffen wrote:
> +static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
> +				   u16 len, const u8 *value)
> +{
> +	struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
> +	int ret;
> +
> +	u8 locality[] = {
> +		0, // TPM_LOC_SEL
> +		addr >> 12, // locality
> +	};
> +
> +	if (phy->iobuf) {
> +		if (len > TPM_BUFSIZE - 1)
> +			return -EIO;
> +
> +		phy->iobuf[0] = address_to_register(addr);
> +		memcpy(phy->iobuf + 1, value, len);
> +
> +		{
> +			struct i2c_msg msgs[] = {
> +				{
> +					.addr = phy->i2c_client->addr,
> +					.len = sizeof(locality),
> +					.buf = locality,
> +				},
> +				{
> +					.addr = phy->i2c_client->addr,
> +					.len = len + 1,
> +					.buf = phy->iobuf,
> +				},
> +			};
> +
> +			ret = i2c_transfer(phy->i2c_client->adapter, msgs,
> +					   ARRAY_SIZE(msgs));
> +		}
> +	} else {
> +		u8 reg = address_to_register(addr);
> +
> +		struct i2c_msg msgs[] = {
> +			{
> +				.addr = phy->i2c_client->addr,
> +				.len = sizeof(locality),
> +				.buf = locality,
> +			},
> +			{
> +				.addr = phy->i2c_client->addr,
> +				.len = sizeof(reg),
> +				.buf = &reg,
> +			},
> +			{
> +				.addr = phy->i2c_client->addr,
> +				.len = len,
> +				.buf = (u8*)value,
> +				.flags = I2C_M_NOSTART,
> +			},
> +		};
> +
> +		ret = i2c_transfer(phy->i2c_client->adapter, msgs,
> +				   ARRAY_SIZE(msgs));
> +	}
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(250, 300); // wait default GUARD_TIME of 250µs

This does not look good. Would prefer to have named constants.

> +
> +	return 0;
> +}

You could probably simplify this by using branching for constructing
the message arrays and then use the same code path for transfer.

/Jarkko