Messages in this thread | | | Date | Sat, 18 Dec 2021 10:40:33 +0100 | From | Wolfram Sang <> | Subject | Re: [PATCH v5 1/1] gpio: add sloppy logic analyzer using polling |
| |
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
[unhandled content-type:application/pgp-signature] | |