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

[Ninad P. <ni...@li...>]

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