lkml.org 
[lkml]   [2021]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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]
\
 
 \ /
  Last update: 2021-12-18 10:40    [W:0.106 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site