| Message ID | 20240508144741.1270912-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] gpiolib: Return label, if set, for IRQ only line | expand |
On Wed, May 8, 2024 at 4:47 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > If line has been locked as IRQ without requesting, > still check its label and return it, if not NULL. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index db536ec9734d..1f1673552767 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -105,16 +105,16 @@ const char *gpiod_get_label(struct gpio_desc *desc) > unsigned long flags; > > flags = READ_ONCE(desc->flags); > - if (test_bit(FLAG_USED_AS_IRQ, &flags) && > - !test_bit(FLAG_REQUESTED, &flags)) > - return "interrupt"; > - > - if (!test_bit(FLAG_REQUESTED, &flags)) > - return NULL; > > label = srcu_dereference_check(desc->label, &desc->srcu, > srcu_read_lock_held(&desc->srcu)); > > + if (test_bit(FLAG_USED_AS_IRQ, &flags)) > + return label->str ?: "interrupt"; > + > + if (!test_bit(FLAG_REQUESTED, &flags)) > + return NULL; > + > return label->str; > } > > -- > 2.43.0.rc1.1336.g36b5255a03ac > What good would it be if gpiochip_dup_line_label() returns NULL for unrequested lines anyway? Bart
Thu, May 09, 2024 at 03:15:05PM +0200, Bartosz Golaszewski kirjoitti: > On Wed, May 8, 2024 at 4:47 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > If line has been locked as IRQ without requesting, > > still check its label and return it, if not NULL. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/gpio/gpiolib.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index db536ec9734d..1f1673552767 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -105,16 +105,16 @@ const char *gpiod_get_label(struct gpio_desc *desc) > > unsigned long flags; > > > > flags = READ_ONCE(desc->flags); > > - if (test_bit(FLAG_USED_AS_IRQ, &flags) && > > - !test_bit(FLAG_REQUESTED, &flags)) > > - return "interrupt"; > > - > > - if (!test_bit(FLAG_REQUESTED, &flags)) > > - return NULL; > > > > label = srcu_dereference_check(desc->label, &desc->srcu, > > srcu_read_lock_held(&desc->srcu)); > > > > + if (test_bit(FLAG_USED_AS_IRQ, &flags)) > > + return label->str ?: "interrupt"; > > + > > + if (!test_bit(FLAG_REQUESTED, &flags)) > > + return NULL; > > + > > return label->str; > > } > > What good would it be if gpiochip_dup_line_label() returns NULL for > unrequested lines anyway? Then it should be handled differently in those cases. So, consider it as a preparatory patch which doesn't change current behaviour. (Yes, I have some hack patches locally which do something useful, but they are not ready. In any case this one looks to me as a good cleanup on its own for the sake of readability of the code and reduced amount of checks.)
On Thu, May 9, 2024 at 3:58 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > Thu, May 09, 2024 at 03:15:05PM +0200, Bartosz Golaszewski kirjoitti: > > On Wed, May 8, 2024 at 4:47 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > If line has been locked as IRQ without requesting, > > > still check its label and return it, if not NULL. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/gpio/gpiolib.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > index db536ec9734d..1f1673552767 100644 > > > --- a/drivers/gpio/gpiolib.c > > > +++ b/drivers/gpio/gpiolib.c > > > @@ -105,16 +105,16 @@ const char *gpiod_get_label(struct gpio_desc *desc) > > > unsigned long flags; > > > > > > flags = READ_ONCE(desc->flags); > > > - if (test_bit(FLAG_USED_AS_IRQ, &flags) && > > > - !test_bit(FLAG_REQUESTED, &flags)) > > > - return "interrupt"; > > > - > > > - if (!test_bit(FLAG_REQUESTED, &flags)) > > > - return NULL; > > > > > > label = srcu_dereference_check(desc->label, &desc->srcu, > > > srcu_read_lock_held(&desc->srcu)); > > > > > > + if (test_bit(FLAG_USED_AS_IRQ, &flags)) > > > + return label->str ?: "interrupt"; > > > + > > > + if (!test_bit(FLAG_REQUESTED, &flags)) > > > + return NULL; > > > + > > > return label->str; > > > } > > > > What good would it be if gpiochip_dup_line_label() returns NULL for > > unrequested lines anyway? > > Then it should be handled differently in those cases. So, consider it as > a preparatory patch which doesn't change current behaviour. > > (Yes, I have some hack patches locally which do something useful, but they are > not ready. In any case this one looks to me as a good cleanup on its own for > the sake of readability of the code and reduced amount of checks.) > Fair enough but I would like to know what your bigger plan is before picking this up. Bart
On Thu, May 09, 2024 at 04:23:07PM +0200, Bartosz Golaszewski wrote: > On Thu, May 9, 2024 at 3:58 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > Thu, May 09, 2024 at 03:15:05PM +0200, Bartosz Golaszewski kirjoitti: > > > On Wed, May 8, 2024 at 4:47 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > If line has been locked as IRQ without requesting, > > > > still check its label and return it, if not NULL. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > drivers/gpio/gpiolib.c | 12 ++++++------ > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > index db536ec9734d..1f1673552767 100644 > > > > --- a/drivers/gpio/gpiolib.c > > > > +++ b/drivers/gpio/gpiolib.c > > > > @@ -105,16 +105,16 @@ const char *gpiod_get_label(struct gpio_desc *desc) > > > > unsigned long flags; > > > > > > > > flags = READ_ONCE(desc->flags); > > > > - if (test_bit(FLAG_USED_AS_IRQ, &flags) && > > > > - !test_bit(FLAG_REQUESTED, &flags)) > > > > - return "interrupt"; > > > > - > > > > - if (!test_bit(FLAG_REQUESTED, &flags)) > > > > - return NULL; > > > > > > > > label = srcu_dereference_check(desc->label, &desc->srcu, > > > > srcu_read_lock_held(&desc->srcu)); > > > > > > > > + if (test_bit(FLAG_USED_AS_IRQ, &flags)) > > > > + return label->str ?: "interrupt"; > > > > + > > > > + if (!test_bit(FLAG_REQUESTED, &flags)) > > > > + return NULL; > > > > + > > > > return label->str; > > > > } > > > > > > What good would it be if gpiochip_dup_line_label() returns NULL for > > > unrequested lines anyway? > > > > Then it should be handled differently in those cases. So, consider it as > > a preparatory patch which doesn't change current behaviour. > > > > (Yes, I have some hack patches locally which do something useful, but they are > > not ready. In any case this one looks to me as a good cleanup on its own for > > the sake of readability of the code and reduced amount of checks.) > > > > Fair enough but I would like to know what your bigger plan is before > picking this up. I stand corrected, this patch has an immediate effect on the generic gpiolib_dbg_show() which does *not* use the above mentioned call..
On Fri, May 10, 2024 at 06:42:28PM +0300, Andy Shevchenko wrote: > On Thu, May 09, 2024 at 04:23:07PM +0200, Bartosz Golaszewski wrote: > > On Thu, May 9, 2024 at 3:58 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: ... > > Fair enough but I would like to know what your bigger plan is before > > picking this up. > > I stand corrected, this patch has an immediate effect on the generic > gpiolib_dbg_show() which does *not* use the above mentioned call.. Ah, but it doesn't use gpiod_get_label() in the else branch either... I want to amend the else branch there to print similar or reuse the main one. For the latter I have locally a patch to modify gpiolib_dbg_show() to show the interrupt lines as well even if they are not requested.
On Fri, May 10, 2024 at 06:48:52PM +0300, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 06:42:28PM +0300, Andy Shevchenko wrote: > > On Thu, May 09, 2024 at 04:23:07PM +0200, Bartosz Golaszewski wrote: > > > On Thu, May 9, 2024 at 3:58 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: ... > > > Fair enough but I would like to know what your bigger plan is before > > > picking this up. > > > > I stand corrected, this patch has an immediate effect on the generic > > gpiolib_dbg_show() which does *not* use the above mentioned call.. > > Ah, but it doesn't use gpiod_get_label() in the else branch either... > > I want to amend the else branch there to print similar or reuse the main one. > For the latter I have locally a patch to modify gpiolib_dbg_show() to show > the interrupt lines as well even if they are not requested. I just shared that patch, if you are okay with both, it would be nice to have them applied.
On Fri, May 10, 2024 at 6:06 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, May 10, 2024 at 06:48:52PM +0300, Andy Shevchenko wrote: > > On Fri, May 10, 2024 at 06:42:28PM +0300, Andy Shevchenko wrote: > > > On Thu, May 09, 2024 at 04:23:07PM +0200, Bartosz Golaszewski wrote: > > > > On Thu, May 9, 2024 at 3:58 PM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > ... > > > > > Fair enough but I would like to know what your bigger plan is before > > > > picking this up. > > > > > > I stand corrected, this patch has an immediate effect on the generic > > > gpiolib_dbg_show() which does *not* use the above mentioned call.. > > > > Ah, but it doesn't use gpiod_get_label() in the else branch either... > > > > I want to amend the else branch there to print similar or reuse the main one. > > For the latter I have locally a patch to modify gpiolib_dbg_show() to show > > the interrupt lines as well even if they are not requested. > > I just shared that patch, if you are okay with both, it would be nice > to have them applied. > Not for this merge window, we'll see in two weeks. Bart
On Fri, May 10, 2024 at 8:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Fri, May 10, 2024 at 6:06 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, May 10, 2024 at 06:48:52PM +0300, Andy Shevchenko wrote: > > > On Fri, May 10, 2024 at 06:42:28PM +0300, Andy Shevchenko wrote: > > > > On Thu, May 09, 2024 at 04:23:07PM +0200, Bartosz Golaszewski wrote: > > > > > On Thu, May 9, 2024 at 3:58 PM Andy Shevchenko > > > > > <andy.shevchenko@gmail.com> wrote: ... > > > > > Fair enough but I would like to know what your bigger plan is before > > > > > picking this up. > > > > > > > > I stand corrected, this patch has an immediate effect on the generic > > > > gpiolib_dbg_show() which does *not* use the above mentioned call.. > > > > > > Ah, but it doesn't use gpiod_get_label() in the else branch either... > > > > > > I want to amend the else branch there to print similar or reuse the main one. > > > For the latter I have locally a patch to modify gpiolib_dbg_show() to show > > > the interrupt lines as well even if they are not requested. > > > > I just shared that patch, if you are okay with both, it would be nice > > to have them applied. > > Not for this merge window, we'll see in two weeks. No hurry, I'm fine with your schedule.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index db536ec9734d..1f1673552767 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -105,16 +105,16 @@ const char *gpiod_get_label(struct gpio_desc *desc) unsigned long flags; flags = READ_ONCE(desc->flags); - if (test_bit(FLAG_USED_AS_IRQ, &flags) && - !test_bit(FLAG_REQUESTED, &flags)) - return "interrupt"; - - if (!test_bit(FLAG_REQUESTED, &flags)) - return NULL; label = srcu_dereference_check(desc->label, &desc->srcu, srcu_read_lock_held(&desc->srcu)); + if (test_bit(FLAG_USED_AS_IRQ, &flags)) + return label->str ?: "interrupt"; + + if (!test_bit(FLAG_REQUESTED, &flags)) + return NULL; + return label->str; }
If line has been locked as IRQ without requesting, still check its label and return it, if not NULL. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)