| Message ID | 20231221175527.2814506-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] gpiolib: cdev: Split line_get_debounce_period() and use | expand |
On Thu, Dec 21, 2023 at 07:55:27PM +0200, Andy Shevchenko wrote: > Instead of repeating the same code and reduce possible miss > of READ_ONCE(), split line_get_debounce_period() heler out > and use in the existing cases. > helper Not a fan of this change. So using READ_ONCE() is repeating code?? Doesn't providing a wrapper around READ_ONCE() just rename that repitition? What of all the other uses of READ_ONCE() in cdev (and there are a lot) - why pick on debounce_period? The line_set_debounce_period() is necessary as the set is now a multi-step process as it can impact whether the line is contained in the supinfo_tree. The get is just a get. And you could've included me in the Cc so I didn't just find it by accident. Cheers, Kent. > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib-cdev.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index 744734405912..c573820d5722 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -651,6 +651,16 @@ static struct line *supinfo_find(struct gpio_desc *desc) > return NULL; > } > > +static unsigned int line_get_debounce_period(struct line *line) > +{ > + return READ_ONCE(line->debounce_period_us); > +} > + > +static inline bool line_has_supinfo(struct line *line) > +{ > + return line_get_debounce_period(line); > +} > + > static void supinfo_to_lineinfo(struct gpio_desc *desc, > struct gpio_v2_line_info *info) > { > @@ -665,15 +675,10 @@ static void supinfo_to_lineinfo(struct gpio_desc *desc, > > attr = &info->attrs[info->num_attrs]; > attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE; > - attr->debounce_period_us = READ_ONCE(line->debounce_period_us); > + attr->debounce_period_us = line_get_debounce_period(line); > info->num_attrs++; > } > > -static inline bool line_has_supinfo(struct line *line) > -{ > - return READ_ONCE(line->debounce_period_us); > -} > - > /* > * Checks line_has_supinfo() before and after the change to avoid unnecessary > * supinfo_tree access. > @@ -846,7 +851,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->debounce_period_us))); > + usecs_to_jiffies(line_get_debounce_period(line))); > } else { > if (unlikely(ts->seq < line->line_seqno)) > return HTE_CB_HANDLED; > @@ -987,7 +992,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->debounce_period_us))); > + usecs_to_jiffies(line_get_debounce_period(line))); > > return IRQ_HANDLED; > } > @@ -1215,7 +1220,7 @@ static int edge_detector_update(struct line *line, > gpio_v2_line_config_debounce_period(lc, line_idx); > > if ((active_edflags == edflags) && > - (READ_ONCE(line->debounce_period_us) == debounce_period_us)) > + (line_get_debounce_period(line) == debounce_period_us)) > return 0; > > /* sw debounced and still will be...*/ > -- > 2.43.0.rc1.1.gbec44491f096 >
On Fri, Dec 22, 2023 at 2:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Dec 21, 2023 at 07:55:27PM +0200, Andy Shevchenko wrote: > > Instead of repeating the same code and reduce possible miss > > of READ_ONCE(), split line_get_debounce_period() heler out > > and use in the existing cases. > > > > helper > > > Not a fan of this change. > Yeah, sorry but NAK. READ_ONCE() is well known and tells you what the code does. Arbitrary line_get_debounce_period() makes me have to look it up. Bart > So using READ_ONCE() is repeating code?? > Doesn't providing a wrapper around READ_ONCE() just rename that repitition? > What of all the other uses of READ_ONCE() in cdev (and there are a lot) - > why pick on debounce_period? > > The line_set_debounce_period() is necessary as the set is now a > multi-step process as it can impact whether the line is contained > in the supinfo_tree. The get is just a get. > > And you could've included me in the Cc so I didn't just find it by > accident. > > Cheers, > Kent. > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/gpio/gpiolib-cdev.c | 23 ++++++++++++++--------- > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > > index 744734405912..c573820d5722 100644 > > --- a/drivers/gpio/gpiolib-cdev.c > > +++ b/drivers/gpio/gpiolib-cdev.c > > @@ -651,6 +651,16 @@ static struct line *supinfo_find(struct gpio_desc *desc) > > return NULL; > > } > > > > +static unsigned int line_get_debounce_period(struct line *line) > > +{ > > + return READ_ONCE(line->debounce_period_us); > > +} > > + > > +static inline bool line_has_supinfo(struct line *line) > > +{ > > + return line_get_debounce_period(line); > > +} > > + > > static void supinfo_to_lineinfo(struct gpio_desc *desc, > > struct gpio_v2_line_info *info) > > { > > @@ -665,15 +675,10 @@ static void supinfo_to_lineinfo(struct gpio_desc *desc, > > > > attr = &info->attrs[info->num_attrs]; > > attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE; > > - attr->debounce_period_us = READ_ONCE(line->debounce_period_us); > > + attr->debounce_period_us = line_get_debounce_period(line); > > info->num_attrs++; > > } > > > > -static inline bool line_has_supinfo(struct line *line) > > -{ > > - return READ_ONCE(line->debounce_period_us); > > -} > > - > > /* > > * Checks line_has_supinfo() before and after the change to avoid unnecessary > > * supinfo_tree access. > > @@ -846,7 +851,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->debounce_period_us))); > > + usecs_to_jiffies(line_get_debounce_period(line))); > > } else { > > if (unlikely(ts->seq < line->line_seqno)) > > return HTE_CB_HANDLED; > > @@ -987,7 +992,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->debounce_period_us))); > > + usecs_to_jiffies(line_get_debounce_period(line))); > > > > return IRQ_HANDLED; > > } > > @@ -1215,7 +1220,7 @@ static int edge_detector_update(struct line *line, > > gpio_v2_line_config_debounce_period(lc, line_idx); > > > > if ((active_edflags == edflags) && > > - (READ_ONCE(line->debounce_period_us) == debounce_period_us)) > > + (line_get_debounce_period(line) == debounce_period_us)) > > return 0; > > > > /* sw debounced and still will be...*/ > > -- > > 2.43.0.rc1.1.gbec44491f096 > >
On Fri, Dec 22, 2023 at 09:12:37AM +0800, Kent Gibson wrote: > On Thu, Dec 21, 2023 at 07:55:27PM +0200, Andy Shevchenko wrote: > > Instead of repeating the same code and reduce possible miss > > of READ_ONCE(), split line_get_debounce_period() heler out > > and use in the existing cases. > > > > helper > > Not a fan of this change. > > So using READ_ONCE() is repeating code?? Yes. Because one may forget about it. > Doesn't providing a wrapper around READ_ONCE() just rename that repitition? > What of all the other uses of READ_ONCE() in cdev (and there are a lot) - > why pick on debounce_period? Because you have a setter, but getter. Inconsistency. > The line_set_debounce_period() is necessary as the set is now a > multi-step process as it can impact whether the line is contained > in the supinfo_tree. The get is just a get. > > And you could've included me in the Cc so I didn't just find it by > accident. Maybe it's time to add you to the MAINTAINERS for this file as a designated reviewer?
On Fri, Dec 22, 2023 at 09:58:48AM +0100, Bartosz Golaszewski wrote: > On Fri, Dec 22, 2023 at 2:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Thu, Dec 21, 2023 at 07:55:27PM +0200, Andy Shevchenko wrote: > > > Instead of repeating the same code and reduce possible miss > > > of READ_ONCE(), split line_get_debounce_period() heler out > > > and use in the existing cases. > > > > > > > helper > > > > > > Not a fan of this change. > > > > Yeah, sorry but NAK. READ_ONCE() is well known and tells you what the > code does. Arbitrary line_get_debounce_period() makes me have to look > it up. We have setter, but not getter. It looks confusing, more over, the setter makes much more than just set. Hence another way to solve this is make clear (by changing name) that the setter is not _just_ a setter.
On Fri, Dec 22, 2023 at 02:39:38PM +0200, Andy Shevchenko wrote: > On Fri, Dec 22, 2023 at 09:12:37AM +0800, Kent Gibson wrote: > > On Thu, Dec 21, 2023 at 07:55:27PM +0200, Andy Shevchenko wrote: > > > Instead of repeating the same code and reduce possible miss > > > of READ_ONCE(), split line_get_debounce_period() heler out > > > and use in the existing cases. > > > > > > > helper > > > > Not a fan of this change. > > > > So using READ_ONCE() is repeating code?? > > Yes. Because one may forget about it. Just as one may forget to use your wrapper. This argument is a NULL - so I'll just forget about it. > > > Doesn't providing a wrapper around READ_ONCE() just rename that repitition? > > What of all the other uses of READ_ONCE() in cdev (and there are a lot) - > > why pick on debounce_period? > > Because you have a setter, but getter. Inconsistency. > But then "for consistency" ALL the struct line fields require accessors and mutators. That path is insanity. The setter is there as setting the value now has side effects - none of which are visible to the caller, hence the usage of the standard setter name. You are siggesting every function name describe everything the function does? And, in case you've forgotten, YOU REVIEWED THIS. > > The line_set_debounce_period() is necessary as the set is now a > > multi-step process as it can impact whether the line is contained > > in the supinfo_tree. The get is just a get. > > > > And you could've included me in the Cc so I didn't just find it by > > accident. > > Maybe it's time to add you to the MAINTAINERS for this file as a designated > reviewer? > You are patching my recent change that you yourself reviewed only days ago. I would think that you would Cc me whether I were a maintainer or not as I'm very likely to have relevant feedback. Cheers, Kent.
On Fri, Dec 22, 2023 at 02:40:59PM +0200, Andy Shevchenko wrote: > On Fri, Dec 22, 2023 at 09:58:48AM +0100, Bartosz Golaszewski wrote: > > On Fri, Dec 22, 2023 at 2:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Thu, Dec 21, 2023 at 07:55:27PM +0200, Andy Shevchenko wrote: > > > > Instead of repeating the same code and reduce possible miss > > > > of READ_ONCE(), split line_get_debounce_period() heler out > > > > and use in the existing cases. > > > > > > > > > > helper > > > > > > > > > Not a fan of this change. > > > > > > > Yeah, sorry but NAK. READ_ONCE() is well known and tells you what the > > code does. Arbitrary line_get_debounce_period() makes me have to look > > it up. > > We have setter, but not getter. It looks confusing, more over, the setter makes > much more than just set. Hence another way to solve this is make clear (by > changing name) that the setter is not _just_ a setter. > As I mentioned elsewhere, the side effects of the setter are irrelevant to the caller, so from their point of view it is _just_ a setter. Calling it something else would actually be more confusing. Cheers, Kent.
On Fri, Dec 22, 2023 at 1:56 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Dec 22, 2023 at 02:39:38PM +0200, Andy Shevchenko wrote: > > On Fri, Dec 22, 2023 at 09:12:37AM +0800, Kent Gibson wrote: > > > On Thu, Dec 21, 2023 at 07:55:27PM +0200, Andy Shevchenko wrote: > > > > Instead of repeating the same code and reduce possible miss > > > > of READ_ONCE(), split line_get_debounce_period() heler out > > > > and use in the existing cases. > > > > > > > > > > helper > > > > > > Not a fan of this change. > > > > > > So using READ_ONCE() is repeating code?? > > > > Yes. Because one may forget about it. > > Just as one may forget to use your wrapper. > This argument is a NULL - so I'll just forget about it. > > > > > > Doesn't providing a wrapper around READ_ONCE() just rename that repitition? > > > What of all the other uses of READ_ONCE() in cdev (and there are a lot) - > > > why pick on debounce_period? > > > > Because you have a setter, but getter. Inconsistency. > > > > But then "for consistency" ALL the struct line fields require accessors > and mutators. That path is insanity. > > The setter is there as setting the value now has side effects - none of > which are visible to the caller, hence the usage of the standard > setter name. > You are siggesting every function name describe everything the function > does? > > And, in case you've forgotten, YOU REVIEWED THIS. > > > > The line_set_debounce_period() is necessary as the set is now a > > > multi-step process as it can impact whether the line is contained > > > in the supinfo_tree. The get is just a get. > > > > > > And you could've included me in the Cc so I didn't just find it by > > > accident. > > > > Maybe it's time to add you to the MAINTAINERS for this file as a designated > > reviewer? > > > > You are patching my recent change that you yourself reviewed only days > ago. I would think that you would Cc me whether I were a maintainer or > not as I'm very likely to have relevant feedback. On that note: do you see yourself as a full GPIO reviewer or do you prefer I split out the uAPI part into a separate section in MAINTAINERS and nominate you as its maintainer? Bart > > Cheers, > Kent.
On Fri, Dec 22, 2023 at 02:37:43PM +0100, Bartosz Golaszewski wrote: > On Fri, Dec 22, 2023 at 1:56 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > And you could've included me in the Cc so I didn't just find it by > > > > accident. > > > > > > Maybe it's time to add you to the MAINTAINERS for this file as a designated > > > reviewer? > > > > > > > You are patching my recent change that you yourself reviewed only days > > ago. I would think that you would Cc me whether I were a maintainer or > > not as I'm very likely to have relevant feedback. > > On that note: do you see yourself as a full GPIO reviewer or do you > prefer I split out the uAPI part into a separate section in > MAINTAINERS and nominate you as its maintainer? > Not sure I'm comfortable with either. Definitely not full GPIO. I don't feel sufficiently familiar with GPIO and the related subsystems to qualify. Splitting out cdev and the uAPI makes more sense to me, but in my mind at least even that requires a level of commitment higher than the rather spotty attention I've been providing recently. I'm more inclined to leave it as is. Cheers, Kent.
On Fri, 22 Dec 2023 at 15:08, Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Dec 22, 2023 at 02:37:43PM +0100, Bartosz Golaszewski wrote: > > On Fri, Dec 22, 2023 at 1:56 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > And you could've included me in the Cc so I didn't just find it by > > > > > accident. > > > > > > > > Maybe it's time to add you to the MAINTAINERS for this file as a designated > > > > reviewer? > > > > > > > > > > You are patching my recent change that you yourself reviewed only days > > > ago. I would think that you would Cc me whether I were a maintainer or > > > not as I'm very likely to have relevant feedback. > > > > On that note: do you see yourself as a full GPIO reviewer or do you > > prefer I split out the uAPI part into a separate section in > > MAINTAINERS and nominate you as its maintainer? > > > > Not sure I'm comfortable with either. > > Definitely not full GPIO. I don't feel sufficiently familiar with GPIO > and the related subsystems to qualify. > > Splitting out cdev and the uAPI makes more sense to me, but in my mind at > least even that requires a level of commitment higher than the rather > spotty attention I've been providing recently. > I'm more inclined to leave it as is. > I can still split the uAPI files into their own section, make Linus and myself maintainers and make you a reviewer, how about that? Bart > Cheers, > Kent.
On Fri, Dec 22, 2023 at 03:09:54PM +0100, Bartosz Golaszewski wrote: > On Fri, 22 Dec 2023 at 15:08, Kent Gibson <warthog618@gmail.com> wrote: > > > > On Fri, Dec 22, 2023 at 02:37:43PM +0100, Bartosz Golaszewski wrote: > > > On Fri, Dec 22, 2023 at 1:56 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > And you could've included me in the Cc so I didn't just find it by > > > > > > accident. > > > > > > > > > > Maybe it's time to add you to the MAINTAINERS for this file as a designated > > > > > reviewer? > > > > > > > > > > > > > You are patching my recent change that you yourself reviewed only days > > > > ago. I would think that you would Cc me whether I were a maintainer or > > > > not as I'm very likely to have relevant feedback. > > > > > > On that note: do you see yourself as a full GPIO reviewer or do you > > > prefer I split out the uAPI part into a separate section in > > > MAINTAINERS and nominate you as its maintainer? > > > > > > > Not sure I'm comfortable with either. > > > > Definitely not full GPIO. I don't feel sufficiently familiar with GPIO > > and the related subsystems to qualify. > > > > Splitting out cdev and the uAPI makes more sense to me, but in my mind at > > least even that requires a level of commitment higher than the rather > > spotty attention I've been providing recently. > > I'm more inclined to leave it as is. > > > > I can still split the uAPI files into their own section, make Linus > and myself maintainers and make you a reviewer, how about that? > That is closer to the reality, so that would work for me. Cheers, Kent.
On Fri, Dec 22, 2023 at 3:15 PM Kent Gibson <warthog618@gmail.com> wrote: > On Fri, Dec 22, 2023 at 03:09:54PM +0100, Bartosz Golaszewski wrote: > > I can still split the uAPI files into their own section, make Linus > > and myself maintainers and make you a reviewer, how about that? > > That is closer to the reality, so that would work for me. Hmm I think of Kent as one of the main architects for UAPI v2 so I would like you as maintainer, and me to be dropped, I already responded to that patch though. Yours, Linus Walleij
On Fri, Dec 22, 2023 at 06:49:03PM +0100, Linus Walleij wrote: > On Fri, Dec 22, 2023 at 3:15 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Dec 22, 2023 at 03:09:54PM +0100, Bartosz Golaszewski wrote: > > > > I can still split the uAPI files into their own section, make Linus > > > and myself maintainers and make you a reviewer, how about that? > > > > That is closer to the reality, so that would work for me. > > Hmm I think of Kent as one of the main architects for UAPI v2 > so I would like you as maintainer, and me to be dropped, I already > responded to that patch though. > There is no escaping that my fingerprints are all over that so it does make sense to list me over you. Given that patch and git-tree management will be deferred to the GPIO subsystem/Bart, there isn't much distinction between a reviewer and a maintainer, so I'm ok with being listed as a maintainer - I'll just have to pay a bit more attention to the list mails than I have been. Cheers, Kent.
Hi Kent, On Sat, Dec 23, 2023 at 3:08 AM Kent Gibson <warthog618@gmail.com> wrote: > There is no escaping that my fingerprints are all over that so it does > make sense to list me over you. Given that patch and git-tree management > will be deferred to the GPIO subsystem/Bart, there isn't much distinction > between a reviewer and a maintainer, so I'm ok with being listed as a > maintainer - I'll just have to pay a bit more attention to the list mails > than I have been. You're doing fine with responsiveness and feedback is always timely and verbose, so just keep doing what you do :) Yours, Linus Walleij
On Thu, Dec 28, 2023 at 01:26:13AM +0100, Linus Walleij wrote: > Hi Kent, > > On Sat, Dec 23, 2023 at 3:08 AM Kent Gibson <warthog618@gmail.com> wrote: > > > There is no escaping that my fingerprints are all over that so it does > > make sense to list me over you. Given that patch and git-tree management > > will be deferred to the GPIO subsystem/Bart, there isn't much distinction > > between a reviewer and a maintainer, so I'm ok with being listed as a > > maintainer - I'll just have to pay a bit more attention to the list mails > > than I have been. > > You're doing fine with responsiveness and feedback is always timely > and verbose, so just keep doing what you do :) > I endeavour to reply to direct mail promptly, but I was thinking more of mail directed generally to the list and there have been times I haven't checked the list for months. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 744734405912..c573820d5722 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -651,6 +651,16 @@ static struct line *supinfo_find(struct gpio_desc *desc) return NULL; } +static unsigned int line_get_debounce_period(struct line *line) +{ + return READ_ONCE(line->debounce_period_us); +} + +static inline bool line_has_supinfo(struct line *line) +{ + return line_get_debounce_period(line); +} + static void supinfo_to_lineinfo(struct gpio_desc *desc, struct gpio_v2_line_info *info) { @@ -665,15 +675,10 @@ static void supinfo_to_lineinfo(struct gpio_desc *desc, attr = &info->attrs[info->num_attrs]; attr->id = GPIO_V2_LINE_ATTR_ID_DEBOUNCE; - attr->debounce_period_us = READ_ONCE(line->debounce_period_us); + attr->debounce_period_us = line_get_debounce_period(line); info->num_attrs++; } -static inline bool line_has_supinfo(struct line *line) -{ - return READ_ONCE(line->debounce_period_us); -} - /* * Checks line_has_supinfo() before and after the change to avoid unnecessary * supinfo_tree access. @@ -846,7 +851,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->debounce_period_us))); + usecs_to_jiffies(line_get_debounce_period(line))); } else { if (unlikely(ts->seq < line->line_seqno)) return HTE_CB_HANDLED; @@ -987,7 +992,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->debounce_period_us))); + usecs_to_jiffies(line_get_debounce_period(line))); return IRQ_HANDLED; } @@ -1215,7 +1220,7 @@ static int edge_detector_update(struct line *line, gpio_v2_line_config_debounce_period(lc, line_idx); if ((active_edflags == edflags) && - (READ_ONCE(line->debounce_period_us) == debounce_period_us)) + (line_get_debounce_period(line) == debounce_period_us)) return 0; /* sw debounced and still will be...*/
Instead of repeating the same code and reduce possible miss of READ_ONCE(), split line_get_debounce_period() heler out and use in the existing cases. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-cdev.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)