Re: [PATCH edk2-platforms v4 31/34] Silicon/Socionext: implement I2C master protocol for SynQuacer I2C

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Nov 10, 2017 at 02:21:24PM +0000, Ard Biesheuvel wrote:
> Add a driver that produces the I2C master protocol on top of the I2C
> controllers that are implemented in the SynQuacer Socionext SoC. Note
> that this supports two modes simultaneously: I2C controllers that are
> only usable at boot time, and usable via the I2C protocol stack, and
> I2C controllers that are dedicated for the RTC or other runtime
> components.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/ComponentName.c     | 185 ++++++
>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/DriverBinding.c     | 238 ++++++++
>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c   | 586 ++++++++++++++++++++
>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.h   | 162 ++++++
>  Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.inf |  59 ++
>  Silicon/Socionext/SynQuacer/SynQuacer.dec                               |   5 +
>  6 files changed, 1235 insertions(+)
> 

<cut>

> diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/DriverBinding.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/DriverBinding.c
> new file mode 100644
> index 000000000000..22e53408f419
> --- /dev/null
> +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/DriverBinding.c
> @@ -0,0 +1,238 @@
> +/** @file
> +
> +  Copyright (c) 2017, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD
> +  License which accompanies this distribution. The full text of the license may
> +  be found at  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "SynQuacerI2cDxe.h"
> +
> +/**
> +  Tests to see if this driver supports a given controller.
> +
> +  @param  This[in]                 A pointer to the EFI_DRIVER_BINDING_PROTOCOL
> +                                   instance.
> +  @param  ControllerHandle[in]     The handle of the controller to test.
> +  @param  RemainingDevicePath[in]  The remaining device path.
> +                                   (Ignored - this is not a bus driver.)
> +
> +  @retval EFI_SUCCESS              The driver supports this controller.
> +  @retval EFI_ALREADY_STARTED      The device specified by ControllerHandle is
> +                                   already being managed by the driver specified
> +                                   by This.
> +  @retval EFI_UNSUPPORTED          The device specified by ControllerHandle is
> +                                   not supported by the driver specified by This.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +SynQuacerI2cDriverBindingSupported (
> +  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
> +  IN EFI_HANDLE                   ControllerHandle,
> +  IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath
> +  )
> +{
> +  NON_DISCOVERABLE_DEVICE    *Dev;
> +  EFI_STATUS                 Status;
> +
> +  //
> +  //  Connect to the USB stack

USB?

<cut>

> diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> new file mode 100644
> index 000000000000..0a2d0528e0a7
> --- /dev/null
> +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.c
> @@ -0,0 +1,586 @@
> +/** @file
> +
> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "SynQuacerI2cDxe.h"
> +
> +#define __DEBUG(x)        do { if (!EfiAtRuntime()) DEBUG (x); } while (0)

Not a huge fan of using __ for this.
Can you call it BOOTTIME_DEBUG?
(This also sounds like a generically useful thing.)

<cut>

> diff --git a/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.h b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.h
> new file mode 100644
> index 000000000000..8c3d7ef67103
> --- /dev/null
> +++ b/Silicon/Socionext/SynQuacer/Drivers/SynQuacerI2cDxe/SynQuacerI2cDxe.h
> @@ -0,0 +1,162 @@
> +/** @file
> +
> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __SYNQUACER_I2C_DXE_H__
> +#define __SYNQUACER_I2C_DXE_H__
> +
> +#include <PiDxe.h>
> +
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/UefiRuntimeLib.h>
> +
> +#include <Protocol/I2cMaster.h>
> +#include <Protocol/NonDiscoverableDevice.h>
> +
> +extern EFI_COMPONENT_NAME2_PROTOCOL gSynQuacerI2cDriverComponentName2;
> +
> +#define SYNQUACER_I2C_SIGNATURE         SIGNATURE_32 ('S', 'I', '2', 'C')
> +#define SYNQUACER_I2C_FROM_THIS(a)      CR (a, SYNQUACER_I2C_MASTER, \

Could we do a ((a), please?
I know this is always meant to be taken on the This pointer, but it
just looks syntactically buggy.

/
    Leif