lkml.org 
[lkml]   [2014]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC PATCH] gpiolib: Provide and export gpiod_export_name
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 <linux@roeck-us.net> 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 <linux@roeck-us.net>
>>>> ---
>>>> 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/

\
 
 \ /
  Last update: 2014-07-25 10:21    [W:0.065 / U:2.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site