lkml.org 
[lkml]   [2023]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 22/43] dt-bindings: mtd: add DT bindings for ts7250 nand
    From
    Date
    Hello Miquel!

    Thank you for looking into it.

    On Tue, 2023-05-02 at 11:48 +0200, Miquel Raynal wrote:
    > Hi Nikita,
    >
    > nikita.shubin@maquefel.me wrote on Mon, 24 Apr 2023 15:34:38 +0300:
    >
    > > Add YAML bindings for ts7250 NAND.
    > >
    > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
    > > ---
    > >  .../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@denx.de>
    > > +
    > > +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;

    And the best solution would be switching to exec_op completely ?

    >
    > > +
    > > +  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

    \
     
     \ /
      Last update: 2023-05-15 14:49    [W:8.841 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site