Messages in this thread |  | | From | Bartosz Golaszewski <> | Date | Fri, 31 Jul 2020 18:05:10 +0200 | Subject | Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL |
| |
On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote: >
[snip]
> > > > > +static bool padding_not_zeroed(__u32 *padding, int pad_size) > > > +{ > > > + int i, sum = 0; > > > + > > > + for (i = 0; i < pad_size; i++) > > > + sum |= padding[i]; > > > + > > > + return sum; > > > +} > > > > Reimplementation of memchr_inv() ? > > > > I was hoping to find an existing function, surely checking a region is > zeroed is a common thing, right?, so this was a place holder as much > as anything. Not sure memchr_inv fits the bill, but I'll give it a > try... >
If you don't find an appropriate function: please put your new implementation in lib/ so that others may reuse it.
> > ... > > > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) > > > +{ > > > + int i; > > > + > > > + for (i = lc->num_attrs - 1; i >= 0; i--) { > > > > Much better to read is > > > > unsigned int i = lc->num_attrs; > > > > while (i--) { > > ... > > } > > > > Really? I find that the post-decrement in the while makes determining the > bounds of the loop more confusing. >
Agreed, Andy: this is too much nit-picking. :)
[snip]
> > ... > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset); > > > > I prefer to see this split, but it's minor. > > > > > + if (IS_ERR(desc)) { > > > + ret = PTR_ERR(desc); > > > + goto out_free_line; > > > + } > > > > ... > > > > > + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > > + offset); > > > > Perhaps tracepoint / event? > > > > Again, a cut-and-paste from V1, and I have no experience with > tracepoints or events, so I have no opinion on that. > > So, yeah - perhaps? >
I think it's a good idea to add some proper instrumentation this time other than much less reliable logs. Can you take a look at include/trace/events/gpio.h? Adding new GPIO trace events should be pretty straightforward by copy-pasti... drawing inspiration from existing ones.
Bart
|  |