Dear Marek Vasut, thank you for your comments, please see below:
On Sat, Oct 15, 2011 at 11:02 AM, Marek Vasut <marek.va...@gmail.com> wrote: > On Saturday, October 15, 2011 05:39:08 AM Vadim Bendebury wrote: >> The command gets an arbitrary number of arguments (up to 30), which >> are interpreted as byte values and are feed into the TPM device after >> proper initialization. Then the return value and data of the TPM >> driver is examined. >> > > Dear Vadim Bendebury, > > [...] > >> diff --git a/common/cmd_tpm.c b/common/cmd_tpm.c >> new file mode 100644 >> index 0000000..e008a78 >> --- /dev/null >> +++ b/common/cmd_tpm.c >> @@ -0,0 +1,111 @@ >> +/* >> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved. >> + * Released under the 2-clause BSD license. > > Are we ok with this ? Also, you say something about GPL in the same comment? > Can someone please tell me what needs to be put in the license headers and I will do it. I hear different suggestions from different people. >> + * >> + * See file CREDITS for list of people who contributed to this >> + * project. >> + * >> + * 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. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, >> + * MA 02111-1307 USA >> + */ >> + > > [...] > >> + puts("Got TPM response:\n"); >> + for (i = 0; i < read_size; i++) >> + printf(" %2.2x", tpm_buffer[i]); >> + puts("\n"); >> + rv = 0; >> + } else { >> + puts("tpm command failed\n"); >> + } >> + return rv; >> +} > > You might want to use debug() where fitting ? > I don't expect failures and if happen prefer them to be printed always, not only if debug is enabled. >> + >> +static int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * const >> argv[]) +{ >> + int rv = 0; >> + >> + /* >> + * Verify that in case it is present, the first argument, it is >> + * exactly one character in size. >> + */ >> + if (argc < 7) { >> + puts("command should be at least six bytes in size\n"); >> + return ~0; > > Ugh, return 1 isn't ok ? Using ~0 on int type is weird. > I was under impression that any nonzero value is good. I see sometimes -1 returned for error in other u-boot sources. Also, I am sorry, I am new to this, when someone says "it is weird" - does this mean that it has to be changed? >> + } >> + >> + if (tis_init()) { >> + puts("tis_init() failed!\n"); >> + return ~0; >> + } >> + >> + if (tis_open()) { >> + puts("tis_open() failed!\n"); >> + return ~0; >> + } >> + >> + rv = tpm_process(argc - 1, argv + 1); >> + >> + if (!rv && tis_close()) { >> + puts("tis_close() failed!\n"); >> + rv = ~0; > > This doesn't make much sense, tis_close() might not be called under all > circumstances, is it ok ? > good point, I thought about this, but left untouched. It does not matter with this driver, but is better to call tis_close() no matter what. I'll fix it. >> + } >> + >> + return rv; >> +} >> + >> +U_BOOT_CMD(tpm, MAX_TRANSACTION_SIZE, 1, do_tpm, >> + "tpm <data> [<data>] - write data and read ressponse\n", >> + "send arbitrary data to the TPM and read the response" >> +); >> + >> +static void report_error(const char *msg) >> +{ >> + if (msg && *msg) > > Uhm, you also check if first character is non-zero? why ? To avoid printing an empty string if someone calls this with an empty message? > >> + printf("%s\n", msg); >> + cmd_usage(&__u_boot_cmd_tpm); > > Two underscores aren't a good practice. > I did this as a result of a previous review. Do you have a suggestion how this should be done instead? cheers, /vb >> +} > > Cheers > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot