All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Device property improvements, add %pfw format specifier
@ 2019-03-22 15:29 Sakari Ailus
  2019-03-22 15:29 ` [PATCH 1/5] device property: Add functions for accessing node's parents Sakari Ailus
                   ` (4 more replies)
  0 siblings, 5 replies; 25+ messages in thread
From: Sakari Ailus @ 2019-03-22 15:29 UTC (permalink / raw)
  To: Petr Mladek, linux-kernel, rafael; +Cc: Andy Shevchenko, linux-acpi, devicetree

Hi all,

This set adds functionality into the device property API (counting a
node's parents as well as obtaining its name) in order to support printing
fwnode names using a new conversion specifier "%pfw". The names that are
produced are equivalent to its OF counterpart "%pOF" on OF systems for the
two supported modifiers ("f" and "P").

Printing a node's name is something that's been available on OF for a long
time and if something is converted to device property API (such as the
V4L2 fwnode framework) it always got removed of a nice feature that was
sometimes essential in debugging. With this set, that no longer is the
case.

This set depends on a cleanup set fully releasing the now-deprecated %pf
for other use:

<URL:https://lore.kernel.org/lkml/20190322135350.2btpno7vspvewxvk@paasikivi.fi.intel.com/T/#t>

I was a bit hesitant on how much of this should be covered by the fwnode
ops and how much would be just better to reside in lib/vsprintf.c, but
opted for putting it to the fwnode interface. This keeps it clean and
still rather lean on the fwnode ops side while making it possible to add
other firmware types (although that is unlikely I guess).

Sakari Ailus (5):
  device property: Add functions for accessing node's parents
  device property: Add fwnode_get_name for returning the name of a node
  device property: Add a function to obtain a node's prefix
  lib/vsprintf: Make use of fwnode API to obtain node names and
    separators
  lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

 Documentation/core-api/printk-formats.rst | 24 +++++++++
 drivers/acpi/property.c                   | 46 ++++++++++++++++
 drivers/base/property.c                   | 86 ++++++++++++++++++++++++++---
 drivers/of/property.c                     | 16 ++++++
 include/linux/fwnode.h                    |  4 ++
 include/linux/property.h                  |  5 ++
 lib/vsprintf.c                            | 89 +++++++++++++++++++++++--------
 scripts/checkpatch.pl                     |  2 +-
 8 files changed, 242 insertions(+), 30 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 1/5] device property: Add functions for accessing node's parents
  2019-03-22 15:29 [PATCH 0/5] Device property improvements, add %pfw format specifier Sakari Ailus
@ 2019-03-22 15:29 ` Sakari Ailus
  2019-03-22 15:29 ` [PATCH 2/5] device property: Add fwnode_get_name for returning the name of a node Sakari Ailus
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2019-03-22 15:29 UTC (permalink / raw)
  To: Petr Mladek, linux-kernel, rafael; +Cc: Andy Shevchenko, linux-acpi, devicetree

Add two convenience functions for accessing node's parents:

fwnode_count_parents() returns the number of parent nodes a given node
has. fwnode_get_nth_parent() returns node's parent at a given distance
from the node itself.

Also reorder fwnode_get_parent() in property.c according to the same order
as in property.h.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/base/property.c  | 61 ++++++++++++++++++++++++++++++++++++++++++------
 include/linux/property.h |  3 +++
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 8b91ab380d14..3f5ff6b23ae4 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -533,6 +533,19 @@ int device_add_properties(struct device *dev,
 EXPORT_SYMBOL_GPL(device_add_properties);
 
 /**
+ * fwnode_get_parent - Return parent firwmare node
+ * @fwnode: Firmware whose parent is retrieved
+ *
+ * Return parent firmware node of the given node if possible or %NULL if no
+ * parent was available.
+ */
+struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode)
+{
+	return fwnode_call_ptr_op(fwnode, get_parent);
+}
+EXPORT_SYMBOL_GPL(fwnode_get_parent);
+
+/**
  * fwnode_get_next_parent - Iterate to the node's parent
  * @fwnode: Firmware whose parent is retrieved
  *
@@ -554,17 +567,51 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode)
 EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
 
 /**
- * fwnode_get_parent - Return parent firwmare node
- * @fwnode: Firmware whose parent is retrieved
+ * fwnode_count_parents - Return the number of parents a node has
  *
- * Return parent firmware node of the given node if possible or %NULL if no
- * parent was available.
+ * @fwnode: The node the parents of which are to be counted
+ *
+ * Returns the number of parents a node has.
  */
-struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode)
+unsigned int fwnode_count_parents(struct fwnode_handle *fwnode)
 {
-	return fwnode_call_ptr_op(fwnode, get_parent);
+	unsigned int count;
+
+	fwnode_handle_get(fwnode);
+
+	for (count = 0; fwnode; count++)
+		fwnode = fwnode_get_next_parent(fwnode);
+
+	return count - 1;
 }
-EXPORT_SYMBOL_GPL(fwnode_get_parent);
+EXPORT_SYMBOL_GPL(fwnode_count_parents);
+
+/**
+ * fwnode_get_nth_parent - Return an nth parent of a node
+ *
+ * @fwnode: The node the parent of which is requested
+ * @depth: Distance of the parent from the node
+ *
+ * Returns the nth parent of a node. If @depth is 0, the functionality is
+ * equivalent to fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent()
+ * and so on.
+ *
+ * The caller is responsible for calling fwnode_handle_put() for the returned
+ * node.
+ */
+struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
+					    unsigned int depth)
+{
+	unsigned int i;
+
+	fwnode_handle_get(fwnode);
+
+	for (i = 0; i < depth && fwnode; i++)
+		fwnode = fwnode_get_next_parent(fwnode);
+
+	return fwnode;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
 
 /**
  * fwnode_get_next_child_node - Return the next child node handle for a node
diff --git a/include/linux/property.h b/include/linux/property.h
index 65d3420dd5d1..1164ac974ee8 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -81,6 +81,9 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
+unsigned int fwnode_count_parents(struct fwnode_handle *fwn);
+struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
+					    unsigned int depth);
 struct fwnode_handle *fwnode_get_next_child_node(
 	const struct fwnode_handle *fwnode, struct fwnode_handle *child);
 struct fwnode_handle *fwnode_get_next_available_child_node(
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/5] device property: Add fwnode_get_name for returning the name of a node
  2019-03-22 15:29 [PATCH 0/5] Device property improvements, add %pfw format specifier Sakari Ailus
  2019-03-22 15:29 ` [PATCH 1/5] device property: Add functions for accessing node's parents Sakari Ailus
@ 2019-03-22 15:29 ` Sakari Ailus
  2019-03-24 17:21   ` Randy Dunlap
  2019-03-22 15:29 ` [PATCH 3/5] device property: Add a function to obtain a node's prefix Sakari Ailus
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2019-03-22 15:29 UTC (permalink / raw)
  To: Petr Mladek, linux-kernel, rafael; +Cc: Andy Shevchenko, linux-acpi, devicetree

The fwnode framework did not have means to obtain the name of a node. Add
that now, in form of the fwnode_get_name() function and a corresponding
get_name fwnode op. OF and ACPI support is included.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c  | 24 ++++++++++++++++++++++++
 drivers/base/property.c  | 12 ++++++++++++
 drivers/of/property.c    |  6 ++++++
 include/linux/fwnode.h   |  2 ++
 include/linux/property.h |  1 +
 5 files changed, 45 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 77abe0ec4043..8c9a4c02cde2 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1291,6 +1291,29 @@ acpi_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
 						  args_count, args);
 }
 
+static const char *acpi_fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	const struct acpi_device *adev;
+	struct fwnode_handle *parent;
+
+	parent = fwnode_get_parent(fwnode);
+	/* Is this the root node? */
+	if (!parent)
+		return "\\";
+
+	fwnode_handle_put(parent);
+
+	if (is_acpi_data_node(fwnode)) {
+		const struct acpi_data_node *dn = to_acpi_data_node(fwnode);
+
+		return dn->name;
+	}
+
+	adev = to_acpi_device_node(fwnode);
+
+	return adev ? acpi_device_bid(adev) : NULL;
+}
+
 static struct fwnode_handle *
 acpi_fwnode_get_parent(struct fwnode_handle *fwnode)
 {
@@ -1331,6 +1354,7 @@ acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 		.get_parent = acpi_node_get_parent,			\
 		.get_next_child_node = acpi_get_next_subnode,		\
 		.get_named_child_node = acpi_fwnode_get_named_child_node, \
+		.get_name = acpi_fwnode_get_name,			\
 		.get_reference_args = acpi_fwnode_get_reference_args,	\
 		.graph_get_next_endpoint =				\
 			acpi_graph_get_next_endpoint,			\
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 3f5ff6b23ae4..7ee3786bcde3 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -533,6 +533,18 @@ int device_add_properties(struct device *dev,
 EXPORT_SYMBOL_GPL(device_add_properties);
 
 /**
+ * fwnode_get_name - Return the name of a node
+ *
+ * @fwnode - The firmware node
+ *
+ * Returns a pointer to the node name.
+ */
+const char *fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	return fwnode_call_ptr_op(fwnode, get_name);
+}
+
+/**
  * fwnode_get_parent - Return parent firwmare node
  * @fwnode: Firmware whose parent is retrieved
  *
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 8631efa1daa1..f0a7b78f2d9f 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -872,6 +872,11 @@ of_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 		of_property_count_strings(node, propname);
 }
 
+static const char *of_fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	return kbasename(to_of_node(fwnode)->full_name);
+}
+
 static struct fwnode_handle *
 of_fwnode_get_parent(const struct fwnode_handle *fwnode)
 {
@@ -993,6 +998,7 @@ const struct fwnode_operations of_fwnode_ops = {
 	.property_present = of_fwnode_property_present,
 	.property_read_int_array = of_fwnode_property_read_int_array,
 	.property_read_string_array = of_fwnode_property_read_string_array,
+	.get_name = of_fwnode_get_name,
 	.get_parent = of_fwnode_get_parent,
 	.get_next_child_node = of_fwnode_get_next_child_node,
 	.get_named_child_node = of_fwnode_get_named_child_node,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index faebf0ca0686..85b7fa4dc727 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -59,6 +59,7 @@ struct fwnode_reference_args {
  *				 otherwise.
  * @property_read_string_array: Read an array of string properties. Return zero
  *				on success, a negative error code otherwise.
+ * @get_name: Return the name of an fwnode.
  * @get_parent: Return the parent of an fwnode.
  * @get_next_child_node: Return the next child node in an iteration.
  * @get_named_child_node: Return a child node with a given name.
@@ -85,6 +86,7 @@ struct fwnode_operations {
 	(*property_read_string_array)(const struct fwnode_handle *fwnode_handle,
 				      const char *propname, const char **val,
 				      size_t nval);
+	const char *(*get_name)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *
 	(*get_next_child_node)(const struct fwnode_handle *fwnode,
diff --git a/include/linux/property.h b/include/linux/property.h
index 1164ac974ee8..2f5df900e310 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -78,6 +78,7 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 				       unsigned int nargs, unsigned int index,
 				       struct fwnode_reference_args *args);
 
+const char *fwnode_get_name(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 3/5] device property: Add a function to obtain a node's prefix
  2019-03-22 15:29 [PATCH 0/5] Device property improvements, add %pfw format specifier Sakari Ailus
  2019-03-22 15:29 ` [PATCH 1/5] device property: Add functions for accessing node's parents Sakari Ailus
  2019-03-22 15:29 ` [PATCH 2/5] device property: Add fwnode_get_name for returning the name of a node Sakari Ailus
@ 2019-03-22 15:29 ` Sakari Ailus
  2019-03-22 15:29 ` [PATCH 4/5] lib/vsprintf: Make use of fwnode API to obtain node names and separators Sakari Ailus
  2019-03-22 15:29 ` [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names Sakari Ailus
  4 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2019-03-22 15:29 UTC (permalink / raw)
  To: Petr Mladek, linux-kernel, rafael; +Cc: Andy Shevchenko, linux-acpi, devicetree

The prefix is used for printing purpose before a node, and it also works
as a separator between two nodes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c  | 22 ++++++++++++++++++++++
 drivers/base/property.c  | 13 +++++++++++++
 drivers/of/property.c    | 10 ++++++++++
 include/linux/fwnode.h   |  2 ++
 include/linux/property.h |  1 +
 5 files changed, 48 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 8c9a4c02cde2..c167fd748f86 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1314,6 +1314,27 @@ static const char *acpi_fwnode_get_name(const struct fwnode_handle *fwnode)
 	return adev ? acpi_device_bid(adev) : NULL;
 }
 
+static const char *
+acpi_fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *parent;
+
+	parent = fwnode_get_parent(fwnode);
+	/* Root node. */
+	if (!parent)
+		return "";
+
+	parent = fwnode_get_next_parent(parent);
+	/* Second node from the root; no prefix here either. */
+	if (!parent)
+		return "";
+
+	fwnode_handle_put(parent);
+
+	/* ACPI device or data node. */
+	return ".";
+}
+
 static struct fwnode_handle *
 acpi_fwnode_get_parent(struct fwnode_handle *fwnode)
 {
@@ -1355,6 +1376,7 @@ acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 		.get_next_child_node = acpi_get_next_subnode,		\
 		.get_named_child_node = acpi_fwnode_get_named_child_node, \
 		.get_name = acpi_fwnode_get_name,			\
+		.get_name_prefix = acpi_fwnode_get_name_prefix,		\
 		.get_reference_args = acpi_fwnode_get_reference_args,	\
 		.graph_get_next_endpoint =				\
 			acpi_graph_get_next_endpoint,			\
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 7ee3786bcde3..68b74e53864b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -545,6 +545,19 @@ const char *fwnode_get_name(const struct fwnode_handle *fwnode)
 }
 
 /**
+ * fwnode_get_name_prefix - Return the prefix of node for printing purposes
+ *
+ * @fwnode: The firmware node
+ *
+ * Returns the prefix of a node, intended to be printed right before the node.
+ * The prefix works also as a separator between the nodes.
+ */
+const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
+{
+	return fwnode_call_ptr_op(fwnode, get_name_prefix);
+}
+
+/**
  * fwnode_get_parent - Return parent firwmare node
  * @fwnode: Firmware whose parent is retrieved
  *
diff --git a/drivers/of/property.c b/drivers/of/property.c
index f0a7b78f2d9f..8fed3b142b41 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -877,6 +877,15 @@ static const char *of_fwnode_get_name(const struct fwnode_handle *fwnode)
 	return kbasename(to_of_node(fwnode)->full_name);
 }
 
+static const char *of_fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
+{
+	/* Root needs no prefix here (its name is "/"). */
+	if (!to_of_node(fwnode)->parent)
+		return "";
+
+	return "/";
+}
+
 static struct fwnode_handle *
 of_fwnode_get_parent(const struct fwnode_handle *fwnode)
 {
@@ -999,6 +1008,7 @@ const struct fwnode_operations of_fwnode_ops = {
 	.property_read_int_array = of_fwnode_property_read_int_array,
 	.property_read_string_array = of_fwnode_property_read_string_array,
 	.get_name = of_fwnode_get_name,
+	.get_name_prefix = of_fwnode_get_name_prefix,
 	.get_parent = of_fwnode_get_parent,
 	.get_next_child_node = of_fwnode_get_next_child_node,
 	.get_named_child_node = of_fwnode_get_named_child_node,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 85b7fa4dc727..592ac6936e14 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -60,6 +60,7 @@ struct fwnode_reference_args {
  * @property_read_string_array: Read an array of string properties. Return zero
  *				on success, a negative error code otherwise.
  * @get_name: Return the name of an fwnode.
+ * @get_name_prefix: Get a prefix for a node (for printing purposes).
  * @get_parent: Return the parent of an fwnode.
  * @get_next_child_node: Return the next child node in an iteration.
  * @get_named_child_node: Return a child node with a given name.
@@ -87,6 +88,7 @@ struct fwnode_operations {
 				      const char *propname, const char **val,
 				      size_t nval);
 	const char *(*get_name)(const struct fwnode_handle *fwnode);
+	const char *(*get_name_prefix)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *
 	(*get_next_child_node)(const struct fwnode_handle *fwnode,
diff --git a/include/linux/property.h b/include/linux/property.h
index 2f5df900e310..0ea466ee8c3d 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -79,6 +79,7 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 				       struct fwnode_reference_args *args);
 
 const char *fwnode_get_name(const struct fwnode_handle *fwnode);
+const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 4/5] lib/vsprintf: Make use of fwnode API to obtain node names and separators
  2019-03-22 15:29 [PATCH 0/5] Device property improvements, add %pfw format specifier Sakari Ailus
                   ` (2 preceding siblings ...)
  2019-03-22 15:29 ` [PATCH 3/5] device property: Add a function to obtain a node's prefix Sakari Ailus
@ 2019-03-22 15:29 ` Sakari Ailus
  2019-03-27 12:53   ` Petr Mladek
  2019-03-22 15:29 ` [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names Sakari Ailus
  4 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2019-03-22 15:29 UTC (permalink / raw)
  To: Petr Mladek, linux-kernel, rafael; +Cc: Andy Shevchenko, linux-acpi, devicetree

Instead of implementing our own means of discovering parent nodes, node
names or counting how many parents a node has, use the newly added
functions in the fwnode API to obtain that information.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 lib/vsprintf.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 5f60b8d41277..91f2a3e4892e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -37,6 +37,7 @@
 #include <net/addrconf.h>
 #include <linux/siphash.h>
 #include <linux/compiler.h>
+#include <linux/property.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
@@ -1720,32 +1721,23 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
 	return format_flags(buf, end, flags, names);
 }
 
-static const char *device_node_name_for_depth(const struct device_node *np, int depth)
-{
-	for ( ; np && depth; depth--)
-		np = np->parent;
-
-	return kbasename(np->full_name);
-}
-
 static noinline_for_stack
-char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
+char *fwnode_gen_full_name(struct fwnode_handle *fwnode, char *buf, char *end)
 {
 	int depth;
-	const struct device_node *parent = np->parent;
 
-	/* special case for root node */
-	if (!parent)
-		return string(buf, end, "/", default_str_spec);
+	for (depth = fwnode_count_parents(fwnode); depth >= 0; depth--) {
+		struct fwnode_handle *__fwnode =
+			fwnode_get_nth_parent(fwnode, depth);
 
-	for (depth = 0; parent->parent; depth++)
-		parent = parent->parent;
-
-	for ( ; depth >= 0; depth--) {
-		buf = string(buf, end, "/", default_str_spec);
-		buf = string(buf, end, device_node_name_for_depth(np, depth),
+		buf = string(buf, end, fwnode_get_name_prefix(__fwnode),
+			     default_str_spec);
+		buf = string(buf, end, fwnode_get_name(__fwnode),
 			     default_str_spec);
+
+		fwnode_handle_put(__fwnode);
 	}
+
 	return buf;
 }
 
@@ -1790,10 +1782,11 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 
 		switch (*fmt) {
 		case 'f':	/* full_name */
-			buf = device_node_gen_full_name(dn, buf, end);
+			buf = fwnode_gen_full_name(of_fwnode_handle(dn), buf,
+						   end);
 			break;
 		case 'n':	/* name */
-			p = kbasename(of_node_full_name(dn));
+			p = fwnode_get_name(of_fwnode_handle(dn));
 			precision = str_spec.precision;
 			str_spec.precision = strchrnul(p, '@') - p;
 			buf = string(buf, end, p, str_spec);
@@ -1803,7 +1796,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
 			break;
 		case 'P':	/* path-spec */
-			p = kbasename(of_node_full_name(dn));
+			p = fwnode_get_name(of_fwnode_handle(dn));
 			if (!p[1])
 				p = "/";
 			buf = string(buf, end, p, str_spec);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-22 15:29 [PATCH 0/5] Device property improvements, add %pfw format specifier Sakari Ailus
                   ` (3 preceding siblings ...)
  2019-03-22 15:29 ` [PATCH 4/5] lib/vsprintf: Make use of fwnode API to obtain node names and separators Sakari Ailus
@ 2019-03-22 15:29 ` Sakari Ailus
  2019-03-22 17:21   ` Andy Shevchenko
  2019-03-26 15:13   ` Petr Mladek
  4 siblings, 2 replies; 25+ messages in thread
From: Sakari Ailus @ 2019-03-22 15:29 UTC (permalink / raw)
  To: Petr Mladek, linux-kernel, rafael; +Cc: Andy Shevchenko, linux-acpi, devicetree

Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
support printing full path of the node, including its name ("f") and only
the node's name ("P") in the printk family of functions. The two flags
have equivalent functionality to existing %pOF with the same two modifiers
("f" and "P") on OF based systems. The ability to do the same on ACPI
based systems is added by this patch.

On ACPI based systems the resulting strings look like

	\_SB.PCI0.CIO2.port@1.endpoint@0

where the nodes are separated by a dot (".") and the first three are
ACPI device nodes and the latter two ACPI data nodes.

Depends-on: ("vsprintf: Remove support for %pF and %pf in favour of %pS and %ps")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/core-api/printk-formats.rst | 24 ++++++++++++++
 lib/vsprintf.c                            | 52 +++++++++++++++++++++++++++++++
 scripts/checkpatch.pl                     |  2 +-
 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c90826a1ff17..209625e0c61d 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -410,6 +410,30 @@ Examples::
 
 Passed by reference.
 
+Fwnode handles
+--------------
+
+::
+
+	%pfw[fP]
+
+For printing information on fwnode handles. The default is to print the full
+node name, including the path. The modifiers are functionally equivalent to
+%pOF above.
+
+	- f - full name of the node, including the path
+	- P - the name of the node including an address (if there is one)
+
+Examples (ACPI):
+
+	%pfwf	\_SB.PCI0.CIO2.port@1.endpoint@0	- Full node name
+	%pfwP	endpoint@0				- Node name
+
+Examples (OF):
+
+	%pfwf	/ocp@68000000/i2c@48072000/camera@10/port/endpoint - Full name
+	%pfwP	endpoint				- Node name
+
 Time and date (struct rtc_time)
 -------------------------------
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 91f2a3e4892e..b94013413b11 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1834,6 +1834,48 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static noinline_for_stack
+char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
+		    struct printf_spec spec, const char *fmt)
+{
+	const char * const modifiers = "fP";
+	struct printf_spec str_spec = spec;
+	char *buf_start = buf;
+	bool pass;
+
+	str_spec.field_width = -1;
+
+	if ((unsigned long)fwnode < PAGE_SIZE)
+		return string(buf, end, "(null)", spec);
+
+	/* simple case without anything any more format specifiers */
+	fmt++;
+	if (fmt[0] == '\0' || strcspn(fmt, modifiers) > 0)
+		fmt = "f";
+
+	for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) {
+		if (pass) {
+			if (buf < end)
+				*buf = ':';
+			buf++;
+		}
+
+		switch (*fmt) {
+		case 'f':	/* full_name */
+			buf = fwnode_gen_full_name(fwnode, buf, end);
+			break;
+		case 'P':	/* name */
+			buf = string(buf, end, fwnode_get_name(fwnode),
+				     str_spec);
+			break;
+		default:
+			break;
+		}
+	}
+
+	return widen_string(buf, buf - buf_start, end, spec);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -1936,6 +1978,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
  *                  F device node flags
  *                  c major compatible string
  *                  C full compatible string
+ * - 'fw[fP]'	For a firmware node (struct fwnode_handle) pointer
+ *		Without an option prints the full name of the node
+ *		f full name
+ *		P node name, including a possible unit address
  * - 'x' For printing the address. Equivalent to "%lx".
  *
  * ** When making changes please also update:
@@ -2060,6 +2106,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return device_node_string(buf, end, ptr, spec, fmt + 1);
 		}
 		break;
+	case 'f':
+		switch (fmt[1]) {
+		case 'w':
+			return fwnode_string(buf, end, ptr, spec, fmt + 1);
+		}
+		break;
 	case 'x':
 		return pointer_string(buf, end, ptr, spec);
 	}
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b4e456b48fd7..10db4354d61b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5977,7 +5977,7 @@ sub process {
 				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
 					$specifier = $1;
 					$extension = $2;
-					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOx]/) {
+					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxf]/) {
 						$bad_specifier = $specifier;
 						last;
 					}
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-22 15:29 ` [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names Sakari Ailus
@ 2019-03-22 17:21   ` Andy Shevchenko
  2019-03-24 18:17     ` Sakari Ailus
  2019-03-26 15:13   ` Petr Mladek
  1 sibling, 1 reply; 25+ messages in thread
From: Andy Shevchenko @ 2019-03-22 17:21 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: Petr Mladek, linux-kernel, rafael, linux-acpi, devicetree

On Fri, Mar 22, 2019 at 05:29:30PM +0200, Sakari Ailus wrote:
> Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> support printing full path of the node, including its name ("f") and only
> the node's name ("P") in the printk family of functions. The two flags
> have equivalent functionality to existing %pOF with the same two modifiers
> ("f" and "P") on OF based systems. The ability to do the same on ACPI
> based systems is added by this patch.

Do we encourage people to use it instead of %pOF cases where it is suitable?

> On ACPI based systems the resulting strings look like
> 
> 	\_SB.PCI0.CIO2.port@1.endpoint@0
> 
> where the nodes are separated by a dot (".") and the first three are
> ACPI device nodes and the latter two ACPI data nodes.

Do we support swnode here?

> +static noinline_for_stack
> +char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
> +		    struct printf_spec spec, const char *fmt)
> +{
> +	const char * const modifiers = "fP";
> +	struct printf_spec str_spec = spec;
> +	char *buf_start = buf;
> +	bool pass;
> +
> +	str_spec.field_width = -1;
> +

> +	if ((unsigned long)fwnode < PAGE_SIZE)
> +		return string(buf, end, "(null)", spec);

Just put there a NULL pointer, we would not like to maintain duplicated strings
over the kernel.

I remember Petr has a patch series related to address space check, though I
don't remember the status of affairs.

> +
> +	/* simple case without anything any more format specifiers */
> +	fmt++;
> +	if (fmt[0] == '\0' || strcspn(fmt, modifiers) > 0)
> +		fmt = "f";
> +
> +	for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) {

I don't see test cases.

What would we get out of %pfwfffPPPfff?

Hint: I'm expecting above to be equivalent to %pfwf

> +		if (pass) {
> +			if (buf < end)
> +				*buf = ':';
> +			buf++;
> +		}
> +
> +		switch (*fmt) {
> +		case 'f':	/* full_name */
> +			buf = fwnode_gen_full_name(fwnode, buf, end);
> +			break;
> +		case 'P':	/* name */
> +			buf = string(buf, end, fwnode_get_name(fwnode),
> +				     str_spec);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return widen_string(buf, buf - buf_start, end, spec);
> +}

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/5] device property: Add fwnode_get_name for returning the name of a node
  2019-03-22 15:29 ` [PATCH 2/5] device property: Add fwnode_get_name for returning the name of a node Sakari Ailus
@ 2019-03-24 17:21   ` Randy Dunlap
  2019-03-24 18:19     ` Sakari Ailus
  0 siblings, 1 reply; 25+ messages in thread
From: Randy Dunlap @ 2019-03-24 17:21 UTC (permalink / raw)
  To: Sakari Ailus, Petr Mladek, linux-kernel, rafael
  Cc: Andy Shevchenko, linux-acpi, devicetree

Hi,

On 3/22/19 8:29 AM, Sakari Ailus wrote:
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 3f5ff6b23ae4..7ee3786bcde3 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -533,6 +533,18 @@ int device_add_properties(struct device *dev,
>  EXPORT_SYMBOL_GPL(device_add_properties);
>  
>  /**
> + * fwnode_get_name - Return the name of a node
> + *
> + * @fwnode - The firmware node

s/-/:/

> + *
> + * Returns a pointer to the node name.
> + */
> +const char *fwnode_get_name(const struct fwnode_handle *fwnode)
> +{
> +	return fwnode_call_ptr_op(fwnode, get_name);
> +}
> +
> +/**
>   * fwnode_get_parent - Return parent firwmare node
>   * @fwnode: Firmware whose parent is retrieved
>   *


-- 
~Randy

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-22 17:21   ` Andy Shevchenko
@ 2019-03-24 18:17     ` Sakari Ailus
  2019-03-26 13:13       ` Andy Shevchenko
  0 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2019-03-24 18:17 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Petr Mladek, linux-kernel, rafael, linux-acpi, devicetree

Hi Andy,

Thanks for the comments.

On Fri, Mar 22, 2019 at 07:21:14PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 22, 2019 at 05:29:30PM +0200, Sakari Ailus wrote:
> > Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> > support printing full path of the node, including its name ("f") and only
> > the node's name ("P") in the printk family of functions. The two flags
> > have equivalent functionality to existing %pOF with the same two modifiers
> > ("f" and "P") on OF based systems. The ability to do the same on ACPI
> > based systems is added by this patch.
> 
> Do we encourage people to use it instead of %pOF cases where it is suitable?

For code that is used on both OF and ACPI based systems, I think so. But if
you have something that is only used on OF, %pOF is better --- it has more
functionality that seems quite OF specific. In general I think the ability
to print a node's full name is way more important on OF. On ACPI you don't
need it so often --- which is probably the reason it hasn't been supported.

> 
> > On ACPI based systems the resulting strings look like
> > 
> > 	\_SB.PCI0.CIO2.port@1.endpoint@0
> > 
> > where the nodes are separated by a dot (".") and the first three are
> > ACPI device nodes and the latter two ACPI data nodes.
> 
> Do we support swnode here?

Good question. The swnodes have no hierarchy at the moment (they're only
created for a struct device as a parent) and they do not have human-readable
names. So I'd say it's not relevant right now. Should these two change,
support for swnode could (and should) be added later on.

> 
> > +static noinline_for_stack
> > +char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
> > +		    struct printf_spec spec, const char *fmt)
> > +{
> > +	const char * const modifiers = "fP";
> > +	struct printf_spec str_spec = spec;
> > +	char *buf_start = buf;
> > +	bool pass;
> > +
> > +	str_spec.field_width = -1;
> > +
> 
> > +	if ((unsigned long)fwnode < PAGE_SIZE)
> > +		return string(buf, end, "(null)", spec);
> 
> Just put there a NULL pointer, we would not like to maintain duplicated strings
> over the kernel.
> 
> I remember Petr has a patch series related to address space check, though I
> don't remember the status of affairs.

This bit has been actually adopted from the OF counterpart. If there are
improvements in this area, then I'd just change both at the same time.

> 
> > +
> > +	/* simple case without anything any more format specifiers */
> > +	fmt++;
> > +	if (fmt[0] == '\0' || strcspn(fmt, modifiers) > 0)
> > +		fmt = "f";
> > +
> > +	for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) {
> 
> I don't see test cases.
> 
> What would we get out of %pfwfffPPPfff?
> 
> Hint: I'm expecting above to be equivalent to %pfwf

I guess it's a matter of expectations. :-) Again this works the same way
than the OF counterpart. Right now there's little to print (just the name
and the full name), but if support is added for more, then this mechanism is
fully relevant again.

The alternative would be to remove that now and add it back if it's needed
again. I have a slight preference towards keeping it extensible (i.e. as
it's now).

> 
> > +		if (pass) {
> > +			if (buf < end)
> > +				*buf = ':';
> > +			buf++;
> > +		}
> > +
> > +		switch (*fmt) {
> > +		case 'f':	/* full_name */
> > +			buf = fwnode_gen_full_name(fwnode, buf, end);
> > +			break;
> > +		case 'P':	/* name */
> > +			buf = string(buf, end, fwnode_get_name(fwnode),
> > +				     str_spec);
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	return widen_string(buf, buf - buf_start, end, spec);
> > +}
> 

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 2/5] device property: Add fwnode_get_name for returning the name of a node
  2019-03-24 17:21   ` Randy Dunlap
@ 2019-03-24 18:19     ` Sakari Ailus
  0 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2019-03-24 18:19 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Petr Mladek, linux-kernel, rafael, Andy Shevchenko, linux-acpi,
	devicetree

Hi Randy,

On Sun, Mar 24, 2019 at 10:21:17AM -0700, Randy Dunlap wrote:
> > + * @fwnode - The firmware node
> 
> s/-/:/

Yes, indeed.

Thanks!

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-24 18:17     ` Sakari Ailus
@ 2019-03-26 13:13       ` Andy Shevchenko
  2019-03-26 13:39         ` Sakari Ailus
  2019-03-26 14:06         ` Heikki Krogerus
  0 siblings, 2 replies; 25+ messages in thread
From: Andy Shevchenko @ 2019-03-26 13:13 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Petr Mladek, linux-kernel, rafael, linux-acpi, devicetree,
	Heikki Krogerus

On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote:
> On Fri, Mar 22, 2019 at 07:21:14PM +0200, Andy Shevchenko wrote:
> > On Fri, Mar 22, 2019 at 05:29:30PM +0200, Sakari Ailus wrote:
> > > Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> > > support printing full path of the node, including its name ("f") and only
> > > the node's name ("P") in the printk family of functions. The two flags
> > > have equivalent functionality to existing %pOF with the same two modifiers
> > > ("f" and "P") on OF based systems. The ability to do the same on ACPI
> > > based systems is added by this patch.
> > 
> > Do we encourage people to use it instead of %pOF cases where it is suitable?
> 
> For code that is used on both OF and ACPI based systems, I think so. But if
> you have something that is only used on OF, %pOF is better --- it has more
> functionality that seems quite OF specific. In general I think the ability
> to print a node's full name is way more important on OF. On ACPI you don't
> need it so often --- which is probably the reason it hasn't been supported.

But if code is going to support ACPI and DT and at the same time use %pOF
extensions that are not covered by %pfw it would be inconsistent somehow.

> > > On ACPI based systems the resulting strings look like
> > > 
> > > 	\_SB.PCI0.CIO2.port@1.endpoint@0
> > > 
> > > where the nodes are separated by a dot (".") and the first three are
> > > ACPI device nodes and the latter two ACPI data nodes.
> > 
> > Do we support swnode here?
> 
> Good question. The swnodes have no hierarchy at the moment (they're only
> created for a struct device as a parent) and they do not have human-readable
> names. So I'd say it's not relevant right now. Should these two change,
> support for swnode could (and should) be added later on.

Heikki, what do you think about this?

> > > +	if ((unsigned long)fwnode < PAGE_SIZE)
> > > +		return string(buf, end, "(null)", spec);
> > 
> > Just put there a NULL pointer, we would not like to maintain duplicated strings
> > over the kernel.
> > 
> > I remember Petr has a patch series related to address space check, though I
> > don't remember the status of affairs.
> 
> This bit has been actually adopted from the OF counterpart. If there are
> improvements in this area, then I'd just change both at the same time.

The patch series by Petr I mentioned takes care about OF case. But it doesn't
have covered yours by obvious reasons.

> > > +	for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) {
> > 
> > I don't see test cases.
> > 
> > What would we get out of %pfwfffPPPfff?
> > 
> > Hint: I'm expecting above to be equivalent to %pfwf
> 
> I guess it's a matter of expectations. :-)

Common sense and basic expectations from all of %p extensions.

> Again this works the same way
> than the OF counterpart.

OF lacks of testing apparently.

> Right now there's little to print (just the name
> and the full name), but if support is added for more, then this mechanism is
> fully relevant again.
> 
> The alternative would be to remove that now and add it back if it's needed
> again. I have a slight preference towards keeping it extensible (i.e. as
> it's now).

See how other helpers do parse this.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 13:13       ` Andy Shevchenko
@ 2019-03-26 13:39         ` Sakari Ailus
  2019-03-26 13:55           ` Andy Shevchenko
  2019-03-26 14:06         ` Heikki Krogerus
  1 sibling, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2019-03-26 13:39 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Petr Mladek, linux-kernel, rafael, linux-acpi, devicetree,
	Heikki Krogerus

Hi Andy,

On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote:
> > On Fri, Mar 22, 2019 at 07:21:14PM +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 22, 2019 at 05:29:30PM +0200, Sakari Ailus wrote:
> > > > Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> > > > support printing full path of the node, including its name ("f") and only
> > > > the node's name ("P") in the printk family of functions. The two flags
> > > > have equivalent functionality to existing %pOF with the same two modifiers
> > > > ("f" and "P") on OF based systems. The ability to do the same on ACPI
> > > > based systems is added by this patch.
> > > 
> > > Do we encourage people to use it instead of %pOF cases where it is suitable?
> > 
> > For code that is used on both OF and ACPI based systems, I think so. But if
> > you have something that is only used on OF, %pOF is better --- it has more
> > functionality that seems quite OF specific. In general I think the ability
> > to print a node's full name is way more important on OF. On ACPI you don't
> > need it so often --- which is probably the reason it hasn't been supported.
> 
> But if code is going to support ACPI and DT and at the same time use %pOF
> extensions that are not covered by %pfw it would be inconsistent somehow.

What you mostly need are the full name and the name of a given node. I
wasn't sure whether adding more would have been relevant, and at least it
is likely to have few if any users, so I didn't add that yet. Do not that
it can be implemented later on if it's needed --- that's also why the
modifiers are aligned with %pOF.

> 
> > > > On ACPI based systems the resulting strings look like
> > > > 
> > > > 	\_SB.PCI0.CIO2.port@1.endpoint@0
> > > > 
> > > > where the nodes are separated by a dot (".") and the first three are
> > > > ACPI device nodes and the latter two ACPI data nodes.
> > > 
> > > Do we support swnode here?
> > 
> > Good question. The swnodes have no hierarchy at the moment (they're only
> > created for a struct device as a parent) and they do not have human-readable
> > names. So I'd say it's not relevant right now. Should these two change,
> > support for swnode could (and should) be added later on.
> 
> Heikki, what do you think about this?
> 
> > > > +	if ((unsigned long)fwnode < PAGE_SIZE)
> > > > +		return string(buf, end, "(null)", spec);
> > > 
> > > Just put there a NULL pointer, we would not like to maintain duplicated strings
> > > over the kernel.
> > > 
> > > I remember Petr has a patch series related to address space check, though I
> > > don't remember the status of affairs.
> > 
> > This bit has been actually adopted from the OF counterpart. If there are
> > improvements in this area, then I'd just change both at the same time.
> 
> The patch series by Petr I mentioned takes care about OF case. But it doesn't
> have covered yours by obvious reasons.

Do you happen to have a pointer to it?

> 
> > > > +	for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) {
> > > 
> > > I don't see test cases.
> > > 
> > > What would we get out of %pfwfffPPPfff?
> > > 
> > > Hint: I'm expecting above to be equivalent to %pfwf
> > 
> > I guess it's a matter of expectations. :-)
> 
> Common sense and basic expectations from all of %p extensions.
> 
> > Again this works the same way
> > than the OF counterpart.
> 
> OF lacks of testing apparently.
> 
> > Right now there's little to print (just the name
> > and the full name), but if support is added for more, then this mechanism is
> > fully relevant again.
> > 
> > The alternative would be to remove that now and add it back if it's needed
> > again. I have a slight preference towards keeping it extensible (i.e. as
> > it's now).
> 
> See how other helpers do parse this.

The behaviour on others is different indeed, you're generally printing a
single item at a time. The question rather is, whether we want to be
compatible with %pOF going forward or not. I'd prefer that, so using the
fwnode API would be easier.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 13:39         ` Sakari Ailus
@ 2019-03-26 13:55           ` Andy Shevchenko
  2019-03-26 14:09             ` Sakari Ailus
  2019-03-26 15:21             ` Petr Mladek
  0 siblings, 2 replies; 25+ messages in thread
From: Andy Shevchenko @ 2019-03-26 13:55 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Petr Mladek, linux-kernel, rafael, linux-acpi, devicetree,
	Heikki Krogerus

On Tue, Mar 26, 2019 at 03:39:47PM +0200, Sakari Ailus wrote:
> On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:
> > On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote:

> > The patch series by Petr I mentioned takes care about OF case. But it doesn't
> > have covered yours by obvious reasons.
> 
> Do you happen to have a pointer to it?

Petr, can you share what is the state of affairs with that series?

> The behaviour on others is different indeed, you're generally printing a
> single item at a time. The question rather is, whether we want to be
> compatible with %pOF going forward or not. I'd prefer that, so using the
> fwnode API would be easier.

I would prefer to mimic %pOF and actually to deprecate it in favour of %pfw.
But it's just mine opinion. I'm skeptical about getting support on it.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 13:13       ` Andy Shevchenko
  2019-03-26 13:39         ` Sakari Ailus
@ 2019-03-26 14:06         ` Heikki Krogerus
  2019-03-26 14:12           ` Sakari Ailus
  1 sibling, 1 reply; 25+ messages in thread
From: Heikki Krogerus @ 2019-03-26 14:06 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Sakari Ailus, Petr Mladek, linux-kernel, rafael, linux-acpi, devicetree

Hi,

On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:
> > > > On ACPI based systems the resulting strings look like
> > > > 
> > > > 	\_SB.PCI0.CIO2.port@1.endpoint@0
> > > > 
> > > > where the nodes are separated by a dot (".") and the first three are
> > > > ACPI device nodes and the latter two ACPI data nodes.
> > > 
> > > Do we support swnode here?
> > 
> > Good question. The swnodes have no hierarchy at the moment (they're only
> > created for a struct device as a parent) and they do not have human-readable
> > names. So I'd say it's not relevant right now. Should these two change,
> > support for swnode could (and should) be added later on.
> 
> Heikki, what do you think about this?

Well, the swnodes do have hierarchy. That was kind of the whole point
of introducing them. They now can also be named using "name" property.
See commit 344798206f171c5abea7ab1f9762fa526d7f539d.

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 13:55           ` Andy Shevchenko
@ 2019-03-26 14:09             ` Sakari Ailus
  2019-03-26 15:21             ` Petr Mladek
  1 sibling, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2019-03-26 14:09 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Petr Mladek, linux-kernel, rafael, linux-acpi, devicetree, robh,
	Heikki Krogerus

Hi Andy,

On Tue, Mar 26, 2019 at 03:55:57PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2019 at 03:39:47PM +0200, Sakari Ailus wrote:
> > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:
> > > On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote:
> 
> > > The patch series by Petr I mentioned takes care about OF case. But it doesn't
> > > have covered yours by obvious reasons.
> > 
> > Do you happen to have a pointer to it?
> 
> Petr, can you share what is the state of affairs with that series?
> 
> > The behaviour on others is different indeed, you're generally printing a
> > single item at a time. The question rather is, whether we want to be
> > compatible with %pOF going forward or not. I'd prefer that, so using the
> > fwnode API would be easier.
> 
> I would prefer to mimic %pOF and actually to deprecate it in favour of %pfw.
> But it's just mine opinion. I'm skeptical about getting support on it.

IMHO code that only deals with OF specifically is better to continue to use
%pOF. You'd have of_fwnode_handle() in places where you just had the name
of the node previously.

What could be done though is to unify the implementations; that's something
which the set does a little of already.

Cc Rob, too.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 14:06         ` Heikki Krogerus
@ 2019-03-26 14:12           ` Sakari Ailus
  2019-03-26 14:30             ` Andy Shevchenko
  2019-03-26 14:30             ` Heikki Krogerus
  0 siblings, 2 replies; 25+ messages in thread
From: Sakari Ailus @ 2019-03-26 14:12 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Andy Shevchenko, Petr Mladek, linux-kernel, rafael, linux-acpi,
	devicetree

Moi,

On Tue, Mar 26, 2019 at 04:06:33PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:
> > > > > On ACPI based systems the resulting strings look like
> > > > > 
> > > > > 	\_SB.PCI0.CIO2.port@1.endpoint@0
> > > > > 
> > > > > where the nodes are separated by a dot (".") and the first three are
> > > > > ACPI device nodes and the latter two ACPI data nodes.
> > > > 
> > > > Do we support swnode here?
> > > 
> > > Good question. The swnodes have no hierarchy at the moment (they're only
> > > created for a struct device as a parent) and they do not have human-readable
> > > names. So I'd say it's not relevant right now. Should these two change,
> > > support for swnode could (and should) be added later on.
> > 
> > Heikki, what do you think about this?
> 
> Well, the swnodes do have hierarchy. That was kind of the whole point
> of introducing them. They now can also be named using "name" property.
> See commit 344798206f171c5abea7ab1f9762fa526d7f539d.

Right; I saw the function after initially replying to Andy but I missed
where the node name came from. :-) Now I know...

I can add support for swnode, too, if you like.

-- 
Terveisin,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 14:12           ` Sakari Ailus
@ 2019-03-26 14:30             ` Andy Shevchenko
  2019-03-26 15:50               ` Petr Mladek
  2019-03-26 14:30             ` Heikki Krogerus
  1 sibling, 1 reply; 25+ messages in thread
From: Andy Shevchenko @ 2019-03-26 14:30 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Heikki Krogerus, Petr Mladek, linux-kernel, rafael, linux-acpi,
	devicetree

On Tue, Mar 26, 2019 at 04:12:43PM +0200, Sakari Ailus wrote:
> On Tue, Mar 26, 2019 at 04:06:33PM +0200, Heikki Krogerus wrote:
> > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:

> > > > > Do we support swnode here?
> > > > 
> > > > Good question. The swnodes have no hierarchy at the moment (they're only
> > > > created for a struct device as a parent) and they do not have human-readable
> > > > names. So I'd say it's not relevant right now. Should these two change,
> > > > support for swnode could (and should) be added later on.
> > > 
> > > Heikki, what do you think about this?
> > 
> > Well, the swnodes do have hierarchy. That was kind of the whole point
> > of introducing them. They now can also be named using "name" property.
> > See commit 344798206f171c5abea7ab1f9762fa526d7f539d.
> 
> Right; I saw the function after initially replying to Andy but I missed
> where the node name came from. :-) Now I know...
> 
> I can add support for swnode, too, if you like.

Definitely!

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 14:12           ` Sakari Ailus
  2019-03-26 14:30             ` Andy Shevchenko
@ 2019-03-26 14:30             ` Heikki Krogerus
  1 sibling, 0 replies; 25+ messages in thread
From: Heikki Krogerus @ 2019-03-26 14:30 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Andy Shevchenko, Petr Mladek, linux-kernel, rafael, linux-acpi,
	devicetree

On Tue, Mar 26, 2019 at 04:12:43PM +0200, Sakari Ailus wrote:
> Moi,
> 
> On Tue, Mar 26, 2019 at 04:06:33PM +0200, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:
> > > > > > On ACPI based systems the resulting strings look like
> > > > > > 
> > > > > > 	\_SB.PCI0.CIO2.port@1.endpoint@0
> > > > > > 
> > > > > > where the nodes are separated by a dot (".") and the first three are
> > > > > > ACPI device nodes and the latter two ACPI data nodes.
> > > > > 
> > > > > Do we support swnode here?
> > > > 
> > > > Good question. The swnodes have no hierarchy at the moment (they're only
> > > > created for a struct device as a parent) and they do not have human-readable
> > > > names. So I'd say it's not relevant right now. Should these two change,
> > > > support for swnode could (and should) be added later on.
> > > 
> > > Heikki, what do you think about this?
> > 
> > Well, the swnodes do have hierarchy. That was kind of the whole point
> > of introducing them. They now can also be named using "name" property.
> > See commit 344798206f171c5abea7ab1f9762fa526d7f539d.
> 
> Right; I saw the function after initially replying to Andy but I missed
> where the node name came from. :-) Now I know...
> 
> I can add support for swnode, too, if you like.

It's up to you.

thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-22 15:29 ` [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names Sakari Ailus
  2019-03-22 17:21   ` Andy Shevchenko
@ 2019-03-26 15:13   ` Petr Mladek
  2019-03-27 14:10     ` Sakari Ailus
  1 sibling, 1 reply; 25+ messages in thread
From: Petr Mladek @ 2019-03-26 15:13 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-kernel, rafael, Andy Shevchenko, linux-acpi, devicetree

On Fri 2019-03-22 17:29:30, Sakari Ailus wrote:
> Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> support printing full path of the node, including its name ("f") and only
> the node's name ("P") in the printk family of functions. The two flags
> have equivalent functionality to existing %pOF with the same two modifiers
> ("f" and "P") on OF based systems. The ability to do the same on ACPI
> based systems is added by this patch.
> 
> On ACPI based systems the resulting strings look like
> 
> 	\_SB.PCI0.CIO2.port@1.endpoint@0
> 
> where the nodes are separated by a dot (".") and the first three are
> ACPI device nodes and the latter two ACPI data nodes.
> 
> Depends-on: ("vsprintf: Remove support for %pF and %pf in favour of %pS and %ps")

Reusing obsolete modifiers is dangerous from many reasons:

   + people might miss the change of the meaning
   + backporting mistakes
   + 3rd party modules

It might be acceptable if the long term gain is bigger
than a short time difficulties. But it would be better
to it a safe way when possible.

Fortunately, we could keep the backward compatibility
for "%pf" and handle only "%pfw*" with the fwnode api.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 13:55           ` Andy Shevchenko
  2019-03-26 14:09             ` Sakari Ailus
@ 2019-03-26 15:21             ` Petr Mladek
  1 sibling, 0 replies; 25+ messages in thread
From: Petr Mladek @ 2019-03-26 15:21 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Sakari Ailus, linux-kernel, rafael, linux-acpi, devicetree,
	Heikki Krogerus

On Tue 2019-03-26 15:55:57, Andy Shevchenko wrote:
> On Tue, Mar 26, 2019 at 03:39:47PM +0200, Sakari Ailus wrote:
> > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:
> > > On Sun, Mar 24, 2019 at 08:17:46PM +0200, Sakari Ailus wrote:
> 
> > > The patch series by Petr I mentioned takes care about OF case. But it doesn't
> > > have covered yours by obvious reasons.
> > 
> > Do you happen to have a pointer to it?
> 
> Petr, can you share what is the state of affairs with that series?

I might send a new version this week but I do not promise it.
It is regularly pushed aside by more urgent work.

But we could work on both changes independently. Both can be
updated easily depending on which one gets accepted earlier.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 14:30             ` Andy Shevchenko
@ 2019-03-26 15:50               ` Petr Mladek
  0 siblings, 0 replies; 25+ messages in thread
From: Petr Mladek @ 2019-03-26 15:50 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Sakari Ailus, Heikki Krogerus, linux-kernel, rafael, linux-acpi,
	devicetree

On Tue 2019-03-26 16:30:21, Andy Shevchenko wrote:
> On Tue, Mar 26, 2019 at 04:12:43PM +0200, Sakari Ailus wrote:
> > On Tue, Mar 26, 2019 at 04:06:33PM +0200, Heikki Krogerus wrote:
> > > On Tue, Mar 26, 2019 at 03:13:53PM +0200, Andy Shevchenko wrote:
> 
> > > > > > Do we support swnode here?
> > > > > 
> > > > > Good question. The swnodes have no hierarchy at the moment (they're only
> > > > > created for a struct device as a parent) and they do not have human-readable
> > > > > names. So I'd say it's not relevant right now. Should these two change,
> > > > > support for swnode could (and should) be added later on.
> > > > 
> > > > Heikki, what do you think about this?
> > > 
> > > Well, the swnodes do have hierarchy. That was kind of the whole point
> > > of introducing them. They now can also be named using "name" property.
> > > See commit 344798206f171c5abea7ab1f9762fa526d7f539d.
> > 
> > Right; I saw the function after initially replying to Andy but I missed
> > where the node name came from. :-) Now I know...
> > 
> > I can add support for swnode, too, if you like.
> 
> Definitely!

It might really make sense to obsolete %pOF and handle all three
(OF, ACPI, Software) nodes using the same %pfw modifiers.

If I get it correctly, we could distinguish them by
fwnode->ops, see is_of_node(), is_acpi_static_node(),
is_software_node().

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/5] lib/vsprintf: Make use of fwnode API to obtain node names and separators
  2019-03-22 15:29 ` [PATCH 4/5] lib/vsprintf: Make use of fwnode API to obtain node names and separators Sakari Ailus
@ 2019-03-27 12:53   ` Petr Mladek
  2019-03-27 13:49     ` Sakari Ailus
  0 siblings, 1 reply; 25+ messages in thread
From: Petr Mladek @ 2019-03-27 12:53 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-kernel, rafael, Andy Shevchenko, linux-acpi, devicetree

On Fri 2019-03-22 17:29:29, Sakari Ailus wrote:
> Instead of implementing our own means of discovering parent nodes, node
> names or counting how many parents a node has, use the newly added
> functions in the fwnode API to obtain that information.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  lib/vsprintf.c | 37 +++++++++++++++----------------------
>  1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 5f60b8d41277..91f2a3e4892e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -37,6 +37,7 @@
>  #include <net/addrconf.h>
>  #include <linux/siphash.h>
>  #include <linux/compiler.h>
> +#include <linux/property.h>
>  #ifdef CONFIG_BLOCK
>  #include <linux/blkdev.h>
>  #endif
> @@ -1720,32 +1721,23 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
>  	return format_flags(buf, end, flags, names);
>  }
>  
> -static const char *device_node_name_for_depth(const struct device_node *np, int depth)
> -{
> -	for ( ; np && depth; depth--)
> -		np = np->parent;
> -
> -	return kbasename(np->full_name);
> -}
> -
>  static noinline_for_stack
> -char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
> +char *fwnode_gen_full_name(struct fwnode_handle *fwnode, char *buf, char *end)

I first read this as fwnode_get_full_name(), see "get" vs. "gen".
I guess that it was because there is used also fwnode_get_name().

Please, rename it to fwnode_full_name_string() or so ;-)

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 4/5] lib/vsprintf: Make use of fwnode API to obtain node names and separators
  2019-03-27 12:53   ` Petr Mladek
@ 2019-03-27 13:49     ` Sakari Ailus
  0 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2019-03-27 13:49 UTC (permalink / raw)
  To: Petr Mladek; +Cc: linux-kernel, rafael, Andy Shevchenko, linux-acpi, devicetree

Hi Petr,

Thank you for reviewing these patches.

On Wed, Mar 27, 2019 at 01:53:56PM +0100, Petr Mladek wrote:
> On Fri 2019-03-22 17:29:29, Sakari Ailus wrote:
> > Instead of implementing our own means of discovering parent nodes, node
> > names or counting how many parents a node has, use the newly added
> > functions in the fwnode API to obtain that information.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  lib/vsprintf.c | 37 +++++++++++++++----------------------
> >  1 file changed, 15 insertions(+), 22 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 5f60b8d41277..91f2a3e4892e 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -37,6 +37,7 @@
> >  #include <net/addrconf.h>
> >  #include <linux/siphash.h>
> >  #include <linux/compiler.h>
> > +#include <linux/property.h>
> >  #ifdef CONFIG_BLOCK
> >  #include <linux/blkdev.h>
> >  #endif
> > @@ -1720,32 +1721,23 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
> >  	return format_flags(buf, end, flags, names);
> >  }
> >  
> > -static const char *device_node_name_for_depth(const struct device_node *np, int depth)
> > -{
> > -	for ( ; np && depth; depth--)
> > -		np = np->parent;
> > -
> > -	return kbasename(np->full_name);
> > -}
> > -
> >  static noinline_for_stack
> > -char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
> > +char *fwnode_gen_full_name(struct fwnode_handle *fwnode, char *buf, char *end)
> 
> I first read this as fwnode_get_full_name(), see "get" vs. "gen".
> I guess that it was because there is used also fwnode_get_name().
> 
> Please, rename it to fwnode_full_name_string() or so ;-)

Works for me.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-26 15:13   ` Petr Mladek
@ 2019-03-27 14:10     ` Sakari Ailus
  2019-03-28 14:35       ` Petr Mladek
  0 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2019-03-27 14:10 UTC (permalink / raw)
  To: Petr Mladek; +Cc: linux-kernel, rafael, Andy Shevchenko, linux-acpi, devicetree

Hi Petr,

On Tue, Mar 26, 2019 at 04:13:07PM +0100, Petr Mladek wrote:
> On Fri 2019-03-22 17:29:30, Sakari Ailus wrote:
> > Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> > support printing full path of the node, including its name ("f") and only
> > the node's name ("P") in the printk family of functions. The two flags
> > have equivalent functionality to existing %pOF with the same two modifiers
> > ("f" and "P") on OF based systems. The ability to do the same on ACPI
> > based systems is added by this patch.
> > 
> > On ACPI based systems the resulting strings look like
> > 
> > 	\_SB.PCI0.CIO2.port@1.endpoint@0
> > 
> > where the nodes are separated by a dot (".") and the first three are
> > ACPI device nodes and the latter two ACPI data nodes.
> > 
> > Depends-on: ("vsprintf: Remove support for %pF and %pf in favour of %pS and %ps")
> 
> Reusing obsolete modifiers is dangerous from many reasons:
> 
>    + people might miss the change of the meaning
>    + backporting mistakes
>    + 3rd party modules
> 
> It might be acceptable if the long term gain is bigger
> than a short time difficulties. But it would be better
> to it a safe way when possible.
> 
> Fortunately, we could keep the backward compatibility
> for "%pf" and handle only "%pfw*" with the fwnode api.

The v2 of this patch produces a warning (using WARN_ONCE()) for "%pf" not
immediately followed by "w". "%pfw" was not a valid conversion specifier
before this set, so we're actually not re-using the exactly same conversion
specifiers.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  2019-03-27 14:10     ` Sakari Ailus
@ 2019-03-28 14:35       ` Petr Mladek
  0 siblings, 0 replies; 25+ messages in thread
From: Petr Mladek @ 2019-03-28 14:35 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-kernel, rafael, Andy Shevchenko, linux-acpi, devicetree

On Wed 2019-03-27 16:10:48, Sakari Ailus wrote:
> Hi Petr,
> 
> On Tue, Mar 26, 2019 at 04:13:07PM +0100, Petr Mladek wrote:
> > On Fri 2019-03-22 17:29:30, Sakari Ailus wrote:
> > > Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> > > support printing full path of the node, including its name ("f") and only
> > > the node's name ("P") in the printk family of functions. The two flags
> > > have equivalent functionality to existing %pOF with the same two modifiers
> > > ("f" and "P") on OF based systems. The ability to do the same on ACPI
> > > based systems is added by this patch.
> > > 
> > > On ACPI based systems the resulting strings look like
> > > 
> > > 	\_SB.PCI0.CIO2.port@1.endpoint@0
> > > 
> > > where the nodes are separated by a dot (".") and the first three are
> > > ACPI device nodes and the latter two ACPI data nodes.
> > > 
> > > Depends-on: ("vsprintf: Remove support for %pF and %pf in favour of %pS and %ps")
> > 
> > Reusing obsolete modifiers is dangerous from many reasons:
> > 
> >    + people might miss the change of the meaning
> >    + backporting mistakes
> >    + 3rd party modules
> > 
> > It might be acceptable if the long term gain is bigger
> > than a short time difficulties. But it would be better
> > to it a safe way when possible.
> > 
> > Fortunately, we could keep the backward compatibility
> > for "%pf" and handle only "%pfw*" with the fwnode api.
> 
> The v2 of this patch produces a warning (using WARN_ONCE()) for "%pf" not
> immediately followed by "w". "%pfw" was not a valid conversion specifier
> before this set, so we're actually not re-using the exactly same conversion
> specifiers.

I see. I would keep the two patchsets separate. I mean that this
patchset should expect that the original handling of %pf is still
there.

The way how to remove or obsolete "%pf" should be handled in
the patchset removing all %pf users or it can be done
in a completely separate patch.

The conflict can get handled when merging the two patchsets.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2019-03-28 14:35 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-22 15:29 [PATCH 0/5] Device property improvements, add %pfw format specifier Sakari Ailus
2019-03-22 15:29 ` [PATCH 1/5] device property: Add functions for accessing node's parents Sakari Ailus
2019-03-22 15:29 ` [PATCH 2/5] device property: Add fwnode_get_name for returning the name of a node Sakari Ailus
2019-03-24 17:21   ` Randy Dunlap
2019-03-24 18:19     ` Sakari Ailus
2019-03-22 15:29 ` [PATCH 3/5] device property: Add a function to obtain a node's prefix Sakari Ailus
2019-03-22 15:29 ` [PATCH 4/5] lib/vsprintf: Make use of fwnode API to obtain node names and separators Sakari Ailus
2019-03-27 12:53   ` Petr Mladek
2019-03-27 13:49     ` Sakari Ailus
2019-03-22 15:29 ` [PATCH 5/5] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names Sakari Ailus
2019-03-22 17:21   ` Andy Shevchenko
2019-03-24 18:17     ` Sakari Ailus
2019-03-26 13:13       ` Andy Shevchenko
2019-03-26 13:39         ` Sakari Ailus
2019-03-26 13:55           ` Andy Shevchenko
2019-03-26 14:09             ` Sakari Ailus
2019-03-26 15:21             ` Petr Mladek
2019-03-26 14:06         ` Heikki Krogerus
2019-03-26 14:12           ` Sakari Ailus
2019-03-26 14:30             ` Andy Shevchenko
2019-03-26 15:50               ` Petr Mladek
2019-03-26 14:30             ` Heikki Krogerus
2019-03-26 15:13   ` Petr Mladek
2019-03-27 14:10     ` Sakari Ailus
2019-03-28 14:35       ` Petr Mladek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.