diff mbox series

i2c: mux: arb-gpio: Rewrite to use GPIO descriptors

Message ID 20190530210421.24941-1-linus.walleij@linaro.org
State Superseded
Delegated to: Peter Rosin
Headers show
Series i2c: mux: arb-gpio: Rewrite to use GPIO descriptors | expand

Commit Message

Linus Walleij May 30, 2019, 9:04 p.m. UTC
Instead of complex code picking GPIOs out of the device tree
and keeping track of polarity for each GPIO line, use descriptors
and pull polarity handling into the gpiolib.

We look for "our-claim" and "their-claim" since the gpiolib
code will try e.g. "our-claim-gpios" and "our-claim-gpio" in
turn to locate these GPIO lines from the device tree.

Cc: Doug Anderson <dianders@chromium.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 75 ++++++----------------
 1 file changed, 21 insertions(+), 54 deletions(-)

Comments

Douglas Anderson May 31, 2019, 11:59 p.m. UTC | #1
Hi,

On Thu, May 30, 2019 at 2:04 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Instead of complex code picking GPIOs out of the device tree
> and keeping track of polarity for each GPIO line, use descriptors
> and pull polarity handling into the gpiolib.

Yay!  Thank you for doing this--the driver looks much cleaner now.
That code was super gnarly before.


> We look for "our-claim" and "their-claim" since the gpiolib
> code will try e.g. "our-claim-gpios" and "our-claim-gpio" in
> turn to locate these GPIO lines from the device tree.
>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 75 ++++++----------------
>  1 file changed, 21 insertions(+), 54 deletions(-)

NOTE: any chance I can convince you to CC LKML on patches like this?
There's no patchwork for i2c so I can't easily grab this from
patchwork unless you CC LKML.  That also has the nice benefit that I
can refer to this conversation in perpetuity using the magic lkml
kernel redirector: https://lkml.kernel.org/r/<msg_id>

I personally am not setup for testing snow these days, but it's
possible that +Marek Szyprowski and +Krzystof Kozlowski might be
interested in testing?  If necessary I can dig through my boxes and
find a snow to test, though.  Also possible that linux-samsumg-soc
could have someone paying attention who cares about exynos-5250-snow.


> @@ -138,43 +130,18 @@ static int i2c_arbitrator_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, muxc);
>
> -       /* Request GPIOs */
> -       ret = of_get_named_gpio_flags(np, "our-claim-gpio", 0, &gpio_flags);
> -       if (!gpio_is_valid(ret)) {
> -               if (ret != -EPROBE_DEFER)
> -                       dev_err(dev, "Error getting our-claim-gpio\n");
> -               return ret;
> -       }
> -       arb->our_gpio = ret;
> -       arb->our_gpio_release = !!(gpio_flags & OF_GPIO_ACTIVE_LOW);
> -       out_init = (gpio_flags & OF_GPIO_ACTIVE_LOW) ?
> -               GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
> -       ret = devm_gpio_request_one(dev, arb->our_gpio, out_init,
> -                                   "our-claim-gpio");
> -       if (ret) {
> -               if (ret != -EPROBE_DEFER)
> -                       dev_err(dev, "Error requesting our-claim-gpio\n");
> -               return ret;
> -       }
> +       /* Request GPIOs, our GPIO as unclaimed to begin with */
> +       arb->our_gpio = devm_gpiod_get(dev, "our-claim", GPIOD_OUT_LOW);
> +       if (IS_ERR(arb->our_gpio))
> +               return PTR_ERR(arb->our_gpio);

I don't think devm_gpiod_get() handles error reporting for us, right?
So do we still need the "dev_err" if not EPROBE_DEFER to let the user
know that something bad happened?


>         /* At the moment we only support a single two master (us + 1 other) */
> -       if (gpio_is_valid(of_get_named_gpio(np, "their-claim-gpios", 1))) {
> +       dummy = devm_gpiod_get_index_optional(dev, "their-claim", 1, GPIOD_IN);
> +       if (dummy && !IS_ERR(dummy)) {

Trying to wrap my head around why you chose devm_gpiod_get_index_optional().

With devm_gpiod_get_index_optional()
- if gpiod_get_index returned a real GPIO: you'll enter "if"
- if gpiod_get_index returned -ENOENT: you won't enter "if"
- if gpiod_get_index returned diff err: you won't enter "if"

If we changed the above to:

  dummy = devm_gpiod_get_index(dev, "their-claim", 1, GPIOD_IN);
  if (!IS_ERR(dummy)) {

...then:
- if gpiod_get_index returned a real GPIO: you'll enter "if"
- if gpiod_get_index returned -ENOENT: you won't enter "if"
- if gpiod_get_index returned diff err: you won't enter "if"

So I _think_ you don't need the _optional() variant, right?

I'll also note that the purist in me would want to handle
-EPROBE_DEFER even though it's highly unlikely you'd end up in that
state.  AKA:

  dummy = devm_gpiod_get_index(dev, "their-claim", 1, GPIOD_IN);
  if (!IS_ERR(dummy)) {
    ...
  } else if (PTR_ERR(dummy) == -EPROBE_DEFER) {
    return -EPROBE_DEFER;
  }


Other than my nits this looks good to me.

-Doug
Wolfram Sang June 1, 2019, 8:02 a.m. UTC | #2
> NOTE: any chance I can convince you to CC LKML on patches like this?
> There's no patchwork for i2c so I can't easily grab this from
> patchwork unless you CC LKML.

See MAINTAINERS, there is a patchwork instance on ozlabs.
Douglas Anderson June 3, 2019, 3:42 p.m. UTC | #3
Hi,

On Sat, Jun 1, 2019 at 1:02 AM Wolfram Sang <wsa@the-dreams.de> wrote:
>
>
> > NOTE: any chance I can convince you to CC LKML on patches like this?
> > There's no patchwork for i2c so I can't easily grab this from
> > patchwork unless you CC LKML.
>
> See MAINTAINERS, there is a patchwork instance on ozlabs.

Ah, right!  Even so, CCing LKML can be a helpful thing to do for
patches.  If nothing else the archiving on lore.kernel.org is
valuable.

-Doug
Wolfram Sang June 3, 2019, 4:06 p.m. UTC | #4
> Ah, right!  Even so, CCing LKML can be a helpful thing to do for
> patches.  If nothing else the archiving on lore.kernel.org is
> valuable.

OK, can do, no problem. Still, wanted to point you to the ozlabs
instance to make your life easier :)
diff mbox series

Patch

diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
index 812b8cff265f..a2c876117ba9 100644
--- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
+++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c
@@ -15,12 +15,11 @@ 
  */
 
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/module.h>
-#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
@@ -28,22 +27,16 @@ 
 /**
  * struct i2c_arbitrator_data - Driver data for I2C arbitrator
  *
- * @our_gpio: GPIO we'll use to claim.
- * @our_gpio_release: 0 if active high; 1 if active low; AKA if the GPIO ==
- *   this then consider it released.
- * @their_gpio: GPIO that the other side will use to claim.
- * @their_gpio_release: 0 if active high; 1 if active low; AKA if the GPIO ==
- *   this then consider it released.
+ * @our_gpio: GPIO descriptor we'll use to claim.
+ * @their_gpio: GPIO descriptor that the other side will use to claim.
  * @slew_delay_us: microseconds to wait for a GPIO to go high.
  * @wait_retry_us: we'll attempt another claim after this many microseconds.
  * @wait_free_us: we'll give up after this many microseconds.
  */
 
 struct i2c_arbitrator_data {
-	int our_gpio;
-	int our_gpio_release;
-	int their_gpio;
-	int their_gpio_release;
+	struct gpio_desc *our_gpio;
+	struct gpio_desc *their_gpio;
 	unsigned int slew_delay_us;
 	unsigned int wait_retry_us;
 	unsigned int wait_free_us;
@@ -64,15 +57,15 @@  static int i2c_arbitrator_select(struct i2c_mux_core *muxc, u32 chan)
 	stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
 	do {
 		/* Indicate that we want to claim the bus */
-		gpio_set_value(arb->our_gpio, !arb->our_gpio_release);
+		gpiod_set_value(arb->our_gpio, 1);
 		udelay(arb->slew_delay_us);
 
 		/* Wait for the other master to release it */
 		stop_retry = jiffies + usecs_to_jiffies(arb->wait_retry_us) + 1;
 		while (time_before(jiffies, stop_retry)) {
-			int gpio_val = !!gpio_get_value(arb->their_gpio);
+			int gpio_val = gpiod_get_value(arb->their_gpio);
 
-			if (gpio_val == arb->their_gpio_release) {
+			if (!gpio_val) {
 				/* We got it, so return */
 				return 0;
 			}
@@ -81,13 +74,13 @@  static int i2c_arbitrator_select(struct i2c_mux_core *muxc, u32 chan)
 		}
 
 		/* It didn't release, so give up, wait, and try again */
-		gpio_set_value(arb->our_gpio, arb->our_gpio_release);
+		gpiod_set_value(arb->our_gpio, 0);
 
 		usleep_range(arb->wait_retry_us, arb->wait_retry_us * 2);
 	} while (time_before(jiffies, stop_time));
 
 	/* Give up, release our claim */
-	gpio_set_value(arb->our_gpio, arb->our_gpio_release);
+	gpiod_set_value(arb->our_gpio, 0);
 	udelay(arb->slew_delay_us);
 	dev_err(muxc->dev, "Could not claim bus, timeout\n");
 	return -EBUSY;
@@ -103,7 +96,7 @@  static int i2c_arbitrator_deselect(struct i2c_mux_core *muxc, u32 chan)
 	const struct i2c_arbitrator_data *arb = i2c_mux_priv(muxc);
 
 	/* Release the bus and wait for the other master to notice */
-	gpio_set_value(arb->our_gpio, arb->our_gpio_release);
+	gpiod_set_value(arb->our_gpio, 0);
 	udelay(arb->slew_delay_us);
 
 	return 0;
@@ -116,8 +109,7 @@  static int i2c_arbitrator_probe(struct platform_device *pdev)
 	struct device_node *parent_np;
 	struct i2c_mux_core *muxc;
 	struct i2c_arbitrator_data *arb;
-	enum of_gpio_flags gpio_flags;
-	unsigned long out_init;
+	struct gpio_desc *dummy;
 	int ret;
 
 	/* We only support probing from device tree; no platform_data */
@@ -138,43 +130,18 @@  static int i2c_arbitrator_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, muxc);
 
-	/* Request GPIOs */
-	ret = of_get_named_gpio_flags(np, "our-claim-gpio", 0, &gpio_flags);
-	if (!gpio_is_valid(ret)) {
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "Error getting our-claim-gpio\n");
-		return ret;
-	}
-	arb->our_gpio = ret;
-	arb->our_gpio_release = !!(gpio_flags & OF_GPIO_ACTIVE_LOW);
-	out_init = (gpio_flags & OF_GPIO_ACTIVE_LOW) ?
-		GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
-	ret = devm_gpio_request_one(dev, arb->our_gpio, out_init,
-				    "our-claim-gpio");
-	if (ret) {
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "Error requesting our-claim-gpio\n");
-		return ret;
-	}
+	/* Request GPIOs, our GPIO as unclaimed to begin with */
+	arb->our_gpio = devm_gpiod_get(dev, "our-claim", GPIOD_OUT_LOW);
+	if (IS_ERR(arb->our_gpio))
+		return PTR_ERR(arb->our_gpio);
 
-	ret = of_get_named_gpio_flags(np, "their-claim-gpios", 0, &gpio_flags);
-	if (!gpio_is_valid(ret)) {
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "Error getting their-claim-gpio\n");
-		return ret;
-	}
-	arb->their_gpio = ret;
-	arb->their_gpio_release = !!(gpio_flags & OF_GPIO_ACTIVE_LOW);
-	ret = devm_gpio_request_one(dev, arb->their_gpio, GPIOF_IN,
-				    "their-claim-gpio");
-	if (ret) {
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "Error requesting their-claim-gpio\n");
-		return ret;
-	}
+	arb->their_gpio = devm_gpiod_get(dev, "their-claim", GPIOD_IN);
+	if (IS_ERR(arb->their_gpio))
+		return PTR_ERR(arb->their_gpio);
 
 	/* At the moment we only support a single two master (us + 1 other) */
-	if (gpio_is_valid(of_get_named_gpio(np, "their-claim-gpios", 1))) {
+	dummy = devm_gpiod_get_index_optional(dev, "their-claim", 1, GPIOD_IN);
+	if (dummy && !IS_ERR(dummy)) {
 		dev_err(dev, "Only one other master is supported\n");
 		return -EINVAL;
 	}