Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758067AbaGYHrQ (ORCPT ); Fri, 25 Jul 2014 03:47:16 -0400 Received: from router.aksignal.cz ([188.175.113.102]:54389 "EHLO router.aksignal.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750818AbaGYHrO (ORCPT ); Fri, 25 Jul 2014 03:47:14 -0400 Message-ID: <53D20B6E.9010808@aksignal.cz> Date: Fri, 25 Jul 2014 09:46:54 +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: Guenter Roeck , Alexandre Courbot 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> <53D0A7E4.1040707@aksignal.cz> <53D0A983.9000900@roeck-us.net> In-Reply-To: <53D0A983.9000900@roeck-us.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org What about this modification? If is defined label, use it prioritlly, at second use name in chip description. @@ -842,7 +842,9 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) spin_unlock_irqrestore(&gpio_lock, flags); offset = gpio_chip_hwgpio(desc); - if (desc->chip->names && desc->chip->names[offset]) + if (desc->label) + ioname = desc->label; + else if (desc->chip->names && desc->chip->names[offset]) ioname = desc->chip->names[offset]; dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0), Dne 24.7.2014 v 08:36 Guenter Roeck napsal(a): > On 07/23/2014 11:29 PM, Jiří Prchal wrote: >> 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. >> > > Yes, for one of my use cases that is how I would probably configure it; > alternatively it would be configured with platform data. It is > somewhat questionable, however, if this approach would be acceptable > for the Linux dt community, as it is a corner case between system > (hardware) description and configuration. > > Guenter > >> 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-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/