Re: [PATCH v5 1/1] gpio: add sloppy logic analyzer using polling

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 4192 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]