[PATCH 0/4] device property: Add fwnode_name() helper

[PATCH 0/4] device property: Add fwnode_name() helper

[Heikki Krogerus]

Hi,

I prepared fwnode_name() for something I'm working on, but then I
realized that it makes it possible to also implement a generic
fwnode_get_named_child_node(). So in practice I'm proposing that we
replace get_named_child_node fwnode op with that new "name" fwnode op
in this series.

thanks,

Heikki Krogerus (4):
  device property: Introduce fwnode_name()
  ACPI: property: Introduce acpi_fwnode_name()
  of/property: Introduce of_fwnode_name()
  device property: Drop get_named_child_node callback

 drivers/acpi/property.c  | 26 +++++++-------------------
 drivers/base/property.c  | 28 +++++++++++++++++++++++++++-
 drivers/of/property.c    | 21 ++++++---------------
 include/linux/fwnode.h   |  4 +---
 include/linux/property.h |  2 ++
 5 files changed, 43 insertions(+), 38 deletions(-)

-- 
2.19.1

[PATCH 1/4] device property: Introduce fwnode_name()

[Heikki Krogerus]

This helper returns the name of the node. The name is
primarily expected to be returned from a new fwnode
operation meant for this purpose, but when no name is
returned, the helper will also attempt to read a device
property "name".

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c  | 18 ++++++++++++++++++
 include/linux/fwnode.h   |  1 +
 include/linux/property.h |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 240ab5230ff6..1e4eb7ab2b84 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1156,6 +1156,24 @@ void fwnode_handle_put(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_handle_put);
 
+/**
+ * fwnode_name - Get the name of a device node
+ * @fwnode: Pointer to the device node.
+ *
+ * Returns the node name of @fwnode. If @fwnode does not have a "unit name",
+ * this code will also attempt to read a string property named "name".
+ */
+const char *fwnode_name(const struct fwnode_handle *fwnode)
+{
+	const char *name = fwnode_call_ptr_op(fwnode, name);
+
+	if (!name)
+		fwnode_call_int_op(fwnode, property_read_string_array,
+				   "name", &name, 1);
+	return name;
+}
+EXPORT_SYMBOL_GPL(fwnode_name);
+
 /**
  * fwnode_device_is_available - check if a device is available for use
  * @fwnode: Pointer to the fwnode of the device.
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index faebf0ca0686..2e445fed4df4 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -72,6 +72,7 @@ struct fwnode_reference_args {
 struct fwnode_operations {
 	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
 	void (*put)(struct fwnode_handle *fwnode);
+	const char *(*name)(const struct fwnode_handle *fwnode);
 	bool (*device_is_available)(const struct fwnode_handle *fwnode);
 	const void *(*device_get_match_data)(const struct fwnode_handle *fwnode,
 					     const struct device *dev);
diff --git a/include/linux/property.h b/include/linux/property.h
index ac8a1ebc4c1b..44781d37dd96 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -109,6 +109,8 @@ struct fwnode_handle *device_get_named_child_node(struct device *dev,
 struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
 
+const char *fwnode_name(const struct fwnode_handle *fwnode);
+
 int fwnode_irq_get(struct fwnode_handle *fwnode, unsigned int index);
 
 unsigned int device_get_child_node_count(struct device *dev);
-- 
2.19.1

[PATCH 2/4] ACPI: property: Introduce acpi_fwnode_name()

[Heikki Krogerus]

ACPI data nodes have a name, so let's return that when
fwnode_name() is called.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/property.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 8c7c4583b52d..75854e07ed64 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1219,6 +1219,11 @@ acpi_graph_get_remote_endpoint(const struct fwnode_handle *__fwnode)
 	return acpi_graph_get_child_prop_value(fwnode, "endpoint", endpoint_nr);
 }
 
+static const char *acpi_fwnode_name(const struct fwnode_handle *node)
+{
+	return is_acpi_data_node(node) ? to_acpi_data_node(node)->name : NULL;
+}
+
 static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
 {
 	if (!is_acpi_device_node(fwnode))
@@ -1310,6 +1315,7 @@ acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 
 #define DECLARE_ACPI_FWNODE_OPS(ops) \
 	const struct fwnode_operations ops = {				\
+		.name = acpi_fwnode_name,				\
 		.device_is_available = acpi_fwnode_device_is_available, \
 		.device_get_match_data = acpi_fwnode_device_get_match_data, \
 		.property_present = acpi_fwnode_property_present,	\
-- 
2.19.1

[PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

Instead of always being forced to read the "name" property
in fwnode_name() with of_nodes, implementing the fwnode
operation meant for getting the node name.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Rob Herring <robh@kernel.org>
---
 drivers/of/property.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index f46828e3b082..ac7b0b6c2d4d 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -823,6 +823,11 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
 	of_node_put(to_of_node(fwnode));
 }
 
+static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
+{
+	return to_of_node(fwnode)->name;
+}
+
 static bool of_fwnode_device_is_available(const struct fwnode_handle *fwnode)
 {
 	return of_device_is_available(to_of_node(fwnode));
@@ -987,6 +992,7 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 const struct fwnode_operations of_fwnode_ops = {
 	.get = of_fwnode_get,
 	.put = of_fwnode_put,
+	.name = of_fwnode_name,
 	.device_is_available = of_fwnode_device_is_available,
 	.device_get_match_data = of_fwnode_device_get_match_data,
 	.property_present = of_fwnode_property_present,
-- 
2.19.1

[PATCH 4/4] device property: Drop get_named_child_node callback

[Heikki Krogerus]

By using fwnode_name() in fwnode_get_named_child_node(),
get_named_child_node implementations become boilerplate.
Removing all of them.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/property.c | 20 +-------------------
 drivers/base/property.c | 10 +++++++++-
 drivers/of/property.c   | 15 ---------------
 include/linux/fwnode.h  |  3 ---
 4 files changed, 10 insertions(+), 38 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 75854e07ed64..6c38cd73e4c0 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -587,23 +587,6 @@ static int acpi_data_get_property_array(const struct acpi_device_data *data,
 	return 0;
 }
 
-static struct fwnode_handle *
-acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
-				 const char *childname)
-{
-	struct fwnode_handle *child;
-
-	/*
-	 * Find first matching named child node of this fwnode.
-	 * For ACPI this will be a data only sub-node.
-	 */
-	fwnode_for_each_child_node(fwnode, child)
-		if (acpi_data_node_match(child, childname))
-			return child;
-
-	return NULL;
-}
-
 /**
  * __acpi_node_get_property_reference - returns handle to the referenced object
  * @fwnode: Firmware node to get the property from
@@ -712,7 +695,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 			for (ref_fwnode = acpi_fwnode_handle(device);
 			     element < end && element->type == ACPI_TYPE_STRING;
 			     element++) {
-				ref_fwnode = acpi_fwnode_get_named_child_node(
+				ref_fwnode = fwnode_get_named_child_node(
 					ref_fwnode, element->string.pointer);
 				if (!ref_fwnode)
 					return -EINVAL;
@@ -1325,7 +1308,6 @@ acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 			acpi_fwnode_property_read_string_array,		\
 		.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_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 1e4eb7ab2b84..06ba83d73517 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1111,7 +1111,15 @@ struct fwnode_handle *
 fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 			    const char *childname)
 {
-	return fwnode_call_ptr_op(fwnode, get_named_child_node, childname);
+	struct fwnode_handle *child;
+	const char *name;
+
+	fwnode_for_each_child_node(fwnode, child) {
+		name = fwnode_name(child);
+		if (name && !strcasecmp(name, childname))
+			return child;
+	}
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(fwnode_get_named_child_node);
 
diff --git a/drivers/of/property.c b/drivers/of/property.c
index ac7b0b6c2d4d..60b590fc5b99 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -890,20 +890,6 @@ of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
 							    to_of_node(child)));
 }
 
-static struct fwnode_handle *
-of_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
-			       const char *childname)
-{
-	const struct device_node *node = to_of_node(fwnode);
-	struct device_node *child;
-
-	for_each_available_child_of_node(node, child)
-		if (!of_node_cmp(child->name, childname))
-			return of_fwnode_handle(child);
-
-	return NULL;
-}
-
 static int
 of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
 			     const char *prop, const char *nargs_prop,
@@ -1000,7 +986,6 @@ const struct fwnode_operations of_fwnode_ops = {
 	.property_read_string_array = of_fwnode_property_read_string_array,
 	.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,
 	.get_reference_args = of_fwnode_get_reference_args,
 	.graph_get_next_endpoint = of_fwnode_graph_get_next_endpoint,
 	.graph_get_remote_endpoint = of_fwnode_graph_get_remote_endpoint,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 2e445fed4df4..83f8ec3754d0 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -90,9 +90,6 @@ struct fwnode_operations {
 	struct fwnode_handle *
 	(*get_next_child_node)(const struct fwnode_handle *fwnode,
 			       struct fwnode_handle *child);
-	struct fwnode_handle *
-	(*get_named_child_node)(const struct fwnode_handle *fwnode,
-				const char *name);
 	int (*get_reference_args)(const struct fwnode_handle *fwnode,
 				  const char *prop, const char *nargs_prop,
 				  unsigned int nargs, unsigned int index,
-- 
2.19.1

Re: [PATCH 1/4] device property: Introduce fwnode_name()

[Andy Shevchenko]

On Mon, Nov 05, 2018 at 12:17:24PM +0300, Heikki Krogerus wrote:
> This helper returns the name of the node. The name is
> primarily expected to be returned from a new fwnode
> operation meant for this purpose, but when no name is
> returned, the helper will also attempt to read a device
> property "name".
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/base/property.c  | 18 ++++++++++++++++++
>  include/linux/fwnode.h   |  1 +
>  include/linux/property.h |  2 ++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 240ab5230ff6..1e4eb7ab2b84 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1156,6 +1156,24 @@ void fwnode_handle_put(struct fwnode_handle *fwnode)
>  }
>  EXPORT_SYMBOL_GPL(fwnode_handle_put);
>  
> +/**
> + * fwnode_name - Get the name of a device node
> + * @fwnode: Pointer to the device node.
> + *
> + * Returns the node name of @fwnode. If @fwnode does not have a "unit name",
> + * this code will also attempt to read a string property named "name".
> + */
> +const char *fwnode_name(const struct fwnode_handle *fwnode)
> +{
> +	const char *name = fwnode_call_ptr_op(fwnode, name);
> +
> +	if (!name)
> +		fwnode_call_int_op(fwnode, property_read_string_array,
> +				   "name", &name, 1);

Is it going to be extended in the future?

Otherwise I would rather go with (few more LOCs, yes)

	const char *name;

	name = fwnode_call_ptr_op(fwnode, name);
	if (name)
		return name;

	fwnode_call_int_op(fwnode, property_read_string_array, "name", &name, 1);
	return name;

> +	return name;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_name);
> +
>  /**
>   * fwnode_device_is_available - check if a device is available for use
>   * @fwnode: Pointer to the fwnode of the device.
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index faebf0ca0686..2e445fed4df4 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -72,6 +72,7 @@ struct fwnode_reference_args {
>  struct fwnode_operations {
>  	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
>  	void (*put)(struct fwnode_handle *fwnode);
> +	const char *(*name)(const struct fwnode_handle *fwnode);
>  	bool (*device_is_available)(const struct fwnode_handle *fwnode);
>  	const void *(*device_get_match_data)(const struct fwnode_handle *fwnode,
>  					     const struct device *dev);
> diff --git a/include/linux/property.h b/include/linux/property.h
> index ac8a1ebc4c1b..44781d37dd96 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -109,6 +109,8 @@ struct fwnode_handle *device_get_named_child_node(struct device *dev,
>  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
>  void fwnode_handle_put(struct fwnode_handle *fwnode);
>  
> +const char *fwnode_name(const struct fwnode_handle *fwnode);
> +
>  int fwnode_irq_get(struct fwnode_handle *fwnode, unsigned int index);
>  
>  unsigned int device_get_child_node_count(struct device *dev);
> -- 
> 2.19.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] device property: Add fwnode_name() helper

[Andy Shevchenko]

On Mon, Nov 05, 2018 at 12:17:23PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> I prepared fwnode_name() for something I'm working on, but then I
> realized that it makes it possible to also implement a generic
> fwnode_get_named_child_node(). So in practice I'm proposing that we
> replace get_named_child_node fwnode op with that new "name" fwnode op
> in this series.
> 

All LGTM, though consider one minor comment in patch 1.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> thanks,
> 
> Heikki Krogerus (4):
>   device property: Introduce fwnode_name()
>   ACPI: property: Introduce acpi_fwnode_name()
>   of/property: Introduce of_fwnode_name()
>   device property: Drop get_named_child_node callback
> 
>  drivers/acpi/property.c  | 26 +++++++-------------------
>  drivers/base/property.c  | 28 +++++++++++++++++++++++++++-
>  drivers/of/property.c    | 21 ++++++---------------
>  include/linux/fwnode.h   |  4 +---
>  include/linux/property.h |  2 ++
>  5 files changed, 43 insertions(+), 38 deletions(-)
> 
> -- 
> 2.19.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] device property: Introduce fwnode_name()

[Heikki Krogerus]

Hi,

> > +const char *fwnode_name(const struct fwnode_handle *fwnode)
> > +{
> > +	const char *name = fwnode_call_ptr_op(fwnode, name);
> > +
> > +	if (!name)
> > +		fwnode_call_int_op(fwnode, property_read_string_array,
> > +				   "name", &name, 1);
> 
> Is it going to be extended in the future?
> 
> Otherwise I would rather go with (few more LOCs, yes)
> 
> 	const char *name;
> 
> 	name = fwnode_call_ptr_op(fwnode, name);
> 	if (name)
> 		return name;
> 
> 	fwnode_call_int_op(fwnode, property_read_string_array, "name", &name, 1);
> 	return name;

That does look better. I'll fix it.

thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Rob Herring]

On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Instead of always being forced to read the "name" property
> in fwnode_name() with of_nodes, implementing the fwnode
> operation meant for getting the node name.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Rob Herring <robh@kernel.org>
> ---
>  drivers/of/property.c | 6 ++++++

Please Cc the DT list for DT changes.

>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index f46828e3b082..ac7b0b6c2d4d 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -823,6 +823,11 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
>         of_node_put(to_of_node(fwnode));
>  }
>
> +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> +{
> +       return to_of_node(fwnode)->name;

I'm trying to get rid of the DT name ptr, so please don't add one. You
can use of_node_full_name() here instead if "<name>@<unit-address>"
instead of <name> is fine. Otherwise, you've got to allocate your own
storage and use "%pOFn" printf specifier.

Rob

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Instead of always being forced to read the "name" property
> > in fwnode_name() with of_nodes, implementing the fwnode
> > operation meant for getting the node name.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Rob Herring <robh@kernel.org>
> > ---
> >  drivers/of/property.c | 6 ++++++
> 
> Please Cc the DT list for DT changes.

OK.

> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index f46828e3b082..ac7b0b6c2d4d 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -823,6 +823,11 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
> >         of_node_put(to_of_node(fwnode));
> >  }
> >
> > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > +{
> > +       return to_of_node(fwnode)->name;
> 
> I'm trying to get rid of the DT name ptr, so please don't add one. You
> can use of_node_full_name() here instead if "<name>@<unit-address>"
> instead of <name> is fine. Otherwise, you've got to allocate your own
> storage and use "%pOFn" printf specifier.

OK, I'll use of_node_full_name().

thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Andy Shevchenko]

On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:

> > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > +{
> > +       return to_of_node(fwnode)->name;
> 
> I'm trying to get rid of the DT name ptr, so please don't add one. You
> can use of_node_full_name() here instead if "<name>@<unit-address>"
> instead of <name> is fine. Otherwise, you've got to allocate your own
> storage and use "%pOFn" printf specifier.

If we do this here, we will change a behaviour of the entire set of
of_fwnode_get_named_child_node() users.

I think this is out of scope of the series.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> 
> > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > +{
> > > +       return to_of_node(fwnode)->name;
> > 
> > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > instead of <name> is fine. Otherwise, you've got to allocate your own
> > storage and use "%pOFn" printf specifier.
> 
> If we do this here, we will change a behaviour of the entire set of
> of_fwnode_get_named_child_node() users.
> 
> I think this is out of scope of the series.

You have a point. We must use the same member that was used in
of_fwnode_get_named_child_node().

The goal of this series if most likely not clear from this patch
alone, so I'll send a second version and make sure to CC the DT list
and Rob.

But in any case, I'll keep this part as it is.


thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Rob Herring]

On Tue, Nov 6, 2018 at 6:27 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> >
> > > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > > +{
> > > > +       return to_of_node(fwnode)->name;
> > >
> > > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > > instead of <name> is fine. Otherwise, you've got to allocate your own
> > > storage and use "%pOFn" printf specifier.
> >
> > If we do this here, we will change a behaviour of the entire set of
> > of_fwnode_get_named_child_node() users.
> >
> > I think this is out of scope of the series.

No, because you are adding a firmware op for something that's going away.

> You have a point. We must use the same member that was used in
> of_fwnode_get_named_child_node().
>
> The goal of this series if most likely not clear from this patch
> alone, so I'll send a second version and make sure to CC the DT list
> and Rob.

Looking at patch 4, if matching the name is what you want to do, then
use the DT name matching functions. They were added in 4.19.

Rob

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Andy Shevchenko]

On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:
> On Tue, Nov 6, 2018 at 6:27 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > > > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > > > +{
> > > > > +       return to_of_node(fwnode)->name;
> > > >
> > > > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > > > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > > > instead of <name> is fine. Otherwise, you've got to allocate your own
> > > > storage and use "%pOFn" printf specifier.
> > >
> > > If we do this here, we will change a behaviour of the entire set of
> > > of_fwnode_get_named_child_node() users.
> > >
> > > I think this is out of scope of the series.
> 
> No, because you are adding a firmware op for something that's going away.

W/o your below explanation it wasn't obvious.

> 
> > You have a point. We must use the same member that was used in
> > of_fwnode_get_named_child_node().
> >
> > The goal of this series if most likely not clear from this patch
> > alone, so I'll send a second version and make sure to CC the DT list
> > and Rob.
> 
> Looking at patch 4, if matching the name is what you want to do, then
> use the DT name matching functions. They were added in 4.19.

Do you mean of_node_name_eq()?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:
> On Tue, Nov 6, 2018 at 6:27 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > > > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > > > +{
> > > > > +       return to_of_node(fwnode)->name;
> > > >
> > > > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > > > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > > > instead of <name> is fine. Otherwise, you've got to allocate your own
> > > > storage and use "%pOFn" printf specifier.
> > >
> > > If we do this here, we will change a behaviour of the entire set of
> > > of_fwnode_get_named_child_node() users.
> > >
> > > I think this is out of scope of the series.
> 
> No, because you are adding a firmware op for something that's going away.
> 
> > You have a point. We must use the same member that was used in
> > of_fwnode_get_named_child_node().
> >
> > The goal of this series if most likely not clear from this patch
> > alone, so I'll send a second version and make sure to CC the DT list
> > and Rob.
> 
> Looking at patch 4, if matching the name is what you want to do, then
> use the DT name matching functions. They were added in 4.19.

That is something that the of_fwnode_get_named_child_node() needs
to use (would have needed).

Regardless of what we do with that callback, fwnode_name() needs to
return the name in from that for example of_node_name_eq() takes as
the second parameter. So "node-name@unit-address" is not OK. Sorry for
not realizeing that before.

So I guess we need to either get the "node-name" from that full_name
member in of_fwnode_name() (Andy, are you OK with that?), or is there
already a helper that does it for us?


Thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Andy Shevchenko]

On Tue, Nov 06, 2018 at 04:40:37PM +0200, Heikki Krogerus wrote:
> On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:

> > Looking at patch 4, if matching the name is what you want to do, then
> > use the DT name matching functions. They were added in 4.19.
> 
> That is something that the of_fwnode_get_named_child_node() needs
> to use (would have needed).
> 
> Regardless of what we do with that callback, fwnode_name() needs to
> return the name in from that for example of_node_name_eq() takes as
> the second parameter. So "node-name@unit-address" is not OK. Sorry for
> not realizeing that before.
> 
> So I guess we need to either get the "node-name" from that full_name
> member in of_fwnode_name() (Andy, are you OK with that?), or is there
> already a helper that does it for us?

Looking into existing API I think we need something like

of_node_name_extract()

of_node_name_eq()
{
	name = of_node_name_extract();
	return strlen()...strncmp()...;
}

The question is who is going to allocate and free memory for the name out of it.

OTOH, of_fwnode_get_named_child_node() might need to copy that code which
brings the consistency issue (several places to maintain the same set of rules,
i.e. how we extract name out of full_name).

So, removal of name field shouldn't be done until we resolve the issue with
of_fwnode_get_named_child_node().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Tue, Nov 06, 2018 at 04:55:37PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 06, 2018 at 04:40:37PM +0200, Heikki Krogerus wrote:
> > On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:
> 
> > > Looking at patch 4, if matching the name is what you want to do, then
> > > use the DT name matching functions. They were added in 4.19.
> > 
> > That is something that the of_fwnode_get_named_child_node() needs
> > to use (would have needed).
> > 
> > Regardless of what we do with that callback, fwnode_name() needs to
> > return the name in from that for example of_node_name_eq() takes as
> > the second parameter. So "node-name@unit-address" is not OK. Sorry for
> > not realizeing that before.
> > 
> > So I guess we need to either get the "node-name" from that full_name
> > member in of_fwnode_name() (Andy, are you OK with that?), or is there
> > already a helper that does it for us?
> 
> Looking into existing API I think we need something like
> 
> of_node_name_extract()
> 
> of_node_name_eq()
> {
> 	name = of_node_name_extract();
> 	return strlen()...strncmp()...;
> }
> 
> The question is who is going to allocate and free memory for the name out of it.

Maybe it would be best to just read the "name" device property in
fwnode_name() and not have of_fwnode_name at all.

> OTOH, of_fwnode_get_named_child_node() might need to copy that code which
> brings the consistency issue (several places to maintain the same set of rules,
> i.e. how we extract name out of full_name).
> 
> So, removal of name field shouldn't be done until we resolve the issue with
> of_fwnode_get_named_child_node().

thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Andy Shevchenko]

On Tue, Nov 06, 2018 at 05:05:03PM +0200, Heikki Krogerus wrote:

> Maybe it would be best to just read the "name" device property in
> fwnode_name() and not have of_fwnode_name at all.

If it's a mandatory property or somehow its presence is guaranteed, it would work.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Rob Herring]

On Tue, Nov 6, 2018 at 9:53 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 06, 2018 at 05:05:03PM +0200, Heikki Krogerus wrote:
>
> > Maybe it would be best to just read the "name" device property in
> > fwnode_name() and not have of_fwnode_name at all.
>
> If it's a mandatory property or somehow its presence is guaranteed, it would work.

It is currently, but after removing the name ptr, my current plan is
to remove the 'name' property too for FDT. On real OpenFirmware, it is
a real property so it will remain for sure in some cases.

Rob

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Rob Herring]

On Tue, Nov 6, 2018 at 8:28 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:
> > On Tue, Nov 6, 2018 at 6:27 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > > > > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > > > > +{
> > > > > > +       return to_of_node(fwnode)->name;
> > > > >
> > > > > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > > > > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > > > > instead of <name> is fine. Otherwise, you've got to allocate your own
> > > > > storage and use "%pOFn" printf specifier.
> > > >
> > > > If we do this here, we will change a behaviour of the entire set of
> > > > of_fwnode_get_named_child_node() users.
> > > >
> > > > I think this is out of scope of the series.
> >
> > No, because you are adding a firmware op for something that's going away.
>
> W/o your below explanation it wasn't obvious.
>
> >
> > > You have a point. We must use the same member that was used in
> > > of_fwnode_get_named_child_node().
> > >
> > > The goal of this series if most likely not clear from this patch
> > > alone, so I'll send a second version and make sure to CC the DT list
> > > and Rob.
> >
> > Looking at patch 4, if matching the name is what you want to do, then
> > use the DT name matching functions. They were added in 4.19.
>
> Do you mean of_node_name_eq()?

Yes or maybe move further up and just retrieve child nodes by name.
There's a recent function Johan added for that too.
of_find_child_node_by_name IIRC.

Rob

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Tue, Nov 06, 2018 at 12:13:30PM -0600, Rob Herring wrote:
> On Tue, Nov 6, 2018 at 9:53 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 06, 2018 at 05:05:03PM +0200, Heikki Krogerus wrote:
> >
> > > Maybe it would be best to just read the "name" device property in
> > > fwnode_name() and not have of_fwnode_name at all.
> >
> > If it's a mandatory property or somehow its presence is guaranteed, it would work.
> 
> It is currently, but after removing the name ptr, my current plan is
> to remove the 'name' property too for FDT. On real OpenFirmware, it is
> a real property so it will remain for sure in some cases.

OK. I think we'll use the full_name after all.

thanks,

-- 
heikki