From mboxrd@z Thu Jan 1 00:00:00 1970 From: Walter Lozano Date: Tue, 5 Nov 2019 15:12:12 -0300 Subject: [U-Boot] [RFC PATCH] dm: core: Remove libfdt dependency when unnecessary In-Reply-To: References: <20191105152628.24581-1-walter.lozano@collabora.com> Message-ID: <6ca53d97-a214-262a-dd5f-80e32a865ae4@collabora.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Ezequiel, On 5/11/19 13:56, Ezequiel Garcia wrote: > Hello Walter, > > Thanks for the patch. > > On Tue, 5 Nov 2019 at 12:27, Walter Lozano wrote: >> The support of libfdt should only be needed when OF_CONTROL >> is enabled and OF_PLATDATA is not, as in other cases there is no >> DT file to query. >> >> This patch fixes this dependency allowing to save some space. >> > Can you add some more information about the space saving? Sure, I will add some additional information about the static footprint. However according to my understanding the impact depends on how well different drivers supports features like OF_PLATDATA. For instance, in my current configuration it saves 2 KB. > The ./scripts/bloat-o-meter will help you get some info > on static footprint. Thanks for your suggestion. I think you are pointing to a script found in the Linux kernel, right? I also think it could be useful to have a deep understanding on how the static footprint of some .o files changes, but it won't give us much information about the end result of the binary image, which is affected by the dependencies and compiler/linker optimizations. Is this correct? >> Signed-off-by: Walter Lozano >> --- >> drivers/core/ofnode.c | 132 +++++++++++++++++++++++++++++++++++++-- >> include/dm/read.h | 141 ++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 267 insertions(+), 6 deletions(-) >> > You should try to avoid adding #ifdefery to the code. Normally, > the way to ifdef code is by having this in a header: > > #ifdef CONFIG_FOO > int foo_feature_optional(struct foo *priv); > #else > static inline int foo_feature_optional(struct foo *priv) > { > return 0; > } > #endif > > The user of foo_feature_optional shouldn't be bothered with > FOO being enabled or not. Pushing ifdefs away from .c to .h > is a common pattern, well described in a classic old article: > > http://www.literateprogramming.com/ifdefs.pdf > > Do you think you can try to rework the patch to reduce its impact > as much as possible? Thanks for your review and your suggestions, I will be happy to rework this to improve it. The intention of this RFC is to get some feedback about if this is something worth to be added and if this is the right approach. The use of OF_PLATDATA is quite limited as it needs drivers to support it, which will probably be a long process. Enabling it but having some drivers to query DT properties has no sense and requires additional dependencies, like libfdt. Also I think it should be possible to remove some extra components but it will require extra work to avoid break some configurations. > Thanks, > Ezequiel Thanks, Walter