Hi Andy,

quoted

+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? :)

quoted

    kgdb
    kselftest
    kunit/index

quoted

+   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.

quoted

+	mutex_lock(&priv->lock);

quoted

+	if (priv->blob_dent) {

Redundant (i.e. duplicate).

Nope, it can be NULL if allocating memory all goes wrong.

quoted

+gpio_err:

A bit confusing name. What about

enable_irq_and_free_data:

Yes, fixed in v6.

quoted

+	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.

quoted

+	if (ret >= 0 && ret != priv->descs->ndescs)

quoted

+		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.

quoted

+		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.

quoted

+	dev_info(dev, "initialized");

Is it useful?

For the third time, yes!

quoted

+	cat <<EOF

	cat << EOF

is slightly easier to read.

I'll fix it.

quoted

+	[ -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.

quoted

+	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?

quoted

+			[ "$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

quoted

+			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

quoted

+		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.

quoted

+	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