[PATCH v1 1/2] cmd: fru: move FRU handling support to common region

[Jae Hyun Yoo]

On 8/1/2022 5:34 AM, Heinrich Schuchardt wrote:
> On 7/29/22 23:54, Jae Hyun Yoo wrote:
>> From: Graeme Gregory <quic_ggregory at 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_ggregory at quicinc.com>
>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo at 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