| Message ID | 20180711163319.28004-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: use better errno if get_direction is not available | expand |
Hi Wolfram, On Wed, Jul 11, 2018 at 6:33 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > @Geert: any reason gpio-rcar is missing it? I would need it for the > i2c-gpio-fault-injector. Because so far no one had a need for it? /me thought gpiolib would return a cached value from a previous set_direction_{in,out}put(), but apparently that must have been a dream... Do you want me to implement it? Gr{oetje,eeting}s, Geert
> Do you want me to implement it?
Yes, please.
Thanks a lot!
On Wed, Jul 11, 2018 at 06:33:19PM +0200, Wolfram Sang wrote: > EINVAL is very generic, use ENOTSUPP in case the gpiochip does not > provide this function. While removing the assignment from the 'status' > variable, use better indentation in the declaration block. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> So, Geert implemented get_direction for gpio-rcar, but this doesn't help my case sadly. Because I2C is open drain, get_direction returns 'input' :( As mentioned in a previous discussion, returning '0' and '1' from get_direction() is confusing. But as it looks now, it also is not enough. Or we'd need another function from which I could determine the OUT_OPEN_DRAIN state. For the short term, I will remove the sanity check from the I2C core and hope the user properly set up the GPIO. It would be nice to check for it somewhen in the future, though. That all being said, I think this patch is still useful as is. Thanks, Wolfram > --- > > I got puzzled by the EINVAL until I found out that gpio-rcar simply does not > implement it. > > @Geert: any reason gpio-rcar is missing it? I would need it for the > i2c-gpio-fault-injector. > > drivers/gpio/gpiolib.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index e11a3bb03820..18719f64e80b 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -210,15 +210,15 @@ static int gpiochip_find_base(int ngpio) > */ > int gpiod_get_direction(struct gpio_desc *desc) > { > - struct gpio_chip *chip; > - unsigned offset; > - int status = -EINVAL; > + struct gpio_chip *chip; > + unsigned offset; > + int status; > > chip = gpiod_to_chip(desc); > offset = gpio_chip_hwgpio(desc); > > if (!chip->get_direction) > - return status; > + return -ENOTSUPP; > > status = chip->get_direction(chip, offset); > if (status > 0) { > -- > 2.11.0 >
> For the short term, I will remove the sanity check from the I2C core and > hope the user properly set up the GPIO. For the record, this patch is here: http://patchwork.ozlabs.org/patch/943115/ Forgot to cc linux-gpio.
On Wed, Jul 11, 2018 at 06:33:19PM +0200, Wolfram Sang wrote: > EINVAL is very generic, use ENOTSUPP in case the gpiochip does not > provide this function. While removing the assignment from the 'status' > variable, use better indentation in the declaration block. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > --- > > I got puzzled by the EINVAL until I found out that gpio-rcar simply does not > implement it. > > @Geert: any reason gpio-rcar is missing it? I would need it for the > i2c-gpio-fault-injector. > > drivers/gpio/gpiolib.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index e11a3bb03820..18719f64e80b 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -210,15 +210,15 @@ static int gpiochip_find_base(int ngpio) > */ > int gpiod_get_direction(struct gpio_desc *desc) > { > - struct gpio_chip *chip; > - unsigned offset; > - int status = -EINVAL; > + struct gpio_chip *chip; > + unsigned offset; > + int status; > > chip = gpiod_to_chip(desc); > offset = gpio_chip_hwgpio(desc); > > if (!chip->get_direction) > - return status; > + return -ENOTSUPP; > > status = chip->get_direction(chip, offset); > if (status > 0) { > -- > 2.11.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> That all being said, I think this patch is still useful as is.
Linus, do you have time to comment on this?
On Wed, Jul 25, 2018 at 10:20 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > That all being said, I think this patch is still useful as is. > > Linus, do you have time to comment on this? This looks like a good solution to me. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Sun, Jul 29, 2018 at 11:33 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Jul 25, 2018 at 10:20 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > That all being said, I think this patch is still useful as is. > > > > Linus, do you have time to comment on this? > > This looks like a good solution to me. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Any plans to apply it? drivers/gpio/gpiolib.c is your territory, I believe ;-) Thanks! Gr{oetje,eeting}s, Geert
On Tue, Sep 11, 2018 at 2:04 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Sun, Jul 29, 2018 at 11:33 PM Linus Walleij <linus.walleij@linaro.org> wrote:> > On Wed, Jul 25, 2018 at 10:20 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > That all being said, I think this patch is still useful as is. > > > > > > Linus, do you have time to comment on this? > > > > This looks like a good solution to me. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Any plans to apply it? drivers/gpio/gpiolib.c is your territory, I believe ;-) Ooops sorry. Patch applied. Yours, Linus Walleij
> > Any plans to apply it? drivers/gpio/gpiolib.c is your territory, I believe ;-) > > Ooops sorry. > > Patch applied. Thanks Geert & Linus!
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e11a3bb03820..18719f64e80b 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -210,15 +210,15 @@ static int gpiochip_find_base(int ngpio) */ int gpiod_get_direction(struct gpio_desc *desc) { - struct gpio_chip *chip; - unsigned offset; - int status = -EINVAL; + struct gpio_chip *chip; + unsigned offset; + int status; chip = gpiod_to_chip(desc); offset = gpio_chip_hwgpio(desc); if (!chip->get_direction) - return status; + return -ENOTSUPP; status = chip->get_direction(chip, offset); if (status > 0) {
EINVAL is very generic, use ENOTSUPP in case the gpiochip does not provide this function. While removing the assignment from the 'status' variable, use better indentation in the declaration block. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- I got puzzled by the EINVAL until I found out that gpio-rcar simply does not implement it. @Geert: any reason gpio-rcar is missing it? I would need it for the i2c-gpio-fault-injector. drivers/gpio/gpiolib.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)