Re: [PATCH v2 02/10] bindings: ipmi: Add binding for IPMB device intf

[Ninad Palsule]

Hello Rob,

Thanks for the review.

On 1/7/25 17:13, Rob Herring wrote:
> On Tue, Jan 07, 2025 at 10:23:39AM -0600, Ninad Palsule wrote:
> > Add device tree binding document for the IPMB device interface driver.
> Please mention this is already is already in use both in a driver and
> .dts files.
>
> > Signed-off-by: Ninad Palsule <ninad@xxxxxxxxxxxxx>
> > ---
> >   .../devicetree/bindings/ipmi/ipmb-dev.yaml    | 42 +++++++++++++++++++
> >   1 file changed, 42 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..9136ac8004dc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/ipmi/ipmb-dev.yaml
> > @@ -0,0 +1,42 @@
> > +# 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: IPMB Device Driver
> Bindings are for devices, not drivers. Drop 'Driver'. It's a stretch
> that IPMB is even a device, but since there are already a few users, I
> guess we're stuck with it.
Updated the title.
> > +
> > +description: IPMB Device Driver bindings
> No point in a description that just repeats the title. Please expand
> this. For example, AIUI, this is for the device end, not the BMC end.
Updated the description.
> > +
> > +maintainers:
> > +  - Ninad Palsule <ninad@xxxxxxxxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ipmb-dev
> > +
> > +  reg:
> > +    maxItems: 1
> As this is the slave end, I2C_OWN_SLAVE_ADDRESS should be set. So:
>
> minimum: 0x40000000
> maximum: 0x4000007f

The dt_check script doesn't allow min, max for the reg type.

/home/ninad/dev/sbp1/linux/Documentation/devicetree/bindings/ipmi/ipmb-dev.yaml: 
properties:reg: 'minimum' should not be valid under {'enum': ['const', 
'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 
'multipleOf', 'pattern']}
    hint: Scalar and array keywords cannot be mixed
    from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/home/ninad/dev/sbp1/linux/Documentation/devicetree/bindings/ipmi/ipmb-dev.yaml: 
properties:reg: 'maximum' should not be valid under {'enum': ['const', 
'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'minimum', 'maximum', 
'multipleOf', 'pattern']}
    hint: Scalar and array keywords cannot be mixed
    from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

> Maybe 10-bit addressing has to be supported too?
Driver only uses 7 and 8 bit addresses
> > +
> > +  i2c-protocol:
> > +    description:
> > +      This property specifies that the I2C block transfer should be performed
> > +      instead of SMBUS block transfer.
> This can be more concisely said:
>
> Use I2C block transfer instead of SMBUS block transfer.
Done
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        i2c@10 {
> 'i2c' node name is for i2c buses and this is not one. 'ipmb' is probably
> fine here.

Done

Regards,

Ninad

> > +            compatible = "ipmb-dev";
> > +            reg = <0x10>;
> > +            i2c-protocol;
> > +        };
> > +    };
> > --
> > 2.43.0