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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Dec 18, 2021 at 11:21 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:

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

I mean that there are similar functionality in different tools and for
one purpose you need one, for another another and there is no format
file convertors available (as far as my shallow googling shows).

...

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

Ah, okay, then choose the one you think fits most.

...

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

No strong opinion, I don't know the Sigrok tool and its file format,
so I can't tell if it makes sense or doesn't.

...

> > `> /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?

I'm a bit lost in the context here, but the ' > /dev/null 2>&1' means
to redirect stdout to the /dev/null followed by redirecting stderr to
stdout (which is redirected to /dev/null). The other construction
might have side effects IIRC.

...

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

The SO talks about _bash_, your script is a plain Shell one, right?
And for the record, I don't like bashisms in some generic code, like
the one we use with Linux kernel.

...

> > > +   taskset "$1" echo 1 > "$lasysfsdir"/capture || fail "Capture error! Check kernel log"
> >
> > Shouldn't this function setup signal TRAPs?
>
> To do what?

To clean up the garbage it may leave in case of the interrupted run, no?

...

> > $@ is better, actually one should never use $*.
>
> What difference does it make when expanding into a string?

The difference is on how the  "foo bar" (with double quotes!) will be
represented. In your case it will be translated as "foo" and "bar", in
the case I'm saying it will be "foo bar".

-- 
With Best Regards,
Andy Shevchenko