diff mbox

[v3,3/9] gpiolib: Use GPIO name from names array for gpio descriptor

Message ID 1439561466-14350-4-git-send-email-mpa@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Markus Pargmann Aug. 14, 2015, 2:11 p.m. UTC
This patch adds GPIO names to the GPIO descriptors when initializing the
gpiochip. It also introduces a check whether any of the new names will
conflict with an existing GPIO name.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Linus Walleij Sept. 22, 2015, 12:56 a.m. UTC | #1
On Fri, Aug 14, 2015 at 7:11 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> This patch adds GPIO names to the GPIO descriptors when initializing the
> gpiochip. It also introduces a check whether any of the new names will
> conflict with an existing GPIO name.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Patch applied. To me this is the magic transition patch that make
things really smooth.

Yours,
Linus Walleij
Johan Hovold Sept. 23, 2015, 10:02 p.m. UTC | #2
On Fri, Aug 14, 2015 at 04:11:00PM +0200, Markus Pargmann wrote:
> This patch adds GPIO names to the GPIO descriptors when initializing the
> gpiochip. It also introduces a check whether any of the new names will
> conflict with an existing GPIO name.

And this is why we should not do this. I've said it it before: we need
to consider dynamic buses, not just static device trees.

We have USB and Greybus that will soon be exposing gpiochips and their
line-names can not be globally unique.

Before figuring out what a sane userspace interface should look like we
should not make ABI changes that we need to support indefinitely. The
legacy sysfs interface should instead be frozen.

Exposing a name attribute for a not-necessarily globally unique name
could perhaps be ok, but by enforcing uniqueness at this point we will
then need to come up with another mechanism for naming gpios on dynamic
buses (while maintaining this one indefinitely in parallel).

Linus, please consider dropping these changes for now.

Thanks,
Johan
Linus Walleij Sept. 24, 2015, 9:50 p.m. UTC | #3
On Wed, Sep 23, 2015 at 3:02 PM, Johan Hovold <johan@kernel.org> wrote:

> Before figuring out what a sane userspace interface should look like we
> should not make ABI changes that we need to support indefinitely. The
> legacy sysfs interface should instead be frozen.
>
> Exposing a name attribute for a not-necessarily globally unique name
> could perhaps be ok, but by enforcing uniqueness at this point we will
> then need to come up with another mechanism for naming gpios on dynamic
> buses (while maintaining this one indefinitely in parallel).
>
> Linus, please consider dropping these changes for now.

I rounded off the series after moving the name attribute to the descriptor
before doing any ABI changes, as it seems we need to discuss this.

This patch concludes that part of the series, sent a separate patch
concluding this first half of the series.

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eed3751fe14f..3908503bebfe 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -250,6 +250,38 @@  static int gpiochip_add_to_list(struct gpio_chip *chip)
 	return err;
 }
 
+/*
+ * Takes the names from gc->names and checks if they are all unique. If they
+ * are, they are assigned to their gpio descriptors.
+ *
+ * Returns -EEXIST if one of the names is already used for a different GPIO.
+ */
+static int gpiochip_set_desc_names(struct gpio_chip *gc)
+{
+	int i;
+
+	if (!gc->names)
+		return 0;
+
+	/* First check all names if they are unique */
+	for (i = 0; i != gc->ngpio; ++i) {
+		struct gpio_desc *gpio;
+
+		gpio = gpio_name_to_desc(gc->names[i]);
+		if (gpio) {
+			dev_err(gc->dev, "Detected name collision for GPIO name '%s'\n",
+				gc->names[i]);
+			return -EEXIST;
+		}
+	}
+
+	/* Then add all names to the GPIO descriptors */
+	for (i = 0; i != gc->ngpio; ++i)
+		gc->desc[i].name = gc->names[i];
+
+	return 0;
+}
+
 /**
  * gpiochip_add() - register a gpio_chip
  * @chip: the chip to register, with chip->base initialized
@@ -319,6 +351,10 @@  int gpiochip_add(struct gpio_chip *chip)
 	INIT_LIST_HEAD(&chip->pin_ranges);
 #endif
 
+	status = gpiochip_set_desc_names(chip);
+	if (status)
+		goto err_remove_from_list;
+
 	of_gpiochip_add(chip);
 	acpi_gpiochip_add(chip);
 
@@ -336,6 +372,7 @@  err_remove_chip:
 	acpi_gpiochip_remove(chip);
 	gpiochip_free_hogs(chip);
 	of_gpiochip_remove(chip);
+err_remove_from_list:
 	spin_lock_irqsave(&gpio_lock, flags);
 	list_del(&chip->list);
 	spin_unlock_irqrestore(&gpio_lock, flags);