Re: [PATCH v3 02/24] dt-bindings: memory: mediatek: Convert SMI to DT schema

From: Krzysztof Kozlowski <krzk@kernel.org>
To: Yong Wu <yong.wu@mediatek.com>
Cc: Rob Herring <robh+dt@kernel.org>, Joerg Roedel <joro@8bytes.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>, Evan Green <evgreen@chromium.org>,
	Tomasz Figa <tfiga@google.com>,
	linux-mediatek@lists.infradead.org, srv_heupstream@mediatek.com,
	devicetree@vger.kernel.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	iommu@lists.linux-foundation.org, youlin.pei@mediatek.com,
	Nicolas Boichat <drinkcat@chromium.org>,
	anan.sun@mediatek.com, chao.hao@mediatek.com,
	ming-fan.chen@mediatek.com,
	Greg Kroah-Hartman <gregkh@google.com>,
	kernel-team@android.com
Subject: Re: [PATCH v3 02/24] dt-bindings: memory: mediatek: Convert SMI to DT schema
Date: Mon, 12 Oct 2020 15:26:05 +0200	[thread overview]
Message-ID: <CAJKOXPcn4scqt2C9eE_EikJ76kqro2QYzThdsXXR_5xtBmyH3g@mail.gmail.com> (raw)
In-Reply-To: <1602504119.26323.54.camel@mhfsdcap03>

On Mon, 12 Oct 2020 at 14:02, Yong Wu <yong.wu@mediatek.com> wrote:
>
> On Mon, 2020-10-12 at 09:18 +0200, Krzysztof Kozlowski wrote:
> > On Sat, Oct 10, 2020 at 02:18:11PM +0800, Yong Wu wrote:
> > > On Tue, 2020-10-06 at 09:15 +0200, Krzysztof Kozlowski wrote:
> > > > On Tue, 6 Oct 2020 at 06:27, Yong Wu <yong.wu@mediatek.com> wrote:
> > > > >
> > > > > On Fri, 2020-10-02 at 13:08 +0200, Krzysztof Kozlowski wrote:
> > > > > > On Wed, Sep 30, 2020 at 03:06:25PM +0800, Yong Wu wrote:
> > > > > > > Convert MediaTek SMI to DT schema.
> > > > > > >
> > > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > > > > > ---
> > > > > > >  .../mediatek,smi-common.txt                   |  49 ---------
> > > > > > >  .../mediatek,smi-common.yaml                  | 100 ++++++++++++++++++
> > > > > > >  .../memory-controllers/mediatek,smi-larb.txt  |  49 ---------
> > > > > > >  .../memory-controllers/mediatek,smi-larb.yaml |  91 ++++++++++++++++
> > > > > > >  4 files changed, 191 insertions(+), 98 deletions(-)
> > > > > > >  delete mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> > > > > > >  delete mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> > > > > ...
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    oneOf:
> > > > > > > +      - enum:
> > > > > > > +          - mediatek,mt2701-smi-common
> > > > > > > +          - mediatek,mt2712-smi-common
> > > > > > > +          - mediatek,mt6779-smi-common
> > > > > > > +          - mediatek,mt8173-smi-common
> > > > > > > +          - mediatek,mt8183-smi-common
> > > > > > > +
> > > > > > > +      - description: for mt7623
> > > > > > > +        items:
> > > > > > > +          - const: mediatek,mt7623-smi-common
> > > > > > > +          - const: mediatek,mt2701-smi-common
> > > > > > > +
> > > > > > > +  reg:
> > > > > > > +    maxItems: 1
> > > > > > > +
> > > > > > > +  clocks:
> > > > > > > +    description: |
> > > > > > > +      apb and smi are mandatory. the async is only for generation 1 smi HW.
> > > > > > > +      gals(global async local sync) also is optional, here is the list which
> > > > > > > +      require gals: mt6779 and mt8183.
> > > > > > > +    minItems: 2
> > > > > > > +    maxItems: 4
> > > > > > > +    items:
> > > > > > > +      - description: apb is Advanced Peripheral Bus clock, It's the clock for
> > > > > > > +          setting the register.
> > > > > > > +      - description: smi is the clock for transfer data and command.
> > > > > > > +      - description: async is asynchronous clock, it help transform the smi clock
> > > > > > > +          into the emi clock domain.
> > > > > > > +      - description: gals0 is the path0 clock of gals.
> > > > > > > +      - description: gals1 is the path1 clock of gals.
> > > > > > > +
> > > > > > > +  clock-names:
> > > > > > > +    oneOf:
> > > > > > > +      - items:
> > > > > > > +          - const: apb
> > > > > > > +          - const: smi
> > > > > > > +      - items:
> > > > > > > +          - const: apb
> > > > > > > +          - const: smi
> > > > > > > +          - const: async
> > > > > > > +      - items:
> > > > > > > +          - const: apb
> > > > > > > +          - const: smi
> > > > > > > +          - const: gals0
> > > > > > > +          - const: gals1
> > > > > >
> > > > > > Similarly to my comment to other properties, this requirement per
> > > > > > compatible should be part of the schema within 'if-then'.
> > > > >
> > > > > I'm not so familiar with this format. Do this has "if-then-'else
> > > > > if'-then-else"?
> > > >
> > > > These are mutually exclusive conditions, so you can skip else:
> > > >  - if-then
> > > >  - if-then
> > > >  - if-then
> > > > It will be more readable then stacking 'if' under 'else'
> > >
> > > Thanks. I will use something like this:
> > >
> > >  anyOf:
> >
> > Then it should be oneOf as only one condition can be valid.
>
> I did do this at the beginning. But I get a warning log when
> dt_binding_check.

Mhmm, right, since "if-else" matches in either of arms, then oneOf
will complain as it expects only one of items to match.  Then just go
with allOf. anyOf might match zero of items, so it would not catch
actual errors, I think.

Best regards,
Krzysztof