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.828 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site