Re: [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions.

["Bi, Dandan" <dandan.bi@intel.com>]

Hi Marvin,

How about your write the ECR doc firstly then I ask Intel architect to help submit it to PIWG?

Thanks,
Dandan

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin Häuser
Sent: Tuesday, March 6, 2018 9:32 PM
To: edk2-devel@lists.01.org; Bi, Dandan <dandan.bi@intel.com>
Subject: Re: [edk2] [PATCH 1/5] MdePkg/SPI: Change function definitions to match their descriptions.

Hey Dandan,

Thanks for your reply.
I issued updates for the current headers because I saw activity in edk2-platforms to implement these incomplete protocols and because I haven't heard back for discussed changes in the specification for some time.
Sure I could help submitting the ECR, however I'm not sure how. I'm not an UEFI Contributor and neither did I find any design flaws, just typos and missing definitions, which obviously must be bit values due to their non-exclusive nature.
If you need me to do something concrete, I will try to do my best.

Regards,
Marvin.

> -----Original Message-----
> From: Bi, Dandan <dandan.bi@intel.com>
> Sent: Monday, March 5, 2018 3:15 AM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- 
> devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming 
> <liming.gao@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: RE: [PATCH 1/5] MdePkg/SPI: Change function definitions to 
> match their descriptions.
> 
> Hi Marvin,
> 
> Thank you very much for your contribution. Could you hold this patch series?
> Since current SPI header files follow PI1.6 spec.
> For this case, we should submit an ECR to update the PI spec firstly. 
> After the ECR has been approved by PIWG, then we can send patch to update them.
> Since you have found a lot of missing/incorrect parts in the SPI part 
> of PI Spec. Could you help to submit an ECR to update them?
> 
> 
> Thanks,
> Dandan
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Marvin Häuser
> Sent: Wednesday, February 28, 2018 12:49 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming 
> <liming.gao@intel.com>
> Subject: [edk2] [PATCH 1/5] MdePkg/SPI: Change function definitions to 
> match their descriptions.
> 
> The PI specification is not continuous with function headers and 
> descriptions for the SPI protocols. Correct and comment these mistakes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
>  MdePkg/Include/Protocol/SpiConfiguration.h | 12 ++++++++----
>  MdePkg/Include/Protocol/SpiHc.h            | 14 +++++++++-----
>  MdePkg/Include/Protocol/SpiIo.h            | 15 ++++++++++-----
>  3 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/MdePkg/Include/Protocol/SpiConfiguration.h
> b/MdePkg/Include/Protocol/SpiConfiguration.h
> index c0df183ec7f0..c36a809f4232 100644
> --- a/MdePkg/Include/Protocol/SpiConfiguration.h
> +++ b/MdePkg/Include/Protocol/SpiConfiguration.h
> @@ -1,7 +1,7 @@
>  /** @file
>    This file defines the SPI Configuration Protocol.
> 
> -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2018, Intel Corporation. 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 @@ -65,6 +65,10 @@ EFI_STATUS
>    IN BOOLEAN                   PinValue
>    );
> 
> +//
> +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is
> +//       not conforming to the parameter description.
> +//
>  /**
>    Set up the clock generator to produce the correct clock frequency, 
> phase and
>    polarity for a SPI chip.
> @@ -78,7 +82,7 @@ EFI_STATUS
>                              ClockPhase and ClockPolarity fields. The routine
>                              also has access to the names for the SPI bus and
>                              chip which can be used during debugging.
> -  @param[in] ClockHz        Pointer to the requested clock frequency. The
> clock
> +  @param[in,out] ClockHz    Pointer to the requested clock frequency. The
> clock
>                              generator will choose a supported clock frequency
>                              which is less then or equal to this value.
>                              Specify zero to turn the clock generator off.
> @@ -92,8 +96,8 @@ EFI_STATUS
>  **/
>  typedef EFI_STATUS
>  (EFIAPI *EFI_SPI_CLOCK) (
> -  IN CONST EFI_SPI_PERIPHERAL  *SpiPeripheral,
> -  IN UINT32                    *ClockHz
> +  IN     CONST EFI_SPI_PERIPHERAL  *SpiPeripheral,
> +  IN OUT UINT32                    *ClockHz
>    );
> 
>  ///
> diff --git a/MdePkg/Include/Protocol/SpiHc.h 
> b/MdePkg/Include/Protocol/SpiHc.h index 12fe5d2dce0a..71c75431e4e8
> 100644
> --- a/MdePkg/Include/Protocol/SpiHc.h
> +++ b/MdePkg/Include/Protocol/SpiHc.h
> @@ -1,7 +1,7 @@
>  /** @file
>    This file defines the SPI Host Controller Protocol.
> 
> -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2018, Intel Corporation. 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 @@ -66,6 +66,10 @@ typedef EFI_STATUS
>    IN BOOLEAN                    PinValue
>    );
> 
> +//
> +// Note: In the PI specification, ClockHz is decorated as only 'IN', which is
> +//       not conforming to the parameter description.
> +//
>  /**
>    Set up the clock generator to produce the correct clock frequency, 
> phase and
>    polarity for a SPI chip.
> @@ -80,7 +84,7 @@ typedef EFI_STATUS
>                              ClockPhase and ClockPolarity fields. The routine
>                              also has access to the names for the SPI bus and
>                              chip which can be used during debugging.
> -  @param[in] ClockHz        Pointer to the requested clock frequency. The SPI
> +  @param[in,out] ClockHz    Pointer to the requested clock frequency. The
> SPI
>                              host controller will choose a supported clock
>                              frequency which is less then or equal to this
>                              value. Specify zero to turn the clock 
> generator @@ -94,9 +98,9 @@ typedef EFI_STATUS  **/  typedef 
> EFI_STATUS  (EFIAPI
> *EFI_SPI_HC_PROTOCOL_CLOCK) (
> -  IN CONST EFI_SPI_HC_PROTOCOL  *This,
> -  IN CONST EFI_SPI_PERIPHERAL   *SpiPeripheral,
> -  IN UINT32                      *ClockHz
> +  IN     CONST EFI_SPI_HC_PROTOCOL  *This,
> +  IN     CONST EFI_SPI_PERIPHERAL   *SpiPeripheral,
> +  IN OUT UINT32                     *ClockHz
>    );
> 
>  /**
> diff --git a/MdePkg/Include/Protocol/SpiIo.h 
> b/MdePkg/Include/Protocol/SpiIo.h index 43e804518f8b..8c5d96bb04b2
> 100644
> --- a/MdePkg/Include/Protocol/SpiIo.h
> +++ b/MdePkg/Include/Protocol/SpiIo.h
> @@ -1,7 +1,7 @@
>  /** @file
>    This file defines the SPI I/O Protocol.
> 
> -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2018, Intel Corporation. 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 @@ -144,6 +144,11 @@ EFI_STATUS
>    OUT UINT8                      *ReadBuffer
>    );
> 
> +//
> +// Note: In the PI specification, 'This' is decorated with 'IN' and 'CONST'.
> +//       However, 'This' needs to be updated in order to reflect the
> +//       Peripheral update.
> +//
>  /**
>    Update the SPI peripheral associated with this SPI 10 instance.
> 
> @@ -152,8 +157,8 @@ EFI_STATUS
>    SPI NOR flash parts, where the size and parameters change depending 
> upon
>    device is in the socket.
> 
> -  @param[in] This           Pointer to an EFI_SPI_IO_PROTOCOL structure.
> -  @param[in] SpiPeripheral  Pointer to an EFI_SPI_PERIPHERAL structure.
> +  @param[in,out] This           Pointer to an EFI_SPI_IO_PROTOCOL structure.
> +  @param[in]     SpiPeripheral  Pointer to an EFI_SPI_PERIPHERAL structure.
> 
>    @retval EFI_SUCCESS            The SPI peripheral was updated successfully
>    @retval EFI_INVALID_PARAMETER  The SpiPeripheral value is NULL, @@ 
> -
> 165,8 +170,8 @@ EFI_STATUS  **/  typedef EFI_STATUS  (EFIAPI
> *EFI_SPI_IO_PROTOCOL_UPDATE_SPI_PERIPHERAL) (
> -  IN CONST EFI_SPI_IO_PROTOCOL  *This,
> -  IN CONST EFI_SPI_PERIPHERAL   *SpiPeripheral
> +  IN OUT EFI_SPI_IO_PROTOCOL       *This,
> +  IN     CONST EFI_SPI_PERIPHERAL  *SpiPeripheral
>    );
> 
>  ///
> --
> 2.16.0.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel