Patchwork [U-Boot,01/14] fdt: Tidy up a few fdtdec problems

login
register
mail settings
Submitter Simon Glass
Date Nov. 24, 2011, 3:54 a.m.
Message ID <1322106896-23054-2-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/127421/
State New, archived
Headers show

Comments

Simon Glass - Nov. 24, 2011, 3:54 a.m.
This fixes three trivial issues in fdtdec.c:
1. fdtdec_get_is_enabled() doesn't really need a default value
2. The fdt must be word-aligned, since otherwise it will fail on ARM
3. The compat_names[] array is missing its first element

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/fdtdec.h |    8 ++++----
 lib/fdtdec.c     |    7 ++++---
 2 files changed, 8 insertions(+), 7 deletions(-)
Stephen Warren - Nov. 28, 2011, 6:33 p.m.
On 11/23/2011 08:54 PM, Simon Glass wrote:
> This fixes three trivial issues in fdtdec.c:
> 1. fdtdec_get_is_enabled() doesn't really need a default value
> 2. The fdt must be word-aligned, since otherwise it will fail on ARM
> 3. The compat_names[] array is missing its first element

> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
...
>  #define COMPAT(id, name) name
>  static const char * const compat_names[COMPAT_COUNT] = {
> +	COMPAT(UNKNOWN, "<none>"),
>  };

Could you educate me on why that change is necessary? Maybe explain this
in the commit description?

> -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
> +int fdtdec_get_is_enabled(const void *blob, int node)
>  {
>  	const char *cell;
>  
>  	cell = fdt_getprop(blob, node, "status", NULL);
>  	if (cell)
>  		return 0 == strcmp(cell, "ok");
> -	return default_val;
> +	return 1;
>  }

Not that this patch changes this, but isn't "okay" also a legal
"enabled" value, and perhaps even the recommended value? See
http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.
David Gibson - Nov. 29, 2011, 1:10 a.m.
On Mon, Nov 28, 2011 at 11:33:22AM -0700, Stephen Warren wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
> > This fixes three trivial issues in fdtdec.c:
> > 1. fdtdec_get_is_enabled() doesn't really need a default value
> > 2. The fdt must be word-aligned, since otherwise it will fail on ARM
> > 3. The compat_names[] array is missing its first element
> 
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> ...
> >  #define COMPAT(id, name) name
> >  static const char * const compat_names[COMPAT_COUNT] = {
> > +	COMPAT(UNKNOWN, "<none>"),
> >  };
> 
> Could you educate me on why that change is necessary? Maybe explain this
> in the commit description?
> 
> > -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
> > +int fdtdec_get_is_enabled(const void *blob, int node)
> >  {
> >  	const char *cell;
> >  
> >  	cell = fdt_getprop(blob, node, "status", NULL);
> >  	if (cell)
> >  		return 0 == strcmp(cell, "ok");
> > -	return default_val;
> > +	return 1;
> >  }
> 
> Not that this patch changes this, but isn't "okay" also a legal
> "enabled" value, and perhaps even the recommended value? See
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.

Yes.  "okay", not "ok" is the standard value for the status property.
Simon Glass - Dec. 1, 2011, 8:59 p.m.
Hi Stephen,

On Mon, Nov 28, 2011 at 10:33 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> This fixes three trivial issues in fdtdec.c:
>> 1. fdtdec_get_is_enabled() doesn't really need a default value
>> 2. The fdt must be word-aligned, since otherwise it will fail on ARM
>> 3. The compat_names[] array is missing its first element
>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> ...
>>  #define COMPAT(id, name) name
>>  static const char * const compat_names[COMPAT_COUNT] = {
>> +     COMPAT(UNKNOWN, "<none>"),
>>  };
>
> Could you educate me on why that change is necessary? Maybe explain this
> in the commit description?

Yes I will update the description. The first element of the array is
supposed to be an invalid entry (see enum fdt_compat_id).

>
>> -int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
>> +int fdtdec_get_is_enabled(const void *blob, int node)
>>  {
>>       const char *cell;
>>
>>       cell = fdt_getprop(blob, node, "status", NULL);
>>       if (cell)
>>               return 0 == strcmp(cell, "ok");
>> -     return default_val;
>> +     return 1;
>>  }
>
> Not that this patch changes this, but isn't "okay" also a legal
> "enabled" value, and perhaps even the recommended value? See
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-July/006389.html.

Yes, I will change this, thanks.

Regards,
Simon

>
> --
> nvpublic
>

Patch

diff --git a/include/fdtdec.h b/include/fdtdec.h
index d871cdd..9018181 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -112,14 +112,14 @@  s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
  * Checks whether a node is enabled.
  * This looks for a 'status' property. If this exists, then returns 1 if
  * the status is 'ok' and 0 otherwise. If there is no status property,
- * it returns the default value.
+ * it returns 1 on the assumption that anything mentioned should be enabled
+ * by default.
  *
  * @param blob	FDT blob
  * @param node	node to examine
- * @param default_val	default value to return if no 'status' property exists
- * @return integer value 0/1, if found, or default_val if not
+ * @return integer value 0 (not enabled) or 1 (enabled)
  */
-int fdtdec_get_is_enabled(const void *blob, int node, int default_val);
+int fdtdec_get_is_enabled(const void *blob, int node);
 
 /**
  * Checks whether we have a valid fdt available to control U-Boot, and panic
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 0f87163..d1321a8 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -33,6 +33,7 @@  DECLARE_GLOBAL_DATA_PTR;
  */
 #define COMPAT(id, name) name
 static const char * const compat_names[COMPAT_COUNT] = {
+	COMPAT(UNKNOWN, "<none>"),
 };
 
 /**
@@ -84,14 +85,14 @@  s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
 	return default_val;
 }
 
-int fdtdec_get_is_enabled(const void *blob, int node, int default_val)
+int fdtdec_get_is_enabled(const void *blob, int node)
 {
 	const char *cell;
 
 	cell = fdt_getprop(blob, node, "status", NULL);
 	if (cell)
 		return 0 == strcmp(cell, "ok");
-	return default_val;
+	return 1;
 }
 
 enum fdt_compat_id fd_dec_lookup(const void *blob, int node)
@@ -140,7 +141,7 @@  int fdtdec_next_alias(const void *blob, const char *name,
 int fdtdec_check_fdt(void)
 {
 	/* We must have an fdt */
-	if (fdt_check_header(gd->fdt_blob))
+	if (((uintptr_t)gd->fdt_blob & 3) || fdt_check_header(gd->fdt_blob))
 		panic("No valid fdt found - please append one to U-Boot\n"
 			"binary or define CONFIG_OF_EMBED\n");
 	return 0;