On Fri, Jul 27, 2018 at 09:01:12AM -0600, Rob Herring wrote:
> On Thu, Jul 26, 2018 at 11:06 PM David Gibson
> <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Jul 12, 2018 at 06:20:06PM -0600, Rob Herring wrote:
> > > From: Grant Likely <grant.likely@xxxxxxx>
> > >
> > > YAML encoded DT is useful for validation of DTs using binding schemas.
>
> [...]
>
> > > + yaml_sequence_start_event_initialize(&event, NULL,
> > > + (yaml_char_t *)NULL, 1, YAML_FLOW_SEQUENCE_STYLE);
> > > + if (!yaml_emitter_emit(emitter, &event))
> > > + yaml_die(emitter);
> >
> > For a library that's supposed to make yaml easy, that's pretty damn
> > verbose, but whatever :/.
>
> Yeah, and I tried to get rid of all the yaml_char_t casts, but that's
> just a mess of char signedness issues.
Heh.
> [...]
>
> > > + switch(m->type) {
> > > + case REF_PHANDLE:
> > > + yaml_propval_phandle_args(emitter, data, chunk_len);
> >
> > Handling the phandle and the "args" together seems dubious to me,
> > since that connection isn't really built into the tree structure, just
> > a common binding pattern. Can't you just deal with the phandle, then
> > deal with the next bit using the type markers just as you would
> > anyway?
>
> The args don't have a marker currently (or it is implicit). If we look
> at the data as matrices, the markers are on the whole matrix, not the
> individual elements as we don't support a mixture of sizes:
>
> [ !u32 [ 1, 2, 3 ], [ 4, 5, 6 ] ]
>
> But for phandles, the marker is on the individual element. it looks
> like this (though the !u32 is implicit):
>
> [ !u32 [ !phandle 1, 2, 3 ], [ !phandle 4, 5, 6 ] ]
Right, which seems reasonable. But your handling above wouldn't
handle a case where the phandle was in the middle of the chunk, rather
than the front. I think you can get that with interrupts-map, maybe
other places.
In any case the marker information isn't actually ambiguous here, just
a bit harder to analyze than your current code handles. A TYPE_*
marker applies until the next TYPE_* marker, whatever other markers
are in between. The PHANDLE_REF marker gives you information *on top
of* the current type (which should always be U32 for a phandle ref),
but doesn't affect the applicability of the current type for things
before or after it.
> I somewhat expect this is an area where the YAML format could change
> and we put the actual label/path ref here instead of the phandle
> number. I'm not really sure. I think that's all a ways off in caring
> about for validation. Really, I could drop the !phandle tag for now
> and go back to emitting just raw ints.
That might be best for now
>
> > > + break;
> > > + case TYPE_UINT16:
> > > + yaml_propval_int(emitter, data, chunk_len, 2);
> > > + break;
> > > + case TYPE_UINT32:
> > > + yaml_propval_int(emitter, data, chunk_len, 4);
> > > + break;
> > > + case TYPE_UINT64:
> > > + yaml_propval_int(emitter, data, chunk_len, 8);
> > > + break;
> > > + case REF_PATH:
> > > + case TYPE_STRING:
> >
> > AFAIK a path reference will generate both a REF_PATH and a TYPE_STRING
> > marker, so won't this try to emit something 0-length followed by the
> > actual output you ant?
>
> I don't think so. This was one of the bugs I fixed in Grant's patch.
> Without the REF_PATH line, I just get a byte array. But maybe the real
> bug is is the lack of TYPE_STRING marker?
Yeah, I think so.
--
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
Attachment:
signature.asc
Description: PGP signature
