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

[Ninad Palsule]

Hi Corey,

On 1/10/25 11:53, Corey Minyard wrote:
> On Fri, Jan 10, 2025 at 10:07:13AM -0600, Rob Herring wrote:
> > On Wed, Jan 08, 2025 at 10:36:30AM -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    | 44 +++++++++++++++++++
> > >   1 file changed, 44 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..a8f46f1b883e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/ipmi/ipmb-dev.yaml
> > > @@ -0,0 +1,44 @@
> > > +# 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
> > > +
> > > +description: IPMB Device interface to receive request and send response
> First, thank you, this does need to be documented.
Thanks for the review.
> > IPMB is not defined anywhere.
> Indeed.  At least reference the spec, but better do that and provide a
> basic description.
>
> > Which side of the interface does this apply to? How do I know if I have
> > an ipmb-dev?
> >
> > This document needs to stand on its own. Bindings exist in a standalone
> > tree without kernel drivers or docs.
> At least to someone who knows what IPMB is, it's pretty clear that you
> are saying "The i2c device this node is in is on an IPMB bus." However,
> to someone who is not, this is all a foreign language.  This definitely
> needs better documentation.
>
> Why do you have a "reg" property?  I don't see it referenced in the
> driver.  I assume that's the I2C address, but that's going to be the
> same as what's in the containing I2C node.  I don't think it's
> necessary.
>
> -corey

I have improved the documentation in version 4. Please check.

Regards,

Ninad