[PATCH 1/2] gpiolib: rename gpio_set_debounce_timeout() for consistency

[PATCH 1/2] gpiolib: rename gpio_set_debounce_timeout() for consistency

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

All other functions that manipulate a struct gpio_desc use the gpiod_
prefix. Follow this convention and rename gpio_set_debounce_timeout() to
gpiod_set_debounce_timeout().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-acpi.c | 5 +++--
 drivers/gpio/gpiolib.c      | 4 ++--
 drivers/gpio/gpiolib.h      | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index fbda452fb4d6..4a390d8f6544 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -341,7 +341,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_set_debounce_timeout(desc, agpio->debounce_timeout * 10);
 	if (ret)
 		dev_warn(chip->parent,
 			 "Failed to set debounce-timeout for pin 0x%04X, err %d\n",
@@ -1087,7 +1087,8 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
 				return ret;
 
 			/* ACPI uses hundredths of milliseconds units */
-			ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
+			ret = gpiod_set_debounce_timeout(desc,
+							 info.debounce * 10);
 			if (ret)
 				return ret;
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index edffa0d2acaa..6fab0f211e67 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2377,7 +2377,7 @@ static int gpio_set_bias(struct gpio_desc *desc)
 }
 
 /**
- * gpio_set_debounce_timeout() - Set debounce timeout
+ * gpiod_set_debounce_timeout() - Set debounce timeout
  * @desc:	GPIO descriptor to set the debounce timeout
  * @debounce:	Debounce timeout in microseconds
  *
@@ -2386,7 +2386,7 @@ static int gpio_set_bias(struct gpio_desc *desc)
  *
  * Returns 0 on success, or negative error code otherwise.
  */
-int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
+int gpiod_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
 {
 	return gpio_set_config_with_argument_optional(desc,
 						      PIN_CONFIG_INPUT_DEBOUNCE,
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 9bff5c2cf720..9ea5b88ad304 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -219,7 +219,7 @@ static inline int gpiod_request_user(struct gpio_desc *desc, const char *label)
 
 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_set_debounce_timeout(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);
-- 
2.39.2

[PATCH 2/2] gpiolib: rename gpio_chip_hwgpio() for consistency

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

All other functions that manipulate a struct gpio_desc use the gpiod_
prefix. Follow this convention and rename gpio_chip_hwgpio() to
gpiod_get_hwgpio().

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-aspeed.c   |  6 ++---
 drivers/gpio/gpiolib-cdev.c  | 12 ++++-----
 drivers/gpio/gpiolib-sysfs.c | 10 ++++----
 drivers/gpio/gpiolib.c       | 48 ++++++++++++++++++------------------
 drivers/gpio/gpiolib.h       |  2 +-
 5 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index da33bbbdacb9..c9cef89e1f65 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -23,7 +23,7 @@
 
 /*
  * These two headers aren't meant to be used by GPIO drivers. We need
- * them in order to access gpio_chip_hwgpio() which we need to implement
+ * them in order to access gpiod_get_hwgpio() which we need to implement
  * the aspeed specific API which allows the coprocessor to request
  * access to some GPIOs and to arbitrate between coprocessor and ARM.
  */
@@ -1013,7 +1013,7 @@ int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc,
 {
 	struct gpio_chip *chip = gpiod_to_chip(desc);
 	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
-	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
+	int rc = 0, bindex, offset = gpiod_get_hwgpio(desc);
 	const struct aspeed_gpio_bank *bank = to_bank(offset);
 	unsigned long flags;
 
@@ -1059,7 +1059,7 @@ int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
 {
 	struct gpio_chip *chip = gpiod_to_chip(desc);
 	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
-	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
+	int rc = 0, bindex, offset = gpiod_get_hwgpio(desc);
 	const struct aspeed_gpio_bank *bank = to_bank(offset);
 	unsigned long flags;
 
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e39d344feb28..5db38d49941f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -698,7 +698,7 @@ static enum hte_return process_hw_ts_thread(void *p)
 	}
 	le.line_seqno = line->line_seqno;
 	le.seqno = (lr->num_lines == 1) ? le.line_seqno : line->req_seqno;
-	le.offset = gpio_chip_hwgpio(line->desc);
+	le.offset = gpiod_get_hwgpio(line->desc);
 
 	linereq_put_event(lr, &le);
 
@@ -815,7 +815,7 @@ static irqreturn_t edge_irq_thread(int irq, void *p)
 	line->line_seqno++;
 	le.line_seqno = line->line_seqno;
 	le.seqno = (lr->num_lines == 1) ? le.line_seqno : line->req_seqno;
-	le.offset = gpio_chip_hwgpio(line->desc);
+	le.offset = gpiod_get_hwgpio(line->desc);
 
 	linereq_put_event(lr, &le);
 
@@ -913,7 +913,7 @@ static void debounce_work_func(struct work_struct *work)
 
 	lr = line->req;
 	le.timestamp_ns = line_event_timestamp(line);
-	le.offset = gpio_chip_hwgpio(line->desc);
+	le.offset = gpiod_get_hwgpio(line->desc);
 #ifdef CONFIG_HTE
 	if (edflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE) {
 		/* discard events except the last one */
@@ -1610,7 +1610,7 @@ static void linereq_show_fdinfo(struct seq_file *out, struct file *file)
 
 	for (i = 0; i < lr->num_lines; i++)
 		seq_printf(out, "gpio-line:\t%d\n",
-			   gpio_chip_hwgpio(lr->lines[i].desc));
+			   gpiod_get_hwgpio(lr->lines[i].desc));
 }
 #endif
 
@@ -2278,7 +2278,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	unsigned int num_attrs = 0;
 
 	memset(info, 0, sizeof(*info));
-	info->offset = gpio_chip_hwgpio(desc);
+	info->offset = gpiod_get_hwgpio(desc);
 
 	/*
 	 * This function takes a mutex so we must check this before taking
@@ -2539,7 +2539,7 @@ static int lineinfo_changed_notify(struct notifier_block *nb,
 	struct gpio_desc *desc = data;
 	int ret;
 
-	if (!test_bit(gpio_chip_hwgpio(desc), cdev->watched_lines))
+	if (!test_bit(gpiod_get_hwgpio(desc), cdev->watched_lines))
 		return NOTIFY_DONE;
 
 	memset(&chg, 0, sizeof(chg));
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 50503a4525eb..ee3a0cb62d20 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -194,7 +194,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	 *        Remove this redundant call (along with the corresponding
 	 *        unlock) when those drivers have been fixed.
 	 */
-	ret = gpiochip_lock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	ret = gpiochip_lock_as_irq(desc->gdev->chip, gpiod_get_hwgpio(desc));
 	if (ret < 0)
 		goto err_put_kn;
 
@@ -208,7 +208,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 	return 0;
 
 err_unlock:
-	gpiochip_unlock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	gpiochip_unlock_as_irq(desc->gdev->chip, gpiod_get_hwgpio(desc));
 err_put_kn:
 	sysfs_put(data->value_kn);
 
@@ -226,7 +226,7 @@ static void gpio_sysfs_free_irq(struct device *dev)
 
 	data->irq_flags = 0;
 	free_irq(data->irq, data);
-	gpiochip_unlock_as_irq(desc->gdev->chip, gpio_chip_hwgpio(desc));
+	gpiochip_unlock_as_irq(desc->gdev->chip, gpiod_get_hwgpio(desc));
 	sysfs_put(data->value_kn);
 }
 
@@ -458,7 +458,7 @@ static ssize_t export_store(const struct class *class,
 		return -EINVAL;
 	}
 	gc = desc->gdev->chip;
-	offset = gpio_chip_hwgpio(desc);
+	offset = gpiod_get_hwgpio(desc);
 	if (!gpiochip_line_is_valid(gc, offset)) {
 		pr_warn("%s: GPIO %ld masked\n", __func__, gpio);
 		return -EINVAL;
@@ -613,7 +613,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	else
 		data->direction_can_change = false;
 
-	offset = gpio_chip_hwgpio(desc);
+	offset = gpiod_get_hwgpio(desc);
 	if (chip->names && chip->names[offset])
 		ioname = chip->names[offset];
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6fab0f211e67..7b380e4b63e3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -237,7 +237,7 @@ int gpiod_get_direction(struct gpio_desc *desc)
 	int ret;
 
 	gc = gpiod_to_chip(desc);
-	offset = gpio_chip_hwgpio(desc);
+	offset = gpiod_get_hwgpio(desc);
 
 	/*
 	 * Open drain emulation using input mode may incorrectly report
@@ -2052,7 +2052,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	if (gc->request) {
 		/* gc->request may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		offset = gpio_chip_hwgpio(desc);
+		offset = gpiod_get_hwgpio(desc);
 		if (gpiochip_line_is_valid(gc, offset))
 			ret = gc->request(gc, offset);
 		else
@@ -2153,7 +2153,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 		if (gc->free) {
 			spin_unlock_irqrestore(&gpio_lock, flags);
 			might_sleep_if(gc->can_sleep);
-			gc->free(gc, gpio_chip_hwgpio(desc));
+			gc->free(gc, gpiod_get_hwgpio(desc));
 			spin_lock_irqsave(&gpio_lock, flags);
 		}
 		kfree_const(desc->label);
@@ -2317,7 +2317,7 @@ static int gpio_set_config_with_argument(struct gpio_desc *desc,
 	unsigned long config;
 
 	config = pinconf_to_config_packed(mode, argument);
-	return gpio_do_set_config(gc, gpio_chip_hwgpio(desc), config);
+	return gpio_do_set_config(gc, gpiod_get_hwgpio(desc), config);
 }
 
 static int gpio_set_config_with_argument_optional(struct gpio_desc *desc,
@@ -2325,7 +2325,7 @@ static int gpio_set_config_with_argument_optional(struct gpio_desc *desc,
 						  u32 argument)
 {
 	struct device *dev = &desc->gdev->dev;
-	int gpio = gpio_chip_hwgpio(desc);
+	int gpio = gpiod_get_hwgpio(desc);
 	int ret;
 
 	ret = gpio_set_config_with_argument(desc, mode, argument);
@@ -2429,9 +2429,9 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	 * assume we are in input mode after this.
 	 */
 	if (gc->direction_input) {
-		ret = gc->direction_input(gc, gpio_chip_hwgpio(desc));
+		ret = gc->direction_input(gc, gpiod_get_hwgpio(desc));
 	} else if (gc->get_direction &&
-		  (gc->get_direction(gc, gpio_chip_hwgpio(desc)) != 1)) {
+		  (gc->get_direction(gc, gpiod_get_hwgpio(desc)) != 1)) {
 		gpiod_warn(desc,
 			   "%s: missing direction_input() operation and line is output\n",
 			   __func__);
@@ -2467,11 +2467,11 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 	}
 
 	if (gc->direction_output) {
-		ret = gc->direction_output(gc, gpio_chip_hwgpio(desc), val);
+		ret = gc->direction_output(gc, gpiod_get_hwgpio(desc), val);
 	} else {
 		/* Check that we are in output mode if we can */
 		if (gc->get_direction &&
-		    gc->get_direction(gc, gpio_chip_hwgpio(desc))) {
+		    gc->get_direction(gc, gpiod_get_hwgpio(desc))) {
 			gpiod_warn(desc,
 				"%s: missing direction_output() operation\n",
 				__func__);
@@ -2481,7 +2481,7 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 		 * If we can't actively set the direction, we are some
 		 * output-only chip, so just drive the output as desired.
 		 */
-		gc->set(gc, gpio_chip_hwgpio(desc), val);
+		gc->set(gc, gpiod_get_hwgpio(desc), val);
 	}
 
 	if (!ret)
@@ -2603,7 +2603,7 @@ int gpiod_enable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 		return -ENOTSUPP;
 	}
 
-	ret = gc->en_hw_timestamp(gc, gpio_chip_hwgpio(desc), flags);
+	ret = gc->en_hw_timestamp(gc, gpiod_get_hwgpio(desc), flags);
 	if (ret)
 		gpiod_warn(desc, "%s: hw ts request failed\n", __func__);
 
@@ -2632,7 +2632,7 @@ int gpiod_disable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 		return -ENOTSUPP;
 	}
 
-	ret = gc->dis_hw_timestamp(gc, gpio_chip_hwgpio(desc), flags);
+	ret = gc->dis_hw_timestamp(gc, gpiod_get_hwgpio(desc), flags);
 	if (ret)
 		gpiod_warn(desc, "%s: hw ts release failed\n", __func__);
 
@@ -2656,7 +2656,7 @@ int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
 	VALIDATE_DESC(desc);
 	gc = desc->gdev->chip;
 
-	return gpio_do_set_config(gc, gpio_chip_hwgpio(desc), config);
+	return gpio_do_set_config(gc, gpiod_get_hwgpio(desc), config);
 }
 EXPORT_SYMBOL_GPL(gpiod_set_config);
 
@@ -2727,7 +2727,7 @@ EXPORT_SYMBOL_GPL(gpiod_toggle_active_low);
 
 static int gpio_chip_get_value(struct gpio_chip *gc, const struct gpio_desc *desc)
 {
-	return gc->get ? gc->get(gc, gpio_chip_hwgpio(desc)) : -EIO;
+	return gc->get ? gc->get(gc, gpiod_get_hwgpio(desc)) : -EIO;
 }
 
 /* I/O calls are only valid after configuration completed; the relevant
@@ -2852,7 +2852,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 		first = i;
 		do {
 			const struct gpio_desc *desc = desc_array[i];
-			int hwgpio = gpio_chip_hwgpio(desc);
+			int hwgpio = gpiod_get_hwgpio(desc);
 
 			__set_bit(hwgpio, mask);
 			i++;
@@ -2874,7 +2874,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 
 		for (j = first; j < i; ) {
 			const struct gpio_desc *desc = desc_array[j];
-			int hwgpio = gpio_chip_hwgpio(desc);
+			int hwgpio = gpiod_get_hwgpio(desc);
 			int value = test_bit(hwgpio, bits);
 
 			if (!raw && test_bit(FLAG_ACTIVE_LOW, &desc->flags))
@@ -3006,7 +3006,7 @@ static void gpio_set_open_drain_value_commit(struct gpio_desc *desc, bool value)
 {
 	int ret = 0;
 	struct gpio_chip *gc = desc->gdev->chip;
-	int offset = gpio_chip_hwgpio(desc);
+	int offset = gpiod_get_hwgpio(desc);
 
 	if (value) {
 		ret = gc->direction_input(gc, offset);
@@ -3031,7 +3031,7 @@ static void gpio_set_open_source_value_commit(struct gpio_desc *desc, bool value
 {
 	int ret = 0;
 	struct gpio_chip *gc = desc->gdev->chip;
-	int offset = gpio_chip_hwgpio(desc);
+	int offset = gpiod_get_hwgpio(desc);
 
 	if (value) {
 		ret = gc->direction_output(gc, offset, 1);
@@ -3053,7 +3053,7 @@ static void gpiod_set_raw_value_commit(struct gpio_desc *desc, bool value)
 
 	gc = desc->gdev->chip;
 	trace_gpio_value(desc_to_gpio(desc), 0, value);
-	gc->set(gc, gpio_chip_hwgpio(desc), value);
+	gc->set(gc, gpiod_get_hwgpio(desc), value);
 }
 
 /*
@@ -3144,7 +3144,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 
 		do {
 			struct gpio_desc *desc = desc_array[i];
-			int hwgpio = gpio_chip_hwgpio(desc);
+			int hwgpio = gpiod_get_hwgpio(desc);
 			int value = test_bit(i, value_bitmap);
 
 			/*
@@ -3355,7 +3355,7 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 		return -EINVAL;
 
 	gc = desc->gdev->chip;
-	offset = gpio_chip_hwgpio(desc);
+	offset = gpiod_get_hwgpio(desc);
 	if (gc->to_irq) {
 		int retirq = gc->to_irq(gc, offset);
 
@@ -4258,7 +4258,7 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 	int ret;
 
 	gc = gpiod_to_chip(desc);
-	hwnum = gpio_chip_hwgpio(desc);
+	hwnum = gpiod_get_hwgpio(desc);
 
 	local_desc = gpiochip_request_own_desc(gc, hwnum, name,
 					       lflags, dflags);
@@ -4338,7 +4338,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 		 * If pin hardware number of array member 0 is also 0, select
 		 * its chip as a candidate for fast bitmap processing path.
 		 */
-		if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
+		if (descs->ndescs == 0 && gpiod_get_hwgpio(desc) == 0) {
 			struct gpio_descs *array;
 
 			bitmap_size = BITS_TO_LONGS(gc->ngpio > count ?
@@ -4383,7 +4383,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 		 * Detect array members which belong to the 'fast' chip
 		 * but their pins are not in hardware order.
 		 */
-		else if (gpio_chip_hwgpio(desc) != descs->ndescs) {
+		else if (gpiod_get_hwgpio(desc) != descs->ndescs) {
 			/*
 			 * Don't use fast path if all array members processed so
 			 * far belong to the same chip as this one but its pin
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 9ea5b88ad304..bddf29699289 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -227,7 +227,7 @@ int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev);
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
  */
-static inline int gpio_chip_hwgpio(const struct gpio_desc *desc)
+static inline int gpiod_get_hwgpio(const struct gpio_desc *desc)
 {
 	return desc - &desc->gdev->descs[0];
 }
-- 
2.39.2

Re: [PATCH 2/2] gpiolib: rename gpio_chip_hwgpio() for consistency

[Andy Shevchenko]

On Mon, Sep 04, 2023 at 09:34:10AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> All other functions that manipulate a struct gpio_desc use the gpiod_
> prefix. Follow this convention and rename gpio_chip_hwgpio() to
> gpiod_get_hwgpio().

Same comment. Also, I don't think it's good idea as it steps on the exported
API's toes. I.o.w. I won't mix those two.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpiolib: rename gpio_chip_hwgpio() for consistency

[Bartosz Golaszewski]

On Mon, Sep 4, 2023 at 11:27 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 04, 2023 at 09:34:10AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > All other functions that manipulate a struct gpio_desc use the gpiod_
> > prefix. Follow this convention and rename gpio_chip_hwgpio() to
> > gpiod_get_hwgpio().
>
> Same comment. Also, I don't think it's good idea as it steps on the exported
> API's toes. I.o.w. I won't mix those two.
>

Even if I agreed with your other comment, gpio_chip_hwgpio() is a
terrible name and if I didn't know, I couldn't tell you what it does
just from looking at the name.

Bart

Re: [PATCH 2/2] gpiolib: rename gpio_chip_hwgpio() for consistency

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 10:37:32AM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 4, 2023 at 11:27 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Sep 04, 2023 at 09:34:10AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > All other functions that manipulate a struct gpio_desc use the gpiod_
> > > prefix. Follow this convention and rename gpio_chip_hwgpio() to
> > > gpiod_get_hwgpio().
> >
> > Same comment. Also, I don't think it's good idea as it steps on the exported
> > API's toes. I.o.w. I won't mix those two.
> 
> Even if I agreed with your other comment, gpio_chip_hwgpio() is a
> terrible name and if I didn't know, I couldn't tell you what it does
> just from looking at the name.

That's can be improved, my previous comments were basically to avoid
mixing prefixes for internal and external APIs, let's say prefix them
similarly, but for internal with space and/or more verbose naming

	gpiod_		gpio_desc_
	gpiochip_	gpio_chip_
	gdev_		gpio_device_

(as an example).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpiolib: rename gpio_chip_hwgpio() for consistency

[Bartosz Golaszewski]

On Tue, Sep 5, 2023 at 12:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 05, 2023 at 10:37:32AM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 4, 2023 at 11:27 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Sep 04, 2023 at 09:34:10AM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > All other functions that manipulate a struct gpio_desc use the gpiod_
> > > > prefix. Follow this convention and rename gpio_chip_hwgpio() to
> > > > gpiod_get_hwgpio().
> > >
> > > Same comment. Also, I don't think it's good idea as it steps on the exported
> > > API's toes. I.o.w. I won't mix those two.
> >
> > Even if I agreed with your other comment, gpio_chip_hwgpio() is a
> > terrible name and if I didn't know, I couldn't tell you what it does
> > just from looking at the name.
>
> That's can be improved, my previous comments were basically to avoid
> mixing prefixes for internal and external APIs, let's say prefix them
> similarly, but for internal with space and/or more verbose naming
>
>         gpiod_          gpio_desc_
>         gpiochip_       gpio_chip_
>         gdev_           gpio_device_
>

There's one more possibility. Have all exported symbols be prefixed
with gpiod in one way or another and the internal symbols just drop
the prefix so it would be like:

gpiod_
gpiochip_
gpio_device_

and

desc_
chip_
device_

Because for internal symbols we already know they refer to gpiolib.

Anyway, I'll drop the patches for now and let's revisit in the future
when the consensus is reached.

Bart

> (as an example).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 2/2] gpiolib: rename gpio_chip_hwgpio() for consistency

[Andy Shevchenko]

On Tue, Sep 05, 2023 at 01:26:34PM +0200, Bartosz Golaszewski wrote:
> On Tue, Sep 5, 2023 at 12:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Sep 05, 2023 at 10:37:32AM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 4, 2023 at 11:27 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Sep 04, 2023 at 09:34:10AM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > All other functions that manipulate a struct gpio_desc use the gpiod_
> > > > > prefix. Follow this convention and rename gpio_chip_hwgpio() to
> > > > > gpiod_get_hwgpio().
> > > >
> > > > Same comment. Also, I don't think it's good idea as it steps on the exported
> > > > API's toes. I.o.w. I won't mix those two.
> > >
> > > Even if I agreed with your other comment, gpio_chip_hwgpio() is a
> > > terrible name and if I didn't know, I couldn't tell you what it does
> > > just from looking at the name.
> >
> > That's can be improved, my previous comments were basically to avoid
> > mixing prefixes for internal and external APIs, let's say prefix them
> > similarly, but for internal with space and/or more verbose naming
> >
> >         gpiod_          gpio_desc_
> >         gpiochip_       gpio_chip_
> >         gdev_           gpio_device_
> 
> There's one more possibility. Have all exported symbols be prefixed
> with gpiod in one way or another and the internal symbols just drop
> the prefix so it would be like:
> 
> gpiod_
> gpiochip_
> gpio_device_
> 
> and
> 
> desc_
> chip_
> device_
> 
> Because for internal symbols we already know they refer to gpiolib.

With the above schema we have two caveats, one is not significant
(as we have desc_to_gpio() and complimentary API). And another one
is device/dev, which is conflicting with global. That's why I still
prefer gpio_desc_ and so on.

> Anyway, I'll drop the patches for now and let's revisit in the future
> when the consensus is reached.

Yes, let's focus on something more important now.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpiolib: rename gpio_chip_hwgpio() for consistency

[Linus Walleij]

On Tue, Sep 5, 2023 at 10:37 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> gpio_chip_hwgpio() is a
> terrible name and if I didn't know, I couldn't tell you what it does
> just from looking at the name.

Probably my fault.

"For the chip containing this desc obtain the corresponding
hardware GPIO number/offset" is what I would put in kerneldoc.
(Which is by the way also horrible.)

Let's rename it to something that says clearly what it does.
(Rusty Russell's API design hierarchy.)

I guess I would just name it something like *_get_chip_hw_offset()
or *_get_chip_hwgpio_offset() but it gets a bit long, maybe it is a bit
talkative but easy to understand. (I stay off the prefix discussion.)

Yours,
Linus Walleij

Re: [PATCH 1/2] gpiolib: rename gpio_set_debounce_timeout() for consistency

[Andy Shevchenko]

On Mon, Sep 04, 2023 at 09:34:09AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> All other functions that manipulate a struct gpio_desc use the gpiod_
> prefix. Follow this convention and rename gpio_set_debounce_timeout() to
> gpiod_set_debounce_timeout().

No, that's not true. This one is inline with the other gpio_set() _internal_
APIs. If renamed, should be done consistently.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpiolib: rename gpio_set_debounce_timeout() for consistency

[Andy Shevchenko]

On Mon, Sep 04, 2023 at 12:24:53PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 04, 2023 at 09:34:09AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > 
> > All other functions that manipulate a struct gpio_desc use the gpiod_
> > prefix. Follow this convention and rename gpio_set_debounce_timeout() to
> > gpiod_set_debounce_timeout().
> 
> No, that's not true. This one is inline with the other gpio_set() _internal_
> APIs. If renamed, should be done consistently.

It's even broader, like for_each_gpio_desc(). So, looking at that one I would
rather use gpio_desc_ prefix / suffix than gpiod in case you are so eager to
rename (Personally I consider this as unneeded churn). Also consider going
thru all _internal_ APIs, like gpiod_not_found(), to be renamed to
gpio_desc_not_found().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpiolib: rename gpio_set_debounce_timeout() for consistency

[Bartosz Golaszewski]

On Mon, Sep 4, 2023 at 11:25 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 04, 2023 at 09:34:09AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > All other functions that manipulate a struct gpio_desc use the gpiod_
> > prefix. Follow this convention and rename gpio_set_debounce_timeout() to
> > gpiod_set_debounce_timeout().
>
> No, that's not true. This one is inline with the other gpio_set() _internal_
> APIs. If renamed, should be done consistently.
>

All the other ones are static to gpiolib.c. With static symbols the
naming convention is a bit more relaxed throughout the kernel. But I
do agree and I will get to them in time. :)

Bart