Hi, On Thu, May 30, 2019 at 2:04 PM Linus Walleij <linus.walleij@xxxxxxxxxx> 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@xxxxxxxxxxxx> > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx> > --- > 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
