RE: [PATCH 1/4] dt-bindings: fsl: scu: add ocotp binding

[Peng Fan]

Hi Aisheng,

> Subject: RE: [PATCH 1/4] dt-bindings: fsl: scu: add ocotp binding
> 
> > From: Peng Fan
> > Sent: Sunday, May 5, 2019 9:28 PM
> >
> > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as system
> > controller(SCU), the ocotp controller is being controlled by the SCU,
> > so Linux need use RPC to SCU for ocotp handling. This patch adds
> > binding doc for i.MX8 SCU OCOTP driver.
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > Cc: Aisheng Dong <aisheng.dong@xxxxxxx>
> > Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> > Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> > Cc: Anson Huang <anson.huang@xxxxxxx>
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > ---
> >  Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 13
> > +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > index 5d7dbabbb784..9cb7d52bdf26 100644
> > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > @@ -100,6 +100,13 @@ ID in its "clocks" phandle cell.
> >  See the full list of clock IDs from:
> >  include/dt-bindings/clock/imx8qxp-clock.h
> >
> > +OCOTP bindings based on SCU Message Protocol
> > +------------------------------------------------------------
> > +Required properties:
> > +- compatible:		Should be "fsl,imx8qxp-ocotp"
> > +- #address-cells:	Must be 1. Contains byte index
> > +- #size-cells:		Must be 1. Contains byte length
> > +
> 
> Please put this unimportant one to the last.

ok. I just followed alphabetical order as OCOTP, Pinctrl, RTC sequence.
Will move it to last in V2.

> And it's better to mention the optional child nodes for data cells as Above
> #address-cells and #size-cells are used for it.
> Just like:
> Documentation/devicetree/bindings/nvmem/imx-ocotp.txt
> 
> >  Pinctrl bindings based on SCU Message Protocol
> >  ------------------------------------------------------------
> >
> > @@ -177,6 +184,12 @@ firmware {
> >  			...
> >  		};
> >
> > +		ocotp: imx8qx-ocotp {
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> 
> Not sure if it's a free choice, but AFAIK we usually write #address-cells and
> #size-cells under compatible string according to the example in DT spec and
> doc.

Fix in V2.

Thanks,
Peng

> https://elinux.org/Device_Tree_Usage
> https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.
> 2
> 
> Maybe Rob can comment to avoid confusing.
> 
> Otherwise, this patch seems good to me.
> 
> Regards
> Dong Aisheng
> 
> > +			compatible = "fsl,imx8qxp-ocotp";
> > +		};
> > +
> >  		pd: imx8qx-pd {
> >  			compatible = "fsl,imx8qxp-scu-pd", "fsl,scu-pd";
> >  			#power-domain-cells = <1>;
> > --
> > 2.16.4