| Message ID | 20250303160341.1322640-3-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: Reduce 'gpio' namespace when operate over GPIOd | expand |
On Mon, Mar 03, 2025 at 06:00:33PM +0200, Andy Shevchenko wrote: > In order to reduce the 'gpio' namespace when operate over GPIO descriptor > rename gpio_set_debounce_timeout() to gpiod_do_set_debounce(). To me anything that has '_do_' in their name sounds like an internal static function that gets wrapped by the actual API function(s). For instance it could be int gpio_set_debounce_timeout() { ... gpiod_do_set_debounce() ... However, gpiod_set_debounce_timeout() or gpiod_set_debounce() sounds good to me.
On Tue, Mar 04, 2025 at 11:18:04AM +0200, Mika Westerberg wrote: > On Mon, Mar 03, 2025 at 06:00:33PM +0200, Andy Shevchenko wrote: > > In order to reduce the 'gpio' namespace when operate over GPIO descriptor > > rename gpio_set_debounce_timeout() to gpiod_do_set_debounce(). > > To me anything that has '_do_' in their name sounds like an internal static > function that gets wrapped by the actual API function(s). > > For instance it could be > > int gpio_set_debounce_timeout() > { > ... > gpiod_do_set_debounce() > ... > > However, gpiod_set_debounce_timeout() or gpiod_set_debounce() sounds good > to me. Then please propose the second name for gpiod_set_config_XXX to follow the same pattern. The series unifies naming and reduces the current inconsistency.
On Tue, Mar 04, 2025 at 01:11:57PM +0200, Mika Westerberg wrote: > On Tue, Mar 04, 2025 at 12:59:25PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 04, 2025 at 11:18:04AM +0200, Mika Westerberg wrote: > > > On Mon, Mar 03, 2025 at 06:00:33PM +0200, Andy Shevchenko wrote: > > > > In order to reduce the 'gpio' namespace when operate over GPIO descriptor > > > > rename gpio_set_debounce_timeout() to gpiod_do_set_debounce(). > > > > > > To me anything that has '_do_' in their name sounds like an internal static > > > function that gets wrapped by the actual API function(s). > > > > > > For instance it could be > > > > > > int gpio_set_debounce_timeout() > > > { > > > ... > > > gpiod_do_set_debounce() > > > ... > > > > > > However, gpiod_set_debounce_timeout() or gpiod_set_debounce() sounds good > > > to me. > > > > Then please propose the second name for gpiod_set_config_XXX to follow > > the same pattern. The series unifies naming and reduces the current > > inconsistency. > gpiod_set_config()? The problem is that gpiod_set_debounce() and gpiod_set_config() are _existing_ public APIs. That's why I considered "_do_" fitting the purpose.
On Tue, Mar 04, 2025 at 01:31:35PM +0200, Mika Westerberg wrote: > On Tue, Mar 04, 2025 at 01:16:54PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 04, 2025 at 01:11:57PM +0200, Mika Westerberg wrote: > > > On Tue, Mar 04, 2025 at 12:59:25PM +0200, Andy Shevchenko wrote: > > > > On Tue, Mar 04, 2025 at 11:18:04AM +0200, Mika Westerberg wrote: > > > > > On Mon, Mar 03, 2025 at 06:00:33PM +0200, Andy Shevchenko wrote: > > > > > > In order to reduce the 'gpio' namespace when operate over GPIO descriptor > > > > > > rename gpio_set_debounce_timeout() to gpiod_do_set_debounce(). > > > > > > > > > > To me anything that has '_do_' in their name sounds like an internal static > > > > > function that gets wrapped by the actual API function(s). > > > > > > > > > > For instance it could be > > > > > > > > > > int gpio_set_debounce_timeout() > > > > > { > > > > > ... > > > > > gpiod_do_set_debounce() > > > > > ... > > > > > > > > > > However, gpiod_set_debounce_timeout() or gpiod_set_debounce() sounds good > > > > > to me. > > > > > > > > Then please propose the second name for gpiod_set_config_XXX to follow > > > > the same pattern. The series unifies naming and reduces the current > > > > inconsistency. > > > > > gpiod_set_config()? > > > > The problem is that > > > > gpiod_set_debounce() and gpiod_set_config() are _existing_ public APIs. > > That's why I considered "_do_" fitting the purpose. > > I see. > > Hmm, we have: > > int gpiod_set_debounce(struct gpio_desc *desc, unsigned int debounce) > { > unsigned long config; > > config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce); > return gpiod_set_config(desc, config); > } > > and > > int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int 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; > } > > I wonder if there is an opportunity to consolidate? ;-) Send a patch! I would be glad to see less functions and internal APIs in GPIOLIB.
On Tue, Mar 4, 2025 at 1:00 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Mar 04, 2025 at 01:31:35PM +0200, Mika Westerberg wrote: > > On Tue, Mar 04, 2025 at 01:16:54PM +0200, Andy Shevchenko wrote: > > > On Tue, Mar 04, 2025 at 01:11:57PM +0200, Mika Westerberg wrote: > > > > On Tue, Mar 04, 2025 at 12:59:25PM +0200, Andy Shevchenko wrote: > > > > > On Tue, Mar 04, 2025 at 11:18:04AM +0200, Mika Westerberg wrote: > > > > > > On Mon, Mar 03, 2025 at 06:00:33PM +0200, Andy Shevchenko wrote: > > > > > > > In order to reduce the 'gpio' namespace when operate over GPIO descriptor > > > > > > > rename gpio_set_debounce_timeout() to gpiod_do_set_debounce(). > > > > > > > > > > > > To me anything that has '_do_' in their name sounds like an internal static > > > > > > function that gets wrapped by the actual API function(s). > > > > > > > > > > > > For instance it could be > > > > > > > > > > > > int gpio_set_debounce_timeout() > > > > > > { > > > > > > ... > > > > > > gpiod_do_set_debounce() > > > > > > ... > > > > > > > > > > > > However, gpiod_set_debounce_timeout() or gpiod_set_debounce() sounds good > > > > > > to me. > > > > > > > > > > Then please propose the second name for gpiod_set_config_XXX to follow > > > > > the same pattern. The series unifies naming and reduces the current > > > > > inconsistency. > > > > > > > gpiod_set_config()? > > > > > > The problem is that > > > > > > gpiod_set_debounce() and gpiod_set_config() are _existing_ public APIs. > > > That's why I considered "_do_" fitting the purpose. > > > > I see. > > > > Hmm, we have: > > > > int gpiod_set_debounce(struct gpio_desc *desc, unsigned int debounce) > > { > > unsigned long config; > > > > config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce); > > return gpiod_set_config(desc, config); > > } > > > > and > > > > int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int 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; > > } > > > > I wonder if there is an opportunity to consolidate? ;-) > > Send a patch! I would be glad to see less functions and internal APIs in > GPIOLIB. > I'm definitely in favor of consolidation instead of renaming to gpiod_go_set_debounce(). If anything a better name would be: gpiod_set_debounce_nocheck() to indicate the actual functionality. How about first extending gpio_set_config_with_argument() to take a boolean "optional" argument and removing gpio_set_config_with_argument_optional() altogether? Both are internal to drivers/gpio/ so it would have no effect on consumers. Bart
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 2aa88ace5868..3f442127222d 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -340,7 +340,7 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip, return desc; /* ACPI uses hundredths of milliseconds units */ - ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout * 10); + ret = gpiod_do_set_debounce(desc, agpio->debounce_timeout * 10); if (ret) dev_warn(chip->parent, "Failed to set debounce-timeout for pin 0x%04X, err %d\n", @@ -1098,7 +1098,7 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id, return ret; /* ACPI uses hundredths of milliseconds units */ - ret = gpio_set_debounce_timeout(desc, info.debounce * 10); + ret = gpiod_do_set_debounce(desc, info.debounce * 10); if (ret) return ret; diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index df5b85284788..8980eef6802c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2697,7 +2697,7 @@ static int gpio_set_bias(struct gpio_desc *desc) } /** - * gpio_set_debounce_timeout() - Set debounce timeout + * gpiod_do_set_debounce() - Set debounce timeout * @desc: GPIO descriptor to set the debounce timeout * @debounce: Debounce timeout in microseconds * @@ -2707,7 +2707,7 @@ static int gpio_set_bias(struct gpio_desc *desc) * Returns: * 0 on success, or negative errno on failure. */ -int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce) +int gpiod_do_set_debounce(struct gpio_desc *desc, unsigned int debounce) { int ret; diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index 65db879d1c74..b3ea7b710995 100644 --- a/drivers/gpio/gpiolib.h +++ b/drivers/gpio/gpiolib.h @@ -263,7 +263,7 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer, 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); +int gpiod_do_set_debounce(struct gpio_desc *desc, unsigned int debounce); int gpiod_hog(struct gpio_desc *desc, const char *name, unsigned long lflags, enum gpiod_flags dflags); int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev);
In order to reduce the 'gpio' namespace when operate over GPIO descriptor rename gpio_set_debounce_timeout() to gpiod_do_set_debounce(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-acpi.c | 4 ++-- drivers/gpio/gpiolib.c | 4 ++-- drivers/gpio/gpiolib.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-)