On Thu, Feb 13, 2020 at 8:53 AM Stefan sr@denx.de wrote:
Hi Simon,
On 13.02.20 08:40, Simon Goldschmidt wrote:
Sorry if it seems I ignored this mail yesterday, I just found it now :)
On Wed, Feb 12, 2020 at 10:05 AM Stefan Roese mail@roese.nl wrote:
On 12.02.20 09:57, Weijie Gao wrote:
<snip>
And more general: why do we need to code this in every loader? I think it would be preferrable to have the loader load the binary data and do the decompression (and entry_point assignment) in a central place so that all loaders can benefit from compression. As it is now, we are duplicating code when implementing LZMA in the next loader.
I agree with Simon, that it would make sense to move this code into a even more generic location, so that other "loaders" can also use this feature. I know, that I suggested to add it here and I think we can make this move into the common SPL interface at a later point, after this patch set has been integrated.
My concern is that we add poor code now and that cleanup at a "later point" just gets forgotten.
I understand.
To me, this patch looks like another "get the stuff I need in fast" thing, without thinking about overall code quality. Yes it might be more work to do it properly, but in my opinion, the result will be code that is easier to maintain in the long run.
I fully agree. But I already pushed Weijie to make many enhancements to the code and I fear that this work gets just too complex (time consuming) right now. As this type of generalization is not restricted on this new lzma implementation, but will very likely touch other features as well.
So again, I'm still okay with adding this feature for spi_nor only right now. But if Weijie volunteers to move it to a even more generic location, that would be very welcome of course. ;)
Ok, I'm not the one in charge to decide if it's ok to merge this code.
Regards, Simon
Thanks, Stefan