[Qemu-devel] [PATCH v2] device_tree: Add support for reading device tree properties

[Qemu-devel] [PATCH v2] device_tree: Add support for reading device tree properties

[Peter Maydell]

Add support for reading device tree properties (both generic
and single-cell ones) to QEMU's convenience wrapper layer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Here's a v2:
 * added qemu_devtree_get_one_cell_from_prop() which reads a single cell from
   a property which is an array of cells
 * NB that qemu_devtree_getprop() isn't implemented in terms of this because
   that would give worse error handling.

I still think that having this new function is misguided:
 * nobody's using it
 * it breaks the current model where functions at the qemu_devtree and
   libfdt levels deal only with entire properties and do not look inside
   them to operate on only part of the property value

But here's a patch so we can argue about something concrete.

 device_tree.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 device_tree.h |    6 ++++++
 2 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index b366fdd..3a8ff13 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -178,6 +178,55 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
     return r;
 }
 
+const void *qemu_devtree_getprop(void *fdt, const char *node_path,
+                                 const char *property, int *lenp)
+{
+    int len;
+    const void *r;
+    if (!lenp) {
+        lenp = &len;
+    }
+    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
+    if (!r) {
+        fprintf(stderr, "%s: Couldn't get %s/%s: %s\n", __func__,
+                node_path, property, fdt_strerror(*lenp));
+        exit(1);
+    }
+    return r;
+}
+
+uint32_t qemu_devtree_get_one_cell_from_prop(void *fdt, const char *node_path,
+                                             const char *property, int idx)
+{
+    int len;
+    const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len);
+    if (len % 4 != 0) {
+        fprintf(stderr, "%s: %s/%s not multiple of 4 bytes long "
+                "(not cells?)\n",
+                __func__, node_path, property);
+        exit(1);
+    }
+    if (len < (idx + 1) * 4) {
+        fprintf(stderr, "%s: %s/%s wrong length to contain %d cells\n",
+                __func__, node_path, property, idx + 1);
+        exit(1);
+    }
+    return be32_to_cpu(p[idx]);
+}
+
+uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
+                                   const char *property)
+{
+    int len;
+    const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len);
+    if (len != 4) {
+        fprintf(stderr, "%s: %s/%s not 4 bytes long (not a cell?)\n",
+                __func__, node_path, property);
+        exit(1);
+    }
+    return be32_to_cpu(*p);
+}
+
 uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
 {
     uint32_t r;
diff --git a/device_tree.h b/device_tree.h
index 2244270..86669ea 100644
--- a/device_tree.h
+++ b/device_tree.h
@@ -28,6 +28,12 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
 int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
                                  const char *property,
                                  const char *target_node_path);
+const void *qemu_devtree_getprop(void *fdt, const char *node_path,
+                                 const char *property, int *lenp);
+uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
+                                   const char *property);
+uint32_t qemu_devtree_get_one_cell_from_prop(void *fdt, const char *node_path,
+                                             const char *property, int idx);
 uint32_t qemu_devtree_get_phandle(void *fdt, const char *path);
 uint32_t qemu_devtree_alloc_phandle(void *fdt);
 int qemu_devtree_nop_node(void *fdt, const char *node_path);
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2] device_tree: Add support for reading device tree properties

[Peter Crosthwaite]

On Tue, Jul 10, 2012 at 11:32 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> Add support for reading device tree properties (both generic
> and single-cell ones) to QEMU's convenience wrapper layer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Here's a v2:
>  * added qemu_devtree_get_one_cell_from_prop() which reads a single cell from
>    a property which is an array of cells
>  * NB that qemu_devtree_getprop() isn't implemented in terms of this because
>    that would give worse error handling.
>
> I still think that having this new function is misguided:
>  * nobody's using it

I wasn't thinking a new dead function. I was suggesting that the one
function you originally proposed was generalised to handle non zero
offsets. Only a small change to what you originally had. Lets not
think about this now though, as its not worth blocking your series
over dead code. Ill fix incrementally when the time comes for me to
push my own dtb work that needs this functionality. But having to
query the whole DTB cell array to get a single property will make a
mess of my code.

>  * it breaks the current model where functions at the qemu_devtree and
>    libfdt levels deal only with entire properties and do not look inside
>    them to operate on only part of the property value

Im not sure this policy is set in stone. I dont see anything
particular evil with querying for part of a property if thats what you
need to do.

Regards,
Peter

>
> But here's a patch so we can argue about something concrete.
>
>  device_tree.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  device_tree.h |    6 ++++++
>  2 files changed, 55 insertions(+), 0 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index b366fdd..3a8ff13 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -178,6 +178,55 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>      return r;
>  }
>
> +const void *qemu_devtree_getprop(void *fdt, const char *node_path,
> +                                 const char *property, int *lenp)
> +{
> +    int len;
> +    const void *r;
> +    if (!lenp) {
> +        lenp = &len;
> +    }
> +    r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp);
> +    if (!r) {
> +        fprintf(stderr, "%s: Couldn't get %s/%s: %s\n", __func__,
> +                node_path, property, fdt_strerror(*lenp));
> +        exit(1);
> +    }
> +    return r;
> +}
> +
> +uint32_t qemu_devtree_get_one_cell_from_prop(void *fdt, const char *node_path,
> +                                             const char *property, int idx)
> +{
> +    int len;
> +    const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len);
> +    if (len % 4 != 0) {
> +        fprintf(stderr, "%s: %s/%s not multiple of 4 bytes long "
> +                "(not cells?)\n",
> +                __func__, node_path, property);
> +        exit(1);
> +    }
> +    if (len < (idx + 1) * 4) {
> +        fprintf(stderr, "%s: %s/%s wrong length to contain %d cells\n",
> +                __func__, node_path, property, idx + 1);
> +        exit(1);
> +    }
> +    return be32_to_cpu(p[idx]);
> +}
> +
> +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
> +                                   const char *property)
> +{
> +    int len;
> +    const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len);
> +    if (len != 4) {
> +        fprintf(stderr, "%s: %s/%s not 4 bytes long (not a cell?)\n",
> +                __func__, node_path, property);
> +        exit(1);
> +    }
> +    return be32_to_cpu(*p);
> +}
> +
>  uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
>  {
>      uint32_t r;
> diff --git a/device_tree.h b/device_tree.h
> index 2244270..86669ea 100644
> --- a/device_tree.h
> +++ b/device_tree.h
> @@ -28,6 +28,12 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>  int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
>                                   const char *property,
>                                   const char *target_node_path);
> +const void *qemu_devtree_getprop(void *fdt, const char *node_path,
> +                                 const char *property, int *lenp);
> +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path,
> +                                   const char *property);
> +uint32_t qemu_devtree_get_one_cell_from_prop(void *fdt, const char *node_path,
> +                                             const char *property, int idx);
>  uint32_t qemu_devtree_get_phandle(void *fdt, const char *path);
>  uint32_t qemu_devtree_alloc_phandle(void *fdt);
>  int qemu_devtree_nop_node(void *fdt, const char *node_path);
> --
> 1.7.1
>

Re: [Qemu-devel] [PATCH v2] device_tree: Add support for reading device tree properties

[Peter Maydell]

On 12 July 2012 01:45, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> On Tue, Jul 10, 2012 at 11:32 PM, Peter Maydell wrote:
>> I still think that having this new function is misguided:
>>  * nobody's using it
>
> I wasn't thinking a new dead function. I was suggesting that the one
> function you originally proposed was generalised to handle non zero
> offsets. Only a small change to what you originally had. Lets not
> think about this now though, as its not worth blocking your series
> over dead code.

Thanks; I appreciate the pragmatism. Do you want to add a
reviewed-by: tag to my v1 patch, then, or was there something
else that I need to change in it?

> Ill fix incrementally when the time comes for me to
> push my own dtb work that needs this functionality. But having to
> query the whole DTB cell array to get a single property will make a
> mess of my code.

Mmm. It'll probably be easier to assess the new/changed API
in the context of something using it anyway.

thanks
-- PMM