Re: [PATCH v4 2/9] bindings: ipmi: Add binding for IPMB device intf

[Ninad Palsule <ninad@linux.ibm.com>]

Hello Corey,

Thanks for the review.

On 1/14/25 10:46, Corey Minyard wrote:
> On Mon, Jan 13, 2025 at 01:48:12PM -0600, Ninad Palsule wrote:
>> Add device tree binding document for the IPMB device interface.
>> This device is already in use in both driver and .dts files.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   .../devicetree/bindings/ipmi/ipmb-dev.yaml    | 55 +++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ipmi/ipmb-dev.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/ipmi/ipmb-dev.yaml b/Documentation/devicetree/bindings/ipmi/ipmb-dev.yaml
>> new file mode 100644
>> index 000000000000..136806cba632
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ipmi/ipmb-dev.yaml
>> @@ -0,0 +1,55 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/ipmi/ipmb-dev.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: The Intelligent Platform Management Bus(IPMB) Device
>> +
>> +description: |
>> +  The IPMB is an I2C bus which provides interconnection between Baseboard
> "Baseboard -> "a Baseboard"
Fixed.
>
>> +  Management Controller(BMC) and chassis electronics. The BMC sends IPMI
>> +  requests to intelligent controllers like Satellite Management Controller(MC)
>> +  device via IPMB and the device sends a response back to the BMC.
> device -> devices
> "a response" -> responses
Fixed
>
>> +  This device binds backend Satelite MC which is a I2C slave device with the BMC
> You use IPMB devices on both the BMC and the MCs.  The sentence above is
> a little confusing, too.  How about:
>
> This device uses an I2C slave device to send and receive IPMB messages,
> either on a BMC or other MC.
Changed
>
>> +  for management purpose. A miscalleneous device provices a user space program
> Misspelling: miscellaneous
Fixed.
>
>> +  to communicate with kernel and backend device. Some IPMB devices only support
> "kernel" -> "the kernel"
Fixed
>
>> +  I2C protocol instead of SMB protocol.
> the I2C protocol and not the SMB protocol.
>
> Yes, the English language uses way too many articles...
>
> That is a lot of detail, but it looks good beyond what I've commented
> on.
Changed.
>
>> +
>> +  IPMB communications protocol Specification V1.0
>> +  https://www.intel.com/content/dam/www/public/us/en/documents/product-briefs/ipmp-spec-v1.0.pdf
>> +
>> +maintainers:
>> +  - Ninad Palsule <ninad@linux.ibm.com>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ipmb-dev
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  i2c-protocol:
>> +    description:
>> +      Use I2C block transfer instead of SMBUS block transfer.
>> +    type: boolean
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        ipmb-dev@10 {
>> +            compatible = "ipmb-dev";
>> +            reg = <0x10>;
> I'm not sure of the conventions around device tree here, but the reg is
> not used in the driver and it will always be the I2C address that
> already in that node just one level up.  It does not serve any purpose
> that I can see.  My suggestion would be to remove it.

There are some boards already using reg so I will not be able to remove

but I have updated the example which reflects what those boards are doing

  which indicates that node address is ORed with I2C slaved address.

Regards,

Ninad

>
> -corey
>
>> +            i2c-protocol;
>> +        };
>> +    };
>> -- 
>> 2.43.0
>>