| Message ID | 20230818190944.22177-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: notify user-space about line state changes triggered by kernel | expand |
On Fri, Aug 18, 2023 at 09:09:44PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Interestingly why you keep submitter and author different... > We currently only emit CHANGED_CONFIG events when the user-space changes > GPIO config. We won't be notified if changes come from in-kernel. Let's > call the notifier chain whenever kernel users change direction or any of > the active-low, debounce or consumer name settings. We don't notify the > user-space about the persistence as the uAPI has no notion of it. ... > - if (!ret) > + if (!ret) { > set_bit(FLAG_IS_OUT, &desc->flags); > + blocking_notifier_call_chain(&desc->gdev->notifier, > + GPIO_V2_LINE_CHANGED_CONFIG, > + desc); > + } > trace_gpio_value(desc_to_gpio(desc), 0, val); > trace_gpio_direction(desc_to_gpio(desc), 0, ret); > return ret; The if (!ret) makes me a bit slower to understand as usual pattern to test for the errors first. That said, perhaps if (ret) goto out_trace_event; set_bit(FLAG_IS_OUT, &desc->flags); blocking_notifier_call_chain(&desc->gdev->notifier, GPIO_V2_LINE_CHANGED_CONFIG, desc); out_trace_event: trace_gpio_value(desc_to_gpio(desc), 0, val); trace_gpio_direction(desc_to_gpio(desc), 0, ret); return ret; ... > + ret = gpiod_set_config(desc, config); > + if (!ret) > + blocking_notifier_call_chain(&desc->gdev->notifier, > + GPIO_V2_LINE_CHANGED_CONFIG, > + desc); > + return ret; Ditto. ret = gpiod_set_config(desc, config); if (ret) return ret; blocking_notifier_call_chain(&desc->gdev->notifier, GPIO_V2_LINE_CHANGED_CONFIG, desc); return 0;
On Mon, Aug 21, 2023 at 12:43 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Aug 18, 2023 at 09:09:44PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Interestingly why you keep submitter and author different... > > > We currently only emit CHANGED_CONFIG events when the user-space changes > > GPIO config. We won't be notified if changes come from in-kernel. Let's > > call the notifier chain whenever kernel users change direction or any of > > the active-low, debounce or consumer name settings. We don't notify the > > user-space about the persistence as the uAPI has no notion of it. > > ... > > > - if (!ret) > > + if (!ret) { > > set_bit(FLAG_IS_OUT, &desc->flags); > > + blocking_notifier_call_chain(&desc->gdev->notifier, > > + GPIO_V2_LINE_CHANGED_CONFIG, > > + desc); > > + } > > trace_gpio_value(desc_to_gpio(desc), 0, val); > > trace_gpio_direction(desc_to_gpio(desc), 0, ret); > > return ret; > > The if (!ret) makes me a bit slower to understand as usual pattern to test > for the errors first. > > That said, perhaps > > if (ret) > goto out_trace_event; > > set_bit(FLAG_IS_OUT, &desc->flags); > blocking_notifier_call_chain(&desc->gdev->notifier, > GPIO_V2_LINE_CHANGED_CONFIG, desc); > > out_trace_event: > trace_gpio_value(desc_to_gpio(desc), 0, val); > trace_gpio_direction(desc_to_gpio(desc), 0, ret); > return ret; > > ... > > > + ret = gpiod_set_config(desc, config); > > + if (!ret) > > + blocking_notifier_call_chain(&desc->gdev->notifier, > > + GPIO_V2_LINE_CHANGED_CONFIG, > > + desc); > > + return ret; > > Ditto. > > ret = gpiod_set_config(desc, config); > if (ret) > return ret; > > blocking_notifier_call_chain(&desc->gdev->notifier, > GPIO_V2_LINE_CHANGED_CONFIG, desc); > return 0; > I agree with the latter but the former introduces an unnecessary goto and looks worse IMO. Bart > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 76e0c38026c3..77e0d0afa2b5 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2421,6 +2421,9 @@ int gpiod_direction_input(struct gpio_desc *desc) } if (ret == 0) { clear_bit(FLAG_IS_OUT, &desc->flags); + blocking_notifier_call_chain(&desc->gdev->notifier, + GPIO_V2_LINE_CHANGED_CONFIG, + desc); ret = gpio_set_bias(desc); } @@ -2466,8 +2469,12 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value) gc->set(gc, gpio_chip_hwgpio(desc), val); } - if (!ret) + if (!ret) { set_bit(FLAG_IS_OUT, &desc->flags); + blocking_notifier_call_chain(&desc->gdev->notifier, + GPIO_V2_LINE_CHANGED_CONFIG, + desc); + } trace_gpio_value(desc_to_gpio(desc), 0, val); trace_gpio_direction(desc_to_gpio(desc), 0, ret); return ret; @@ -2654,9 +2661,15 @@ EXPORT_SYMBOL_GPL(gpiod_set_config); int gpiod_set_debounce(struct gpio_desc *desc, unsigned int debounce) { unsigned long config; + int ret; config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce); - return gpiod_set_config(desc, config); + ret = gpiod_set_config(desc, config); + if (!ret) + blocking_notifier_call_chain(&desc->gdev->notifier, + GPIO_V2_LINE_CHANGED_CONFIG, + desc); + return ret; } EXPORT_SYMBOL_GPL(gpiod_set_debounce); @@ -2705,6 +2718,8 @@ void gpiod_toggle_active_low(struct gpio_desc *desc) { VALIDATE_DESC_VOID(desc); change_bit(FLAG_ACTIVE_LOW, &desc->flags); + blocking_notifier_call_chain(&desc->gdev->notifier, + GPIO_V2_LINE_CHANGED_CONFIG, desc); } EXPORT_SYMBOL_GPL(gpiod_toggle_active_low); @@ -3312,6 +3327,8 @@ int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name) kfree_const(desc->label); desc_set_label(desc, name); + blocking_notifier_call_chain(&desc->gdev->notifier, + GPIO_V2_LINE_CHANGED_CONFIG, desc); return 0; }