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

[Wolfgang Denk <wd@denx.de>]

Dear Mathias leblanc,

In message <1365429818-2090-1-git-send-email-mathias.leblanc@st.com> you wrote:
> 
>  * STMicroelectronics version 1.2.0, Copyright (C) 2013

Who exactly is the copyright holder?

>  * STMicroelectronics comes with ABSOLUTELY NO WARRANTY.
>  * This is free software, and you are welcome to redistribute it
>  * under certain conditions.

And these would be what exactly?

Sorry, this makes no sense.  Such statements must be precise to the
finest detail , and they belong into the code, and not into the commit
message.

In the next step, please run your patch through checkpatch and fix the
reported issues.

Please make sure to get rid of all these "__attribute__ ((packed))"
stuff.  Make sure that structs are properly aligned, and use explicit
conversion where and if necessary.


> diff --git a/Makefile b/Makefile
> index 12763ce..ef954a4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -314,7 +314,7 @@ endif
>  LIBS-y += drivers/rtc/librtc.o
>  LIBS-y += drivers/serial/libserial.o
>  LIBS-y += drivers/sound/libsound.o
> -LIBS-$(CONFIG_GENERIC_LPC_TPM) += drivers/tpm/libtpm.o
> +LIBS-$(CONFIG_TPM) += drivers/tpm/libtpm.o

If you change this, you must also update the realted documentation (in
the README) and _all_ related code (I think you missed at least
include/configs/coreboot.h).

ALso please make sure to add the documentation to the proper groups,
i. e. CONFIG_CMD_TPM should be added to the CONFIG_CMD_* group.

> +	if (tis_init()) {
> +		puts("tis_init() failed!\n");
> +		return -1;
> +	}
> +
> +	if (tis_open()) {
> +		puts("tis_open() failed!\n");
> +		return -1;

Can we have some better diagnostics, for example including the rason
for the failure?

> +	rv = tis_sendrecv(Startup, sizeof(Startup), &response, &rlength);
> +	if (rv) {
> +		printf("tpm test Startup failed\n");
> +		CHECK(tis_close());
> +	}
> +
> +	rv = tis_sendrecv(SelfTestFull, sizeof(SelfTestFull), &response,

Startup, SelfTestFull, ...

We do not allow CamelCase indentifiers.

> +U_BOOT_CMD(tpm_hash, MAX_TRANSACTION_SIZE, 1, do_tpm_hash,
...
> +U_BOOT_CMD(tpm_get_flag, 1, 1, do_tpm_get_flag,

Instead of adding a number of different commands, this should be
implemented as sub-commands under a common one, i. e. make this:

	tpm_hash     --> tpm hash ...
	tpm_get_flag --> tpm get_flag ...

[eventually there is even a better name than "get_flag"?]

...
> +++ b/drivers/tpm/tis_i2c.c
> @@ -0,0 +1,211 @@
> +/*
> + * Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
> + * Use of this source code is governed by a BSD-style license that can be
> + * found in the LICENSE file.

This is not acceptable.  Please be precise here.  We need to know
exacly which license terms apply.  Please see item # 1 at
http://www.denx.de/wiki/view/U-Boot/Patches#Notes  for details.


> diff --git a/drivers/tpm/tpm.c b/drivers/tpm/tpm.c
> new file mode 100644
> index 0000000..3f4f66e
> --- /dev/null
> +++ b/drivers/tpm/tpm.c
> @@ -0,0 +1,472 @@
> +/*
> + * Copyright (C) 2011 Infineon Technologies
> + *
> + * Authors:
> + * Peter Huewe <huewe.external@infineon.com>
> + *
> + * Description:
> + * Device driver for TCG/TCPA TPM (trusted platform module).
> + * Specifications at www.trustedcomputinggroup.org
> + *
> + * It is based on the Linux kernel driver tpm.c from Leendert van
> + * Dorn, Dave Safford, Reiner Sailer, and Kyleen Hall.

Do you have a more precise reference where the code is coming from?
What we would like to see is described in bullet # 4 at
http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's hard to think of you as the end result of millions of  years  of
evolution.