[PATCH 0/3] Introduce of_match_machine() helper

[PATCH 0/3] Introduce of_match_machine() helper

[Tuomas Tynkkynen]

Some mach-specific drivers that don't require their own node in the device tree
(for example drivers/soc/*, cpufreq, cpuidle etc.) want to match on the device
tree root compatible property instead. Instead of open-coding (and forgetting
to call of_node_put on the root node) everywhere, add a helper to the OF core.

Tuomas Tynkkynen (3):
  of: Add of_match_machine helper
  soc/tegra: Use of_match_machine in soc_is_tegra()
  cpuidle: big.LITTLE: Use of_match_machine

 drivers/cpuidle/cpuidle-big_little.c |  6 +-----
 drivers/of/base.c                    | 21 +++++++++++++++++++++
 drivers/soc/tegra/common.c           |  8 +-------
 include/linux/of.h                   |  3 +++
 4 files changed, 26 insertions(+), 12 deletions(-)

-- 
1.8.1.5

[PATCH 1/3] of: Add of_match_machine helper

[Tuomas Tynkkynen]

Add of_match_machine function to test the device tree root for an
of_match array. This can be useful when testing SoC versions at runtime,
for example.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
 drivers/of/base.c  | 21 +++++++++++++++++++++
 include/linux/of.h |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d8574ad..37798ea 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -977,6 +977,27 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
 EXPORT_SYMBOL(of_find_matching_node_and_match);
 
 /**
+ * of_match_machine - Tell if root of device tree has a matching of_match struct
+ *	@matches:	array of of device match structures to search in
+ *
+ *	Returns the result of of_match_node for the root node.
+ */
+const struct of_device_id *of_match_machine(const struct of_device_id *matches)
+{
+	const struct of_device_id *match;
+	struct device_node *root;
+
+	root = of_find_node_by_path("/");
+	if (!root)
+		return NULL;
+
+	match = of_match_node(matches, root);
+	of_node_put(root);
+	return match;
+}
+EXPORT_SYMBOL(of_match_machine);
+
+/**
  * of_modalias_node - Lookup appropriate modalias for a device node
  * @node:	pointer to a device tree node
  * @modalias:	Pointer to buffer that modalias value will be copied into
diff --git a/include/linux/of.h b/include/linux/of.h
index 6c4363b..b7a8817 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -289,6 +289,8 @@ extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
 extern const struct of_device_id *of_match_node(
 	const struct of_device_id *matches, const struct device_node *node);
+extern const struct of_device_id *of_match_machine(
+	const struct of_device_id *matches);
 extern int of_modalias_node(struct device_node *node, char *modalias, int len);
 extern void of_print_phandle_args(const char *msg, const struct of_phandle_args *args);
 extern struct device_node *of_parse_phandle(const struct device_node *np,
@@ -584,6 +586,7 @@ static inline const char *of_prop_next_string(struct property *prop,
 
 #define of_match_ptr(_ptr)	NULL
 #define of_match_node(_matches, _node)	NULL
+#define of_match_machine(_matches)	NULL
 #endif /* CONFIG_OF */
 
 #if defined(CONFIG_OF) && defined(CONFIG_NUMA)
-- 
1.8.1.5

[PATCH 1/3] of: Add of_match_machine helper

[Thierry Reding]

On Fri, Aug 08, 2014 at 02:01:53AM +0300, Tuomas Tynkkynen wrote:
> Add of_match_machine function to test the device tree root for an
> of_match array. This can be useful when testing SoC versions at runtime,
> for example.
> 
> Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
> ---
>  drivers/of/base.c  | 21 +++++++++++++++++++++
>  include/linux/of.h |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d8574ad..37798ea 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -977,6 +977,27 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
>  EXPORT_SYMBOL(of_find_matching_node_and_match);
>  
>  /**
> + * of_match_machine - Tell if root of device tree has a matching of_match struct
> + *	@matches:	array of of device match structures to search in
> + *
> + *	Returns the result of of_match_node for the root node.
> + */

I was going to say that this kerneldoc is weirdly formatted, but when I
looked at the other kerneldoc comments in this file it seems that they
use similarly weird formatting...

> +const struct of_device_id *of_match_machine(const struct of_device_id *matches)
> +{
> +	const struct of_device_id *match;
> +	struct device_node *root;
> +
> +	root = of_find_node_by_path("/");
> +	if (!root)
> +		return NULL;
> +
> +	match = of_match_node(matches, root);
> +	of_node_put(root);
> +	return match;
> +}
> +EXPORT_SYMBOL(of_match_machine);

I wonder if of_find_node_by_path("/") is somewhat overkill here. Perhaps
simply of_node_get(of_allnodes) would be more appropriate here since the
function is implemented in the core?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140808/67e6af16/attachment.sig>

[PATCH 1/3] of: Add of_match_machine helper

[Tuomas Tynkkynen]

On 08/08/14 12:41, Thierry Reding wrote:
> 
>> +const struct of_device_id *of_match_machine(const struct of_device_id *matches)
>> +{
>> +	const struct of_device_id *match;
>> +	struct device_node *root;
>> +
>> +	root = of_find_node_by_path("/");
>> +	if (!root)
>> +		return NULL;
>> +
>> +	match = of_match_node(matches, root);
>> +	of_node_put(root);
>> +	return match;
>> +}
>> +EXPORT_SYMBOL(of_match_machine);
> 
> I wonder if of_find_node_by_path("/") is somewhat overkill here. Perhaps
> simply of_node_get(of_allnodes) would be more appropriate here since the
> function is implemented in the core?

of_machine_is_compatible() uses of_find_node_by_path("/") as well, of_allnodes
seems to be only used when during iterating. So I'd prefer to have them
consistent.

-- 
nvpublic

[PATCH 1/3] of: Add of_match_machine helper

[Rob Herring]

On Fri, Aug 8, 2014 at 8:23 AM, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote:
>
>
> On 08/08/14 12:41, Thierry Reding wrote:
>>
>>> +const struct of_device_id *of_match_machine(const struct of_device_id *matches)
>>> +{
>>> +    const struct of_device_id *match;
>>> +    struct device_node *root;
>>> +
>>> +    root = of_find_node_by_path("/");
>>> +    if (!root)
>>> +            return NULL;
>>> +
>>> +    match = of_match_node(matches, root);
>>> +    of_node_put(root);
>>> +    return match;
>>> +}
>>> +EXPORT_SYMBOL(of_match_machine);
>>
>> I wonder if of_find_node_by_path("/") is somewhat overkill here. Perhaps
>> simply of_node_get(of_allnodes) would be more appropriate here since the
>> function is implemented in the core?
>
> of_machine_is_compatible() uses of_find_node_by_path("/") as well, of_allnodes
> seems to be only used when during iterating. So I'd prefer to have them
> consistent.

Agreed.

For the series:

Acked-by: Rob Herring <robh@kernel.org>

Rob

[PATCH 1/3] of: Add of_match_machine helper

[Grant Likely]

On Fri, 8 Aug 2014 14:01:57 -0500, Rob Herring <robherring2@gmail.com> wrote:
> On Fri, Aug 8, 2014 at 8:23 AM, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote:
> >
> >
> > On 08/08/14 12:41, Thierry Reding wrote:
> >>
> >>> +const struct of_device_id *of_match_machine(const struct of_device_id *matches)
> >>> +{
> >>> +    const struct of_device_id *match;
> >>> +    struct device_node *root;
> >>> +
> >>> +    root = of_find_node_by_path("/");
> >>> +    if (!root)
> >>> +            return NULL;
> >>> +
> >>> +    match = of_match_node(matches, root);
> >>> +    of_node_put(root);
> >>> +    return match;
> >>> +}
> >>> +EXPORT_SYMBOL(of_match_machine);
> >>
> >> I wonder if of_find_node_by_path("/") is somewhat overkill here. Perhaps
> >> simply of_node_get(of_allnodes) would be more appropriate here since the
> >> function is implemented in the core?
> >
> > of_machine_is_compatible() uses of_find_node_by_path("/") as well, of_allnodes
> > seems to be only used when during iterating. So I'd prefer to have them
> > consistent.
> 
> Agreed.

Disagreed. of_machine_is_compatible should be simplified.

g.

[PATCH 1/3] of: Add of_match_machine helper

[Grant Likely]

On Fri, 8 Aug 2014 02:01:53 +0300, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote:
> Add of_match_machine function to test the device tree root for an
> of_match array. This can be useful when testing SoC versions at runtime,
> for example.
> 
> Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
> ---
>  drivers/of/base.c  | 21 +++++++++++++++++++++
>  include/linux/of.h |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d8574ad..37798ea 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -977,6 +977,27 @@ struct device_node *of_find_matching_node_and_match(struct device_node *from,
>  EXPORT_SYMBOL(of_find_matching_node_and_match);
>  
>  /**
> + * of_match_machine - Tell if root of device tree has a matching of_match struct
> + *	@matches:	array of of device match structures to search in
> + *
> + *	Returns the result of of_match_node for the root node.
> + */
> +const struct of_device_id *of_match_machine(const struct of_device_id *matches)
> +{
> +	const struct of_device_id *match;
> +	struct device_node *root;
> +
> +	root = of_find_node_by_path("/");
> +	if (!root)
> +		return NULL;
> +
> +	match = of_match_node(matches, root);
> +	of_node_put(root);
> +	return match;
> +}
> +EXPORT_SYMBOL(of_match_machine);

Too wordy...

	return of_match_node(matches, of_allnodes);

:-)

It could be a static inline, but I don't think it's even worth having a
helper. The callers could just open code the above.

g.

[PATCH 1/3] of: Add of_match_machine helper

[Tuomas Tynkkynen]

On 17/08/14 18:28, Grant Likely wrote:
> On Fri, 8 Aug 2014 02:01:53 +0300, Tuomas Tynkkynen <ttynkkynen@nvidia.com> wrote:
[...]
>> +EXPORT_SYMBOL(of_match_machine);
> 
> Too wordy...
> 
> 	return of_match_node(matches, of_allnodes);
> 
> :-)
> 
> It could be a static inline, but I don't think it's even worth having a
> helper. The callers could just open code the above.
> 

Do you mean all the drivers should be referring to of_allnodes directly?
I see that it's indeed exported, but to me that sounds like relying too
much on an implementation detail. In fact, Documentation/devicetree/todo.txt
even seems to have a TODO entry for its removal ("Remove of_allnodes list
and iterate using list of child nodes alone").

-- 
nvpublic

[PATCH 2/3] soc/tegra: Use of_match_machine in soc_is_tegra()

[Tuomas Tynkkynen]

Use the new helper function, also fixing a device_node refcount leak.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
 drivers/soc/tegra/common.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
index a71cb74..f6571f5 100644
--- a/drivers/soc/tegra/common.c
+++ b/drivers/soc/tegra/common.c
@@ -20,11 +20,5 @@ static const struct of_device_id tegra_machine_match[] = {
 
 bool soc_is_tegra(void)
 {
-	struct device_node *root;
-
-	root = of_find_node_by_path("/");
-	if (!root)
-		return false;
-
-	return of_match_node(tegra_machine_match, root) != NULL;
+	return of_match_machine(tegra_machine_match) != NULL;
 }
-- 
1.8.1.5

[PATCH 2/3] soc/tegra: Use of_match_machine in soc_is_tegra()

[Stephen Warren]

On 08/07/2014 05:01 PM, Tuomas Tynkkynen wrote:
> Use the new helper function, also fixing a device_node refcount leak.

Acked-by: Stephen Warren <swarren@nvidia.com>

I assume this will go through some common branch in arm-soc so others 
can build on it?

[PATCH 2/3] soc/tegra: Use of_match_machine in soc_is_tegra()

[Thierry Reding]

On Fri, Aug 08, 2014 at 02:01:54AM +0300, Tuomas Tynkkynen wrote:
> Use the new helper function, also fixing a device_node refcount leak.
> 
> Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
> ---
>  drivers/soc/tegra/common.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140808/18991536/attachment.sig>

[PATCH 3/3] cpuidle: big.LITTLE: Use of_match_machine

[Tuomas Tynkkynen]

Use the new helper function, also fixing a device_node refcount leak.

Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
---
Compile tested only.

 drivers/cpuidle/cpuidle-big_little.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-big_little.c b/drivers/cpuidle/cpuidle-big_little.c
index 344d79fa..53524c8 100644
--- a/drivers/cpuidle/cpuidle-big_little.c
+++ b/drivers/cpuidle/cpuidle-big_little.c
@@ -172,15 +172,11 @@ static const struct of_device_id compatible_machine_match[] = {
 static int __init bl_idle_init(void)
 {
 	int ret;
-	struct device_node *root = of_find_node_by_path("/");
-
-	if (!root)
-		return -ENODEV;
 
 	/*
 	 * Initialize the driver just for a compliant set of machines
 	 */
-	if (!of_match_node(compatible_machine_match, root))
+	if (!of_match_machine(compatible_machine_match))
 		return -ENODEV;
 	/*
 	 * For now the differentiation between little and big cores
-- 
1.8.1.5

[PATCH 3/3] cpuidle: big.LITTLE: Use of_match_machine

[Thierry Reding]

On Fri, Aug 08, 2014 at 02:01:55AM +0300, Tuomas Tynkkynen wrote:
> Use the new helper function, also fixing a device_node refcount leak.
> 
> Signed-off-by: Tuomas Tynkkynen <ttynkkynen@nvidia.com>
> ---
> Compile tested only.
> 
>  drivers/cpuidle/cpuidle-big_little.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140808/d9b58a66/attachment.sig>

[PATCH 0/3] Introduce of_match_machine() helper

[Stephen Warren]

On 08/07/2014 05:01 PM, Tuomas Tynkkynen wrote:
> Some mach-specific drivers that don't require their own node in the device tree
> (for example drivers/soc/*, cpufreq, cpuidle etc.) want to match on the device
> tree root compatible property instead. Instead of open-coding (and forgetting
> to call of_node_put on the root node) everywhere, add a helper to the OF core.

Tuomas, I assume there will be a new version of this series to address 
the review comments? For now, I'm not intending on applying this.

[PATCH 0/3] Introduce of_match_machine() helper

[Peter De Schrijver]

On Mon, Aug 25, 2014 at 07:45:38PM +0200, Stephen Warren wrote:
> On 08/07/2014 05:01 PM, Tuomas Tynkkynen wrote:
> > Some mach-specific drivers that don't require their own node in the device tree
> > (for example drivers/soc/*, cpufreq, cpuidle etc.) want to match on the device
> > tree root compatible property instead. Instead of open-coding (and forgetting
> > to call of_node_put on the root node) everywhere, add a helper to the OF core.
> 
> Tuomas, I assume there will be a new version of this series to address 
> the review comments? For now, I'm not intending on applying this.

Given that Tuomas's internship ended, I'm willing to push this patch set
forward, if there is interest. So are people still interested in this idea
or should we drop it?

Thanks,

Peter.

[PATCH 0/3] Introduce of_match_machine() helper

[Peter De Schrijver]

On Fri, Sep 19, 2014 at 04:12:45PM +0300, Peter De Schrijver wrote:
> On Mon, Aug 25, 2014 at 07:45:38PM +0200, Stephen Warren wrote:
> > On 08/07/2014 05:01 PM, Tuomas Tynkkynen wrote:
> > > Some mach-specific drivers that don't require their own node in the device tree
> > > (for example drivers/soc/*, cpufreq, cpuidle etc.) want to match on the device
> > > tree root compatible property instead. Instead of open-coding (and forgetting
> > > to call of_node_put on the root node) everywhere, add a helper to the OF core.
> > 
> > Tuomas, I assume there will be a new version of this series to address 
> > the review comments? For now, I'm not intending on applying this.
> 
> Given that Tuomas's internship ended, I'm willing to push this patch set
> forward, if there is interest. So are people still interested in this idea
> or should we drop it?

Seems there's no interest. I won't bother with this then.

Cheers,

Peter.