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

[Vadim Bendebury]

On Sat, Oct 15, 2011 at 12:42 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:
>> --- /dev/null
>> +++ b/drivers/tpm/generic_lpc_tpm.c
>>
>> +#define TPM_TIMEOUT_ERR                      (~0)
>> +#define TPM_DRIVER_ERR               (-1)
>
> these are the same thing.  another reason why you shouldn't mix ~ with normal
> values.  use -2 or something.
>

ok

>> +/* 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; })
>
> that last "__ret;" is indented way too far.  it should be on the same level as
> "u32 __ret;" and such.
>

ok

>> +#define tpm_write(value, ptr) ({                \
>> +     u32 __v = value;        \
>> +     debug(PREFIX "Write reg 0x%x with 0x%x\n", \
>> +            (u32)ptr - (u32)lpc_tpm_dev, __v); \
>> +     if (sizeof(*ptr) == 1) \
>> +             writeb(__v, ptr); \
>> +     else \
>> +             writel(__v, ptr); })
>
> ({...}) doesn't make sense here.  this should be a do{...}while(0).
>

ok

>> +             printf("%s:%d - failed to get 'command_ready' status\n",
>> +                    __FILE__, __LINE__);
>
> replace __FILE__/__LINE__ with __func__ here and everywhere else in the file
> -mike
>
Mike, you seem the only one concerned with this. As I described in our
previous exchange, I believe specifying file and line number is
better. So, I would like to hear a second opinion on this.

cheers,
/vb