[PATCH v2 06/10] rtc: add rtc command
Rasmus Villemoes
rasmus.villemoes at prevas.dk
Tue Jun 2 11:13:18 CEST 2020
On 31/05/2020 16.07, Simon Glass wrote:
> Hi Rasmus,
>
> On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
> <rasmus.villemoes at prevas.dk> wrote:
>>
>> +static int do_rtc_read(struct udevice *dev, int argc, char * const argv[])
>> +{
>> + u8 buf[MAX_RTC_BYTES];
>> + int reg, len, ret;
>> + u8 *addr;
>> +
>> + if (argc < 2 || argc > 3)
>> + return CMD_RET_USAGE;
>> +
>> + reg = simple_strtoul(argv[0], NULL, 0);
>
> I think these should be hex (i.e. 16), since that is the norm in U-Boot.
OK.
>> + len = simple_strtoul(argv[1], NULL, 0);
>> + if (argc == 3) {
>> + addr = map_sysmem(simple_strtoul(argv[2], NULL, 16), len);
>> + } else {
>> + if (len > sizeof(buf)) {
>> + printf("can read at most %d registers at a time\n", MAX_RTC_BYTES);
>
> It would be better to loop like print_buffer() does.
Both read and write have been rewritten to avoid that arbitrary limit.
>> +
>> + if (argc == 2) {
>> + while (len--)
>> + printf("%d: 0x%02x\n", reg++, *addr++);
>
> Perhaps use print_buffer()?
Done.
>> + const char *s = argv[1];
>> +
>> + /* Convert hexstring, bail out if too long. */
>> + addr = buf;
>> + len = strlen(s);
>> + if (len > 2*MAX_RTC_BYTES) {
>
> Spaces around *
>
>> + printf("hex string too long, can write at most %d bytes\n", MAX_RTC_BYTES);
>
> Please can you try checkpatch or patman? This lines seems too long
The rewrite to avoid the 32 byte limit made me handle the "argc==3" case
separately (it wasn't worth the complexity trying to stick to just one
rtc_{read,write} call, which also automatically dealt with this one.
>> +
>> +int do_rtc(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> + static int curr_rtc = 0;
>> + struct udevice *dev;
>> + int ret, idx;
>> +
>> + if (argc < 2)
>> + return CMD_RET_USAGE;
>> +
>> + argc--;
>> + argv++;
>> +
>> + if (!strcmp(argv[0], "list")) {
>
> It is comment in U-Boot to just check the letters that are needed. So
> here you could do (*argv[0] == 'l')
Yes, and I consider that an anti-pattern. It makes it impossible to
later introduce another (sub)command which starts with a
previously-unique prefix. Now, if that "just type a unique prefix"
wasn't official, so scripts were always supposed to use the full names,
it wouldn't be that big a problem (scripts written for later versions of
U-Boot, or U-Boots configured with more (sub)commands, could still fail
silently if used on an earlier U-Boot or one with fewer (sub)commands
instead of producing a "usage" error message), but
https://www.denx.de/wiki/view/DULG/UBootCommandLineInterface explicitly
mentions that as a feature (and says h can be used for help, which it
can't when the hash command is built in, perfectly exemplifying what I'm
talking about).
Thanks,
Rasmus
More information about the U-Boot
mailing list