Re: [PATCH 1/4] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc

[Kent Gibson <warthog618@xxxxxxxxx>]

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?

> >  };
>
> ...
>
> > +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.
>

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.

>
> > +out_unlock:
> > +	spin_unlock(&supinfo.lock);
>
> No use of cleanup.h?
>

Again, that is new to me, so no not yet.

> ...
>
> > +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.