| Message ID | 1322106896-23054-2-git-send-email-sjg@chromium.org |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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 >
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;
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(-)