Re: [PATCH v2 1/1] gpiolib: Fix debug messaging in gpiod_find_and_request()

[Ferry Toth <fntoth@gmail.com>]

Hi

Op 20-03-2024 om 17:58 schreef Andy Shevchenko:
> When consolidating GPIO lookups in ACPI code, the debug messaging
> had been reworked that the user may see
> 
>    [   13.401147] (NULL device *): using ACPI '\_SB.LEDS.led-0' for '(null)' GPIO lookup
>    [   13.401378] gpio gpiochip0: Persistence not supported for GPIO 40
>    [   13.401402] gpio-40 (?): no flags found for (null)
> 
> instead of
> 
>    [   14.182962] gpio gpiochip0: Persistence not supported for GPIO 40
>    [   14.182994] gpio-40 (?): no flags found for gpios
> 
> The '(null)' parts are less informative and likely scare the users.
> Replace them by '(default)' which can point out to the default connection
> IDs, such as 'gpios'.
> 
> While at it, amend other places where con_id is used in the messages.
> 
> Reported-by: Ferry Toth <ftoth@exalondelft.nl>
> Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups")
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.c > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: completele reworked solution of
> 20231019173457.2445119-1-andriy.shevchenko@linux.intel.com
>   drivers/gpio/gpiolib.c | 32 ++++++++++++++++++--------------
>   1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e2e583b40207..7d26e5de0b44 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2401,6 +2401,11 @@ char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
>   }
>   EXPORT_SYMBOL_GPL(gpiochip_dup_line_label);
>   
> +static inline const char *function_name_or_default(const char *con_id)
> +{
> +	return con_id ?: "(default)";
> +}
> +
>   /**
>    * gpiochip_request_own_desc - Allow GPIO chip to request its own descriptor
>    * @gc: GPIO chip
> @@ -2429,10 +2434,11 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
>   					    enum gpiod_flags dflags)
>   {
>   	struct gpio_desc *desc = gpiochip_get_desc(gc, hwnum);
> +	const char *name = function_name_or_default(label);
>   	int ret;
>   
>   	if (IS_ERR(desc)) {
> -		chip_err(gc, "failed to get GPIO descriptor\n");
> +		chip_err(gc, "failed to get GPIO %s descriptor\n", name);
>   		return desc;
>   	}
>   
> @@ -2442,8 +2448,8 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
>   
>   	ret = gpiod_configure_flags(desc, label, lflags, dflags);
>   	if (ret) {
> -		chip_err(gc, "setup of own GPIO %s failed\n", label);
>   		gpiod_free_commit(desc);
> +		chip_err(gc, "setup of own GPIO %s failed\n", name);
>   		return ERR_PTR(ret);
>   	}
>   
> @@ -4157,19 +4163,17 @@ static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
>   					      enum gpiod_flags *flags,
>   					      unsigned long *lookupflags)
>   {
> +	const char *name = function_name_or_default(con_id);
>   	struct gpio_desc *desc = ERR_PTR(-ENOENT);
>   
>   	if (is_of_node(fwnode)) {
> -		dev_dbg(consumer, "using DT '%pfw' for '%s' GPIO lookup\n",
> -			fwnode, con_id);
> +		dev_dbg(consumer, "using DT '%pfw' for '%s' GPIO lookup\n", fwnode, name);
>   		desc = of_find_gpio(to_of_node(fwnode), con_id, idx, lookupflags);
>   	} else if (is_acpi_node(fwnode)) {
> -		dev_dbg(consumer, "using ACPI '%pfw' for '%s' GPIO lookup\n",
> -			fwnode, con_id);
> +		dev_dbg(consumer, "using ACPI '%pfw' for '%s' GPIO lookup\n", fwnode, name);
>   		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
>   	} else if (is_software_node(fwnode)) {
> -		dev_dbg(consumer, "using swnode '%pfw' for '%s' GPIO lookup\n",
> -			fwnode, con_id);
> +		dev_dbg(consumer, "using swnode '%pfw' for '%s' GPIO lookup\n", fwnode, name);
>   		desc = swnode_find_gpio(fwnode, con_id, idx, lookupflags);
>   	}
>   
> @@ -4185,6 +4189,7 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
>   					 bool platform_lookup_allowed)
>   {
>   	unsigned long lookupflags = GPIO_LOOKUP_FLAGS_DEFAULT;
> +	const char *name = function_name_or_default(con_id);
>   	/*
>   	 * scoped_guard() is implemented as a for loop, meaning static
>   	 * analyzers will complain about these two not being initialized.
> @@ -4207,8 +4212,7 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
>   		}
>   
>   		if (IS_ERR(desc)) {
> -			dev_dbg(consumer, "No GPIO consumer %s found\n",
> -				con_id);
> +			dev_dbg(consumer, "No GPIO consumer %s found\n", name);
>   			return desc;
>   		}
>   
> @@ -4230,15 +4234,14 @@ struct gpio_desc *gpiod_find_and_request(struct device *consumer,
>   		 *
>   		 * FIXME: Make this more sane and safe.
>   		 */
> -		dev_info(consumer,
> -			 "nonexclusive access to GPIO for %s\n", con_id);
> +		dev_info(consumer, "nonexclusive access to GPIO for %s\n", name);
>   		return desc;
>   	}
>   
>   	ret = gpiod_configure_flags(desc, con_id, lookupflags, flags);
>   	if (ret < 0) {
> -		dev_dbg(consumer, "setup of GPIO %s failed\n", con_id);
>   		gpiod_put(desc);
> +		dev_dbg(consumer, "setup of GPIO %s failed\n", name);
>   		return ERR_PTR(ret);
>   	}
>   
> @@ -4354,6 +4357,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional);
>   int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>   		unsigned long lflags, enum gpiod_flags dflags)
>   {
> +	const char *name = function_name_or_default(con_id);
>   	int ret;
>   
>   	if (lflags & GPIO_ACTIVE_LOW)
> @@ -4397,7 +4401,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>   
>   	/* No particular flag request, return here... */
>   	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
> -		gpiod_dbg(desc, "no flags found for %s\n", con_id);
> +		gpiod_dbg(desc, "no flags found for GPIO %s\n", name);
>   		return 0;
>   	}
>   

Tested-by: Ferry Toth <ftoth@exalondelft.nl> on Intel Edison (mrfld)