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:

quoted

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.

quoted

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
  .../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.

quoted

+
+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.

quoted

+
+maintainers:
+  - Ninad Palsule [off-list ref]
+
+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

quoted

+
+  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

quoted

+    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

quoted

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