[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-gpio
Subject: Re: [PATCH] gpio: omap: Give unique labels to each GPIO bank/chip
From: Johan Hovold <johan () kernel ! org>
Date: 2017-12-29 14:03:42
Message-ID: 20171229140342.GD16993 () localhost
[Download RAW message or body]
On Fri, Dec 29, 2017 at 01:28:12PM +0100, Linus Walleij wrote:
> As we need to add GPIO lookup tables to the OMAP platforms, we
> need to reference each GPIO chip with a unique label. Use the GPIO
> base to name each chip, "gpio-0-31", "gpio-32-63" etc. Also fix
> a small error on the error path of the probe() where the chip
> would not be free()ed proberly under all circumstances.
Since these two changes appear to be entirely unrelated, I suggest
splitting this one up into two patches.
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: linux-omap@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/gpio/gpio-omap.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index e136d666f1e5..14fc3fb6f96e 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1059,6 +1059,7 @@ static void omap_gpio_mod_init(struct gpio_bank *bank)
> static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
> {
> static int gpio;
> + const char *label;
> int irq_base = 0;
> int ret;
>
> @@ -1080,7 +1081,11 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
> bank->chip.parent = &omap_mpuio_device.dev;
> bank->chip.base = OMAP_MPUIO(0);
> } else {
> - bank->chip.label = "gpio";
> + label = devm_kasprintf(bank->chip.parent, GFP_KERNEL, "gpio-%d-%d",
> + gpio, gpio + bank->width - 1);
> + if (!label)
> + return -ENOMEM;
> + bank->chip.label = label;
> bank->chip.base = gpio;
> }
> bank->chip.ngpio = bank->width;
> @@ -1104,7 +1109,8 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
> -1, 0, bank->width, 0);
> if (irq_base < 0) {
> dev_err(bank->chip.parent, "Couldn't allocate IRQ numbers\n");
> - return -ENODEV;
> + ret = -ENODEV;
> + goto out_err_chip;
> }
> #endif
>
> @@ -1123,7 +1129,8 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
> dev_err(bank->chip.parent,
> "Couldn't add irqchip to gpiochip %d\n", ret);
> gpiochip_remove(&bank->chip);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto out_err_chip;
Here it looks like you are now introducing a double free
(gpiochip_remove) instead.
> }
>
> gpiochip_set_chained_irqchip(&bank->chip, irqc, bank->irq, NULL);
> @@ -1132,8 +1139,12 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
> omap_gpio_irq_handler,
> 0, dev_name(bank->chip.parent), bank);
> if (ret)
> - gpiochip_remove(&bank->chip);
> + goto out_err_chip;
> +
> + return ret;
I'd use "return 0" here for readability reasons.
>
> +out_err_chip:
And name this label after what it does, for example, "err_remove_chip".
> + gpiochip_remove(&bank->chip);
> return ret;
> }
Johan
--
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic