Re: [PATCH v2] gpio: consumer: new virtual driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Aug 09, 2023 at 09:29:14AM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 8, 2023 at 8:11 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Aug 08, 2023 at 04:56:05PM +0200, Bartosz Golaszewski wrote:

...

> > But again, why not
> >
> >         timer_data->val ^= 1;
> 
> This is not ok in my book. If I need to think for more than a second
> about what it does, then it's worse. I put clarity over brevity.

Yes, this will require bit ops to check.

...

> > This can be avoided by
> >
> >         key = kstrndup(skip_spaces(page), count, GFP_KERNEL);
> >
> > no?
> >
> 
> No, because we also want to remove the trailing spaces and newlines.
> But if you have a different suggestion with existing helpers, let me
> know. I didn't find any.

kstrto*() are newline friendly. The rest as you noted can be covered with
sysfs_streq() / sysfs_match_string().

...

> > > +     ret = kstrtoint(page, 0, &offset);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /* Use -1 to indicate lookup by name. */
> >
> > This comment is unclear as offset can be -1 given by the user.
> > What does above mean in that context?
> 
> I added this to the documentation. Negative number means: lookup by
> line name, positive or zero - lookup offset in chip.

Then add a link to the documentation from here? Because reading this comment
and the code is confusing.

> > > +     if (offset > (U16_MAX - 1))
> >
> > And how does it related to this -1 if related at all?
> 
> GPIOLIB interprets U16_MAX as "lookup by line name". So we can allow
> max (U16_MAX - 1). I will add a comment.
> 
> > > +             return -EINVAL;

...

> > > +     struct gpio_consumer_device *dev;
> > > +
> > > +     dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > > +     if (!dev)
> > > +             return ERR_PTR(-ENOMEM);
> > > +
> > > +     dev->id = ida_alloc(&gpio_consumer_ida, GFP_KERNEL);
> > > +     if (dev->id < 0) {
> > > +             kfree(dev);
> >
> > Wondering if you can utilize cleanup.h.
> 
> Whooaah! In february this year I suggested basic C RAII during my talk
> at fosdem and here we are? I missed this one. Yeah, I will use it!
> Even better, I will abuse the cr*p out of it in gpio-sim as well!
> Thanks for bringing this to my attention. This may be the best thing
> that happened to kernel C code in years if people widely adopt it.
> (This paragraph was written by a fan of GLib's autopointer paradigm.
> :) )

So let it be!

-- 
With Best Regards,
Andy Shevchenko