Messages in this thread | | | Date | Thu, 17 Aug 2023 15:38:50 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v5] gpio: consumer: new virtual driver |
| |
On Thu, Aug 17, 2023 at 02:14:04PM +0200, Bartosz Golaszewski wrote: > On Thu, Aug 17, 2023 at 12:03 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Aug 15, 2023 at 08:56:50PM +0200, Bartosz Golaszewski wrote:
...
> > > + struct gpio_consumer_device *dev = lookup->parent; > > > + > > > + guard(mutex)(&dev->lock); > > > + > > > + return sprintf(page, "%s\n", lookup->key);
(1)
...
> > > +static ssize_t > > > +gpio_consumer_lookup_config_offset_show(struct config_item *item, char *page) > > > +{ > > > + struct gpio_consumer_lookup *lookup = to_gpio_consumer_lookup(item); > > > + struct gpio_consumer_device *dev = lookup->parent; > > > + unsigned int offset; > > > + > > > + scoped_guard(mutex, &dev->lock) > > > + offset = lookup->offset; > > > + > > > + return sprintf(page, "%d\n", offset); > > > > Consistently it can be simplified same way > > > > guard(mutex)(&dev->lock); > > > > return sprintf(page, "%d\n", lookup->offset); > > > > BUT. Thinking about this more. With guard() we put sprintf() inside the lock, > > which is suboptimal from runtime point of view. So, I think now that all these > > should actually use scoped_guard() rather than guard(). > > > > Precisely why I used a scoped guard here. Same elsewhere.
So the 1) has to be amended then.
> > > +}
...
> > > + enum gpio_lookup_flags flags; > > > + > > > + flags = gpio_consumer_lookup_get_flags(item); > > > > This is perfectly one line < 80 characters. > > There's nothing wrong with setting the variable on another line though.
Why do we need 3 LoCs instead of a single one? Do you increase your line statistics? :-) I really would like to know the rationale behind this.
-- With Best Regards, Andy Shevchenko
| |