[U-Boot] [PATCH] cmd_time: add time command

[Che-liang Chiou]

Hi Mike,

Thanks for comments. Reply inlined.

On Thu, Oct 6, 2011 at 1:24 AM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Wednesday 05 October 2011 03:09:15 Che-Liang Chiou wrote:
>> The 'time' command runs and reports execution time of commands.
>
> cool
>
>> Sameple usage:
>
> typo: Sample
Done.
>
>> --- /dev/null
>> +++ b/common/cmd_time.c
>>
>> +static void report_time(unsigned long int cycles)
>> +{
>> +#ifdef CONFIG_SYS_HZ
>
> CONFIG_SYS_HZ is required to be defined, so please drop this ifdef check
>
Done.
>> +     unsigned long int minutes, seconds, milliseconds;
>> +     unsigned long int total_seconds, remainder;
>> +
>> +     total_seconds = cycles / CONFIG_SYS_HZ;
>> +     remainder = cycles % CONFIG_SYS_HZ;
>> +     minutes = total_seconds / 60;
>> +     seconds = total_seconds % 60;
>> +     /* approximate millisecond value */
>> +     milliseconds = (remainder * 1000 + CONFIG_SYS_HZ / 2) / CONFIG_SYS_HZ;
>> +
>> +     printf("time:");
>> +     if (minutes)
>> +             printf(" %lu minutes,", minutes);
>> +     printf(" %lu.%03lu seconds, %lu ticks\n",
>> +                     seconds, milliseconds, cycles);
>
> looks like this could use the new timer api that Graeme is throwing around
>
Exactly. Add TODO comments.
>> +int do_time(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>
> static
>
Done.
>> +     cmd_tbl_t *target_cmdtp = NULL;
>
> const
>
I guess I cannot change it to a const pointer. cmdtp->cmd() signature
requires a non-const pointer to cmdtp.
>> +     if (argc == 1) {
>> +             printf("no command provided\n");
>> +             return 1;
>> +     }
>
> return cmd_usage(cmdtp);
>
Done.
>> +     if (!target_cmdtp) {
>> +             printf("command not found: %s\n", argv[1]);
>> +             return 1;
>> +     }
>
> shells typically display:
>        %s: command not found
>
Done.
>> +     if (target_argc > target_cmdtp->maxargs) {
>> +             printf("maxarags exceeded: %d > %d\n", target_argc,
>> +                             target_cmdtp->maxargs);
>> +             return 1;
>> +     }
>> ...
>> +     retval = target_cmdtp->cmd(target_cmdtp, 0, target_argc, argv + 1);
>
> hmm, this looks like we should add a helper like run_command() and put this
> logic there ...
>
I agree. I moved these codes to a separate function. It can be merged
with run_command() if necessary after the timer API is landed.
>> +     putc('\n');
>> +     report_time(cycles);
>
> why not just integrate that \n into the report_time() code ?
Done.
> -mike
>

Regards,
Che-Liang