Hi Andy, > > +Result is a .sr file to be consumed with PulseView or sigrok-cli from the free > > +`sigrok`_ project. It is a zip file which also contains the binary sample data > > +which may be consumed by other software. The filename is the logic analyzer > > +instance name plus a since-epoch timestamp. > > + > > +.. _sigrok: https://sigrok.org/ > > Alas, yet another tool required... (Sad thoughts since recently has installed > PicoScope software). ? For sure, another tool is required. Do you want the analyzer itself to output pretty SVG files? :) > > > kgdb > > kselftest > > kunit/index > > > + gpio-sloppy-logic-analyzer > > Above looks like ordered, do we need some groups here or so? No feedback from the doc-maintainers so far. Can easily be fixed afterwards if needed. > > + mutex_lock(&priv->lock); > > > + if (priv->blob_dent) { > > Redundant (i.e. duplicate). Nope, it can be NULL if allocating memory all goes wrong. > > +gpio_err: > > A bit confusing name. What about > > enable_irq_and_free_data: Yes, fixed in v6. > > + char *meta = NULL; > > + unsigned int i, meta_len = 0; > > + int ret; > > Perhaps > > unsigned int i, meta_len = 0; > char *meta = NULL; > int ret; I'd like to keep the pointers grouped together. > > + if (ret >= 0 && ret != priv->descs->ndescs) > > > + ret = -ENODATA; > > Don't remember if we already discussed this error code, but data is there, > it's not correct. EBADSLT? EBADR? ECHRNG? In your V1 review, you suggested -ENODATA. I will pick yet another one, but it really matters zero in practice. > > + meta_len += snprintf(meta + meta_len, add_len, "probe%02u=%s\n", > > + i + 1, gpio_names[i]); > > Do we really need the 'probe%02u=' part? It's redundant since it may be derived > from the line number of the output (and it always in [1..ndescs+1]). It makes creating the .sr-file a lot easier. If you feel strong about it, then you can later remove it and also update the script, I'd say. > > + dev_info(dev, "initialized"); > > Is it useful? For the third time, yes! > > + cat <<EOF > > cat << EOF > > is slightly easier to read. I'll fix it. > > + [ -d $cpusetdir ] || mkdir $cpusetdir > > `mkdir -p` and drop needless test. It is not the same. I prefer to bail out if e.g. '/dev/' does not exist rather than silently create it. > > + val=$((0x$oldmask & ~(1 << isol_cpu))) > > + newmask=$(printf "%x" $val) > > Can be on one line (in a single expression). Ok. > `> /dev/null 2>&1` is idiomatic. And I think there is actually a subtle > difference between two. What is the difference? Does it matter here? > > + [ "$chan" != "$elem" ] && [ "$chan" -le $max_chans ] || fail "Trigger syntax error: $elem" > > No need to execute `test` twice: > > [ "$chan" != "$elem" -a "$chan" -le $max_chans ] || fail "Trigger syntax error: $elem" I read that '-a' and '-o' are deprecated. Dunno where but looking again I found this: https://stackoverflow.com/questions/20449680/boolean-operators-a-o-in-bash > > > + bit=$((1 << (chan - 1))) > > + mask=$((mask | bit)) > > + case $mode in > > + [hH]) val1=$((val1 | bit)); val2=$((val2 | bit));; > > + [fF]) val1=$((val1 | bit));; > > + [rR]) val2=$((val2 | bit));; > > + esac > > + done > > > + trigger_bindat="$trigger_bindat$(printf '\\%o\\%o' $mask $val1)" > > + [ $val1 -ne $val2 ] && trigger_bindat="$trigger_bindat$(printf '\\%o\\%o' $mask $val2)" > > `printf` with arguments may be split to a separate helper function. I think this is a micro-optimization, but feel free to change it later. > > + taskset "$1" echo 1 > "$lasysfsdir"/capture || fail "Capture error! Check kernel log" > > Shouldn't this function setup signal TRAPs? To do what? > $@ is better, actually one should never use $*. What difference does it make when expanding into a string? > Wondering, shouldn't be a simple validator before start that we have commands > present, such as zip? This is what the variable 'neededcmds' is for... Kind regards, Wolfram
Attachment:
signature.asc
Description: PGP signature
