Re: [PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod
Hi Ricardo,
On Wed, 3 Oct 2018 17:56:03 +0200
Ricardo Ribalda Delgado wrote:
> Hi Boris
> On Wed, Oct 3, 2018 at 5:17 PM Boris Brezillon
> wrote:
> >
> > On Wed, 3 Oct 2018 17:11:14 +0200
> > Boris Brezillon wrote:
> >
> > > Hi Ricardo,
> > >
> > > On Mon, 1 Oct 2018 14:43:49 +0200
> > > Ricardo Ribalda Delgado wrote:
> > >
> > > > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct
> > > > platform_device *pdev)
> > > >
> > > > i = 0;
> > > > do {
> > > > - if (devm_gpio_request(>dev, state->gpio_addrs[i],
> > > > - DRIVER_NAME)) {
> > > > + unsigned int *gpio_id = (unsigned int *)gpios->start;
> > > > +
> > > > + if (devm_gpio_request_one(>dev, gpio_id[i],
> > > > GPIOD_OUT_LOW,
> > > > + DRIVER_NAME)) {
> > > > dev_err(>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
Re: [PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod
Hi Ricardo,
On Wed, 3 Oct 2018 17:56:03 +0200
Ricardo Ribalda Delgado wrote:
> Hi Boris
> On Wed, Oct 3, 2018 at 5:17 PM Boris Brezillon
> wrote:
> >
> > On Wed, 3 Oct 2018 17:11:14 +0200
> > Boris Brezillon wrote:
> >
> > > Hi Ricardo,
> > >
> > > On Mon, 1 Oct 2018 14:43:49 +0200
> > > Ricardo Ribalda Delgado wrote:
> > >
> > > > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct
> > > > platform_device *pdev)
> > > >
> > > > i = 0;
> > > > do {
> > > > - if (devm_gpio_request(>dev, state->gpio_addrs[i],
> > > > - DRIVER_NAME)) {
> > > > + unsigned int *gpio_id = (unsigned int *)gpios->start;
> > > > +
> > > > + if (devm_gpio_request_one(>dev, gpio_id[i],
> > > > GPIOD_OUT_LOW,
> > > > + DRIVER_NAME)) {
> > > > dev_err(>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
Re: [PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod
Hi Boris
On Wed, Oct 3, 2018 at 5:17 PM Boris Brezillon
wrote:
>
> On Wed, 3 Oct 2018 17:11:14 +0200
> Boris Brezillon wrote:
>
> > Hi Ricardo,
> >
> > On Mon, 1 Oct 2018 14:43:49 +0200
> > Ricardo Ribalda Delgado wrote:
> >
> > > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device
> > > *pdev)
> > >
> > > i = 0;
> > > do {
> > > - if (devm_gpio_request(>dev, state->gpio_addrs[i],
> > > - DRIVER_NAME)) {
> > > + unsigned int *gpio_id = (unsigned int *)gpios->start;
> > > +
> > > + if (devm_gpio_request_one(>dev, gpio_id[i],
> > > GPIOD_OUT_LOW,
> > > + DRIVER_NAME)) {
> > > dev_err(>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
Hi Boris
On Wed, Oct 3, 2018 at 5:17 PM Boris Brezillon
wrote:
>
> On Wed, 3 Oct 2018 17:11:14 +0200
> Boris Brezillon wrote:
>
> > Hi Ricardo,
> >
> > On Mon, 1 Oct 2018 14:43:49 +0200
> > Ricardo Ribalda Delgado wrote:
> >
> > > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device
> > > *pdev)
> > >
> > > i = 0;
> > > do {
> > > - if (devm_gpio_request(>dev, state->gpio_addrs[i],
> > > - DRIVER_NAME)) {
> > > + unsigned int *gpio_id = (unsigned int *)gpios->start;
> > > +
> > > + if (devm_gpio_request_one(>dev, gpio_id[i],
> > > GPIOD_OUT_LOW,
> > > + DRIVER_NAME)) {
> > > dev_err(>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
On Wed, 3 Oct 2018 17:11:14 +0200
Boris Brezillon wrote:
> Hi Ricardo,
>
> On Mon, 1 Oct 2018 14:43:49 +0200
> Ricardo Ribalda Delgado wrote:
>
> > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device
> > *pdev)
> >
> > i = 0;
> > do {
> > - if (devm_gpio_request(>dev, state->gpio_addrs[i],
> > - DRIVER_NAME)) {
> > + unsigned int *gpio_id = (unsigned int *)gpios->start;
> > +
> > + if (devm_gpio_request_one(>dev, gpio_id[i], GPIOD_OUT_LOW,
> > + DRIVER_NAME)) {
> > dev_err(>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
On Wed, 3 Oct 2018 17:11:14 +0200
Boris Brezillon wrote:
> Hi Ricardo,
>
> On Mon, 1 Oct 2018 14:43:49 +0200
> Ricardo Ribalda Delgado wrote:
>
> > @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device
> > *pdev)
> >
> > i = 0;
> > do {
> > - if (devm_gpio_request(>dev, state->gpio_addrs[i],
> > - DRIVER_NAME)) {
> > + unsigned int *gpio_id = (unsigned int *)gpios->start;
> > +
> > + if (devm_gpio_request_one(>dev, gpio_id[i], GPIOD_OUT_LOW,
> > + DRIVER_NAME)) {
> > dev_err(>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
Hi Ricardo,
On Mon, 1 Oct 2018 14:43:49 +0200
Ricardo Ribalda Delgado wrote:
> @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device
> *pdev)
>
> i = 0;
> do {
> - if (devm_gpio_request(>dev, state->gpio_addrs[i],
> - DRIVER_NAME)) {
> + unsigned int *gpio_id = (unsigned int *)gpios->start;
> +
> + if (devm_gpio_request_one(>dev, gpio_id[i], GPIOD_OUT_LOW,
> + DRIVER_NAME)) {
> dev_err(>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
Hi Ricardo,
On Mon, 1 Oct 2018 14:43:49 +0200
Ricardo Ribalda Delgado wrote:
> @@ -248,14 +252,19 @@ static int gpio_flash_probe(struct platform_device
> *pdev)
>
> i = 0;
> do {
> - if (devm_gpio_request(>dev, state->gpio_addrs[i],
> - DRIVER_NAME)) {
> + unsigned int *gpio_id = (unsigned int *)gpios->start;
> +
> + if (devm_gpio_request_one(>dev, gpio_id[i], GPIOD_OUT_LOW,
> + DRIVER_NAME)) {
> dev_err(>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
[PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod
Convert from legacy gpio API to gpiod.
Signed-off-by: Ricardo Ribalda Delgado
Suggested-by: Boris Brezillon
---
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
+#include
#include
#include
#include
@@ -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(>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(>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(>dev, state->gpio_addrs[i],
- DRIVER_NAME)) {
+ unsigned int *gpio_id = (unsigned int *)gpios->start;
+
+ if (devm_gpio_request_one(>dev, gpio_id[i], GPIOD_OUT_LOW,
+ DRIVER_NAME)) {
dev_err(>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(>dev, "probing %d-bit flash bus\n",
state->map.bankwidth * 8);
--
2.19.0
[PATCH v4 6/8] mtd: maps: gpio-addr-flash: Convert to gpiod
Convert from legacy gpio API to gpiod.
Signed-off-by: Ricardo Ribalda Delgado
Suggested-by: Boris Brezillon
---
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
+#include
#include
#include
#include
@@ -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(>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(>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(>dev, state->gpio_addrs[i],
- DRIVER_NAME)) {
+ unsigned int *gpio_id = (unsigned int *)gpios->start;
+
+ if (devm_gpio_request_one(>dev, gpio_id[i], GPIOD_OUT_LOW,
+ DRIVER_NAME)) {
dev_err(>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(>dev, "probing %d-bit flash bus\n",
state->map.bankwidth * 8);
--
2.19.0
