On 31/05/2020 16.07, Simon Glass wrote:
Hi Rasmus,
On Tue, 19 May 2020 at 16:01, Rasmus Villemoes rasmus.villemoes@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