Re: [PATCH v2 1/2] dt-bindings: mtd: spi-nand: Convert to DT schema format

[Rob Herring <robh@kernel.org>]

On Tue, Sep 21, 2021 at 8:43 AM Apurva Nandan <a-nandan@ti.com> wrote:
>
> Hi Rob,
>
> On 21/09/21 2:46 am, Rob Herring wrote:
> > On Mon, Sep 20, 2021 at 06:06:51PM +0300, Serge Semin wrote:
> >> Hello Apurva
> >>
> >> On Mon, Sep 20, 2021 at 07:57:12PM +0530, Apurva Nandan wrote:
> >>> Convert spi-nand.txt binding to YAML format with an added example.
> >>>
> >>> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
> >>> ---
> >>>   .../devicetree/bindings/mtd/spi-nand.txt      |  5 --
> >>>   .../devicetree/bindings/mtd/spi-nand.yaml     | 62 +++++++++++++++++++
> >>>   2 files changed, 62 insertions(+), 5 deletions(-)
> >>>   delete mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt
> >>>   create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.yaml
> >> Thanks for the bindings conversion patch. There are several comments
> >> below. But before addressing them it would be better to also get a
> >> response from Rob.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/mtd/spi-nand.txt b/Documentation/devicetree/bindings/mtd/spi-nand.txt
> >>> deleted file mode 100644
> >>> index 8b51f3b6d55c..000000000000
> >>> --- a/Documentation/devicetree/bindings/mtd/spi-nand.txt
> >>> +++ /dev/null
> >>> @@ -1,5 +0,0 @@
> >>> -SPI NAND flash
> >>> -
> >>> -Required properties:
> >>> -- compatible: should be "spi-nand"
> >>> -- reg: should encode the chip-select line used to access the NAND chip
> >>> diff --git a/Documentation/devicetree/bindings/mtd/spi-nand.yaml b/Documentation/devicetree/bindings/mtd/spi-nand.yaml
> >>> new file mode 100644
> >>> index 000000000000..601beba8d971
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mtd/spi-nand.yaml
> >>> @@ -0,0 +1,62 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/mtd/spi-nand.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: SPI NAND flash
> >>> +
> >>> +maintainers:
> >>> +  - Apurva Nandan <a-nandan@ti.com>
> >>> +
> >>> +allOf:
> >>> +  - $ref: "mtd.yaml#"
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: spi-nand
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  spi-max-frequency: true
> >>> +  spi-rx-bus-width: true
> >>> +  spi-tx-bus-width: true
> >>> +  rx-sample-delay-ns: true
> >> Since it's an SPI-client device there are more than these properties
> >> could be set for it. See the SPI-controller bindings schema:
> >> Documentation/devicetree/bindings/spi/spi-controller.yaml
> >> So there is two possible ways to make it more generic:
> >> 1) Detach the spi-client part from the spi-controller.yaml bindings
> >> into a dedicated DT-schema file and refer to that new scheme from
> >> here.
> > Yes, as mentioned there's patches doing this. But the above is fine.
> > There's some value in defining here which properties are valid.
> Yeah right
> >> 2) Forget about these controller-specific properties and let the
> >> parental SPI-controller bindings parsing them. Of course there must be
> >> at least one of the next properties declared for it to work:
> >> {unevaluatedProperties, additionalProperties}.
> >>
> >> It's up to Rob to decided which approach is better though...
> >>
> >>> +
> >>> +  '#address-cells': true
> >>> +  '#size-cells': true
> >> Aren't they always equal to 1?
> > No SPI nand devices >4GB?
> Yeah, we have SPI NANDs >4GB, and "'#address-cells': true" allows those
> sizes.
> >
> >>> +
> >>> +additionalProperties:
> >>> +  type: object
> >> I'd suggest to elaborate the way the partition sub-nodes looks
> >> like, for instance, the node names, supported compatible names,
> >> labels, etc.
> > That should probably all be in mtd.yaml. The question here is whether
> > partitions are always under a 'partitions' node. Maybe this is new
> > enough that only the new way has to be supported. Though if mtd.yaml
> > supported both forms, allowing both all the time is okay IMO.
> >
> > Rob
>
> I had added the "partition" node properties in the v1 patch, but as per
> the reviews I removed it.
> I think we can prefer having them in mtd.yaml if needed, in a separate
> patch series.
> Do you prefer the mtd.yaml changes as a part of this series or as a
> separate patch?
>
> Other than that, I don't find any need for a v3 patch re-roll, do you agree?

Yeah, I suppose not.

Reviewed-by: Rob Herring <robh@kernel.org>

And actually, it's Miquel that should apply these 2 rather than Mark.

Rob