From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
We currently only notify user-space about line config changes that are
made from user-space. Any kernel config changes are not signalled.
Let's improve the situation by emitting the events closer to the source.
To that end let's call the relevant notifier chain from the functions
setting direction, gpiod_set_config(), gpiod_set_consumer_name() and
gpiod_toggle_active_low(). This covers all the options that we can
inform the user-space about. We ignore events which don't have
corresponding flags exported to user-space on purpose - otherwise the
user would see a config-changed event but the associated line-info would
remain unchanged.
gpiod_direction_output/input() can be called from any context.
Fortunately, we now emit line state events using an atomic notifier
chain, so it's no longer an issue.
Let's also add non-notifying wrappers around the direction setters in
order to not emit superfluous reconfigure events when requesting the
lines as the initial config should be part of the request notification.
Use gpio_do_set_config() instead of gpiod_set_debounce() for configuring
debouncing via hardware from the character device code to avoid multiple
reconfigure events.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/gpio/gpiolib-cdev.c | 21 ++++++-----
drivers/gpio/gpiolib.c | 89 ++++++++++++++++++++++++++++++++++++++-------
drivers/gpio/gpiolib.h | 3 ++
3 files changed, 90 insertions(+), 23 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index cb4fb55e2696..e5b15d96e952 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -195,8 +195,6 @@ static long linehandle_set_config(struct linehandle_state *lh,
if (ret)
return ret;
}
-
- gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
}
return 0;
}
@@ -362,11 +360,11 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
int val = !!handlereq.default_values[i];
- ret = gpiod_direction_output(desc, val);
+ ret = gpiod_direction_output_nonotify(desc, val);
if (ret)
goto out_free_lh;
} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
- ret = gpiod_direction_input(desc);
+ ret = gpiod_direction_input_nonotify(desc);
if (ret)
goto out_free_lh;
}
@@ -922,8 +920,13 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
int ret, level, irq;
char *label;
- /* try hardware */
- ret = gpiod_set_debounce(line->desc, debounce_period_us);
+ /*
+ * Try hardware. Skip gpiod_set_config() to avoid emitting two
+ * CHANGED_CONFIG line state events.
+ */
+ ret = gpio_do_set_config(line->desc,
+ pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE,
+ debounce_period_us));
if (!ret) {
WRITE_ONCE(line->desc->debounce_period_us, debounce_period_us);
return ret;
@@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
}
WRITE_ONCE(line->edflags, edflags);
-
- gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
}
return 0;
}
@@ -1700,11 +1701,11 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
int val = gpio_v2_line_config_output_value(lc, i);
- ret = gpiod_direction_output(desc, val);
+ ret = gpiod_direction_output_nonotify(desc, val);
if (ret)
goto out_free_linereq;
} else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
- ret = gpiod_direction_input(desc);
+ ret = gpiod_direction_input_nonotify(desc);
if (ret)
goto out_free_linereq;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 83e85dbfdeed..ae758ba6dc3d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2564,7 +2564,7 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc);
* rely on gpio_request() having been called beforehand.
*/
-static int gpio_do_set_config(struct gpio_desc *desc, unsigned long config)
+int gpio_do_set_config(struct gpio_desc *desc, unsigned long config)
{
int ret;
@@ -2670,9 +2670,15 @@ static int gpio_set_bias(struct gpio_desc *desc)
*/
int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
{
- return gpio_set_config_with_argument_optional(desc,
- PIN_CONFIG_INPUT_DEBOUNCE,
- debounce);
+ int ret;
+
+ ret = gpio_set_config_with_argument_optional(desc,
+ PIN_CONFIG_INPUT_DEBOUNCE,
+ debounce);
+ if (!ret)
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+ return ret;
}
/**
@@ -2686,6 +2692,18 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
* 0 on success, or negative errno on failure.
*/
int gpiod_direction_input(struct gpio_desc *desc)
+{
+ int ret;
+
+ ret = gpiod_direction_input_nonotify(desc);
+ if (ret == 0)
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gpiod_direction_input);
+
+int gpiod_direction_input_nonotify(struct gpio_desc *desc)
{
int ret = 0;
@@ -2733,7 +2751,6 @@ int gpiod_direction_input(struct gpio_desc *desc)
return ret;
}
-EXPORT_SYMBOL_GPL(gpiod_direction_input);
static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
{
@@ -2795,8 +2812,15 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
*/
int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
{
+ int ret;
+
VALIDATE_DESC(desc);
- return gpiod_direction_output_raw_commit(desc, value);
+
+ ret = gpiod_direction_output_raw_commit(desc, value);
+ if (ret == 0)
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
@@ -2814,6 +2838,18 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
* 0 on success, or negative errno on failure.
*/
int gpiod_direction_output(struct gpio_desc *desc, int value)
+{
+ int ret;
+
+ ret = gpiod_direction_output_nonotify(desc, value);
+ if (ret == 0)
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gpiod_direction_output);
+
+int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
{
unsigned long flags;
int ret;
@@ -2843,7 +2879,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
goto set_output_value;
/* Emulate open drain by not actively driving the line high */
if (value) {
- ret = gpiod_direction_input(desc);
+ ret = gpiod_direction_input_nonotify(desc);
goto set_output_flag;
}
} else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
@@ -2852,7 +2888,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
goto set_output_value;
/* Emulate open source by not actively driving the line low */
if (!value) {
- ret = gpiod_direction_input(desc);
+ ret = gpiod_direction_input_nonotify(desc);
goto set_output_flag;
}
} else {
@@ -2876,7 +2912,6 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
set_bit(FLAG_IS_OUT, &desc->flags);
return ret;
}
-EXPORT_SYMBOL_GPL(gpiod_direction_output);
/**
* gpiod_enable_hw_timestamp_ns - Enable hardware timestamp in nanoseconds.
@@ -2955,9 +2990,30 @@ EXPORT_SYMBOL_GPL(gpiod_disable_hw_timestamp_ns);
*/
int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
{
+ int ret;
+
VALIDATE_DESC(desc);
- return gpio_do_set_config(desc, config);
+ ret = gpio_do_set_config(desc, config);
+ if (!ret) {
+ /* These are the only options we notify the userspace about. */
+ switch (pinconf_to_config_param(config)) {
+ case PIN_CONFIG_BIAS_DISABLE:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_BIAS_PULL_UP:
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
+ case PIN_CONFIG_INPUT_DEBOUNCE:
+ gpiod_line_state_notify(desc,
+ GPIO_V2_LINE_CHANGED_CONFIG);
+ break;
+ default:
+ break;
+ }
+ }
+
+ return ret;
}
EXPORT_SYMBOL_GPL(gpiod_set_config);
@@ -3024,6 +3080,7 @@ void gpiod_toggle_active_low(struct gpio_desc *desc)
{
VALIDATE_DESC_VOID(desc);
change_bit(FLAG_ACTIVE_LOW, &desc->flags);
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
}
EXPORT_SYMBOL_GPL(gpiod_toggle_active_low);
@@ -3668,9 +3725,15 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep);
*/
int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
{
+ int ret;
+
VALIDATE_DESC(desc);
- return desc_set_label(desc, name);
+ ret = desc_set_label(desc, name);
+ if (ret == 0)
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
@@ -4548,10 +4611,10 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
/* Process flags */
if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
- ret = gpiod_direction_output(desc,
+ ret = gpiod_direction_output_nonotify(desc,
!!(dflags & GPIOD_FLAGS_BIT_DIR_VAL));
else
- ret = gpiod_direction_input(desc);
+ ret = gpiod_direction_input_nonotify(desc);
return ret;
}
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index a54be597d21a..83690f72f7e5 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -155,6 +155,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
+int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value);
+int gpiod_direction_input_nonotify(struct gpio_desc *desc);
struct gpio_desc_label {
struct rcu_head rh;
@@ -258,6 +260,7 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
const char *label,
bool platform_lookup_allowed);
+int gpio_do_set_config(struct gpio_desc *desc, unsigned long config);
int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
unsigned long lflags, enum gpiod_flags dflags);
int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);
--
2.43.0On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> }
>
> WRITE_ONCE(line->edflags, edflags);
> -
> - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> }
> return 0;
> }
I still get errors from this when reconfiguring lines with debounce.
You should leave this notify in place and use _nonotify when setting the
direction.
i.e.
@@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
int val = gpio_v2_line_config_output_value(&lc, i);
edge_detector_stop(line);
- ret = gpiod_direction_output(desc, val);
+ ret = gpiod_direction_output_nonotify(desc, val);
if (ret)
return ret;
} else {
- ret = gpiod_direction_input(desc);
+ ret = gpiod_direction_input_nonotify(desc);
if (ret)
return ret;
@@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
}
WRITE_ONCE(line->edflags, edflags);
+
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
}
return 0;
}
Given that, all my current tests are passing.
Cheers,
Kent.
On Thu, Oct 17, 2024 at 2:53 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > }
> >
> > WRITE_ONCE(line->edflags, edflags);
> > -
> > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > }
> > return 0;
> > }
>
> I still get errors from this when reconfiguring lines with debounce.
> You should leave this notify in place and use _nonotify when setting the
> direction.
> i.e.
>
> @@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> int val = gpio_v2_line_config_output_value(&lc, i);
>
> edge_detector_stop(line);
> - ret = gpiod_direction_output(desc, val);
> + ret = gpiod_direction_output_nonotify(desc, val);
> if (ret)
> return ret;
> } else {
> - ret = gpiod_direction_input(desc);
> + ret = gpiod_direction_input_nonotify(desc);
> if (ret)
> return ret;
>
> @@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> }
>
> WRITE_ONCE(line->edflags, edflags);
> +
> + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> }
> return 0;
> }
>
> Given that, all my current tests are passing.
>
That looks good - after all we no longer notify from any place in
gpiolib-cdev.c anymore - but I'd like to learn what's wrong exactly.
Are you getting more events with debounce? Are you not getting any?
Bart
On Thu, Oct 17, 2024 at 04:14:24PM +0200, Bartosz Golaszewski wrote:
> On Thu, Oct 17, 2024 at 2:53 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > }
> > >
> > > WRITE_ONCE(line->edflags, edflags);
> > > -
> > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > }
> > > return 0;
> > > }
> >
> > I still get errors from this when reconfiguring lines with debounce.
> > You should leave this notify in place and use _nonotify when setting the
> > direction.
> > i.e.
> >
> > @@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > int val = gpio_v2_line_config_output_value(&lc, i);
> >
> > edge_detector_stop(line);
> > - ret = gpiod_direction_output(desc, val);
> > + ret = gpiod_direction_output_nonotify(desc, val);
> > if (ret)
> > return ret;
> > } else {
> > - ret = gpiod_direction_input(desc);
> > + ret = gpiod_direction_input_nonotify(desc);
> > if (ret)
> > return ret;
> >
> > @@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > }
> >
> > WRITE_ONCE(line->edflags, edflags);
> > +
> > + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > }
> > return 0;
> > }
> >
> > Given that, all my current tests are passing.
> >
>
> That looks good - after all we no longer notify from any place in
> gpiolib-cdev.c anymore - but I'd like to learn what's wrong exactly.
> Are you getting more events with debounce? Are you not getting any?
>
In linereq_set_config(), the notify comes from the gpiod_direction_input() -
before the edge_detector_setup() is called (not visible in the patch) and that
sets the debounce value in the desc.
So you get an event without the debounce set, or with a stale value.
Keeping the gpiod_line_state_notify() and using the _nonotify()
functions means the notify comes after the debounce has been set.
Cheers,
Kent.
On Thu, Oct 17, 2024 at 4:20 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Oct 17, 2024 at 04:14:24PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Oct 17, 2024 at 2:53 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > }
> > > >
> > > > WRITE_ONCE(line->edflags, edflags);
> > > > -
> > > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > }
> > > > return 0;
> > > > }
> > >
> > > I still get errors from this when reconfiguring lines with debounce.
> > > You should leave this notify in place and use _nonotify when setting the
> > > direction.
> > > i.e.
> > >
> > > @@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > int val = gpio_v2_line_config_output_value(&lc, i);
> > >
> > > edge_detector_stop(line);
> > > - ret = gpiod_direction_output(desc, val);
> > > + ret = gpiod_direction_output_nonotify(desc, val);
> > > if (ret)
> > > return ret;
> > > } else {
> > > - ret = gpiod_direction_input(desc);
> > > + ret = gpiod_direction_input_nonotify(desc);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > }
> > >
> > > WRITE_ONCE(line->edflags, edflags);
> > > +
> > > + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > }
> > > return 0;
> > > }
> > >
> > > Given that, all my current tests are passing.
> > >
> >
> > That looks good - after all we no longer notify from any place in
> > gpiolib-cdev.c anymore - but I'd like to learn what's wrong exactly.
> > Are you getting more events with debounce? Are you not getting any?
> >
>
> In linereq_set_config(), the notify comes from the gpiod_direction_input() -
> before the edge_detector_setup() is called (not visible in the patch) and that
> sets the debounce value in the desc.
> So you get an event without the debounce set, or with a stale value.
> Keeping the gpiod_line_state_notify() and using the _nonotify()
> functions means the notify comes after the debounce has been set.
>
I see. I guess I should do the same both for linehandle_set_config()
and linereq_set_config()?
Bart
On Thu, Oct 17, 2024 at 04:59:46PM +0200, Bartosz Golaszewski wrote:
> On Thu, Oct 17, 2024 at 4:20 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Oct 17, 2024 at 04:14:24PM +0200, Bartosz Golaszewski wrote:
> > > On Thu, Oct 17, 2024 at 2:53 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > }
> > > > >
> > > > > WRITE_ONCE(line->edflags, edflags);
> > > > > -
> > > > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > }
> > > > > return 0;
> > > > > }
> > > >
> > > > I still get errors from this when reconfiguring lines with debounce.
> > > > You should leave this notify in place and use _nonotify when setting the
> > > > direction.
> > > > i.e.
> > > >
> > > > @@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > int val = gpio_v2_line_config_output_value(&lc, i);
> > > >
> > > > edge_detector_stop(line);
> > > > - ret = gpiod_direction_output(desc, val);
> > > > + ret = gpiod_direction_output_nonotify(desc, val);
> > > > if (ret)
> > > > return ret;
> > > > } else {
> > > > - ret = gpiod_direction_input(desc);
> > > > + ret = gpiod_direction_input_nonotify(desc);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > @@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > }
> > > >
> > > > WRITE_ONCE(line->edflags, edflags);
> > > > +
> > > > + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > }
> > > > return 0;
> > > > }
> > > >
> > > > Given that, all my current tests are passing.
> > > >
> > >
> > > That looks good - after all we no longer notify from any place in
> > > gpiolib-cdev.c anymore - but I'd like to learn what's wrong exactly.
> > > Are you getting more events with debounce? Are you not getting any?
> > >
> >
> > In linereq_set_config(), the notify comes from the gpiod_direction_input() -
> > before the edge_detector_setup() is called (not visible in the patch) and that
> > sets the debounce value in the desc.
> > So you get an event without the debounce set, or with a stale value.
> > Keeping the gpiod_line_state_notify() and using the _nonotify()
> > functions means the notify comes after the debounce has been set.
> >
>
> I see. I guess I should do the same both for linehandle_set_config()
> and linereq_set_config()?
>
linehandles don't support debounce, so it's good as is.
Cheers,
Kent.
On Thu, Oct 17, 2024 at 5:02 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Oct 17, 2024 at 04:59:46PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Oct 17, 2024 at 4:20 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Oct 17, 2024 at 04:14:24PM +0200, Bartosz Golaszewski wrote:
> > > > On Thu, Oct 17, 2024 at 2:53 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > >
> > > > > > @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > > }
> > > > > >
> > > > > > WRITE_ONCE(line->edflags, edflags);
> > > > > > -
> > > > > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > > }
> > > > > > return 0;
> > > > > > }
> > > > >
> > > > > I still get errors from this when reconfiguring lines with debounce.
> > > > > You should leave this notify in place and use _nonotify when setting the
> > > > > direction.
> > > > > i.e.
> > > > >
> > > > > @@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > int val = gpio_v2_line_config_output_value(&lc, i);
> > > > >
> > > > > edge_detector_stop(line);
> > > > > - ret = gpiod_direction_output(desc, val);
> > > > > + ret = gpiod_direction_output_nonotify(desc, val);
> > > > > if (ret)
> > > > > return ret;
> > > > > } else {
> > > > > - ret = gpiod_direction_input(desc);
> > > > > + ret = gpiod_direction_input_nonotify(desc);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > @@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > }
> > > > >
> > > > > WRITE_ONCE(line->edflags, edflags);
> > > > > +
> > > > > + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > }
> > > > > return 0;
> > > > > }
> > > > >
> > > > > Given that, all my current tests are passing.
> > > > >
> > > >
> > > > That looks good - after all we no longer notify from any place in
> > > > gpiolib-cdev.c anymore - but I'd like to learn what's wrong exactly.
> > > > Are you getting more events with debounce? Are you not getting any?
> > > >
> > >
> > > In linereq_set_config(), the notify comes from the gpiod_direction_input() -
> > > before the edge_detector_setup() is called (not visible in the patch) and that
> > > sets the debounce value in the desc.
> > > So you get an event without the debounce set, or with a stale value.
> > > Keeping the gpiod_line_state_notify() and using the _nonotify()
> > > functions means the notify comes after the debounce has been set.
> > >
> >
> > I see. I guess I should do the same both for linehandle_set_config()
> > and linereq_set_config()?
> >
>
> linehandles don't support debounce, so it's good as is.
>
Right, but I'm wondering if it isn't better for consistency.
Otherwise, someone may ask themselves why there's no event being
emitted if they're not familiar with the gpiod_direction_*()
internals.
Bart
On Thu, Oct 17, 2024 at 05:04:13PM +0200, Bartosz Golaszewski wrote:
> On Thu, Oct 17, 2024 at 5:02 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Oct 17, 2024 at 04:59:46PM +0200, Bartosz Golaszewski wrote:
> > > On Thu, Oct 17, 2024 at 4:20 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Thu, Oct 17, 2024 at 04:14:24PM +0200, Bartosz Golaszewski wrote:
> > > > > On Thu, Oct 17, 2024 at 2:53 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> > > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > > >
> > > > > > > @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > > > }
> > > > > > >
> > > > > > > WRITE_ONCE(line->edflags, edflags);
> > > > > > > -
> > > > > > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > > > }
> > > > > > > return 0;
> > > > > > > }
> > > > > >
> > > > > > I still get errors from this when reconfiguring lines with debounce.
> > > > > > You should leave this notify in place and use _nonotify when setting the
> > > > > > direction.
> > > > > > i.e.
> > > > > >
> > > > > > @@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > > int val = gpio_v2_line_config_output_value(&lc, i);
> > > > > >
> > > > > > edge_detector_stop(line);
> > > > > > - ret = gpiod_direction_output(desc, val);
> > > > > > + ret = gpiod_direction_output_nonotify(desc, val);
> > > > > > if (ret)
> > > > > > return ret;
> > > > > > } else {
> > > > > > - ret = gpiod_direction_input(desc);
> > > > > > + ret = gpiod_direction_input_nonotify(desc);
> > > > > > if (ret)
> > > > > > return ret;
> > > > > >
> > > > > > @@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > > > }
> > > > > >
> > > > > > WRITE_ONCE(line->edflags, edflags);
> > > > > > +
> > > > > > + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > > > }
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > Given that, all my current tests are passing.
> > > > > >
> > > > >
> > > > > That looks good - after all we no longer notify from any place in
> > > > > gpiolib-cdev.c anymore - but I'd like to learn what's wrong exactly.
> > > > > Are you getting more events with debounce? Are you not getting any?
> > > > >
> > > >
> > > > In linereq_set_config(), the notify comes from the gpiod_direction_input() -
> > > > before the edge_detector_setup() is called (not visible in the patch) and that
> > > > sets the debounce value in the desc.
> > > > So you get an event without the debounce set, or with a stale value.
> > > > Keeping the gpiod_line_state_notify() and using the _nonotify()
> > > > functions means the notify comes after the debounce has been set.
> > > >
> > >
> > > I see. I guess I should do the same both for linehandle_set_config()
> > > and linereq_set_config()?
> > >
> >
> > linehandles don't support debounce, so it's good as is.
> >
>
> Right, but I'm wondering if it isn't better for consistency.
> Otherwise, someone may ask themselves why there's no event being
> emitted if they're not familiar with the gpiod_direction_*()
> internals.
>
I prefer the more succinct form myself, and if you are working in the
kernel you should be capable of determining what the functions do, but
whichever works for you.
Cheers,
Kent.
On Thu, Oct 17, 2024 at 10:20:53PM +0800, Kent Gibson wrote:
> On Thu, Oct 17, 2024 at 04:14:24PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Oct 17, 2024 at 2:53 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Oct 17, 2024 at 10:14:16AM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > @@ -1447,8 +1450,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > > }
> > > >
> > > > WRITE_ONCE(line->edflags, edflags);
> > > > -
> > > > - gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > > }
> > > > return 0;
> > > > }
> > >
> > > I still get errors from this when reconfiguring lines with debounce.
> > > You should leave this notify in place and use _nonotify when setting the
> > > direction.
> > > i.e.
> > >
> > > @@ -1436,11 +1432,11 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > int val = gpio_v2_line_config_output_value(&lc, i);
> > >
> > > edge_detector_stop(line);
> > > - ret = gpiod_direction_output(desc, val);
> > > + ret = gpiod_direction_output_nonotify(desc, val);
> > > if (ret)
> > > return ret;
> > > } else {
> > > - ret = gpiod_direction_input(desc);
> > > + ret = gpiod_direction_input_nonotify(desc);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -1450,6 +1446,8 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
> > > }
> > >
> > > WRITE_ONCE(line->edflags, edflags);
> > > +
> > > + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > }
> > > return 0;
> > > }
> > >
> > > Given that, all my current tests are passing.
> > >
> >
> > That looks good - after all we no longer notify from any place in
> > gpiolib-cdev.c anymore - but I'd like to learn what's wrong exactly.
> > Are you getting more events with debounce? Are you not getting any?
> >
>
> In linereq_set_config(), the notify comes from the gpiod_direction_input() -
> before the edge_detector_setup() is called (not visible in the patch) and that
> sets the debounce value in the desc.
That should be edge_detector_update().
> So you get an event without the debounce set, or with a stale value.
> Keeping the gpiod_line_state_notify() and using the _nonotify()
> functions means the notify comes after the debounce has been set.
>
> Cheers,
> Kent.
>
© 2016 - 2024 Red Hat, Inc.