Re: [PATCH] i2c: mux: arb-gpio: Rewrite to use GPIO descriptors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux