lkml.org 
[lkml]   [2021]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling
Hi Andy,

> I would like to look at it closer, but don't have time right now. So,
> some kind of a shallow review.

Still, thanks for that!

> But the idea is, let's say, interesting.

:)

> > +The binding documentation is in the ``misc`` folder of the Kernel binding
> > +documentation.
>
> Can't you give a reference in terms of reST format?

Sure. Still need to practice reST.

> > +config GPIO_LOGIC_ANALYZER
> > + tristate "Simple GPIO logic analyzer"
> > + depends on GPIOLIB || COMPILE_TEST
> > + help
> > + This option enables support for a simple logic analyzer using polled
> > + GPIOs. Use the 'tools/debugging/gpio-logic-analyzer' script with this
> > + driver. The script will make using it easier and can also isolate a
> > + CPU for the polling task. Note that this is still a last resort
> > + analyzer which can be affected by latencies and non-determinant code
> > + paths. However, for e.g. remote development, it may be useful to get
> > + a first view and aid further debugging.
>
> Module name?

Yup, willl add.

> > +#include <linux/of.h>
>
> Can you switch to use device property API?

IIRC I checked that and I couldn't find a replacement for
of_property_read_string_index().

> > +/* can be increased if needed */
> > +#define GPIO_LA_MAX_PROBES 8
> > +#define GPIO_LA_PROBES_MASK 7
>
> Does this assume the power-of-two number of probes?
> Perhaps using BIT(x) and (BIT(x) - 1) will clarify that.

The arbitrary limit of 8 probes is solely to get this out now for
initial review, to check if this is upstreamable at all. If this is
considered acceptable, I can also update this to 64 probes and can get
rid of some more hackish code (e.g. fallback names of probes), too.

> > +struct gpio_la_poll_priv {
> > + unsigned long ndelay;
> > + u32 buf_idx;
> > + struct mutex lock;
> > + struct debugfs_blob_wrapper blob;
> > + struct gpio_descs *descs;
> > + struct dentry *debug_dir, *blob_dent;
> > + struct debugfs_blob_wrapper meta;
> > + unsigned long gpio_delay;
> > + unsigned int trigger_len;
>
> > + u8 trigger_data[PAGE_SIZE];
>
> This is not good for fragmentation (basically you make your struct to
> occupy 2 pages, one of which will be almost wasted). Better to have a
> pointer here and allocate one page by get_zero_page() or so.

Point taken. I will have a look.

> > + if (val) {
>
> if (!val)
> return 0;
>
> makes your life easier.

Yeah, it is cruft from an earlier version

> > + if (ret)
>
> > + pr_err("%s: couldn't read GPIOs: %d\n", __func__, ret);
>
> Haven't noticed if you are using pr_fmt(). It may be better than using __func__.
>
> Btw, it seems you have a struct device for that or so. Why don't you
> use dev_err()?

Will check.

> > + if (buf[i] < '1' || buf[i] > '0' + GPIO_LA_MAX_PROBES)
>
> So, you can't increase the amount of probes without breaking this
> entire parser (it will go somewhere to symbols and letters...).

Yeah. This is why I put GPIO_LA_MAX_PROBES there. When I upgrade the
number of probes, I need to have a look at all place using this define.
This code is ugly, I know.

> Shouldn't you return OVERFLOW here or something like that?

I could. But 4K of trigger data is also invalid. It is an academic
discussion, though.

> I'm not a fan of yet another parser in the kernel. Can you provide a
> bit of description of the format?

It is in the help of the script. I could maybe add it to the docs, too.

> > + if (IS_ERR(priv->debug_dir))
> > + return PTR_ERR(priv->debug_dir);
>
> Shouldn't be checked AFAIU.

Oh, really? Will check.

> > +static const struct of_device_id gpio_la_poll_of_match[] = {
> > + { .compatible = GPIO_LA_NAME, },
>
> > + { },
>
> No comma needed.

OK.

Thanks for your time!

Wolfram

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-03-30 17:43    [W:0.203 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site