From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mateusz Zalega Date: Thu, 06 Feb 2014 12:56:58 +0100 Subject: [U-Boot] [PATCH v2 06/12] USB: gadget: added a saner gadget downloader registration API In-Reply-To: <201402051900.47518.marex@denx.de> References: <1389277919-15279-1-git-send-email-m.zalega@samsung.com> <201402050813.17899.marex@denx.de> <52F2313B.2010302@samsung.com> <201402051900.47518.marex@denx.de> Message-ID: <52F3788A.6080705@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 02/05/14 19:00, Marek Vasut wrote: > On Wednesday, February 05, 2014 at 01:40:27 PM, Mateusz Zalega wrote: > > [...] > >>> Are these two new functions called from multiple places at all? If not, >>> just inline these ll_foo() calls and be done with it. FYI you can also >>> make macros for these to avoid having to type all these args all around >>> and duplicating the code. >> >> Macros or static inlines, it's all the same > > NAK, what you say is seriously wrong, you should know that by now! > > Macros do not do any kind of typechecking, functions do typechecking. > Macros are expanded in place during preprocessing, functions are (usually) > single-instance. > > etc. Yeah, and it's all the same if you don't care for typechecking and all that, which I assumed from _you_ proposing usage of macros here. >> there's no point in >> changing that. The symbols aren't visible outside this compilation unit >> and function calls are, well, inlined. > > It's pointless to have them pulled out so explicitly. Or would you prefer to > have each function encapsulated in another function ? This doesn't make does > now, does it ? Pardon? > What I would like to do is for you to follow the advice in linker_lists.h and > produce a small shim over these crude linker lists prototypes there, so that the > users here in the g_* world don't have to type the whole linker list macros with > all the arguments, which do not ever change for this g_* . Does it make sense > please? It's taken care of by static inlines. >>>> static int g_dnl_do_config(struct usb_configuration *c) >>>> { >>>> >>>> const char *s = c->cdev->driver->name; >>>> >>>> - int ret = -1; >>>> >>>> debug("%s: configuration: 0x%p composite dev: 0x%p\n", >>>> >>>> - __func__, c, c->cdev); >>>> - >>>> + __func__, c, c->cdev); >>>> >>>> printf("GADGET DRIVER: %s\n", s); >>>> >>>> - if (!strcmp(s, "usb_dnl_dfu")) >>>> - ret = dfu_add(c); >>>> - else if (!strcmp(s, "usb_dnl_ums")) >>>> - ret = fsg_add(c); >>>> - else if (!strcmp(s, "usb_dnl_thor")) >>>> - ret = thor_add(c); >>>> - >>>> - return ret; >>>> + >>>> + struct g_dnl_bind_callback *callback = g_dnl_first_bind_callback(); >>>> + for (; callback != g_dnl_last_bind_callback(); ++callback) >>> >>> callback++ , this is not C++ where the order might matter. Nonetheless, >>> you do >> >> It doesn't matter anyway and can't be supported on Coding Style grounds, >> don't bug me. > > Can be done on purely statistical grounds, try this: > > $ git grep 'for.*(.* *++[:alnum:]\+ *)' | wc -l > 13 > $ git grep 'for.*(.* *[:alnum:]\+++ *)' | wc -l > 183 > > Please fix, thank you. Okay, whatever. >>> want to use ll_entry_count() and ll_entry_get() with an iterator variable >> >> I don't think using ll_entry_get() in this way is possible with current >> implementation of linker lists. Moreover, there's plenty of code doing >> just the same already accepted to U-Boot. > > Ah meh, sorry. Seems like someone was messing with the linkerlists and > misdesigned it. Dang. $ git show 42eba Yeah, it's a pity. >>> instead to make sure you don't step onto a corrupted field and crash in >>> some weird way. >> >> Linker would have to split sections to make this sort of thing possible. >> Won't happen. > > Can you please elaborate ? > [...] > You're guaranteed by the linker, and our setup, that all your linker-list data will end up in a contiguous block. -- Mateusz Zalega Samsung R&D Institute Poland