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

[Vadim Bendebury]

On Sat, Oct 15, 2011 at 12:25 PM, Wolfgang Denk <wd at denx.de> wrote:
> 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.
>

done

> ...
>> +++ 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.
>

done

> ...
>> +/* 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.
>

did not see any difference in the code layout as reported by objdump
of the module

> ...
>> +     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.
>

done

>
>> +             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.
>

done

>
> 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."
>

chrrs,
/vb