Re: [platforms: PATCH 2/4] Marvell/Drivers: MvSpiDxe: Enable using driver in RT

[Marcin Wojtas <mw@semihalf.com>]

Leif

2017-11-09 14:44 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Sun, Nov 05, 2017 at 11:55:37AM +0100, Marcin Wojtas wrote:
>> This patch applies necessary modifications, which allow to use
>> MvSpiDxe driver in variable support as a runtime service.
>> Its type is modified to DXE_RUNTIME_DRIVER, as well as
>> a new callback is introduced as a part of the SpiMasterProtocol.
>> It is supposed to configure the memory space for mmio access to
>> the host controller registers. Moreover gBS locking usage in
>> MvSpiTransfer is limited to the firmware, as the runtime access
>> to the flash is protected within the OS.
>
> Break the commit message up a bit:
> ---
> This patch applies necessary modifications, which allow to use
> MvSpiDxe driver in variable support as a runtime service.
>
> Its type is modified to DXE_RUNTIME_DRIVER, as well as
> a new callback is introduced as a part of the SpiMasterProtocol.
> ---
>
> And then this needs rewording
> ---
> It is supposed to configure the memory space for mmio access to
> the host controller registers.
> ---
> (Say what it does, not what it should be doing.)
>
> ---
> Moreover gBS locking usage in MvSpiTransfer is limited to the
> firmware, as the runtime access to the flash is protected within the
> OS.
> ---
> And "is limited to the firmware". Just because it is used at runtime
> does not make it not firmware. I would say something like:
> "Apply locking in the driver only during boot services. Once at
> runtime, resource protection is handled by the operating system.".
>
> Also, "Its" -> "The driver's" and "It" -> "The driver".
>
> Other than that, can you move the Depex addition here from 4/4 please?
>
> /

Sure.

Thanks,
Marcin