From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::229; helo=mail-it0-x229.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x229.google.com (mail-it0-x229.google.com [IPv6:2607:f8b0:4001:c0b::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 86F912215BD91 for ; Tue, 30 Jan 2018 04:58:32 -0800 (PST) Received: by mail-it0-x229.google.com with SMTP id k131so510785ith.4 for ; Tue, 30 Jan 2018 05:04:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=PtnPK/FTW8bV0etxvT0iZWwcDQCs/eEToI6Nx8ozbSc=; b=VI1zkfFMQP89jVYwUSGPNTqknq6PGSJGbC9m6I4LM+Ai10DYMUzGsJJCLsJTkhKHPb /jaZ8KOuoey1UbjQIWOXqe4TvLL5bEcDxZYgJHBzI8ICL4TmuJm5iuFnQc1pFvk2VShJ gH7CxaTPTFxRi6yseXSnKlL3S3z7yZwMTCekw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=PtnPK/FTW8bV0etxvT0iZWwcDQCs/eEToI6Nx8ozbSc=; b=phb/KuMoEHCTwOeXmlVPamSqCGzwIvyQI3OGJINemRJhE7Uo+Q3s2OCiEdkH3pSzAu VgrmhSoIKtoqGSbh/ef+AAmwafj/DjCTzcKfRjvNRMBshbFOzeT6vcJ23JObwNP0fCIy vdRBKlEUFXqR2sF8eBhRhlf2n2jBx4973iiKZZcXGO1F5wovJICoud/3AYHtYPf4KQiF OB9gEhR5pWWZO8KUSkv123m/0fGqpwnNavxCwaxPBPkNXD7qDijRlaO8+/VFJWcesSkK 6bcb7knvRQJ2nMyF6VvAuuUvQ0Kpr/+edWzatY9gQWZZmQsBDjatH4R056tg84J6kMf6 equg== X-Gm-Message-State: AKwxytcajf5TQF/JVo+rbW7JxbOM59QlEfxANT2S/Sdqwn5lLr5EeWae 9PASUTzeWPeJOb937UxAOhbTlDDoeg+J4RMu/Zs1qw== X-Google-Smtp-Source: AH8x2273MUulLyf6LvesB2+NsK+8DNwK1E1t8VDoo9JIHm6LH7CbDmthxxGNmNfu5amIMB1Sb5pJ6LZiC1eRMeyQNmA= X-Received: by 10.36.128.5 with SMTP id g5mr31053535itd.17.1517317446741; Tue, 30 Jan 2018 05:04:06 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.112.13 with HTTP; Tue, 30 Jan 2018 05:04:06 -0800 (PST) In-Reply-To: <20180130125213.gtyhz2nirprz4gjt@bivouac.eciton.net> References: <20180130103240.4669-1-ard.biesheuvel@linaro.org> <20180130110044.ftayhc37kfh6pw27@bivouac.eciton.net> <20180130114738.7m6e46bx2ug7fa63@bivouac.eciton.net> <20180130125213.gtyhz2nirprz4gjt@bivouac.eciton.net> From: Ard Biesheuvel Date: Tue, 30 Jan 2018 13:04:06 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Jan 2018 12:58:33 -0000 Content-Type: text/plain; charset="UTF-8" On 30 January 2018 at 12:52, Leif Lindholm wrote: > On Tue, Jan 30, 2018 at 11:52:41AM +0000, Ard Biesheuvel wrote: >> >> >> +STATIC >> >> >> +EFI_STATUS >> >> >> +EFIAPI >> >> >> +SynQuacerSdMmcCapability ( >> >> >> + IN EFI_HANDLE ControllerHandle, >> >> >> + IN UINT8 Slot, >> >> >> + IN OUT VOID *SdMmcHcSlotCapability >> >> >> + ) >> >> >> +{ >> >> >> + UINT64 Capability; >> >> >> + >> >> >> + if (ControllerHandle != mSdMmcControllerHandle || Slot != 0) { >> >> > >> >> > This test pattern repeats below, does it suggest a macro? >> >> > >> >> >> >> I don't see how that would clear things up tbh, and the pattern occurs >> >> only twice >> >> >> >> #define IS_OUR_QUIRKY_SDMMC_CONTROLLER(Handle, Slot) \ >> >> ((Handle) == mSdMmcControllerHandle && (Slot) == 0) >> >> >> >> if (!IS_OUR_QUIRKY_SDMMC_CONTROLLER(ControllerHandle, Slot) { >> >> return EFI_SUCCESS; >> >> } >> >> >> >> I can change it if you want, or add a comment if the condition is not >> >> self-explanatory enough. >> > >> > It's just an awful lot of logical operations on a single line. >> > 'ControllerHandle != mSdMmcControllerHandle' is reasonably easy to >> > figure out, but '|| Slot != 0' looks a bit random there. >> > >> > A comment would be sufficient. >> > >> > Another option would be to reduce the number of logical operations by >> > two by doing >> > if (ControllerHandle == mSdMmcControllerHandle && Slot == 0) { >> > and doing the body inside the if-statement. >> > >> > That's a bit uglier in the next function, but I would expect it >> > follows the paradigm of "handle most likely case first"? >> > >> >> Actually, come to think of it, the slot number is completely >> redundant. The non-discoverable SDHCI controller we expose only >> implements a single slot, so something is terribly wrong if >> ControllerHandle == mSdMmcControllerHandle && Slot != 0. >> >> So I will reduce the sequence to >> >> if (ControllerHandle != mSdMmcControllerHandle) { >> return EFI_SUCCESS; >> } >> >> ASSERT (Slot == 0); >> >> instead. Ok? > > Sure, that works for me. > With that (in both places), and the indentation fix: > Reviewed-by: Leif Lindholm Thanks. Pushed as c733b7ef291f