_florent_ changed the topic of #litex to: LiteX FPGA SoC builder and Cores / Github : https://github.com/enjoy-digital, https://github.com/litex-hub / Logs: https://freenode.irclog.whitequark.org/litex
tpb has quit [Remote host closed the connection]
tpb has joined #litex
lf has quit [Ping timeout: 260 seconds]
lf_ has joined #litex
<somlo> shorne: thanks, all taken care of; and no, I hadn't seen that particular post before :)
_whitelogger has joined #litex
futarisIRCcloud has joined #litex
_whitelogger has joined #litex
_whitelogger has joined #litex
Degi has quit [Ping timeout: 264 seconds]
Degi has joined #litex
Bertl_oO is now known as Bertl_zZ
lambda has quit [Ping timeout: 258 seconds]
lambda has joined #litex
futarisIRCcloud has quit [Quit: Connection closed for inactivity]
lambda has quit [Ping timeout: 272 seconds]
lambda has joined #litex
_whitelogger has joined #litex
leons has quit [Ping timeout: 260 seconds]
leons has joined #litex
lkcl has quit [Ping timeout: 264 seconds]
lkcl has joined #litex
futarisIRCcloud has joined #litex
McJ19 has joined #litex
McJ19 has quit [Ping timeout: 245 seconds]
noziar has joined #litex
noziar has quit [Remote host closed the connection]
noziar has joined #litex
noziar is now known as noziar2
noziar2 is now known as noziar
noziar has quit [Remote host closed the connection]
noziar has joined #litex
noziar is now known as noziar2
noziar2 is now known as noziar
martinraison has joined #litex
noziar has quit [Remote host closed the connection]
martinraison has quit [Remote host closed the connection]
martinraison has joined #litex
martinraison has quit [Remote host closed the connection]
Bertl_zZ is now known as Bertl
peeps[zen] has joined #litex
peepsalot has quit [Ping timeout: 264 seconds]
martinraison has joined #litex
martinraison has quit [Remote host closed the connection]
Dolu has joined #litex
peeps[zen] is now known as peepsalot
<shorne> somlo: great, I for me the changes look good, I will wait to see if any other reviewers pick anything up
<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...)
<shorne> Also, some minor points, usually on each commit we add "Changes since vN:", in the dead space after "---" explained here https://kernelnewbies.org/PatchTipsAndTricks
<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
<shorne> right
<somlo> right now they're not used by any LiteX driver I know of (see https://github.com/torvalds/linux/compare/master...litex-hub:litex-rebase) -- everyone just uses litex_[read|write][8|16|32|64]()
<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...
<daveshah> BUG_ON might be even neater?
<daveshah> Unless you really need the pr_info