[PATCH v2 0/5] gpiolib: clean up and potential bug fix

[PATCH v2 0/5] gpiolib: clean up and potential bug fix

[Andy Shevchenko]

The patch series mostly addressing messaging format and fix for one potential
bug. It depends on recent Alex's patch [1] and Mika's patchset [2].

Compile tested and tested on Medfield with SFI.

Changes since v1:
 - applied Acked and Reviewed tags
 - instead of patch 4/4 create patch 4/5 and 5/5 that don't change
   functionality, and indentation things moved to patch 1/5

[1] http://www.spinics.net/lists/kernel/msg1645907.html
[2] http://www.spinics.net/lists/linux-acpi/msg47572.html

Andy Shevchenko (5):
  gpiolib: unify pr_* messages format
  gpiolib: introduce chip_* to print with chip->label prefix
  gpiolib: convert gpiod_lookup description to kernel-doc
  gpiolib: update commentary at gpiod_get_index()
  gpiolib: uniform messages in gpiod_find()

 drivers/gpio/gpiolib.c      | 121 ++++++++++++++++++++++++--------------------
 include/linux/gpio/driver.h |  26 ++++------
 2 files changed, 75 insertions(+), 72 deletions(-)

-- 
1.8.4.4

[PATCH v2 1/5] gpiolib: unify pr_* messages format

[Andy Shevchenko]

This patch includes the following amendments:
 1) use "?" as a label when the last one is not defined in gpiod_*;
 2) whenever it's possible gpiod_* are used;
 3) print a function name, if it's already used in other messages.

Additionally it fixes an indentation in few places.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 74 +++++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f7af309..7cf3f84 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -86,36 +86,36 @@ static int gpiod_request(struct gpio_desc *desc, const char *label);
 static void gpiod_free(struct gpio_desc *desc);
 
 #ifdef CONFIG_DEBUG_FS
-#define gpiod_emerg(desc, fmt, ...)			                \
-	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label, \
+#define gpiod_emerg(desc, fmt, ...)					       \
+	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
                  ##__VA_ARGS__)
-#define gpiod_crit(desc, fmt, ...)			                \
-	pr_crit("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label,  \
+#define gpiod_crit(desc, fmt, ...)					       \
+	pr_crit("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
                  ##__VA_ARGS__)
-#define gpiod_err(desc, fmt, ...)				        \
-	pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label,   \
+#define gpiod_err(desc, fmt, ...)					       \
+	pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",  \
                  ##__VA_ARGS__)
-#define gpiod_warn(desc, fmt, ...)				        \
-	pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label,  \
+#define gpiod_warn(desc, fmt, ...)					       \
+	pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
                  ##__VA_ARGS__)
-#define gpiod_info(desc, fmt, ...)				        \
-	pr_info("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label,  \
+#define gpiod_info(desc, fmt, ...)					       \
+	pr_info("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
                 ##__VA_ARGS__)
-#define gpiod_dbg(desc, fmt, ...)				   \
-	pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label, \
+#define gpiod_dbg(desc, fmt, ...)					       \
+	pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
                  ##__VA_ARGS__)
 #else
-#define gpiod_emerg(desc, fmt, ...)			           \
+#define gpiod_emerg(desc, fmt, ...)					\
 	pr_emerg("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_crit(desc, fmt, ...)			           \
+#define gpiod_crit(desc, fmt, ...)					\
 	pr_crit("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_err(desc, fmt, ...)				   \
+#define gpiod_err(desc, fmt, ...)					\
 	pr_err("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_warn(desc, fmt, ...)				   \
+#define gpiod_warn(desc, fmt, ...)					\
 	pr_warn("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_info(desc, fmt, ...)				   \
+#define gpiod_info(desc, fmt, ...)					\
 	pr_info("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_dbg(desc, fmt, ...)				   \
+#define gpiod_dbg(desc, fmt, ...)					\
 	pr_debug("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
 #endif
 
@@ -188,7 +188,8 @@ static int gpio_ensure_requested(struct gpio_desc *desc)
 	if (WARN(test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0,
 			"autorequest GPIO-%d\n", gpio)) {
 		if (!try_module_get(chip->owner)) {
-			pr_err("GPIO-%d: module can't be gotten \n", gpio);
+			gpiod_err(desc, "%s: module can't be gotten\n",
+					__func__);
 			clear_bit(FLAG_REQUESTED, &desc->flags);
 			/* lose */
 			return -EIO;
@@ -809,8 +810,8 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
 	     test_bit(FLAG_EXPORT, &desc->flags)) {
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		pr_debug("%s: gpio %d unavailable (requested=%d, exported=%d)\n",
-				__func__, desc_to_gpio(desc),
+		gpiod_dbg(desc, "%s: unavailable (requested=%d, exported=%d)\n",
+				__func__,
 				test_bit(FLAG_REQUESTED, &desc->flags),
 				test_bit(FLAG_EXPORT, &desc->flags));
 		status = -EPERM;
@@ -858,8 +859,7 @@ fail_unregister_device:
 	device_unregister(dev);
 fail_unlock:
 	mutex_unlock(&sysfs_lock);
-	pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-		 status);
+	gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiod_export);
@@ -907,8 +907,7 @@ int gpiod_export_link(struct device *dev, const char *name,
 	mutex_unlock(&sysfs_lock);
 
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 
 	return status;
 }
@@ -952,8 +951,7 @@ unlock:
 	mutex_unlock(&sysfs_lock);
 
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 
 	return status;
 }
@@ -995,8 +993,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 	}
 
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 }
 EXPORT_SYMBOL_GPL(gpiod_unexport);
 
@@ -1223,7 +1220,7 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (status)
 		goto fail;
 
-	pr_debug("gpiochip_add: registered GPIOs %d to %d on device: %s\n",
+	pr_debug("%s: registered GPIOs %d to %d on device: %s\n", __func__,
 		chip->base, chip->base + chip->ngpio - 1,
 		chip->label ? : "generic");
 
@@ -1233,7 +1230,7 @@ unlock:
 	spin_unlock_irqrestore(&gpio_lock, flags);
 fail:
 	/* failures here can mean systems won't boot... */
-	pr_err("gpiochip_add: gpios %d..%d (%s) failed to register\n",
+	pr_err("%s: GPIOs %d..%d (%s) failed to register\n", __func__,
 		chip->base, chip->base + chip->ngpio - 1,
 		chip->label ? : "generic");
 	return status;
@@ -1502,8 +1499,7 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 	}
 done:
 	if (status)
-		pr_debug("_gpio_request: gpio-%d (%s) status %d\n",
-			 desc_to_gpio(desc), label ? : "?", status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
@@ -1704,7 +1700,7 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	if (!chip->get || !chip->direction_input) {
 		gpiod_warn(desc,
 			"%s: missing get() or direction_input() operations\n",
-			 __func__);
+			__func__);
 		return -EIO;
 	}
 
@@ -1724,7 +1720,8 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	if (status) {
 		status = chip->request(chip, offset);
 		if (status < 0) {
-			gpiod_dbg(desc, "chip request fail, %d\n", status);
+			gpiod_dbg(desc, "%s: chip request fail, %d\n",
+					__func__, status);
 			/* and it's not available to anyone else ...
 			 * gpio_request() is the fully clean solution.
 			 */
@@ -1742,7 +1739,7 @@ lose:
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	if (status)
-		gpiod_dbg(desc, "%s status %d\n", __func__, status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_input);
@@ -1809,7 +1806,8 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	if (status) {
 		status = chip->request(chip, offset);
 		if (status < 0) {
-			gpiod_dbg(desc, "chip request fail, %d\n", status);
+			gpiod_dbg(desc, "%s: chip request fail, %d\n",
+					__func__, status);
 			/* and it's not available to anyone else ...
 			 * gpio_request() is the fully clean solution.
 			 */
@@ -2450,8 +2448,10 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	 */
 	if (!desc || IS_ERR(desc)) {
 		struct gpio_desc *pdesc;
+
 		dev_dbg(dev, "using lookup tables for GPIO lookup");
 		pdesc = gpiod_find(dev, con_id, idx, &flags);
+
 		/* If used as fallback, do not replace the previous error */
 		if (!IS_ERR(pdesc) || !desc)
 			desc = pdesc;
-- 
1.8.4.4

[PATCH v2 2/5] gpiolib: introduce chip_* to print with chip->label prefix

[Andy Shevchenko]

In several places we are printing messages with prefix based on chip->label.
Introduced macros help us to do this easier and in uniform way.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7cf3f84..70e560c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -85,6 +85,8 @@ static DEFINE_IDR(dirent_idr);
 static int gpiod_request(struct gpio_desc *desc, const char *label);
 static void gpiod_free(struct gpio_desc *desc);
 
+/* With descriptor prefix */
+
 #ifdef CONFIG_DEBUG_FS
 #define gpiod_emerg(desc, fmt, ...)					       \
 	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
@@ -119,6 +121,21 @@ static void gpiod_free(struct gpio_desc *desc);
 	pr_debug("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
 #endif
 
+/* With chip prefix */
+
+#define chip_emerg(chip, fmt, ...)					\
+	pr_emerg("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_crit(chip, fmt, ...)					\
+	pr_crit("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_err(chip, fmt, ...)					\
+	pr_err("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_warn(chip, fmt, ...)					\
+	pr_warn("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_info(chip, fmt, ...)					\
+	pr_info("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_dbg(chip, fmt, ...)					\
+	pr_debug("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
 {
 #ifdef CONFIG_DEBUG_FS
@@ -1032,8 +1049,7 @@ static int gpiochip_export(struct gpio_chip *chip)
 			chip->desc[gpio++].chip = NULL;
 		spin_unlock_irqrestore(&gpio_lock, flags);
 
-		pr_debug("%s: chip %s status %d\n", __func__,
-				chip->label, status);
+		chip_dbg(chip, "%s: status %d\n", __func__, status);
 	}
 
 	return status;
@@ -1056,8 +1072,7 @@ static void gpiochip_unexport(struct gpio_chip *chip)
 	mutex_unlock(&sysfs_lock);
 
 	if (status)
-		pr_debug("%s: chip %s status %d\n", __func__,
-				chip->label, status);
+		chip_dbg(chip, "%s: status %d\n", __func__, status);
 }
 
 static int __init gpiolib_sysfs_init(void)
@@ -1339,8 +1354,7 @@ int gpiochip_add_pingroup_range(struct gpio_chip *chip,
 
 	pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
 	if (!pin_range) {
-		pr_err("%s: GPIO chip: failed to allocate pin ranges\n",
-				chip->label);
+		chip_err(chip, "failed to allocate pin ranges\n");
 		return -ENOMEM;
 	}
 
@@ -1361,9 +1375,8 @@ int gpiochip_add_pingroup_range(struct gpio_chip *chip,
 
 	pinctrl_add_gpio_range(pctldev, &pin_range->range);
 
-	pr_debug("GPIO chip %s: created GPIO range %d->%d ==> %s PINGRP %s\n",
-		 chip->label, gpio_offset,
-		 gpio_offset + pin_range->range.npins - 1,
+	chip_dbg(chip, "created GPIO range %d->%d ==> %s PINGRP %s\n",
+		 gpio_offset, gpio_offset + pin_range->range.npins - 1,
 		 pinctrl_dev_get_devname(pctldev), pin_group);
 
 	list_add_tail(&pin_range->node, &chip->pin_ranges);
@@ -1390,8 +1403,7 @@ int gpiochip_add_pin_range(struct gpio_chip *chip, const char *pinctl_name,
 
 	pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
 	if (!pin_range) {
-		pr_err("%s: GPIO chip: failed to allocate pin ranges\n",
-				chip->label);
+		chip_err(chip, "failed to allocate pin ranges\n");
 		return -ENOMEM;
 	}
 
@@ -1406,13 +1418,12 @@ int gpiochip_add_pin_range(struct gpio_chip *chip, const char *pinctl_name,
 			&pin_range->range);
 	if (IS_ERR(pin_range->pctldev)) {
 		ret = PTR_ERR(pin_range->pctldev);
-		pr_err("%s: GPIO chip: could not create pin range\n",
-		       chip->label);
+		chip_err(chip, "could not create pin range\n");
 		kfree(pin_range);
 		return ret;
 	}
-	pr_debug("GPIO chip %s: created GPIO range %d->%d ==> %s PIN %d->%d\n",
-		 chip->label, gpio_offset, gpio_offset + npins - 1,
+	chip_dbg(chip, "created GPIO range %d->%d ==> %s PIN %d->%d\n",
+		 gpio_offset, gpio_offset + npins - 1,
 		 pinctl_name,
 		 pin_offset, pin_offset + npins - 1);
 
-- 
1.8.4.4

[PATCH v2 3/5] gpiolib: convert gpiod_lookup description to kernel-doc

[Andy Shevchenko]

The patch moves description of the fields to the top of struct definition and
converts them to the kernel-doc format.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Alexandre Courbot <acourbot@nvidia.com>
---
 include/linux/gpio/driver.h | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 7193f43..64f2c38 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -136,29 +136,21 @@ enum gpio_lookup_flags {
 };
 
 /**
- * Lookup table for associating GPIOs to specific devices and functions using
- * platform data.
+ * struct gpiod_lookup - lookup table
+ * @chip_label: name of the chip the GPIO belongs to
+ * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
+ * @con_id: name of the GPIO from the device's point of view
+ * @idx: index of the GPIO in case several GPIOs share the same name
+ * @flags: mask of GPIO_* values
+ *
+ * gpiod_lookup is a lookup table for associating GPIOs to specific devices and
+ * functions using platform data.
  */
 struct gpiod_lookup {
-	/*
-	 * name of the chip the GPIO belongs to
-	 */
 	const char *chip_label;
-	/*
-	 * hardware number (i.e. relative to the chip) of the GPIO
-	 */
 	u16 chip_hwnum;
-	/*
-	 * name of the GPIO from the device's point of view
-	 */
 	const char *con_id;
-	/*
-	 * index of the GPIO in case several GPIOs share the same name
-	 */
 	unsigned int idx;
-	/*
-	 * mask of GPIO_* values
-	 */
 	enum gpio_lookup_flags flags;
 };
 
-- 
1.8.4.4

[PATCH v2 4/5] gpiolib: update commentary at gpiod_get_index()

[Andy Shevchenko]

THe patch just accents that @dev could be NULL.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 70e560c..013e5a5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2425,7 +2425,7 @@ EXPORT_SYMBOL_GPL(gpiod_get);
 
 /**
  * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
- * @dev:	GPIO consumer
+ * @dev:	GPIO consumer, can be NULL for system-global GPIOs
  * @con_id:	function within the GPIO consumer
  * @idx:	index of the GPIO to obtain in the consumer
  *
-- 
1.8.4.4

[PATCH v2 5/5] gpiolib: uniform messages in gpiod_find()

[Andy Shevchenko]

This patch uniforms messages in gpiod_find() to follow what chip_* do.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 013e5a5..cbd7b34 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2391,13 +2391,13 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 		chip = find_chip_by_name(p->chip_label);
 
 		if (!chip) {
-			dev_warn(dev, "cannot find GPIO chip %s\n",
+			dev_warn(dev, "%s: GPIO chip: cannot find\n",
 				 p->chip_label);
 			continue;
 		}
 
 		if (chip->ngpio <= p->chip_hwnum) {
-			dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
+			dev_warn(dev, "%s: GPIO chip: has %d GPIOs\n",
 				 chip->label, chip->ngpio);
 			continue;
 		}
-- 
1.8.4.4

Re: [PATCH v2 0/5] gpiolib: clean up and potential bug fix

[Mika Westerberg]

On Wed, Dec 04, 2013 at 03:42:55PM +0200, Andy Shevchenko wrote:
> The patch series mostly addressing messaging format and fix for one potential
> bug. It depends on recent Alex's patch [1] and Mika's patchset [2].
> 
> Compile tested and tested on Medfield with SFI.
> 
> Changes since v1:
>  - applied Acked and Reviewed tags
>  - instead of patch 4/4 create patch 4/5 and 5/5 that don't change
>    functionality, and indentation things moved to patch 1/5
> 
> [1] http://www.spinics.net/lists/kernel/msg1645907.html
> [2] http://www.spinics.net/lists/linux-acpi/msg47572.html
> 
> Andy Shevchenko (5):
>   gpiolib: unify pr_* messages format
>   gpiolib: introduce chip_* to print with chip->label prefix
>   gpiolib: convert gpiod_lookup description to kernel-doc
>   gpiolib: update commentary at gpiod_get_index()
>   gpiolib: uniform messages in gpiod_find()

For the series,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 4/5] gpiolib: update commentary at gpiod_get_index()

[Alex Courbot]

On 12/04/2013 10:42 PM, Andy Shevchenko wrote:
> THe patch just accents that @dev could be NULL.

s/THe/This

Maybe "commentary by" can also be replaced by "description for" or 
"inline documentation of"?

>
> There is no functional change.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/gpio/gpiolib.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 70e560c..013e5a5 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2425,7 +2425,7 @@ EXPORT_SYMBOL_GPL(gpiod_get);
>
>   /**
>    * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> - * @dev:	GPIO consumer
> + * @dev:	GPIO consumer, can be NULL for system-global GPIOs

Looks ok, but could you also perform the same change for gpiod_get() 
that is just a little bit above since it applies to it as well?

Re: [PATCH v2 5/5] gpiolib: uniform messages in gpiod_find()

[Alex Courbot]

On 12/04/2013 10:43 PM, Andy Shevchenko wrote:
> This patch uniforms messages in gpiod_find() to follow what chip_* do.
>
> There is no functional change.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/gpio/gpiolib.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 013e5a5..cbd7b34 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2391,13 +2391,13 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
>   		chip = find_chip_by_name(p->chip_label);
>
>   		if (!chip) {
> -			dev_warn(dev, "cannot find GPIO chip %s\n",
> +			dev_warn(dev, "%s: GPIO chip: cannot find\n",
>   				 p->chip_label);

Changing the format of the message makes it look like the chip has 
already been resolved while this is not the case yet.

>   			continue;
>   		}
>
>   		if (chip->ngpio <= p->chip_hwnum) {
> -			dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
> +			dev_warn(dev, "%s: GPIO chip: has %d GPIOs\n",
>   				 chip->label, chip->ngpio);
>   			continue;
>   		}

The message is going to look something like:

foo.0: gpiochip.0: GPIO chip: has X GPIOs

... which seems kind of confusing.

All in all I'm not convinced this patch is necessary.

Re: [PATCH v2 2/5] gpiolib: introduce chip_* to print with chip->label prefix

[Alex Courbot]

On 12/04/2013 10:42 PM, Andy Shevchenko wrote:
> In several places we are printing messages with prefix based on chip->label.
> Introduced macros help us to do this easier and in uniform way.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>   drivers/gpio/gpiolib.c | 41 ++++++++++++++++++++++++++---------------
>   1 file changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7cf3f84..70e560c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -85,6 +85,8 @@ static DEFINE_IDR(dirent_idr);
>   static int gpiod_request(struct gpio_desc *desc, const char *label);
>   static void gpiod_free(struct gpio_desc *desc);
>
> +/* With descriptor prefix */
> +
>   #ifdef CONFIG_DEBUG_FS
>   #define gpiod_emerg(desc, fmt, ...)					       \
>   	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
> @@ -119,6 +121,21 @@ static void gpiod_free(struct gpio_desc *desc);
>   	pr_debug("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
>   #endif
>
> +/* With chip prefix */
> +
> +#define chip_emerg(chip, fmt, ...)					\
> +	pr_emerg("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> +#define chip_crit(chip, fmt, ...)					\
> +	pr_crit("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> +#define chip_err(chip, fmt, ...)					\
> +	pr_err("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> +#define chip_warn(chip, fmt, ...)					\
> +	pr_warn("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> +#define chip_info(chip, fmt, ...)					\
> +	pr_info("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> +#define chip_dbg(chip, fmt, ...)					\
> +	pr_debug("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)

One last comment on this: how about using "GPIO chip %s: " for message 
prefix instead? (less ':' and less characters overall).

Re: [PATCH v2 2/5] gpiolib: introduce chip_* to print with chip->label prefix

[Andy Shevchenko]

On Thu, 2013-12-05 at 11:22 +0900, Alex Courbot wrote:
> On 12/04/2013 10:42 PM, Andy Shevchenko wrote:
> > In several places we are printing messages with prefix based on chip->label.
> > Introduced macros help us to do this easier and in uniform way.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> > ---
> >   drivers/gpio/gpiolib.c | 41 ++++++++++++++++++++++++++---------------
> >   1 file changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 7cf3f84..70e560c 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -85,6 +85,8 @@ static DEFINE_IDR(dirent_idr);
> >   static int gpiod_request(struct gpio_desc *desc, const char *label);
> >   static void gpiod_free(struct gpio_desc *desc);
> >
> > +/* With descriptor prefix */
> > +
> >   #ifdef CONFIG_DEBUG_FS
> >   #define gpiod_emerg(desc, fmt, ...)					       \
> >   	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
> > @@ -119,6 +121,21 @@ static void gpiod_free(struct gpio_desc *desc);
> >   	pr_debug("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
> >   #endif
> >
> > +/* With chip prefix */
> > +
> > +#define chip_emerg(chip, fmt, ...)					\
> > +	pr_emerg("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> > +#define chip_crit(chip, fmt, ...)					\
> > +	pr_crit("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> > +#define chip_err(chip, fmt, ...)					\
> > +	pr_err("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> > +#define chip_warn(chip, fmt, ...)					\
> > +	pr_warn("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> > +#define chip_info(chip, fmt, ...)					\
> > +	pr_info("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> > +#define chip_dbg(chip, fmt, ...)					\
> > +	pr_debug("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
> 
> One last comment on this: how about using "GPIO chip %s: " for message 
> prefix instead? (less ':' and less characters overall).

Agree. Will change this one.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 4/5] gpiolib: update commentary at gpiod_get_index()

[Andy Shevchenko]

On Thu, 2013-12-05 at 11:10 +0900, Alex Courbot wrote:
> On 12/04/2013 10:42 PM, Andy Shevchenko wrote:
> > THe patch just accents that @dev could be NULL.
> 
> s/THe/This

Will fix.

> 
> Maybe "commentary by" can also be replaced by "description for" or 
> "inline documentation of"?

Okay.

> 
> >
> > There is no functional change.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/gpio/gpiolib.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 70e560c..013e5a5 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2425,7 +2425,7 @@ EXPORT_SYMBOL_GPL(gpiod_get);
> >
> >   /**
> >    * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> > - * @dev:	GPIO consumer
> > + * @dev:	GPIO consumer, can be NULL for system-global GPIOs
> 
> Looks ok, but could you also perform the same change for gpiod_get() 
> that is just a little bit above since it applies to it as well?

Hmm... I copied that line from description of gpiod_get().

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 5/5] gpiolib: uniform messages in gpiod_find()

[Andy Shevchenko]

On Thu, 2013-12-05 at 11:19 +0900, Alex Courbot wrote:
> On 12/04/2013 10:43 PM, Andy Shevchenko wrote:
> > This patch uniforms messages in gpiod_find() to follow what chip_* do.
> >
> > There is no functional change.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/gpio/gpiolib.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 013e5a5..cbd7b34 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2391,13 +2391,13 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
> >   		chip = find_chip_by_name(p->chip_label);
> >
> >   		if (!chip) {
> > -			dev_warn(dev, "cannot find GPIO chip %s\n",
> > +			dev_warn(dev, "%s: GPIO chip: cannot find\n",
> >   				 p->chip_label);
> 
> Changing the format of the message makes it look like the chip has 
> already been resolved while this is not the case yet.
> 
> >   			continue;
> >   		}
> >
> >   		if (chip->ngpio <= p->chip_hwnum) {
> > -			dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
> > +			dev_warn(dev, "%s: GPIO chip: has %d GPIOs\n",
> >   				 chip->label, chip->ngpio);
> >   			continue;
> >   		}
> 
> The message is going to look something like:
> 
> foo.0: gpiochip.0: GPIO chip: has X GPIOs
> 
> ... which seems kind of confusing.
> 
> All in all I'm not convinced this patch is necessary.

Okay, will drop it away.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy