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

[Apurva Nandan]

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@xxxxxx>
> > > ---
> > >   .../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@xxxxxx>
> > > +
> > > +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?

Thanks,
Apurva Nandan