Hi Nikita, nikita.shubin@xxxxxxxxxxx wrote on Mon, 15 May 2023 18:48:31 +0300: > Hello Miquel! > > Thank you for looking into it. > > On Tue, 2023-05-02 at 11:48 +0200, Miquel Raynal wrote: > > Hi Nikita, > > > > nikita.shubin@xxxxxxxxxxx wrote on Mon, 24 Apr 2023 15:34:38 +0300: > > > > > Add YAML bindings for ts7250 NAND. > > > > > > Signed-off-by: Nikita Shubin <nikita.shubin@xxxxxxxxxxx> > > > --- > > > .../bindings/mtd/technologic,nand.yaml | 56 > > > +++++++++++++++++++ > > > 1 file changed, 56 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/mtd/technologic,nand.yaml > > > > > > diff --git > > > a/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > > > b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > > > new file mode 100644 > > > index 000000000000..3234d93a1c21 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mtd/technologic,nand.yaml > > > @@ -0,0 +1,56 @@ > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/mtd/technologic,nand.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Technologic Systems NAND controller > > > + > > > +maintainers: > > > + - Lukasz Majewski <lukma@xxxxxxx> > > > + > > > +properties: > > > + compatible: > > > + items: > > > + - const: technologic,ts7200-nand > > > > would -nand-controller instead of -nand work as a suffix here? > > > > You mention ts7250 in the title, should we have a more specific > > compatible than ts7200 as well? > > > > I see by looking at the mtd patch that you actually try to match > > both, > > so they should both be defined in the bindings. > > > > > + - const: gen_nand > > > > This is a old hack for very simple controllers (converted to DT > > probing > > 12 years ago). The logic used by this driver has been deprecated for > > like 10 years and does not really apply to modern APIs. I would > > really > > like to keep this driver contained with platform data coming from > > arch/ > > data only. > > > > I suggest you create a real NAND controller driver based on the > > generic one (should not be very complex, just duplicate the code so > > the > > migration to the up-to-date API is eased) and you flag it as "must be > > updated to ->exec_op() somehow. This way if someone starts the > > conversion, it does not need to cope with the 5 other users of the > > generic driver which anyway share nothing in common besides the > > deprecated ->cmd_ctrl() backbone. > > > > I read the comments on the cover letter, people are kind of pushing > > on > > having this merged quickly. I am fine accepting a legacy controller > > driver and migrating it to ->exec_op() later, but the current driver > > conversion does not fit the approach taken years ago towards a > > cleaner > > mtd tree. > > Did you mean that i should at least implement legacy nand controller, > like, for example, Xway (xway_nand.c) ?: > > data->chip.legacy.cmd_ctrl = xway_cmd_ctrl; > data->chip.legacy.dev_ready = xway_dev_ready; > data->chip.legacy.select_chip = xway_select_chip; > data->chip.legacy.write_buf = xway_write_buf; > data->chip.legacy.read_buf = xway_read_buf; > data->chip.legacy.read_byte = xway_read_byte; > data->chip.legacy.chip_delay = 30; I don't know how urgent this conversion is, this is really the minimal step... > And the best solution would be switching to exec_op completely ? ...and this is what I would really prefer, yes. I don't think it's huge, the controller being very simple and straightforward. > > > > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + '#address-cells': true > > > + '#size-cells': true > > > + > > > +required: > > > + - compatible > > > + - reg > > > + > > > +unevaluatedProperties: true > > > + > > > +examples: > > > + - | > > > + nand-parts@0 { > > > + compatible = "technologic,ts7200-nand", "gen_nand"; > > > + reg = <0x60000000 0x8000000>; > > > + #address-cells = <1>; > > > + #size-cells = <1>; > > > + > > > + partition@0 { > > > + label = "TS-BOOTROM"; > > > + reg = <0x00000000 0x00020000>; > > > + read-only; > > > + }; > > > > Partitions are not useful here, but if you want them, use the > > partitions container instead, please. > > > > > + > > > + partition@20000 { > > > + label = "Linux"; > > > + reg = <0x00020000 0x07d00000>; > > > + }; > > > + > > > + partition@7d20000 { > > > + label = "RedBoot"; > > > + reg = <0x07d20000 0x002e0000>; > > > + read-only; > > > + }; > > > + }; > > > + > > > +... > > > > > > Thanks, > > Miquèl > Thanks, Miquèl
