Re: [PATCH edk2-platforms] Silicon/Socionext/SynQuacer: add configurable eMMC support

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 30 January 2018 at 12:52, Leif Lindholm <leif.lindholm@linaro.org> 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 <leif.lindholm@linaro.org>

Thanks. Pushed as c733b7ef291f