From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grant Likely Date: Wed, 4 Jul 2007 10:23:53 -0600 Subject: [U-Boot-Users] [PATCH] add: fpga loadi sub command. In-Reply-To: References: Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 7/4/07, eran liberty wrote: > On 7/3/07, Grant Likely wrote: > > I might be missing something here; wouldn't it be better to extend the > > loadmk command? It looks to me that loadmk currently only handles > > uncompressed images, but that it could be fixed to do compressed ones > > too. Correct me if I'm wrong. > > > > You are not wrong. > Still I would like to argue 4 points > > 1. To start with, when i wrote this functionality, loadmk was not > patched in yet... (not a very strong point :) ) > > 2. If we take a closer look at loadmk, its 10 lines long and this same > functionality is within my loadi under the "IH_COMP_NONE" case. (some > what stornger) > > 3. with the addition of the decompression functionality, loadmk will > be too long to be inlined in the the switch - case as it is right now. > It would be exported to a function loadmk_func() and it will be called > from the switch case. That is what loadi does. So you can either > trash the original loadmk or rename my function to loadmk function and > call it from loadmk... the out come is the same, only syntactical > change. (Even stronger) > > 4. I prefer the name loadi over loadmk. (The best argue in the world :) ) Heh; I also hate it when things change underneath me. :-) Regardless, you should consolidate. The name doesn't matter much, and loadmk was in first; so you should stick with loadmk. It's probably fine if you swap in your implementation, but make sure you cc the author of loadmk when you submit the patch. > > > +extern int gunzip (void *dst, int dstlen, unsigned char *src, > > > + unsigned long *lenp); > > > + > > > > Very bad; please don't do this (even if it is done elsewhere). Please > > fix/create the appropriate gzip header file instead. > > > > Agree! > the gunzip function is not exported (defined in the header file) > I wanted not to change any files I had no direct need to. > should I add gunzip declaration to a header file? Add the gunzip decl to the header please. Submit as a separate patch. For extra credit; include a patch to remove all the spurious extern gunzip() decls scattered about the various .c files. :-) > > > + break; > > > + case IH_COMP_GZIP: > > > + if (gunzip > > > + ((char *)CFG_LOAD_ADDR, unc_len, (uchar *) data, &len) > > > + != 0) { > > > > nit: a little ugly; can you split the gunzip call and if() into > > separate lines for easy readability? > > > > not ugly in my book, but can do. :) Heh; only ugly because the if() is longer than 80chars and spills over 3 lines. :-) Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. grant.likely at secretlab.ca (403) 399-0195