Messages in this thread | | | Date | Tue, 30 Mar 2021 17:41:31 +0200 | From | Wolfram Sang <> | Subject | Re: [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] | |