[PATCH v5 4/4] mdio-mux-gpio: use new gpiod_get_array and gpiod_put_array functions

Use the new gpiod_get_array and gpiod_put_array functions for obtaining and
disposing of GPIO descriptors.

Cc: David Miller <davem@davemloft.net>
Signed-off-by: Rojhalat Ibrahim <redacted>
---
Change log:
  v5: no change
  v4: use shorter names for members of struct gpio_descs
  v3: no change
  v2: use the new interface

 drivers/net/phy/mdio-mux-gpio.c | 60 ++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/net/phy/mdio-mux-gpio.c b/drivers/net/phy/mdio-mux-gpio.c
index 320eb15..c49ad09 100644
--- a/drivers/net/phy/mdio-mux-gpio.c
+++ b/drivers/net/phy/mdio-mux-gpio.c

@@ -12,33 +12,30 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/mdio-mux.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 
 #define DRV_VERSION "1.1"
 #define DRV_DESCRIPTION "GPIO controlled MDIO bus multiplexer driver"
 
-#define MDIO_MUX_GPIO_MAX_BITS 8
-
 struct mdio_mux_gpio_state {
-	struct gpio_desc *gpio[MDIO_MUX_GPIO_MAX_BITS];
-	unsigned int num_gpios;
+	struct gpio_descs *gpios;
 	void *mux_handle;
 };
 
 static int mdio_mux_gpio_switch_fn(int current_child, int desired_child,
 				   void *data)
 {
-	int values[MDIO_MUX_GPIO_MAX_BITS];
-	unsigned int n;
 	struct mdio_mux_gpio_state *s = data;
+	int values[s->gpios->ndescs];
+	unsigned int n;
 
 	if (current_child == desired_child)
 		return 0;
 
-	for (n = 0; n < s->num_gpios; n++) {
+	for (n = 0; n < s->gpios->ndescs; n++)
 		values[n] = (desired_child >> n) & 1;
-	}
-	gpiod_set_array_cansleep(s->num_gpios, s->gpio, values);
+
+	gpiod_set_array_cansleep(s->gpios->ndescs, s->gpios->desc, values);
 
 	return 0;
 }

@@ -46,56 +43,33 @@ static int mdio_mux_gpio_switch_fn(int current_child, int desired_child,
 static int mdio_mux_gpio_probe(struct platform_device *pdev)
 {
 	struct mdio_mux_gpio_state *s;
-	int num_gpios;
-	unsigned int n;
 	int r;
 
-	if (!pdev->dev.of_node)
-		return -ENODEV;
-
-	num_gpios = of_gpio_count(pdev->dev.of_node);
-	if (num_gpios <= 0 || num_gpios > MDIO_MUX_GPIO_MAX_BITS)
-		return -ENODEV;
-
 	s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
 	if (!s)
 		return -ENOMEM;
 
-	s->num_gpios = num_gpios;
-
-	for (n = 0; n < num_gpios; ) {
-		struct gpio_desc *gpio = gpiod_get_index(&pdev->dev, NULL, n,
-							 GPIOD_OUT_LOW);
-		if (IS_ERR(gpio)) {
-			r = PTR_ERR(gpio);
-			goto err;
-		}
-		s->gpio[n] = gpio;
-		n++;
-	}
+	s->gpios = gpiod_get_array(&pdev->dev, NULL, GPIOD_OUT_LOW);
+	if (IS_ERR(s->gpios))
+		return PTR_ERR(s->gpios);
 
 	r = mdio_mux_init(&pdev->dev,
 			  mdio_mux_gpio_switch_fn, &s->mux_handle, s);
 
-	if (r == 0) {
-		pdev->dev.platform_data = s;
-		return 0;
-	}
-err:
-	while (n) {
-		n--;
-		gpiod_put(s->gpio[n]);
+	if (r != 0) {
+		gpiod_put_array(s->gpios);
+		return r;
 	}
-	return r;
+
+	pdev->dev.platform_data = s;
+	return 0;
 }
 
 static int mdio_mux_gpio_remove(struct platform_device *pdev)
 {
-	unsigned int n;
 	struct mdio_mux_gpio_state *s = dev_get_platdata(&pdev->dev);
 	mdio_mux_uninit(s->mux_handle);
-	for (n = 0; n < s->num_gpios; n++)
-		gpiod_put(s->gpio[n]);
+	gpiod_put_array(s->gpios);
 	return 0;
 }
 

-- 
2.0.5

On Thu, Feb 12, 2015 at 1:28 AM, Rojhalat Ibrahim [off-list ref] wrote:

17 more quoted lines

One suggestion for a possible further improvement: it would be great
if the gpiod_set/get_array() functions would work on a struct
gpio_descs so users don't have to pass both the number of GPIOs and
the array.

I don't know whether it would be desirable to keep alternative
functions that preserve the current form, for users who want to set
multiple GPIOs but cannot use gpiod_get_array(). struct gpiod_descs is
easy to build, so maybe we don't need them?

On Thursday 26 February 2015 18:54:53 Alexandre Courbot wrote:

On Thu, Feb 12, 2015 at 1:28 AM, Rojhalat Ibrahim [off-list ref] wrote:

quoted

Use the new gpiod_get_array and gpiod_put_array functions for obtaining and
disposing of GPIO descriptors.

Cc: David Miller <davem@davemloft.net>
Signed-off-by: Rojhalat Ibrahim <redacted>
---
Change log:
  v5: no change
  v4: use shorter names for members of struct gpio_descs
  v3: no change
  v2: use the new interface

 drivers/net/phy/mdio-mux-gpio.c | 60 ++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/net/phy/mdio-mux-gpio.c b/drivers/net/phy/mdio-mux-gpio.c
index 320eb15..c49ad09 100644
--- a/drivers/net/phy/mdio-mux-gpio.c
+++ b/drivers/net/phy/mdio-mux-gpio.c

@@ -12,33 +12,30 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/mdio-mux.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>

 #define DRV_VERSION "1.1"
 #define DRV_DESCRIPTION "GPIO controlled MDIO bus multiplexer driver"

-#define MDIO_MUX_GPIO_MAX_BITS 8
-
 struct mdio_mux_gpio_state {
-       struct gpio_desc *gpio[MDIO_MUX_GPIO_MAX_BITS];
-       unsigned int num_gpios;
+       struct gpio_descs *gpios;
        void *mux_handle;
 };

 static int mdio_mux_gpio_switch_fn(int current_child, int desired_child,

29 more quoted lines

I thought about that, but didn't want to change the interface in this
patch series.

Furthermore there is this use case (my use case):

I acquire a descriptor array for multiple data outputs and (among others) a
single descriptor for a clock output. Afterwards I want to set the data bits
and simultaneously clear the clock bit (using gpiod_set_array) before setting
only the clock output (using gpiod_set_value).

Therefore I need an array containing the data bits and the clock bit which
is easy to build.

I could also create a struct gpiod_descs but it would be more complicated
since I would have to allocate a new struct before populating it with the
descriptors and also free the allocated memory afterwards. It's not really
a big deal but more complicated than before.

But this might not be a very common use case.

If we can assume that for the common use case the group of descriptors that
can be acquired using gpiod_get_array() is the same group that should be
set using gpiod_set_array(), it might make sense to change the interface.

   Rojhalat

On Tue, Mar 3, 2015 at 6:45 PM, Rojhalat Ibrahim [off-list ref] wrote:

On Thursday 26 February 2015 18:54:53 Alexandre Courbot wrote:

quoted

On Thu, Feb 12, 2015 at 1:28 AM, Rojhalat Ibrahim [off-list ref] wrote:

quoted

Use the new gpiod_get_array and gpiod_put_array functions for obtaining and
disposing of GPIO descriptors.

Cc: David Miller <davem@davemloft.net>
Signed-off-by: Rojhalat Ibrahim <redacted>
---
Change log:
  v5: no change
  v4: use shorter names for members of struct gpio_descs
  v3: no change
  v2: use the new interface

 drivers/net/phy/mdio-mux-gpio.c | 60 ++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/net/phy/mdio-mux-gpio.c b/drivers/net/phy/mdio-mux-gpio.c
index 320eb15..c49ad09 100644
--- a/drivers/net/phy/mdio-mux-gpio.c
+++ b/drivers/net/phy/mdio-mux-gpio.c

@@ -12,33 +12,30 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/mdio-mux.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>

 #define DRV_VERSION "1.1"
 #define DRV_DESCRIPTION "GPIO controlled MDIO bus multiplexer driver"

-#define MDIO_MUX_GPIO_MAX_BITS 8
-
 struct mdio_mux_gpio_state {
-       struct gpio_desc *gpio[MDIO_MUX_GPIO_MAX_BITS];
-       unsigned int num_gpios;
+       struct gpio_descs *gpios;
        void *mux_handle;
 };

54 more quoted lines

Ah, thanks for sharing your use-case. I wish I had heard it earlier as
it seems we should make things more flexible than they currently are.

If I followed you correctly, you need to call gpiod_get_array() to
obtain the data lines, and gpiod_get() for the clock line. Then you
need to allocate a new array of gpio_desc * and copy all the
descriptors there before calling gpiod_set_array(). So simply put, the
struct gpio_descs you obtained is just useless to you.

It seems like we have been doing things wrong. Maybe gpiod_get_array()
should simply take a pointer to a gpio_desc * array that it would
fill, as you originally proposed?

So now, the question is: do we need struct gpiod_descs at all? It can
help reducing the number of arguments passed to functions, but also
makes the whole API more rigid. Use it with gpiod_get_array(), and you
end up with unneeded copies and memory allocations. Pass it to
gpiod_set_array() and you cannot do things like setting only part of
the GPIOs you requested.

Argh, and looking closer there is some possible confusion between
gpiod_set_array() and gpiod_get_array(). One might expect the latter
to return the *values* of the GPIOs, considering the name of the
former, while it actually is the array counterpart of gpiod_get(). To
be consistent with the single descriptor API, I suppose we should
rename gpiod_set_array() to gpiod_set_array_value(). But that's a
separate issue.

For now, since you are the main user of the array API, what is your
opinion about gpiod_descs? Do you think it is worth making the API
less flexible just to not have to carry an array lengh separately?
Should we just get rid of it?

On Wednesday 04 March 2015 22:22:24 Alexandre Courbot wrote:

On Tue, Mar 3, 2015 at 6:45 PM, Rojhalat Ibrahim [off-list ref] wrote:

quoted

On Thursday 26 February 2015 18:54:53 Alexandre Courbot wrote:

quoted

quoted

One suggestion for a possible further improvement: it would be great
if the gpiod_set/get_array() functions would work on a struct
gpio_descs so users don't have to pass both the number of GPIOs and
the array.

I don't know whether it would be desirable to keep alternative
functions that preserve the current form, for users who want to set
multiple GPIOs but cannot use gpiod_get_array(). struct gpiod_descs is
easy to build, so maybe we don't need them?

I thought about that, but didn't want to change the interface in this
patch series.

Furthermore there is this use case (my use case):

I acquire a descriptor array for multiple data outputs and (among others) a
single descriptor for a clock output. Afterwards I want to set the data bits
and simultaneously clear the clock bit (using gpiod_set_array) before setting
only the clock output (using gpiod_set_value).

Therefore I need an array containing the data bits and the clock bit which
is easy to build.

I could also create a struct gpiod_descs but it would be more complicated
since I would have to allocate a new struct before populating it with the
descriptors and also free the allocated memory afterwards. It's not really
a big deal but more complicated than before.

But this might not be a very common use case.

If we can assume that for the common use case the group of descriptors that
can be acquired using gpiod_get_array() is the same group that should be
set using gpiod_set_array(), it might make sense to change the interface.

Ah, thanks for sharing your use-case. I wish I had heard it earlier as
it seems we should make things more flexible than they currently are.

If I followed you correctly, you need to call gpiod_get_array() to
obtain the data lines, and gpiod_get() for the clock line. Then you

27 more quoted lines

I don't think it's that bad.

As I said before my use case might be very different from the common use case.

Furthermore the alternative API, that fills a pre-allocated array, wouldn't
make things easier. I would still have an array of data lines and a separate
descriptor for the clock. For setting them all together I would still have to
create an array containing the data and the clock descriptors. Creating this
array is not a big deal because it can be a static global array or an on stack
array in the context of a function. So no calls to kalloc / kfree are needed.

And the interface as proposed in this series is very convenient for obtaining
all the GPIOs belonging to a group with a single function call and without
having to know the number of GPIOs within the group beforehand.

So if we want to support different use cases, I think it's quite good as it is.
People who want to set a group of GPIOs as obtained by gpiod_get_array() can
do so with a single call to gpiod_set_array(), the only overhead being that
they have to specify the two elements of struct gpiod_descs explicitly.
Likewise people who want to set a group of GPIOs obtained with a combination
of calls to gpiod_get_array() and gpiod_get() can do so too. They just have
to create that group first.

On the other hand if gpiod_set_array() would require a struct gpiod_descs as
argument the creation of a group for the second use case would become more
complicated as you would have to allocate a struct instead of an array, etc.

So let's just keep it the way it is and get this series merged.

About the confusing function names: I would be happy to submit a patch
renaming gpiod_set_array() to gpiod_set_array_value(), once this has been
merged. I'm a little concerned about the length of some function names though.
Isn't gpiod_set_raw_array_value_cansleep() a bit long?

On Wed, Feb 11, 2015 at 5:28 PM, Rojhalat Ibrahim [off-list ref] wrote:

Use the new gpiod_get_array and gpiod_put_array functions for obtaining and
disposing of GPIO descriptors.

Cc: David Miller <davem@davemloft.net>
Signed-off-by: Rojhalat Ibrahim <redacted>

This looks good to me, but I need David's ACK before I can
merge it through the GPIO tree.

David: is this OK with you?

Yours,
Linus Walleij

On Wed, Mar 4, 2015 at 3:33 PM, Rojhalat Ibrahim [off-list ref] wrote:

And the interface as proposed in this series is very convenient for obtaining
all the GPIOs belonging to a group with a single function call and without
having to know the number of GPIOs within the group beforehand.

So if we want to support different use cases, I think it's quite good as it is.
People who want to set a group of GPIOs as obtained by gpiod_get_array() can
do so with a single call to gpiod_set_array(), the only overhead being that
they have to specify the two elements of struct gpiod_descs explicitly.
Likewise people who want to set a group of GPIOs obtained with a combination
of calls to gpiod_get_array() and gpiod_get() can do so too. They just have
to create that group first.

On the other hand if gpiod_set_array() would require a struct gpiod_descs as
argument the creation of a group for the second use case would become more
complicated as you would have to allocate a struct instead of an array, etc.

So let's just keep it the way it is and get this series merged.

I've merged it.

But can you make a separate patch to Documentation/gpio/consumer.txt
describing the array usecase(s) a bit in detail so people realize when it's
good to use these functions?

About the confusing function names: I would be happy to submit a patch
renaming gpiod_set_array() to gpiod_set_array_value(), once this has been
merged. I'm a little concerned about the length of some function names though.
Isn't gpiod_set_raw_array_value_cansleep() a bit long?

Just patch it and we'll discuss it... :)

Yours,
Linus Walleij

On Thursday 05 March 2015 10:04:20 Linus Walleij wrote:

On Wed, Mar 4, 2015 at 3:33 PM, Rojhalat Ibrahim [off-list ref] wrote:

quoted

And the interface as proposed in this series is very convenient for obtaining
all the GPIOs belonging to a group with a single function call and without
having to know the number of GPIOs within the group beforehand.

So if we want to support different use cases, I think it's quite good as it is.
People who want to set a group of GPIOs as obtained by gpiod_get_array() can
do so with a single call to gpiod_set_array(), the only overhead being that
they have to specify the two elements of struct gpiod_descs explicitly.
Likewise people who want to set a group of GPIOs obtained with a combination
of calls to gpiod_get_array() and gpiod_get() can do so too. They just have
to create that group first.

On the other hand if gpiod_set_array() would require a struct gpiod_descs as
argument the creation of a group for the second use case would become more
complicated as you would have to allocate a struct instead of an array, etc.

So let's just keep it the way it is and get this series merged.

I've merged it.

Thanks.

But can you make a separate patch to Documentation/gpio/consumer.txt
describing the array usecase(s) a bit in detail so people realize when it's
good to use these functions?

Sure.

quoted

About the confusing function names: I would be happy to submit a patch
renaming gpiod_set_array() to gpiod_set_array_value(), once this has been
merged. I'm a little concerned about the length of some function names though.
Isn't gpiod_set_raw_array_value_cansleep() a bit long?

Just patch it and we'll discuss it... :)

Ok. I'll submit a patch after you've merged patch 4/4 in this series in order
to avoid conflicts.

Ping?

On Thursday 05 March 2015 09:58:00 Linus Walleij wrote:

On Wed, Feb 11, 2015 at 5:28 PM, Rojhalat Ibrahim [off-list ref] wrote:

quoted

Use the new gpiod_get_array and gpiod_put_array functions for obtaining and
disposing of GPIO descriptors.

Cc: David Miller <davem@davemloft.net>
Signed-off-by: Rojhalat Ibrahim <redacted>

This looks good to me, but I need David's ACK before I can
merge it through the GPIO tree.

David: is this OK with you?

Yours,
Linus Walleij