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
By using devm functions we can make the code cleaner.
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Add documentation for gpio-addr-flash. This binding allow creating
flash devices that are paged using GPIOs.
Cc: [email protected]
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
.../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
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 <[email protected]>
---
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 <[email protected]>
* Copyright © 2005-2009 Analog Devices Inc.
+ * Copyright © 2018 Ricardo Ribalda <[email protected]>
*
* 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
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 <[email protected]>
---
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
Use preferred print methods dev_*
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
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
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 <[email protected]>
Fixes: ebd71e3a4861 ("mtd: maps: gpio-addr-flash: fix warnings and make more portable")
Cc: <[email protected]>
---
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
Ping?
No comments?
Thanks!
On Wed, Sep 5, 2018 at 4:36 PM Ricardo Ribalda Delgado
<[email protected]> 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
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: [email protected]
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> .../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
>
Hi Rob
On Tue, Sep 25, 2018 at 10:48 PM Rob Herring <[email protected]> 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: [email protected]
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > ---
> > .../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
Add documentation for gpio-addr-flash. This binding allow creating
flash devices that are paged using GPIOs.
Cc: [email protected]
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
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
On Wed, 5 Sep 2018 16:36:38 +0200
Ricardo Ribalda Delgado <[email protected]> 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);
> + }
On Wed, 5 Sep 2018 16:36:42 +0200
Ricardo Ribalda Delgado <[email protected]> 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;
> +}
On Wed, 5 Sep 2018 16:36:40 +0200
Ricardo Ribalda Delgado <[email protected]> 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 <[email protected]>
> ---
> 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;
On Wed, 5 Sep 2018 16:36:35 +0200
Ricardo Ribalda Delgado <[email protected]> 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.
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: [email protected]
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
>
> 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 <[email protected]>
Hi Boris
On Thu, 27 Sep 2018, 13:35 Boris Brezillon, <[email protected]> wrote:
>
> On Wed, 5 Sep 2018 16:36:38 +0200
> Ricardo Ribalda Delgado <[email protected]> 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);
> > + }
>
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
<[email protected]> 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 <[email protected]>
> ---
> 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
On Mon, 1 Oct 2018 11:58:49 +0200
Ricardo Ribalda Delgado <[email protected]> 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?
Hi
On Mon, Oct 1, 2018 at 1:39 PM Boris Brezillon
<[email protected]> wrote:
>
> On Mon, 1 Oct 2018 11:58:49 +0200
> Ricardo Ribalda Delgado <[email protected]> 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
Hi Boris
On Thu, Sep 27, 2018 at 1:42 PM Boris Brezillon
<[email protected]> wrote:
>
> On Wed, 5 Sep 2018 16:36:40 +0200
> Ricardo Ribalda Delgado <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Mon, 1 Oct 2018 14:10:03 +0200
Ricardo Ribalda Delgado <[email protected]> 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.