Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling

[Wolfram Sang]

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