[U-Boot] [PATCH 1/2] fdt: Add a fdt_set_node_status function

[U-Boot] [PATCH 1/2] fdt: Add a fdt_set_node_status function

[Chunhe Lan]

Add a fdt_set_node_status function that will set the specified
node's status to the value contained in "status". If the node
doesn't have "status" property that will be created.

Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
---
 common/fdt_support.c  |   17 ++++++++++++++++-
 include/fdt_support.h |    3 +++
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 19b2ef6..384523b 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -2,7 +2,7 @@
  * (C) Copyright 2007
  * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com
  *
- * Copyright 2010 Freescale Semiconductor, Inc.
+ * Copyright 2010-2011 Freescale Semiconductor, Inc.
  *
  * See file CREDITS for list of people who contributed to this
  * project.
@@ -904,6 +904,21 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
 	fdt_delprop(blob, off, alias);
 }
 
+/*
+ * Sets the specified node's status to the value contained in "status".
+ * If the first character of the specified path is "/" then we use
+ * alias as a path. Otherwise, we look for an alias of that name.
+ */
+void fdt_set_node_status(void *fdt, const char *alias, const char *status)
+{
+	const char *path = fdt_get_alias(fdt, alias);
+
+	if (!path)
+		path = alias;
+
+	do_fixup_by_path(fdt, path, "status", status, strlen(status) + 1, 1);
+}
+
 /* Helper to read a big number; size is in cells (not bytes) */
 static inline u64 of_read_number(const __be32 *cell, int size)
 {
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 863024f..74d7c83 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -2,6 +2,8 @@
  * (C) Copyright 2007
  * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com
  *
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ *
  * See file CREDITS for list of people who contributed to this
  * project.
  *
@@ -85,6 +87,7 @@ int fdt_fixup_nor_flash_size(void *blob);
 
 void fdt_fixup_mtdparts(void *fdt, void *node_info, int node_info_size);
 void fdt_del_node_and_alias(void *blob, const char *alias);
+void fdt_set_node_status(void *fdt, const char *alias, const char *status);
 u64 fdt_translate_address(void *blob, int node_offset, const u32 *in_addr);
 int fdt_node_offset_by_compat_reg(void *blob, const char *compat,
 					phys_addr_t compat_off);
-- 
1.5.6.5

[U-Boot] [PATCH 1/2] fdt: Add a fdt_set_node_status function

[Scott Wood]

On 08/11/2011 10:18 PM, Chunhe Lan wrote:
> Add a fdt_set_node_status function that will set the specified
> node's status to the value contained in "status". If the node
> doesn't have "status" property that will be created.
> 
> Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
> ---
>  common/fdt_support.c  |   17 ++++++++++++++++-
>  include/fdt_support.h |    3 +++
>  2 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 19b2ef6..384523b 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -2,7 +2,7 @@
>   * (C) Copyright 2007
>   * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com
>   *
> - * Copyright 2010 Freescale Semiconductor, Inc.
> + * Copyright 2010-2011 Freescale Semiconductor, Inc.
>   *
>   * See file CREDITS for list of people who contributed to this
>   * project.
> @@ -904,6 +904,21 @@ void fdt_del_node_and_alias(void *blob, const char *alias)
>  	fdt_delprop(blob, off, alias);
>  }
>  
> +/*
> + * Sets the specified node's status to the value contained in "status".
> + * If the first character of the specified path is "/" then we use
> + * alias as a path. Otherwise, we look for an alias of that name.
> + */
> +void fdt_set_node_status(void *fdt, const char *alias, const char *status)
> +{
> +	const char *path = fdt_get_alias(fdt, alias);
> +
> +	if (!path)
> +		path = alias;
> +
> +	do_fixup_by_path(fdt, path, "status", status, strlen(status) + 1, 1);
> +}

fdt_get_alias() is not needed -- do_fixup_by_path() calls
fdt_path_offset() which handles aliases.

Instead of something specific to status, how about adding
do_fixup_by_path_string()?

>  /* Helper to read a big number; size is in cells (not bytes) */
>  static inline u64 of_read_number(const __be32 *cell, int size)
>  {
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 863024f..74d7c83 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -2,6 +2,8 @@
>   * (C) Copyright 2007
>   * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com
>   *
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + *
>   * See file CREDITS for list of people who contributed to this
>   * project.
>   *
> @@ -85,6 +87,7 @@ int fdt_fixup_nor_flash_size(void *blob);
>  
>  void fdt_fixup_mtdparts(void *fdt, void *node_info, int node_info_size);
>  void fdt_del_node_and_alias(void *blob, const char *alias);
> +void fdt_set_node_status(void *fdt, const char *alias, const char *status);
>  u64 fdt_translate_address(void *blob, int node_offset, const u32 *in_addr);
>  int fdt_node_offset_by_compat_reg(void *blob, const char *compat,
>  					phys_addr_t compat_off);

A copyright line for a single function prototype?

-Scott

[U-Boot] [PATCH 1/2] fdt: Add a fdt_set_node_status function

[Tabi Timur-B04825]

On Thu, Aug 11, 2011 at 11:18 PM, Chunhe Lan <Chunhe.Lan@freescale.com> wrote:

> +/*
> + * Sets the specified node's status to the value contained in "status".
> + * If the first character of the specified path is "/" then we use
> + * alias as a path. Otherwise, we look for an alias of that name.
> + */
> +void fdt_set_node_status(void *fdt, const char *alias, const char *status)
> +{
> + ? ? ? const char *path = fdt_get_alias(fdt, alias);
> +
> + ? ? ? if (!path)
> + ? ? ? ? ? ? ? path = alias;
> +
> + ? ? ? do_fixup_by_path(fdt, path, "status", status, strlen(status) + 1, 1);
> +}

If you're going to take some internal code and post it upstream, you
should at least use the latest version of that code.  The latest
version of this function has this comment:

/*
 * Given an alias or a path for a node, set the status of that node.
 *
 * If 'alias' is not a valid alias, then it is treated as a full path to the
 * node.  No error checking is performed.
 *
 * This function is normally called to set the status for a virtual MDIO node.
 */

However, since you're moving this function to fdt_support, the last
sentence should be dropped.

> +++ b/include/fdt_support.h
> @@ -2,6 +2,8 @@
> ?* (C) Copyright 2007
> ?* Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com
> ?*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + *

Like Scott said, adding a single function prototype is not sufficient
for a new copyright claim.  You have to add a significant amount of
original code in order to add a new copyright line.


-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 1/2] fdt: Add a fdt_set_node_status function

[Lan Chunhe]

On Mon, 15 Aug 2011 08:08:04 +0800, Tabi Timur-B04825  
<B04825@freescale.com> wrote:

> On Thu, Aug 11, 2011 at 11:18 PM, Chunhe Lan <Chunhe.Lan@freescale.com>  
> wrote:
>
>> +/*
>> + * Sets the specified node's status to the value contained in "status".
>> + * If the first character of the specified path is "/" then we use
>> + * alias as a path. Otherwise, we look for an alias of that name.
>> + */
>> +void fdt_set_node_status(void *fdt, const char *alias, const char  
>> *status)
>> +{
>> +       const char *path = fdt_get_alias(fdt, alias);
>> +
>> +       if (!path)
>> +               path = alias;
>> +
>> +       do_fixup_by_path(fdt, path, "status", status, strlen(status) +  
>> 1, 1);
>> +}
>
> If you're going to take some internal code and post it upstream, you
> should at least use the latest version of that code.  The latest
> version of this function has this comment:
>
> /*
>  * Given an alias or a path for a node, set the status of that node.
>  *
>  * If 'alias' is not a valid alias, then it is treated as a full path to  
> the
>  * node.  No error checking is performed.
>  *
>  * This function is normally called to set the status for a virtual MDIO  
> node.
>  */
>
> However, since you're moving this function to fdt_support, the last
> sentence should be dropped.

    OK.
    Thanks.

>> +++ b/include/fdt_support.h
>> @@ -2,6 +2,8 @@
>>  * (C) Copyright 2007
>>  * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com
>>  *
>> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
>> + *
>
> Like Scott said, adding a single function prototype is not sufficient
> for a new copyright claim.  You have to add a significant amount of
> original code in order to add a new copyright line.

    Thanks for your comments.

    -Jack Lan

[U-Boot] [PATCH 1/2] fdt: Add a fdt_set_node_status function

[Lan Chunhe]

On Sat, 13 Aug 2011 00:24:33 +0800, Scott Wood <scottwood@freescale.com>  
wrote:

> On 08/11/2011 10:18 PM, Chunhe Lan wrote:
>> Add a fdt_set_node_status function that will set the specified
>> node's status to the value contained in "status". If the node
>> doesn't have "status" property that will be created.
>>
>> Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
>> ---
>>  common/fdt_support.c  |   17 ++++++++++++++++-
>>  include/fdt_support.h |    3 +++
>>  2 files changed, 19 insertions(+), 1 deletions(-)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index 19b2ef6..384523b 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -2,7 +2,7 @@
>>   * (C) Copyright 2007
>>   * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com
>>   *
>> - * Copyright 2010 Freescale Semiconductor, Inc.
>> + * Copyright 2010-2011 Freescale Semiconductor, Inc.
>>   *
>>   * See file CREDITS for list of people who contributed to this
>>   * project.
>> @@ -904,6 +904,21 @@ void fdt_del_node_and_alias(void *blob, const char  
>> *alias)
>>  	fdt_delprop(blob, off, alias);
>>  }
>>
>> +/*
>> + * Sets the specified node's status to the value contained in "status".
>> + * If the first character of the specified path is "/" then we use
>> + * alias as a path. Otherwise, we look for an alias of that name.
>> + */
>> +void fdt_set_node_status(void *fdt, const char *alias, const char  
>> *status)
>> +{
>> +	const char *path = fdt_get_alias(fdt, alias);
>> +
>> +	if (!path)
>> +		path = alias;
>> +
>> +	do_fixup_by_path(fdt, path, "status", status, strlen(status) + 1, 1);
>> +}
>
> fdt_get_alias() is not needed -- do_fixup_by_path() calls
> fdt_path_offset() which handles aliases.
>
> Instead of something specific to status, how about adding
> do_fixup_by_path_string()?

    I think that function of do_fixup_by_path should have
    completed that function.

   Here,  we only want to set the status property of node.

   At the same time, Tabi Timur also is using the function
   of fdt_set_node_status.

>>  /* Helper to read a big number; size is in cells (not bytes) */
>>  static inline u64 of_read_number(const __be32 *cell, int size)
>>  {
>> diff --git a/include/fdt_support.h b/include/fdt_support.h
>> index 863024f..74d7c83 100644
>> --- a/include/fdt_support.h
>> +++ b/include/fdt_support.h
>> @@ -2,6 +2,8 @@
>>   * (C) Copyright 2007
>>   * Gerald Van Baren, Custom IDEAS, vanbaren at cideas.com
>>   *
>> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
>> + *
>>   * See file CREDITS for list of people who contributed to this
>>   * project.
>>   *
>> @@ -85,6 +87,7 @@ int fdt_fixup_nor_flash_size(void *blob);
>>
>>  void fdt_fixup_mtdparts(void *fdt, void *node_info, int  
>> node_info_size);
>>  void fdt_del_node_and_alias(void *blob, const char *alias);
>> +void fdt_set_node_status(void *fdt, const char *alias, const char  
>> *status);
>>  u64 fdt_translate_address(void *blob, int node_offset, const u32  
>> *in_addr);
>>  int fdt_node_offset_by_compat_reg(void *blob, const char *compat,
>>  					phys_addr_t compat_off);
>
> A copyright line for a single function prototype?

    Thanks.
    I will remove this line.

   -Jack Lan

[U-Boot] [PATCH 1/2] fdt: Add a fdt_set_node_status function

[Scott Wood]

On 08/14/2011 10:40 PM, Lan Chunhe wrote:
> On Sat, 13 Aug 2011 00:24:33 +0800, Scott Wood <scottwood@freescale.com>
> wrote:
> 
>> fdt_get_alias() is not needed -- do_fixup_by_path() calls
>> fdt_path_offset() which handles aliases.
>>
>> Instead of something specific to status, how about adding
>> do_fixup_by_path_string()?
> 
>    I think that function of do_fixup_by_path should have
>    completed that function.

do_fixup_by_path_string() would just be an inline wrapper for
do_fixup_by_path() that calls strlen on the argument.

And then you could just do:

do_fixup_by_path_string(fdt, "nand_flash", "status", "okay");

I don't see a need for a function dedicated to the status property.

-Scott

[U-Boot] [PATCH 1/2] fdt: Add a fdt_set_node_status function

[Lan Chunhe]

On Tue, 16 Aug 2011 02:12:17 +0800, Scott Wood <scottwood@freescale.com>  
wrote:

> On 08/14/2011 10:40 PM, Lan Chunhe wrote:
>> On Sat, 13 Aug 2011 00:24:33 +0800, Scott Wood <scottwood@freescale.com>
>> wrote:
>>
>>> fdt_get_alias() is not needed -- do_fixup_by_path() calls
>>> fdt_path_offset() which handles aliases.
>>>
>>> Instead of something specific to status, how about adding
>>> do_fixup_by_path_string()?
>>
>>    I think that function of do_fixup_by_path should have
>>    completed that function.
>
> do_fixup_by_path_string() would just be an inline wrapper for
> do_fixup_by_path() that calls strlen on the argument.
>
> And then you could just do:
>
> do_fixup_by_path_string(fdt, "nand_flash", "status", "okay");
>
> I don't see a need for a function dedicated to the status property.

    OK.
    Thanks.

   -Jack Lan