linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] of: base: add support to get machine model name
@ 2016-11-17 15:32 Sudeep Holla
  2016-11-17 15:32 ` [PATCH 2/2] of: base: replace all duplicate code with of_machine_get_model_name Sudeep Holla
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Sudeep Holla @ 2016-11-17 15:32 UTC (permalink / raw)
  To: linux-kernel
  Cc: Sudeep Holla, Rob Herring, Arnd Bergmann, devicetree, Frank Rowand

Currently platforms/drivers needing to get the machine model name are
replicating the same snippet of code. In some case, the OF reference
counting is either missing or incorrect.

This patch adds support to read the machine model name either using
the "model" or the "compatible" property in the device tree root node
to the core OF/DT code.

This can be used to remove all the duplicate code snippets doing exactly
same thing later.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
 include/linux/of.h |  6 ++++++
 2 files changed, 38 insertions(+)

Hi Rob,

It would be good if we can target this for v4.10, so that we have no
dependencies to push PATCH 2/2 in v4.11

Regards,
Sudeep

diff --git a/drivers/of/base.c b/drivers/of/base.c
index a0bccb54a9bd..0810c5ecf1aa 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -546,6 +546,38 @@ int of_machine_is_compatible(const char *compat)
 EXPORT_SYMBOL(of_machine_is_compatible);

 /**
+ * of_machine_get_model_name - Find and read the model name or the compatible
+ *		value for the machine.
+ * @model:	pointer to null terminated return string, modified only if
+ *		return value is 0.
+ *
+ * Returns a string containing either the model name or the compatible value
+ * of the machine if found, else return error.
+ *
+ * Search for a machine model name or the compatible if model name is missing
+ * in a device tree node and retrieve a null terminated string value (pointer
+ * to data, not a copy). Returns 0 on success, -EINVAL if root of the device
+ * tree is not found and other error returned by of_property_read_string on
+ * failure.
+ */
+int of_machine_get_model_name(const char **model)
+{
+	int error;
+
+	if (!of_node_get(of_root))
+		return -EINVAL;
+
+	error = of_property_read_string(of_root, "model", model);
+	if (error)
+		error = of_property_read_string_index(of_root, "compatible",
+						      0, model);
+	of_node_put(of_root);
+
+	return error;
+}
+EXPORT_SYMBOL(of_machine_get_model_name);
+
+/**
  *  __of_device_is_available - check if a device is available for use
  *
  *  @device: Node to check for availability, with locks already held
diff --git a/include/linux/of.h b/include/linux/of.h
index d72f01009297..13fc66531f1b 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -367,6 +367,7 @@ extern int of_alias_get_id(struct device_node *np, const char *stem);
 extern int of_alias_get_highest_id(const char *stem);

 extern int of_machine_is_compatible(const char *compat);
+extern int of_machine_get_model_name(const char **model);

 extern int of_add_property(struct device_node *np, struct property *prop);
 extern int of_remove_property(struct device_node *np, struct property *prop);
@@ -788,6 +789,11 @@ static inline int of_machine_is_compatible(const char *compat)
 	return 0;
 }

+static inline int of_machine_get_model_name(const char **model)
+{
+	return -EINVAL;
+}
+
 static inline bool of_console_check(const struct device_node *dn, const char *name, int index)
 {
 	return false;
--
2.7.4

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

* [PATCH 2/2] of: base: replace all duplicate code with of_machine_get_model_name
  2016-11-17 15:32 [PATCH 1/2] of: base: add support to get machine model name Sudeep Holla
@ 2016-11-17 15:32 ` Sudeep Holla
  2016-11-17 21:00 ` [PATCH 1/2] of: base: add support to get machine model name Frank Rowand
  2016-11-18 14:46 ` Rob Herring
  2 siblings, 0 replies; 21+ messages in thread
From: Sudeep Holla @ 2016-11-17 15:32 UTC (permalink / raw)
  To: linux-kernel; +Cc: Sudeep Holla, Rob Herring, Arnd Bergmann, devicetree

This patch replaces all the code snippets that reads the model name
from the device tree with the newly added helper function:
of_machine_get_model_name

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm/mach-imx/cpu.c           |  5 +----
 arch/arm/mach-mxs/mach-mxs.c      |  4 +---
 arch/mips/cavium-octeon/setup.c   | 12 ++----------
 arch/mips/generic/proc.c          | 15 +++------------
 arch/sh/boards/of-generic.c       |  8 +-------
 drivers/soc/fsl/guts.c            |  8 ++------
 drivers/soc/renesas/renesas-soc.c |  4 +---
 7 files changed, 11 insertions(+), 45 deletions(-)

Hi,

This patch is just to show the use of PATCH 1/2. It needs to go after
1/2 is merged for sake of simplicity and avoid cross dependencies
(targetting v4.11)

Regards,
Sudeep

diff --git a/arch/arm/mach-imx/cpu.c b/arch/arm/mach-imx/cpu.c
index b3347d32349f..dc3ebc1560dd 100644
--- a/arch/arm/mach-imx/cpu.c
+++ b/arch/arm/mach-imx/cpu.c
@@ -75,7 +75,6 @@ struct device * __init imx_soc_device_init(void)
 {
 	struct soc_device_attribute *soc_dev_attr;
 	struct soc_device *soc_dev;
-	struct device_node *root;
 	const char *soc_id;
 	int ret;

@@ -85,9 +84,7 @@ struct device * __init imx_soc_device_init(void)

 	soc_dev_attr->family = "Freescale i.MX";

-	root = of_find_node_by_path("/");
-	ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
-	of_node_put(root);
+	ret = of_machine_get_model_name(&soc_dev_attr->machine);
 	if (ret)
 		goto free_soc;

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index e4f21086b42b..871c65f02fca 100644
--- a/arch/arm/mach-mxs/mach-mxs.c
+++ b/arch/arm/mach-mxs/mach-mxs.c
@@ -381,7 +381,6 @@ static void __init eukrea_mbmx283lc_init(void)

 static void __init mxs_machine_init(void)
 {
-	struct device_node *root;
 	struct device *parent;
 	struct soc_device *soc_dev;
 	struct soc_device_attribute *soc_dev_attr;
@@ -391,8 +390,7 @@ static void __init mxs_machine_init(void)
 	if (!soc_dev_attr)
 		return;

-	root = of_find_node_by_path("/");
-	ret = of_property_read_string(root, "model", &soc_dev_attr->machine);
+	ret = of_machine_get_model_name(&soc_dev_attr->machine);
 	if (ret)
 		return;

diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
index 9a2db1c013d9..2e2b1b5befa4 100644
--- a/arch/mips/cavium-octeon/setup.c
+++ b/arch/mips/cavium-octeon/setup.c
@@ -498,16 +498,8 @@ static void __init init_octeon_system_type(void)
 	char const *board_type;

 	board_type = cvmx_board_type_to_string(octeon_bootinfo->board_type);
-	if (board_type == NULL) {
-		struct device_node *root;
-		int ret;
-
-		root = of_find_node_by_path("/");
-		ret = of_property_read_string(root, "model", &board_type);
-		of_node_put(root);
-		if (ret)
-			board_type = "Unsupported Board";
-	}
+	if (!board_type && of_machine_get_model_name(&board_type))
+		board_type = "Unsupported Board";

 	snprintf(octeon_system_type, sizeof(octeon_system_type), "%s (%s)",
 		 board_type, octeon_model_get_string(read_c0_prid()));
diff --git a/arch/mips/generic/proc.c b/arch/mips/generic/proc.c
index 42b33250a4a2..f7fc067bf908 100644
--- a/arch/mips/generic/proc.c
+++ b/arch/mips/generic/proc.c
@@ -10,20 +10,11 @@

 #include <linux/of.h>

-#include <asm/bootinfo.h>
-
 const char *get_system_type(void)
 {
 	const char *str;
-	int err;
-
-	err = of_property_read_string(of_root, "model", &str);
-	if (!err)
-		return str;
-
-	err = of_property_read_string_index(of_root, "compatible", 0, &str);
-	if (!err)
-		return str;

-	return "Unknown";
+	if (of_machine_get_model_name(&str))
+		return "Unknown";
+	return str;
 }
diff --git a/arch/sh/boards/of-generic.c b/arch/sh/boards/of-generic.c
index 1fb6d5714bae..9de3f81b2d22 100644
--- a/arch/sh/boards/of-generic.c
+++ b/arch/sh/boards/of-generic.c
@@ -124,8 +124,6 @@ static void __init sh_of_time_init(void)

 static void __init sh_of_setup(char **cmdline_p)
 {
-	struct device_node *root;
-
 #ifdef CONFIG_USE_BUILTIN_DTB
 	unflatten_and_copy_device_tree();
 #else
@@ -135,11 +133,7 @@ static void __init sh_of_setup(char **cmdline_p)
 	board_time_init = sh_of_time_init;

 	sh_mv.mv_name = "Unknown SH model";
-	root = of_find_node_by_path("/");
-	if (root) {
-		of_property_read_string(root, "model", &sh_mv.mv_name);
-		of_node_put(root);
-	}
+	of_machine_get_model_name(&sh_mv.mv_name);

 	sh_of_smp_probe();
 }
diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
index 6af7a11f09a5..94aef0465451 100644
--- a/drivers/soc/fsl/guts.c
+++ b/drivers/soc/fsl/guts.c
@@ -132,7 +132,7 @@ EXPORT_SYMBOL(fsl_guts_get_svr);

 static int fsl_guts_probe(struct platform_device *pdev)
 {
-	struct device_node *root, *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	const struct fsl_soc_die_attr *soc_die;
@@ -152,11 +152,7 @@ static int fsl_guts_probe(struct platform_device *pdev)
 		return PTR_ERR(guts->regs);

 	/* Register soc device */
-	root = of_find_node_by_path("/");
-	if (of_property_read_string(root, "model", &machine))
-		of_property_read_string_index(root, "compatible", 0, &machine);
-	of_node_put(root);
-	if (machine)
+	if (!of_machine_get_model_name(&machine))
 		soc_dev_attr.machine = devm_kstrdup(dev, machine, GFP_KERNEL);

 	svr = fsl_guts_get_svr();
diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
index 330960312296..d9a119073de5 100644
--- a/drivers/soc/renesas/renesas-soc.c
+++ b/drivers/soc/renesas/renesas-soc.c
@@ -228,9 +228,7 @@ static int __init renesas_soc_init(void)
 	if (!soc_dev_attr)
 		return -ENOMEM;

-	np = of_find_node_by_path("/");
-	of_property_read_string(np, "model", &soc_dev_attr->machine);
-	of_node_put(np);
+	of_machine_get_model_name(&soc_dev_attr->machine);

 	soc_dev_attr->family = kstrdup_const(family->name, GFP_KERNEL);
 	soc_dev_attr->soc_id = kstrdup_const(strchr(match->compatible, ',') + 1,
--
2.7.4

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-17 15:32 [PATCH 1/2] of: base: add support to get machine model name Sudeep Holla
  2016-11-17 15:32 ` [PATCH 2/2] of: base: replace all duplicate code with of_machine_get_model_name Sudeep Holla
@ 2016-11-17 21:00 ` Frank Rowand
  2016-11-17 22:12   ` Frank Rowand
  2016-11-18 10:41   ` Sudeep Holla
  2016-11-18 14:46 ` Rob Herring
  2 siblings, 2 replies; 21+ messages in thread
From: Frank Rowand @ 2016-11-17 21:00 UTC (permalink / raw)
  To: Sudeep Holla, linux-kernel; +Cc: Rob Herring, Arnd Bergmann, devicetree

On 11/17/16 07:32, Sudeep Holla wrote:
> Currently platforms/drivers needing to get the machine model name are
> replicating the same snippet of code. In some case, the OF reference
> counting is either missing or incorrect.
> 
> This patch adds support to read the machine model name either using
> the "model" or the "compatible" property in the device tree root node
> to the core OF/DT code.
> 
> This can be used to remove all the duplicate code snippets doing exactly
> same thing later.

I find five instances of reading only property "model":

  arch/arm/mach-imx/cpu.c
  arch/arm/mach-mxs/mach-mxs.c
  arch/c6x/kernel/setup.c
  arch/mips/cavium-octeon/setup.c
  arch/sh/boards/of-generic.c

I find one instance of reading property "model", then if
that does not exist, property "compatible":

  arch/mips/generic/proc.c

The proposed patch matches the code used in one place, and thus
current usage does not match the patch description.

Is my search bad?  Are you planning to add additional instances
of reading "model" then "compatible"?

-Frank

> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
>  include/linux/of.h |  6 ++++++
>  2 files changed, 38 insertions(+)
> 
> Hi Rob,
> 
> It would be good if we can target this for v4.10, so that we have no
> dependencies to push PATCH 2/2 in v4.11
> 
> Regards,
> Sudeep
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index a0bccb54a9bd..0810c5ecf1aa 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -546,6 +546,38 @@ int of_machine_is_compatible(const char *compat)
>  EXPORT_SYMBOL(of_machine_is_compatible);
> 
>  /**
> + * of_machine_get_model_name - Find and read the model name or the compatible
> + *		value for the machine.
> + * @model:	pointer to null terminated return string, modified only if
> + *		return value is 0.
> + *
> + * Returns a string containing either the model name or the compatible value
> + * of the machine if found, else return error.
> + *
> + * Search for a machine model name or the compatible if model name is missing
> + * in a device tree node and retrieve a null terminated string value (pointer
> + * to data, not a copy). Returns 0 on success, -EINVAL if root of the device
> + * tree is not found and other error returned by of_property_read_string on
> + * failure.
> + */
> +int of_machine_get_model_name(const char **model)
> +{
> +	int error;
> +
> +	if (!of_node_get(of_root))
> +		return -EINVAL;
> +
> +	error = of_property_read_string(of_root, "model", model);
> +	if (error)
> +		error = of_property_read_string_index(of_root, "compatible",
> +						      0, model);
> +	of_node_put(of_root);
> +
> +	return error;
> +}
> +EXPORT_SYMBOL(of_machine_get_model_name);
> +
> +/**
>   *  __of_device_is_available - check if a device is available for use
>   *
>   *  @device: Node to check for availability, with locks already held
> diff --git a/include/linux/of.h b/include/linux/of.h
> index d72f01009297..13fc66531f1b 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -367,6 +367,7 @@ extern int of_alias_get_id(struct device_node *np, const char *stem);
>  extern int of_alias_get_highest_id(const char *stem);
> 
>  extern int of_machine_is_compatible(const char *compat);
> +extern int of_machine_get_model_name(const char **model);
> 
>  extern int of_add_property(struct device_node *np, struct property *prop);
>  extern int of_remove_property(struct device_node *np, struct property *prop);
> @@ -788,6 +789,11 @@ static inline int of_machine_is_compatible(const char *compat)
>  	return 0;
>  }
> 
> +static inline int of_machine_get_model_name(const char **model)
> +{
> +	return -EINVAL;
> +}
> +
>  static inline bool of_console_check(const struct device_node *dn, const char *name, int index)
>  {
>  	return false;
> --
> 2.7.4
> 
> 

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-17 21:00 ` [PATCH 1/2] of: base: add support to get machine model name Frank Rowand
@ 2016-11-17 22:12   ` Frank Rowand
  2016-11-18 10:41   ` Sudeep Holla
  1 sibling, 0 replies; 21+ messages in thread
From: Frank Rowand @ 2016-11-17 22:12 UTC (permalink / raw)
  To: Sudeep Holla, linux-kernel; +Cc: Rob Herring, Arnd Bergmann, devicetree

On 11/17/16 13:00, Frank Rowand wrote:
> On 11/17/16 07:32, Sudeep Holla wrote:
>> Currently platforms/drivers needing to get the machine model name are
>> replicating the same snippet of code. In some case, the OF reference
>> counting is either missing or incorrect.
>>
>> This patch adds support to read the machine model name either using
>> the "model" or the "compatible" property in the device tree root node
>> to the core OF/DT code.
>>
>> This can be used to remove all the duplicate code snippets doing exactly
>> same thing later.
> 
> I find five instances of reading only property "model":
> 
>   arch/arm/mach-imx/cpu.c
>   arch/arm/mach-mxs/mach-mxs.c
>   arch/c6x/kernel/setup.c
>   arch/mips/cavium-octeon/setup.c
>   arch/sh/boards/of-generic.c

My initial search was a little too strict. With a less restrictive
search I find 16 more instances of reading property "model" and
not reading property "compatible".

> 
> I find one instance of reading property "model", then if
> that does not exist, property "compatible":
> 
>   arch/mips/generic/proc.c
> 
> The proposed patch matches the code used in one place, and thus
> current usage does not match the patch description.
> 
> Is my search bad?  Are you planning to add additional instances
> of reading "model" then "compatible"?
> 
> -Frank
> 
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
>>  include/linux/of.h |  6 ++++++
>>  2 files changed, 38 insertions(+)
>>
>> Hi Rob,
>>
>> It would be good if we can target this for v4.10, so that we have no
>> dependencies to push PATCH 2/2 in v4.11
>>
>> Regards,
>> Sudeep
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index a0bccb54a9bd..0810c5ecf1aa 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -546,6 +546,38 @@ int of_machine_is_compatible(const char *compat)
>>  EXPORT_SYMBOL(of_machine_is_compatible);
>>
>>  /**
>> + * of_machine_get_model_name - Find and read the model name or the compatible
>> + *		value for the machine.
>> + * @model:	pointer to null terminated return string, modified only if
>> + *		return value is 0.
>> + *
>> + * Returns a string containing either the model name or the compatible value
>> + * of the machine if found, else return error.
>> + *
>> + * Search for a machine model name or the compatible if model name is missing
>> + * in a device tree node and retrieve a null terminated string value (pointer
>> + * to data, not a copy). Returns 0 on success, -EINVAL if root of the device
>> + * tree is not found and other error returned by of_property_read_string on
>> + * failure.
>> + */
>> +int of_machine_get_model_name(const char **model)
>> +{
>> +	int error;
>> +
>> +	if (!of_node_get(of_root))
>> +		return -EINVAL;
>> +
>> +	error = of_property_read_string(of_root, "model", model);
>> +	if (error)
>> +		error = of_property_read_string_index(of_root, "compatible",
>> +						      0, model);
>> +	of_node_put(of_root);
>> +
>> +	return error;
>> +}
>> +EXPORT_SYMBOL(of_machine_get_model_name);
>> +
>> +/**
>>   *  __of_device_is_available - check if a device is available for use
>>   *
>>   *  @device: Node to check for availability, with locks already held
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index d72f01009297..13fc66531f1b 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -367,6 +367,7 @@ extern int of_alias_get_id(struct device_node *np, const char *stem);
>>  extern int of_alias_get_highest_id(const char *stem);
>>
>>  extern int of_machine_is_compatible(const char *compat);
>> +extern int of_machine_get_model_name(const char **model);
>>
>>  extern int of_add_property(struct device_node *np, struct property *prop);
>>  extern int of_remove_property(struct device_node *np, struct property *prop);
>> @@ -788,6 +789,11 @@ static inline int of_machine_is_compatible(const char *compat)
>>  	return 0;
>>  }
>>
>> +static inline int of_machine_get_model_name(const char **model)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>>  static inline bool of_console_check(const struct device_node *dn, const char *name, int index)
>>  {
>>  	return false;
>> --
>> 2.7.4
>>
>>
> 
> 

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-17 21:00 ` [PATCH 1/2] of: base: add support to get machine model name Frank Rowand
  2016-11-17 22:12   ` Frank Rowand
@ 2016-11-18 10:41   ` Sudeep Holla
  2016-11-18 20:22     ` Frank Rowand
  1 sibling, 1 reply; 21+ messages in thread
From: Sudeep Holla @ 2016-11-18 10:41 UTC (permalink / raw)
  To: Frank Rowand, linux-kernel
  Cc: Sudeep Holla, Rob Herring, Arnd Bergmann, devicetree



On 17/11/16 21:00, Frank Rowand wrote:
> On 11/17/16 07:32, Sudeep Holla wrote:
>> Currently platforms/drivers needing to get the machine model name are
>> replicating the same snippet of code. In some case, the OF reference
>> counting is either missing or incorrect.
>>
>> This patch adds support to read the machine model name either using
>> the "model" or the "compatible" property in the device tree root node
>> to the core OF/DT code.
>>
>> This can be used to remove all the duplicate code snippets doing exactly
>> same thing later.
>
> I find five instances of reading only property "model":
>
>   arch/arm/mach-imx/cpu.c
>   arch/arm/mach-mxs/mach-mxs.c
>   arch/c6x/kernel/setup.c
>   arch/mips/cavium-octeon/setup.c
>   arch/sh/boards/of-generic.c
>

Ah sorry you were not Cc-ed in 2/2, but that shows all the instances
that this will be used for.

> I find one instance of reading property "model", then if
> that does not exist, property "compatible":
>
>   arch/mips/generic/proc.c
>

Correct as you can check in patch 2/2

> The proposed patch matches the code used in one place, and thus
> current usage does not match the patch description.
>

Yes, but does it matter ? compatibles are somewhat informative about the
model IMO.

> Is my search bad?  Are you planning to add additional instances
> of reading "model" then "compatible"?
>

No, just replacing the existing ones as in patch 2/2

-- 
Regards,
Sudeep

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-17 15:32 [PATCH 1/2] of: base: add support to get machine model name Sudeep Holla
  2016-11-17 15:32 ` [PATCH 2/2] of: base: replace all duplicate code with of_machine_get_model_name Sudeep Holla
  2016-11-17 21:00 ` [PATCH 1/2] of: base: add support to get machine model name Frank Rowand
@ 2016-11-18 14:46 ` Rob Herring
  2016-11-18 20:00   ` Frank Rowand
  2 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2016-11-18 14:46 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-kernel, Arnd Bergmann, devicetree, Frank Rowand

On Thu, Nov 17, 2016 at 03:32:54PM +0000, Sudeep Holla wrote:
> Currently platforms/drivers needing to get the machine model name are
> replicating the same snippet of code. In some case, the OF reference
> counting is either missing or incorrect.
> 
> This patch adds support to read the machine model name either using
> the "model" or the "compatible" property in the device tree root node
> to the core OF/DT code.
> 
> This can be used to remove all the duplicate code snippets doing exactly
> same thing later.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
>  include/linux/of.h |  6 ++++++
>  2 files changed, 38 insertions(+)
> 
> Hi Rob,
> 
> It would be good if we can target this for v4.10, so that we have no
> dependencies to push PATCH 2/2 in v4.11

Applied.

Rob

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-18 14:46 ` Rob Herring
@ 2016-11-18 20:00   ` Frank Rowand
  2016-11-22 18:44     ` Frank Rowand
  0 siblings, 1 reply; 21+ messages in thread
From: Frank Rowand @ 2016-11-18 20:00 UTC (permalink / raw)
  To: Rob Herring, Sudeep Holla; +Cc: linux-kernel, Arnd Bergmann, devicetree

On 11/18/16 06:46, Rob Herring wrote:
> On Thu, Nov 17, 2016 at 03:32:54PM +0000, Sudeep Holla wrote:
>> Currently platforms/drivers needing to get the machine model name are
>> replicating the same snippet of code. In some case, the OF reference
>> counting is either missing or incorrect.
>>
>> This patch adds support to read the machine model name either using
>> the "model" or the "compatible" property in the device tree root node
>> to the core OF/DT code.
>>
>> This can be used to remove all the duplicate code snippets doing exactly
>> same thing later.
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
>>  include/linux/of.h |  6 ++++++
>>  2 files changed, 38 insertions(+)
>>
>> Hi Rob,
>>
>> It would be good if we can target this for v4.10, so that we have no
>> dependencies to push PATCH 2/2 in v4.11
> 
> Applied.
> 
> Rob
> 

A little fast on the trigger Rob.

-Frank

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-18 10:41   ` Sudeep Holla
@ 2016-11-18 20:22     ` Frank Rowand
  2016-11-21 16:05       ` Frank Rowand
  2016-11-21 16:20       ` Sudeep Holla
  0 siblings, 2 replies; 21+ messages in thread
From: Frank Rowand @ 2016-11-18 20:22 UTC (permalink / raw)
  To: Sudeep Holla, linux-kernel, Rob Herring; +Cc: Arnd Bergmann, devicetree

On 11/18/16 02:41, Sudeep Holla wrote:
> 
> 
> On 17/11/16 21:00, Frank Rowand wrote:
>> On 11/17/16 07:32, Sudeep Holla wrote:
>>> Currently platforms/drivers needing to get the machine model name are
>>> replicating the same snippet of code. In some case, the OF reference
>>> counting is either missing or incorrect.
>>>
>>> This patch adds support to read the machine model name either using
>>> the "model" or the "compatible" property in the device tree root node
>>> to the core OF/DT code.
>>>
>>> This can be used to remove all the duplicate code snippets doing exactly
>>> same thing later.
>>
>> I find five instances of reading only property "model":
>>
>>   arch/arm/mach-imx/cpu.c
>>   arch/arm/mach-mxs/mach-mxs.c
>>   arch/c6x/kernel/setup.c
>>   arch/mips/cavium-octeon/setup.c
>>   arch/sh/boards/of-generic.c
>>
> 
> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances
> that this will be used for.

I have not seen 2/2.  I do not see it on the devicetree list or on lkml.

I did see a list of drivers in the RFC patch that you sent several hours
before this patch.

In that patch you replaced reading the model name from the _flat_ device
tree with the new function in at least one location.  That is not
correct.


> 
>> I find one instance of reading property "model", then if
>> that does not exist, property "compatible":
>>
>>   arch/mips/generic/proc.c
>>
> 
> Correct as you can check in patch 2/2
> 
>> The proposed patch matches the code used in one place, and thus
>> current usage does not match the patch description.
>>
> 
> Yes, but does it matter ? compatibles are somewhat informative about the
> model IMO.

Yes it does matter.  That is just sloppy and makes devicetree yet harder
to understand.  It hurts clarity.  The new function name says get "model",
not get "model" or "first element of the compatible list".

And using the _first_ element only of the compatible list to determine
model is not a good paradigm.  It is yet another hidden, special case,
undocumented trap to lure in the unwary.

It is extremely unlikely that the change actually changes behavior for an
existing device tree because there is probably no dts that does not
contain the model property but does contain the proper magic value in
the compatible property.  But did you actually check for that?

> 
>> Is my search bad?  Are you planning to add additional instances
>> of reading "model" then "compatible"?
>>
> 
> No, just replacing the existing ones as in patch 2/2
> 

You also ignored Arnd's comment in reply to your RFC patch.

-Frank

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-18 20:22     ` Frank Rowand
@ 2016-11-21 16:05       ` Frank Rowand
  2016-11-21 16:23         ` Sudeep Holla
  2016-11-21 16:20       ` Sudeep Holla
  1 sibling, 1 reply; 21+ messages in thread
From: Frank Rowand @ 2016-11-21 16:05 UTC (permalink / raw)
  To: Sudeep Holla, linux-kernel, Rob Herring; +Cc: Arnd Bergmann, devicetree

Hi Sudeep,

On 11/18/16 12:22, Frank Rowand wrote:
> On 11/18/16 02:41, Sudeep Holla wrote:
>>
>>
>> On 17/11/16 21:00, Frank Rowand wrote:
>>> On 11/17/16 07:32, Sudeep Holla wrote:
>>>> Currently platforms/drivers needing to get the machine model name are
>>>> replicating the same snippet of code. In some case, the OF reference
>>>> counting is either missing or incorrect.
>>>>
>>>> This patch adds support to read the machine model name either using
>>>> the "model" or the "compatible" property in the device tree root node
>>>> to the core OF/DT code.
>>>>
>>>> This can be used to remove all the duplicate code snippets doing exactly
>>>> same thing later.
>>>
>>> I find five instances of reading only property "model":
>>>
>>>   arch/arm/mach-imx/cpu.c
>>>   arch/arm/mach-mxs/mach-mxs.c
>>>   arch/c6x/kernel/setup.c
>>>   arch/mips/cavium-octeon/setup.c
>>>   arch/sh/boards/of-generic.c
>>>
>>
>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances
>> that this will be used for.
> 
> I have not seen 2/2.  I do not see it on the devicetree list or on lkml.

Can you please re-send patch 2/2?

-Frank

> 
> I did see a list of drivers in the RFC patch that you sent several hours
> before this patch.
> 
> In that patch you replaced reading the model name from the _flat_ device
> tree with the new function in at least one location.  That is not
> correct.
> 
> 
>>
>>> I find one instance of reading property "model", then if
>>> that does not exist, property "compatible":
>>>
>>>   arch/mips/generic/proc.c
>>>
>>
>> Correct as you can check in patch 2/2
>>
>>> The proposed patch matches the code used in one place, and thus
>>> current usage does not match the patch description.
>>>
>>
>> Yes, but does it matter ? compatibles are somewhat informative about the
>> model IMO.
> 
> Yes it does matter.  That is just sloppy and makes devicetree yet harder
> to understand.  It hurts clarity.  The new function name says get "model",
> not get "model" or "first element of the compatible list".
> 
> And using the _first_ element only of the compatible list to determine
> model is not a good paradigm.  It is yet another hidden, special case,
> undocumented trap to lure in the unwary.
> 
> It is extremely unlikely that the change actually changes behavior for an
> existing device tree because there is probably no dts that does not
> contain the model property but does contain the proper magic value in
> the compatible property.  But did you actually check for that?
> 
>>
>>> Is my search bad?  Are you planning to add additional instances
>>> of reading "model" then "compatible"?
>>>
>>
>> No, just replacing the existing ones as in patch 2/2
>>
> 
> You also ignored Arnd's comment in reply to your RFC patch.
> 
> -Frank
> 

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-18 20:22     ` Frank Rowand
  2016-11-21 16:05       ` Frank Rowand
@ 2016-11-21 16:20       ` Sudeep Holla
  2016-11-21 20:21         ` Frank Rowand
  1 sibling, 1 reply; 21+ messages in thread
From: Sudeep Holla @ 2016-11-21 16:20 UTC (permalink / raw)
  To: Frank Rowand
  Cc: linux-kernel, Rob Herring, Sudeep Holla, Arnd Bergmann, devicetree



On 18/11/16 20:22, Frank Rowand wrote:
> On 11/18/16 02:41, Sudeep Holla wrote:
>>
>>
>> On 17/11/16 21:00, Frank Rowand wrote:
>>> On 11/17/16 07:32, Sudeep Holla wrote:
>>>> Currently platforms/drivers needing to get the machine model name are
>>>> replicating the same snippet of code. In some case, the OF reference
>>>> counting is either missing or incorrect.
>>>>
>>>> This patch adds support to read the machine model name either using
>>>> the "model" or the "compatible" property in the device tree root node
>>>> to the core OF/DT code.
>>>>
>>>> This can be used to remove all the duplicate code snippets doing exactly
>>>> same thing later.
>>>
>>> I find five instances of reading only property "model":
>>>
>>>   arch/arm/mach-imx/cpu.c
>>>   arch/arm/mach-mxs/mach-mxs.c
>>>   arch/c6x/kernel/setup.c
>>>   arch/mips/cavium-octeon/setup.c
>>>   arch/sh/boards/of-generic.c
>>>
>>
>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances
>> that this will be used for.
>
> I have not seen 2/2.  I do not see it on the devicetree list or on lkml.
>

Yes on both [1][2]

> I did see a list of drivers in the RFC patch that you sent several hours
> before this patch.
>
> In that patch you replaced reading the model name from the _flat_ device
> tree with the new function in at least one location.  That is not
> correct.
>
>
>>
>>> I find one instance of reading property "model", then if
>>> that does not exist, property "compatible":
>>>
>>>   arch/mips/generic/proc.c
>>>
>>
>> Correct as you can check in patch 2/2
>>
>>> The proposed patch matches the code used in one place, and thus
>>> current usage does not match the patch description.
>>>
>>
>> Yes, but does it matter ? compatibles are somewhat informative about the
>> model IMO.
>
> Yes it does matter.  That is just sloppy and makes devicetree yet harder
> to understand.  It hurts clarity.  The new function name says get "model",
> not get "model" or "first element of the compatible list".
>

This is a implementation in the Linux and it doesn't change anything in
DT semantics. I am not able to get your concern.

> And using the _first_ element only of the compatible list to determine
> model is not a good paradigm.  It is yet another hidden, special case,
> undocumented trap to lure in the unwary.
>

The function is documented and again this doesn't enforce anything in 
the bindings. It's just the way it's used by the Linux kernel.

[...]

>
> You also ignored Arnd's comment in reply to your RFC patch.
>

OK, all I can see is that Arnd wanted to reuse of_root, which I did.
Did I miss anything else ?

-- 
Regards,
Sudeep

[1] http://marc.info/?l=linux-kernel&m=147940586616629&w=2
[2] http://marc.info/?l=linux-kernel&m=147940575116579&w=2

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-21 16:05       ` Frank Rowand
@ 2016-11-21 16:23         ` Sudeep Holla
  2016-11-21 19:24           ` Frank Rowand
  0 siblings, 1 reply; 21+ messages in thread
From: Sudeep Holla @ 2016-11-21 16:23 UTC (permalink / raw)
  To: Frank Rowand
  Cc: linux-kernel, Rob Herring, Sudeep Holla, Arnd Bergmann, devicetree



On 21/11/16 16:05, Frank Rowand wrote:
> Hi Sudeep,
>
> On 11/18/16 12:22, Frank Rowand wrote:
>> On 11/18/16 02:41, Sudeep Holla wrote:
>>>
>>>
>>> On 17/11/16 21:00, Frank Rowand wrote:
>>>> On 11/17/16 07:32, Sudeep Holla wrote:
>>>>> Currently platforms/drivers needing to get the machine model name are
>>>>> replicating the same snippet of code. In some case, the OF reference
>>>>> counting is either missing or incorrect.
>>>>>
>>>>> This patch adds support to read the machine model name either using
>>>>> the "model" or the "compatible" property in the device tree root node
>>>>> to the core OF/DT code.
>>>>>
>>>>> This can be used to remove all the duplicate code snippets doing exactly
>>>>> same thing later.
>>>>
>>>> I find five instances of reading only property "model":
>>>>
>>>>   arch/arm/mach-imx/cpu.c
>>>>   arch/arm/mach-mxs/mach-mxs.c
>>>>   arch/c6x/kernel/setup.c
>>>>   arch/mips/cavium-octeon/setup.c
>>>>   arch/sh/boards/of-generic.c
>>>>
>>>
>>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances
>>> that this will be used for.
>>
>> I have not seen 2/2.  I do not see it on the devicetree list or on lkml.
>
> Can you please re-send patch 2/2?
>

Since it is based on -next, I would prefer to wait until next merge
window to resend. You should be able to check in the link I sent if
that's OK.

-- 
Regards,
Sudeep

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-21 16:23         ` Sudeep Holla
@ 2016-11-21 19:24           ` Frank Rowand
  2016-11-21 20:49             ` Frank Rowand
  0 siblings, 1 reply; 21+ messages in thread
From: Frank Rowand @ 2016-11-21 19:24 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-kernel, Rob Herring, Arnd Bergmann, devicetree

On 11/21/16 08:23, Sudeep Holla wrote:
> 
> 
> On 21/11/16 16:05, Frank Rowand wrote:
>> Hi Sudeep,
>>
>> On 11/18/16 12:22, Frank Rowand wrote:
>>> On 11/18/16 02:41, Sudeep Holla wrote:
>>>>
>>>>
>>>> On 17/11/16 21:00, Frank Rowand wrote:
>>>>> On 11/17/16 07:32, Sudeep Holla wrote:
>>>>>> Currently platforms/drivers needing to get the machine model name are
>>>>>> replicating the same snippet of code. In some case, the OF reference
>>>>>> counting is either missing or incorrect.
>>>>>>
>>>>>> This patch adds support to read the machine model name either using
>>>>>> the "model" or the "compatible" property in the device tree root node
>>>>>> to the core OF/DT code.
>>>>>>
>>>>>> This can be used to remove all the duplicate code snippets doing exactly
>>>>>> same thing later.
>>>>>
>>>>> I find five instances of reading only property "model":
>>>>>
>>>>>   arch/arm/mach-imx/cpu.c
>>>>>   arch/arm/mach-mxs/mach-mxs.c
>>>>>   arch/c6x/kernel/setup.c
>>>>>   arch/mips/cavium-octeon/setup.c
>>>>>   arch/sh/boards/of-generic.c
>>>>>
>>>>
>>>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances
>>>> that this will be used for.
>>>
>>> I have not seen 2/2.  I do not see it on the devicetree list or on lkml.
>>
>> Can you please re-send patch 2/2?
>>
> 
> Since it is based on -next, I would prefer to wait until next merge
> window to resend. You should be able to check in the link I sent if
> that's OK.

I am missing or misunderstanding something.

I do not know what "the link I sent" means.

For some reason, the devicetree mail list and lmkl mail failed to send
me a copy of patch 2/2.  Or my mail server failed to receive them.  That
is why I asked you to resend the patch. I just now looked in the devicetree
archive and found it there.

So I now can see how you plan to use the new function.

-Frank

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-21 16:20       ` Sudeep Holla
@ 2016-11-21 20:21         ` Frank Rowand
  0 siblings, 0 replies; 21+ messages in thread
From: Frank Rowand @ 2016-11-21 20:21 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-kernel, Rob Herring, Arnd Bergmann, devicetree

On 11/21/16 08:20, Sudeep Holla wrote:
> 
> 
> On 18/11/16 20:22, Frank Rowand wrote:
>> On 11/18/16 02:41, Sudeep Holla wrote:
>>>
>>>
>>> On 17/11/16 21:00, Frank Rowand wrote:
>>>> On 11/17/16 07:32, Sudeep Holla wrote:
>>>>> Currently platforms/drivers needing to get the machine model name are
>>>>> replicating the same snippet of code. In some case, the OF reference
>>>>> counting is either missing or incorrect.
>>>>>
>>>>> This patch adds support to read the machine model name either using
>>>>> the "model" or the "compatible" property in the device tree root node
>>>>> to the core OF/DT code.
>>>>>
>>>>> This can be used to remove all the duplicate code snippets doing exactly
>>>>> same thing later.
>>>>
>>>> I find five instances of reading only property "model":
>>>>
>>>>   arch/arm/mach-imx/cpu.c
>>>>   arch/arm/mach-mxs/mach-mxs.c
>>>>   arch/c6x/kernel/setup.c
>>>>   arch/mips/cavium-octeon/setup.c
>>>>   arch/sh/boards/of-generic.c
>>>>
>>>
>>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances
>>> that this will be used for.
>>
>> I have not seen 2/2.  I do not see it on the devicetree list or on lkml.
>>
> 
> Yes on both [1][2]
> 
>> I did see a list of drivers in the RFC patch that you sent several hours
>> before this patch.
>>
>> In that patch you replaced reading the model name from the _flat_ device
>> tree with the new function in at least one location.  That is not
>> correct.
>>
>>
>>>
>>>> I find one instance of reading property "model", then if
>>>> that does not exist, property "compatible":
>>>>
>>>>   arch/mips/generic/proc.c

Just for completeness, now that I have seen patch 2/2, there is a
second location that currently uses "compatible" if "model" does
not exist: drivers/soc/fsl/guts.c

>>>>
>>>
>>> Correct as you can check in patch 2/2
>>>
>>>> The proposed patch matches the code used in one place, and thus
>>>> current usage does not match the patch description.
>>>>
>>>
>>> Yes, but does it matter ? compatibles are somewhat informative about the
>>> model IMO.
>>
>> Yes it does matter.  That is just sloppy and makes devicetree yet harder
>> to understand.  It hurts clarity.  The new function name says get "model",
>> not get "model" or "first element of the compatible list".

An example of a function name that would not hurt clarity would be
of_model_or_1st_compatible().


>>
> 
> This is a implementation in the Linux and it doesn't change anything in
> DT semantics. I am not able to get your concern.

The existing code in five locations that patch 2/2 changes only attempt
to read the value of property "model".  Changing those five locations
to use of_machine_get_model_name() results in those locations using the
first string of the "compatible" property if "model" does not exist.

The value found is potentially used to determine whether to execute
model specific code.  An example of this is: octeon_pcie_pcibios_map_irq().
Can you guarantee that there is no device tree that does not contain
a "model" property in the root node, but does contains a "compatible"
property in the root node whose first value is "EBH5600"?

I have pasted the relevant code from octeon_pcie_pcibios_map_irq()
below for convenient reference:

int __init octeon_pcie_pcibios_map_irq(const struct pci_dev *dev,
                                       u8 slot, u8 pin)
{
        /*
         * The EBH5600 board with the PCI to PCIe bridge mistakenly
         * wires the first slot for both device id 2 and interrupt
         * A. According to the PCI spec, device id 2 should be C. The
         * following kludge attempts to fix this.
         */
        if (strstr(octeon_board_type_string(), "EBH5600") &&
            dev->bus && dev->bus->parent) {

My point is that it is not possible to review patch 2/2 to verify whether
any change in kernel behavior results from the change, because we do not
have access to all device tree sources.  patch 2/2 is intended to clean
up code, not to change behavior.


> 
>> And using the _first_ element only of the compatible list to determine
>> model is not a good paradigm.  It is yet another hidden, special case,
>> undocumented trap to lure in the unwary.
>>
> 
> The function is documented and again this doesn't enforce anything in
> the bindings. It's just the way it's used by the Linux kernel.
> 
> [...]
> 
>>
>> You also ignored Arnd's comment in reply to your RFC patch.
>>
> 
> OK, all I can see is that Arnd wanted to reuse of_root, which I did.
> Did I miss anything else ?
> 

My mistake, sorry.  I misread the patch.

-Frank

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-21 19:24           ` Frank Rowand
@ 2016-11-21 20:49             ` Frank Rowand
  0 siblings, 0 replies; 21+ messages in thread
From: Frank Rowand @ 2016-11-21 20:49 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-kernel, Rob Herring, Arnd Bergmann, devicetree

On 11/21/16 11:24, Frank Rowand wrote:
> On 11/21/16 08:23, Sudeep Holla wrote:
>>
>>
>> On 21/11/16 16:05, Frank Rowand wrote:
>>> Hi Sudeep,
>>>
>>> On 11/18/16 12:22, Frank Rowand wrote:
>>>> On 11/18/16 02:41, Sudeep Holla wrote:
>>>>>
>>>>>
>>>>> On 17/11/16 21:00, Frank Rowand wrote:
>>>>>> On 11/17/16 07:32, Sudeep Holla wrote:
>>>>>>> Currently platforms/drivers needing to get the machine model name are
>>>>>>> replicating the same snippet of code. In some case, the OF reference
>>>>>>> counting is either missing or incorrect.
>>>>>>>
>>>>>>> This patch adds support to read the machine model name either using
>>>>>>> the "model" or the "compatible" property in the device tree root node
>>>>>>> to the core OF/DT code.
>>>>>>>
>>>>>>> This can be used to remove all the duplicate code snippets doing exactly
>>>>>>> same thing later.
>>>>>>
>>>>>> I find five instances of reading only property "model":
>>>>>>
>>>>>>   arch/arm/mach-imx/cpu.c
>>>>>>   arch/arm/mach-mxs/mach-mxs.c
>>>>>>   arch/c6x/kernel/setup.c
>>>>>>   arch/mips/cavium-octeon/setup.c
>>>>>>   arch/sh/boards/of-generic.c
>>>>>>
>>>>>
>>>>> Ah sorry you were not Cc-ed in 2/2, but that shows all the instances
>>>>> that this will be used for.
>>>>
>>>> I have not seen 2/2.  I do not see it on the devicetree list or on lkml.
>>>
>>> Can you please re-send patch 2/2?
>>>
>>
>> Since it is based on -next, I would prefer to wait until next merge
>> window to resend. You should be able to check in the link I sent if
>> that's OK.
> 
> I am missing or misunderstanding something.
> 
> I do not know what "the link I sent" means.

Ah, the links were in the email you sent before this one, but I read this
one first.  Got it now.


> 
> For some reason, the devicetree mail list and lmkl mail failed to send
> me a copy of patch 2/2.  Or my mail server failed to receive them.  That
> is why I asked you to resend the patch. I just now looked in the devicetree
> archive and found it there.
> 
> So I now can see how you plan to use the new function.
> 
> -Frank
> 
> 
> 

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-18 20:00   ` Frank Rowand
@ 2016-11-22 18:44     ` Frank Rowand
  2016-11-22 21:35       ` Rob Herring
  2016-11-23 10:23       ` Sudeep Holla
  0 siblings, 2 replies; 21+ messages in thread
From: Frank Rowand @ 2016-11-22 18:44 UTC (permalink / raw)
  To: Rob Herring, Sudeep Holla; +Cc: linux-kernel, Arnd Bergmann, devicetree

Hi Rob,

On 11/18/16 12:00, Frank Rowand wrote:
> On 11/18/16 06:46, Rob Herring wrote:
>> On Thu, Nov 17, 2016 at 03:32:54PM +0000, Sudeep Holla wrote:
>>> Currently platforms/drivers needing to get the machine model name are
>>> replicating the same snippet of code. In some case, the OF reference
>>> counting is either missing or incorrect.
>>>
>>> This patch adds support to read the machine model name either using
>>> the "model" or the "compatible" property in the device tree root node
>>> to the core OF/DT code.
>>>
>>> This can be used to remove all the duplicate code snippets doing exactly
>>> same thing later.
>>>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>>  drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
>>>  include/linux/of.h |  6 ++++++
>>>  2 files changed, 38 insertions(+)
>>>
>>> Hi Rob,
>>>
>>> It would be good if we can target this for v4.10, so that we have no
>>> dependencies to push PATCH 2/2 in v4.11
>>
>> Applied.
>>
>> Rob
>>
> 
> A little fast on the trigger Rob.
> 
> -Frank

This patch adds a function that leads to conflating the "model" property
and the "compatible" property. This leads to opaque, confusing and unclear
code where ever it is used.   I think it is not good for the device tree
framework to contribute to writing unclear code.

Further, only two of the proposed users of this new function appear to
be proper usage.  I do not think that the small amount of reduced lines
of code is a good trade off for the reduced code clarity and for the
potential for future mis-use of this function.

Can I convince you to revert this patch?

If not, will you accept a patch to change the function name to more
clearly indicate what it does?  (One possible name would be
of_model_or_1st_compatible().)

-Frank

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-22 18:44     ` Frank Rowand
@ 2016-11-22 21:35       ` Rob Herring
  2016-11-23 10:25         ` Sudeep Holla
  2016-11-23 10:23       ` Sudeep Holla
  1 sibling, 1 reply; 21+ messages in thread
From: Rob Herring @ 2016-11-22 21:35 UTC (permalink / raw)
  To: Frank Rowand; +Cc: Sudeep Holla, linux-kernel, Arnd Bergmann, devicetree

On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> Hi Rob,
>
> On 11/18/16 12:00, Frank Rowand wrote:
>> On 11/18/16 06:46, Rob Herring wrote:
>>> On Thu, Nov 17, 2016 at 03:32:54PM +0000, Sudeep Holla wrote:
>>>> Currently platforms/drivers needing to get the machine model name are
>>>> replicating the same snippet of code. In some case, the OF reference
>>>> counting is either missing or incorrect.
>>>>
>>>> This patch adds support to read the machine model name either using
>>>> the "model" or the "compatible" property in the device tree root node
>>>> to the core OF/DT code.
>>>>
>>>> This can be used to remove all the duplicate code snippets doing exactly
>>>> same thing later.
>>>>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>  drivers/of/base.c  | 32 ++++++++++++++++++++++++++++++++
>>>>  include/linux/of.h |  6 ++++++
>>>>  2 files changed, 38 insertions(+)
>>>>
>>>> Hi Rob,
>>>>
>>>> It would be good if we can target this for v4.10, so that we have no
>>>> dependencies to push PATCH 2/2 in v4.11
>>>
>>> Applied.
>>>
>>> Rob
>>>
>>
>> A little fast on the trigger Rob.
>>
>> -Frank
>
> This patch adds a function that leads to conflating the "model" property
> and the "compatible" property. This leads to opaque, confusing and unclear
> code where ever it is used.   I think it is not good for the device tree
> framework to contribute to writing unclear code.
>
> Further, only two of the proposed users of this new function appear to
> be proper usage.  I do not think that the small amount of reduced lines
> of code is a good trade off for the reduced code clarity and for the
> potential for future mis-use of this function.
>
> Can I convince you to revert this patch?

Yes, I will revert.

> If not, will you accept a patch to change the function name to more
> clearly indicate what it does?  (One possible name would be
> of_model_or_1st_compatible().)

I took it as there's already the FDT equivalent function. I don't have
an issue with the name as the purpose is to get the best name string
for the machine which is model if present and most specific compatible
if not. However, any use of it beyond informational purpose is wrong.
For matching purposes, only compatible should be used.

Rob

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-22 18:44     ` Frank Rowand
  2016-11-22 21:35       ` Rob Herring
@ 2016-11-23 10:23       ` Sudeep Holla
  1 sibling, 0 replies; 21+ messages in thread
From: Sudeep Holla @ 2016-11-23 10:23 UTC (permalink / raw)
  To: Frank Rowand
  Cc: Rob Herring, Sudeep Holla, linux-kernel, Arnd Bergmann, devicetree



On 22/11/16 18:44, Frank Rowand wrote:
> Hi Rob,

[...]

>
> This patch adds a function that leads to conflating the "model"
> property and the "compatible" property. This leads to opaque,
> confusing and unclear code where ever it is used.   I think it is
> not good for the device tree framework to contribute to writing
> unclear code.
>

I agree, the main intention of this patch initially was to have a non
flat_* version of of_flat_dt_get_machine_name

> Further, only two of the proposed users of this new function appear
> to be proper usage.  I do not think that the small amount of reduced
> lines of code is a good trade off for the reduced code clarity and
> for the potential for future mis-use of this function.
>

OK, most of the place I found it used for logging/informational purpose
and hence I thought it could replace in places where even compatible is
used. If that's wrong or leads to misuse of this API, then fine we
should not have one.

-- 
Regards,
Sudeep

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-22 21:35       ` Rob Herring
@ 2016-11-23 10:25         ` Sudeep Holla
  2016-12-09 16:03           ` Rob Herring
  0 siblings, 1 reply; 21+ messages in thread
From: Sudeep Holla @ 2016-11-23 10:25 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, Sudeep Holla, linux-kernel, Arnd Bergmann, devicetree



On 22/11/16 21:35, Rob Herring wrote:
> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com> wrote:

[...]

>>
>> This patch adds a function that leads to conflating the "model" property
>> and the "compatible" property. This leads to opaque, confusing and unclear
>> code where ever it is used.   I think it is not good for the device tree
>> framework to contribute to writing unclear code.
>>
>> Further, only two of the proposed users of this new function appear to
>> be proper usage.  I do not think that the small amount of reduced lines
>> of code is a good trade off for the reduced code clarity and for the
>> potential for future mis-use of this function.
>>
>> Can I convince you to revert this patch?
>
> Yes, I will revert.
>
>> If not, will you accept a patch to change the function name to more
>> clearly indicate what it does?  (One possible name would be
>> of_model_or_1st_compatible().)
>
> I took it as there's already the FDT equivalent function.

Yes it was mainly for non of_flat_* replacement for
of_flat_dt_get_machine_name

-- 
Regards,
Sudeep

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-11-23 10:25         ` Sudeep Holla
@ 2016-12-09 16:03           ` Rob Herring
  2016-12-09 23:54             ` Frank Rowand
  0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2016-12-09 16:03 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: Frank Rowand, linux-kernel, Arnd Bergmann, devicetree

On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 22/11/16 21:35, Rob Herring wrote:
>>
>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com>
>> wrote:
>
>
> [...]
>
>>>
>>> This patch adds a function that leads to conflating the "model" property
>>> and the "compatible" property. This leads to opaque, confusing and
>>> unclear
>>> code where ever it is used.   I think it is not good for the device tree
>>> framework to contribute to writing unclear code.
>>>
>>> Further, only two of the proposed users of this new function appear to
>>> be proper usage.  I do not think that the small amount of reduced lines
>>> of code is a good trade off for the reduced code clarity and for the
>>> potential for future mis-use of this function.
>>>
>>> Can I convince you to revert this patch?
>>
>>
>> Yes, I will revert.

I looked at this again and the users. They are all informational, so
I'm not worried if a compatible string could be returned with this
change. The function returns the best name for the machine and having
consistency is a good thing.

I was considering not reverting (as I'd not yet gotten around to it),
but I'm still going to revert for the naming.

>>
>>> If not, will you accept a patch to change the function name to more
>>> clearly indicate what it does?  (One possible name would be
>>> of_model_or_1st_compatible().)
>>
>>
>> I took it as there's already the FDT equivalent function.
>
>
> Yes it was mainly for non of_flat_* replacement for
> of_flat_dt_get_machine_name

I would suggest just of_get_machine_name().

You might also add a fallback to return "unknown", and drop some of
the custom strings. I don't think anyone should care about the actual
string. However, it's an error to have a DT with no model or top level
compatible, so maybe a WARN.

Rob

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-12-09 16:03           ` Rob Herring
@ 2016-12-09 23:54             ` Frank Rowand
  2016-12-12 15:17               ` Rob Herring
  0 siblings, 1 reply; 21+ messages in thread
From: Frank Rowand @ 2016-12-09 23:54 UTC (permalink / raw)
  To: Rob Herring, Sudeep Holla; +Cc: linux-kernel, Arnd Bergmann, devicetree

On 12/09/16 08:03, Rob Herring wrote:
> On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 22/11/16 21:35, Rob Herring wrote:
>>>
>>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com>
>>> wrote:
>>
>>
>> [...]
>>
>>>>
>>>> This patch adds a function that leads to conflating the "model" property
>>>> and the "compatible" property. This leads to opaque, confusing and
>>>> unclear
>>>> code where ever it is used.   I think it is not good for the device tree
>>>> framework to contribute to writing unclear code.
>>>>
>>>> Further, only two of the proposed users of this new function appear to
>>>> be proper usage.  I do not think that the small amount of reduced lines
>>>> of code is a good trade off for the reduced code clarity and for the
>>>> potential for future mis-use of this function.
>>>>
>>>> Can I convince you to revert this patch?
>>>
>>>
>>> Yes, I will revert.
> 
> I looked at this again and the users. They are all informational, so

A comment in the function docbook header stating that the intent of the
returned value is for informational use only would make me happy.

There is at least on proposed use in patch 2/2 that is not just
informational.  init_octeon_system_type() sometimes uses the value of
the model property to create the value of variable octeon_system_type.
octeon_pcie_pcibios_map_irq() checks the value of octeon_system_type
(via the function octeon_board_type_string()) to determine whether
to apply a fixup:

int __init octeon_pcie_pcibios_map_irq(const struct pci_dev *dev,
                                       u8 slot, u8 pin)
{
        /*
         * The EBH5600 board with the PCI to PCIe bridge mistakenly
         * wires the first slot for both device id 2 and interrupt
         * A. According to the PCI spec, device id 2 should be C. The
         * following kludge attempts to fix this.
         */
        if (strstr(octeon_board_type_string(), "EBH5600") &&
            dev->bus && dev->bus->parent) {


> I'm not worried if a compatible string could be returned with this
> change. The function returns the best name for the machine and having
> consistency is a good thing.

> 
> I was considering not reverting (as I'd not yet gotten around to it),
> but I'm still going to revert for the naming.
> 
>>>
>>>> If not, will you accept a patch to change the function name to more
>>>> clearly indicate what it does?  (One possible name would be
>>>> of_model_or_1st_compatible().)
>>>
>>>
>>> I took it as there's already the FDT equivalent function.
>>
>>
>> Yes it was mainly for non of_flat_* replacement for
>> of_flat_dt_get_machine_name
> 
> I would suggest just of_get_machine_name().
> 
> You might also add a fallback to return "unknown", and drop some of
> the custom strings. I don't think anyone should care about the actual
> string. However, it's an error to have a DT with no model or top level
> compatible, so maybe a WARN.

The name and other suggestions sound fine to me.

-Frank

> 
> Rob
> 

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

* Re: [PATCH 1/2] of: base: add support to get machine model name
  2016-12-09 23:54             ` Frank Rowand
@ 2016-12-12 15:17               ` Rob Herring
  0 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2016-12-12 15:17 UTC (permalink / raw)
  To: Frank Rowand; +Cc: Sudeep Holla, linux-kernel, Arnd Bergmann, devicetree

On Fri, Dec 9, 2016 at 5:54 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 12/09/16 08:03, Rob Herring wrote:
>> On Wed, Nov 23, 2016 at 4:25 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>>
>>> On 22/11/16 21:35, Rob Herring wrote:
>>>>
>>>> On Tue, Nov 22, 2016 at 12:44 PM, Frank Rowand <frowand.list@gmail.com>
>>>> wrote:
>>>
>>>
>>> [...]
>>>
>>>>>
>>>>> This patch adds a function that leads to conflating the "model" property
>>>>> and the "compatible" property. This leads to opaque, confusing and
>>>>> unclear
>>>>> code where ever it is used.   I think it is not good for the device tree
>>>>> framework to contribute to writing unclear code.
>>>>>
>>>>> Further, only two of the proposed users of this new function appear to
>>>>> be proper usage.  I do not think that the small amount of reduced lines
>>>>> of code is a good trade off for the reduced code clarity and for the
>>>>> potential for future mis-use of this function.
>>>>>
>>>>> Can I convince you to revert this patch?
>>>>
>>>>
>>>> Yes, I will revert.
>>
>> I looked at this again and the users. They are all informational, so
>
> A comment in the function docbook header stating that the intent of the
> returned value is for informational use only would make me happy.
>
> There is at least on proposed use in patch 2/2 that is not just
> informational.  init_octeon_system_type() sometimes uses the value of
> the model property to create the value of variable octeon_system_type.
> octeon_pcie_pcibios_map_irq() checks the value of octeon_system_type
> (via the function octeon_board_type_string()) to determine whether
> to apply a fixup:
>
> int __init octeon_pcie_pcibios_map_irq(const struct pci_dev *dev,
>                                        u8 slot, u8 pin)
> {
>         /*
>          * The EBH5600 board with the PCI to PCIe bridge mistakenly
>          * wires the first slot for both device id 2 and interrupt
>          * A. According to the PCI spec, device id 2 should be C. The
>          * following kludge attempts to fix this.
>          */
>         if (strstr(octeon_board_type_string(), "EBH5600") &&
>             dev->bus && dev->bus->parent) {

True, it is more than informational, but let's think about what would
have to happen for the change here to matter. We would have to have a
board with no model property. Then we'd have to have a compatible
string of "EBH5600" on a board which is not the same one as model
EBH5600 and wouldn't be valid anyway with no vendor prefix. IMO, this
code should be using of_machine_is_compatible() anyway. MIPS SoC and
board code is a mess anyway. Linus needs to yell at them.

Rob

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

end of thread, other threads:[~2016-12-12 15:17 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-17 15:32 [PATCH 1/2] of: base: add support to get machine model name Sudeep Holla
2016-11-17 15:32 ` [PATCH 2/2] of: base: replace all duplicate code with of_machine_get_model_name Sudeep Holla
2016-11-17 21:00 ` [PATCH 1/2] of: base: add support to get machine model name Frank Rowand
2016-11-17 22:12   ` Frank Rowand
2016-11-18 10:41   ` Sudeep Holla
2016-11-18 20:22     ` Frank Rowand
2016-11-21 16:05       ` Frank Rowand
2016-11-21 16:23         ` Sudeep Holla
2016-11-21 19:24           ` Frank Rowand
2016-11-21 20:49             ` Frank Rowand
2016-11-21 16:20       ` Sudeep Holla
2016-11-21 20:21         ` Frank Rowand
2016-11-18 14:46 ` Rob Herring
2016-11-18 20:00   ` Frank Rowand
2016-11-22 18:44     ` Frank Rowand
2016-11-22 21:35       ` Rob Herring
2016-11-23 10:25         ` Sudeep Holla
2016-12-09 16:03           ` Rob Herring
2016-12-09 23:54             ` Frank Rowand
2016-12-12 15:17               ` Rob Herring
2016-11-23 10:23       ` Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).