From mboxrd@z Thu Jan  1 00:00:00 1970
From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Date: Tue, 2 Jun 2020 11:13:18 +0200
Subject: [PATCH v2 06/10] rtc: add rtc command
In-Reply-To: <CAPnjgZ2t7-itT7q+ZEqfXVJYkwDL9=FXOqBvcpUBWVfxkzayEQ@mail.gmail.com>
References: <20200504212032.3759-1-rasmus.villemoes@prevas.dk>
 <20200519220117.24448-1-rasmus.villemoes@prevas.dk>
 <20200519220117.24448-7-rasmus.villemoes@prevas.dk>
 <CAPnjgZ2t7-itT7q+ZEqfXVJYkwDL9=FXOqBvcpUBWVfxkzayEQ@mail.gmail.com>
Message-ID: <3d3529b8-e018-07e8-cbd1-39946d61b2f0@prevas.dk>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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