Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@xxxxxxxxx> 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