On Wed, Dec 13, 2023 at 3:27 PM Kent Gibson <warthog618@xxxxxxxxx> wrote: > > On Wed, Dec 13, 2023 at 03:54:53PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 12, 2023 at 01:42:50PM +0800, Kent Gibson wrote: > > > Store the debounce period for a requested line locally, rather than in > > > the debounce_period_us field in the gpiolib struct gpio_desc. > > > > > > Add a global tree of lines containing supplemental line information > > > to make the debounce period available to be reported by the > > > GPIO_V2_GET_LINEINFO_IOCTL and the line change notifier. > > > > ... > > > > > struct line { > > > struct gpio_desc *desc; > > > + struct rb_node node; > > > > If you swap them, would it benefit in a code generation (bloat-o-meter)? > > > > Didn't consider that placement within the scruct could impact code > generation. > Having the rb_nodes at the beginning of struct is preferable? > I suppose it has something to do with 0 offset when using container_of(). Not sure if that really matters though. > > > }; > > > > ... > > > > > +struct supinfo { > > > + spinlock_t lock; > > > + struct rb_root tree; > > > +}; > > > > Same Q. > > > > Same - I tend to put locks before the field(s) they cover. > But if the node being first results in nicer code then happy to swap. > > > ... > > > > > +static struct supinfo supinfo; > > > > Why supinfo should be a struct to begin with? Seems to me as an unneeded > > complication. > > I think we should keep it as a struct but defined the following way: struct { spinlock_t lock; struct rb_root tree; } supinfo; > > Yeah, that is a hangover from an earlier iteration where supinfo was > contained in other object rather than being a global. > Could merge the struct definition into the variable now. > > > ... > > > > > + pr_warn("%s: duplicate line inserted\n", __func__); > > > > I hope at bare minimum we have pr_fmt(), but even though this is poor message > > that might require some information about exact duplication (GPIO chip label / > > name, line number, etc). Generally speaking the __func__ in non-debug messages > > _usually_ is a symptom of poorly written message. > > > > ... > > Yeah, I wasn't sure about the best way to log here. > > The details of chip or line etc don't add anything - seeing this error > means there is a logic error in the code - we have inserted a line > without erasing it. Knowing which chip or line it happened to occur on > wont help debug it. It should never happen, but you can't just leave it > unhandled, so I went with a basic log. > We should yell loudly in that case - use one of the WARN() variants that'll print a stack trace too and point you to the relevant line in the code. > > > > > +out_unlock: > > > + spin_unlock(&supinfo.lock); > > > > No use of cleanup.h? > > > > Again, that is new to me, so no not yet. > Yep, please use a guard, they're awesome. :) > > ... > > > > > +static inline bool line_is_supplemental(struct line *line) > > > +{ > > > + return READ_ONCE(line->debounce_period_us) != 0; > > > > " != 0" is redundant. > > > > I prefer conversion from int to bool to be explicit, but if you > insist... > > > > +} > > > > ... > > > > > for (i = 0; i < lr->num_lines; i++) { > > > - if (lr->lines[i].desc) { > > > - edge_detector_stop(&lr->lines[i]); > > > - gpiod_free(lr->lines[i].desc); > > > + line = &lr->lines[i]; > > > + if (line->desc) { > > > > Perhaps > > > > if (!line->desc) > > continue; > > > > ? > > Seems reasonable - I was just going with what was already there. > > > > > > + edge_detector_stop(line); > > > + if (line_is_supplemental(line)) > > > + supinfo_erase(line); > > > + gpiod_free(line->desc); > > > } > > > } > > > > ... > > > > > +static int __init gpiolib_cdev_init(void) > > > +{ > > > + supinfo_init(); > > > + return 0; > > > +} > > > > It's a good practice to explain initcalls (different to the default ones), > > can you add a comment on top to explain the choice of this initcall, please? > > > > Not sure what you mean. This section used gpiolib-sysfs as a template, > and that has no documentation. > > > > +postcore_initcall(gpiolib_cdev_init); > > > > Thanks for the review - always instructive. > > Cheers, > Kent. Bart