[U-Boot] [PATCH v2 1/2] Introduce generic TPM support in u-boot

[Wolfgang Denk]

Dear Vadim Bendebury,

In message <20111015033850.74AD5419FF at eskimo.mtv.corp.google.com> you wrote:
> TPM (Trusted Platform Module) is an integrated circuit and
> software platform that provides computer manufacturers with the
> core components of a subsystem used to assure authenticity,
> integrity and confidentiality.
...
> --- /dev/null
> +++ b/drivers/tpm/Makefile
> @@ -0,0 +1,27 @@
> +# Copyright (c) 2011 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.

Please fix this.   This has been pointed out before.  Please work a
bit more careful.  Thanks.

...
> +++ b/drivers/tpm/generic_lpc_tpm.c
> @@ -0,0 +1,488 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors.
> + * Released under the 2-clause BSD license.
...
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.

If we can have it under GPLv2+, then we don't need the BSD part.
Please drop these lines.

...
> +/* TPM access going through macros to make tracing easier. */
> +#define tpm_read(ptr) ({ \
> +	u32  __ret; \
> +	__ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
> +	 debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
> +	       (u32)ptr - (u32)lpc_tpm_dev, __ret); \
> +					 __ret; })

Please test with something like this instead:

	debug("%sRead reg 0x%x returns 0x%x\n", PREFIX, ...

It might result in smaller code.  Please verify.  If so, please fix
globally.

...
> +	vid = didvid & 0xffff;
> +	did = (didvid >> 16) & 0xffff;
> +	for (i = 0; i < ARRAY_SIZE(vendor_names); i++) {
> +		int j = 0;
> +		u16 known_did;
> +		if (vid == vendor_names[i].vendor_id)
> +			vendor_name = vendor_names[i].vendor_name;

Please separate declarations and code by a blank line.


> +		burst = BURST_COUNT(value);
> +		if ((offset == (len - 1)) && burst)
> +			/*
> +			 * We need to be able to send the last byte to the
> +			 * device, so burst size must be nonzero before we
> +			 * break out.
> +			 */
> +			break;

We require braces around multiline statements.


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
"Deliver yesterday, code today, think tomorrow."