Messages in this thread |  | | From | Andy Shevchenko <> | Date | Sat, 25 Jul 2020 23:51:54 +0300 | Subject | Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL |
| |
On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and > returning their current values using GPIOLINE_GET_VALUES_IOCTL.
...
> +struct line { > + struct gpio_device *gdev; > + const char *label; > + u32 num_descs;
> + /* descs must be last so it can be dynamically sized */
I guess [] implies above comment and thus comment can be dropped.
> + struct gpio_desc *descs[]; > +};
...
> +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() ?
...
> +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--) { ... }
> + if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) &&
> + test_bit(line_idx, (unsigned long *)lc->attrs[i].mask))
This casting is not good. What about BE 32-bit architecture?
> + return lc->attrs[i].attr.flags; > + } > + return lc->flags; > +} > + > +static int gpioline_config_output_value(struct gpioline_config *lc, > + int line_idx) > +{
Same comments as per above.
> +}
...
> +static long line_get_values(struct line *line, void __user *ip) > +{ > + struct gpioline_values lv;
> + unsigned long *vals = (unsigned long *)lv.bits;
Casting u64 to unsigned long is not good.
> +}
...
> +static void line_free(struct line *line) > +{ > + int i; > + > + for (i = 0; i < line->num_descs; i++) {
> + if (line->descs[i])
Redundant?
> + gpiod_free(line->descs[i]); > + } > + kfree(line->label); > + put_device(&line->gdev->dev); > + kfree(line); > +}
...
> + /* Make sure this is terminated */ > + linereq.consumer[sizeof(linereq.consumer)-1] = '\0'; > + if (strlen(linereq.consumer)) { > + line->label = kstrdup(linereq.consumer, GFP_KERNEL);
kstrndup() ?
> + if (!line->label) { > + ret = -ENOMEM; > + goto out_free_line; > + } > + }
...
> + 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?
-- With Best Regards, Andy Shevchenko
|  |