Re: [PATCH 02/14] fdt: Add functions to access phandles, arrays and bools

[Simon Glass <sjg@chromium.org>]

Hi Stephen,

On Mon, Nov 28, 2011 at 10:41 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 11/23/2011 08:54 PM, Simon Glass wrote:
>> Add a function to lookup a property which is a phandle in a node, and
>> another to read a fixed-length integer array from an fdt property.
>> Also add a function to read boolean properties.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Looking at the U-Boot custodians web page, you need to send the core DT
> changes (well, probably anything DT related) to Jerry Van Baren.

Yes the tag was there but not picked up as I didn't have Mike's alias
file in my tree. Will fix.

>
>> +/**
>> + * Look up a property in a node and return its contents in an integer
>> + * array of given length. The property must have at least enough data for
>> + * the array (4*count bytes). It may have more, but this will be ignored.
>> + *
>> + * @param blob               FDT blob
>> + * @param node               node to examine
>> + * @param prop_name  name of property to find
>> + * @param array              array to fill with data
>> + * @param count              number of array elements
>> + * @return 0 if ok, or -FDT_ERR_NOTFOUND if the property is not found,
>> + *           or -FDT_ERR_BADLAYOUT if not enough data
>> + */
>> +int fdtdec_get_int_array(const void *blob, int node, const char *prop_name,
>> +             int *array, int count);
>
> The kernel's equivalent of this function retrieves an array of U32s. Is
> one version more correct than the other?

I would prefer to have signed, but I will change it to use u32 *.

>
>> +/**
>> + * Look up a boolean property in a node and return it.
>> + *
>> + * A boolean properly is true if present in the device tree and false if not
>> + * present, or present with a 0 value.
>> + *
>> + * @param blob       FDT blob
>> + * @param node       node to examine
>> + * @param prop_name  name of property to find
>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>> + */
>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name);
>
> Does U-Boot allow use of the "bool" type here?

Which bool type? It is returning an int.

>
>
>> +/**
>> + * Look up a property in a node and check that it has a minimum length.
>> + *
>> + * @param blob               FDT blob
>> + * @param node               node to examine
>> + * @param prop_name  name of property to find
>> + * @param min_len    minimum property length in bytes
>> + * @param err                0 if ok, or -FDT_ERR_NOTFOUND if the property is not
>> +                     found, or -FDT_ERR_BADLAYOUT if not enough data
>> + * @return pointer to cell, which is only valid if err == 0
>> + */
>> +static const void *get_prop_len(const void *blob, int node,
>> +             const char *prop_name, int min_len, int *err)
>
> Based on the function name, I'd expect it to return the length of the
> property; perhaps get_prop_check_min_len?

Changed, thanks.
>
>> +/**
>> + * Look up a boolean property in a node and return it.
>> + *
>> + * A boolean properly is true if present in the device tree and false if not
>> + * present, or present with a 0 value.
>> + *
>> + * @param blob       FDT blob
>> + * @param node       node to examine
>> + * @param prop_name  name of property to find
>> + * @return 1 if the properly is present; 0 if it isn't present or is 0
>> + */
>> +int fdtdec_get_bool(const void *blob, int node, const char *prop_name)
>> +{
>> +     const s32 *cell;
>> +     int len;
>> +
>> +     debug("%s: %s\n", __func__, prop_name);
>> +     cell = fdt_getprop(blob, node, prop_name, &len);
>> +     if (!cell)
>> +             return 0;
>> +     if (len >= sizeof(u32) && *cell == 0)
>> +             return 0;
>> +
>> +     return 1;
>> +}
>
> In the kernel, I believe that property existence is all that's usually
> checked. Is that wrong? Did the definition of a boolean property's value
> in the function description above come from the specification? If a
> property had a length of 0/1/2/3 with a zero value, it seems very odd to
> treat that as true.

It is useful to be able to set the value to 0 or 1 (with fdtget/put),
rather than remove or add the property. A value with a length of less
than one cell is considered illegal here.

The basic idea is that the presence of the property means that it is
'true'. If it happens to have a value, then we allow that to specify
'false' if it is zero.

Thoughts?

Regards,
Simon

>
> --
> nvpublic
>