| Message ID | 20200706200120.23093-1-rasmus.villemoes@prevas.dk |
|---|---|
| Headers | show |
| Series | new rtc methods, rtc command, and tests | expand |
Hello Rasmus, Am 06.07.2020 um 22:01 schrieb Rasmus Villemoes: > I need access to registers other than just the timekeeping ones of the > pcf2127, so I wanted to implement ->read8 and ->write8. But for > testing these it appeared there was no convenient way to invoke those > from the shell, so I also ended up adding such a command. > > Also, it seemed more natural to provide array variants that can read > or write several registers at once, so rtc_ops is expanded a bit. > > Changes in v4: > > - Add CONFIG_CMD_RTC to sandbox defconfigs (new patch 10/11). Not > quite sure exactly which it needed to be added to, but at least > sandbox and sandbox_flattree showed CI failures. > > - Add Heiko's R-B to the 10 v3 patches (1-9 + 11), and Simon's R-B to 6/11. > > - Fix some checkpatch warnings - I don't really agree with most of Sorry, I should have mentioned which warnings you should fix ... > them - e.g. having to add an empty line in > > int foo = something(); > if (foo < 0) > return foo; > return something_else(foo); > > doesn't make the code more readable IMO. You find this rule all over the source code in U-Boot... > The remaining checkpatch blurps are things I really don't think > warrant "fixing", e.g. "WARNING: ENOSYS means 'invalid syscall nr' > and nothing else" seems irrelevant in context of U-Boot, and in any > case I've only copied existing practice. For "WARNING: please write > a paragraph that describes the config symbol fully", that seems to > be a false positive, there's certainly a full help text for CMD_RTC. Yes, this is fine. Just applied your patches now, there is one warning in patch "test: dm: rtc: add test of dm_rtc_read, dm_rtc_write" CHECK: Comparison to NULL could be written "emul" #218: FILE: test/dm/rtc.c:162: + ut_assert(emul != NULL); I think, this should be fixed! But looking into the source file, there are more such lines, so I let this at it is... may this should be cleaned! Thanks for your work! bye, Heiko
I need access to registers other than just the timekeeping ones of the pcf2127, so I wanted to implement ->read8 and ->write8. But for testing these it appeared there was no convenient way to invoke those from the shell, so I also ended up adding such a command. Also, it seemed more natural to provide array variants that can read or write several registers at once, so rtc_ops is expanded a bit. Changes in v4: - Add CONFIG_CMD_RTC to sandbox defconfigs (new patch 10/11). Not quite sure exactly which it needed to be added to, but at least sandbox and sandbox_flattree showed CI failures. - Add Heiko's R-B to the 10 v3 patches (1-9 + 11), and Simon's R-B to 6/11. - Fix some checkpatch warnings - I don't really agree with most of them - e.g. having to add an empty line in int foo = something(); if (foo < 0) return foo; return something_else(foo); doesn't make the code more readable IMO. The remaining checkpatch blurps are things I really don't think warrant "fixing", e.g. "WARNING: ENOSYS means 'invalid syscall nr' and nothing else" seems irrelevant in context of U-Boot, and in any case I've only copied existing practice. For "WARNING: please write a paragraph that describes the config symbol fully", that seems to be a false positive, there's certainly a full help text for CMD_RTC. Changes in v3: - Call the functions dm_rtc_read/dm_rtc_write rather than just rtc_read/rtc_write, since the latter names are used for local helpers by a number of drivers. That also matches the existing dm_rtc_set/dm_rtc_get (though then not the existing rtc_read{8,16,32}). - Update the rtc command (patch 6) as per Simon's feedback (parse input as hex, avoid overlong lines, use print_buffer()). - Update the tests (patches 9 and 10) according to these changes. Changes in v2: - Use simply "read" and "write" instead of "read8_array", "write8_array", both for functions and methods, as suggested by Simon. - The rtc command's interface has been simplified a bit (no separate read/readm; the number of arguments determines whether the user wants the result on the console or to a memory address) - Add tests, both of rtc_{read,write}() and of the shell command, fixing a few things I stumbled on. Rasmus Villemoes (11): rtc: add dm_rtc_read helper and ->read method rtc: add dm_rtc_write() helper rtc: fall back to ->{read,write} if ->{read,write}8 are not provided rtc: pcf2127: provide ->read method rtc: pcf2127: provide ->write method rtc: add rtc command rtc: sandbox-rtc: fix set method rtc: i2c_rtc_emul: catch any write to the "reset" register test: dm: rtc: add test of dm_rtc_read, dm_rtc_write sandbox: add rtc command to defconfigs test: dm: rtc: add tests of rtc shell command arch/sandbox/include/asm/rtc.h | 5 + cmd/Kconfig | 6 ++ cmd/Makefile | 1 + cmd/rtc.c | 167 +++++++++++++++++++++++++++++ configs/sandbox64_defconfig | 1 + configs/sandbox_defconfig | 1 + configs/sandbox_flattree_defconfig | 1 + drivers/rtc/i2c_rtc_emul.c | 3 +- drivers/rtc/pcf2127.c | 13 ++- drivers/rtc/rtc-uclass.c | 59 +++++++++- drivers/rtc/sandbox_rtc.c | 65 +++++------ include/rtc.h | 47 ++++++++ test/dm/rtc.c | 118 +++++++++++++++++++- 13 files changed, 437 insertions(+), 50 deletions(-) create mode 100644 cmd/rtc.c