From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Instead of emitting the line state change event on request in three
different places, just do it once, closer to the source: in
gpiod_request_commit().
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/gpio/gpiolib-cdev.c | 6 ------
drivers/gpio/gpiolib.c | 4 ++--
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index b0050250ac3a..f614a981253d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -372,8 +372,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
goto out_free_lh;
}
- gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-
dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
offset);
}
@@ -1842,8 +1840,6 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
lr->lines[i].edflags = edflags;
- gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-
dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
offset);
}
@@ -2234,8 +2230,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
if (ret)
goto out_free_le;
- gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-
irq = gpiod_to_irq(desc);
if (irq <= 0) {
ret = -ENODEV;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 97346b746ef5..190ece4d6850 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2345,6 +2345,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
if (ret)
goto out_clear_bit;
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
+
return 0;
out_clear_bit:
@@ -4365,8 +4367,6 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
return ERR_PTR(ret);
}
- gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
-
return desc;
}
--
2.43.0On Fri, Oct 04, 2024 at 04:43:25PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Instead of emitting the line state change event on request in three > different places, just do it once, closer to the source: in > gpiod_request_commit(). > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpiolib-cdev.c | 6 ------ > drivers/gpio/gpiolib.c | 4 ++-- > 2 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index b0050250ac3a..f614a981253d 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -372,8 +372,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > goto out_free_lh; > } > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED); > - > dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > offset); This moves the notify to before the desc->flags have been set. So the notified will now see the flags as previously set, not what they have been requested as. That might be acceptible if you subsequently issue GPIO_V2_LINE_CHANGED_CONFIG when the flags are set, but that is not done here and you explicitly don't notify from here in patch 5 when you add notifying to gpiod_direction_output() etc. Cheers, Kent.
On Sat, Oct 5, 2024 at 5:46 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Oct 04, 2024 at 04:43:25PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Instead of emitting the line state change event on request in three > > different places, just do it once, closer to the source: in > > gpiod_request_commit(). > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/gpio/gpiolib-cdev.c | 6 ------ > > drivers/gpio/gpiolib.c | 4 ++-- > > 2 files changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > > index b0050250ac3a..f614a981253d 100644 > > --- a/drivers/gpio/gpiolib-cdev.c > > +++ b/drivers/gpio/gpiolib-cdev.c > > @@ -372,8 +372,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > goto out_free_lh; > > } > > > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED); > > - > > dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > offset); > > This moves the notify to before the desc->flags have been set. > So the notified will now see the flags as previously set, not what they > have been requested as. > Ah, I got fooled by the libgpiod tests passing. I guess we should cover that first in tests-kernel-uapi.c. > That might be acceptible if you subsequently issue GPIO_V2_LINE_CHANGED_CONFIG > when the flags are set, but that is not done here and you explicitly don't > notify from here in patch 5 when you add notifying to gpiod_direction_output() > etc. > IMO it doesn't make sense to always emit REQUESTED and CONFIG_CHANGED events together. The initial config should be part of the request event. I'll get back to the drawing board. Bart
On Sat, Oct 05, 2024 at 11:34:26AM +0200, Bartosz Golaszewski wrote: > On Sat, Oct 5, 2024 at 5:46 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Fri, Oct 04, 2024 at 04:43:25PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Instead of emitting the line state change event on request in three > > > different places, just do it once, closer to the source: in > > > gpiod_request_commit(). > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > > drivers/gpio/gpiolib-cdev.c | 6 ------ > > > drivers/gpio/gpiolib.c | 4 ++-- > > > 2 files changed, 2 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > > > index b0050250ac3a..f614a981253d 100644 > > > --- a/drivers/gpio/gpiolib-cdev.c > > > +++ b/drivers/gpio/gpiolib-cdev.c > > > @@ -372,8 +372,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) > > > goto out_free_lh; > > > } > > > > > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED); > > > - > > > dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > > offset); > > > > This moves the notify to before the desc->flags have been set. > > So the notified will now see the flags as previously set, not what they > > have been requested as. > > > > Ah, I got fooled by the libgpiod tests passing. I guess we should > cover that first in tests-kernel-uapi.c. > > > That might be acceptible if you subsequently issue GPIO_V2_LINE_CHANGED_CONFIG > > when the flags are set, but that is not done here and you explicitly don't > > notify from here in patch 5 when you add notifying to gpiod_direction_output() > > etc. > > > > IMO it doesn't make sense to always emit REQUESTED and CONFIG_CHANGED > events together. The initial config should be part of the request > event. I'll get back to the drawing board. > Oh, I agree - that "might" is doing a lot of heavy lifting - there should only be the one event. Cheers, Kent.
© 2016 - 2024 Red Hat, Inc.