Hi David, On 13 June 2018 at 00:39, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Wed, Jun 13, 2018 at 12:29:20AM -0600, Simon Glass wrote: > > Hi David, > > > > On 11 June 2018 at 23:51, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > On Wed, Jun 06, 2018 at 03:36:59PM -0600, Simon Glass wrote: > > >> > > >> This adds the sequential-write interface which provides a way to create a > > >> device tree from scratch. It also includes various other improvements and > > >> additions to the existing functionality. > > >> > > >> This requires the following v4 patches: > > >> > > >> pylibfdt: Return string instead of bytearray from getprop() > > >> pylibfdt: Allow reading integer values from properties > > >> > > >> Note: I'm sending this now rather than waiting on comments for patch > > 10/10 > > >> since there is enough rework here to be getting on with. > > > > > > I've now applied most of these. A few I had further comments on. > > > > > > One overall comment that applies to both the sw stuff and your > > > existing rw code. At the moment, you mirror the C interface by > > > requiring explicit resizing of the buffers holding the dt. That's a > > > somewhat awkward interface in C, but we have a reason for it (allowing > > > embedding in limited environments without an allocator). In Python, > > > that interface is even odder, and there's not much reason for it - > > > Python already requires an allocator. > > > > > > So, I'm wondering if it makes sense to change that interface - remove > > > most of the 'size' parameters (except possibly as hints), trap > > > FDT_ERR_NOSPACE errors from all the write functions and resize and > > > retry. > > > > > > I've been thinking of adding an (optional) C interface that does that > > > for ages, though I've never had time to implement it. In python it > > > should be easier to do, though. > > > > It's been on my mind. There are three reasons I haven't gone this way so > > far so far: > > > > 1. It changes what people might expect the functions to do, so might > > introduce confusion (since we use the same names as libfdt) > > I guess, but remember you still have the "raw" C functions available > to Python. This is the class interface on top of that, which can > reasonably be expected to do a little more. > > > 2. It makes it impossible to detect and prevent an expansion of the DT. > > This was just a vague feeling, but I now have a use case for this in the > > U-Boot binman tool. It wants to set up the size of everything including the > > DT so that it can pack the image together and figure out what goes where. > > Then later it wants to update the DT without changing the size, so that it > > doesn't need to repack. So I do want to support that mode of operation. > > That's a pretty solid reason, though. > > > 3. I figured it could be added later > > > > I do feel that the above reasons have less relevance with the > > sequential-write interface, though. > > > > Since you brought it up, how best to implement it? I've tried hard to avoid > > having state in the Fdt class, so for example you have to specify the > > 'quiet' parameter to each method rather than setting it for all methods. > > But an obvious answer is something like Fdt.set_auto_resize(bool). It seems > > easier to justify handling this with a state variable, since people are > > likely to want it one way or the other. > > I wouldn't suggest having it runtime changeable, so I'd make it an > __init__ parameter. In fact I'd probably make the class with the init > parameter internal and just expose two trivial subclasses which have > it either on or off (class Fdt and class FdtResizing, say). OK, I like that idea. > > > Another option would be to add another arg to the methods, e.g. auto_resize: > > > > def setprop_u32(self, nodeoffset, prop_name, val, quiet=(), > > auto_resize=False): > > > > Or perhaps we should support both, with the sw interface defaulting to True > > for this and the rw interface defaulting to False? In that case the default > > value of auto_resize would be None, meaning to do what the current default > > says. > > > > There are also minor questions like how much to expand by each time we run > > out of space > > TBH, I don't think it'll matter much - just adding 1kiB until it works > will probably do fine. > > > and whether to auto-pack at the end for the sw > > interface. > > Well, fdt_finish() does effectively pack anyway. If you want to use > sw, then rw functions, you'll generally need to fdt_open_into() after > you fdt_finish(). > > You would, obviously need a method to return the current size. OK I'll have a crack at this first by updating this patch, and then see what is needed to support this for the rw case. > > Oh.. which makes me realize - your pack() method should probably > truncate self._fdt after calling fdt_pack(). Yes I have a patch for this so I'll tidy it up and send it separately before looking at the above. Regards, Simon -- To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
