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

[PATCH v4 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.19.0

[PATCH v4 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>
---
 drivers/mtd/maps/gpio-addr-flash.c | 2 +-
 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.19.0

[PATCH v4 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 | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 6de16e81994c..84404dcc7824 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,9 @@ 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))
+		return PTR_ERR(state->map.virt);
 
 	state->map.phys       = NO_XIP;
 	state->map.map_priv_1 = (unsigned long)state;
@@ -245,12 +245,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 +257,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 +270,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.19.0

[PATCH v4 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 84404dcc7824..89cc8cce161b 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))
 		return PTR_ERR(state->map.virt);
-- 
2.19.0

[PATCH v4 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 | 34 +++++++++++++++---------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 89cc8cce161b..47b12a6fead4 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;
 }
 
 /**
@@ -202,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;
@@ -215,8 +218,7 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	if (!memory || !gpios || !gpios->end)
 		return -EINVAL;
 
-	arr_size = sizeof(int) * 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;
 
@@ -226,9 +228,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.19.0

[PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod

[Ricardo Ribalda Delgado]

Convert from legacy gpio API to gpiod.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/mtd/maps/gpio-addr-flash.c | 45 ++++++++++++++++++------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index 47b12a6fead4..c6fe1de33a82 100644
--- a/drivers/mtd/maps/gpio-addr-flash.c
+++ b/drivers/mtd/maps/gpio-addr-flash.c
@@ -14,6 +14,7 @@
  */
 
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -33,16 +34,14 @@
  * struct async_state - keep GPIO flash state
  *	@mtd:         MTD state for this mapping
  *	@map:         MTD map state for this flash
- *	@gpio_count:  number of GPIOs used to address
- *	@gpio_addrs:  array of GPIOs to twiddle
+ *	@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;
-	size_t gpio_count;
-	unsigned *gpio_addrs;
+	struct gpio_descs *gpios;
 	unsigned int gpio_values;
 	unsigned int win_order;
 };
@@ -66,11 +65,11 @@ static void gf_set_gpios(struct async_state *state, unsigned long ofs)
 	if (ofs == state->gpio_values)
 		return;
 
-	for (i = 0; i < state->gpio_count; i++) {
+	for (i = 0; i < state->gpios->ndescs; i++) {
 		if ((ofs & BIT(i)) == (state->gpio_values & BIT(i)))
 			continue;
 
-		gpio_set_value(state->gpio_addrs[i], !!(ofs & BIT(i)));
+		gpiod_set_value(state->gpios->desc[i], !!(ofs & BIT(i)));
 	}
 
 	state->gpio_values = ofs;
@@ -222,12 +221,17 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	if (!state)
 		return -ENOMEM;
 
-	/*
-	 * 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     = (void *)(unsigned long)gpios->start;
+	state->gpios = devm_kzalloc(&pdev->dev,
+			sizeof(*state->gpios) +
+			gpios->end * sizeof(state->gpios->desc[0]),
+			GFP_KERNEL);
+	if (!state->gpios)
+		return -ENOMEM;
+	state->gpios->ndescs = gpios->end;
+
+	if (!state->gpios->desc)
+		return -ENOMEM;
+
 	state->win_order      = get_bitmask_order(resource_size(memory)) - 1;
 
 	state->map.name       = DRIVER_NAME;
@@ -236,7 +240,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       = BIT(state->win_order + state->gpio_count);
+	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);
@@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device *pdev)
 
 	i = 0;
 	do {
-		if (devm_gpio_request(&pdev->dev, state->gpio_addrs[i],
-				      DRIVER_NAME)) {
+		unsigned int *gpio_id = (unsigned int *)gpios->start;
+
+		if (devm_gpio_request_one(&pdev->dev, gpio_id[i], GPIOD_OUT_LOW,
+					DRIVER_NAME)) {
 			dev_err(&pdev->dev, "failed to request gpio %d\n",
-				state->gpio_addrs[i]);
+				gpio_id[i]);
 			return -EBUSY;
 		}
-		gpio_direction_output(state->gpio_addrs[i], 0);
-	} while (++i < state->gpio_count);
+
+		state->gpios->desc[i] = gpio_to_desc(gpio_id[i]);
+		if (!state->gpios->desc[i])
+			return -EINVAL;
+	} while (++i < state->gpios->ndescs);
 
 	dev_notice(&pdev->dev, "probing %d-bit flash bus\n",
 		   state->map.bankwidth * 8);
-- 
2.19.0

Re: [PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod

[Boris Brezillon]

Hi Ricardo,

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

> @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device *pdev)
>  
>  	i = 0;
>  	do {
> -		if (devm_gpio_request(&pdev->dev, state->gpio_addrs[i],
> -				      DRIVER_NAME)) {
> +		unsigned int *gpio_id = (unsigned int *)gpios->start;
> +
> +		if (devm_gpio_request_one(&pdev->dev, gpio_id[i], GPIOD_OUT_LOW,
> +					DRIVER_NAME)) {
>  			dev_err(&pdev->dev, "failed to request gpio %d\n",
> -				state->gpio_addrs[i]);
> +				gpio_id[i]);
>  			return -EBUSY;
>  		}
> -		gpio_direction_output(state->gpio_addrs[i], 0);
> -	} while (++i < state->gpio_count);
> +
> +		state->gpios->desc[i] = gpio_to_desc(gpio_id[i]);
> +		if (!state->gpios->desc[i])
> +			return -EINVAL;
> +	} while (++i < state->gpios->ndescs);

Actually, I was thinking about using devm_gpiod_get_array() here and
defining a gpio lookup table in the board file registering the device.
This way, adding support for DT based parsing is transparent.

Regards,

Boris

Re: [PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod

[Boris Brezillon]

On Wed, 3 Oct 2018 17:11:14 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hi Ricardo,
> 
> On Mon,  1 Oct 2018 14:43:49 +0200
> Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> 
> > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device *pdev)
> >  
> >  	i = 0;
> >  	do {
> > -		if (devm_gpio_request(&pdev->dev, state->gpio_addrs[i],
> > -				      DRIVER_NAME)) {
> > +		unsigned int *gpio_id = (unsigned int *)gpios->start;
> > +
> > +		if (devm_gpio_request_one(&pdev->dev, gpio_id[i], GPIOD_OUT_LOW,
> > +					DRIVER_NAME)) {
> >  			dev_err(&pdev->dev, "failed to request gpio %d\n",
> > -				state->gpio_addrs[i]);
> > +				gpio_id[i]);
> >  			return -EBUSY;
> >  		}
> > -		gpio_direction_output(state->gpio_addrs[i], 0);
> > -	} while (++i < state->gpio_count);
> > +
> > +		state->gpios->desc[i] = gpio_to_desc(gpio_id[i]);
> > +		if (!state->gpios->desc[i])
> > +			return -EINVAL;
> > +	} while (++i < state->gpios->ndescs);  
> 
> Actually, I was thinking about using devm_gpiod_get_array() here and
> defining a gpio lookup table in the board file registering the device.
> This way, adding support for DT based parsing is transparent.

It's actually easier than I thought since no one is registering such a
device, so all you have to do is call devm_gpiod_get_array() and have a
struct gpio_descs pointer in struct async_state.

Re: [PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod

[Ricardo Ribalda Delgado]

Hi Boris
On Wed, Oct 3, 2018 at 5:17 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Wed, 3 Oct 2018 17:11:14 +0200
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
>
> > Hi Ricardo,
> >
> > On Mon,  1 Oct 2018 14:43:49 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> >
> > > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device *pdev)
> > >
> > >     i = 0;
> > >     do {
> > > -           if (devm_gpio_request(&pdev->dev, state->gpio_addrs[i],
> > > -                                 DRIVER_NAME)) {
> > > +           unsigned int *gpio_id = (unsigned int *)gpios->start;
> > > +
> > > +           if (devm_gpio_request_one(&pdev->dev, gpio_id[i], GPIOD_OUT_LOW,
> > > +                                   DRIVER_NAME)) {
> > >                     dev_err(&pdev->dev, "failed to request gpio %d\n",
> > > -                           state->gpio_addrs[i]);
> > > +                           gpio_id[i]);
> > >                     return -EBUSY;
> > >             }
> > > -           gpio_direction_output(state->gpio_addrs[i], 0);
> > > -   } while (++i < state->gpio_count);
> > > +
> > > +           state->gpios->desc[i] = gpio_to_desc(gpio_id[i]);
> > > +           if (!state->gpios->desc[i])
> > > +                   return -EINVAL;
> > > +   } while (++i < state->gpios->ndescs);
> >
> > Actually, I was thinking about using devm_gpiod_get_array() here and
> > defining a gpio lookup table in the board file registering the device.
> > This way, adding support for DT based parsing is transparent.
>
> It's actually easier than I thought since no one is registering such a
> device, so all you have to do is call devm_gpiod_get_array() and have a
> struct gpio_descs pointer in struct async_state.

That is what I do in patch 8/8 for DT based parsing.

I am am doing the gpio_to_desc and other to maintain compatibility
with old platform board files (in and out tree).

I think is important to maintain that compatibility, but you decide ;)

Cheers

-- 
Ricardo Ribalda

Re: [PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod

[Boris Brezillon]

Hi Ricardo,

On Wed, 3 Oct 2018 17:56:03 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:

> Hi Boris
> On Wed, Oct 3, 2018 at 5:17 PM Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > On Wed, 3 Oct 2018 17:11:14 +0200
> > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> >  
> > > Hi Ricardo,
> > >
> > > On Mon,  1 Oct 2018 14:43:49 +0200
> > > Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote:
> > >  
> > > > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device *pdev)
> > > >
> > > >     i = 0;
> > > >     do {
> > > > -           if (devm_gpio_request(&pdev->dev, state->gpio_addrs[i],
> > > > -                                 DRIVER_NAME)) {
> > > > +           unsigned int *gpio_id = (unsigned int *)gpios->start;
> > > > +
> > > > +           if (devm_gpio_request_one(&pdev->dev, gpio_id[i], GPIOD_OUT_LOW,
> > > > +                                   DRIVER_NAME)) {
> > > >                     dev_err(&pdev->dev, "failed to request gpio %d\n",
> > > > -                           state->gpio_addrs[i]);
> > > > +                           gpio_id[i]);
> > > >                     return -EBUSY;
> > > >             }
> > > > -           gpio_direction_output(state->gpio_addrs[i], 0);
> > > > -   } while (++i < state->gpio_count);
> > > > +
> > > > +           state->gpios->desc[i] = gpio_to_desc(gpio_id[i]);
> > > > +           if (!state->gpios->desc[i])
> > > > +                   return -EINVAL;
> > > > +   } while (++i < state->gpios->ndescs);  
> > >
> > > Actually, I was thinking about using devm_gpiod_get_array() here and
> > > defining a gpio lookup table in the board file registering the device.
> > > This way, adding support for DT based parsing is transparent.  
> >
> > It's actually easier than I thought since no one is registering such a
> > device, so all you have to do is call devm_gpiod_get_array() and have a
> > struct gpio_descs pointer in struct async_state.  
> 
> That is what I do in patch 8/8 for DT based parsing.

Well, yes, except you do it differently for the !DT case, while I'm
suggesting to use the same patch for both DT and !DT.

> 
> I am am doing the gpio_to_desc and other to maintain compatibility
> with old platform board files (in and out tree).

I do care about keeping in-tree users functional (which is why I
suggested to declare gpio lookup tables in the first place), but
keeping out-of-tree code functional is on a best-effort basis. If it
breaks because we have to rework an internal API then that's not our
fault. Actually, that's one of the reason we push people to upstream
their code => their maintenance burden is greatly reduced once the code
has reached mainline.

> 
> I think is important to maintain that compatibility, but you decide ;)

And my decision is, keep the code as simple as possible even if it
breaks out of tree users.

Regards,

Boris

[PATCH v4 7/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
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 .../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

[PATCH v4 8/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>
---
 drivers/mtd/maps/gpio-addr-flash.c | 184 ++++++++++++++++++++++-------
 1 file changed, 144 insertions(+), 40 deletions(-)

diff --git a/drivers/mtd/maps/gpio-addr-flash.c b/drivers/mtd/maps/gpio-addr-flash.c
index c6fe1de33a82..afa9dce14f68 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/
  *
@@ -171,8 +172,120 @@ 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->gpios = devm_gpiod_get_array(&pdev->dev, NULL,
+						    GPIOD_OUT_LOW);
+		if (IS_ERR(state->gpios)) {
+			dev_err(&pdev->dev, "Error getting gpio array %ld\n",
+				   PTR_ERR(state->gpios));
+			return PTR_ERR(state->gpios);
+		}
+		return 0;
+	}
+
+	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->gpios = devm_kzalloc(&pdev->dev,
+				    sizeof(*state->gpios) +
+				    gpios->end * sizeof(state->gpios->desc[0]),
+				    GFP_KERNEL);
+	if (!state->gpios)
+		return -ENOMEM;
+
+	for (i = 0; i < gpios->end ; i++) {
+		unsigned int *gpio_id = (unsigned int *)gpios->start;
+
+		if (devm_gpio_request_one(&pdev->dev, gpio_id[i], GPIOD_OUT_LOW,
+					DRIVER_NAME)) {
+			dev_err(&pdev->dev, "failed to request gpio %d\n",
+				gpio_id[i]);
+			return -EBUSY;
+		}
+
+		state->gpios->desc[i] = gpio_to_desc(gpio_id[i]);
+		if (!state->gpios->desc[i])
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 
 /**
  * gpio_flash_probe() - setup a mapping for a GPIO assisted flash
@@ -204,42 +317,42 @@ 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);
 
-	state->gpios = devm_kzalloc(&pdev->dev,
-			sizeof(*state->gpios) +
-			gpios->end * sizeof(state->gpios->desc[0]),
-			GFP_KERNEL);
-	if (!state->gpios)
-		return -ENOMEM;
-	state->gpios->ndescs = gpios->end;
-
-	if (!state->gpios->desc)
-		return -ENOMEM;
+	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->gpios->ndescs);
 	state->map.virt	      = devm_ioremap_resource(&pdev->dev, memory);
 	if (IS_ERR(state->map.virt))
@@ -248,33 +361,15 @@ static int gpio_flash_probe(struct platform_device *pdev)
 	state->map.phys       = NO_XIP;
 	state->map.map_priv_1 = (unsigned long)state;
 
-	platform_set_drvdata(pdev, state);
-
-	i = 0;
-	do {
-		unsigned int *gpio_id = (unsigned int *)gpios->start;
-
-		if (devm_gpio_request_one(&pdev->dev, gpio_id[i], GPIOD_OUT_LOW,
-					DRIVER_NAME)) {
-			dev_err(&pdev->dev, "failed to request gpio %d\n",
-				gpio_id[i]);
-			return -EBUSY;
-		}
-
-		state->gpios->desc[i] = gpio_to_desc(gpio_id[i]);
-		if (!state->gpios->desc[i])
-			return -EINVAL;
-	} while (++i < state->gpios->ndescs);
-
 	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;
 }
@@ -288,11 +383,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.19.0

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

[Boris Brezillon]

On Mon,  1 Oct 2018 14:43:51 +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->gpios = devm_gpiod_get_array(&pdev->dev, NULL,
> +						    GPIOD_OUT_LOW);

Did you consider using gpiod_set_array_value() to update the gpio vals
instead of having your own update loop (the for loop in the
gf_set_gpios() func)?

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

[Ricardo Ribalda Delgado]

Hi Boris
On Wed, Oct 3, 2018 at 7:01 PM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> On Mon,  1 Oct 2018 14:43:51 +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->gpios = devm_gpiod_get_array(&pdev->dev, NULL,
> > +                                                 GPIOD_OUT_LOW);
>
> Did you consider using gpiod_set_array_value() to update the gpio vals
> instead of having your own update loop (the for loop in the
> gf_set_gpios() func)?

Yes briefly but there are some cons:

- I need to allocate the value_array
- I want to relay on my cached old values, so I still have to loop


-- 
Ricardo Ribalda