* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
@ 2023-12-14 16:46 Andy Shevchenko
2023-12-14 21:05 ` Bartosz Golaszewski
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2023-12-14 16:46 UTC (permalink / raw)
To: Kent Gibson; +Cc: Bartosz Golaszewski, linux-kernel, linux-gpio, linus.walleij
On Thu, Dec 14, 2023 at 11:08:05PM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 03:56:37PM +0100, Bartosz Golaszewski wrote:
...
> While I think of it, what tree should I be basing on?
> These patches are based on v6.7-rc5, and I'm not aware of any other
> changes they may contend with, but best to be on the right tree to be
> sure.
General rule is to base on the target subsystem tree. In this case
it's Bart's gpio/for-next AFAIU.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 16:46 [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc Andy Shevchenko
@ 2023-12-14 21:05 ` Bartosz Golaszewski
0 siblings, 0 replies; 17+ messages in thread
From: Bartosz Golaszewski @ 2023-12-14 21:05 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Kent Gibson, linux-kernel, linux-gpio, linus.walleij
On Thu, Dec 14, 2023 at 5:47 PM Andy Shevchenko <andy@kernel.org> wrote:
>
>
> On Thu, Dec 14, 2023 at 11:08:05PM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 03:56:37PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > While I think of it, what tree should I be basing on?
> > These patches are based on v6.7-rc5, and I'm not aware of any other
> > changes they may contend with, but best to be on the right tree to be
> > sure.
>
> General rule is to base on the target subsystem tree. In this case
> it's Bart's gpio/for-next AFAIU.
>
Normally the patches should apply on top of
git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
Any conflicts between maintainer trees are handled upstream.
Bart
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
@ 2023-12-15 20:23 Andy Shevchenko
0 siblings, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2023-12-15 20:23 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Kent Gibson, linux-kernel, linux-gpio, linus.walleij
On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
...
> > > > > > +static void supinfo_init(void)
> > > > > > +{
> > > > > > + supinfo.tree = RB_ROOT;
> > > > > > + spin_lock_init(&supinfo.lock);
> > > > > > +}
> > > > >
> > > > > Can it be done statically?
> > > > >
> > > > > supinfo = {
> > > > > .tree = RB_ROOT,
> > > > > .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
>
> Double underscore typically means it's private and shouldn't be used.
Right, but when you have a struct you have no other means to initialize this
directly.
> > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
See, there are users of the __ initializers.
> > > Ah, that is what you meant. Yeah sure can - the supinfo_init() is
> > > another hangover from when I was trying to create the supinfo per chip,
> > > but now it is a global a static initialiser makes sense.
> >
> > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > than above.
>
> Yeah, so maybe we should use non-struct, global variables after all.
At least this will allow to get rid of (questionable) initcall.
> > > And I still haven't received the email you quote there.
> >
> > :-( I'm not sure we will get it, it most likely that I removed it already
> > and it has disappeared due to problems with email server...
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 21:06 ` Bartosz Golaszewski
2023-12-15 1:04 ` Kent Gibson
@ 2023-12-15 16:31 ` Andy Shevchenko
1 sibling, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2023-12-15 16:31 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: Kent Gibson, linux-kernel, linux-gpio, linus.walleij
On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
...
> > > > > > +static void supinfo_init(void)
> > > > > > +{
> > > > > > + supinfo.tree = RB_ROOT;
> > > > > > + spin_lock_init(&supinfo.lock);
> > > > > > +}
> > > > >
> > > > > Can it be done statically?
> > > > >
> > > > > supinfo = {
> > > > > .tree = RB_ROOT,
> > > > > .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
>
> Double underscore typically means it's private and shouldn't be used.
Right, but when you have a struct you have no other means to initialize this
directly.
> > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
See, there are users of the __ initializers.
> > > Ah, that is what you meant. Yeah sure can - the supinfo_init() is
> > > another hangover from when I was trying to create the supinfo per chip,
> > > but now it is a global a static initialiser makes sense.
> >
> > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > than above.
>
> Yeah, so maybe we should use non-struct, global variables after all.
At least this will allow to get rid of (questionable) initcall.
> > > And I still haven't received the email you quote there.
> >
> > :-( I'm not sure we will get it, it most likely that I removed it already
> > and it has disappeared due to problems with email server...
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-15 8:07 ` Bartosz Golaszewski
@ 2023-12-15 8:15 ` Kent Gibson
0 siblings, 0 replies; 17+ messages in thread
From: Kent Gibson @ 2023-12-15 8:15 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Andy Shevchenko, linux-kernel, linux-gpio, linus.walleij
On Fri, Dec 15, 2023 at 09:07:48AM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 15, 2023 at 2:04 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > > >
> > > > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > +static void supinfo_init(void)
> > > > > > > > +{
> > > > > > > > + supinfo.tree = RB_ROOT;
> > > > > > > > + spin_lock_init(&supinfo.lock);
> > > > > > > > +}
> > > > > > >
> > > > > > > Can it be done statically?
> > > > > > >
> > > > > > > supinfo = {
> > > > > > > .tree = RB_ROOT,
> > > > > > > .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> > >
> > > Double underscore typically means it's private and shouldn't be used.
> > >
> >
> > You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
> > all used in gpiolib.c?
> >
>
> Touché. But this is just lack of strict naming conventions. :( Another
> common use of leading underscores are "unlocked" (or in this case:
> non-atomic) variants of functions.
>
Sorry, should've added a ;-) to the end of that one - not giving you a
hard time, just found it amusing.
> > > > > >
> > > > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > > > >
>
> As opposed to ~1200 uses of DEFINE_SPINLOCK if you really want to go there. :)
>
To be clear, that is Andy's quote you are replying to :-).
> > > > > Ah, that is what you meant. Yeah sure can - the supinfo_init() is
> > > > > another hangover from when I was trying to create the supinfo per chip,
> > > > > but now it is a global a static initialiser makes sense.
> > > >
> > > > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > > > than above.
> > >
> > > Yeah, so maybe we should use non-struct, global variables after all.
> > >
> >
> > Despite the 32 cases cited that already use that pattern?
> > 9 of which use __SPIN_LOCK_UNLOCKED().
> > Sounds like a pretty convincing argument to use the struct ;-).
> >
> > But lets keep it as kosher as possible and split out the struct :-(.
> >
>
> I'll leave it for you to decide, I don't have a strong opinion and the
> entire file is your code so you should pick.
>
I've split it out in v3.
Cheers,
Kent.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-15 1:04 ` Kent Gibson
@ 2023-12-15 8:07 ` Bartosz Golaszewski
2023-12-15 8:15 ` Kent Gibson
0 siblings, 1 reply; 17+ messages in thread
From: Bartosz Golaszewski @ 2023-12-15 8:07 UTC (permalink / raw)
To: Kent Gibson; +Cc: Andy Shevchenko, linux-kernel, linux-gpio, linus.walleij
On Fri, Dec 15, 2023 at 2:04 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> > >
> > > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> > >
> > > ...
> > >
> > > > > > > +static void supinfo_init(void)
> > > > > > > +{
> > > > > > > + supinfo.tree = RB_ROOT;
> > > > > > > + spin_lock_init(&supinfo.lock);
> > > > > > > +}
> > > > > >
> > > > > > Can it be done statically?
> > > > > >
> > > > > > supinfo = {
> > > > > > .tree = RB_ROOT,
> > > > > > .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> >
> > Double underscore typically means it's private and shouldn't be used.
> >
>
> You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
> all used in gpiolib.c?
>
Touché. But this is just lack of strict naming conventions. :( Another
common use of leading underscores are "unlocked" (or in this case:
non-atomic) variants of functions.
> > > > >
> > > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > > >
As opposed to ~1200 uses of DEFINE_SPINLOCK if you really want to go there. :)
> > > > Ah, that is what you meant. Yeah sure can - the supinfo_init() is
> > > > another hangover from when I was trying to create the supinfo per chip,
> > > > but now it is a global a static initialiser makes sense.
> > >
> > > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > > than above.
> >
> > Yeah, so maybe we should use non-struct, global variables after all.
> >
>
> Despite the 32 cases cited that already use that pattern?
> 9 of which use __SPIN_LOCK_UNLOCKED().
> Sounds like a pretty convincing argument to use the struct ;-).
>
> But lets keep it as kosher as possible and split out the struct :-(.
>
I'll leave it for you to decide, I don't have a strong opinion and the
entire file is your code so you should pick.
Bart
> Cheers,
> Kent.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 21:06 ` Bartosz Golaszewski
@ 2023-12-15 1:04 ` Kent Gibson
2023-12-15 8:07 ` Bartosz Golaszewski
2023-12-15 16:31 ` Andy Shevchenko
1 sibling, 1 reply; 17+ messages in thread
From: Kent Gibson @ 2023-12-15 1:04 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Andy Shevchenko, linux-kernel, linux-gpio, linus.walleij
On Thu, Dec 14, 2023 at 10:06:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
> >
> > On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
> >
> > ...
> >
> > > > > > +static void supinfo_init(void)
> > > > > > +{
> > > > > > + supinfo.tree = RB_ROOT;
> > > > > > + spin_lock_init(&supinfo.lock);
> > > > > > +}
> > > > >
> > > > > Can it be done statically?
> > > > >
> > > > > supinfo = {
> > > > > .tree = RB_ROOT,
> > > > > .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
>
> Double underscore typically means it's private and shouldn't be used.
>
You mean like __assign_bit(), __set_bit(), __clear_bit() and __free() -
all used in gpiolib.c?
> > > >
> > > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> > >
> > > Ah, that is what you meant. Yeah sure can - the supinfo_init() is
> > > another hangover from when I was trying to create the supinfo per chip,
> > > but now it is a global a static initialiser makes sense.
> >
> > Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> > than above.
>
> Yeah, so maybe we should use non-struct, global variables after all.
>
Despite the 32 cases cited that already use that pattern?
9 of which use __SPIN_LOCK_UNLOCKED().
Sounds like a pretty convincing argument to use the struct ;-).
But lets keep it as kosher as possible and split out the struct :-(.
Cheers,
Kent.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 16:41 ` Andy Shevchenko
@ 2023-12-14 21:06 ` Bartosz Golaszewski
2023-12-15 1:04 ` Kent Gibson
2023-12-15 16:31 ` Andy Shevchenko
0 siblings, 2 replies; 17+ messages in thread
From: Bartosz Golaszewski @ 2023-12-14 21:06 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Kent Gibson, linux-kernel, linux-gpio, linus.walleij
On Thu, Dec 14, 2023 at 5:41 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> > On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
>
> ...
>
> > > > > +static void supinfo_init(void)
> > > > > +{
> > > > > + supinfo.tree = RB_ROOT;
> > > > > + spin_lock_init(&supinfo.lock);
> > > > > +}
> > > >
> > > > Can it be done statically?
> > > >
> > > > supinfo = {
> > > > .tree = RB_ROOT,
> > > > .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
Double underscore typically means it's private and shouldn't be used.
> > >
> > > I even checked the current tree, we have 32 users of this pattern in drivers/.
> >
> > Ah, that is what you meant. Yeah sure can - the supinfo_init() is
> > another hangover from when I was trying to create the supinfo per chip,
> > but now it is a global a static initialiser makes sense.
>
> Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
> than above.
Yeah, so maybe we should use non-struct, global variables after all.
Bart
>
> > And I still haven't received the email you quote there.
>
> :-( I'm not sure we will get it, it most likely that I removed it already
> and it has disappeared due to problems with email server...
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 16:14 ` Kent Gibson
@ 2023-12-14 16:41 ` Andy Shevchenko
2023-12-14 21:06 ` Bartosz Golaszewski
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2023-12-14 16:41 UTC (permalink / raw)
To: Kent Gibson; +Cc: linux-kernel, linux-gpio, brgl, linus.walleij
On Fri, Dec 15, 2023 at 12:14:41AM +0800, Kent Gibson wrote:
> On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
...
> > > > +static void supinfo_init(void)
> > > > +{
> > > > + supinfo.tree = RB_ROOT;
> > > > + spin_lock_init(&supinfo.lock);
> > > > +}
> > >
> > > Can it be done statically?
> > >
> > > supinfo = {
> > > .tree = RB_ROOT,
> > > .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
> >
> > I even checked the current tree, we have 32 users of this pattern in drivers/.
>
> Ah, that is what you meant. Yeah sure can - the supinfo_init() is
> another hangover from when I was trying to create the supinfo per chip,
> but now it is a global a static initialiser makes sense.
Yep, the DEFINE_MUTEX() / DEFINE_SPINLOCK() / etc looks better naturally
than above.
> And I still haven't received the email you quote there.
:-( I'm not sure we will get it, it most likely that I removed it already
and it has disappeared due to problems with email server...
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 15:09 ` Andy Shevchenko
@ 2023-12-14 16:14 ` Kent Gibson
2023-12-14 16:41 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Kent Gibson @ 2023-12-14 16:14 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-kernel, linux-gpio, brgl, linus.walleij
On Thu, Dec 14, 2023 at 05:09:01PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
>
> ...
>
> > > +/*
> > > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > > + * in the struct gpio_desc.
> > > + * A line is determined to contain supplemental information by
> > > + * line_is_supplemental().
> > > + */
> > > +static struct {
> > > + /* a rbtree of the struct lines containing the supplemental info */
> > > + struct rb_root tree;
> > > + /* covers tree */
> > > + spinlock_t lock;
> > > +} supinfo;
>
> Hmm... If I read the kernel-doc script it should support anonymous structs
> and unions...
>
> ...
>
> > > +static void supinfo_init(void)
> > > +{
> > > + supinfo.tree = RB_ROOT;
> > > + spin_lock_init(&supinfo.lock);
> > > +}
> >
> > Can it be done statically?
> >
> > supinfo = {
> > .tree = RB_ROOT,
> > .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
>
> I even checked the current tree, we have 32 users of this pattern in drivers/.
>
Ah, that is what you meant. Yeah sure can - the supinfo_init() is
another hangover from when I was trying to create the supinfo per chip,
but now it is a global a static initialiser makes sense.
And I still haven't received the email you quote there.
Cheers,
Kent.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 15:03 ` Andy Shevchenko
@ 2023-12-14 15:09 ` Andy Shevchenko
2023-12-14 16:14 ` Kent Gibson
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2023-12-14 15:09 UTC (permalink / raw)
To: Kent Gibson; +Cc: linux-kernel, linux-gpio, brgl, linus.walleij
On Thu, Dec 14, 2023 at 05:03:03PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 14, 2023 at 05:58:11PM +0800, Kent Gibson wrote:
...
> > +/*
> > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > + * in the struct gpio_desc.
> > + * A line is determined to contain supplemental information by
> > + * line_is_supplemental().
> > + */
> > +static struct {
> > + /* a rbtree of the struct lines containing the supplemental info */
> > + struct rb_root tree;
> > + /* covers tree */
> > + spinlock_t lock;
> > +} supinfo;
Hmm... If I read the kernel-doc script it should support anonymous structs
and unions...
...
> > +static void supinfo_init(void)
> > +{
> > + supinfo.tree = RB_ROOT;
> > + spin_lock_init(&supinfo.lock);
> > +}
>
> Can it be done statically?
>
> supinfo = {
> .tree = RB_ROOT,
> .lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
I even checked the current tree, we have 32 users of this pattern in drivers/.
> };
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 14:56 ` Bartosz Golaszewski
@ 2023-12-14 15:08 ` Kent Gibson
0 siblings, 0 replies; 17+ messages in thread
From: Kent Gibson @ 2023-12-14 15:08 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-kernel, linux-gpio, linus.walleij, andy
On Thu, Dec 14, 2023 at 03:56:37PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 3:45 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > >
> > > > +/*
> > > > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > > > + * in the struct gpio_desc.
> > > > + * A line is determined to contain supplemental information by
> > > > + * line_is_supplemental().
> > > > + */
> > > > +static struct {
> > > > + /* a rbtree of the struct lines containing the supplemental info */
> > > > + struct rb_root tree;
> > > > + /* covers tree */
> > > > + spinlock_t lock;
> > >
> > > Looks like this is never taken from atomic context? Can this be a mutex instead?
> > >
> >
> > Correct, only from thread context.
> >
> > Can be a mutex, but it only covers tree lookups which should be quick
> > as the tree is kept minimal, and I wouldn't expect it to ever get to the
> > mutex slowpath, so a spinlock seemed more appropriate.
> >
>
> Fair enough.
>
> Bart
>
While I think of it, what tree should I be basing on?
These patches are based on v6.7-rc5, and I'm not aware of any other
changes they may contend with, but best to be on the right tree to be
sure.
Cheers,
Kent.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 9:58 ` [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc Kent Gibson
2023-12-14 14:29 ` Bartosz Golaszewski
@ 2023-12-14 15:03 ` Andy Shevchenko
2023-12-14 15:09 ` Andy Shevchenko
1 sibling, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2023-12-14 15:03 UTC (permalink / raw)
To: Kent Gibson; +Cc: linux-kernel, linux-gpio, brgl, linus.walleij
On Thu, Dec 14, 2023 at 05:58:11PM +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.
...
> +/*
> + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> + * in the struct gpio_desc.
> + * A line is determined to contain supplemental information by
> + * line_is_supplemental().
> + */
> +static struct {
> + /* a rbtree of the struct lines containing the supplemental info */
> + struct rb_root tree;
> + /* covers tree */
> + spinlock_t lock;
> +} supinfo;
...
> +static void supinfo_init(void)
> +{
> + supinfo.tree = RB_ROOT;
> + spin_lock_init(&supinfo.lock);
> +}
Can it be done statically?
supinfo = {
.tree = RB_ROOT,
.lock = __SPIN_LOCK_UNLOCKED(supinfo.lock),
};
...
> +static int __init gpiolib_cdev_init(void)
> +{
> + supinfo_init();
> + return 0;
> +}
A comment why it's postcore initcall?
/* postcore initcall is chosen because ... */
> +postcore_initcall(gpiolib_cdev_init);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 14:45 ` Kent Gibson
@ 2023-12-14 14:56 ` Bartosz Golaszewski
2023-12-14 15:08 ` Kent Gibson
0 siblings, 1 reply; 17+ messages in thread
From: Bartosz Golaszewski @ 2023-12-14 14:56 UTC (permalink / raw)
To: Kent Gibson; +Cc: linux-kernel, linux-gpio, linus.walleij, andy
On Thu, Dec 14, 2023 at 3:45 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > >
> > > +/*
> > > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > > + * in the struct gpio_desc.
> > > + * A line is determined to contain supplemental information by
> > > + * line_is_supplemental().
> > > + */
> > > +static struct {
> > > + /* a rbtree of the struct lines containing the supplemental info */
> > > + struct rb_root tree;
> > > + /* covers tree */
> > > + spinlock_t lock;
> >
> > Looks like this is never taken from atomic context? Can this be a mutex instead?
> >
>
> Correct, only from thread context.
>
> Can be a mutex, but it only covers tree lookups which should be quick
> as the tree is kept minimal, and I wouldn't expect it to ever get to the
> mutex slowpath, so a spinlock seemed more appropriate.
>
Fair enough.
Bart
> Cheers,
> Kent.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 14:29 ` Bartosz Golaszewski
@ 2023-12-14 14:45 ` Kent Gibson
2023-12-14 14:56 ` Bartosz Golaszewski
0 siblings, 1 reply; 17+ messages in thread
From: Kent Gibson @ 2023-12-14 14:45 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-kernel, linux-gpio, linus.walleij, andy
On Thu, Dec 14, 2023 at 03:29:28PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> >
> > +/*
> > + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> > + * in the struct gpio_desc.
> > + * A line is determined to contain supplemental information by
> > + * line_is_supplemental().
> > + */
> > +static struct {
> > + /* a rbtree of the struct lines containing the supplemental info */
> > + struct rb_root tree;
> > + /* covers tree */
> > + spinlock_t lock;
>
> Looks like this is never taken from atomic context? Can this be a mutex instead?
>
Correct, only from thread context.
Can be a mutex, but it only covers tree lookups which should be quick
as the tree is kept minimal, and I wouldn't expect it to ever get to the
mutex slowpath, so a spinlock seemed more appropriate.
Cheers,
Kent.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 9:58 ` [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc Kent Gibson
@ 2023-12-14 14:29 ` Bartosz Golaszewski
2023-12-14 14:45 ` Kent Gibson
2023-12-14 15:03 ` Andy Shevchenko
1 sibling, 1 reply; 17+ messages in thread
From: Bartosz Golaszewski @ 2023-12-14 14:29 UTC (permalink / raw)
To: Kent Gibson; +Cc: linux-kernel, linux-gpio, linus.walleij, andy
On Thu, Dec 14, 2023 at 10:58 AM Kent Gibson <warthog618@gmail.com> 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.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
> drivers/gpio/gpiolib-cdev.c | 167 +++++++++++++++++++++++++++++++-----
> 1 file changed, 145 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index d03698812f61..7da3b3706547 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -21,6 +21,7 @@
> #include <linux/mutex.h>
> #include <linux/pinctrl/consumer.h>
> #include <linux/poll.h>
> +#include <linux/rbtree.h>
> #include <linux/seq_file.h>
> #include <linux/spinlock.h>
> #include <linux/timekeeping.h>
> @@ -461,6 +462,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>
> /**
> * struct line - contains the state of a requested line
> + * @node: to store the object in supinfo if supplemental
> * @desc: the GPIO descriptor for this line.
> * @req: the corresponding line request
> * @irq: the interrupt triggered in response to events on this GPIO
> @@ -473,6 +475,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> * @line_seqno: the seqno for the current edge event in the sequence of
> * events for this line.
> * @work: the worker that implements software debouncing
> + * @debounce_period_us: the debounce period in microseconds
> * @sw_debounced: flag indicating if the software debouncer is active
> * @level: the current debounced physical level of the line
> * @hdesc: the Hardware Timestamp Engine (HTE) descriptor
> @@ -481,6 +484,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> * @last_seqno: the last sequence number before debounce period expires
> */
> struct line {
> + struct rb_node node;
> struct gpio_desc *desc;
> /*
> * -- edge detector specific fields --
> @@ -514,6 +518,15 @@ struct line {
> * -- debouncer specific fields --
> */
> struct delayed_work work;
> + /*
> + * debounce_period_us is accessed by debounce_irq_handler() and
> + * process_hw_ts() which are disabled when modified by
> + * debounce_setup(), edge_detector_setup() or edge_detector_stop()
> + * or can live with a stale version when updated by
> + * edge_detector_update().
> + * The modifying functions are themselves mutually exclusive.
> + */
> + unsigned int debounce_period_us;
> /*
> * sw_debounce is accessed by linereq_set_config(), which is the
> * only setter, and linereq_get_values(), which can live with a
> @@ -546,6 +559,19 @@ struct line {
> #endif /* CONFIG_HTE */
> };
>
> +/*
> + * Used to populate gpio_v2_line_info with cdev specific fields not contained
> + * in the struct gpio_desc.
> + * A line is determined to contain supplemental information by
> + * line_is_supplemental().
> + */
> +static struct {
> + /* a rbtree of the struct lines containing the supplemental info */
> + struct rb_root tree;
> + /* covers tree */
> + spinlock_t lock;
Looks like this is never taken from atomic context? Can this be a mutex instead?
> +} supinfo;
> +
> /**
> * struct linereq - contains the state of a userspace line request
> * @gdev: the GPIO device the line request pertains to
> @@ -575,6 +601,100 @@ struct linereq {
> struct line lines[] __counted_by(num_lines);
> };
>
> +static void supinfo_init(void)
> +{
> + supinfo.tree = RB_ROOT;
> + spin_lock_init(&supinfo.lock);
> +}
> +
> +static void supinfo_insert(struct line *line)
> +{
> + struct rb_node **new = &(supinfo.tree.rb_node), *parent = NULL;
> + struct line *entry;
> +
> + scoped_guard(spinlock, &supinfo.lock) {
> + while (*new) {
> + entry = container_of(*new, struct line, node);
> +
> + parent = *new;
> + if (line->desc < entry->desc) {
> + new = &((*new)->rb_left);
> + } else if (line->desc > entry->desc) {
> + new = &((*new)->rb_right);
> + } else {
> + /* this should never happen */
> + WARN(1, "duplicate line inserted");
> + return;
> + }
> + }
> +
> + rb_link_node(&line->node, parent, new);
> + rb_insert_color(&line->node, &supinfo.tree);
> + }
> +}
> +
> +static void supinfo_erase(struct line *line)
> +{
> + scoped_guard(spinlock, &supinfo.lock)
> + rb_erase(&line->node, &supinfo.tree);
> +}
> +
> +static struct line *supinfo_find(struct gpio_desc *desc)
> +{
> + struct rb_node *node = supinfo.tree.rb_node;
> + struct line *line;
> +
> + while (node) {
> + line = container_of(node, struct line, node);
> + if (desc < line->desc)
> + node = node->rb_left;
> + else if (desc > line->desc)
> + node = node->rb_right;
> + else
> + return line;
> + }
> + return NULL;
> +}
> +
> +static void supinfo_to_lineinfo(struct gpio_desc *desc,
> + struct gpio_v2_line_info *info)
> +{
> + struct gpio_v2_line_attribute *attr;
> + struct line *line;
> +
> + scoped_guard(spinlock, &supinfo.lock) {
> + line = supinfo_find(desc);
> + if (line) {
> + attr = &info->attrs[info->num_attrs];
> + attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> + attr->debounce_period_us =
> + READ_ONCE(line->debounce_period_us);
> + info->num_attrs++;
> + }
> + }
> +}
> +
> +static inline bool line_is_supplemental(struct line *line)
I would call this function line_has_suppinfo().
> +{
> + return READ_ONCE(line->debounce_period_us);
> +}
> +
> +static void line_set_debounce_period(struct line *line,
> + unsigned int debounce_period_us)
> +{
> + bool was_suppl = line_is_supplemental(line);
This logic could use some comments, it took me a while to figure out
what it's doing.
> +
> + WRITE_ONCE(line->debounce_period_us, debounce_period_us);
> +
> + if (line_is_supplemental(line) == was_suppl)
> + return;
> +
> + if (was_suppl)
> + supinfo_erase(line);
> + else
> + supinfo_insert(line);
> +}
> +
> #define GPIO_V2_LINE_BIAS_FLAGS \
> (GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
> GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
> @@ -723,7 +843,7 @@ static enum hte_return process_hw_ts(struct hte_ts_data *ts, void *p)
> line->total_discard_seq++;
> line->last_seqno = ts->seq;
> mod_delayed_work(system_wq, &line->work,
> - usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
> + usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
> } else {
> if (unlikely(ts->seq < line->line_seqno))
> return HTE_CB_HANDLED;
> @@ -864,7 +984,7 @@ static irqreturn_t debounce_irq_handler(int irq, void *p)
> struct line *line = p;
>
> mod_delayed_work(system_wq, &line->work,
> - usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
> + usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
>
> return IRQ_HANDLED;
> }
> @@ -946,7 +1066,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
> /* try hardware */
> ret = gpiod_set_debounce(line->desc, debounce_period_us);
> if (!ret) {
> - WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> + line_set_debounce_period(line, debounce_period_us);
> return ret;
> }
> if (ret != -ENOTSUPP)
> @@ -1025,8 +1145,7 @@ static void edge_detector_stop(struct line *line)
> cancel_delayed_work_sync(&line->work);
> WRITE_ONCE(line->sw_debounced, 0);
> WRITE_ONCE(line->edflags, 0);
> - if (line->desc)
> - WRITE_ONCE(line->desc->debounce_period_us, 0);
> + line_set_debounce_period(line, 0);
> /* do not change line->level - see comment in debounced_value() */
> }
>
> @@ -1051,7 +1170,7 @@ static int edge_detector_setup(struct line *line,
> ret = debounce_setup(line, debounce_period_us);
> if (ret)
> return ret;
> - WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> + line_set_debounce_period(line, debounce_period_us);
> }
>
> /* detection disabled or sw debouncer will provide edge detection */
> @@ -1093,12 +1212,12 @@ static int edge_detector_update(struct line *line,
> gpio_v2_line_config_debounce_period(lc, line_idx);
>
> if ((active_edflags == edflags) &&
> - (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us))
> + (READ_ONCE(line->debounce_period_us) == debounce_period_us))
> return 0;
>
> /* sw debounced and still will be...*/
> if (debounce_period_us && READ_ONCE(line->sw_debounced)) {
> - WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
> + line_set_debounce_period(line, debounce_period_us);
> return 0;
> }
>
> @@ -1561,6 +1680,7 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
>
> static void linereq_free(struct linereq *lr)
> {
> + struct line *line;
> unsigned int i;
>
> if (lr->device_unregistered_nb.notifier_call)
> @@ -1568,10 +1688,14 @@ static void linereq_free(struct linereq *lr)
> &lr->device_unregistered_nb);
>
> 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)
> + continue;
> +
> + edge_detector_stop(line);
> + if (line_is_supplemental(line))
> + supinfo_erase(line);
> + gpiod_free(line->desc);
> }
> kfifo_free(&lr->events);
> kfree(lr->label);
> @@ -2256,8 +2380,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> struct gpio_chip *gc = desc->gdev->chip;
> bool ok_for_pinctrl;
> unsigned long flags;
> - u32 debounce_period_us;
> - unsigned int num_attrs = 0;
>
> memset(info, 0, sizeof(*info));
> info->offset = gpio_chip_hwgpio(desc);
> @@ -2323,14 +2445,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags))
> info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
>
> - debounce_period_us = READ_ONCE(desc->debounce_period_us);
> - if (debounce_period_us) {
> - info->attrs[num_attrs].id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
> - info->attrs[num_attrs].debounce_period_us = debounce_period_us;
> - num_attrs++;
> - }
> - info->num_attrs = num_attrs;
> -
> spin_unlock_irqrestore(&gpio_lock, flags);
> }
>
> @@ -2437,6 +2551,7 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
> return -EBUSY;
> }
> gpio_desc_to_lineinfo(desc, &lineinfo);
> + supinfo_to_lineinfo(desc, &lineinfo);
>
> if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
> if (watch)
> @@ -2527,6 +2642,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
> chg.event_type = action;
> chg.timestamp_ns = ktime_get_ns();
> gpio_desc_to_lineinfo(desc, &chg.info);
> + supinfo_to_lineinfo(desc, &chg.info);
>
> ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
> if (ret)
> @@ -2786,3 +2902,10 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev)
> cdev_device_del(&gdev->chrdev, &gdev->dev);
> blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
> }
> +
> +static int __init gpiolib_cdev_init(void)
> +{
> + supinfo_init();
> + return 0;
> +}
> +postcore_initcall(gpiolib_cdev_init);
> --
> 2.39.2
>
Bart
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
2023-12-14 9:58 [PATCH v2 0/5] gpiolib: cdev: relocate debounce_period_us Kent Gibson
@ 2023-12-14 9:58 ` Kent Gibson
2023-12-14 14:29 ` Bartosz Golaszewski
2023-12-14 15:03 ` Andy Shevchenko
0 siblings, 2 replies; 17+ messages in thread
From: Kent Gibson @ 2023-12-14 9:58 UTC (permalink / raw)
To: linux-kernel, linux-gpio, brgl, linus.walleij, andy; +Cc: Kent Gibson
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.
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
drivers/gpio/gpiolib-cdev.c | 167 +++++++++++++++++++++++++++++++-----
1 file changed, 145 insertions(+), 22 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index d03698812f61..7da3b3706547 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -21,6 +21,7 @@
#include <linux/mutex.h>
#include <linux/pinctrl/consumer.h>
#include <linux/poll.h>
+#include <linux/rbtree.h>
#include <linux/seq_file.h>
#include <linux/spinlock.h>
#include <linux/timekeeping.h>
@@ -461,6 +462,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
/**
* struct line - contains the state of a requested line
+ * @node: to store the object in supinfo if supplemental
* @desc: the GPIO descriptor for this line.
* @req: the corresponding line request
* @irq: the interrupt triggered in response to events on this GPIO
@@ -473,6 +475,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
* @line_seqno: the seqno for the current edge event in the sequence of
* events for this line.
* @work: the worker that implements software debouncing
+ * @debounce_period_us: the debounce period in microseconds
* @sw_debounced: flag indicating if the software debouncer is active
* @level: the current debounced physical level of the line
* @hdesc: the Hardware Timestamp Engine (HTE) descriptor
@@ -481,6 +484,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
* @last_seqno: the last sequence number before debounce period expires
*/
struct line {
+ struct rb_node node;
struct gpio_desc *desc;
/*
* -- edge detector specific fields --
@@ -514,6 +518,15 @@ struct line {
* -- debouncer specific fields --
*/
struct delayed_work work;
+ /*
+ * debounce_period_us is accessed by debounce_irq_handler() and
+ * process_hw_ts() which are disabled when modified by
+ * debounce_setup(), edge_detector_setup() or edge_detector_stop()
+ * or can live with a stale version when updated by
+ * edge_detector_update().
+ * The modifying functions are themselves mutually exclusive.
+ */
+ unsigned int debounce_period_us;
/*
* sw_debounce is accessed by linereq_set_config(), which is the
* only setter, and linereq_get_values(), which can live with a
@@ -546,6 +559,19 @@ struct line {
#endif /* CONFIG_HTE */
};
+/*
+ * Used to populate gpio_v2_line_info with cdev specific fields not contained
+ * in the struct gpio_desc.
+ * A line is determined to contain supplemental information by
+ * line_is_supplemental().
+ */
+static struct {
+ /* a rbtree of the struct lines containing the supplemental info */
+ struct rb_root tree;
+ /* covers tree */
+ spinlock_t lock;
+} supinfo;
+
/**
* struct linereq - contains the state of a userspace line request
* @gdev: the GPIO device the line request pertains to
@@ -575,6 +601,100 @@ struct linereq {
struct line lines[] __counted_by(num_lines);
};
+static void supinfo_init(void)
+{
+ supinfo.tree = RB_ROOT;
+ spin_lock_init(&supinfo.lock);
+}
+
+static void supinfo_insert(struct line *line)
+{
+ struct rb_node **new = &(supinfo.tree.rb_node), *parent = NULL;
+ struct line *entry;
+
+ scoped_guard(spinlock, &supinfo.lock) {
+ while (*new) {
+ entry = container_of(*new, struct line, node);
+
+ parent = *new;
+ if (line->desc < entry->desc) {
+ new = &((*new)->rb_left);
+ } else if (line->desc > entry->desc) {
+ new = &((*new)->rb_right);
+ } else {
+ /* this should never happen */
+ WARN(1, "duplicate line inserted");
+ return;
+ }
+ }
+
+ rb_link_node(&line->node, parent, new);
+ rb_insert_color(&line->node, &supinfo.tree);
+ }
+}
+
+static void supinfo_erase(struct line *line)
+{
+ scoped_guard(spinlock, &supinfo.lock)
+ rb_erase(&line->node, &supinfo.tree);
+}
+
+static struct line *supinfo_find(struct gpio_desc *desc)
+{
+ struct rb_node *node = supinfo.tree.rb_node;
+ struct line *line;
+
+ while (node) {
+ line = container_of(node, struct line, node);
+ if (desc < line->desc)
+ node = node->rb_left;
+ else if (desc > line->desc)
+ node = node->rb_right;
+ else
+ return line;
+ }
+ return NULL;
+}
+
+static void supinfo_to_lineinfo(struct gpio_desc *desc,
+ struct gpio_v2_line_info *info)
+{
+ struct gpio_v2_line_attribute *attr;
+ struct line *line;
+
+ scoped_guard(spinlock, &supinfo.lock) {
+ line = supinfo_find(desc);
+ if (line) {
+ attr = &info->attrs[info->num_attrs];
+ attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
+ attr->debounce_period_us =
+ READ_ONCE(line->debounce_period_us);
+ info->num_attrs++;
+ }
+ }
+}
+
+static inline bool line_is_supplemental(struct line *line)
+{
+ return READ_ONCE(line->debounce_period_us);
+}
+
+static void line_set_debounce_period(struct line *line,
+ unsigned int debounce_period_us)
+{
+ bool was_suppl = line_is_supplemental(line);
+
+ WRITE_ONCE(line->debounce_period_us, debounce_period_us);
+
+ if (line_is_supplemental(line) == was_suppl)
+ return;
+
+ if (was_suppl)
+ supinfo_erase(line);
+ else
+ supinfo_insert(line);
+}
+
#define GPIO_V2_LINE_BIAS_FLAGS \
(GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
@@ -723,7 +843,7 @@ static enum hte_return process_hw_ts(struct hte_ts_data *ts, void *p)
line->total_discard_seq++;
line->last_seqno = ts->seq;
mod_delayed_work(system_wq, &line->work,
- usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
+ usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
} else {
if (unlikely(ts->seq < line->line_seqno))
return HTE_CB_HANDLED;
@@ -864,7 +984,7 @@ static irqreturn_t debounce_irq_handler(int irq, void *p)
struct line *line = p;
mod_delayed_work(system_wq, &line->work,
- usecs_to_jiffies(READ_ONCE(line->desc->debounce_period_us)));
+ usecs_to_jiffies(READ_ONCE(line->debounce_period_us)));
return IRQ_HANDLED;
}
@@ -946,7 +1066,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
/* try hardware */
ret = gpiod_set_debounce(line->desc, debounce_period_us);
if (!ret) {
- WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
+ line_set_debounce_period(line, debounce_period_us);
return ret;
}
if (ret != -ENOTSUPP)
@@ -1025,8 +1145,7 @@ static void edge_detector_stop(struct line *line)
cancel_delayed_work_sync(&line->work);
WRITE_ONCE(line->sw_debounced, 0);
WRITE_ONCE(line->edflags, 0);
- if (line->desc)
- WRITE_ONCE(line->desc->debounce_period_us, 0);
+ line_set_debounce_period(line, 0);
/* do not change line->level - see comment in debounced_value() */
}
@@ -1051,7 +1170,7 @@ static int edge_detector_setup(struct line *line,
ret = debounce_setup(line, debounce_period_us);
if (ret)
return ret;
- WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
+ line_set_debounce_period(line, debounce_period_us);
}
/* detection disabled or sw debouncer will provide edge detection */
@@ -1093,12 +1212,12 @@ static int edge_detector_update(struct line *line,
gpio_v2_line_config_debounce_period(lc, line_idx);
if ((active_edflags == edflags) &&
- (READ_ONCE(line->desc->debounce_period_us) == debounce_period_us))
+ (READ_ONCE(line->debounce_period_us) == debounce_period_us))
return 0;
/* sw debounced and still will be...*/
if (debounce_period_us && READ_ONCE(line->sw_debounced)) {
- WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
+ line_set_debounce_period(line, debounce_period_us);
return 0;
}
@@ -1561,6 +1680,7 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
static void linereq_free(struct linereq *lr)
{
+ struct line *line;
unsigned int i;
if (lr->device_unregistered_nb.notifier_call)
@@ -1568,10 +1688,14 @@ static void linereq_free(struct linereq *lr)
&lr->device_unregistered_nb);
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)
+ continue;
+
+ edge_detector_stop(line);
+ if (line_is_supplemental(line))
+ supinfo_erase(line);
+ gpiod_free(line->desc);
}
kfifo_free(&lr->events);
kfree(lr->label);
@@ -2256,8 +2380,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
struct gpio_chip *gc = desc->gdev->chip;
bool ok_for_pinctrl;
unsigned long flags;
- u32 debounce_period_us;
- unsigned int num_attrs = 0;
memset(info, 0, sizeof(*info));
info->offset = gpio_chip_hwgpio(desc);
@@ -2323,14 +2445,6 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
else if (test_bit(FLAG_EVENT_CLOCK_HTE, &desc->flags))
info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE;
- debounce_period_us = READ_ONCE(desc->debounce_period_us);
- if (debounce_period_us) {
- info->attrs[num_attrs].id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE;
- info->attrs[num_attrs].debounce_period_us = debounce_period_us;
- num_attrs++;
- }
- info->num_attrs = num_attrs;
-
spin_unlock_irqrestore(&gpio_lock, flags);
}
@@ -2437,6 +2551,7 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
return -EBUSY;
}
gpio_desc_to_lineinfo(desc, &lineinfo);
+ supinfo_to_lineinfo(desc, &lineinfo);
if (copy_to_user(ip, &lineinfo, sizeof(lineinfo))) {
if (watch)
@@ -2527,6 +2642,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
chg.event_type = action;
chg.timestamp_ns = ktime_get_ns();
gpio_desc_to_lineinfo(desc, &chg.info);
+ supinfo_to_lineinfo(desc, &chg.info);
ret = kfifo_in_spinlocked(&cdev->events, &chg, 1, &cdev->wait.lock);
if (ret)
@@ -2786,3 +2902,10 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev)
cdev_device_del(&gdev->chrdev, &gdev->dev);
blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
}
+
+static int __init gpiolib_cdev_init(void)
+{
+ supinfo_init();
+ return 0;
+}
+postcore_initcall(gpiolib_cdev_init);
--
2.39.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-12-18 10:08 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-14 16:46 [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc Andy Shevchenko
2023-12-14 21:05 ` Bartosz Golaszewski
-- strict thread matches above, loose matches on Subject: below --
2023-12-15 20:23 Andy Shevchenko
2023-12-14 9:58 [PATCH v2 0/5] gpiolib: cdev: relocate debounce_period_us Kent Gibson
2023-12-14 9:58 ` [PATCH v2 2/5] gpiolib: cdev: relocate debounce_period_us from struct gpio_desc Kent Gibson
2023-12-14 14:29 ` Bartosz Golaszewski
2023-12-14 14:45 ` Kent Gibson
2023-12-14 14:56 ` Bartosz Golaszewski
2023-12-14 15:08 ` Kent Gibson
2023-12-14 15:03 ` Andy Shevchenko
2023-12-14 15:09 ` Andy Shevchenko
2023-12-14 16:14 ` Kent Gibson
2023-12-14 16:41 ` Andy Shevchenko
2023-12-14 21:06 ` Bartosz Golaszewski
2023-12-15 1:04 ` Kent Gibson
2023-12-15 8:07 ` Bartosz Golaszewski
2023-12-15 8:15 ` Kent Gibson
2023-12-15 16:31 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.