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

[Ninad Palsule]

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@xxxxxxxxxxxxx>
> > ---
> >   .../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@xxxxxxxxxxxxx>
> > +
> > +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