Re: [PATCH v5 1/2] dt-bindings: phy: intel-emmc-phy: Add YAML schema for LGM eMMC PHY

[Ramuthevar, Vadivel MuruganX]

Hi Rob,

On 18/9/2019 10:23 AM, Ramuthevar, Vadivel MuruganX wrote:
> Hi Rob,
>
> Thank you for the review comments.
>
> On 17/9/2019 10:23 PM, Rob Herring wrote:
> > On Wed, Sep 04, 2019 at 01:53:43PM +0800, Ramuthevar,Vadivel MuruganX 
> > wrote:
> > > From: Ramuthevar Vadivel Murugan 
> > > <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
> > >
> > > Add a YAML schema to use the host controller driver with the
> > > eMMC PHY on Intel's Lightning Mountain SoC.
> > >
> > > Signed-off-by: Ramuthevar Vadivel Murugan 
> > > <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
> > > ---
> > > changes in v5:
> > > ÂÂ - earlier Review-by tag given by Rob
> > > ÂÂ - rework done with syscon parent node.
> > >
> > > changes in v4:
> > > ÂÂ - As per Rob's review: validate 5.2 and 5.3
> > > ÂÂ - drop unrelated items.
> > >
> > > changes in v3:
> > > ÂÂ - resolve 'make dt_binding_check' warnings
> > >
> > > changes in v2:
> > > ÂÂ As per Rob Herring review comments, the following updates
> > > Â - change GPL-2.0 -> (GPL-2.0-only OR BSD-2-Clause)
> > > Â - filename is the compatible string plus .yaml
> > > Â - LGM: Lightning Mountain
> > > Â - update maintainer
> > > Â - add intel,syscon under property list
> > > Â - keep one example instead of two
> > > ---
> > > Â .../bindings/phy/intel,lgm-emmc-phy.yamlÂÂÂÂÂÂÂÂÂÂ | 69 
> > > ++++++++++++++++++++++
> > > Â 1 file changed, 69 insertions(+)
> > > Â create mode 100644 
> > > Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml
> > >
> > > diff --git 
> > > a/Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml 
> > > b/Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml
> > > new file mode 100644
> > > index 000000000000..8f6ac8b3da42
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml
> > > @@ -0,0 +1,69 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/phy/intel,lgm-emmc-phy.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Intel Lightning Mountain(LGM) eMMC PHY Device Tree Bindings
> > > +
> > > +maintainers:
> > > +Â - Ramuthevar Vadivel Murugan 
> > > <vadivel.muruganx.ramuthevar@xxxxxxxxxxxxxxx>
> > > +
> > > +description: Bindings for eMMC PHY on Intel's Lightning Mountain 
> > > SoC, syscon
> > > +Â node is used to reference the base address of eMMC phy registers.
> > > +
> > > +select:
> > > +Â properties:
> > > +ÂÂÂ compatible:
> > > +ÂÂÂÂÂ contains:
> > > +ÂÂÂÂÂÂÂ const: intel,lgm-syscon
> > This, plus...
>
> you mean, need to add two compatible here-itself look like below
>
> const: intel,lgm-syscon
> const: intel,lgm-emmc-phy
>
> Is it right?
Shall I proceed with above compatibles if you agree upon. Thanks!

With Best Regards
Vadivel Murugan
> > > +
> > > +ÂÂÂ reg:
> > > +ÂÂÂÂÂ maxItems: 1
> > > +
> > > +Â required:
> > > +ÂÂÂ - compatible
> > > +ÂÂÂ - reg
> > > +
> > > +properties:
> > > +Â "#phy-cells":
> > > +ÂÂÂ const: 0
> > > +
> > > +Â compatible:
> > > +ÂÂÂ contains:
> > > +ÂÂÂÂÂ const: intel,lgm-emmc-phy
> > ...this should not pass validation as they contradict each other.
> when I do "make dt_binding_check" didn't throw an error, let me 
> double confirm once clarified first comment.
> > > +
> > > +Â reg:
> > > +ÂÂÂ maxItems: 1
> > > +
> > > +Â clocks:
> > > +ÂÂÂ maxItems: 1
> > > +
> > > +Â clock-names:
> > > +ÂÂÂ maxItems: 1
> > > +
> > > +required:
> > > +Â - "#phy-cells"
> > > +Â - compatible
> > > +Â - reg
> > > +Â - clocks
> > > +Â - clock-names
> > > +
> > > +examples:
> > > +Â - |
> > > +ÂÂÂ sysconf: chiptop@e0200000 {
> > > +ÂÂÂÂÂ compatible = "intel,lgm-syscon";
> > > +ÂÂÂÂÂ reg = <0xe0200000 0x100>;
> > I'm still waiting for a complete description of what all is in this
> > block.
> Agree!, I will add it.
> > > +
> > > +ÂÂÂÂÂ emmc-phy: emmc-phy {
> > > +ÂÂÂÂÂÂÂ compatible = "intel,lgm-emmc-phy";
> > > +ÂÂÂÂÂÂÂ reg = <0x00a8 0x4>,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ <0x00ac 0x4>,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ <0x00b0 0x4>,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ <0x00b4 0x4>;
> > Looks contiguous and can be a single entry:
> >
> > <0xa8 0x10>
> Agreed, will fix it.
>
> Best Regards
> Vadivel
> > > +ÂÂÂÂÂÂÂ clocks = <&emmc>;
> > > +ÂÂÂÂÂÂÂ clock-names = "emmcclk";
> > > +ÂÂÂÂÂÂÂ #phy-cells = <0>;
> > > +ÂÂÂÂÂ };
> > > +ÂÂÂ };
> > > +...
> > > --
> > > 2.11.0