Re: [RFC PATCH 6/9] gpiolib: add API to get gpio desc and flags
From: Alexandre Courbot <hidden>
Date: 2014-08-18 16:24:48
On Fri, Aug 15, 2014 at 11:53 PM, Mika Westerberg [off-list ref] wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 2ebc9071e354..e6c2413a6fbf 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c@@ -2644,6 +2644,24 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id, return desc; } +struct gpio_desc *dev_get_gpiod_flags(struct device *dev, unsigned int idx, + enum gpio_lookup_flags *flags) +{ + struct gpio_desc *desc = ERR_PTR(-ENOENT); + + if (!dev || !flags) + return ERR_PTR(-EINVAL); + + /* Using device tree? */ + if (IS_ENABLED(CONFIG_OF) && dev->of_node) + desc = of_find_gpio(dev, NULL, idx, flags); + else if (IS_ENABLED(CONFIG_ACPI) && ACPI_COMPANION(dev)) + desc = acpi_get_gpiod_flags(dev, idx, flags); + + return desc; +} +EXPORT_SYMBOL(dev_get_gpiod_flags);
Putting aside the fact that this function is clearly ACPI-centric (no con_id parameter and no handling of the platform interface), I have two big problems with it and it ending up in the consumer interface: 1) The returned descriptor is not requested by gpiolib, which means no check is made about whether the GPIO has already been requested by someone else, and another driver can very well request the same GPIO later and obtain it. Any descriptor returned by a function in consumer.h *must* be properly requested. Furthermore the 1:1 mapping between GPIO descriptors and GPIO numbers is not something we can take for granted (since it will likely change soon), so this practice is definitely to ban. 2) It exposes the GPIO flags, while they are supposed to be opaque to consumers. These two points would somehow be acceptable if this function was gpiolib-private, but here it is clearly not the case and this allows pretty nasty thing to happen. Basically you are using it to take advantage of the gpiod lookup mechanism and then quickly fall back to the legacy integer interface. That's really not something to encourage - these drivers should be converted to use gpiod internally (while preserving integer-based lookup for compatiblity, if needed). In patch 8 you say: "this can be solved by adding a new field of type struct gpio_desc but then there is another problem: the devm_gpiod_get needs to operate on the button device instead of its parent device that has the driver binded, so when the driver is unloaded, the resources for the gpio will not get freed automatically." I'd very much prefer that you use the non-devm variant of gpiod_get() and free the resources manually when the driver is unloaded than this workaround that introduces an loophole in the gpiod consumer lookup functions.