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
Attachment:
signature.asc
Description: PGP signature
