[PATCH v2 0/8] gpio-addr-flash: Support for device-tree and cleanup

[PATCH v2 0/8] gpio-addr-flash: Support for device-tree and cleanup

[Ricardo Ribalda Delgado]

This patch series does the following:

1) Fix bug regarding ioremap size
2) Cleanup code to use new APIs
3) Simplify numerical operations
4) Add support for device-tree devices

Thanks!

Changelog v2:

From Boris Brezillon:
-Add Fixes and cc:stable

From kbuild:
- Fix warnings

- Rebase

Ricardo Ribalda Delgado (8):
  mtd: maps: gpio-addr-flash: Replace custom printk
  mtd: maps: gpio-addr-flash: Fix ioremapped size
  mtd: maps: gpio-addr-flash: Use devm_* functions
  mtd: maps: gpio-addr-flash: Use order insted of size
  mtd: maps: gpio-addr-flash: Replace array with an integer
  mtd: maps: gpio-addr-flash: Split allocation in two
  mtd: maps: gpio-addr-flash: Add support for device-tree devices
  dt-binding: mtd: Document gpio-addr-flash

 .../bindings/mtd/gpio-addr-flash.txt          |  46 +++
 drivers/mtd/maps/gpio-addr-flash.c            | 276 ++++++++++++------
 2 files changed, 236 insertions(+), 86 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt

-- 
2.18.0

[PATCH v2 1/8] mtd: maps: gpio-addr-flash: Replace custom printk

[Ricardo Ribalda Delgado]

Use preferred print methods dev_*

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/mtd/maps/gpio-addr-flash.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 9d9723693217..17be47f72973 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -25,11 +25,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 
-#define pr_devinit(fmt, args...) \
-	({ static const char __fmt[] = fmt; printk(__fmt, ## args); })
-
 #define DRIVER_NAME "gpio-addr-flash"
-#define PFX DRIVER_NAME ": "
 
 /**
  * struct async_state - keep GPIO flash state
@@ -250,7 +246,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	i = 0;
 	do {
 		if (gpio_request(state->gpio_addrs[i], DRIVER_NAME)) {
-			pr_devinit(KERN_ERR PFX "failed to request gpio %d\n",
+			dev_err(&pdev->dev, "failed to request gpio %d\n",
 				state->gpio_addrs[i]);
 			while (i--)
 				gpio_free(state->gpio_addrs[i]);
@@ -260,8 +256,8 @@ static int gpio_flash_probe(struct platform_device *pdev)
 		gpio_direction_output(state->gpio_addrs[i], 0);
 	} while (++i < state->gpio_count);
 
-	pr_devinit(KERN_NOTICE PFX "probing %d-bit flash bus\n",
-		state->map.bankwidth * 8);
+	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) {
 		for (i = 0; i < state->gpio_count; ++i)
-- 
2.18.0

[PATCH v2 2/8] mtd: maps: gpio-addr-flash: Fix ioremapped size

[Ricardo Ribalda Delgado]

We should only iomap the area of the chip that is memory mapped.
Otherwise we could be mapping devices beyond the memory space or that
belong to other devices.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Fixes: ebd71e3a4861 ("mtd: maps: gpio-addr-flash: fix warnings and make more portable")
Cc: <stable@vger.kernel.org>
---
Changelog v2:

From Boris Brezillon:
-Add Fixes and cc:stable

 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 17be47f72973..6de16e81994c 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -234,7 +234,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	state->map.copy_to    = gf_copy_to;
 	state->map.bankwidth  = pdata->width;
 	state->map.size       = state->win_size * (1 << state->gpio_count);
-	state->map.virt       = ioremap_nocache(memory->start, state->map.size);
+	state->map.virt       = ioremap_nocache(memory->start, state->win_size);
 	if (!state->map.virt)
 		return -ENOMEM;
 
-- 
2.18.0

[PATCH v2 3/8] mtd: maps: gpio-addr-flash: Use devm_* functions

[Ricardo Ribalda Delgado]

By using devm functions we can make the code cleaner.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/mtd/maps/gpio-addr-flash.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 6de16e81994c..54a0d7d2365a 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -213,7 +213,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	arr_size = sizeof(int) * gpios->end;
-	state = kzalloc(sizeof(*state) + arr_size, GFP_KERNEL);
+	state = devm_kzalloc(&pdev->dev, sizeof(*state) + arr_size, GFP_KERNEL);
 	if (!state)
 		return -ENOMEM;
 
@@ -234,9 +234,11 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	state->map.copy_to    = gf_copy_to;
 	state->map.bankwidth  = pdata->width;
 	state->map.size       = state->win_size * (1 << state->gpio_count);
-	state->map.virt       = ioremap_nocache(memory->start, state->win_size);
-	if (!state->map.virt)
-		return -ENOMEM;
+	state->map.virt	      = devm_ioremap_resource(&pdev->dev, memory);
+	if (IS_ERR(state->map.virt)) {
+		dev_err(&pdev->dev, "failed to map memory\n");
+		return PTR_ERR(state->map.virt);
+	}
 
 	state->map.phys       = NO_XIP;
 	state->map.map_priv_1 = (unsigned long)state;
@@ -245,12 +247,10 @@ static int gpio_flash_probe(struct platform_device *pdev)
 
 	i = 0;
 	do {
-		if (gpio_request(state->gpio_addrs[i], DRIVER_NAME)) {
+		if (devm_gpio_request(&pdev->dev, state->gpio_addrs[i],
+				      DRIVER_NAME)) {
 			dev_err(&pdev->dev, "failed to request gpio %d\n",
 				state->gpio_addrs[i]);
-			while (i--)
-				gpio_free(state->gpio_addrs[i]);
-			kfree(state);
 			return -EBUSY;
 		}
 		gpio_direction_output(state->gpio_addrs[i], 0);
@@ -259,12 +259,8 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	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) {
-		for (i = 0; i < state->gpio_count; ++i)
-			gpio_free(state->gpio_addrs[i]);
-		kfree(state);
+	if (!state->mtd)
 		return -ENXIO;
-	}
 	state->mtd->dev.parent = &pdev->dev;
 
 	mtd_device_parse_register(state->mtd, part_probe_types, NULL,
@@ -276,13 +272,9 @@ static int gpio_flash_probe(struct platform_device *pdev)
 static int gpio_flash_remove(struct platform_device *pdev)
 {
 	struct async_state *state = platform_get_drvdata(pdev);
-	size_t i = 0;
-	do {
-		gpio_free(state->gpio_addrs[i]);
-	} while (++i < state->gpio_count);
+
 	mtd_device_unregister(state->mtd);
 	map_destroy(state->mtd);
-	kfree(state);
 	return 0;
 }
 
-- 
2.18.0

Re: [PATCH v2 3/8] mtd: maps: gpio-addr-flash: Use devm_* functions

[Boris Brezillon]

On Wed,  5 Sep 2018 16:36:38 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> @@ -234,9 +234,11 @@ static int gpio_flash_probe(struct platform_device *pdev)
>  	state->map.copy_to    = gf_copy_to;
>  	state->map.bankwidth  = pdata->width;
>  	state->map.size       = state->win_size * (1 << state->gpio_count);
> -	state->map.virt       = ioremap_nocache(memory->start, state->win_size);
> -	if (!state->map.virt)
> -		return -ENOMEM;
> +	state->map.virt	      = devm_ioremap_resource(&pdev->dev, memory);
> +	if (IS_ERR(state->map.virt)) {
> +		dev_err(&pdev->dev, "failed to map memory\n");

You can drop this error message, devm_ioremap_resource() already takes
care of that (no need to send a new version, I'll fix it when applying).

> +		return PTR_ERR(state->map.virt);
> +	}

Re: [PATCH v2 3/8] mtd: maps: gpio-addr-flash: Use devm_* functions

[Ricardo Ribalda Delgado]

Hi Boris

On Thu, 27 Sep 2018, 13:35 Boris Brezillon, <boris.brezillon@bootlin.com> wrote:
>
> On Wed,  5 Sep 2018 16:36:38 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > @@ -234,9 +234,11 @@ static int gpio_flash_probe(struct platform_device *pdev)
> >       state->map.copy_to    = gf_copy_to;
> >       state->map.bankwidth  = pdata->width;
> >       state->map.size       = state->win_size * (1 << state->gpio_count);
> > -     state->map.virt       = ioremap_nocache(memory->start, state->win_size);
> > -     if (!state->map.virt)
> > -             return -ENOMEM;
> > +     state->map.virt       = devm_ioremap_resource(&pdev->dev, memory);
> > +     if (IS_ERR(state->map.virt)) {
> > +             dev_err(&pdev->dev, "failed to map memory\n");
>
> You can drop this error message, devm_ioremap_resource() already takes
> care of that (no need to send a new version, I'll fix it when applying).

Thanks for you review. I will do the port to gpiod next monday (I want
to try it on real hw)

Cheers!
>
> > +             return PTR_ERR(state->map.virt);
> > +     }
>

[PATCH v2 4/8] mtd: maps: gpio-addr-flash: Use order insted of size

[Ricardo Ribalda Delgado]

By using the order of the window instead of the size, we can replace a
lot of expensive division and modulus on the code with simple bit
operations.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/mtd/maps/gpio-addr-flash.c | 39 ++++++++++++++++--------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 54a0d7d2365a..22e100f07112 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -25,6 +25,8 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 
+#define win_mask(x) ((BIT(x)) - 1)
+
 #define DRIVER_NAME "gpio-addr-flash"
 
 /**
@@ -34,7 +36,7 @@
  *	@gpio_count:  number of GPIOs used to address
  *	@gpio_addrs:  array of GPIOs to twiddle
  *	@gpio_values: cached GPIO values
- *	@win_size:    dedicated memory size (if no GPIOs)
+ *	@win_order:   dedicated memory size (if no GPIOs)
  */
 struct async_state {
 	struct mtd_info *mtd;
@@ -42,7 +44,7 @@ struct async_state {
 	size_t gpio_count;
 	unsigned *gpio_addrs;
 	int *gpio_values;
-	unsigned long win_size;
+	unsigned int win_order;
 };
 #define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
 
@@ -60,7 +62,8 @@ static void gf_set_gpios(struct async_state *state, unsigned long ofs)
 {
 	size_t i = 0;
 	int value;
-	ofs /= state->win_size;
+
+	ofs >>= state->win_order;
 	do {
 		value = ofs & (1 << i);
 		if (state->gpio_values[i] != value) {
@@ -83,7 +86,7 @@ static map_word gf_read(struct map_info *map, unsigned long ofs)
 
 	gf_set_gpios(state, ofs);
 
-	word = readw(map->virt + (ofs % state->win_size));
+	word = readw(map->virt + (ofs & win_mask(state->win_order)));
 	test.x[0] = word;
 	return test;
 }
@@ -105,14 +108,14 @@ static void gf_copy_from(struct map_info *map, void *to, unsigned long from, ssi
 	int this_len;
 
 	while (len) {
-		if ((from % state->win_size) + len > state->win_size)
-			this_len = state->win_size - (from % state->win_size);
-		else
-			this_len = 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 % state->win_size),
-			 this_len);
+		memcpy_fromio(to,
+			      map->virt + (from & win_mask(state->win_order)),
+			      this_len);
 		len -= this_len;
 		from += this_len;
 		to += this_len;
@@ -132,7 +135,7 @@ static void gf_write(struct map_info *map, map_word d1, unsigned long ofs)
 	gf_set_gpios(state, ofs);
 
 	d = d1.x[0];
-	writew(d, map->virt + (ofs % state->win_size));
+	writew(d, map->virt + (ofs & win_mask(state->win_order)));
 }
 
 /**
@@ -152,13 +155,13 @@ static void gf_copy_to(struct map_info *map, unsigned long to,
 	int this_len;
 
 	while (len) {
-		if ((to % state->win_size) + len > state->win_size)
-			this_len = state->win_size - (to % state->win_size);
-		else
-			this_len = 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 % state->win_size), from, len);
+		memcpy_toio(map->virt + (to & win_mask(state->win_order)),
+			    from, len);
 
 		len -= this_len;
 		to += this_len;
@@ -224,7 +227,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	state->gpio_count     = gpios->end;
 	state->gpio_addrs     = (void *)(unsigned long)gpios->start;
 	state->gpio_values    = (void *)(state + 1);
-	state->win_size       = resource_size(memory);
+	state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
 	memset(state->gpio_values, 0xff, arr_size);
 
 	state->map.name       = DRIVER_NAME;
@@ -233,7 +236,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	state->map.write      = gf_write;
 	state->map.copy_to    = gf_copy_to;
 	state->map.bankwidth  = pdata->width;
-	state->map.size       = state->win_size * (1 << state->gpio_count);
+	state->map.size       = BIT(state->win_order + state->gpio_count);
 	state->map.virt	      = devm_ioremap_resource(&pdev->dev, memory);
 	if (IS_ERR(state->map.virt)) {
 		dev_err(&pdev->dev, "failed to map memory\n");
-- 
2.18.0

[PATCH v2 5/8] mtd: maps: gpio-addr-flash: Replace array with an integer

[Ricardo Ribalda Delgado]

By replacing the array with an integer we can avoid completely
the bit comparison loop if the value has not changed (by far
the most common case).

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/mtd/maps/gpio-addr-flash.c | 31 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 22e100f07112..8f5e3dce9be3 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -43,7 +43,7 @@ struct async_state {
 	struct map_info map;
 	size_t gpio_count;
 	unsigned *gpio_addrs;
-	int *gpio_values;
+	unsigned int gpio_values;
 	unsigned int win_order;
 };
 #define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
@@ -55,22 +55,25 @@ struct async_state {
  *
  * 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).  We rely on the GPIO framework to treat non-zero value as high so
- * that we don't have to normalize the bits.
+ * type).
  */
 static void gf_set_gpios(struct async_state *state, unsigned long ofs)
 {
-	size_t i = 0;
-	int value;
+	int i;
 
 	ofs >>= state->win_order;
-	do {
-		value = ofs & (1 << i);
-		if (state->gpio_values[i] != value) {
-			gpio_set_value(state->gpio_addrs[i], value);
-			state->gpio_values[i] = value;
-		}
-	} while (++i < state->gpio_count);
+
+	if (ofs == state->gpio_values)
+		return;
+
+	for (i = 0; i < state->gpio_count; i++) {
+		if ((ofs & BIT(i)) == (state->gpio_values & BIT(i)))
+			continue;
+
+		gpio_set_value(state->gpio_addrs[i], !!(ofs & BIT(i)));
+	}
+
+	state->gpio_values = ofs;
 }
 
 /**
@@ -215,7 +218,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	if (!memory || !gpios || !gpios->end)
 		return -EINVAL;
 
-	arr_size = sizeof(int) * gpios->end;
+	arr_size = sizeof(state->gpio_addrs[0]) * gpios->end;
 	state = devm_kzalloc(&pdev->dev, sizeof(*state) + arr_size, GFP_KERNEL);
 	if (!state)
 		return -ENOMEM;
@@ -226,9 +229,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	 */
 	state->gpio_count     = gpios->end;
 	state->gpio_addrs     = (void *)(unsigned long)gpios->start;
-	state->gpio_values    = (void *)(state + 1);
 	state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
-	memset(state->gpio_values, 0xff, arr_size);
 
 	state->map.name       = DRIVER_NAME;
 	state->map.read       = gf_read;
-- 
2.18.0

Re: [PATCH v2 5/8] mtd: maps: gpio-addr-flash: Replace array with an integer

[Boris Brezillon]

On Wed,  5 Sep 2018 16:36:40 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> By replacing the array with an integer we can avoid completely
> the bit comparison loop if the value has not changed (by far
> the most common case).
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/mtd/maps/gpio-addr-flash.c | 31 +++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
> index 22e100f07112..8f5e3dce9be3 100644
> --- a/drivers/mtd/maps/gpio-addr-flash.c
> +++ b/drivers/mtd/maps/gpio-addr-flash.c
> @@ -43,7 +43,7 @@ struct async_state {
>  	struct map_info map;
>  	size_t gpio_count;
>  	unsigned *gpio_addrs;
> -	int *gpio_values;
> +	unsigned int gpio_values;
>  	unsigned int win_order;
>  };
>  #define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
> @@ -55,22 +55,25 @@ struct async_state {
>   *
>   * 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).  We rely on the GPIO framework to treat non-zero value as high so
> - * that we don't have to normalize the bits.
> + * type).
>   */
>  static void gf_set_gpios(struct async_state *state, unsigned long ofs)
>  {
> -	size_t i = 0;
> -	int value;
> +	int i;
>  
>  	ofs >>= state->win_order;
> -	do {
> -		value = ofs & (1 << i);
> -		if (state->gpio_values[i] != value) {
> -			gpio_set_value(state->gpio_addrs[i], value);
> -			state->gpio_values[i] = value;
> -		}
> -	} while (++i < state->gpio_count);
> +
> +	if (ofs == state->gpio_values)
> +		return;
> +
> +	for (i = 0; i < state->gpio_count; i++) {
> +		if ((ofs & BIT(i)) == (state->gpio_values & BIT(i)))

Parens around the xx & BIT(i) operations are unneeded.

> +			continue;
> +
> +		gpio_set_value(state->gpio_addrs[i], !!(ofs & BIT(i)));
> +	}
> +
> +	state->gpio_values = ofs;
>  }
>  
>  /**
> @@ -215,7 +218,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
>  	if (!memory || !gpios || !gpios->end)
>  		return -EINVAL;
>  
> -	arr_size = sizeof(int) * gpios->end;
> +	arr_size = sizeof(state->gpio_addrs[0]) * gpios->end;
>  	state = devm_kzalloc(&pdev->dev, sizeof(*state) + arr_size, GFP_KERNEL);
>  	if (!state)
>  		return -ENOMEM;
> @@ -226,9 +229,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
>  	 */
>  	state->gpio_count     = gpios->end;
>  	state->gpio_addrs     = (void *)(unsigned long)gpios->start;
> -	state->gpio_values    = (void *)(state + 1);
>  	state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
> -	memset(state->gpio_values, 0xff, arr_size);
>  
>  	state->map.name       = DRIVER_NAME;
>  	state->map.read       = gf_read;

Re: [PATCH v2 5/8] mtd: maps: gpio-addr-flash: Replace array with an integer

[Ricardo Ribalda Delgado]

Hi Boris

On Thu, Sep 27, 2018 at 1:42 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Wed,  5 Sep 2018 16:36:40 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > By replacing the array with an integer we can avoid completely
> > the bit comparison loop if the value has not changed (by far
> > the most common case).
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > ---
> >  drivers/mtd/maps/gpio-addr-flash.c | 31 +++++++++++++++---------------
> >  1 file changed, 16 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
> > index 22e100f07112..8f5e3dce9be3 100644
> > --- a/drivers/mtd/maps/gpio-addr-flash.c
> > +++ b/drivers/mtd/maps/gpio-addr-flash.c
> > @@ -43,7 +43,7 @@ struct async_state {
> >       struct map_info map;
> >       size_t gpio_count;
> >       unsigned *gpio_addrs;
> > -     int *gpio_values;
> > +     unsigned int gpio_values;
> >       unsigned int win_order;
> >  };
> >  #define gf_map_info_to_state(mi) ((struct async_state *)(mi)->map_priv_1)
> > @@ -55,22 +55,25 @@ struct async_state {
> >   *
> >   * 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).  We rely on the GPIO framework to treat non-zero value as high so
> > - * that we don't have to normalize the bits.
> > + * type).
> >   */
> >  static void gf_set_gpios(struct async_state *state, unsigned long ofs)
> >  {
> > -     size_t i = 0;
> > -     int value;
> > +     int i;
> >
> >       ofs >>= state->win_order;
> > -     do {
> > -             value = ofs & (1 << i);
> > -             if (state->gpio_values[i] != value) {
> > -                     gpio_set_value(state->gpio_addrs[i], value);
> > -                     state->gpio_values[i] = value;
> > -             }
> > -     } while (++i < state->gpio_count);
> > +
> > +     if (ofs == state->gpio_values)
> > +             return;
> > +
> > +     for (i = 0; i < state->gpio_count; i++) {
> > +             if ((ofs & BIT(i)) == (state->gpio_values & BIT(i)))
>
> Parens around the xx & BIT(i) operations are unneeded.

If I remove it:


ricardo@neopili:~/curro/kernel-upstream$ make drivers/mtd/maps/gpio-addr-flash.o
  CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CC      drivers/mtd/maps/gpio-addr-flash.o
drivers/mtd/maps/gpio-addr-flash.c: In function ‘gf_set_gpios’:
drivers/mtd/maps/gpio-addr-flash.c:70:20: warning: suggest parentheses
around comparison in operand of ‘&’ [-Wparentheses]
   if (ofs & BIT(i) == (state->gpio_values & BIT(i)))


>
> > +                     continue;
> > +
> > +             gpio_set_value(state->gpio_addrs[i], !!(ofs & BIT(i)));
> > +     }
> > +
> > +     state->gpio_values = ofs;
> >  }
> >
> >  /**
> > @@ -215,7 +218,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
> >       if (!memory || !gpios || !gpios->end)
> >               return -EINVAL;
> >
> > -     arr_size = sizeof(int) * gpios->end;
> > +     arr_size = sizeof(state->gpio_addrs[0]) * gpios->end;
> >       state = devm_kzalloc(&pdev->dev, sizeof(*state) + arr_size, GFP_KERNEL);
> >       if (!state)
> >               return -ENOMEM;
> > @@ -226,9 +229,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
> >        */
> >       state->gpio_count     = gpios->end;
> >       state->gpio_addrs     = (void *)(unsigned long)gpios->start;
> > -     state->gpio_values    = (void *)(state + 1);
> >       state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
> > -     memset(state->gpio_values, 0xff, arr_size);
> >
> >       state->map.name       = DRIVER_NAME;
> >       state->map.read       = gf_read;
>


-- 
Ricardo Ribalda

Re: [PATCH v2 5/8] mtd: maps: gpio-addr-flash: Replace array with an integer

[Boris Brezillon]

On Mon, 1 Oct 2018 14:10:03 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> > > +             return;
> > > +
> > > +     for (i = 0; i < state->gpio_count; i++) {
> > > +             if ((ofs & BIT(i)) == (state->gpio_values & BIT(i)))  
> >
> > Parens around the xx & BIT(i) operations are unneeded.  
> 
> If I remove it:
> 
> 
> ricardo@neopili:~/curro/kernel-upstream$ make drivers/mtd/maps/gpio-addr-flash.o
>   CALL    scripts/checksyscalls.sh
>   DESCEND  objtool
>   CC      drivers/mtd/maps/gpio-addr-flash.o
> drivers/mtd/maps/gpio-addr-flash.c: In function ‘gf_set_gpios’:
> drivers/mtd/maps/gpio-addr-flash.c:70:20: warning: suggest parentheses
> around comparison in operand of ‘&’ [-Wparentheses]
>    if (ofs & BIT(i) == (state->gpio_values & BIT(i)))

Hm, okay. I remember having a similar discussion on one of my patch and
people suggesting to drop it because of the == precedence on x & a.
Anyway, let's just keep it like that.

[PATCH v2 6/8] mtd: maps: gpio-addr-flash: Split allocation in two

[Ricardo Ribalda Delgado]

Instead of making one allocation and then calculating the addresses of
those two pointers in that area make two allocations. This simplifies
the code.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/mtd/maps/gpio-addr-flash.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 8f5e3dce9be3..9455a8448064 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -205,7 +205,7 @@ static const char * const part_probe_types[] = {
  */
 static int gpio_flash_probe(struct platform_device *pdev)
 {
-	size_t i, arr_size;
+	size_t i;
 	struct physmap_flash_data *pdata;
 	struct resource *memory;
 	struct resource *gpios;
@@ -218,8 +218,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	if (!memory || !gpios || !gpios->end)
 		return -EINVAL;
 
-	arr_size = sizeof(state->gpio_addrs[0]) * gpios->end;
-	state = devm_kzalloc(&pdev->dev, sizeof(*state) + arr_size, GFP_KERNEL);
+	state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return -ENOMEM;
 
@@ -228,7 +227,12 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	 * away their pointer types here to the known types (gpios->xxx).
 	 */
 	state->gpio_count     = gpios->end;
-	state->gpio_addrs     = (void *)(unsigned long)gpios->start;
+	state->gpio_addrs     = devm_kzalloc(&pdev->dev,
+					     sizeof(state->gpio_addrs[0]) *
+								gpios->end,
+					     GFP_KERNEL);
+	if (!state->gpio_addrs)
+		return -ENOMEM;
 	state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
 
 	state->map.name       = DRIVER_NAME;
-- 
2.18.0

Re: [PATCH v2 6/8] mtd: maps: gpio-addr-flash: Split allocation in two

[Ricardo Ribalda Delgado]

Hi Boris

PLEASE do not apply this one!

There is a bug, value from gpio->addr is lost.
On Wed, Sep 5, 2018 at 4:37 PM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
>
> Instead of making one allocation and then calculating the addresses of
> those two pointers in that area make two allocations. This simplifies
> the code.
>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/mtd/maps/gpio-addr-flash.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
> index 8f5e3dce9be3..9455a8448064 100644
> --- a/drivers/mtd/maps/gpio-addr-flash.c
> +++ b/drivers/mtd/maps/gpio-addr-flash.c
> @@ -205,7 +205,7 @@ static const char * const part_probe_types[] = {
>   */
>  static int gpio_flash_probe(struct platform_device *pdev)
>  {
> -       size_t i, arr_size;
> +       size_t i;
>         struct physmap_flash_data *pdata;
>         struct resource *memory;
>         struct resource *gpios;
> @@ -218,8 +218,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
>         if (!memory || !gpios || !gpios->end)
>                 return -EINVAL;
>
> -       arr_size = sizeof(state->gpio_addrs[0]) * gpios->end;
> -       state = devm_kzalloc(&pdev->dev, sizeof(*state) + arr_size, GFP_KERNEL);
> +       state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
>         if (!state)
>                 return -ENOMEM;
>
> @@ -228,7 +227,12 @@ static int gpio_flash_probe(struct platform_device *pdev)
>          * away their pointer types here to the known types (gpios->xxx).
>          */
>         state->gpio_count     = gpios->end;
> -       state->gpio_addrs     = (void *)(unsigned long)gpios->start;
> +       state->gpio_addrs     = devm_kzalloc(&pdev->dev,
> +                                            sizeof(state->gpio_addrs[0]) *
> +                                                               gpios->end,
> +                                            GFP_KERNEL);
> +       if (!state->gpio_addrs)
> +               return -ENOMEM;
>         state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
>
>         state->map.name       = DRIVER_NAME;
> --
> 2.18.0
>


-- 
Ricardo Ribalda

Re: [PATCH v2 6/8] mtd: maps: gpio-addr-flash: Split allocation in two

[Boris Brezillon]

On Mon, 1 Oct 2018 11:58:49 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> 
> PLEASE do not apply this one!

Okay, should I wait for a v6 of the whole series, or do you still want
me to apply patches 1 to 5?

Re: [PATCH v2 6/8] mtd: maps: gpio-addr-flash: Split allocation in two

[Ricardo Ribalda Delgado]

Hi
On Mon, Oct 1, 2018 at 1:39 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Mon, 1 Oct 2018 11:58:49 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
>
> > Hi Boris
> >
> > PLEASE do not apply this one!
>
> Okay, should I wait for a v6 of the whole series, or do you still want
> me to apply patches 1 to 5?

I think is better to send v6 of the whole series. Thanks!



-- 
Ricardo Ribalda

[PATCH v2 7/8] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Ricardo Ribalda Delgado]

Allow creating gpio-addr-flash via device-tree and not just via platform
data.

The gpio probing has been moved to a different function allowing
deferred probing if they are not ready.

Option parsing has been also moved to separated functions.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
Changelog v2:

From kbuild:
- Fix warnings

 drivers/mtd/maps/gpio-addr-flash.c | 186 +++++++++++++++++++++++------
 1 file changed, 147 insertions(+), 39 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 9455a8448064..fea1e3ae3745 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -7,6 +7,7 @@
  *
  * Copyright © 2000 Nicolas Pitre <nico@cam.org>
  * Copyright © 2005-2009 Analog Devices Inc.
+ * Copyright © 2018 Ricardo Ribalda <ricardo.ribalda@gmail.com>
  *
  * Enter bugs at http://blackfin.uclinux.org/
  *
@@ -24,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/of_gpio.h>
 
 #define win_mask(x) ((BIT(x)) - 1)
 
@@ -172,8 +174,121 @@ static void gf_copy_to(struct map_info *map, unsigned long to,
 	}
 }
 
-static const char * const part_probe_types[] = {
-	"cmdlinepart", "RedBoot", NULL };
+static int gf_bankwidth(struct platform_device *pdev)
+{
+	struct device_node *dn;
+	int ret;
+	u32 bankwidth;
+
+	dn = pdev->dev.of_node;
+	if (!dn) {
+		struct physmap_flash_data *pdata;
+
+		pdata = dev_get_platdata(&pdev->dev);
+		return pdata->width;
+	}
+
+	ret = of_property_read_u32(dn, "bank-width", &bankwidth);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to get bank-width\n");
+		return -EINVAL;
+	}
+
+	return bankwidth;
+}
+
+static const char *gf_probe_type(struct platform_device *pdev)
+{
+	struct device_node *dn;
+	struct resource *memory;
+	const char *of_probe;
+
+	dn = pdev->dev.of_node;
+	if (!dn) {
+		memory = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		return memory->name;
+	}
+
+	of_probe = of_get_property(dn, "probe-type", NULL);
+	if (of_probe)
+		return of_probe;
+
+	return "cfi_probe";
+}
+
+static void gf_device_parse_register(struct platform_device *pdev,
+				     struct async_state *state)
+{
+	static const char * const part_probe_types[] = {
+		"cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
+	struct device_node *dn;
+
+	dn = pdev->dev.of_node;
+	if (!dn) {
+		struct physmap_flash_data *pdata;
+
+		pdata = dev_get_platdata(&pdev->dev);
+		mtd_device_parse_register(state->mtd, part_probe_types, NULL,
+					  pdata->parts, pdata->nr_parts);
+		return;
+	}
+
+	mtd_device_parse_register(state->mtd, part_probe_types, NULL, NULL, 0);
+}
+
+static int gpio_flash_probe_gpios(struct platform_device *pdev,
+				  struct async_state *state)
+{
+	struct physmap_flash_data *pdata;
+	struct device_node *dn;
+	struct resource *gpios = NULL;
+	int i;
+
+	dn = pdev->dev.of_node;
+	if (dn) {
+		state->gpio_count = of_gpio_count(dn);
+	} else {
+		pdata = dev_get_platdata(&pdev->dev);
+		if (!pdata)
+			return -EINVAL;
+		gpios = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+		if (IS_ERR(gpios) || !gpios->end)
+			return -EINVAL;
+		state->gpio_count = gpios->end;
+	}
+
+	state->gpio_addrs = devm_kzalloc(&pdev->dev,
+					 sizeof(state->gpio_addrs[0])
+						* state->gpio_count,
+					 GFP_KERNEL);
+	if (!state->gpio_addrs)
+		return -ENOMEM;
+
+	for (i = 0; i < state->gpio_count; i++) {
+		long gpio;
+		int ret;
+
+		if (dn)
+			gpio = of_get_gpio(dn, i);
+		else
+			gpio = ((unsigned long *)gpios->start)[i];
+
+		if (gpio < 0)
+			return gpio;
+
+		ret =  devm_gpio_request(&pdev->dev, gpio, DRIVER_NAME);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to request gpio %ld\n",
+				gpio);
+			return ret;
+		}
+		state->gpio_addrs[i] = gpio;
+		gpio_direction_output(state->gpio_addrs[i], 0);
+	}
+
+	return 0;
+}
+
 
 /**
  * gpio_flash_probe() - setup a mapping for a GPIO assisted flash
@@ -205,74 +320,58 @@ static const char * const part_probe_types[] = {
  */
 static int gpio_flash_probe(struct platform_device *pdev)
 {
-	size_t i;
-	struct physmap_flash_data *pdata;
 	struct resource *memory;
-	struct resource *gpios;
 	struct async_state *state;
+	int ret;
 
-	pdata = dev_get_platdata(&pdev->dev);
 	memory = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	gpios = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-
-	if (!memory || !gpios || !gpios->end)
+	if (!memory)
 		return -EINVAL;
 
+	if (!is_power_of_2(resource_size(memory))) {
+		dev_err(&pdev->dev, "Window size must be aligned\n");
+		return -EIO;
+	}
+
 	state = devm_kzalloc(&pdev->dev, sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return -ENOMEM;
+	platform_set_drvdata(pdev, state);
 
-	/*
-	 * We cast start/end to known types in the boards file, so cast
-	 * away their pointer types here to the known types (gpios->xxx).
-	 */
-	state->gpio_count     = gpios->end;
-	state->gpio_addrs     = devm_kzalloc(&pdev->dev,
-					     sizeof(state->gpio_addrs[0]) *
-								gpios->end,
-					     GFP_KERNEL);
-	if (!state->gpio_addrs)
-		return -ENOMEM;
-	state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
+	ret = gpio_flash_probe_gpios(pdev, state);
+	if (ret < 0)
+		return ret;
 
+	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;
+
+	ret = gf_bankwidth(pdev);
+	if (ret < 0)
+		return ret;
+	state->map.bankwidth = ret;
+
 	state->map.size       = BIT(state->win_order + state->gpio_count);
 	state->map.virt	      = devm_ioremap_resource(&pdev->dev, memory);
 	if (IS_ERR(state->map.virt)) {
 		dev_err(&pdev->dev, "failed to map memory\n");
 		return PTR_ERR(state->map.virt);
 	}
-
 	state->map.phys       = NO_XIP;
 	state->map.map_priv_1 = (unsigned long)state;
 
-	platform_set_drvdata(pdev, state);
-
-	i = 0;
-	do {
-		if (devm_gpio_request(&pdev->dev, state->gpio_addrs[i],
-				      DRIVER_NAME)) {
-			dev_err(&pdev->dev, "failed to request gpio %d\n",
-				state->gpio_addrs[i]);
-			return -EBUSY;
-		}
-		gpio_direction_output(state->gpio_addrs[i], 0);
-	} while (++i < state->gpio_count);
-
 	dev_notice(&pdev->dev, "probing %d-bit flash bus\n",
 		   state->map.bankwidth * 8);
-	state->mtd = do_map_probe(memory->name, &state->map);
+	state->mtd = do_map_probe(gf_probe_type(pdev), &state->map);
 	if (!state->mtd)
 		return -ENXIO;
 	state->mtd->dev.parent = &pdev->dev;
+	mtd_set_of_node(state->mtd, pdev->dev.of_node);
 
-	mtd_device_parse_register(state->mtd, part_probe_types, NULL,
-				  pdata->parts, pdata->nr_parts);
+	gf_device_parse_register(pdev, state);
 
 	return 0;
 }
@@ -286,11 +385,20 @@ static int gpio_flash_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id gpio_flash_match[] = {
+	{
+		.compatible	= "cfi-gpio-addr-flash",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, gpio_flash_match);
+
 static struct platform_driver gpio_flash_driver = {
 	.probe		= gpio_flash_probe,
 	.remove		= gpio_flash_remove,
 	.driver		= {
 		.name	= DRIVER_NAME,
+		.of_match_table = gpio_flash_match,
 	},
 };
 
-- 
2.18.0

Re: [PATCH v2 7/8] mtd: maps: gpio-addr-flash: Add support for device-tree devices

[Boris Brezillon]

On Wed,  5 Sep 2018 16:36:42 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> +static int gpio_flash_probe_gpios(struct platform_device *pdev,
> +				  struct async_state *state)
> +{
> +	struct physmap_flash_data *pdata;
> +	struct device_node *dn;
> +	struct resource *gpios = NULL;
> +	int i;
> +
> +	dn = pdev->dev.of_node;
> +	if (dn) {
> +		state->gpio_count = of_gpio_count(dn);
> +	} else {
> +		pdata = dev_get_platdata(&pdev->dev);
> +		if (!pdata)
> +			return -EINVAL;
> +		gpios = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +		if (IS_ERR(gpios) || !gpios->end)
> +			return -EINVAL;
> +		state->gpio_count = gpios->end;
> +	}
> +
> +	state->gpio_addrs = devm_kzalloc(&pdev->dev,
> +					 sizeof(state->gpio_addrs[0])
> +						* state->gpio_count,
> +					 GFP_KERNEL);
> +	if (!state->gpio_addrs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < state->gpio_count; i++) {
> +		long gpio;
> +		int ret;
> +
> +		if (dn)
> +			gpio = of_get_gpio(dn, i);
> +		else
> +			gpio = ((unsigned long *)gpios->start)[i];
> +

Hm, we should probably switch to GPIO descs (instead of GPIO numbers)
before adding DT support. This should make the code much more simpler
and remove one more user of the old/deprecated GPIO API.

> +		if (gpio < 0)
> +			return gpio;
> +
> +		ret =  devm_gpio_request(&pdev->dev, gpio, DRIVER_NAME);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to request gpio %ld\n",
> +				gpio);
> +			return ret;
> +		}
> +		state->gpio_addrs[i] = gpio;
> +		gpio_direction_output(state->gpio_addrs[i], 0);
> +	}
> +
> +	return 0;
> +}

[PATCH v2 8/8] dt-binding: mtd: Document gpio-addr-flash

[Ricardo Ribalda Delgado]

Add documentation for gpio-addr-flash. This binding allow creating
flash devices that are paged using GPIOs.

Cc: devicetree@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 .../bindings/mtd/gpio-addr-flash.txt          | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt

diff --git a/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt b/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
new file mode 100644
index 000000000000..4279e8cad09b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
@@ -0,0 +1,46 @@
+Memory Mapped flash with some address lines addressed using GPIOs
+
+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.
+
+ - compatible : "cfi-gpio-addr-flash"
+ - reg : Address range of the mtd chip that is memory mapped, this is,
+   on the previous example 2MiB.
+ - bank-width : Width (in bytes) of the bank.  Equal to the
+   device width times the number of interleaved chips.
+ - probe-type : (optional) "cfi_probe", "jedec_probe". How the mtd chip
+   is going to be probed. If omitted, assumed to be equal to "cfi_probe".
+ - #address-cells, #size-cells : Must be present if the device has
+   sub-nodes representing partitions (see below).  In this case
+   both #address-cells and #size-cells must be equal to 1.
+
+The device tree may optionally contain sub-nodes describing partitions of the
+address space. See partition.txt for more detail.
+
+Example:
+
+	cfi_flash_0: cfi_flash {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "cfi-gpio-addr-flash";
+		bank-width = <2>;
+		reg = < 0x00300000 0x00200000 >;
+		gpios = <&gpio_0 3 0>, <&gpio_0 4 0>;
+		partition@0 {
+			reg = < 0x0 0x200000 >;
+			label = "Golden Bitstream";
+		};
+		partition@200000 {
+			reg = < 0x200000 0x200000 >;
+			label = "User Bitstream";
+		};
+		partition@400000 {
+			reg = < 0x400000 0x200000 >;
+			label = "V4L Controls";
+		};
+		partition@600000 {
+			reg = < 0x600000 0x200000 >;
+			label = "Production Data";
+		};
+	} ;
-- 
2.18.0

Re: [PATCH v2 8/8] dt-binding: mtd: Document gpio-addr-flash

[Rob Herring]

On Wed, Sep 05, 2018 at 04:36:43PM +0200, Ricardo Ribalda Delgado wrote:
> Add documentation for gpio-addr-flash. This binding allow creating
> flash devices that are paged using GPIOs.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  .../bindings/mtd/gpio-addr-flash.txt          | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt b/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
> new file mode 100644
> index 000000000000..4279e8cad09b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
> @@ -0,0 +1,46 @@
> +Memory Mapped flash with some address lines addressed using GPIOs
> +
> +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.
> +
> + - compatible : "cfi-gpio-addr-flash"
> + - reg : Address range of the mtd chip that is memory mapped, this is,
> +   on the previous example 2MiB.
> + - bank-width : Width (in bytes) of the bank.  Equal to the
> +   device width times the number of interleaved chips.
> + - probe-type : (optional) "cfi_probe", "jedec_probe". How the mtd chip
> +   is going to be probed. If omitted, assumed to be equal to "cfi_probe".

Why not just a bool prop for jedec mode? As-is, the '_probe' part is 
redundant.

> + - #address-cells, #size-cells : Must be present if the device has
> +   sub-nodes representing partitions (see below).  In this case
> +   both #address-cells and #size-cells must be equal to 1.

Perhaps most importantly, where's 'gpios' property?

> +
> +The device tree may optionally contain sub-nodes describing partitions of the
> +address space. See partition.txt for more detail.
> +
> +Example:
> +
> +	cfi_flash_0: cfi_flash {

flash@300000

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "cfi-gpio-addr-flash";
> +		bank-width = <2>;
> +		reg = < 0x00300000 0x00200000 >;
> +		gpios = <&gpio_0 3 0>, <&gpio_0 4 0>;
> +		partition@0 {

All these under a partitions node is now recommended.

> +			reg = < 0x0 0x200000 >;
> +			label = "Golden Bitstream";
> +		};
> +		partition@200000 {
> +			reg = < 0x200000 0x200000 >;
> +			label = "User Bitstream";
> +		};
> +		partition@400000 {
> +			reg = < 0x400000 0x200000 >;
> +			label = "V4L Controls";
> +		};
> +		partition@600000 {
> +			reg = < 0x600000 0x200000 >;
> +			label = "Production Data";
> +		};
> +	} ;
> -- 
> 2.18.0
> 

Re: [PATCH v2 8/8] dt-binding: mtd: Document gpio-addr-flash

[Ricardo Ribalda Delgado]

Hi Rob
On Tue, Sep 25, 2018 at 10:48 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Sep 05, 2018 at 04:36:43PM +0200, Ricardo Ribalda Delgado wrote:
> > Add documentation for gpio-addr-flash. This binding allow creating
> > flash devices that are paged using GPIOs.
> >
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > ---
> >  .../bindings/mtd/gpio-addr-flash.txt          | 46 +++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt b/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
> > new file mode 100644
> > index 000000000000..4279e8cad09b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
> > @@ -0,0 +1,46 @@
> > +Memory Mapped flash with some address lines addressed using GPIOs
> > +
> > +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.
> > +
> > + - compatible : "cfi-gpio-addr-flash"
> > + - reg : Address range of the mtd chip that is memory mapped, this is,
> > +   on the previous example 2MiB.
> > + - bank-width : Width (in bytes) of the bank.  Equal to the
> > +   device width times the number of interleaved chips.
> > + - probe-type : (optional) "cfi_probe", "jedec_probe". How the mtd chip
> > +   is going to be probed. If omitted, assumed to be equal to "cfi_probe".
>
> Why not just a bool prop for jedec mode? As-is, the '_probe' part is
> redundant.

I wanted to mimic gpio-addr-flash. Which btw does not document this
property. Shall I send a patch adding it?

>
> > + - #address-cells, #size-cells : Must be present if the device has
> > +   sub-nodes representing partitions (see below).  In this case
> > +   both #address-cells and #size-cells must be equal to 1.
>
> Perhaps most importantly, where's 'gpios' property?
>
> > +
> > +The device tree may optionally contain sub-nodes describing partitions of the
> > +address space. See partition.txt for more detail.
> > +
> > +Example:
> > +
> > +     cfi_flash_0: cfi_flash {
>
> flash@300000
>
> > +             #address-cells = <1>;
> > +             #size-cells = <1>;
> > +             compatible = "cfi-gpio-addr-flash";
> > +             bank-width = <2>;
> > +             reg = < 0x00300000 0x00200000 >;
> > +             gpios = <&gpio_0 3 0>, <&gpio_0 4 0>;
> > +             partition@0 {
>
> All these under a partitions node is now recommended.
>
> > +                     reg = < 0x0 0x200000 >;
> > +                     label = "Golden Bitstream";
> > +             };
> > +             partition@200000 {
> > +                     reg = < 0x200000 0x200000 >;
> > +                     label = "User Bitstream";
> > +             };
> > +             partition@400000 {
> > +                     reg = < 0x400000 0x200000 >;
> > +                     label = "V4L Controls";
> > +             };
> > +             partition@600000 {
> > +                     reg = < 0x600000 0x200000 >;
> > +                     label = "Production Data";
> > +             };
> > +     } ;
> > --
> > 2.18.0
> >



-- 
Ricardo Ribalda

[PATCH v3 8/8] dt-binding: mtd: Document gpio-addr-flash

[Ricardo Ribalda Delgado]

Add documentation for gpio-addr-flash. This binding allow creating
flash devices that are paged using GPIOs.

Cc: devicetree@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---

Changelog v3: 

- Changes suggested by Rob Herring (Thanks!)
  - Add gpio description
  - Improve partition

 .../bindings/mtd/gpio-addr-flash.txt          | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt

diff --git a/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt b/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
new file mode 100644
index 000000000000..5006a26e1753
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
@@ -0,0 +1,54 @@
+Memory Mapped flash with some address lines addressed using GPIOs
+
+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.
+
+ - compatible : "cfi-gpio-addr-flash"
+ - reg : Address range of the mtd chip that is memory mapped, this is,
+   on the previous example 2MiB.
+ - bank-width : Width (in bytes) of the bank.  Equal to the
+   device width times the number of interleaved chips.
+ - gpios: List of GPIO specifiers that will be used to address the MSBs address
+   lines. The order goes from LSB to MSB.
+ - probe-type : (optional) "cfi_probe", "jedec_probe". How the mtd chip
+   is going to be probed. If omitted, assumed to be equal to "cfi_probe".
+ - #address-cells, #size-cells : Must be present if the device has
+   sub-nodes representing partitions (see below).  In this case
+   both #address-cells and #size-cells must be equal to 1.
+
+The device tree may optionally contain sub-nodes describing partitions of the
+address space. Check partition.txt for more details.
+
+Example:
+
+	flash@300000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "cfi-gpio-addr-flash";
+		bank-width = <2>;
+		reg = < 0x00300000 0x00200000 >;
+		gpios = <&gpio_0 3 0>, <&gpio_0 4 0>;
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			partition@0 {
+				reg = < 0x0 0x200000 >;
+				label = "Golden Bitstream";
+			};
+			partition@200000 {
+				reg = < 0x200000 0x200000 >;
+				label = "User Bitstream";
+			};
+			partition@400000 {
+				reg = < 0x400000 0x200000 >;
+				label = "V4L Controls";
+			};
+			partition@600000 {
+				reg = < 0x600000 0x200000 >;
+				label = "Production Data";
+			};
+		}
+	} ;
-- 
2.19.0

Re: [PATCH v3 8/8] dt-binding: mtd: Document gpio-addr-flash

[Rob Herring]

On Wed, 26 Sep 2018 08:39:34 +0200, Ricardo Ribalda Delgado wrote:
> Add documentation for gpio-addr-flash. This binding allow creating
> flash devices that are paged using GPIOs.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> 
> Changelog v3: 
> 
> - Changes suggested by Rob Herring (Thanks!)
>   - Add gpio description
>   - Improve partition
> 
>  .../bindings/mtd/gpio-addr-flash.txt          | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
> 

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

Re: [PATCH v2 0/8] gpio-addr-flash: Support for device-tree and cleanup

[Ricardo Ribalda Delgado]

Ping?

No comments?

Thanks!
On Wed, Sep 5, 2018 at 4:36 PM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
>
> This patch series does the following:
>
> 1) Fix bug regarding ioremap size
> 2) Cleanup code to use new APIs
> 3) Simplify numerical operations
> 4) Add support for device-tree devices
>
> Thanks!
>
> Changelog v2:
>
> From Boris Brezillon:
> -Add Fixes and cc:stable
>
> From kbuild:
> - Fix warnings
>
> - Rebase
>
> Ricardo Ribalda Delgado (8):
>   mtd: maps: gpio-addr-flash: Replace custom printk
>   mtd: maps: gpio-addr-flash: Fix ioremapped size
>   mtd: maps: gpio-addr-flash: Use devm_* functions
>   mtd: maps: gpio-addr-flash: Use order insted of size
>   mtd: maps: gpio-addr-flash: Replace array with an integer
>   mtd: maps: gpio-addr-flash: Split allocation in two
>   mtd: maps: gpio-addr-flash: Add support for device-tree devices
>   dt-binding: mtd: Document gpio-addr-flash
>
>  .../bindings/mtd/gpio-addr-flash.txt          |  46 +++
>  drivers/mtd/maps/gpio-addr-flash.c            | 276 ++++++++++++------
>  2 files changed, 236 insertions(+), 86 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/gpio-addr-flash.txt
>
> --
> 2.18.0
>


-- 
Ricardo Ribalda

Re: [PATCH v2 0/8] gpio-addr-flash: Support for device-tree and cleanup

[Boris Brezillon]

On Wed,  5 Sep 2018 16:36:35 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> This patch series does the following:
> 
> 1) Fix bug regarding ioremap size
> 2) Cleanup code to use new APIs
> 3) Simplify numerical operations
> 4) Add support for device-tree devices
> 
> Thanks!
> 
> Changelog v2:
> 
> From Boris Brezillon:
> -Add Fixes and cc:stable
> 
> From kbuild:
> - Fix warnings
> 
> - Rebase
> 
> Ricardo Ribalda Delgado (8):
>   mtd: maps: gpio-addr-flash: Replace custom printk
>   mtd: maps: gpio-addr-flash: Fix ioremapped size
>   mtd: maps: gpio-addr-flash: Use devm_* functions
>   mtd: maps: gpio-addr-flash: Use order insted of size
>   mtd: maps: gpio-addr-flash: Replace array with an integer
>   mtd: maps: gpio-addr-flash: Split allocation in two

I'll apply patches 1 to 6 soon.

>   mtd: maps: gpio-addr-flash: Add support for device-tree devices
>   dt-binding: mtd: Document gpio-addr-flash

Still think we should move to gpio_desc before adding support for
DT.