[no subject]

> > > > > +}

...

> > > > > +static int gpio_virtuser_sysfs_parse_value_array(const char *buf, size_t len,
> > > > > +                                              unsigned long *values)
> > > > > +{
> > > > > +     size_t i;
> > > > > +
> > > > > +     for (i = 0; i < len; i++) {
> > > >
> > > > Perhaps
> > > >
> > > >                 bool val;
> > > >                 int ret;
> > > >
> > > >                 ret = kstrtobool(...);
> > >
> > > kstrtobool() accepts values we don't want here like [Tt]rue and [Ff]alse.
> >
> > Yes, see below.
> >
> > > >                 if (ret)
> > > >                         return ret;
> > > >
> > > >                 assign_bit(...); // btw, why atomic?
> > > >
> > > > > +             if (buf[i] == '0')
> > > > > +                     clear_bit(i, values);
> > > > > +             else if (buf[i] == '1')
> > > > > +                     set_bit(i, values);
> > > > > +             else
> > > > > +                     return -EINVAL;
> > > >
> > > > > +     }
> > > >
> > > > BUT, why not bitmap_parse()?
> > >
> > > Because it parses hex, not binary.
> >
> > So, why do we reinvent a wheel? Wouldn't be better that users may apply the
> > knowledge they are familiar with (and I believe the group of the users who know
> > about bitmaps is much bigger than those who will use this driver).

You see, you ignored this comment.

> > > > > +     return 0;
> > > > > +}

...

> > At bare minumum you can reduce the range by using kstrtou8().
>
> As opposed to just checking '0' and '1'? Meh...

Okay, can you explain why you need to take bigger numbers when it's
going to be used only in a very reduced range? Esp. negative ones.
Whatever, your choice.

...

> > > > > +     int i = 0;
> > > >
> > > > Why signed? And in all this kind of case, I would split assignment...
> >
> > (1)
> >
> > > > > +     memset(properties, 0, sizeof(properties));
> > > > > +
> > > > > +     num_ids = list_count_nodes(&dev->lookup_list);
> > > > > +     char **ids __free(kfree) = kcalloc(num_ids + 1, sizeof(*ids),
> > > > > +                                        GFP_KERNEL);
> > > > > +     if (!ids)
> > > > > +             return ERR_PTR(-ENOMEM);
> > > > > +
> > > >
> > > > To be here, that the reader will see immediately (close enough) what is the
> > > > initial values. Moreover this code will be robuse against changes in between
> > > > (if i become reusable).
> > >
> > > Sorry, I can't parse it.
> >
> > I meant to see here
> >
> >         i = 0;
> >
> > instead of the above (1).
>
> Why? I see no good reason.

There are two:
1) readability as explained above;
2) maintenance.

The second one is good in case the same variable (that's the very
often case for such as "i" is going to be reused in the future by some
new code. But okay, in this case you probably will be the only one for
maintenance.

> > > > > +     list_for_each_entry(lookup, &dev->lookup_list, siblings)
> > > > > +             ids[i++] = lookup->con_id;


-- 
With Best Regards,
Andy Shevchenko