On 8/1/2022 5:34 AM, Heinrich Schuchardt wrote:
On 7/29/22 23:54, Jae Hyun Yoo wrote:
From: Graeme Gregory <quic_ggreg...@quicinc.com>

The FRU handling was added as a Xilinx board dependent support but it
would be useful for other boards too, so this commit moves the FRU
handling support to the common region so that it can be enabled by
CONFIG_CMD_FRU.

To provide manufacturer specific custom board info field parsing,
it defines 'fru_parse_board_custom' as a weak function so that it can
be replaced with the board specific implementation. In the same way,
OEM Multirecord type (0xc0 - 0xff) parsing logic can be replaced with
a board specific 'fru_parse_multirec' implementation.

Signed-off-by: Graeme Gregory <quic_ggreg...@quicinc.com>
Signed-off-by: Jae Hyun Yoo <quic_jaeh...@quicinc.com>

Your patchset lacks documentation.

Please, add a man-page in doc/usage/cmd/fru.rst.

Okay. I'll add a man-page in v2.

---
Changes from RFC:
  * Made FRU typecode string table as a generic and sharable table. (Michal)   * Made OEM multirecord parsing call happen only on customizable type IDs.
    (Michal)
  * Added manufacturer custom board info fields parsing flow. (Michal)

  board/xilinx/Kconfig                      |   8 --
  board/xilinx/common/Makefile              |   3 -
  board/xilinx/common/board.c               | 108 +++++++++++++++++--
  cmd/Kconfig                               |   8 ++
  cmd/Makefile                              |   1 +
  {board/xilinx/common => cmd}/fru.c        |   5 +-
  common/Makefile                           |   2 +
  {board/xilinx/common => common}/fru_ops.c | 126 ++++++++++++++--------

Why should this live in common/. lib/ would make more sense.

Right, lib/ would be a right place. Will fix it in v2.

[...]

diff --git a/cmd/Kconfig b/cmd/Kconfig
index a8260aa170d0..644c907bf83a 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1053,6 +1053,14 @@ config CMD_FPGAD
        fpga_get_reg() function. This functions similarly to the 'md'
        command.

+config CMD_FRU
+    bool "FRU information for product"
+    help
+      This option enables FRU commands to capture and display FRU
+      information present in the device. The FRU Information is used
+      to primarily to provide "inventory" information about the boards
+      that the FRU Information Device is located on.

Don't assume that a user knows what that FRU abbreviation stands for.
Should it relate to a Field Replaceable Unit, please, write once the
full text.

Right, it relates to a Field Replaceable Unit. I'll add the full text in
the next spin.

Which revision of the IPMI Storage FRU Layout does it support?

It supports v1.0.
https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/ipmi-platform-mgt-fru-info-storage-def-v1-0-rev-1-3-spec-update.pdf

Will add this link into commit message.

Where is the unit test for the command?

I'll add a unit test in v2.

Best Regards,
Jae

Reply via email to