Re: [PATCH v2 06/10] rtc: add rtc command
Hi Rasmus,
On Tue, 2 Jun 2020 at 08:36, Rasmus Villemoes
wrote:
>
> On 02/06/2020 15.22, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 2 Jun 2020 at 03:13, Rasmus Villemoes
> > wrote:
> >>
> >> On 31/05/2020 16.07, Simon Glass wrote:
> >>> Hi Rasmus,
> >>>
> >>> On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
> >>> wrote:
>
> >
> > [..]
> >
> +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).
> >
> > Hah funny. Using an abbreviation is only possible if no other command
> > starts with the same leters.
> >
> > It is certainly very risky to use abbreviations in scripts. I would
> > not recommend it. Abbreviations are for interactive use. If you have
> > auto-completion on you can use tab.
>
> Exactly, so the ability to use the abbreviated form doesn't really buy
> anything - it's risky in scripts, and interactively, it merely saves a
> tab keystroke (and that's all lost in the cognitive overhead of having
> to remember just what abbrev is enough).
Not quite:
- tab is an extra, unnecessary keystroke
- many comments don't implement auto-complete (e.g. try 'gpio i'
- auto-complete adds to code size
- fully checking the string adds to code size
>
> > But here we are talking about a sub-command, which is a bit more
> > controlled, in that it doesn't depend on what other commands the user
> > enables.
>
> True, but the same point applies; if I allowed "rtc w", one couldn't
> easily later add an "rtc wobble" subcommand (ok, my imagination is
> lacking, but you get the idea).
>
> > Anyway, it's up to you what you want to do here.
>
> In that case I'll keep checking for the full name of subcommands.
OK.
Regards,
Simon
Re: [PATCH v2 06/10] rtc: add rtc command
On 02/06/2020 15.22, Simon Glass wrote:
> Hi Rasmus,
>
> On Tue, 2 Jun 2020 at 03:13, Rasmus Villemoes
> wrote:
>>
>> On 31/05/2020 16.07, Simon Glass wrote:
>>> Hi Rasmus,
>>>
>>> On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
>>> wrote:
>
> [..]
>
+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).
>
> Hah funny. Using an abbreviation is only possible if no other command
> starts with the same leters.
>
> It is certainly very risky to use abbreviations in scripts. I would
> not recommend it. Abbreviations are for interactive use. If you have
> auto-completion on you can use tab.
Exactly, so the ability to use the abbreviated form doesn't really buy
anything - it's risky in scripts, and interactively, it merely saves a
tab keystroke (and that's all lost in the cognitive overhead of having
to remember just what abbrev is enough).
> But here we are talking about a sub-command, which is a bit more
> controlled, in that it doesn't depend on what other commands the user
> enables.
True, but the same point applies; if I allowed "rtc w", one couldn't
easily later add an "rtc wobble" subcommand (ok, my imagination is
lacking, but you get the idea).
> Anyway, it's up to you what you want to do here.
In that case I'll keep checking for the full name of subcommands.
Thanks,
Rasmus
Re: [PATCH v2 06/10] rtc: add rtc command
Hi Rasmus,
On Tue, 2 Jun 2020 at 03:13, Rasmus Villemoes
wrote:
>
> On 31/05/2020 16.07, Simon Glass wrote:
> > Hi Rasmus,
> >
> > On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
> > wrote:
> >>
[..]
> >> +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).
Hah funny. Using an abbreviation is only possible if no other command
starts with the same leters.
It is certainly very risky to use abbreviations in scripts. I would
not recommend it. Abbreviations are for interactive use. If you have
auto-completion on you can use tab.
But here we are talking about a sub-command, which is a bit more
controlled, in that it doesn't depend on what other commands the user
enables.
Anyway, it's up to you what you want to do here.
Regards,
Simon
Re: [PATCH v2 06/10] rtc: add rtc command
On 31/05/2020 16.07, Simon Glass wrote:
> Hi Rasmus,
>
> On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
> 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
Re: [PATCH v2 06/10] rtc: add rtc command
Hi Rasmus,
On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
wrote:
>
> Mostly as an aid for debugging RTC drivers, provide a command that can
> be used to read/write arbitrary registers (assuming the driver
> provides the read/write methods or their single-register-at-a-time
> variants).
>
> Signed-off-by: Rasmus Villemoes
> ---
> cmd/Kconfig | 6 ++
> cmd/Makefile | 1 +
> cmd/rtc.c| 159 +++
> 3 files changed, 166 insertions(+)
> create mode 100644 cmd/rtc.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index f9be1988f6..7eea25facd 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1715,6 +1715,12 @@ config CMD_DATE
> Enable the 'date' command for getting/setting the time/date in RTC
> devices.
>
> +config CMD_RTC
> + bool "rtc"
> + depends on DM_RTC
> + help
> + Enable the 'rtc' command for low-level access to RTC devices.
> +
> config CMD_TIME
> bool "time"
> help
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 974ad48b0a..c7992113e4 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -120,6 +120,7 @@ obj-$(CONFIG_CMD_REISER) += reiser.o
> obj-$(CONFIG_CMD_REMOTEPROC) += remoteproc.o
> obj-$(CONFIG_CMD_RNG) += rng.o
> obj-$(CONFIG_CMD_ROCKUSB) += rockusb.o
> +obj-$(CONFIG_CMD_RTC) += rtc.o
> obj-$(CONFIG_SANDBOX) += host.o
> obj-$(CONFIG_CMD_SATA) += sata.o
> obj-$(CONFIG_CMD_NVME) += nvme.o
> diff --git a/cmd/rtc.c b/cmd/rtc.c
> new file mode 100644
> index 00..e26b7ca18f
> --- /dev/null
> +++ b/cmd/rtc.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MAX_RTC_BYTES 32
> +
> +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.
> + 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.
> + return CMD_RET_FAILURE;
> + }
> + addr = buf;
> + }
> +
> + ret = rtc_read(dev, reg, addr, len);
> + if (ret) {
> + printf("rtc_read() failed: %d\n", ret);
> + ret = CMD_RET_FAILURE;
> + goto out;
> + } else {
> + ret = CMD_RET_SUCCESS;
> + }
> +
> + if (argc == 2) {
> + while (len--)
> + printf("%d: 0x%02x\n", reg++, *addr++);
Perhaps use print_buffer()?
> + }
> +out:
> + if (argc == 3)
> + unmap_sysmem(addr);
> + return ret;
> +}
> +
> +static int do_rtc_write(struct udevice *dev, int argc, char * const argv[])
> +{
> + u8 buf[MAX_RTC_BYTES];
> + u8 *addr;
> + int reg, len, ret;
> +
> + if (argc < 2 || argc > 3)
> + return CMD_RET_USAGE;
> +
> + reg = simple_strtoul(argv[0], NULL, 0);
> +
> + if (argc == 3) {
> + len = simple_strtoul(argv[1], NULL, 0);
> + addr = map_sysmem(simple_strtoul(argv[2], NULL, 16), len);
> + } else {
> + 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
> + return CMD_RET_FAILURE;
> + }
> + len /= 2;
> + if (hex2bin(addr, s, len)) {
> + printf("invalid hex string\n");
> + return CMD_RET_FAILURE;
> + }
> + }
> +
> + ret = rtc_write(dev, reg, addr, len);
> + if (ret) {
> + printf("rtc_write() failed: %d\n", ret);
> + ret = CMD_RET_FAILURE;
> + } else {
> + ret = CMD_RET_SUCCESS;
> + }
> +
> + if (argc == 3)
> + unmap_sysmem(addr);
> + return ret;
> +}
> +
> +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
[PATCH v2 06/10] rtc: add rtc command
Mostly as an aid for debugging RTC drivers, provide a command that can
be used to read/write arbitrary registers (assuming the driver
provides the read/write methods or their single-register-at-a-time
variants).
Signed-off-by: Rasmus Villemoes
---
cmd/Kconfig | 6 ++
cmd/Makefile | 1 +
cmd/rtc.c| 159 +++
3 files changed, 166 insertions(+)
create mode 100644 cmd/rtc.c
diff --git a/cmd/Kconfig b/cmd/Kconfig
index f9be1988f6..7eea25facd 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1715,6 +1715,12 @@ config CMD_DATE
Enable the 'date' command for getting/setting the time/date in RTC
devices.
+config CMD_RTC
+ bool "rtc"
+ depends on DM_RTC
+ help
+ Enable the 'rtc' command for low-level access to RTC devices.
+
config CMD_TIME
bool "time"
help
diff --git a/cmd/Makefile b/cmd/Makefile
index 974ad48b0a..c7992113e4 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -120,6 +120,7 @@ obj-$(CONFIG_CMD_REISER) += reiser.o
obj-$(CONFIG_CMD_REMOTEPROC) += remoteproc.o
obj-$(CONFIG_CMD_RNG) += rng.o
obj-$(CONFIG_CMD_ROCKUSB) += rockusb.o
+obj-$(CONFIG_CMD_RTC) += rtc.o
obj-$(CONFIG_SANDBOX) += host.o
obj-$(CONFIG_CMD_SATA) += sata.o
obj-$(CONFIG_CMD_NVME) += nvme.o
diff --git a/cmd/rtc.c b/cmd/rtc.c
new file mode 100644
index 00..e26b7ca18f
--- /dev/null
+++ b/cmd/rtc.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MAX_RTC_BYTES 32
+
+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);
+ 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);
+ return CMD_RET_FAILURE;
+ }
+ addr = buf;
+ }
+
+ ret = rtc_read(dev, reg, addr, len);
+ if (ret) {
+ printf("rtc_read() failed: %d\n", ret);
+ ret = CMD_RET_FAILURE;
+ goto out;
+ } else {
+ ret = CMD_RET_SUCCESS;
+ }
+
+ if (argc == 2) {
+ while (len--)
+ printf("%d: 0x%02x\n", reg++, *addr++);
+ }
+out:
+ if (argc == 3)
+ unmap_sysmem(addr);
+ return ret;
+}
+
+static int do_rtc_write(struct udevice *dev, int argc, char * const argv[])
+{
+ u8 buf[MAX_RTC_BYTES];
+ u8 *addr;
+ int reg, len, ret;
+
+ if (argc < 2 || argc > 3)
+ return CMD_RET_USAGE;
+
+ reg = simple_strtoul(argv[0], NULL, 0);
+
+ if (argc == 3) {
+ len = simple_strtoul(argv[1], NULL, 0);
+ addr = map_sysmem(simple_strtoul(argv[2], NULL, 16), len);
+ } else {
+ const char *s = argv[1];
+
+ /* Convert hexstring, bail out if too long. */
+ addr = buf;
+ len = strlen(s);
+ if (len > 2*MAX_RTC_BYTES) {
+ printf("hex string too long, can write at most %d
bytes\n", MAX_RTC_BYTES);
+ return CMD_RET_FAILURE;
+ }
+ len /= 2;
+ if (hex2bin(addr, s, len)) {
+ printf("invalid hex string\n");
+ return CMD_RET_FAILURE;
+ }
+ }
+
+ ret = rtc_write(dev, reg, addr, len);
+ if (ret) {
+ printf("rtc_write() failed: %d\n", ret);
+ ret = CMD_RET_FAILURE;
+ } else {
+ ret = CMD_RET_SUCCESS;
+ }
+
+ if (argc == 3)
+ unmap_sysmem(addr);
+ return ret;
+}
+
+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")) {
+ struct uclass *uc;
+ idx = 0;
+
+ uclass_id_foreach_dev(UCLASS_RTC, dev, uc) {
+ printf("RTC #%d - %s\n", idx++, dev->name);
+ }
+ if (!idx) {
+ printf("*** no RTC devices available ***\n");
+ return CMD_RET_FAILURE;
+ }
+ return CMD_RET_SUCCESS;
+ }
+
+ idx = curr_rtc;
+ if (!strcmp(argv[0], "dev") && argc >= 2)
+ idx = simple_strtoul(argv[1], NULL, 10);
+
+ ret = uclass_get_device(UCLASS_RTC, idx, );
+
