Re: [PATCH 1/2] dt-bindings: soc: Add binding doc for iProc IDM device

[Rob Herring <robh@kernel.org>]

On Fri, Dec 06, 2019 at 05:09:34PM -0800, Ray Jui wrote:
> 
> 
> On 12/5/19 4:09 PM, Florian Fainelli wrote:
> > On 12/2/19 3:31 PM, Ray Jui wrote:
> > > Add binding document for iProc based IDM devices.
> > > 
> > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > 
> > Looks good to me, it's 2019, nearly 2020, maybe make this a YAML
> > compatible binding since it is a new one?
> > 
> 
> Sorry I am not aware of this YAML requirement until now.
> 
> Is this a new requirement that new DT binding document should be made with
> YAML format?

The format has been in place in the kernel for a year now and we've 
moved slowly towards it being required. If you're paying that little 
attention to upstream, then yes it's definitely required so someone else 
doesn't get stuck converting your binding later.

BTW, I think all but RPi chips still need their SoC/board bindings 
converted. One of the few not yet converted...

> Thanks,
> 
> Ray
> 
> 
> > > ---
> > >   .../bindings/soc/bcm/brcm,iproc-idm.txt       | 44 +++++++++++++++++++
> > >   1 file changed, 44 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> > > new file mode 100644
> > > index 000000000000..388c6b036d7e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> > > @@ -0,0 +1,44 @@
> > > +Broadcom iProc Interconnect Device Management (IDM) device
> > > +
> > > +The Broadcom iProc IDM device allows control and monitoring of ASIC internal
> > > +bus transactions. Most importantly, it can be configured to detect bus
> > > +transaction timeout. In such case, critical information such as transaction
> > > +address that caused the error, bus master ID of the transaction that caused
> > > +the error, and etc., are made available from the IDM device.
> > > +
> > > +-------------------------------------------------------------------------------
> > > +
> > > +Required properties for IDM device node:
> > > +- compatible: must be "brcm,iproc-idm"
> > > +- reg: base address and length of the IDM register space
> > > +- interrupt: IDM interrupt number
> > > +- brcm,iproc-idm-bus: IDM bus string

What are possible values?

> > > +
> > > +Optional properties for IDM device node:
> > > +- brcm,iproc-idm-elog: phandle to the device node of the IDM logging device
> > > +
> > > +-------------------------------------------------------------------------------
> > > +
> > > +Required properties for IDM error logging device node:
> > > +- compatible: must be "brcm,iproc-idm-elog";
> > > +- reg: base address and length of reserved memory location where IDM error
> > > +  events can be saved
> > > +
> > > +-------------------------------------------------------------------------------
> > > +
> > > +Example:
> > > +
> > > +idm {
> > > +	idm-elog {
> > > +		compatible = "brcm,iproc-idm-elog";
> > > +		reg = <0x8f221000 0x1000>;
> > > +	};
> > > +
> > > +	idm-mhb-paxc-axi {

Needs a unit-address.

> > > +		compatible = "brcm,iproc-idm";
> > > +		reg = <0x60406900 0x200>;
> > > +		interrupt = <GIC_SPI 516 IRQ_TYPE_LEVEL_HIGH>;
> > > +		brcm,iproc-idm-bus = "idm-mhb-paxc-axi";

Can't you just use the node name? Though if this is something that can 
be a standard class of h/w (i.e. EDAC), then we'd want the node name to 
be generic.

> > > +		brcm,iproc-idm-elog = <&idm-elog>;
> > > +	};
> > > +};
> > > 
> > 
> >