[U-Boot] [PATCH 1/1] TPM: STMicroelectronics u-boot driver I2C

[Tom Rini]

On Mon, Apr 22, 2013 at 05:55:50PM +0200, Mathias leblanc wrote:

> From: Mathias Leblanc <mathias.leblanc at st.com>
> 
>  * STMicroelectronics version 1.2.0, Copyright (C) 2013
>  * This is free software, and you are welcome to redistribute it
>  * under certain conditions.

Just leave these lines out of the commit message.

> This is the driver for TPM chip from ST Microelectronics.
> 
> If you have a TPM security chip from STMicroelectronics working with
> an I2C, read the README file and add the correct defines regarding
> the tpm in the configuration file of your board.
> This file is located in include/configs/your_board.h
> 
> The driver will be accessible from within uboot terminal.

Please see
http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions
as this is the 3rd or 4th posting of these changes.

[snip]
> diff --git a/common/cmd_tpm.c b/common/cmd_tpm.c
> index 0970a6f..2b7bf35 100644
> --- a/common/cmd_tpm.c
> +++ b/common/cmd_tpm.c
> @@ -145,10 +145,177 @@ static int do_tpm_many(cmd_tbl_t *cmdtp, int flag,
>  	return rv;
>  }
>  
> +static int do_tpm_hash(cmd_tbl_t *cmdtp, int flag, int argc,
> +char * const argv[])

checkpatch warning there, please run tools/checkpatch.pl and fix all
errors.

> +	u8 startup[] = {
> +		0x00, 0xc1,
> +		0x00, 0x00, 0x00, 0x0c,
> +		0x00, 0x00, 0x00, 0x99,
> +		0x00, 0x01
> +	};
> +
> +	u8 selftestfull[] = {
> +		0x00, 0xc1,
> +		0x00, 0x00, 0x00, 0x0a,
> +		0x00, 0x00, 0x00, 0x50
> +	};
> +
> +	u8 readpcr17[] = {
> +		0x00, 0xc1,
> +		0x00, 0x00, 0x00, 0x0e,
> +		0x00, 0x00, 0x00, 0x15,
> +		0x00, 0x00, 0x00, 0x11
> +	};

Comment what these are and/or where they come from.

[snip]
> +	if (!
> +	tis_sendrecv(readpcr17, sizeof(readpcr17), &response, &read_size)) {

Funny place to break the line, split it on the tis_sendrecv params.  And
fix anything else like this too.


[snip]
> +	u8 startup[] = {
> +		0x00, 0xc1,
> +		0x00, 0x00, 0x00, 0x0c,
> +		0x00, 0x00, 0x00, 0x99,
> +		0x00, 0x01
> +	};
> +	u8 selftestfull[] = {
> +		0x00, 0xc1,
> +		0x00, 0x00, 0x00, 0x0a,
> +		0x00, 0x00, 0x00, 0x50
> +	};
> +	u8 getpermflags[] = {
> +		0x00, 0xc1,
> +		0x00, 0x00, 0x00, 0x16,
> +		0x00, 0x00, 0x00, 0x65,
> +		0x00, 0x00, 0x00, 0x04,
> +		0x00, 0x00, 0x00, 0x04,
> +		0x00, 0x00, 0x01, 0x08
> +	};

startup and selftestfull are the same as before?  Shouldn't duplicate
them then.

> +
> +
> +	CHECK(tis_init());
> +
> +

Too much whitespace around the line.

[snip]
> +/* extended error numbers from linux (see errno.h) */
> +#define	ECANCELED	125	/* Operation Canceled */

'#define<space>' please.

> +#define msleep(t) udelay((t)*1000)

We already have a few msleep compatibility defines.  Can you please do a
separate prepatch that adds msleep to <common.h> after the extern for
udelay?

> +
> +/* Timer frequency. Corresponds to msec timer resolution*/
> +#define HZ             1000

Please use CONFIG_SYS_HZ

> +++ b/drivers/tpm/tis_i2c.c
[snip]
> + *	[backport from https://github.com/theopolis/u-boot-sboot/
> + *		blob/master/drivers/tpm/tis_i2c.c]

You're giving the original author credit in the file already, yes?  If
not, please do, and drop these lines.

> +/* Define in board config */
> +#ifndef CONFIG_TPM_I2C_BUS
> +	#define CONFIG_TPM_I2C_BUS 0
> +	#define CONFIG_TPM_I2C_ADDR 0
> +#endif

No extra indentation.

[snip]
> +	if (i2c_probe(tpm.slave_addr) && i2c_probe(tpm.slave_addr)) {
> +		debug(
> +		"%s: fail to probe i2c addr 0x%x\n", __func__, tpm.slave_addr);

Split after __func__ instead.

> diff --git a/drivers/tpm/tpm.c b/drivers/tpm/tpm.c
[snip]

We have this as drivers/tpm/slb9635_i2c/tpm.c now, so I think we need to
be renaming the existing one, same with tpm.h

[snip]
> diff --git a/drivers/tpm/tpm_i2c_st.c b/drivers/tpm/tpm_i2c_st.c
[snip]
> +/*
> + * write8_reg
> + * Send byte to the TIS register according to the ST33ZP24 I2C protocol.
> + * @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: Returns zero in case of success else the negative error code.
> + */
> +static int write8_reg(u8 addr, u8 tpm_register,
> +		      u8 *tpm_data, u16 tpm_size)
> +{
> +	u8 data;
> +	data = tpm_register;
> +	memcpy(&(tpm_dev.buf[0]), &data, sizeof(data));
> +	memcpy(&(tpm_dev.buf[0])+1, tpm_data, tpm_size);
> +
> +	return i2c_write(addr, 0, 0, &tpm_dev.buf[0],
> +				tpm_size + 1);
> +
> +} /* write8_reg() */

Since this is kerneldoc/etc style commenting, it should be /** at the
start, yes?  And we don't do comments like that at the end of a
function.  And since it is kerneldoc style, can you do a template for
them?  See doc/DocBook/  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130422/ef503703/attachment.pgp>