From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from metis.ext.pengutronix.de ([2001:6f8:1178:4:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Y0TwD-0005Rx-JZ for barebox@lists.infradead.org; Mon, 15 Dec 2014 11:35:43 +0000 Message-ID: <1418643316.2274.1.camel@pengutronix.de> From: Lucas Stach Date: Mon, 15 Dec 2014 12:35:16 +0100 In-Reply-To: <20141211073614.GB30369@pengutronix.de> References: <1418123146-13235-1-git-send-email-l.stach@pengutronix.de> <20141211073614.GB30369@pengutronix.de> Mime-Version: 1.0 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "barebox" Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 1/5] usb: gadget: fastboot: allow to build without BOOTM To: Sascha Hauer Cc: barebox@lists.infradead.org Am Donnerstag, den 11.12.2014, 08:36 +0100 schrieb Sascha Hauer: > On Tue, Dec 09, 2014 at 12:05:42PM +0100, Lucas Stach wrote: > > This will disable the capability to boot an uploaded image directly, > > but keeps other fastboot functionality. This seems like a valid config. > > > > Fixes: > > In function `do_bootm_on_complete': undefined reference to `bootm_boot' > > > > Signed-off-by: Lucas Stach > > --- > > drivers/usb/gadget/f_fastboot.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c > > index 92a1a218c643..76879db1f15d 100644 > > --- a/drivers/usb/gadget/f_fastboot.c > > +++ b/drivers/usb/gadget/f_fastboot.c > > @@ -653,7 +653,8 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) > > pr_err("Booting failed\n"); > > } > > > > -static void cb_boot(struct usb_ep *ep, struct usb_request *req, const char *opt) > > +static void __maybe_unused cb_boot(struct usb_ep *ep, struct usb_request *req, > > + const char *opt) > > { > > struct f_fastboot *f_fb = req->context; > > > > @@ -857,9 +858,11 @@ static const struct cmd_dispatch_info cmd_dispatch_info[] = { > > }, { > > .cmd = "download:", > > .cb = cb_download, > > +#if defined(CONFIG_BOOTM) > > }, { > > .cmd = "boot", > > .cb = cb_boot, > > +#endif > > Instead of the #ifdef can we do a: > > if (!IS_ENABLED(CONFIG_BOOTM)) { > fastboot_tx_print(f_fb, "FAILCommand not supported"); > return; > } > > in cb_boot? > Actually I like they way I did it in this patch better, as it is explicit about why something is not available. Chasing through the code just to find out why cb_boot returned early with an error seems like obfuscation to me. Also it spreads the same error path through 2 functions which isn't nice if we ever have to change something there. Regards, Lucas -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox