<somlo>
it's a small extra tightening of litex_set_reg() compared to what's in v4 right now
<somlo>
fewer variables, same effect
<somlo>
also, I've been thinking: I'm a bit uncomfortable exposing litex_[get|set]_reg() publicly as-is; would like to rename them to _litex_[get|set]_reg(), and use them internally from within litex_[read|write][8|16|32|64](); For "externally visible" purposes, I'd like the litex_[get|set]_reg() wrappers to check reg_size and throw some sort of error if it's outside the [1,8] interval (inclusive)
<somlo>
this latter thing would be a separate patch, of course
<somlo>
just looking for good examples on how to throw an error (which would have to be run-time, as call sites could pass in any valid "size_t" argument for reg_size...)
<tpb>
Title: PatchTipsAndTricks - Linux Kernel Newbies (at kernelnewbies.org)
<shorne>
oh sorry, I didn't mean to post that
<shorne>
I am sending that on a mail now
<somlo>
which is also why I don't want to do the error check as part of litex_[read|write][8|16|32|64]() (where the reg_size is a compile-time constant, and guaranteed to be in the right interval) :)
<somlo>
ah, I've been doing that in the cover letter, but could certainly do it inside each commit as well
<shorne>
somlo: Its not a big thing for me, or since this series is not huge, but for others it may be hard to understand what is changing
<somlo>
makes sense
<shorne>
Maybe move litex_get|set_reg back into the .c file so its not exposed? I guess it could still be inline
<somlo>
They used to be explicitly exported via "EXPORT_SYMBOL_GPL()" for generic/arbitrary width registers
<somlo>
so we can explicitly "unpublish" litex_[get|set]_reg() by prefixing them with a _ (iirc that's the convention for "don't call this directly as there's no promise it'll stay the same)
<somlo>
so that litex_[read|write][8|16|32|64]() can still use them
<shorne>
yeah, it makes sense
<somlo>
and for "public consumption" we either add wrappers with error checking (and those can go into the soc driver file or stay in the header, I don't care much), or we can *not* do that until it becomes necessary :)
<shorne>
I think usually the kernel uses 2 _, so it would be __litex_[get|set]_reg()
<shorne>
have a look around how they do it
<somlo>
actually I'm lying, some of the un-"curated" drivers that still only target 8-bit CSR data width do in fact use them directly, so adding wrappers is back on my todo list :)
<somlo>
right, the more underscores prefix a thing, the more "mere mortals" are discouraged from using it :)
<somlo>
I'll figure out the right amount of underscores for v5 as well
<somlo>
soon as I figure out how to deal with a failed error check on "reg_size" :)
<somlo>
I did check-patch and get_maintainer for v1; I'll run another checkpatch before v5
<somlo>
I've been doing `git send-email -3 -vX --cover-letter --annotate` which is why adding change logs to individual patches wasn't on my radar... guess I'll get back to doing separate git-format-patch commands and tweaking the ascii patch files before sending the emails :)
<somlo>
shorne: I guess the canonical way to do error checking would be something like "if (reg_size < 1 || reg_size > 8) { pr_info(...); BUG(); }"
lkcl has quit [Ping timeout: 240 seconds]
<somlo>
now trying to figure out the tradeoff between BUG() and WARN(), former feels more appropriate, since we're waaay off the reservation if the condition fails...