On Sun, Sep 25, 2011 at 4:04 AM, David Gibson
<da...@gibson.dropbear.id.au> wrote:
> On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
>> Cells of size 8, 16, 32, and 64 bits are supported. The new
>> /bits/ syntax was selected so as to not pollute the reserved
>> keyword space with uint8/uint16/... type names.
>>
>> With this patch the following property assignment:
>>
>> property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
>>
>> is equivalent to:
>>
>> property = <0x12345678 0x0000ffff>;
>>
>> It is now also possible to directly specify a 64 bit literal in a
>> cell list using:
>>
>> property = /bits/ 64 <0xdeadbeef00000000>;
>>
>> It is an error to attempt to store a literal into a cell that is too
>> small to hold the literal, and the compiler will generate an error
>> when it detects this. For instance:
>>
>> property = /bits/ 8 <256>;
>>
>> Will fail to compile. It is also an error to attempt to place a
>> reference in a non 32-bit cell.
>
> So, there's one small but serious error left in the patch, see below.
>
> Other than that, only two things concern me:
>
> 1) Exactly what to call the keyword. /bits/ is better than
> /size/, but I'd still like to stew a bit on it to see if we can come
> up with anything better.
Sounds good. I have a slight preference for /bits/ over something like
/element-bits/ or /elementbits/. But could be convinced if that was
preferred.
> 2) In the documentation and internal naming, we're using
> the term "cell" ambiguously. In the discussion of this extension
> we've been using "cell" to mean one array element, which is fair
> enough since it's more-or-less standard CS terminology. However, in
> previous FDT and OF terminology "cell" refers to an exactly 32-bit
> quantity [well, to be exact OF old-timers say this isn't strictly
> right in old OF-ese, but it's close enough to meaning that for most
> purposes].
> So, I think we need to find a different name for array cells
> to clarify the terminology. "elements" maybe?
The documentation all talks about arrays of cells. Where as the source code
always calls them lists of cells. I agree that calling them cells once they are
no longer fixed to 32-bits should be avoided. One solution would be to rename
the non-terminal and semantic variables in the parser from celllistprefix and
celllist to arrayprefix and array respectively. I would also change the error
message about the size not being one of the supported sizes to say "element"
instead of "cell".
I could likewise clean up the documentation so that the only mentions of the
cell type were consistent with 32-bit cells. In fact, reading over the
documentation, it looks like a lot of the references to "cell array"
can be turned
into just "array", and the remaining references to "cell" can become "element".
How does this sound?
> [snip]
>> + | celllistprefix DT_REF
>> {
>> - $$ = eval_literal($1, 0, 32);
>> + uint64_t val = ~0ULL >> (64 - $1.bits);
>> +
>> + if ($1.bits != 32)
>> + print_error("References only allowed in 32-bit"
>> + " cell lists");
>> +
>> + $$.data = data_add_marker($1.data, REF_PHANDLE, $2);
>> + $$.data = data_append_integer($$.data, val, $1.bits);
>
> Uh, so, this is actually worse than what you had before. Remember
> print_error() just prints an error, without aborting the parse. So
> now, if bits != 32 it will print the error, then add a phandle marker
> *and* a bits-sized -1. So later, when we go through and fix up the
> REF_PHANDLE markers, we'll assume we're replacing 32-bits of data,
> which could overrun the end of the data if the element size is less
> than 32.
>
> So, basically the data_add_marker() needs to be in an else clause.
Done.
Thanks,
Anton
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
