lkml.org 
[lkml]   [2020]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2020-07-25 22:53    [W:4.111 / U:0.868 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site