Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758679AbaGXGaF (ORCPT ); Thu, 24 Jul 2014 02:30:05 -0400 Received: from router.aksignal.cz ([188.175.113.102]:34827 "EHLO router.aksignal.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751171AbaGXGaD (ORCPT ); Thu, 24 Jul 2014 02:30:03 -0400 Message-ID: <53D0A7E4.1040707@aksignal.cz> Date: Thu, 24 Jul 2014 08:29:56 +0200 From: =?UTF-8?B?SmnFmcOtIFByY2hhbA==?= Reply-To: jiri.prchal@aksignal.cz Organization: AK signal Brno User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: Alexandre Courbot , Guenter Roeck CC: "linux-gpio@vger.kernel.org" , Linux Kernel Mailing List , "linux-doc@vger.kernel.org" , Linus Walleij , Randy Dunlap Subject: Re: [RFC PATCH] gpiolib: Provide and export gpiod_export_name References: <1406139132-5404-1-git-send-email-linux@roeck-us.net> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, just to append to this: is in plan some kind of exporting named GPIOs from device tree to /sys/* or /dev/*? It would be very useful not only for me. Because I thing what GPIO is used for what is hardware design dependent same as for example what SPI chips are used. And SPI chips are in DT. Dne 24.7.2014 v 07:44 Alexandre Courbot napsal(a): > On Thu, Jul 24, 2014 at 3:12 AM, Guenter Roeck wrote: >> gpiod_export_name is similar to gpiod_export, but lets the user >> determine the name used to export a gpio pin. >> >> Currently, the pin name is determined by the chip driver with >> the 'names' array in the gpio_chip data structure, or it is set >> to gpioX, where X is the pin number, if no name is provided by >> the chip driver. > > Oh, my. I did not even know about this 'names' array. This is pretty > bad - a GPIO provider should not decide what its GPIOs are used for. > > Luckily for you, this creates a precedent that makes this patch more > acceptable, in that it is not making the situation worse. Even though > I consider both solutions to be bad, I actually prefer your > gpiod_export_name() function to that 'names' array in gpio_chip... > >> >> It is, however, desirable to be able to provide the pin name when >> exporting the pin, for example from platform code. In other words, >> it would be useful to move the naming decision from the pin provider >> to the pin consumer. The gpio-pca953x driver provides this capability >> as part of its platform data. Other drivers could be enhanced in a >> similar way; however, this is not always possible or easy to accomplish. >> For example, mfd client drivers such as gpio-ich already use platform >> data to pass information from the mfd master driver to the client driver. >> Overloading this platform data to also provide an array of gpio pin names >> would be a challenge if not impossible. >> >> The alternative to use gpiod_export_link is also not always desirable, >> since it only creates a named link to a different directory, meaning >> the named gpio pin is not available in /sys/class/gpio but only >> in some platform specific directory and thus not as generic as possible >> and/or useful. >> >> A specific example for a use case is a gpio pin which reports AC power >> loss to user space. Depending on the platform and platform variant, >> the pin can be provided by various gpio chip drivers and pin numbers. >> It would be very desirable to have a well defined location such as >> /sys/class/gpio/ac_power_loss for this pin, so user space knows where >> to find the attribute without knowledge of the underlying platform >> variant or oher hardware details. > > As I explained on the other thread, I still encourage you to have > these GPIOs under device nodes that give a hint of who is provided the > GPIO (effectively exporting the (dev, function) tuple to user-space) > instead of having them popping out under /sys/class/gpio where nobody > knows where they come from and name collisions are much more likely. > > Your message sounds like you have no choice but have the named GPIO > link under the gpiochip's device node, but this is not the case - > gpio_export_link() let's you specify the device under which the link > should appear. Make that device be your "scu" device and you can have > a consistent sysfs path to access your GPIOs. > > Allowing GPIOs to pop up in the same directory with an arbitrary name > is just a recipe for a mess. But that's a recipe that is already > allowed thanks to that 'names' array. So I'm really confused about > what to do with this patch. If you can do with gpio_export_link() (and > I have not seen evidence of the contrary), please go that way instead. > >> >> Signed-off-by: Guenter Roeck >> --- >> Applies to tip of linux-gpio/for-next. >> >> Documentation/gpio/sysfs.txt | 12 ++++++++---- >> drivers/gpio/gpiolib-sysfs.c | 23 ++++++++++++++++------- >> include/linux/gpio/consumer.h | 9 +++++++++ >> 3 files changed, 33 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt >> index c2c3a97..8e301b2 100644 >> --- a/Documentation/gpio/sysfs.txt >> +++ b/Documentation/gpio/sysfs.txt >> @@ -125,7 +125,11 @@ requested using gpio_request(): >> /* export the GPIO to userspace */ >> int gpiod_export(struct gpio_desc *desc, bool direction_may_change); >> >> - /* reverse gpio_export() */ >> + /* export named GPIO to userspace */ >> + int gpiod_export_name(struct gpio_desc *desc, const char *ioname, >> + bool direction_may_change); >> + >> + /* reverse gpio_export() / gpiod_export_name() */ >> void gpiod_unexport(struct gpio_desc *desc); >> >> /* create a sysfs link to an exported GPIO node */ >> @@ -136,9 +140,9 @@ requested using gpio_request(): >> int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value); >> >> After a kernel driver requests a GPIO, it may only be made available in >> -the sysfs interface by gpiod_export(). The driver can control whether the >> -signal direction may change. This helps drivers prevent userspace code >> -from accidentally clobbering important system state. >> +the sysfs interface by gpiod_export() or gpiod_export_name(). The driver >> +can control whether the signal direction may change. This helps drivers >> +prevent userspace code from accidentally clobbering important system state. >> >> This explicit exporting can help with debugging (by making some kinds >> of experiments easier), or can provide an always-there interface that's >> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c >> index be45a92..7d36230 100644 >> --- a/drivers/gpio/gpiolib-sysfs.c >> +++ b/drivers/gpio/gpiolib-sysfs.c >> @@ -523,13 +523,12 @@ static struct class gpio_class = { >> * >> * Returns zero on success, else an error. >> */ >> -int gpiod_export(struct gpio_desc *desc, bool direction_may_change) >> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname, >> + bool direction_may_change) >> { >> unsigned long flags; >> int status; >> - const char *ioname = NULL; >> struct device *dev; >> - int offset; >> >> /* can't export until sysfs is available ... */ >> if (!gpio_class.p) { >> @@ -560,10 +559,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) >> direction_may_change = false; >> spin_unlock_irqrestore(&gpio_lock, flags); >> >> - offset = gpio_chip_hwgpio(desc); >> - if (desc->chip->names && desc->chip->names[offset]) >> - ioname = desc->chip->names[offset]; >> - >> dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0), >> desc, ioname ? ioname : "gpio%u", >> desc_to_gpio(desc)); >> @@ -600,6 +595,20 @@ fail_unlock: >> gpiod_dbg(desc, "%s: status %d\n", __func__, status); >> return status; >> } >> +EXPORT_SYMBOL_GPL(gpiod_export_name); >> + >> +int gpiod_export(struct gpio_desc *desc, bool direction_may_change) >> +{ >> + const char *ioname = NULL; >> + >> + if (desc) { >> + int offset = gpio_chip_hwgpio(desc); >> + >> + if (desc->chip->names && desc->chip->names[offset]) >> + ioname = desc->chip->names[offset]; > > I'd add a > > else > ioname = "gpio%u"; > > so all the name-chosing code is grouped in the same place. Then you > can remove that same check from gpiod_export_name(). > >> + } >> + return gpiod_export_name(desc, ioname, direction_may_change); >> +} >> EXPORT_SYMBOL_GPL(gpiod_export); >> >> static int match_export(struct device *dev, const void *data) >> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h >> index 05e53cc..986da3e 100644 >> --- a/include/linux/gpio/consumer.h >> +++ b/include/linux/gpio/consumer.h >> @@ -260,6 +260,8 @@ static inline int desc_to_gpio(const struct gpio_desc *desc) >> #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS) >> >> int gpiod_export(struct gpio_desc *desc, bool direction_may_change); >> +int gpiod_export_name(struct gpio_desc *desc, const char *ioname, >> + bool direction_may_change); >> int gpiod_export_link(struct device *dev, const char *name, >> struct gpio_desc *desc); >> int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value); >> @@ -273,6 +275,13 @@ static inline int gpiod_export(struct gpio_desc *desc, >> return -ENOSYS; >> } >> >> +static inline int gpiod_export_name(struct gpio_desc *desc, >> + const char *ioname, >> + bool direction_may_change) >> +{ >> + return -ENOSYS; >> +} >> + >> static inline int gpiod_export_link(struct device *dev, const char *name, >> struct gpio_desc *desc) >> { >> -- >> 1.9.1 >> > -- > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/