[00/14] mtd: maps: physmap cleanups

Hello,

This is an attempt at merging physmap_of.c, gpio-addr-flash.c and
physmap.c. The rational behind this decision is code sharing and
consistency with what's done in other parts of the kernel:

- physmap_of is just adding OF/DT support to the physmap logic, and we
  usually put that code in the pre-existing driver instead of creating
  a new one

- gpio-addr-flash is just an extension of the physmap logic which is
  needed when the platform does not have enough native ADDR lines
  to address the whole flash. Again, I think this core belongs in the
  physmap driver

Patches 1 to 9 are just cleanups, and are not really related to the
merging of physmap_of and gpio-addr-flash into physmap.c.

Patch 10 to 13 are doing the actual merging work, and patch 14 is
documenting the addr-gpios property that has been added to support
the gpio-addr-flash on DT platforms.

Let me know what you think of this rework.

Regards,

Boris

Boris Brezillon (13):
  mtd: maps: physmap: Add SPDX header
  mtd: maps: physmap: Rename ->map and ->mtd into ->maps and ->mtds
  mtd: maps: physmap: Use platform_get_resource() to retrieve iomem
    resources
  mtd: maps: physmap: Use dev_notice() and a %pR specifier
  mtd: maps: physmap: Use devm_ioremap_resource()
  mtd: maps: physmap: Remove the MAX_RESOURCES limitation
  mtd: maps: physmap: Check mtd_device_{parse_register,unregister}() ret
    code
  mtd: maps: physmap: Return -ENOMEM directly when info allocation fails
  mtd: maps: physmap: Fix coding style issues reported by checkpatch
  mtd: maps: Prepare merging of physmap and physmap_of
  mtd: maps: Merge physmap_of.c into physmap-core.c
  mtd: maps: Merge gpio-addr-flash.c into physmap-core.c
  mtd: maps: Rename physmap_of_{versatile,gemini} into
    physmap-{versatile,gemini}

Ricardo Ribalda Delgado (1):
  dt-binding: mtd: physmap: Document the addr-gpios property

 .../devicetree/bindings/mtd/mtd-physmap.txt        |   2 +
 drivers/mtd/maps/Kconfig                           |  27 +-
 drivers/mtd/maps/Makefile                          |  10 +-
 drivers/mtd/maps/gpio-addr-flash.c                 | 281 ---------
 drivers/mtd/maps/physmap-core.c                    | 651 +++++++++++++++++++++
 .../maps/{physmap_of_gemini.c => physmap-gemini.c} |   2 +-
 .../maps/{physmap_of_gemini.h => physmap-gemini.h} |   2 +-
 ...{physmap_of_versatile.c => physmap-versatile.c} |   2 +-
 ...{physmap_of_versatile.h => physmap-versatile.h} |   2 +-
 drivers/mtd/maps/physmap.c                         | 280 ---------
 drivers/mtd/maps/physmap_of_core.c                 | 368 ------------
 11 files changed, 674 insertions(+), 953 deletions(-)
 delete mode 100644 drivers/mtd/maps/gpio-addr-flash.c
 create mode 100644 drivers/mtd/maps/physmap-core.c
 rename drivers/mtd/maps/{physmap_of_gemini.c => physmap-gemini.c} (98%)
 rename drivers/mtd/maps/{physmap_of_gemini.h => physmap-gemini.h} (90%)
 rename drivers/mtd/maps/{physmap_of_versatile.c => physmap-versatile.c} (99%)
 rename drivers/mtd/maps/{physmap_of_versatile.h => physmap-versatile.h} (90%)
 delete mode 100644 drivers/mtd/maps/physmap.c
 delete mode 100644 drivers/mtd/maps/physmap_of_core.c

Hi Boris
On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> There's no real reason to have two separate driver for the DT and pdata
> case. Just do what we do everywhere else and handle DT and pdata
> parsing in the same driver.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/maps/Kconfig           |   4 +-
>  drivers/mtd/maps/Makefile          |   7 +-
>  drivers/mtd/maps/physmap-core.c    | 260 +++++++++++++++++++++++---
>  drivers/mtd/maps/physmap_of_core.c | 368 -------------------------------------
>  4 files changed, 239 insertions(+), 400 deletions(-)
>  delete mode 100644 drivers/mtd/maps/physmap_of_core.c
>
> diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> index afb36bff13a7..5bffebacce86 100644
> --- a/drivers/mtd/maps/Kconfig
> +++ b/drivers/mtd/maps/Kconfig
> @@ -66,8 +66,8 @@ config MTD_PHYSMAP_BANKWIDTH
>           used internally by the CFI drivers.
>
>  config MTD_PHYSMAP_OF
> -       tristate "Memory device in physical memory map based on OF description"
> -       depends on OF && (MTD_CFI || MTD_JEDECPROBE || MTD_ROM || MTD_RAM)
> +       bool "Memory device in physical memory map based on OF description"
> +       depends on OF && MTD_PHYSMAP
>         help
>           This provides a 'mapping' driver which allows the NOR Flash, ROM
>           and RAM driver code to communicate with chips which are mapped
> diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
> index 2574909edffd..ad32b185a120 100644
> --- a/drivers/mtd/maps/Makefile
> +++ b/drivers/mtd/maps/Makefile
> @@ -18,13 +18,10 @@ obj-$(CONFIG_MTD_CK804XROM) += ck804xrom.o
>  obj-$(CONFIG_MTD_TSUNAMI)      += tsunami_flash.o
>  obj-$(CONFIG_MTD_PXA2XX)       += pxa2xx-flash.o
>  physmap-objs-y                 += physmap-core.o
> +physmap-objs-$(CONFIG_MTD_PHYSMAP_OF_VERSATILE) += physmap_of_versatile.o
> +physmap-objs-$(CONFIG_MTD_PHYSMAP_OF_GEMINI) += physmap_of_gemini.o
>  physmap-objs                   := $(physmap-objs-y)
>  obj-$(CONFIG_MTD_PHYSMAP)      += physmap.o
> -physmap_of-objs-y              += physmap_of_core.o
> -physmap_of-objs-$(CONFIG_MTD_PHYSMAP_OF_VERSATILE) += physmap_of_versatile.o
> -physmap_of-objs-$(CONFIG_MTD_PHYSMAP_OF_GEMINI) += physmap_of_gemini.o
> -physmap_of-objs                        := $(physmap_of-objs-y)
> -obj-$(CONFIG_MTD_PHYSMAP_OF)   += physmap_of.o
>  obj-$(CONFIG_MTD_PISMO)                += pismo.o
>  obj-$(CONFIG_MTD_PMC_MSP_EVM)   += pmcmsp-flash.o
>  obj-$(CONFIG_MTD_PCMCIA)       += pcmciamtd.o
> diff --git a/drivers/mtd/maps/physmap-core.c b/drivers/mtd/maps/physmap-core.c
> index e27051bc5dc6..7a50ff9ef812 100644
> --- a/drivers/mtd/maps/physmap-core.c
> +++ b/drivers/mtd/maps/physmap-core.c
> @@ -6,6 +6,13 @@
>   * Author: Jun Sun, jsun@mvista.com or jsun@junsun.net
>   *
>   * 031022 - [jsun] add run-time configure and partition setup
> + *
> + * Device tree support:
> + *    Copyright (C) 2006 MontaVista Software Inc.
> + *    Author: Vitaly Wool <vwool@ru.mvista.com>
> + *
> + *    Revised to handle newer style flash binding by:
> + *    Copyright (C) 2007 David Gibson, IBM Corporation.
>   */
>
>  #include <linux/module.h>
> @@ -20,7 +27,12 @@
>  #include <linux/mtd/partitions.h>
>  #include <linux/mtd/physmap.h>
>  #include <linux/mtd/concat.h>
> +#include <linux/mtd/cfi_endian.h>
>  #include <linux/io.h>
> +#include <linux/of_device.h>
> +
> +#include "physmap_of_gemini.h"
> +#include "physmap_of_versatile.h"
>
>  struct physmap_flash_info {
>         unsigned int            nmaps;
> @@ -29,6 +41,10 @@ struct physmap_flash_info {
>         struct map_info         *maps;
>         spinlock_t              vpp_lock;
>         int                     vpp_refcnt;
> +       const char              *probe_type;
> +       const char * const      *part_types;
> +       unsigned int            nparts;
> +       const struct mtd_partition *parts;
>  };
>
>  static int physmap_flash_remove(struct platform_device *dev)
> @@ -41,8 +57,6 @@ static int physmap_flash_remove(struct platform_device *dev)
>         if (!info)
>                 return 0;
>
> -       physmap_data = dev_get_platdata(&dev->dev);
> -
>         if (info->cmtd) {
>                 err = mtd_device_unregister(info->cmtd);
>                 if (err)
> @@ -57,7 +71,8 @@ static int physmap_flash_remove(struct platform_device *dev)
>                         map_destroy(info->mtds[i]);
>         }
>
> -       if (physmap_data->exit)
> +       physmap_data = dev_get_platdata(&dev->dev);
> +       if (physmap_data && physmap_data->exit)
>                 physmap_data->exit(dev);
>
>         return 0;
> @@ -89,6 +104,172 @@ static void physmap_set_vpp(struct map_info *map, int state)
>         spin_unlock_irqrestore(&info->vpp_lock, flags);
>  }
>
> +#if IS_ENABLED(CONFIG_MTD_PHYSMAP_OF)
> +static const struct of_device_id of_flash_match[] = {
> +       {
> +               .compatible = "cfi-flash",
> +               .data = "cfi_probe",
> +       },
> +       {
> +               /*
> +                * FIXME: JEDEC chips can't be safely and reliably
> +                * probed, although the mtd code gets it right in
> +                * practice most of the time.  We should use the
> +                * vendor and device ids specified by the binding to
> +                * bypass the heuristic probe code, but the mtd layer
> +                * provides, at present, no interface for doing so
> +                * :(.
> +                */
> +               .compatible = "jedec-flash",
> +               .data = "jedec_probe",
> +       },
> +       {
> +               .compatible = "mtd-ram",
> +               .data = "map_ram",
> +       },
> +       {
> +               .compatible = "mtd-rom",
> +               .data = "map_rom",
> +       },
> +       {
> +               .type = "rom",
> +               .compatible = "direct-mapped"
> +       },
> +       { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, of_flash_match);
> +
> +static const char * const of_default_part_probes[] = {
> +       "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL
> +};
> +
> +static const char * const *of_get_part_probes(struct platform_device *dev)
> +{
> +       struct device_node *dp = dev->dev.of_node;
> +       const char **res;
> +       int count;
> +
> +       count = of_property_count_strings(dp, "linux,part-probe");
> +       if (count < 0)
> +               return of_default_part_probes;
> +
> +       res = devm_kcalloc(&dev->dev, count + 1, sizeof(*res), GFP_KERNEL);
> +       if (!res)
> +               return NULL;
> +
> +       count = of_property_read_string_array(dp, "linux,part-probe", res,
> +                                             count);
> +       if (count < 0)
> +               return NULL;
> +
> +       return res;
> +}
> +
> +static const char *of_select_probe_type(struct platform_device *dev)
> +{
> +       struct device_node *dp = dev->dev.of_node;
> +       const struct of_device_id *match;
> +       const char *probe_type;
> +
> +       match = of_match_device(of_flash_match, &dev->dev);
> +       probe_type = match->data;
> +       if (probe_type)
> +               return probe_type;
> +
> +       dev_warn(&dev->dev,
> +                "Device tree uses obsolete \"direct-mapped\" flash binding\n");
> +
> +       of_property_read_string(dp, "probe-type", &probe_type);
> +       if (!probe_type)
> +               return NULL;
> +
> +       if (!strcmp(probe_type, "CFI")) {
> +               probe_type = "cfi_probe";
> +       } else if (!strcmp(probe_type, "JEDEC")) {
> +               probe_type = "jedec_probe";
> +       } else if (!strcmp(probe_type, "ROM")) {
> +               probe_type = "map_rom";
> +       } else {
> +               dev_warn(&dev->dev,
> +                        "obsolete_probe: don't know probe type '%s', mapping as rom\n",
> +                        probe_type);
> +               probe_type = "map_rom";
> +       }
> +
> +       return probe_type;
> +}
> +
> +static int physmap_flash_of_init(struct platform_device *dev)
> +{
> +       struct physmap_flash_info *info = platform_get_drvdata(dev);
> +       struct device_node *dp = dev->dev.of_node;
> +       const char *mtd_name = NULL;
> +       int err, swap = 0;
> +       bool map_indirect;
> +       unsigned int i;
> +       u32 bankwidth;
> +
> +       if (!dp)
> +               return -EINVAL;
> +
> +       info->probe_type = of_select_probe_type(dev);
> +
> +       info->part_types = of_get_part_probes(dev);
> +       if (!info->part_types)
> +               return -ENOMEM;
> +
> +       of_property_read_string(dp, "linux,mtd-name", &mtd_name);
> +
> +       map_indirect = of_property_read_bool(dp, "no-unaligned-direct-access");
> +
> +       err = of_property_read_u32(dp, "bank-width", &bankwidth);
> +       if (err) {
> +               dev_err(&dev->dev, "Can't get bank width from device tree\n");
> +               return err;
> +       }
> +
> +       if (of_property_read_bool(dp, "big-endian"))
> +               swap = CFI_BIG_ENDIAN;
> +       else if (of_property_read_bool(dp, "little-endian"))
> +               swap = CFI_LITTLE_ENDIAN;
> +
> +       for (i = 0; i < info->nmaps; i++) {
> +               info->maps[i].name = mtd_name;
> +               info->maps[i].swap = swap;
> +               info->maps[i].bankwidth = bankwidth;
> +               info->maps[i].device_node = dp;
> +
> +               err = of_flash_probe_gemini(dev, dp, &info->maps[i]);
> +               if (err)
> +                       return err;
> +
> +               err = of_flash_probe_versatile(dev, dp, &info->maps[i]);
> +               if (err)
> +                       return err;
> +
> +               /*
> +                * On some platforms (e.g. MPC5200) a direct 1:1 mapping
> +                * may cause problems with JFFS2 usage, as the local bus (LPB)
> +                * doesn't support unaligned accesses as implemented in the
> +                * JFFS2 code via memcpy(). By setting NO_XIP, the
> +                * flash will not be exposed directly to the MTD users
> +                * (e.g. JFFS2) any more.
> +                */
> +               if (map_indirect)
> +                       info->maps[i].phys = NO_XIP;
> +       }
> +
> +       return 0;
> +}
> +#else /* IS_ENABLED(CONFIG_MTD_PHYSMAP_OF) */
> +#define of_flash_match NULL
> +
> +static int physmap_flash_of_init(struct platform_device *dev)
> +{
> +       return -ENOTSUPP;
> +}
> +#endif /* IS_ENABLED(CONFIG_MTD_PHYSMAP_OF) */
> +
>  static const char * const rom_probe_types[] = {
>         "cfi_probe", "jedec_probe", "qinfo_probe", "map_rom", NULL
>  };
> @@ -97,18 +278,46 @@ static const char * const part_probe_types[] = {
>         "cmdlinepart", "RedBoot", "afs", NULL
>  };
>
> -static int physmap_flash_probe(struct platform_device *dev)
> +static int physmap_flash_pdata_init(struct platform_device *dev)
>  {
> +       struct physmap_flash_info *info = platform_get_drvdata(dev);
>         struct physmap_flash_data *physmap_data;
> +       unsigned int i;
> +       int err;
> +
> +       physmap_data = dev_get_platdata(&dev->dev);
> +       if (!physmap_data)
> +               return -EINVAL;
> +
> +       info->probe_type = physmap_data->probe_type;
> +       info->part_types = physmap_data->part_probe_types ? : part_probe_types;
> +       info->parts = physmap_data->parts;
> +       info->nparts = physmap_data->nr_parts;
> +
> +       if (physmap_data->init) {
> +               err = physmap_data->init(dev);
> +               if (err)
> +                       return err;
> +       }
> +
> +       for (i = 0; i < info->nmaps; i++) {
> +               info->maps[i].bankwidth = physmap_data->width;
> +               info->maps[i].pfow_base = physmap_data->pfow_base;
> +               info->maps[i].set_vpp = physmap_set_vpp;
> +       }
> +
> +       return 0;
> +}
> +
> +static int physmap_flash_probe(struct platform_device *dev)
> +{
>         struct physmap_flash_info *info;
>         const char * const *probe_type;
> -       const char * const *part_types;
>         int err = 0;
>         int i;
>
> -       physmap_data = dev_get_platdata(&dev->dev);
> -       if (!physmap_data)
> -               return -ENODEV;
> +       if (!dev->dev.of_node && dev_get_platdata(&dev->dev))
> +               return -EINVAL;

Maybe you want:
            if (!dev->dev.of_node && !dev_get_platdata(&dev->dev))
>
>         info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
>         if (!info)
> @@ -132,14 +341,15 @@ static int physmap_flash_probe(struct platform_device *dev)
>         if (!info->mtds)
>                 return -ENOMEM;
>
> -       if (physmap_data->init) {
> -               err = physmap_data->init(dev);
> -               if (err)
> -                       goto err_out;
> -       }
> -
>         platform_set_drvdata(dev, info);
>
> +       err = physmap_flash_of_init(dev);
> +       if (err)
> +               err = physmap_flash_pdata_init(dev);
> +

Maybe replace this with:
  if (dev->dev.of_node)
     err = physmap_flash_of_init(dev);
 else
    err = physmap_flash_pdata_init(dev);

Otherwise we are hiding the returncode from  of_init, which might be
defered probing.

> +       if (err)
> +               return err;
> +
>         for (i = 0; i < info->nmaps; i++) {
>                 struct resource *res;
>
> @@ -154,22 +364,22 @@ static int physmap_flash_probe(struct platform_device *dev)
>                            res);
>
>                 info->maps[i].name = dev_name(&dev->dev);
> -               info->maps[i].phys = res->start;
> +
> +               if (!info->maps[i].phys)
> +                       info->maps[i].phys = res->start;
> +
>                 info->maps[i].size = resource_size(res);
> -               info->maps[i].bankwidth = physmap_data->width;
> -               info->maps[i].set_vpp = physmap_set_vpp;
> -               info->maps[i].pfow_base = physmap_data->pfow_base;
>                 info->maps[i].map_priv_1 = (unsigned long)dev;
>
>                 simple_map_init(&info->maps[i]);
>
>                 probe_type = rom_probe_types;
> -               if (!physmap_data->probe_type) {
> +               if (!info->probe_type) {
>                         for (; !info->mtds[i] && *probe_type; probe_type++)
>                                 info->mtds[i] = do_map_probe(*probe_type,
>                                                              &info->maps[i]);
>                 } else {
> -                       info->mtds[i] = do_map_probe(physmap_data->probe_type,
> +                       info->mtds[i] = do_map_probe(info->probe_type,
>                                                      &info->maps[i]);
>                 }
>
> @@ -197,11 +407,9 @@ static int physmap_flash_probe(struct platform_device *dev)
>
>         spin_lock_init(&info->vpp_lock);
>
> -       part_types = physmap_data->part_probe_types ? : part_probe_types;
> -
> -       err = mtd_device_parse_register(info->cmtd, part_types, NULL,
> -                                       physmap_data->parts,
> -                                       physmap_data->nr_parts);
> +       mtd_set_of_node(info->cmtd, dev->dev.of_node);
> +       err = mtd_device_parse_register(info->cmtd, info->part_types, NULL,
> +                                       info->parts, info->nparts);
>         if (err)
>                 goto err_out;
>
> @@ -232,6 +440,7 @@ static struct platform_driver physmap_flash_driver = {
>         .shutdown       = physmap_flash_shutdown,
>         .driver         = {
>                 .name   = "physmap-flash",
> +               .of_match_table = of_flash_match,
>         },
>  };
>
> @@ -286,6 +495,7 @@ module_exit(physmap_exit);
>
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
> +MODULE_AUTHOR("Vitaly Wool <vwool@ru.mvista.com>");
>  MODULE_DESCRIPTION("Generic configurable MTD map driver");
>
>  /* legacy platform drivers can't hotplug or coldplg */
> diff --git a/drivers/mtd/maps/physmap_of_core.c b/drivers/mtd/maps/physmap_of_core.c
> deleted file mode 100644
> index ece605d78c21..000000000000
> --- a/drivers/mtd/maps/physmap_of_core.c
> +++ /dev/null
> @@ -1,368 +0,0 @@
> -/*
> - * Flash mappings described by the OF (or flattened) device tree
> - *
> - * Copyright (C) 2006 MontaVista Software Inc.
> - * Author: Vitaly Wool <vwool@ru.mvista.com>
> - *
> - * Revised to handle newer style flash binding by:
> - *   Copyright (C) 2007 David Gibson, IBM Corporation.
> - *
> - * This program is free software; you can redistribute  it and/or modify it
> - * under  the terms of  the GNU General  Public License as published by the
> - * Free Software Foundation;  either version 2 of the  License, or (at your
> - * option) any later version.
> - */
> -
> -#include <linux/module.h>
> -#include <linux/types.h>
> -#include <linux/device.h>
> -#include <linux/mtd/mtd.h>
> -#include <linux/mtd/map.h>
> -#include <linux/mtd/partitions.h>
> -#include <linux/mtd/concat.h>
> -#include <linux/mtd/cfi_endian.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
> -#include <linux/of_platform.h>
> -#include <linux/slab.h>
> -#include "physmap_of_gemini.h"
> -#include "physmap_of_versatile.h"
> -
> -struct of_flash_list {
> -       struct mtd_info *mtd;
> -       struct map_info map;
> -};
> -
> -struct of_flash {
> -       struct mtd_info         *cmtd;
> -       int list_size; /* number of elements in of_flash_list */
> -       struct of_flash_list    list[0];
> -};
> -
> -static int of_flash_remove(struct platform_device *dev)
> -{
> -       struct of_flash *info;
> -       int i;
> -
> -       info = dev_get_drvdata(&dev->dev);
> -       if (!info)
> -               return 0;
> -       dev_set_drvdata(&dev->dev, NULL);
> -
> -       if (info->cmtd) {
> -               mtd_device_unregister(info->cmtd);
> -               if (info->cmtd != info->list[0].mtd)
> -                       mtd_concat_destroy(info->cmtd);
> -       }
> -
> -       for (i = 0; i < info->list_size; i++)
> -               if (info->list[i].mtd)
> -                       map_destroy(info->list[i].mtd);
> -
> -       return 0;
> -}
> -
> -static const char * const rom_probe_types[] = {
> -       "cfi_probe", "jedec_probe", "map_rom" };
> -
> -/* Helper function to handle probing of the obsolete "direct-mapped"
> - * compatible binding, which has an extra "probe-type" property
> - * describing the type of flash probe necessary. */
> -static struct mtd_info *obsolete_probe(struct platform_device *dev,
> -                                      struct map_info *map)
> -{
> -       struct device_node *dp = dev->dev.of_node;
> -       const char *of_probe;
> -       struct mtd_info *mtd;
> -       int i;
> -
> -       dev_warn(&dev->dev, "Device tree uses obsolete \"direct-mapped\" "
> -                "flash binding\n");
> -
> -       of_probe = of_get_property(dp, "probe-type", NULL);
> -       if (!of_probe) {
> -               for (i = 0; i < ARRAY_SIZE(rom_probe_types); i++) {
> -                       mtd = do_map_probe(rom_probe_types[i], map);
> -                       if (mtd)
> -                               return mtd;
> -               }
> -               return NULL;
> -       } else if (strcmp(of_probe, "CFI") == 0) {
> -               return do_map_probe("cfi_probe", map);
> -       } else if (strcmp(of_probe, "JEDEC") == 0) {
> -               return do_map_probe("jedec_probe", map);
> -       } else {
> -               if (strcmp(of_probe, "ROM") != 0)
> -                       dev_warn(&dev->dev, "obsolete_probe: don't know probe "
> -                                "type '%s', mapping as rom\n", of_probe);
> -               return do_map_probe("map_rom", map);
> -       }
> -}
> -
> -/* When partitions are set we look for a linux,part-probe property which
> -   specifies the list of partition probers to use. If none is given then the
> -   default is use. These take precedence over other device tree
> -   information. */
> -static const char * const part_probe_types_def[] = {
> -       "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
> -
> -static const char * const *of_get_probes(struct device_node *dp)
> -{
> -       const char **res;
> -       int count;
> -
> -       count = of_property_count_strings(dp, "linux,part-probe");
> -       if (count < 0)
> -               return part_probe_types_def;
> -
> -       res = kcalloc(count + 1, sizeof(*res), GFP_KERNEL);
> -       if (!res)
> -               return NULL;
> -
> -       count = of_property_read_string_array(dp, "linux,part-probe", res,
> -                                             count);
> -       if (count < 0)
> -               return NULL;
> -
> -       return res;
> -}
> -
> -static void of_free_probes(const char * const *probes)
> -{
> -       if (probes != part_probe_types_def)
> -               kfree(probes);
> -}
> -
> -static const struct of_device_id of_flash_match[];
> -static int of_flash_probe(struct platform_device *dev)
> -{
> -       const char * const *part_probe_types;
> -       const struct of_device_id *match;
> -       struct device_node *dp = dev->dev.of_node;
> -       struct resource res;
> -       struct of_flash *info;
> -       const char *probe_type;
> -       const __be32 *width;
> -       int err;
> -       int i;
> -       int count;
> -       const __be32 *p;
> -       int reg_tuple_size;
> -       struct mtd_info **mtd_list = NULL;
> -       resource_size_t res_size;
> -       bool map_indirect;
> -       const char *mtd_name = NULL;
> -
> -       match = of_match_device(of_flash_match, &dev->dev);
> -       if (!match)
> -               return -EINVAL;
> -       probe_type = match->data;
> -
> -       reg_tuple_size = (of_n_addr_cells(dp) + of_n_size_cells(dp)) * sizeof(u32);
> -
> -       of_property_read_string(dp, "linux,mtd-name", &mtd_name);
> -
> -       /*
> -        * Get number of "reg" tuples. Scan for MTD devices on area's
> -        * described by each "reg" region. This makes it possible (including
> -        * the concat support) to support the Intel P30 48F4400 chips which
> -        * consists internally of 2 non-identical NOR chips on one die.
> -        */
> -       p = of_get_property(dp, "reg", &count);
> -       if (!p || count % reg_tuple_size != 0) {
> -               dev_err(&dev->dev, "Malformed reg property on %pOF\n",
> -                               dev->dev.of_node);
> -               err = -EINVAL;
> -               goto err_flash_remove;
> -       }
> -       count /= reg_tuple_size;
> -
> -       map_indirect = of_property_read_bool(dp, "no-unaligned-direct-access");
> -
> -       err = -ENOMEM;
> -       info = devm_kzalloc(&dev->dev,
> -                           sizeof(struct of_flash) +
> -                           sizeof(struct of_flash_list) * count, GFP_KERNEL);
> -       if (!info)
> -               goto err_flash_remove;
> -
> -       dev_set_drvdata(&dev->dev, info);
> -
> -       mtd_list = kcalloc(count, sizeof(*mtd_list), GFP_KERNEL);
> -       if (!mtd_list)
> -               goto err_flash_remove;
> -
> -       for (i = 0; i < count; i++) {
> -               err = -ENXIO;
> -               if (of_address_to_resource(dp, i, &res)) {
> -                       /*
> -                        * Continue with next register tuple if this
> -                        * one is not mappable
> -                        */
> -                       continue;
> -               }
> -
> -               dev_dbg(&dev->dev, "of_flash device: %pR\n", &res);
> -
> -               err = -EBUSY;
> -               res_size = resource_size(&res);
> -               info->list[i].map.virt = devm_ioremap_resource(&dev->dev, &res);
> -               if (IS_ERR(info->list[i].map.virt)) {
> -                       err = PTR_ERR(info->list[i].map.virt);
> -                       goto err_out;
> -               }
> -
> -               err = -ENXIO;
> -               width = of_get_property(dp, "bank-width", NULL);
> -               if (!width) {
> -                       dev_err(&dev->dev, "Can't get bank width from device"
> -                               " tree\n");
> -                       goto err_out;
> -               }
> -
> -               info->list[i].map.name = mtd_name ?: dev_name(&dev->dev);
> -               info->list[i].map.phys = res.start;
> -               info->list[i].map.size = res_size;
> -               info->list[i].map.bankwidth = be32_to_cpup(width);
> -               info->list[i].map.device_node = dp;
> -
> -               if (of_property_read_bool(dp, "big-endian"))
> -                       info->list[i].map.swap = CFI_BIG_ENDIAN;
> -               else if (of_property_read_bool(dp, "little-endian"))
> -                       info->list[i].map.swap = CFI_LITTLE_ENDIAN;
> -
> -               err = of_flash_probe_gemini(dev, dp, &info->list[i].map);
> -               if (err)
> -                       goto err_out;
> -               err = of_flash_probe_versatile(dev, dp, &info->list[i].map);
> -               if (err)
> -                       goto err_out;
> -
> -               simple_map_init(&info->list[i].map);
> -
> -               /*
> -                * On some platforms (e.g. MPC5200) a direct 1:1 mapping
> -                * may cause problems with JFFS2 usage, as the local bus (LPB)
> -                * doesn't support unaligned accesses as implemented in the
> -                * JFFS2 code via memcpy(). By setting NO_XIP, the
> -                * flash will not be exposed directly to the MTD users
> -                * (e.g. JFFS2) any more.
> -                */
> -               if (map_indirect)
> -                       info->list[i].map.phys = NO_XIP;
> -
> -               if (probe_type) {
> -                       info->list[i].mtd = do_map_probe(probe_type,
> -                                                        &info->list[i].map);
> -               } else {
> -                       info->list[i].mtd = obsolete_probe(dev,
> -                                                          &info->list[i].map);
> -               }
> -
> -               /* Fall back to mapping region as ROM */
> -               if (!info->list[i].mtd) {
> -                       dev_warn(&dev->dev,
> -                               "do_map_probe() failed for type %s\n",
> -                                probe_type);
> -
> -                       info->list[i].mtd = do_map_probe("map_rom",
> -                                                        &info->list[i].map);
> -               }
> -               mtd_list[i] = info->list[i].mtd;
> -
> -               err = -ENXIO;
> -               if (!info->list[i].mtd) {
> -                       dev_err(&dev->dev, "do_map_probe() failed\n");
> -                       goto err_out;
> -               } else {
> -                       info->list_size++;
> -               }
> -               info->list[i].mtd->dev.parent = &dev->dev;
> -       }
> -
> -       err = 0;
> -       info->cmtd = NULL;
> -       if (info->list_size == 1) {
> -               info->cmtd = info->list[0].mtd;
> -       } else if (info->list_size > 1) {
> -               /*
> -                * We detected multiple devices. Concatenate them together.
> -                */
> -               info->cmtd = mtd_concat_create(mtd_list, info->list_size,
> -                                              dev_name(&dev->dev));
> -       }
> -       if (info->cmtd == NULL)
> -               err = -ENXIO;
> -
> -       if (err)
> -               goto err_out;
> -
> -       info->cmtd->dev.parent = &dev->dev;
> -       mtd_set_of_node(info->cmtd, dp);
> -       part_probe_types = of_get_probes(dp);
> -       if (!part_probe_types) {
> -               err = -ENOMEM;
> -               goto err_out;
> -       }
> -       mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
> -                       NULL, 0);
> -       of_free_probes(part_probe_types);
> -
> -       kfree(mtd_list);
> -
> -       return 0;
> -
> -err_out:
> -       kfree(mtd_list);
> -err_flash_remove:
> -       of_flash_remove(dev);
> -
> -       return err;
> -}
> -
> -static const struct of_device_id of_flash_match[] = {
> -       {
> -               .compatible     = "cfi-flash",
> -               .data           = (void *)"cfi_probe",
> -       },
> -       {
> -               /* FIXME: JEDEC chips can't be safely and reliably
> -                * probed, although the mtd code gets it right in
> -                * practice most of the time.  We should use the
> -                * vendor and device ids specified by the binding to
> -                * bypass the heuristic probe code, but the mtd layer
> -                * provides, at present, no interface for doing so
> -                * :(. */
> -               .compatible     = "jedec-flash",
> -               .data           = (void *)"jedec_probe",
> -       },
> -       {
> -               .compatible     = "mtd-ram",
> -               .data           = (void *)"map_ram",
> -       },
> -       {
> -               .compatible     = "mtd-rom",
> -               .data           = (void *)"map_rom",
> -       },
> -       {
> -               .type           = "rom",
> -               .compatible     = "direct-mapped"
> -       },
> -       { },
> -};
> -MODULE_DEVICE_TABLE(of, of_flash_match);
> -
> -static struct platform_driver of_flash_driver = {
> -       .driver = {
> -               .name = "of-flash",
> -               .of_match_table = of_flash_match,
> -       },
> -       .probe          = of_flash_probe,
> -       .remove         = of_flash_remove,
> -};
> -
> -module_platform_driver(of_flash_driver);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Vitaly Wool <vwool@ru.mvista.com>");
> -MODULE_DESCRIPTION("Device tree based MTD map driver");
> --
> 2.14.1
>

Hi Boris

Maybe we want to leave the pdata example.

/**
 * The platform resource layout expected looks something like:
 * struct mtd_partition partitions[] = { ... };
 * struct physmap_flash_data flash_data =
.....


On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> Controlling some MSB address lines using GPIOs is just a small
> deviation of the generic physmap logic, and merging those two drivers
> allows us to share most of the probe logic, which is a good thing.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/maps/Kconfig           |  19 ++-
>  drivers/mtd/maps/Makefile          |   1 -
>  drivers/mtd/maps/gpio-addr-flash.c | 281 -------------------------------------
>  drivers/mtd/maps/physmap-core.c    | 150 +++++++++++++++++++-
>  4 files changed, 157 insertions(+), 294 deletions(-)
>  delete mode 100644 drivers/mtd/maps/gpio-addr-flash.c
>
> diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> index 5bffebacce86..4fd13d76b7b5 100644
> --- a/drivers/mtd/maps/Kconfig
> +++ b/drivers/mtd/maps/Kconfig
> @@ -94,6 +94,15 @@ config MTD_PHYSMAP_OF_GEMINI
>           platforms, some detection and setting up parallel mode on the
>           external interface.
>
> +config MTD_PHYSMAP_GPIO_ADDR
> +       bool "GPIO-assisted Flash Chip Support"
> +       depends on MTD_PHYSMAP
> +       depends on GPIOLIB || COMPILE_TEST
> +       depends on MTD_COMPLEX_MAPPINGS
> +       help
> +         Extend the physmap driver to allow flashes to be partially
> +         physically addressed and assisted by GPIOs.
> +
>  config MTD_PMC_MSP_EVM
>         tristate "CFI Flash device mapped on PMC-Sierra MSP"
>         depends on PMC_MSP && MTD_CFI
> @@ -334,16 +343,6 @@ config MTD_PCMCIA_ANONYMOUS
>
>           If unsure, say N.
>
> -config MTD_GPIO_ADDR
> -       tristate "GPIO-assisted Flash Chip Support"
> -       depends on GPIOLIB || COMPILE_TEST
> -       depends on MTD_COMPLEX_MAPPINGS
> -       help
> -         Map driver which allows flashes to be partially physically addressed
> -         and assisted by GPIOs.
> -
> -         If compiled as a module, it will be called gpio-addr-flash.
> -
>  config MTD_UCLINUX
>         bool "Generic uClinux RAM/ROM filesystem support"
>         depends on (MTD_RAM=y || MTD_ROM=y) && (!MMU || COLDFIRE)
> diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
> index ad32b185a120..acec0fbfa18d 100644
> --- a/drivers/mtd/maps/Makefile
> +++ b/drivers/mtd/maps/Makefile
> @@ -43,6 +43,5 @@ obj-$(CONFIG_MTD_PLATRAM)     += plat-ram.o
>  obj-$(CONFIG_MTD_INTEL_VR_NOR) += intel_vr_nor.o
>  obj-$(CONFIG_MTD_RBTX4939)     += rbtx4939-flash.o
>  obj-$(CONFIG_MTD_VMU)          += vmu-flash.o
> -obj-$(CONFIG_MTD_GPIO_ADDR)    += gpio-addr-flash.o
>  obj-$(CONFIG_MTD_LATCH_ADDR)   += latch-addr-flash.o
>  obj-$(CONFIG_MTD_LANTIQ)       += lantiq-flash.o
> diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
> deleted file mode 100644
> index a20e85aa770e..000000000000
> --- a/drivers/mtd/maps/gpio-addr-flash.c
> +++ /dev/null
> @@ -1,281 +0,0 @@
> -/*
> - * drivers/mtd/maps/gpio-addr-flash.c
> - *
> - * Handle the case where a flash device is mostly addressed using physical
> - * line and supplemented by GPIOs.  This way you can hook up say a 8MiB flash
> - * to a 2MiB memory range and use the GPIOs to select a particular range.
> - *
> - * Copyright © 2000 Nicolas Pitre <nico@cam.org>
> - * Copyright © 2005-2009 Analog Devices Inc.
> - *
> - * Enter bugs at http://blackfin.uclinux.org/
> - *
> - * Licensed under the GPL-2 or later.
> - */
> -
> -#include <linux/gpio.h>
> -#include <linux/gpio/consumer.h>
> -#include <linux/io.h>
> -#include <linux/kernel.h>
> -#include <linux/module.h>
> -#include <linux/mtd/mtd.h>
> -#include <linux/mtd/map.h>
> -#include <linux/mtd/partitions.h>
> -#include <linux/mtd/physmap.h>
> -#include <linux/platform_device.h>
> -#include <linux/slab.h>
> -#include <linux/types.h>
> -
> -#define win_mask(x) ((BIT(x)) - 1)
> -
> -#define DRIVER_NAME "gpio-addr-flash"
> -
> -/**
> - * struct async_state - keep GPIO flash state
> - *     @mtd:         MTD state for this mapping
> - *     @map:         MTD map state for this flash
> - *     @gpios:       Struct containing the array of GPIO descriptors
> - *     @gpio_values: cached GPIO values
> - *     @win_order:   dedicated memory size (if no GPIOs)
> - */
> -struct async_state {
> -       struct mtd_info *mtd;
> -       struct map_info map;
> -       struct gpio_descs *gpios;
> -       unsigned int gpio_values;
> -       unsigned int win_order;
> -};
> -#define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
> -
> -/**
> - * gf_set_gpios() - set GPIO address lines to access specified flash offset
> - *     @state: GPIO flash state
> - *     @ofs:   desired offset to access
> - *
> - * Rather than call the GPIO framework every time, cache the last-programmed
> - * value.  This speeds up sequential accesses (which are by far the most common
> - * type).
> - */
> -static void gf_set_gpios(struct async_state *state, unsigned long ofs)
> -{
> -       int i;
> -
> -       ofs >>= state->win_order;
> -
> -       if (ofs == state->gpio_values)
> -               return;
> -
> -       for (i = 0; i < state->gpios->ndescs; i++) {
> -               if ((ofs & BIT(i)) == (state->gpio_values & BIT(i)))
> -                       continue;
> -
> -               gpiod_set_value(state->gpios->desc[i], !!(ofs & BIT(i)));
> -       }
> -
> -       state->gpio_values = ofs;
> -}
> -
> -/**
> - * gf_read() - read a word at the specified offset
> - *     @map: MTD map state
> - *     @ofs: desired offset to read
> - */
> -static map_word gf_read(struct map_info *map, unsigned long ofs)
> -{
> -       struct async_state *state = gf_map_info_to_state(map);
> -       uint16_t word;
> -       map_word test;
> -
> -       gf_set_gpios(state, ofs);
> -
> -       word = readw(map->virt + (ofs & win_mask(state->win_order)));
> -       test.x[0] = word;
> -       return test;
> -}
> -
> -/**
> - * gf_copy_from() - copy a chunk of data from the flash
> - *     @map:  MTD map state
> - *     @to:   memory to copy to
> - *     @from: flash offset to copy from
> - *     @len:  how much to copy
> - *
> - * The "from" region may straddle more than one window, so toggle the GPIOs for
> - * each window region before reading its data.
> - */
> -static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssize_t len)
> -{
> -       struct async_state *state = gf_map_info_to_state(map);
> -
> -       int this_len;
> -
> -       while (len) {
> -               this_len = from & win_mask(state->win_order);
> -               this_len = BIT(state->win_order) - this_len;
> -               this_len = min_t(int, len, this_len);
> -
> -               gf_set_gpios(state, from);
> -               memcpy_fromio(to,
> -                             map->virt + (from & win_mask(state->win_order)),
> -                             this_len);
> -               len -= this_len;
> -               from += this_len;
> -               to += this_len;
> -       }
> -}
> -
> -/**
> - * gf_write() - write a word at the specified offset
> - *     @map: MTD map state
> - *     @ofs: desired offset to write
> - */
> -static void gf_write(struct map_info *map, map_word d1, unsigned long ofs)
> -{
> -       struct async_state *state = gf_map_info_to_state(map);
> -       uint16_t d;
> -
> -       gf_set_gpios(state, ofs);
> -
> -       d = d1.x[0];
> -       writew(d, map->virt + (ofs & win_mask(state->win_order)));
> -}
> -
> -/**
> - * gf_copy_to() - copy a chunk of data to the flash
> - *     @map:  MTD map state
> - *     @to:   flash offset to copy to
> - *     @from: memory to copy from
> - *     @len:  how much to copy
> - *
> - * See gf_copy_from() caveat.
> - */
> -static void gf_copy_to(struct map_info *map, unsigned long to,
> -                      const void *from, ssize_t len)
> -{
> -       struct async_state *state = gf_map_info_to_state(map);
> -
> -       int this_len;
> -
> -       while (len) {
> -               this_len = to & win_mask(state->win_order);
> -               this_len = BIT(state->win_order) - this_len;
> -               this_len = min_t(int, len, this_len);
> -
> -               gf_set_gpios(state, to);
> -               memcpy_toio(map->virt + (to & win_mask(state->win_order)),
> -                           from, len);
> -
> -               len -= this_len;
> -               to += this_len;
> -               from += this_len;
> -       }
> -}
> -
> -static const char * const part_probe_types[] = {
> -       "cmdlinepart", "RedBoot", NULL };
> -
> -/**
> - * gpio_flash_probe() - setup a mapping for a GPIO assisted flash
> - *     @pdev: platform device
> - *
> - * The platform resource layout expected looks something like:
> - * struct mtd_partition partitions[] = { ... };
> - * struct physmap_flash_data flash_data = { ... };
> - * static struct gpiod_lookup_table addr_flash_gpios = {
> - *             .dev_id = "gpio-addr-flash.0",
> - *             .table = {
> - *             GPIO_LOOKUP_IDX("gpio.0", 15, "addr", 0, GPIO_ACTIVE_HIGH),
> - *             GPIO_LOOKUP_IDX("gpio.0", 16, "addr", 1, GPIO_ACTIVE_HIGH),
> - *             );
> - * };
> - * gpiod_add_lookup_table(&addr_flash_gpios);
> - *
> - * struct resource flash_resource[] = {
> - *     {
> - *             .name  = "cfi_probe",
> - *             .start = 0x20000000,
> - *             .end   = 0x201fffff,
> - *             .flags = IORESOURCE_MEM,
> - *     },
> - * };
> - * struct platform_device flash_device = {
> - *     .name          = "gpio-addr-flash",
> - *     .dev           = { .platform_data = &flash_data, },
> - *     .num_resources = ARRAY_SIZE(flash_resource),
> - *     .resource      = flash_resource,
> - *     ...
> - * };
> - */
> -static int gpio_flash_probe(struct platform_device *pdev)
> -{
> -       struct physmap_flash_data *pdata;
> -       struct resource *memory;
> -       struct async_state *state;
> -
> -       pdata = dev_get_platdata(&pdev->dev);
> -       memory = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -       if (!memory)
> -               return -EINVAL;
> -
> -       state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
> -       if (!state)
> -               return -ENOMEM;
> -
> -       state->gpios = devm_gpiod_get_array(&pdev->dev, "addr", GPIOD_OUT_LOW);
> -       if (IS_ERR(state->gpios))
> -               return PTR_ERR(state->gpios);
> -
> -       state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
> -
> -       state->map.name       = DRIVER_NAME;
> -       state->map.read       = gf_read;
> -       state->map.copy_from  = gf_copy_from;
> -       state->map.write      = gf_write;
> -       state->map.copy_to    = gf_copy_to;
> -       state->map.bankwidth  = pdata->width;
> -       state->map.size       = BIT(state->win_order + state->gpios->ndescs);
> -       state->map.virt       = devm_ioremap_resource(&pdev->dev, memory);
> -       if (IS_ERR(state->map.virt))
> -               return PTR_ERR(state->map.virt);
> -
> -       state->map.phys       = NO_XIP;
> -       state->map.map_priv_1 = (unsigned long)state;
> -
> -       platform_set_drvdata(pdev, state);
> -
> -       dev_notice(&pdev->dev, "probing %d-bit flash bus\n",
> -                  state->map.bankwidth * 8);
> -       state->mtd = do_map_probe(memory->name, &state->map);
> -       if (!state->mtd)
> -               return -ENXIO;
> -       state->mtd->dev.parent = &pdev->dev;
> -
> -       mtd_device_parse_register(state->mtd, part_probe_types, NULL,
> -                                 pdata->parts, pdata->nr_parts);
> -
> -       return 0;
> -}
> -
> -static int gpio_flash_remove(struct platform_device *pdev)
> -{
> -       struct async_state *state = platform_get_drvdata(pdev);
> -
> -       mtd_device_unregister(state->mtd);
> -       map_destroy(state->mtd);
> -       return 0;
> -}
> -
> -static struct platform_driver gpio_flash_driver = {
> -       .probe          = gpio_flash_probe,
> -       .remove         = gpio_flash_remove,
> -       .driver         = {
> -               .name   = DRIVER_NAME,
> -       },
> -};
> -
> -module_platform_driver(gpio_flash_driver);
> -
> -MODULE_AUTHOR("Mike Frysinger <vapier@gentoo.org>");
> -MODULE_DESCRIPTION("MTD map driver for flashes addressed physically and with gpios");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/mtd/maps/physmap-core.c b/drivers/mtd/maps/physmap-core.c
> index 7a50ff9ef812..2dc33ae71335 100644
> --- a/drivers/mtd/maps/physmap-core.c
> +++ b/drivers/mtd/maps/physmap-core.c
> @@ -13,6 +13,14 @@
>   *
>   *    Revised to handle newer style flash binding by:
>   *    Copyright (C) 2007 David Gibson, IBM Corporation.
> + *
> + * GPIO address extension:
> + *    Handle the case where a flash device is mostly addressed using physical
> + *    line and supplemented by GPIOs.  This way you can hook up say a 8MiB flash
> + *    to a 2MiB memory range and use the GPIOs to select a particular range.
> + *
> + *    Copyright © 2000 Nicolas Pitre <nico@cam.org>
> + *    Copyright © 2005-2009 Analog Devices Inc.
>   */
>
>  #include <linux/module.h>
> @@ -30,6 +38,7 @@
>  #include <linux/mtd/cfi_endian.h>
>  #include <linux/io.h>
>  #include <linux/of_device.h>
> +#include <linux/gpio/consumer.h>
>
>  #include "physmap_of_gemini.h"
>  #include "physmap_of_versatile.h"
> @@ -45,6 +54,9 @@ struct physmap_flash_info {
>         const char * const      *part_types;
>         unsigned int            nparts;
>         const struct mtd_partition *parts;
> +       struct gpio_descs       *gpios;
> +       unsigned int            gpio_values;
> +       unsigned int            win_order;
>  };
>
>  static int physmap_flash_remove(struct platform_device *dev)
> @@ -104,6 +116,119 @@ static void physmap_set_vpp(struct map_info *map, int state)
>         spin_unlock_irqrestore(&info->vpp_lock, flags);
>  }
>
> +#if IS_ENABLED(CONFIG_MTD_PHYSMAP_GPIO_ADDR)
> +static void physmap_set_addr_gpios(struct physmap_flash_info *info,
> +                                  unsigned long ofs)
> +{
> +       unsigned int i;
> +
> +       ofs >>= info->win_order;
> +       if (info->gpio_values == ofs)
> +               return;
> +
> +       for (i = 0; i < info->gpios->ndescs; i++) {
> +               if ((BIT(i) & ofs) == (BIT(i) & info->gpio_values))
> +                       continue;
> +
> +               gpiod_set_value(info->gpios->desc[i], !!(BIT(i) & ofs));
> +       }
> +}
> +
> +#define win_mask(order)                (BIT(order) - 1)
> +
> +static map_word physmap_addr_gpios_read(struct map_info *map,
> +                                       unsigned long ofs)
> +{
> +       struct platform_device *pdev;
> +       struct physmap_flash_info *info;
> +       map_word mw;
> +       u16 word;
> +
> +       pdev = (struct platform_device *)map->map_priv_1;
> +       info = platform_get_drvdata(pdev);
> +       physmap_set_addr_gpios(info, ofs);
> +
> +       word = readw(map->virt + (ofs & win_mask(info->win_order)));
> +       mw.x[0] = word;
> +       return mw;
> +}
> +
> +static void physmap_addr_gpios_copy_from(struct map_info *map, void *buf,
> +                                        unsigned long ofs, ssize_t len)
> +{
> +       struct platform_device *pdev;
> +       struct physmap_flash_info *info;
> +
> +       pdev = (struct platform_device *)map->map_priv_1;
> +       info = platform_get_drvdata(pdev);
> +
> +       while (len) {
> +               unsigned int winofs = ofs & win_mask(info->win_order);
> +               unsigned int chunklen = min_t(unsigned int, len,
> +                                             BIT(info->win_order) - winofs);
> +
> +               physmap_set_addr_gpios(info, ofs);
> +               memcpy_fromio(buf, map->virt + winofs, chunklen);
> +               len -= chunklen;
> +               buf += chunklen;
> +               ofs += chunklen;
> +       }
> +}
> +
> +static void physmap_addr_gpios_write(struct map_info *map, map_word mw,
> +                                    unsigned long ofs)
> +{
> +       struct platform_device *pdev;
> +       struct physmap_flash_info *info;
> +       u16 word;
> +
> +       pdev = (struct platform_device *)map->map_priv_1;
> +       info = platform_get_drvdata(pdev);
> +       physmap_set_addr_gpios(info, ofs);
> +
> +       word = mw.x[0];
> +       writew(word, map->virt + (ofs & win_mask(info->win_order)));
> +}
> +
> +static void physmap_addr_gpios_copy_to(struct map_info *map, unsigned long ofs,
> +                                      const void *buf, ssize_t len)
> +{
> +       struct platform_device *pdev;
> +       struct physmap_flash_info *info;
> +
> +       pdev = (struct platform_device *)map->map_priv_1;
> +       info = platform_get_drvdata(pdev);
> +
> +       while (len) {
> +               unsigned int winofs = ofs & win_mask(info->win_order);
> +               unsigned int chunklen = min_t(unsigned int, len,
> +                                             BIT(info->win_order) - winofs);
> +
> +               physmap_set_addr_gpios(info, ofs);
> +               memcpy_toio(map->virt + winofs, buf, chunklen);
> +               len -= chunklen;
> +               buf += chunklen;
> +               ofs += chunklen;
> +       }
> +}
> +
> +static int physmap_addr_gpios_map_init(struct map_info *map)
> +{
> +       map->phys = NO_XIP;
> +       map->read = physmap_addr_gpios_read;
> +       map->copy_from = physmap_addr_gpios_copy_from;
> +       map->write = physmap_addr_gpios_write;
> +       map->copy_to = physmap_addr_gpios_copy_to;
> +
> +       return 0;
> +}
> +#else
> +static int physmap_addr_gpios_map_init(struct map_info *map)
> +{
> +       return -ENOTSUPP;
> +}
> +#endif
> +
>  #if IS_ENABLED(CONFIG_MTD_PHYSMAP_OF)
>  static const struct of_device_id of_flash_match[] = {
>         {
> @@ -343,6 +468,16 @@ static int physmap_flash_probe(struct platform_device *dev)
>
>         platform_set_drvdata(dev, info);
>
> +       info->gpios = devm_gpiod_get_array_optional(&dev->dev, "addr",
> +                                                   GPIOD_OUT_LOW);
> +       if (IS_ERR(info->gpios))
> +               return PTR_ERR(info->gpios);
> +
> +       if (info->gpios && info->nmaps > 1) {
> +               dev_err(&dev->dev, "addr-gpios only supported for nmaps == 1\n");
> +               return -EINVAL;
> +       }
> +
>         err = physmap_flash_of_init(dev);
>         if (err)
>                 err = physmap_flash_pdata_init(dev);
> @@ -368,10 +503,20 @@ static int physmap_flash_probe(struct platform_device *dev)
>                 if (!info->maps[i].phys)
>                         info->maps[i].phys = res->start;
>
> -               info->maps[i].size = resource_size(res);
> +               info->win_order = get_bitmask_order(resource_size(res)) - 1;
> +               info->maps[i].size = BIT(info->win_order +
> +                                        (info->gpios ?
> +                                         info->gpios->ndescs : 0));
> +
>                 info->maps[i].map_priv_1 = (unsigned long)dev;
>
> -               simple_map_init(&info->maps[i]);
> +               if (info->gpios) {
> +                       err = physmap_addr_gpios_map_init(&info->maps[i]);
> +                       if (err)
> +                               goto err_out;
> +               } else {
> +                       simple_map_init(&info->maps[i]);
> +               }
>
>                 probe_type = rom_probe_types;
>                 if (!info->probe_type) {
> @@ -496,6 +641,7 @@ module_exit(physmap_exit);
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
>  MODULE_AUTHOR("Vitaly Wool <vwool@ru.mvista.com>");
> +MODULE_AUTHOR("Mike Frysinger <vapier@gentoo.org>");
>  MODULE_DESCRIPTION("Generic configurable MTD map driver");
>
>  /* legacy platform drivers can't hotplug or coldplg */
> --
> 2.14.1
>

Hi Ricardo,

On Tue, 9 Oct 2018 08:58:40 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:


> > +
> > +static int physmap_flash_probe(struct platform_device *dev)
> > +{
> >         struct physmap_flash_info *info;
> >         const char * const *probe_type;
> > -       const char * const *part_types;
> >         int err = 0;
> >         int i;
> >
> > -       physmap_data = dev_get_platdata(&dev->dev);
> > -       if (!physmap_data)
> > -               return -ENODEV;
> > +       if (!dev->dev.of_node && dev_get_platdata(&dev->dev))
> > +               return -EINVAL;  
> 
> Maybe you want:
>             if (!dev->dev.of_node && !dev_get_platdata(&dev->dev))

Yes, I'll fix that.

> >
> >         info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
> >         if (!info)
> > @@ -132,14 +341,15 @@ static int physmap_flash_probe(struct platform_device *dev)
> >         if (!info->mtds)
> >                 return -ENOMEM;
> >
> > -       if (physmap_data->init) {
> > -               err = physmap_data->init(dev);
> > -               if (err)
> > -                       goto err_out;
> > -       }
> > -
> >         platform_set_drvdata(dev, info);
> >
> > +       err = physmap_flash_of_init(dev);
> > +       if (err)
> > +               err = physmap_flash_pdata_init(dev);
> > +  
> 
> Maybe replace this with:
>   if (dev->dev.of_node)
>      err = physmap_flash_of_init(dev);
>  else
>     err = physmap_flash_pdata_init(dev);
> 
> Otherwise we are hiding the returncode from  of_init, which might be
> defered probing.

Will fix that one too.

Thanks for your review.

Boris

On Tue, 9 Oct 2018 09:04:51 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> 
> Maybe we want to leave the pdata example.
> 
> /**
>  * The platform resource layout expected looks something like:
>  * struct mtd_partition partitions[] = { ... };
>  * struct physmap_flash_data flash_data =
> .....

Sure, I'll add it back.

> 
> 
> On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > Controlling some MSB address lines using GPIOs is just a small
> > deviation of the generic physmap logic, and merging those two drivers
> > allows us to share most of the probe logic, which is a good thing.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/mtd/maps/Kconfig           |  19 ++-
> >  drivers/mtd/maps/Makefile          |   1 -
> >  drivers/mtd/maps/gpio-addr-flash.c | 281 -------------------------------------
> >  drivers/mtd/maps/physmap-core.c    | 150 +++++++++++++++++++-
> >  4 files changed, 157 insertions(+), 294 deletions(-)
> >  delete mode 100644 drivers/mtd/maps/gpio-addr-flash.c
> >
> > diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> > index 5bffebacce86..4fd13d76b7b5 100644
> > --- a/drivers/mtd/maps/Kconfig
> > +++ b/drivers/mtd/maps/Kconfig
> > @@ -94,6 +94,15 @@ config MTD_PHYSMAP_OF_GEMINI
> >           platforms, some detection and setting up parallel mode on the
> >           external interface.
> >
> > +config MTD_PHYSMAP_GPIO_ADDR
> > +       bool "GPIO-assisted Flash Chip Support"
> > +       depends on MTD_PHYSMAP
> > +       depends on GPIOLIB || COMPILE_TEST
> > +       depends on MTD_COMPLEX_MAPPINGS
> > +       help
> > +         Extend the physmap driver to allow flashes to be partially
> > +         physically addressed and assisted by GPIOs.
> > +
> >  config MTD_PMC_MSP_EVM
> >         tristate "CFI Flash device mapped on PMC-Sierra MSP"
> >         depends on PMC_MSP && MTD_CFI
> > @@ -334,16 +343,6 @@ config MTD_PCMCIA_ANONYMOUS
> >
> >           If unsure, say N.
> >
> > -config MTD_GPIO_ADDR
> > -       tristate "GPIO-assisted Flash Chip Support"
> > -       depends on GPIOLIB || COMPILE_TEST
> > -       depends on MTD_COMPLEX_MAPPINGS
> > -       help
> > -         Map driver which allows flashes to be partially physically addressed
> > -         and assisted by GPIOs.
> > -
> > -         If compiled as a module, it will be called gpio-addr-flash.
> > -
> >  config MTD_UCLINUX
> >         bool "Generic uClinux RAM/ROM filesystem support"
> >         depends on (MTD_RAM=y || MTD_ROM=y) && (!MMU || COLDFIRE)
> > diff --git a/drivers/mtd/maps/Makefile b/drivers/mtd/maps/Makefile
> > index ad32b185a120..acec0fbfa18d 100644
> > --- a/drivers/mtd/maps/Makefile
> > +++ b/drivers/mtd/maps/Makefile
> > @@ -43,6 +43,5 @@ obj-$(CONFIG_MTD_PLATRAM)     += plat-ram.o
> >  obj-$(CONFIG_MTD_INTEL_VR_NOR) += intel_vr_nor.o
> >  obj-$(CONFIG_MTD_RBTX4939)     += rbtx4939-flash.o
> >  obj-$(CONFIG_MTD_VMU)          += vmu-flash.o
> > -obj-$(CONFIG_MTD_GPIO_ADDR)    += gpio-addr-flash.o
> >  obj-$(CONFIG_MTD_LATCH_ADDR)   += latch-addr-flash.o
> >  obj-$(CONFIG_MTD_LANTIQ)       += lantiq-flash.o
> > diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
> > deleted file mode 100644
> > index a20e85aa770e..000000000000
> > --- a/drivers/mtd/maps/gpio-addr-flash.c
> > +++ /dev/null
> > @@ -1,281 +0,0 @@
> > -/*
> > - * drivers/mtd/maps/gpio-addr-flash.c
> > - *
> > - * Handle the case where a flash device is mostly addressed using physical
> > - * line and supplemented by GPIOs.  This way you can hook up say a 8MiB flash
> > - * to a 2MiB memory range and use the GPIOs to select a particular range.
> > - *
> > - * Copyright © 2000 Nicolas Pitre <nico@cam.org>
> > - * Copyright © 2005-2009 Analog Devices Inc.
> > - *
> > - * Enter bugs at http://blackfin.uclinux.org/
> > - *
> > - * Licensed under the GPL-2 or later.
> > - */
> > -
> > -#include <linux/gpio.h>
> > -#include <linux/gpio/consumer.h>
> > -#include <linux/io.h>
> > -#include <linux/kernel.h>
> > -#include <linux/module.h>
> > -#include <linux/mtd/mtd.h>
> > -#include <linux/mtd/map.h>
> > -#include <linux/mtd/partitions.h>
> > -#include <linux/mtd/physmap.h>
> > -#include <linux/platform_device.h>
> > -#include <linux/slab.h>
> > -#include <linux/types.h>
> > -
> > -#define win_mask(x) ((BIT(x)) - 1)
> > -
> > -#define DRIVER_NAME "gpio-addr-flash"
> > -
> > -/**
> > - * struct async_state - keep GPIO flash state
> > - *     @mtd:         MTD state for this mapping
> > - *     @map:         MTD map state for this flash
> > - *     @gpios:       Struct containing the array of GPIO descriptors
> > - *     @gpio_values: cached GPIO values
> > - *     @win_order:   dedicated memory size (if no GPIOs)
> > - */
> > -struct async_state {
> > -       struct mtd_info *mtd;
> > -       struct map_info map;
> > -       struct gpio_descs *gpios;
> > -       unsigned int gpio_values;
> > -       unsigned int win_order;
> > -};
> > -#define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
> > -
> > -/**
> > - * gf_set_gpios() - set GPIO address lines to access specified flash offset
> > - *     @state: GPIO flash state
> > - *     @ofs:   desired offset to access
> > - *
> > - * Rather than call the GPIO framework every time, cache the last-programmed
> > - * value.  This speeds up sequential accesses (which are by far the most common
> > - * type).
> > - */
> > -static void gf_set_gpios(struct async_state *state, unsigned long ofs)
> > -{
> > -       int i;
> > -
> > -       ofs >>= state->win_order;
> > -
> > -       if (ofs == state->gpio_values)
> > -               return;
> > -
> > -       for (i = 0; i < state->gpios->ndescs; i++) {
> > -               if ((ofs & BIT(i)) == (state->gpio_values & BIT(i)))
> > -                       continue;
> > -
> > -               gpiod_set_value(state->gpios->desc[i], !!(ofs & BIT(i)));
> > -       }
> > -
> > -       state->gpio_values = ofs;
> > -}
> > -
> > -/**
> > - * gf_read() - read a word at the specified offset
> > - *     @map: MTD map state
> > - *     @ofs: desired offset to read
> > - */
> > -static map_word gf_read(struct map_info *map, unsigned long ofs)
> > -{
> > -       struct async_state *state = gf_map_info_to_state(map);
> > -       uint16_t word;
> > -       map_word test;
> > -
> > -       gf_set_gpios(state, ofs);
> > -
> > -       word = readw(map->virt + (ofs & win_mask(state->win_order)));
> > -       test.x[0] = word;
> > -       return test;
> > -}
> > -
> > -/**
> > - * gf_copy_from() - copy a chunk of data from the flash
> > - *     @map:  MTD map state
> > - *     @to:   memory to copy to
> > - *     @from: flash offset to copy from
> > - *     @len:  how much to copy
> > - *
> > - * The "from" region may straddle more than one window, so toggle the GPIOs for
> > - * each window region before reading its data.
> > - */
> > -static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssize_t len)
> > -{
> > -       struct async_state *state = gf_map_info_to_state(map);
> > -
> > -       int this_len;
> > -
> > -       while (len) {
> > -               this_len = from & win_mask(state->win_order);
> > -               this_len = BIT(state->win_order) - this_len;
> > -               this_len = min_t(int, len, this_len);
> > -
> > -               gf_set_gpios(state, from);
> > -               memcpy_fromio(to,
> > -                             map->virt + (from & win_mask(state->win_order)),
> > -                             this_len);
> > -               len -= this_len;
> > -               from += this_len;
> > -               to += this_len;
> > -       }
> > -}
> > -
> > -/**
> > - * gf_write() - write a word at the specified offset
> > - *     @map: MTD map state
> > - *     @ofs: desired offset to write
> > - */
> > -static void gf_write(struct map_info *map, map_word d1, unsigned long ofs)
> > -{
> > -       struct async_state *state = gf_map_info_to_state(map);
> > -       uint16_t d;
> > -
> > -       gf_set_gpios(state, ofs);
> > -
> > -       d = d1.x[0];
> > -       writew(d, map->virt + (ofs & win_mask(state->win_order)));
> > -}
> > -
> > -/**
> > - * gf_copy_to() - copy a chunk of data to the flash
> > - *     @map:  MTD map state
> > - *     @to:   flash offset to copy to
> > - *     @from: memory to copy from
> > - *     @len:  how much to copy
> > - *
> > - * See gf_copy_from() caveat.
> > - */
> > -static void gf_copy_to(struct map_info *map, unsigned long to,
> > -                      const void *from, ssize_t len)
> > -{
> > -       struct async_state *state = gf_map_info_to_state(map);
> > -
> > -       int this_len;
> > -
> > -       while (len) {
> > -               this_len = to & win_mask(state->win_order);
> > -               this_len = BIT(state->win_order) - this_len;
> > -               this_len = min_t(int, len, this_len);
> > -
> > -               gf_set_gpios(state, to);
> > -               memcpy_toio(map->virt + (to & win_mask(state->win_order)),
> > -                           from, len);
> > -
> > -               len -= this_len;
> > -               to += this_len;
> > -               from += this_len;
> > -       }
> > -}
> > -
> > -static const char * const part_probe_types[] = {
> > -       "cmdlinepart", "RedBoot", NULL };
> > -
> > -/**
> > - * gpio_flash_probe() - setup a mapping for a GPIO assisted flash
> > - *     @pdev: platform device
> > - *
> > - * The platform resource layout expected looks something like:
> > - * struct mtd_partition partitions[] = { ... };
> > - * struct physmap_flash_data flash_data = { ... };
> > - * static struct gpiod_lookup_table addr_flash_gpios = {
> > - *             .dev_id = "gpio-addr-flash.0",
> > - *             .table = {
> > - *             GPIO_LOOKUP_IDX("gpio.0", 15, "addr", 0, GPIO_ACTIVE_HIGH),
> > - *             GPIO_LOOKUP_IDX("gpio.0", 16, "addr", 1, GPIO_ACTIVE_HIGH),
> > - *             );
> > - * };
> > - * gpiod_add_lookup_table(&addr_flash_gpios);
> > - *
> > - * struct resource flash_resource[] = {
> > - *     {
> > - *             .name  = "cfi_probe",
> > - *             .start = 0x20000000,
> > - *             .end   = 0x201fffff,
> > - *             .flags = IORESOURCE_MEM,
> > - *     },
> > - * };
> > - * struct platform_device flash_device = {
> > - *     .name          = "gpio-addr-flash",
> > - *     .dev           = { .platform_data = &flash_data, },
> > - *     .num_resources = ARRAY_SIZE(flash_resource),
> > - *     .resource      = flash_resource,
> > - *     ...
> > - * };
> > - */
> > -static int gpio_flash_probe(struct platform_device *pdev)
> > -{
> > -       struct physmap_flash_data *pdata;
> > -       struct resource *memory;
> > -       struct async_state *state;
> > -
> > -       pdata = dev_get_platdata(&pdev->dev);
> > -       memory = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -
> > -       if (!memory)
> > -               return -EINVAL;
> > -
> > -       state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
> > -       if (!state)
> > -               return -ENOMEM;
> > -
> > -       state->gpios = devm_gpiod_get_array(&pdev->dev, "addr", GPIOD_OUT_LOW);
> > -       if (IS_ERR(state->gpios))
> > -               return PTR_ERR(state->gpios);
> > -
> > -       state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
> > -
> > -       state->map.name       = DRIVER_NAME;
> > -       state->map.read       = gf_read;
> > -       state->map.copy_from  = gf_copy_from;
> > -       state->map.write      = gf_write;
> > -       state->map.copy_to    = gf_copy_to;
> > -       state->map.bankwidth  = pdata->width;
> > -       state->map.size       = BIT(state->win_order + state->gpios->ndescs);
> > -       state->map.virt       = devm_ioremap_resource(&pdev->dev, memory);
> > -       if (IS_ERR(state->map.virt))
> > -               return PTR_ERR(state->map.virt);
> > -
> > -       state->map.phys       = NO_XIP;
> > -       state->map.map_priv_1 = (unsigned long)state;
> > -
> > -       platform_set_drvdata(pdev, state);
> > -
> > -       dev_notice(&pdev->dev, "probing %d-bit flash bus\n",
> > -                  state->map.bankwidth * 8);
> > -       state->mtd = do_map_probe(memory->name, &state->map);
> > -       if (!state->mtd)
> > -               return -ENXIO;
> > -       state->mtd->dev.parent = &pdev->dev;
> > -
> > -       mtd_device_parse_register(state->mtd, part_probe_types, NULL,
> > -                                 pdata->parts, pdata->nr_parts);
> > -
> > -       return 0;
> > -}
> > -
> > -static int gpio_flash_remove(struct platform_device *pdev)
> > -{
> > -       struct async_state *state = platform_get_drvdata(pdev);
> > -
> > -       mtd_device_unregister(state->mtd);
> > -       map_destroy(state->mtd);
> > -       return 0;
> > -}
> > -
> > -static struct platform_driver gpio_flash_driver = {
> > -       .probe          = gpio_flash_probe,
> > -       .remove         = gpio_flash_remove,
> > -       .driver         = {
> > -               .name   = DRIVER_NAME,
> > -       },
> > -};
> > -
> > -module_platform_driver(gpio_flash_driver);
> > -
> > -MODULE_AUTHOR("Mike Frysinger <vapier@gentoo.org>");
> > -MODULE_DESCRIPTION("MTD map driver for flashes addressed physically and with gpios");
> > -MODULE_LICENSE("GPL");
> > diff --git a/drivers/mtd/maps/physmap-core.c b/drivers/mtd/maps/physmap-core.c
> > index 7a50ff9ef812..2dc33ae71335 100644
> > --- a/drivers/mtd/maps/physmap-core.c
> > +++ b/drivers/mtd/maps/physmap-core.c
> > @@ -13,6 +13,14 @@
> >   *
> >   *    Revised to handle newer style flash binding by:
> >   *    Copyright (C) 2007 David Gibson, IBM Corporation.
> > + *
> > + * GPIO address extension:
> > + *    Handle the case where a flash device is mostly addressed using physical
> > + *    line and supplemented by GPIOs.  This way you can hook up say a 8MiB flash
> > + *    to a 2MiB memory range and use the GPIOs to select a particular range.
> > + *
> > + *    Copyright © 2000 Nicolas Pitre <nico@cam.org>
> > + *    Copyright © 2005-2009 Analog Devices Inc.
> >   */
> >
> >  #include <linux/module.h>
> > @@ -30,6 +38,7 @@
> >  #include <linux/mtd/cfi_endian.h>
> >  #include <linux/io.h>
> >  #include <linux/of_device.h>
> > +#include <linux/gpio/consumer.h>
> >
> >  #include "physmap_of_gemini.h"
> >  #include "physmap_of_versatile.h"
> > @@ -45,6 +54,9 @@ struct physmap_flash_info {
> >         const char * const      *part_types;
> >         unsigned int            nparts;
> >         const struct mtd_partition *parts;
> > +       struct gpio_descs       *gpios;
> > +       unsigned int            gpio_values;
> > +       unsigned int            win_order;
> >  };
> >
> >  static int physmap_flash_remove(struct platform_device *dev)
> > @@ -104,6 +116,119 @@ static void physmap_set_vpp(struct map_info *map, int state)
> >         spin_unlock_irqrestore(&info->vpp_lock, flags);
> >  }
> >
> > +#if IS_ENABLED(CONFIG_MTD_PHYSMAP_GPIO_ADDR)
> > +static void physmap_set_addr_gpios(struct physmap_flash_info *info,
> > +                                  unsigned long ofs)
> > +{
> > +       unsigned int i;
> > +
> > +       ofs >>= info->win_order;
> > +       if (info->gpio_values == ofs)
> > +               return;
> > +
> > +       for (i = 0; i < info->gpios->ndescs; i++) {
> > +               if ((BIT(i) & ofs) == (BIT(i) & info->gpio_values))
> > +                       continue;
> > +
> > +               gpiod_set_value(info->gpios->desc[i], !!(BIT(i) & ofs));
> > +       }
> > +}
> > +
> > +#define win_mask(order)                (BIT(order) - 1)
> > +
> > +static map_word physmap_addr_gpios_read(struct map_info *map,
> > +                                       unsigned long ofs)
> > +{
> > +       struct platform_device *pdev;
> > +       struct physmap_flash_info *info;
> > +       map_word mw;
> > +       u16 word;
> > +
> > +       pdev = (struct platform_device *)map->map_priv_1;
> > +       info = platform_get_drvdata(pdev);
> > +       physmap_set_addr_gpios(info, ofs);
> > +
> > +       word = readw(map->virt + (ofs & win_mask(info->win_order)));
> > +       mw.x[0] = word;
> > +       return mw;
> > +}
> > +
> > +static void physmap_addr_gpios_copy_from(struct map_info *map, void *buf,
> > +                                        unsigned long ofs, ssize_t len)
> > +{
> > +       struct platform_device *pdev;
> > +       struct physmap_flash_info *info;
> > +
> > +       pdev = (struct platform_device *)map->map_priv_1;
> > +       info = platform_get_drvdata(pdev);
> > +
> > +       while (len) {
> > +               unsigned int winofs = ofs & win_mask(info->win_order);
> > +               unsigned int chunklen = min_t(unsigned int, len,
> > +                                             BIT(info->win_order) - winofs);
> > +
> > +               physmap_set_addr_gpios(info, ofs);
> > +               memcpy_fromio(buf, map->virt + winofs, chunklen);
> > +               len -= chunklen;
> > +               buf += chunklen;
> > +               ofs += chunklen;
> > +       }
> > +}
> > +
> > +static void physmap_addr_gpios_write(struct map_info *map, map_word mw,
> > +                                    unsigned long ofs)
> > +{
> > +       struct platform_device *pdev;
> > +       struct physmap_flash_info *info;
> > +       u16 word;
> > +
> > +       pdev = (struct platform_device *)map->map_priv_1;
> > +       info = platform_get_drvdata(pdev);
> > +       physmap_set_addr_gpios(info, ofs);
> > +
> > +       word = mw.x[0];
> > +       writew(word, map->virt + (ofs & win_mask(info->win_order)));
> > +}
> > +
> > +static void physmap_addr_gpios_copy_to(struct map_info *map, unsigned long ofs,
> > +                                      const void *buf, ssize_t len)
> > +{
> > +       struct platform_device *pdev;
> > +       struct physmap_flash_info *info;
> > +
> > +       pdev = (struct platform_device *)map->map_priv_1;
> > +       info = platform_get_drvdata(pdev);
> > +
> > +       while (len) {
> > +               unsigned int winofs = ofs & win_mask(info->win_order);
> > +               unsigned int chunklen = min_t(unsigned int, len,
> > +                                             BIT(info->win_order) - winofs);
> > +
> > +               physmap_set_addr_gpios(info, ofs);
> > +               memcpy_toio(map->virt + winofs, buf, chunklen);
> > +               len -= chunklen;
> > +               buf += chunklen;
> > +               ofs += chunklen;
> > +       }
> > +}
> > +
> > +static int physmap_addr_gpios_map_init(struct map_info *map)
> > +{
> > +       map->phys = NO_XIP;
> > +       map->read = physmap_addr_gpios_read;
> > +       map->copy_from = physmap_addr_gpios_copy_from;
> > +       map->write = physmap_addr_gpios_write;
> > +       map->copy_to = physmap_addr_gpios_copy_to;
> > +
> > +       return 0;
> > +}
> > +#else
> > +static int physmap_addr_gpios_map_init(struct map_info *map)
> > +{
> > +       return -ENOTSUPP;
> > +}
> > +#endif
> > +
> >  #if IS_ENABLED(CONFIG_MTD_PHYSMAP_OF)
> >  static const struct of_device_id of_flash_match[] = {
> >         {
> > @@ -343,6 +468,16 @@ static int physmap_flash_probe(struct platform_device *dev)
> >
> >         platform_set_drvdata(dev, info);
> >
> > +       info->gpios = devm_gpiod_get_array_optional(&dev->dev, "addr",
> > +                                                   GPIOD_OUT_LOW);
> > +       if (IS_ERR(info->gpios))
> > +               return PTR_ERR(info->gpios);
> > +
> > +       if (info->gpios && info->nmaps > 1) {
> > +               dev_err(&dev->dev, "addr-gpios only supported for nmaps == 1\n");
> > +               return -EINVAL;
> > +       }
> > +
> >         err = physmap_flash_of_init(dev);
> >         if (err)
> >                 err = physmap_flash_pdata_init(dev);
> > @@ -368,10 +503,20 @@ static int physmap_flash_probe(struct platform_device *dev)
> >                 if (!info->maps[i].phys)
> >                         info->maps[i].phys = res->start;
> >
> > -               info->maps[i].size = resource_size(res);
> > +               info->win_order = get_bitmask_order(resource_size(res)) - 1;
> > +               info->maps[i].size = BIT(info->win_order +
> > +                                        (info->gpios ?
> > +                                         info->gpios->ndescs : 0));
> > +
> >                 info->maps[i].map_priv_1 = (unsigned long)dev;
> >
> > -               simple_map_init(&info->maps[i]);
> > +               if (info->gpios) {
> > +                       err = physmap_addr_gpios_map_init(&info->maps[i]);
> > +                       if (err)
> > +                               goto err_out;
> > +               } else {
> > +                       simple_map_init(&info->maps[i]);
> > +               }
> >
> >                 probe_type = rom_probe_types;
> >                 if (!info->probe_type) {
> > @@ -496,6 +641,7 @@ module_exit(physmap_exit);
> >  MODULE_LICENSE("GPL");
> >  MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
> >  MODULE_AUTHOR("Vitaly Wool <vwool@ru.mvista.com>");
> > +MODULE_AUTHOR("Mike Frysinger <vapier@gentoo.org>");
> >  MODULE_DESCRIPTION("Generic configurable MTD map driver");
> >
> >  /* legacy platform drivers can't hotplug or coldplg */
> > --
> > 2.14.1
> >  
> 
>

Hi Boris
On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> Stop manipulating the dev->resource array directly and use the
> platform_get_resource() helper instead.
>
> While at it, fix the loop check so that we never overflow the info->maps
> and info->mtds array even if the number of resources attached to the
> platform dev is higher than MAX_RESOURCES.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/maps/physmap.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
> index 4010afee4a33..e5b15ec2cb04 100644
> --- a/drivers/mtd/maps/physmap.c
> +++ b/drivers/mtd/maps/physmap.c
> @@ -122,23 +122,28 @@ static int physmap_flash_probe(struct platform_device *dev)
>
>         platform_set_drvdata(dev, info);
>
> -       for (i = 0; i < dev->num_resources; i++) {
> +       for (i = 0; i < MAX_RESOURCES; i++) {
> +               struct resource *res;
> +
> +               res = platform_get_resource(dev, IORESOURCE_MEM, i);
> +               if (res)
> +                       break;

Maybe if (!res)

> +
>                 printk(KERN_NOTICE "physmap platform flash device: %.8llx at %.8llx\n",
> -                      (unsigned long long)resource_size(&dev->resource[i]),
> -                      (unsigned long long)dev->resource[i].start);
> +                      (unsigned long long)resource_size(res),
> +                      (unsigned long long)res->start);
>
> -               if (!devm_request_mem_region(&dev->dev,
> -                       dev->resource[i].start,
> -                       resource_size(&dev->resource[i]),
> -                       dev_name(&dev->dev))) {
> +               if (!devm_request_mem_region(&dev->dev, res->start,
> +                                            resource_size(res),
> +                                            dev_name(&dev->dev))) {
>                         dev_err(&dev->dev, "Could not reserve memory region\n");
>                         err = -ENOMEM;
>                         goto err_out;
>                 }
>
>                 info->maps[i].name = dev_name(&dev->dev);
> -               info->maps[i].phys = dev->resource[i].start;
> -               info->maps[i].size = resource_size(&dev->resource[i]);
> +               info->maps[i].phys = res->start;
> +               info->maps[i].size = resource_size(res);
>                 info->maps[i].bankwidth = physmap_data->width;
>                 info->maps[i].set_vpp = physmap_set_vpp;
>                 info->maps[i].pfow_base = physmap_data->pfow_base;
> @@ -172,9 +177,11 @@ static int physmap_flash_probe(struct platform_device *dev)
>                 info->mtds[i]->dev.parent = &dev->dev;
>         }
>
> -       if (devices_found == 1) {
> +       if (!devices_found) {
> +               err = -ENODEV;
> +       } else if (devices_found == 1) {
>                 info->cmtd = info->mtds[0];
> -       } else if (devices_found > 1) {
> +       } else {
>                 /*
>                  * We detected multiple devices. Concatenate them together.
>                  */
> --
> 2.14.1
>

Hi Boris
On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> Remove the MAX_RESOURCES limitation by dynamically allocating the
> ->mtds[] and ->maps[] at probe time based on the number of iomem
> resources attached to the platform device.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/maps/physmap.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
> index cbc50b53ec9d..86679d149a49 100644
> --- a/drivers/mtd/maps/physmap.c
> +++ b/drivers/mtd/maps/physmap.c
> @@ -22,12 +22,11 @@
>  #include <linux/mtd/concat.h>
>  #include <linux/io.h>
>
> -#define MAX_RESOURCES          4
> -
>  struct physmap_flash_info {
> -       struct mtd_info         *mtds[MAX_RESOURCES];
> +       unsigned int            nmaps;
> +       struct mtd_info         **mtds;
>         struct mtd_info         *cmtd;
> -       struct map_info         maps[MAX_RESOURCES];
> +       struct map_info         *maps;
>         spinlock_t              vpp_lock;
>         int                     vpp_refcnt;
>  };
> @@ -50,7 +49,7 @@ static int physmap_flash_remove(struct platform_device *dev)
>                         mtd_concat_destroy(info->cmtd);
>         }
>
> -       for (i = 0; i < MAX_RESOURCES; i++) {
> +       for (i = 0; i < info->nmaps; i++) {
>                 if (info->mtds[i] != NULL)
>                         map_destroy(info->mtds[i]);
>         }
> @@ -101,7 +100,6 @@ static int physmap_flash_probe(struct platform_device *dev)
>         const char * const *part_types;
>         int err = 0;
>         int i;
> -       int devices_found = 0;
>
>         physmap_data = dev_get_platdata(&dev->dev);
>         if (physmap_data == NULL)
> @@ -114,6 +112,24 @@ static int physmap_flash_probe(struct platform_device *dev)
>                 goto err_out;
>         }
>
> +       while (platform_get_resource(dev, IORESOURCE_MEM, info->nmaps))
> +               info->nmaps++;

Maybe you prefer:
for (info->nmaps = 0; platform_get_resource(dev, IORESOURCE_MEM,
info->nmaps) ; info->nmaps++);

(your choice, they are probably compiled into the same assembly)

> +
> +       if (!info->nmaps)
> +               return -ENODEV;
> +
> +       info->maps = devm_kzalloc(&dev->dev,
> +                                 sizeof(*info->maps) * info->nmaps,
> +                                 GFP_KERNEL);
> +       if (!info->maps)
> +               return -ENOMEM;
> +
> +       info->mtds = devm_kzalloc(&dev->dev,
> +                                 sizeof(*info->mtds) * info->nmaps,
> +                                 GFP_KERNEL);
> +       if (!info->mtds)
> +               return -ENOMEM;
> +
>         if (physmap_data->init) {
>                 err = physmap_data->init(dev);
>                 if (err)
> @@ -122,13 +138,10 @@ static int physmap_flash_probe(struct platform_device *dev)
>
>         platform_set_drvdata(dev, info);
>
> -       for (i = 0; i < MAX_RESOURCES; i++) {
> +       for (i = 0; i < info->nmaps; i++) {
>                 struct resource *res;
>
>                 res = platform_get_resource(dev, IORESOURCE_MEM, i);
> -               if (res)
> -                       break;
> -
>                 info->maps[i].virt = devm_ioremap_resource(&dev->dev, res);
>                 if (IS_ERR(info->maps[i].virt)) {
>                         err = PTR_ERR(info->maps[i].virt);
> @@ -159,21 +172,18 @@ static int physmap_flash_probe(struct platform_device *dev)
>                         dev_err(&dev->dev, "map_probe failed\n");
>                         err = -ENXIO;
>                         goto err_out;
> -               } else {
> -                       devices_found++;
>                 }
>                 info->mtds[i]->dev.parent = &dev->dev;
>         }
>
> -       if (!devices_found) {
> -               err = -ENODEV;
> -       } else if (devices_found == 1) {
> +       if (info->nmaps == 1) {
>                 info->cmtd = info->mtds[0];
>         } else {
>                 /*
>                  * We detected multiple devices. Concatenate them together.
>                  */
> -               info->cmtd = mtd_concat_create(info->mtds, devices_found, dev_name(&dev->dev));
> +               info->cmtd = mtd_concat_create(info->mtds, info->nmaps,
> +                                              dev_name(&dev->dev));
>                 if (info->cmtd == NULL)
>                         err = -ENXIO;
>         }
> @@ -199,7 +209,7 @@ static void physmap_flash_shutdown(struct platform_device *dev)
>         struct physmap_flash_info *info = platform_get_drvdata(dev);
>         int i;
>
> -       for (i = 0; i < MAX_RESOURCES && info->mtds[i]; i++)
> +       for (i = 0; i < info->nmaps && info->mtds[i]; i++)
>                 if (mtd_suspend(info->mtds[i]) == 0)
>                         mtd_resume(info->mtds[i]);
>  }
> --
> 2.14.1
>

Hi Boris
On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> Fix the following coding style issues:
> - != NULL and == NULL test replaced by ! (or nothing)
> - split over 80 chars lines
> - add missing braces in multi-line if() {} else {} statements
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/maps/physmap.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
> index 7d30f3524d35..e27051bc5dc6 100644
> --- a/drivers/mtd/maps/physmap.c
> +++ b/drivers/mtd/maps/physmap.c
> @@ -38,7 +38,7 @@ static int physmap_flash_remove(struct platform_device *dev)
>         int i, err;
>
>         info = platform_get_drvdata(dev);
> -       if (info == NULL)
> +       if (!info)
>                 return 0;
>
>         physmap_data = dev_get_platdata(&dev->dev);
> @@ -53,7 +53,7 @@ static int physmap_flash_remove(struct platform_device *dev)
>         }
>
>         for (i = 0; i < info->nmaps; i++) {
> -               if (info->mtds[i] != NULL)
> +               if (info->mtds[i])
>                         map_destroy(info->mtds[i]);
>         }
>
> @@ -90,10 +90,12 @@ static void physmap_set_vpp(struct map_info *map, int state)
>  }
>
>  static const char * const rom_probe_types[] = {
> -       "cfi_probe", "jedec_probe", "qinfo_probe", "map_rom", NULL };
> +       "cfi_probe", "jedec_probe", "qinfo_probe", "map_rom", NULL
> +};
>
>  static const char * const part_probe_types[] = {
> -       "cmdlinepart", "RedBoot", "afs", NULL };
> +       "cmdlinepart", "RedBoot", "afs", NULL
> +};
>
>  static int physmap_flash_probe(struct platform_device *dev)
>  {
> @@ -105,11 +107,10 @@ static int physmap_flash_probe(struct platform_device *dev)
>         int i;
>
>         physmap_data = dev_get_platdata(&dev->dev);
> -       if (physmap_data == NULL)
> +       if (!physmap_data)
>                 return -ENODEV;
>
> -       info = devm_kzalloc(&dev->dev, sizeof(struct physmap_flash_info),
> -                           GFP_KERNEL);
> +       info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
>         if (!info)
>                 return -ENOMEM;
>
> @@ -163,13 +164,16 @@ static int physmap_flash_probe(struct platform_device *dev)
>                 simple_map_init(&info->maps[i]);
>
>                 probe_type = rom_probe_types;
> -               if (physmap_data->probe_type == NULL) {
> -                       for (; info->mtds[i] == NULL && *probe_type != NULL; probe_type++)
> -                               info->mtds[i] = do_map_probe(*probe_type, &info->maps[i]);
> -               } else
> -                       info->mtds[i] = do_map_probe(physmap_data->probe_type, &info->maps[i]);
> +               if (!physmap_data->probe_type) {
> +                       for (; !info->mtds[i] && *probe_type; probe_type++)
> +                               info->mtds[i] = do_map_probe(*probe_type,
> +                                                            &info->maps[i]);
> +               } else {
> +                       info->mtds[i] = do_map_probe(physmap_data->probe_type,
> +                                                    &info->maps[i]);
> +               }

Now that you are at it, maybe you want to change the order of the if branches:

if (true){
} else{
}

and move:
probe_type = rom_probe_types;
into the branch

and fix:
const char * const *probe_type;


If you do not want to change it, is also fine :P

>
> -               if (info->mtds[i] == NULL) {
> +               if (!info->mtds[i]) {
>                         dev_err(&dev->dev, "map_probe failed\n");
>                         err = -ENXIO;
>                         goto err_out;
> @@ -185,7 +189,7 @@ static int physmap_flash_probe(struct platform_device *dev)
>                  */
>                 info->cmtd = mtd_concat_create(info->mtds, info->nmaps,
>                                                dev_name(&dev->dev));
> -               if (info->cmtd == NULL)
> +               if (!info->cmtd)
>                         err = -ENXIO;
>         }
>         if (err)
> @@ -231,7 +235,6 @@ static struct platform_driver physmap_flash_driver = {
>         },
>  };
>
> -
>  #ifdef CONFIG_MTD_PHYSMAP_COMPAT
>  static struct physmap_flash_data physmap_flash_data = {
>         .width          = CONFIG_MTD_PHYSMAP_BANKWIDTH,
> --
> 2.14.1
>

On Tue, 9 Oct 2018 09:37:19 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > Fix the following coding style issues:
> > - != NULL and == NULL test replaced by ! (or nothing)
> > - split over 80 chars lines
> > - add missing braces in multi-line if() {} else {} statements
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/mtd/maps/physmap.c | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
> > index 7d30f3524d35..e27051bc5dc6 100644
> > --- a/drivers/mtd/maps/physmap.c
> > +++ b/drivers/mtd/maps/physmap.c
> > @@ -38,7 +38,7 @@ static int physmap_flash_remove(struct platform_device *dev)
> >         int i, err;
> >
> >         info = platform_get_drvdata(dev);
> > -       if (info == NULL)
> > +       if (!info)
> >                 return 0;
> >
> >         physmap_data = dev_get_platdata(&dev->dev);
> > @@ -53,7 +53,7 @@ static int physmap_flash_remove(struct platform_device *dev)
> >         }
> >
> >         for (i = 0; i < info->nmaps; i++) {
> > -               if (info->mtds[i] != NULL)
> > +               if (info->mtds[i])
> >                         map_destroy(info->mtds[i]);
> >         }
> >
> > @@ -90,10 +90,12 @@ static void physmap_set_vpp(struct map_info *map, int state)
> >  }
> >
> >  static const char * const rom_probe_types[] = {
> > -       "cfi_probe", "jedec_probe", "qinfo_probe", "map_rom", NULL };
> > +       "cfi_probe", "jedec_probe", "qinfo_probe", "map_rom", NULL
> > +};
> >
> >  static const char * const part_probe_types[] = {
> > -       "cmdlinepart", "RedBoot", "afs", NULL };
> > +       "cmdlinepart", "RedBoot", "afs", NULL
> > +};
> >
> >  static int physmap_flash_probe(struct platform_device *dev)
> >  {
> > @@ -105,11 +107,10 @@ static int physmap_flash_probe(struct platform_device *dev)
> >         int i;
> >
> >         physmap_data = dev_get_platdata(&dev->dev);
> > -       if (physmap_data == NULL)
> > +       if (!physmap_data)
> >                 return -ENODEV;
> >
> > -       info = devm_kzalloc(&dev->dev, sizeof(struct physmap_flash_info),
> > -                           GFP_KERNEL);
> > +       info = devm_kzalloc(&dev->dev, sizeof(*info), GFP_KERNEL);
> >         if (!info)
> >                 return -ENOMEM;
> >
> > @@ -163,13 +164,16 @@ static int physmap_flash_probe(struct platform_device *dev)
> >                 simple_map_init(&info->maps[i]);
> >
> >                 probe_type = rom_probe_types;
> > -               if (physmap_data->probe_type == NULL) {
> > -                       for (; info->mtds[i] == NULL && *probe_type != NULL; probe_type++)
> > -                               info->mtds[i] = do_map_probe(*probe_type, &info->maps[i]);
> > -               } else
> > -                       info->mtds[i] = do_map_probe(physmap_data->probe_type, &info->maps[i]);
> > +               if (!physmap_data->probe_type) {
> > +                       for (; !info->mtds[i] && *probe_type; probe_type++)
> > +                               info->mtds[i] = do_map_probe(*probe_type,
> > +                                                            &info->maps[i]);
> > +               } else {
> > +                       info->mtds[i] = do_map_probe(physmap_data->probe_type,
> > +                                                    &info->maps[i]);
> > +               }  
> 
> Now that you are at it, maybe you want to change the order of the if branches:
> 
> if (true){
> } else{
> }
> 
> and move:
> probe_type = rom_probe_types;
> into the branch
> 
> and fix:
> const char * const *probe_type;
> 
> 
> If you do not want to change it, is also fine :P

I don't mind changing that, but not in this patch :-).

> 
> >
> > -               if (info->mtds[i] == NULL) {
> > +               if (!info->mtds[i]) {
> >                         dev_err(&dev->dev, "map_probe failed\n");
> >                         err = -ENXIO;
> >                         goto err_out;
> > @@ -185,7 +189,7 @@ static int physmap_flash_probe(struct platform_device *dev)
> >                  */
> >                 info->cmtd = mtd_concat_create(info->mtds, info->nmaps,
> >                                                dev_name(&dev->dev));
> > -               if (info->cmtd == NULL)
> > +               if (!info->cmtd)
> >                         err = -ENXIO;
> >         }
> >         if (err)
> > @@ -231,7 +235,6 @@ static struct platform_driver physmap_flash_driver = {
> >         },
> >  };
> >
> > -
> >  #ifdef CONFIG_MTD_PHYSMAP_COMPAT
> >  static struct physmap_flash_data physmap_flash_data = {
> >         .width          = CONFIG_MTD_PHYSMAP_BANKWIDTH,
> > --
> > 2.14.1
> >  
> 
>

On Tue, 9 Oct 2018 09:31:44 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > Remove the MAX_RESOURCES limitation by dynamically allocating the  
> > ->mtds[] and ->maps[] at probe time based on the number of iomem  
> > resources attached to the platform device.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/mtd/maps/physmap.c | 44 +++++++++++++++++++++++++++-----------------
> >  1 file changed, 27 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
> > index cbc50b53ec9d..86679d149a49 100644
> > --- a/drivers/mtd/maps/physmap.c
> > +++ b/drivers/mtd/maps/physmap.c
> > @@ -22,12 +22,11 @@
> >  #include <linux/mtd/concat.h>
> >  #include <linux/io.h>
> >
> > -#define MAX_RESOURCES          4
> > -
> >  struct physmap_flash_info {
> > -       struct mtd_info         *mtds[MAX_RESOURCES];
> > +       unsigned int            nmaps;
> > +       struct mtd_info         **mtds;
> >         struct mtd_info         *cmtd;
> > -       struct map_info         maps[MAX_RESOURCES];
> > +       struct map_info         *maps;
> >         spinlock_t              vpp_lock;
> >         int                     vpp_refcnt;
> >  };
> > @@ -50,7 +49,7 @@ static int physmap_flash_remove(struct platform_device *dev)
> >                         mtd_concat_destroy(info->cmtd);
> >         }
> >
> > -       for (i = 0; i < MAX_RESOURCES; i++) {
> > +       for (i = 0; i < info->nmaps; i++) {
> >                 if (info->mtds[i] != NULL)
> >                         map_destroy(info->mtds[i]);
> >         }
> > @@ -101,7 +100,6 @@ static int physmap_flash_probe(struct platform_device *dev)
> >         const char * const *part_types;
> >         int err = 0;
> >         int i;
> > -       int devices_found = 0;
> >
> >         physmap_data = dev_get_platdata(&dev->dev);
> >         if (physmap_data == NULL)
> > @@ -114,6 +112,24 @@ static int physmap_flash_probe(struct platform_device *dev)
> >                 goto err_out;
> >         }
> >
> > +       while (platform_get_resource(dev, IORESOURCE_MEM, info->nmaps))
> > +               info->nmaps++;  
> 
> Maybe you prefer:
> for (info->nmaps = 0; platform_get_resource(dev, IORESOURCE_MEM,
> info->nmaps) ; info->nmaps++);

I don't like when while/for loop bodies are empty, that's why I went for
the while() loop ;-).

> 
> (your choice, they are probably compiled into the same assembly)
> 
> > +
> > +       if (!info->nmaps)
> > +               return -ENODEV;
> > +
> > +       info->maps = devm_kzalloc(&dev->dev,
> > +                                 sizeof(*info->maps) * info->nmaps,
> > +                                 GFP_KERNEL);
> > +       if (!info->maps)
> > +               return -ENOMEM;
> > +
> > +       info->mtds = devm_kzalloc(&dev->dev,
> > +                                 sizeof(*info->mtds) * info->nmaps,
> > +                                 GFP_KERNEL);
> > +       if (!info->mtds)
> > +               return -ENOMEM;
> > +
> >         if (physmap_data->init) {
> >                 err = physmap_data->init(dev);
> >                 if (err)
> > @@ -122,13 +138,10 @@ static int physmap_flash_probe(struct platform_device *dev)
> >
> >         platform_set_drvdata(dev, info);
> >
> > -       for (i = 0; i < MAX_RESOURCES; i++) {
> > +       for (i = 0; i < info->nmaps; i++) {
> >                 struct resource *res;
> >
> >                 res = platform_get_resource(dev, IORESOURCE_MEM, i);
> > -               if (res)
> > -                       break;
> > -
> >                 info->maps[i].virt = devm_ioremap_resource(&dev->dev, res);
> >                 if (IS_ERR(info->maps[i].virt)) {
> >                         err = PTR_ERR(info->maps[i].virt);
> > @@ -159,21 +172,18 @@ static int physmap_flash_probe(struct platform_device *dev)
> >                         dev_err(&dev->dev, "map_probe failed\n");
> >                         err = -ENXIO;
> >                         goto err_out;
> > -               } else {
> > -                       devices_found++;
> >                 }
> >                 info->mtds[i]->dev.parent = &dev->dev;
> >         }
> >
> > -       if (!devices_found) {
> > -               err = -ENODEV;
> > -       } else if (devices_found == 1) {
> > +       if (info->nmaps == 1) {
> >                 info->cmtd = info->mtds[0];
> >         } else {
> >                 /*
> >                  * We detected multiple devices. Concatenate them together.
> >                  */
> > -               info->cmtd = mtd_concat_create(info->mtds, devices_found, dev_name(&dev->dev));
> > +               info->cmtd = mtd_concat_create(info->mtds, info->nmaps,
> > +                                              dev_name(&dev->dev));
> >                 if (info->cmtd == NULL)
> >                         err = -ENXIO;
> >         }
> > @@ -199,7 +209,7 @@ static void physmap_flash_shutdown(struct platform_device *dev)
> >         struct physmap_flash_info *info = platform_get_drvdata(dev);
> >         int i;
> >
> > -       for (i = 0; i < MAX_RESOURCES && info->mtds[i]; i++)
> > +       for (i = 0; i < info->nmaps && info->mtds[i]; i++)
> >                 if (mtd_suspend(info->mtds[i]) == 0)
> >                         mtd_resume(info->mtds[i]);
> >  }
> > --
> > 2.14.1
> >  
> 
>

On Tue, 9 Oct 2018 09:16:41 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Mon, Oct 8, 2018 at 10:10 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > Stop manipulating the dev->resource array directly and use the
> > platform_get_resource() helper instead.
> >
> > While at it, fix the loop check so that we never overflow the info->maps
> > and info->mtds array even if the number of resources attached to the
> > platform dev is higher than MAX_RESOURCES.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/mtd/maps/physmap.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
> > index 4010afee4a33..e5b15ec2cb04 100644
> > --- a/drivers/mtd/maps/physmap.c
> > +++ b/drivers/mtd/maps/physmap.c
> > @@ -122,23 +122,28 @@ static int physmap_flash_probe(struct platform_device *dev)
> >
> >         platform_set_drvdata(dev, info);
> >
> > -       for (i = 0; i < dev->num_resources; i++) {
> > +       for (i = 0; i < MAX_RESOURCES; i++) {
> > +               struct resource *res;
> > +
> > +               res = platform_get_resource(dev, IORESOURCE_MEM, i);
> > +               if (res)
> > +                       break;  
> 
> Maybe if (!res)

I'll fix that one.

On Tue, 9 Oct 2018 09:11:06 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Tue, 9 Oct 2018 09:04:51 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> 
> > Hi Boris
> > 
> > Maybe we want to leave the pdata example.
> > 
> > /**
> >  * The platform resource layout expected looks something like:
> >  * struct mtd_partition partitions[] = { ... };
> >  * struct physmap_flash_data flash_data =
> > .....  
> 
> Sure, I'll add it back.

Actually I changed my mind on that one. The physmap driver did not
document that in the first place, so, if we want to add this example,
we should do it in a separate patch, and maybe place it next to the
physmap_flash_data struct definition. This being said, I don't see a
lot of drivers documenting this sort of things and I doubt new
platforms will use pdata to describe their device anyway.

On Tue, 9 Oct 2018 09:52:05 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> > > @@ -163,13 +164,16 @@ static int physmap_flash_probe(struct platform_device *dev)
> > >                 simple_map_init(&info->maps[i]);
> > >
> > >                 probe_type = rom_probe_types;
> > > -               if (physmap_data->probe_type == NULL) {
> > > -                       for (; info->mtds[i] == NULL && *probe_type != NULL; probe_type++)
> > > -                               info->mtds[i] = do_map_probe(*probe_type, &info->maps[i]);
> > > -               } else
> > > -                       info->mtds[i] = do_map_probe(physmap_data->probe_type, &info->maps[i]);
> > > +               if (!physmap_data->probe_type) {
> > > +                       for (; !info->mtds[i] && *probe_type; probe_type++)
> > > +                               info->mtds[i] = do_map_probe(*probe_type,
> > > +                                                            &info->maps[i]);
> > > +               } else {
> > > +                       info->mtds[i] = do_map_probe(physmap_data->probe_type,
> > > +                                                    &info->maps[i]);
> > > +               }    
> > 
> > Now that you are at it, maybe you want to change the order of the if branches:
> > 
> > if (true){
> > } else{
> > }
> > 
> > and move:
> > probe_type = rom_probe_types;
> > into the branch
> > 
> > and fix:
> > const char * const *probe_type;
> > 
> > 
> > If you do not want to change it, is also fine :P  
> 
> I don't mind changing that, but not in this patch :-).

Actually, I'll let you send a patch on top of my series if you still
want to address that ;-).