diff mbox series

gpiolib: Drop cargo-culted comment

Message ID 20231125-dropcomment-v1-1-15800415aae0@linaro.org
State New
Headers
Series gpiolib: Drop cargo-culted comment |

Commit Message

Linus Walleij Nov. 24, 2023, 11:25 p.m. UTC 
  This comment about the gpio_lock is just completely confusing and
misleading. This refers to a gpio_desc that would in 2008 be used
to hold the list of gpio_chips, but nowadays gpio_desc refers to
descriptors of individual GPIO lines and this comment is completely
unparseable. Delete it.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 4 ----
 1 file changed, 4 deletions(-)


---
base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
change-id: 20231125-dropcomment-89e5b7b4cc3f

Best regards,

Comments

Kent Gibson Nov. 25, 2023, 2:40 a.m. UTC | #1 
On Sat, Nov 25, 2023 at 12:25:47AM +0100, Linus Walleij wrote:
> This comment about the gpio_lock is just completely confusing and
> misleading. This refers to a gpio_desc that would in 2008 be used
> to hold the list of gpio_chips, but nowadays gpio_desc refers to
> descriptors of individual GPIO lines and this comment is completely
> unparseable. Delete it.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 95d2a7b2ea3e..1c47af866bf6 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -86,10 +86,6 @@ static struct bus_type gpio_bus_type = {
>   */
>  #define FASTPATH_NGPIO CONFIG_GPIOLIB_FASTPATH_LIMIT
>
> -/* gpio_lock prevents conflicts during gpio_desc[] table updates.
> - * While any GPIO is requested, its gpio_chip is not removable;
> - * each GPIO's "requested" flag serves as a lock and refcount.
> - */

Perhaps provide a comment as to what the gpio_lock DOES cover?

>  DEFINE_SPINLOCK(gpio_lock);
>
>  static DEFINE_MUTEX(gpio_lookup_lock);
>
> ---
> base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
> change-id: 20231125-dropcomment-89e5b7b4cc3f
>
> Best regards,
> --
> Linus Walleij <linus.walleij@linaro.org>
>
Linus Walleij Nov. 25, 2023, 11:05 p.m. UTC | #2 
On Sat, Nov 25, 2023 at 3:40 AM Kent Gibson <warthog618@gmail.com> wrote:
> On Sat, Nov 25, 2023 at 12:25:47AM +0100, Linus Walleij wrote:

> > -/* gpio_lock prevents conflicts during gpio_desc[] table updates.
> > - * While any GPIO is requested, its gpio_chip is not removable;
> > - * each GPIO's "requested" flag serves as a lock and refcount.
> > - */
>
> Perhaps provide a comment as to what the gpio_lock DOES cover?

Normally yes, but Bartosz just said he is going to replace this spinlock
with a mutex so it's better if he adds it then.

Yours,
Linus Walleij
Kent Gibson Nov. 26, 2023, 12:14 a.m. UTC | #3 
On Sun, Nov 26, 2023 at 12:05:08AM +0100, Linus Walleij wrote:
> On Sat, Nov 25, 2023 at 3:40 AM Kent Gibson <warthog618@gmail.com> wrote:
> > On Sat, Nov 25, 2023 at 12:25:47AM +0100, Linus Walleij wrote:
>
> > > -/* gpio_lock prevents conflicts during gpio_desc[] table updates.
> > > - * While any GPIO is requested, its gpio_chip is not removable;
> > > - * each GPIO's "requested" flag serves as a lock and refcount.
> > > - */
> >
> > Perhaps provide a comment as to what the gpio_lock DOES cover?
>
> Normally yes, but Bartosz just said he is going to replace this spinlock
> with a mutex so it's better if he adds it then.
>

If that is happening soon then leave it to Bart to change both the
comment and lock.

If not, then we now have an undocumented lock.  If the coverage of the
spinlock and proposed mutex are the same why not describe what the lock
covers now?  Then Bart wont have to update the comment.

Cheers,
Kent.
Bartosz Golaszewski Nov. 27, 2023, 7:44 p.m. UTC | #4 
On Sun, Nov 26, 2023 at 1:14 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sun, Nov 26, 2023 at 12:05:08AM +0100, Linus Walleij wrote:
> > On Sat, Nov 25, 2023 at 3:40 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > On Sat, Nov 25, 2023 at 12:25:47AM +0100, Linus Walleij wrote:
> >
> > > > -/* gpio_lock prevents conflicts during gpio_desc[] table updates.
> > > > - * While any GPIO is requested, its gpio_chip is not removable;
> > > > - * each GPIO's "requested" flag serves as a lock and refcount.
> > > > - */
> > >
> > > Perhaps provide a comment as to what the gpio_lock DOES cover?
> >
> > Normally yes, but Bartosz just said he is going to replace this spinlock
> > with a mutex so it's better if he adds it then.
> >
>
> If that is happening soon then leave it to Bart to change both the
> comment and lock.
>
> If not, then we now have an undocumented lock.  If the coverage of the
> spinlock and proposed mutex are the same why not describe what the lock
> covers now?  Then Bart wont have to update the comment.
>
> Cheers,
> Kent.
>

Yeah, I think we should maybe leave some temporary FIXME comment once
the mutex patch is in saying this must go as well but it'll take more
time because the problem is quite tricky.

Bart
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 95d2a7b2ea3e..1c47af866bf6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -86,10 +86,6 @@  static struct bus_type gpio_bus_type = {
  */
 #define FASTPATH_NGPIO CONFIG_GPIOLIB_FASTPATH_LIMIT
 
-/* gpio_lock prevents conflicts during gpio_desc[] table updates.
- * While any GPIO is requested, its gpio_chip is not removable;
- * each GPIO's "requested" flag serves as a lock and refcount.
- */
 DEFINE_SPINLOCK(gpio_lock);
 
 static DEFINE_MUTEX(gpio_lookup_lock);