Re: [Qemu-devel] [PATCH 5/9] FDT: Add additional access methods for array types and walking children.

[Peter Crosthwaite]

On Sun, 2012-09-09 at 20:20 -0400, address@hidden wrote:
> From: Chris Wulff <address@hidden>
> 
> Signed-off-by: Chris Wulff <address@hidden>
> ---
>  device_tree.c |   88 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  device_tree.h |   18 ++++++++++++
>  2 files changed, 106 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index d7a9b6b..7fa7646 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -27,6 +27,14 @@
>  
>  #include <libfdt.h>
>  
> +#define CHECK_HEADER(fdt) \
> +    { \
> +        int err = fdt_check_header(fdt); \
> +        if (err != 0) { \
> +            return err; \
> +        } \
> +    }
> +

Im thinking the best way (and I have recently done some patches along
this line) is to use the qemu error reporting mechanism. Rather than
return 1, populate an Error ** with an error code. Not a blocker as this
is out of scope of this series, but would make this macro foo go away
with a nice inline function.

>  #define FDT_MAX_SIZE  0x10000
>  
>  void *create_device_tree(int *sizep)
> @@ -304,3 +312,83 @@ int qemu_devtree_add_subnode(void *fdt, const char *name)
>      g_free(dupname);
>      return retval;
>  }
> +
> +int qemu_devtree_node_offset(void *fdt, const char *node_path)
> +{
> +    return fdt_path_offset(fdt, node_path);
> +}

device_tree so far has been able to insulate clients from dealing with
offsets. Good to keep it that way and use node_paths instead. Offsets
have the problem where they are globally invalidated as soon as the
device tree is manipulated.

I have an alternate API extension that does a lot of what you are trying
to do but nodepaths are used as handles to nodes rather than offsets.
Will send patch.

> +
> +int qemu_devtree_subnode_offset_namelen(void *fdt, int parentoffset,
> +                                        const char *name, int namelen)
> +{
> +    return fdt_subnode_offset_namelen(fdt, parentoffset, name, namelen);
> +}

No need for no-operation wrappers, just call into lib-fdt directly. Most
device-tree api calls exit(1) on failure which is why we have these thin
wrappers.

> +
> +int qemu_devtree_next_child_offset(void *fdt, int parentoffset, int 
> childoffset)
> +{
> +    int level = 0;
> +    uint32_t tag;
> +    int offset, nextoffset;
> +
> +    CHECK_HEADER(fdt);
> +    tag = fdt_next_tag(fdt, parentoffset, &nextoffset);
> +    if (tag != FDT_BEGIN_NODE) {
> +        return -FDT_ERR_BADOFFSET;
> +    }
> +
> +    do {
> +        offset = nextoffset;
> +        tag = fdt_next_tag(fdt, offset, &nextoffset);
> +
> +        switch (tag) {
> +        case FDT_END:
> +            return -FDT_ERR_TRUNCATED;
> +
> +        case FDT_BEGIN_NODE:
> +            level++;
> +            if (level != 1) {
> +                continue;
> +            }
> +            if (offset > childoffset) {
> +                return offset;
> +            }
> +            break;
> +
> +        case FDT_END_NODE:
> +            level--;
> +            break;
> +
> +        case FDT_PROP:
> +        case FDT_NOP:
> +            break;
> +
> +        default:
> +            return -FDT_ERR_BADSTRUCTURE;
> +        }
> +    } while (level >= 0);
> +
> +    return -FDT_ERR_NOTFOUND;
> +}
> +
> +const char *qemu_devtree_get_name(const void *fdt, int nodeoffset, int *lenp)
> +{
> +    return fdt_get_name(fdt, nodeoffset, lenp);
> +}
> +
> +const void *qemu_devtree_getprop_offset(const void *fdt, int nodeoffset,
> +                                        const char *name, int *lenp)
> +{
> +    return fdt_getprop(fdt, nodeoffset, name, lenp);
> +}
> +
> +uint32_t qemu_devtree_int_array_index(const void *propval, unsigned int 
> index)
> +{
> +    return be32_to_cpu(((uint32_t *)propval)[index]);
> +}
> +
> +int qemu_devtree_node_check_compatible(const void *fdt, int nodeoffset,
> +                                       const char *compatible)
> +{
> +    return fdt_node_check_compatible(fdt, nodeoffset, compatible);
> +}
> +
> diff --git a/device_tree.h b/device_tree.h
> index f7a3e6c..e01b460 100644
> --- a/device_tree.h
> +++ b/device_tree.h
> @@ -49,4 +49,22 @@ int qemu_devtree_add_subnode(void *fdt, const char *name);
>                               sizeof(qdt_tmp));                               
>  \
>      } while (0)
>  
> +int qemu_devtree_node_offset(void *fdt, const char *node_path);
> +
> +int qemu_devtree_subnode_offset_namelen(void *fdt, int parentoffset,
> +                                        const char *name, int namelen);
> +
> +int qemu_devtree_next_child_offset(void *fdt, int parentoffset,
> +                                   int childoffset);
> +
> +const char *qemu_devtree_get_name(const void *fdt, int nodeoffset, int 
> *lenp);
> +
> +const void *qemu_devtree_getprop_offset(const void *fdt, int nodeoffset,
> +                                        const char *name, int *lenp);
> +
> +uint32_t qemu_devtree_int_array_index(const void *propval, unsigned int 
> index);

This is get_prop_cell but with an index. We debated this on the list
recently (cc PMM) that get_prop_cell should(nt) have an index field into
it that lets you get the non-zeroth property. I have a patch in my tree
that changes this that ill send you along with the rest. 

> +
> +int qemu_devtree_node_check_compatible(const void *fdt, int nodeoffset,
> +                                       const char *compatible);
> +
>  #endif /* __DEVICE_TREE_H__ */

Regards,
Peter