gpiolib: Add GPIO name support
cover gpiolib: Add GPIO name support
Hi,
This is a proposal to add GPIO names to the kernel based on devicetree descriptions.
This series adds GPIO name support. Until now it is only possible to use names for already requested GPIOs (for example what they are used for). It is not possible to identify GPIOs by a name although most of them have a name for example in the schematics of the board. This makes it difficult to identify a specific GPIO from userspace.
As the GPIO name information is a hardware description this series uses the devicetree bindings introduced by the GPIO hogging mechanism, specifically 'line-name', to identify GPIOs. The sysfs 'export' file is changed to accept names as fallback. The gpio numbers still have a higher priority to ensure backwards compatibility.
Exported GPIOs are still using their number as directory name (gpio<ID>). But the directories now contain a 'name' file which is '(null)' for non-existent names and the name otherwise.
This series can be used to have an easy name mapping for udev with a quite simple rule similar to this:
SUBSYSTEM=="gpio", KERNEL=="gpio*", ATTR{name}!="(null)", ACTION=="add", \
PROGRAM+="/bin/sh -c 'mkdir -p /dev/gpios; rm -f /dev/gpios/$attr{name}; ln -s /sys%p/ /dev/gpios/$attr{name}"With this rule udev adds a link for each exported GPIO with a name into /dev/gpios/. This way it is not necessary to know the number of a GPIO to use it.
Best Regards,
Markus
Markus Pargmann (9):
gpiolib: Fix possible use of wrong name
gpiolib-of: Rename gpio_hog functions to be generic
gpio: Allow hogged gpios to be requested
gpio: Add 'name' to the gpio descriptor struct
gpiolib: Implement gpio_name_to_desc()
gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name
gpiolib-sysfs: Add gpio name parsing for sysfs export
gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name
gpiolib: Add gpio name information to /sys/kernel/debug/gpio
drivers/gpio/gpiolib-of.c | 45 +++++++++++++++++++------------ drivers/gpio/gpiolib-sysfs.c | 61 ++++++++++++++++++++++++++++++++++--------- drivers/gpio/gpiolib.c | 46 +++++++++++++++++++++++++++++--- drivers/gpio/gpiolib.h | 3 +++ include/linux/gpio/consumer.h | 7 +++++ 5 files changed, 128 insertions(+), 34 deletions(-)
Signature · 1 line
2.1.4
PATCH 1/9 gpiolib: Fix possible use of wrong name
The name is set optionally from DT. Therefore we need to reset the name variable for every loop iteration. Otherwise we could use a wrong or uninitialized name.
--- drivers/gpio/gpiolib-of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpiolib-of.c +1 -1
@@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip) {struct gpio_desc *desc =NULL ;struct device_node *np; -const char *name;enum gpio_lookup_flags lflags;enum gpiod_flags dflags;for_each_child_of_node (chip->of_node, np) { +const char *name =NULL ;if (!of_property_read_bool (np,"gpio-hog" ))continue ;
Signature · 1 line
2.1.4
PATCH 1/9 gpiolib: Fix possible use of wrong name
Quoted 3 lines · Markus Pargmann
The name is set optionally from DT. Therefore we need to reset the name variable for every loop iteration. Otherwise we could use a wrong or uninitialized name.
This doesn't make sense. of_get_gpio_hog falls back to using the node name so name will always be updated (unless there's an error).
Could be better documented, though.
Quoted 11 lines
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib-of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpio/gpiolib-of.c +0
-0@@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip) {
drivers/gpio/gpiolib-of.c static void of_gpiochip_scan_hogs(struct gpio_chip *chip) · line 218
Quoted 10 lines
drivers/gpio/gpiolib-of.c +1
-1struct gpio_desc *desc =NULL ;struct device_node *np; -const char *name;enum gpio_lookup_flags lflags;enum gpiod_flags dflags;for_each_child_of_node (chip->of_node, np) { +const char *name =NULL ;if (!of_property_read_bool (np,"gpio-hog" ))continue ;
Johan
PATCH 1/9 gpiolib: Fix possible use of wrong name
Quoted 9 lines · Johan Hovold
Quoted 3 lines · Markus Pargmann
The name is set optionally from DT. Therefore we need to reset the name variable for every loop iteration. Otherwise we could use a wrong or uninitialized name.
This doesn't make sense.
of_get_gpio_hogfalls back to using the node name so name will always be updated (unless there's an error).Could be better documented, though.
Thanks, just got the two lines wrong. Yes the name is always set correctly.
Will fix it.
Best regards,
Markus
Quoted 30 lines
Quoted 12 lines
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib-of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpio/gpiolib-of.c +0
-0@@ -209,11 +209,11 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip) {drivers/gpio/gpiolib-of.c static void of_gpiochip_scan_hogs(struct gpio_chip *chip) · line 218
Quoted 10 lines
drivers/gpio/gpiolib-of.c +1
-1struct gpio_desc *desc =NULL ;struct device_node *np; -const char *name;enum gpio_lookup_flags lflags;enum gpiod_flags dflags;for_each_child_of_node (chip->of_node, np) { +const char *name =NULL ;if (!of_property_read_bool (np,"gpio-hog" ))continue ;Johan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/d49f77bb/attachment.sig>
PATCH 2/9 gpiolib-of: Rename gpio_hog functions to be generic
In reply to Markus PargmannThe gpio hogging functions are currently only used for gpio-hogging. But these functions are widely generic ones which parse gpio device nodes in the DT.
--- drivers/gpio/gpiolib-of.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/gpio/gpiolib-of.c +7 -7
@@ -119,17 +119,17 @@ int of_get_named_gpio_flags(struct device_node *np, const char *list_name,EXPORT_SYMBOL (of_get_named_gpio_flags);/** -* of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API +* of_parse_gpio() - Get a GPIO hog descriptor, names and flags for GPIO API * @np: device node to get GPIO from * @name: GPIO line name * @lflags: gpio_lookup_flags - returned from of_find_gpio() or -* of_get_gpio_hog() +* of_parse_gpio() * @dflags: gpiod_flags - optional GPIO initialization flags * * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno * value on the error condition. */ -static struct gpio_desc *of_get_gpio_hog (struct device_node *np, +static struct gpio_desc *of_parse_gpio (struct device_node *np,const char **name,enum gpio_lookup_flags *lflags,enum gpiod_flags *dflags)
@@ -199,13 +199,13 @@ static struct gpio_desc *of_get_gpio_hog(struct device_node *np, }/** -* of_gpiochip_scan_hogs - Scan gpio-controller and apply GPIO hog as requested +* of_gpiochip_scan_gpios - Scan gpio-controller for gpio definitions * @chip: gpio chip to act on * * This is only used by of_gpiochip_add to request/set GPIO initial * configuration. */ -static void of_gpiochip_scan_hogs (struct gpio_chip *chip) +static void of_gpiochip_scan_gpios (struct gpio_chip *chip) {struct gpio_desc *desc =NULL ;struct device_node *np;
@@ -217,7 +217,7 @@ static void of_gpiochip_scan_hogs(struct gpio_chip *chip)if (!of_property_read_bool (np,"gpio-hog" ))continue ; - desc =of_get_gpio_hog (np, &name, &lflags, &dflags); + desc =of_parse_gpio (np, &name, &lflags, &dflags);if (IS_ERR (desc))continue ;
@@ -436,7 +436,7 @@ void of_gpiochip_add(struct gpio_chip *chip)of_gpiochip_add_pin_range (chip);of_node_get (chip->of_node); -of_gpiochip_scan_hogs (chip); +of_gpiochip_scan_gpios (chip); }void of_gpiochip_remove (struct gpio_chip *chip)
Signature · 1 line
2.1.4
PATCH 3/9 gpio: Allow hogged gpios to be requested
In reply to Markus PargmannIt can be useful to claim hogged gpios later, for example from userspace. This allows to set defaults for GPIOs using the hogging mechanism and override the setup later from userspace or a kernel driver.
This patch adds a check for hogged gpios to allow requesting them. If the gpio is not hogged but marked as requested, it still fails with -EBUSY.
--- drivers/gpio/gpiolib.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpio/gpiolib.c +2 -1
@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)* before IRQs are enabled, for non-sleeping (SOC) GPIOs. */ -if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 ) { +if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 || +test_and_clear_bit (FLAG_IS_HOGGED , &desc->flags) ==1 ) {desc_set_label (desc, label ? :"?" ); status =0 ; }else {
Signature · 1 line
2.1.4
PATCH 3/9 gpio: Allow hogged gpios to be requested
Hello,
Quoted 18 lines · Markus Pargmann
It can be useful to claim hogged gpios later, for example from userspace. This allows to set defaults for GPIOs using the hogging mechanism and override the setup later from userspace or a kernel driver.
This patch adds a check for hogged gpios to allow requesting them. If the gpio is not hogged but marked as requested, it still fails with
-EBUSY.Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/gpio/gpiolib.c +0
-0
drivers/gpio/gpiolib.c static int __gpiod_request(struct gpio_desc *desc, const char *label) · line 804
Quoted 8 lines
drivers/gpio/gpiolib.c +2
-1@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)* before IRQs are enabled, for non-sleeping (SOC) GPIOs. */ -if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 ) { +if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 || +test_and_clear_bit (FLAG_IS_HOGGED , &desc->flags) ==1 ) {desc_set_label (desc, label ? :"?" ); status =0 ;
I don't like this patch. IMHO hogging is a "use" of a GPIO that should prevent it being requested.
While I think it's useful to be able to export some hogged pins I don't think this should be possible for all hogged pins unconditionally. And for gpios being used by drivers I'd expect they don't need to be hogged at all.
I don't have a good idea how to solve that. Adding another property to a gpio that should be allowd to be exported can hardly count as hardware description and so doesn't belong in a device tree?!
Please don't consider this objection as a reason for a NACK, only as starting point for a discussion.
Apart from that: Does this result in hogged gpios being able to be requested by two additional drivers in parallel? Maybe the IS_HOGGED flag should be dropped when the gpio is requested?
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 3/9 gpio: Allow hogged gpios to be requested
Hi Uwe,
Quoted 24 lines · Uwe Kleine-K?nig
Hello,
Quoted 21 lines · Markus Pargmann
It can be useful to claim hogged gpios later, for example from userspace. This allows to set defaults for GPIOs using the hogging mechanism and override the setup later from userspace or a kernel driver.
This patch adds a check for hogged gpios to allow requesting them. If the gpio is not hogged but marked as requested, it still fails with
-EBUSY.Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/gpio/gpiolib.c +0
-0@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)* before IRQs are enabled, for non-sleeping (SOC) GPIOs. */
Quoted 7 lines
drivers/gpio/gpiolib.c static int __gpiod_request(struct gpio_desc *desc, const char *label) · line 804
Quoted 5 lines
drivers/gpio/gpiolib.c +2
-1-if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 ) { +if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 || +test_and_clear_bit (FLAG_IS_HOGGED , &desc->flags) ==1 ) {desc_set_label (desc, label ? :"?" ); status =0 ;I don't like this patch. IMHO hogging is a "use" of a GPIO that should prevent it being requested.
I disagree with you here. The original patch stated in its description that it was designed to initialize GPIOs. In my understanding this does not necessarily mean that a hogged GPIO has to be blocked forever.
It may be a use case for example to initialize multiplexers to known safe values to work with the system right from the beginning. Later it may be necessary to change them.
Quoted 10 lines
While I think it's useful to be able to export some hogged pins I don't think this should be possible for all hogged pins unconditionally. And for gpios being used by drivers I'd expect they don't need to be hogged at all.
I don't have a good idea how to solve that. Adding another property to a gpio that should be allowd to be exported can hardly count as hardware description and so doesn't belong in a device tree?!
Quoted 6 lines
Please don't consider this objection as a reason for a NACK, only as starting point for a discussion.
Apart from that: Does this result in hogged gpios being able to be requested by two additional drivers in parallel? Maybe the
IS_HOGGEDflag should be dropped when the gpio is requested?
The IS_HOGGED flag is cleared at the same time it is tested so only one consumer can request one hogged GPIO. The GPIO is not considered to be hogged after it is normally requested.
Best regards,
Markus
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150719/b22ebe66/attachment.sig>
PATCH 3/9 gpio: Allow hogged gpios to be requested
Hello Markus,
Quoted 18 lines · Markus Pargmann
Quoted 9 lines · Uwe Kleine-K?nig
Quoted 8 lines · Markus Pargmann
drivers/gpio/gpiolib.c +0
-0@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)* before IRQs are enabled, for non-sleeping (SOC) GPIOs. */ Quoted 7 lines
drivers/gpio/gpiolib.c static int __gpiod_request(struct gpio_desc *desc, const char *label) · line 804
Quoted 5 lines
drivers/gpio/gpiolib.c +2
-1-if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 ) { +if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 || +test_and_clear_bit (FLAG_IS_HOGGED , &desc->flags) ==1 ) {desc_set_label (desc, label ? :"?" ); status =0 ;I don't like this patch. IMHO hogging is a "use" of a GPIO that should prevent it being requested.
Quoted 3 lines
I disagree with you here. The original patch stated in its description that it was designed to initialize GPIOs. In my understanding this does not necessarily mean that a hogged GPIO has to be blocked forever.
Assume for a moment I can agree with "not necessarily". But now, what about the cases where a hogged GPIO should be blocked? IMHO, if you want to drive the GPIO from userspace anyhow, you don't need to add a hog for it.
Quoted 3 lines
The
IS_HOGGEDflag is cleared at the same time it is tested so only one consumer can request one hogged GPIO. The GPIO is not considered to be hogged after it is normally requested.
You're right here, I missed the and_clear_bit part on the test.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 3/9 gpio: Allow hogged gpios to be requested
Quoted 21 lines · Uwe Kleine-K?nig
Hello Markus,
Quoted 18 lines · Markus Pargmann
Quoted 16 lines · Uwe Kleine-K?nig
drivers/gpio/gpiolib.c static int __gpiod_request(struct gpio_desc *desc, const char *label) · line 804
Quoted 13 lines · Markus Pargmann
drivers/gpio/gpiolib.c +2
-1@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)* before IRQs are enabled, for non-sleeping (SOC) GPIOs. */ -if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 ) { +if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 || +test_and_clear_bit (FLAG_IS_HOGGED , &desc->flags) ==1 ) {desc_set_label (desc, label ? :"?" ); status =0 ;I don't like this patch. IMHO hogging is a "use" of a GPIO that should prevent it being requested.
Quoted 7 lines
Quoted 3 lines
I disagree with you here. The original patch stated in its description that it was designed to initialize GPIOs. In my understanding this does not necessarily mean that a hogged GPIO has to be blocked forever.
Assume for a moment I can agree with "not necessarily". But now, what about the cases where a hogged GPIO should be blocked? IMHO, if you want to drive the GPIO from userspace anyhow, you don't need to add a hog for it.
If I don't use a hog I leave the system in an undefined state until the userspace can initialize the GPIOs. So all the userspace controlled GPIOs are undefined when all the drivers probe which can lead to problems if the GPIOs have any indirect influence on hardware components which drivers probe before the userspace.
I think it is valid to somehow define a safe state for GPIOs in the devicetree while you can later change this state from userspace?!
Best regards,
Markus
Quoted 13 lines
Quoted 4 lines
The
IS_HOGGEDflag is cleared at the same time it is tested so only one consumer can request one hogged GPIO. The GPIO is not considered to be hogged after it is normally requested.You're right here, I missed the
and_clear_bitpart on the test.Best regards
UweSignature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150720/382d68cd/attachment.sig>
PATCH 3/9 gpio: Allow hogged gpios to be requested
In reply to Markus PargmannQuoted 35 lines · Markus Pargmann
Hi Uwe,
Quoted 24 lines · Uwe Kleine-K?nig
Hello,
Quoted 21 lines · Markus Pargmann
It can be useful to claim hogged gpios later, for example from userspace. This allows to set defaults for GPIOs using the hogging mechanism and override the setup later from userspace or a kernel driver.
This patch adds a check for hogged gpios to allow requesting them. If the gpio is not hogged but marked as requested, it still fails with
-EBUSY.Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/gpio/gpiolib.c +0
-0@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)* before IRQs are enabled, for non-sleeping (SOC) GPIOs. */ Quoted 7 lines
drivers/gpio/gpiolib.c static int __gpiod_request(struct gpio_desc *desc, const char *label) · line 804
Quoted 5 lines
drivers/gpio/gpiolib.c +2
-1-if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 ) { +if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 || +test_and_clear_bit (FLAG_IS_HOGGED , &desc->flags) ==1 ) {desc_set_label (desc, label ? :"?" ); status =0 ;I don't like this patch. IMHO hogging is a "use" of a GPIO that should prevent it being requested.
Quoted 3 lines
I disagree with you here. The original patch stated in its description that it was designed to initialize GPIOs. In my understanding this does not necessarily mean that a hogged GPIO has to be blocked forever.
IIRC, this use case was discussed but was rejected by Linus when hogs were added:
https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g at mail.gmail.com
Linus?
Johan
PATCH 3/9 gpio: Allow hogged gpios to be requested
Quoted 36 lines · Johan Hovold
Quoted 35 lines · Markus Pargmann
Hi Uwe,
Quoted 31 lines · Uwe Kleine-K?nig
Hello,
Quoted 18 lines · Markus Pargmann
It can be useful to claim hogged gpios later, for example from userspace. This allows to set defaults for GPIOs using the hogging mechanism and override the setup later from userspace or a kernel driver.
This patch adds a check for hogged gpios to allow requesting them. If the gpio is not hogged but marked as requested, it still fails with
-EBUSY.Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/gpio/gpiolib.c +0
-0drivers/gpio/gpiolib.c static int __gpiod_request(struct gpio_desc *desc, const char *label) · line 804
Quoted 8 lines
drivers/gpio/gpiolib.c +2
-1@@ -798,7 +798,8 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)* before IRQs are enabled, for non-sleeping (SOC) GPIOs. */ -if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 ) { +if (test_and_set_bit (FLAG_REQUESTED , &desc->flags) ==0 || +test_and_clear_bit (FLAG_IS_HOGGED , &desc->flags) ==1 ) {desc_set_label (desc, label ? :"?" ); status =0 ;I don't like this patch. IMHO hogging is a "use" of a GPIO that should prevent it being requested.
Quoted 8 lines
Quoted 3 lines
I disagree with you here. The original patch stated in its description that it was designed to initialize GPIOs. In my understanding this does not necessarily mean that a hogged GPIO has to be blocked forever.
IIRC, this use case was discussed but was rejected by Linus when hogs were added:
https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g at mail.gmail.com
I see. Yes I really would like to have this 'initial settings' feature. Any suggestions for a better way?
Best regards,
Markus
Quoted 5 lines
Linus?
Johan
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/9f394a80/attachment.sig>
PATCH 3/9 gpio: Allow hogged gpios to be requested
In reply to Johan HovoldQuoted 9 lines · Johan Hovold
Quoted 6 lines · Markus Pargmann
Quoted 2 lines
I don't like this patch. IMHO hogging is a "use" of a GPIO that should prevent it being requested.
I disagree with you here. The original patch stated in its description that it was designed to initialize GPIOs. In my understanding this does not necessarily mean that a hogged GPIO has to be blocked forever.
Quoted 6 lines
IIRC, this use case was discussed but was rejected by Linus when hogs were added:
https://lkml.kernel.org/r/CACRpkdZcNcPBYQM438CZJx1gYst9BFBSTj-3Qv2aPGF9pdWa5g at mail.gmail.com
Linus?
Yeah that is true. But maybe I softened up a bit.
Basically I want to avoid having two very similar mechanisms.
But that is maybe more of a question of how the code is arranged and named than what it is called. It would be nicer if this was referring to initial values rather than hogging, the mechanism inside the kernel could be the same.
But then there is the path where something that is initially a hog is turned into a userspace GPIO. Then it is essentially not a hog anymore, so this property is not static.
Yours,
Linus Walleij
PATCH 4/9 gpio: Add 'name' to the gpio descriptor struct
In reply to Markus PargmannThe latest gpio hogging mechanism assigns each gpio a 'line-name' in the devicetree. The 'name' field is different from the 'label' field. 'label' is only used for requested GPIOs to describe its current use by driver or userspace.
The 'name' field describes the GPIO itself, not the use. This is most likely identical to the label in the schematic on the GPIO line and should help to find this particular GPIO.
--- drivers/gpio/gpiolib.h | 3 +++ 1 file changed, 3 insertions(+)
drivers/gpio/gpiolib.h +3 -0
@@ -89,7 +89,10 @@ struct gpio_desc {#define FLAG_USED_AS_IRQ 9/* GPIO is connected to an IRQ */ #define FLAG_IS_HOGGED 11/* GPIO is hogged */ +/* Connection label */ const char *label; +/* Name of the GPIO */ +const char *name; };int gpiod_request (struct gpio_desc *desc,const char *label);
Signature · 1 line
2.1.4
PATCH 4/9 gpio: Add 'name' to the gpio descriptor struct
Quoted 8 lines · Markus Pargmann
The latest gpio hogging mechanism assigns each gpio a 'line-name' in the devicetree. The 'name' field is different from the 'label' field. 'label' is only used for requested GPIOs to describe its current use by driver or userspace.
The 'name' field describes the GPIO itself, not the use. This is most likely identical to the label in the schematic on the GPIO line and should help to find this particular GPIO.
You merge this one with whatever patch starts using the name field.
Johan
PATCH 5/9 gpiolib: Implement gpio_name_to_desc()
In reply to Markus PargmannImplement a function to translate a gpio name to a descriptor.
--- drivers/gpio/gpiolib.c | 32 ++++++++++++++++++++++++++++++++ include/linux/gpio/consumer.h | 7 +++++++ 2 files changed, 39 insertions(+)
drivers/gpio/gpiolib.c +32 -0
@@ -90,6 +90,38 @@ struct gpio_desc *gpio_to_desc(unsigned gpio)EXPORT_SYMBOL_GPL (gpio_to_desc);/** +* Convert a GPIO name to its descriptor +*/ +struct gpio_desc *gpio_name_to_desc (const char *name) +{ +struct gpio_chip *chip; +unsigned long flags; + +spin_lock_irqsave (&gpio_lock, flags); + +list_for_each_entry (chip, &gpio_chips, list) { +int i; + +for (i =0 ; i != chip->ngpio; ++i) { +struct gpio_desc *gpio = &chip->desc[i]; + +if (!gpio->name) +continue ; + +if (!strcmp (gpio->name, name)) { +spin_unlock_irqrestore (&gpio_lock, flags); +return gpio; + } + } + } + +spin_unlock_irqrestore (&gpio_lock, flags); + +return NULL ; +} +EXPORT_SYMBOL_GPL (gpio_name_to_desc); + +/** * Get the GPIO descriptor corresponding to the given hw number for this chip. */ struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip,
include/linux/gpio/consumer.h +7 -0
@@ -130,6 +130,7 @@ int gpiod_to_irq(const struct gpio_desc *desc);/* Convert between the old gpio_ and new gpiod_ interfaces */ struct gpio_desc *gpio_to_desc (unsigned gpio);int desc_to_gpio (const struct gpio_desc *desc); +struct gpio_desc *gpio_name_to_desc (const char *name);/* Child properties interface */ struct fwnode_handle ;
@@ -400,6 +401,12 @@ static inline struct gpio_desc *gpio_to_desc(unsigned gpio) {return ERR_PTR (-EINVAL ); } + +static inline struct gpio_desc *gpio_name_to_desc (const char *name) +{ +return ERR_PTR (-EINVAL ); +} +static inline int desc_to_gpio (const struct gpio_desc *desc) {/* GPIO can never have been requested */
Signature · 1 line
2.1.4
PATCH 6/9 gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name
In reply to Markus PargmannThe DT describes gpios with subnodes. These subnodes can contain a property 'line-name'. This information can be useful in other areas where we want to identify a GPIO by its name.
This patch stores the line-name in the gpio descriptor.
--- drivers/gpio/gpiolib-of.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)
drivers/gpio/gpiolib-of.c +20 -9
@@ -184,11 +184,6 @@ static struct gpio_desc *of_parse_gpio(struct device_node *np, *dflags |=GPIOD_OUT_LOW ; else if (of_property_read_bool(np, "output-high")) *dflags |=GPIOD_OUT_HIGH ; - else { -pr_warn ("GPIO line %d (%s): no hogging state specified, bailing out\n" , -desc_to_gpio (gg_data.out_gpio), np->name); -return ERR_PTR (-EINVAL ); - }if (name &&of_property_read_string (np,"line-name" , name)) *name = np->name;
@@ -214,15 +209,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)for_each_child_of_node (chip->of_node, np) {const char *name =NULL ; -if (!of_property_read_bool (np,"gpio-hog" )) -continue ; +struct gpio_desc *name_desc; desc =of_parse_gpio (np, &name, &lflags, &dflags);if (IS_ERR (desc))continue ; -if (gpiod_hog (desc, name, lflags, dflags)) -continue ; +if (name) { + name_desc =gpio_name_to_desc (name); +if (name_desc) +pr_err ("GPIO name collision for '%s' detected at GPIO line %d (%s)\n" , + name,desc_to_gpio (desc), np->name); +else + desc->name = name; + } + +if (of_property_read_bool (np,"gpio-hog" )) { +if (!dflags) { +pr_warn ("GPIO line %d (%s): no hogging state specified, bailing out\n" , +desc_to_gpio (desc), np->name); +continue ; + } + +if (gpiod_hog (desc, name, lflags, dflags)) +continue ; + } } }
Signature · 1 line
2.1.4
PATCH 6/9 gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name
Quoted 38 lines · Markus Pargmann
The DT describes gpios with subnodes. These subnodes can contain a property 'line-name'. This information can be useful in other areas where we want to identify a GPIO by its name.
This patch stores the line-name in the gpio descriptor.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib-of.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)drivers/gpio/gpiolib-of.c +1
-7@@ -184,11 +184,6 @@ static struct gpio_desc *of_parse_gpio(struct device_node *np, *dflags |=GPIOD_OUT_LOW ; else if (of_property_read_bool(np, "output-high")) *dflags |=GPIOD_OUT_HIGH ; - else { -pr_warn ("GPIO line %d (%s): no hogging state specified, bailing out\n" , -desc_to_gpio (gg_data.out_gpio), np->name); -return ERR_PTR (-EINVAL ); - }if (name &&of_property_read_string (np,"line-name" , name)) *name = np->name;@@ -214,15 +209,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)for_each_child_of_node (chip->of_node, np) {const char *name =NULL ; -if (!of_property_read_bool (np,"gpio-hog" )) -continue ; +struct gpio_desc *name_desc; desc =of_parse_gpio (np, &name, &lflags, &dflags);if (IS_ERR (desc))continue ;
drivers/gpio/gpiolib-of.c static void of_gpiochip_scan_gpios(struct gpio_chip *chip) · line 224
Quoted 10 lines
drivers/gpio/gpiolib-of.c +7
-2-if (gpiod_hog (desc, name, lflags, dflags)) -continue ; +if (name) { + name_desc =gpio_name_to_desc (name); +if (name_desc) +pr_err ("GPIO name collision for '%s' detected at GPIO line %d (%s)\n" , + name,desc_to_gpio (desc), np->name); +else + desc->name = name;
This will introduce a regression. As I mentioned in a comment on an earlier patch, of_get_gpio_hog falls back to using the node name and those need not be unique.
Johan
PATCH 6/9 gpiolib-of: Reuse 'line-name' from DT as gpio descriptor name
Quoted 40 lines · Johan Hovold
Quoted 39 lines · Markus Pargmann
The DT describes gpios with subnodes. These subnodes can contain a property 'line-name'. This information can be useful in other areas where we want to identify a GPIO by its name.
This patch stores the line-name in the gpio descriptor.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib-of.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)drivers/gpio/gpiolib-of.c +1
-7@@ -184,11 +184,6 @@ static struct gpio_desc *of_parse_gpio(struct device_node *np, *dflags |=GPIOD_OUT_LOW ; else if (of_property_read_bool(np, "output-high")) *dflags |=GPIOD_OUT_HIGH ; - else { -pr_warn ("GPIO line %d (%s): no hogging state specified, bailing out\n" , -desc_to_gpio (gg_data.out_gpio), np->name); -return ERR_PTR (-EINVAL ); - }if (name &&of_property_read_string (np,"line-name" , name)) *name = np->name;@@ -214,15 +209,31 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)for_each_child_of_node (chip->of_node, np) {const char *name =NULL ; -if (!of_property_read_bool (np,"gpio-hog" )) -continue ; +struct gpio_desc *name_desc; desc =of_parse_gpio (np, &name, &lflags, &dflags);if (IS_ERR (desc))continue ;
Quoted 13 lines
drivers/gpio/gpiolib-of.c static void of_gpiochip_scan_gpios(struct gpio_chip *chip) · line 224
Quoted 9 lines
drivers/gpio/gpiolib-of.c +7
-2-if (gpiod_hog (desc, name, lflags, dflags)) -continue ; +if (name) { + name_desc =gpio_name_to_desc (name); +if (name_desc) +pr_err ("GPIO name collision for '%s' detected at GPIO line %d (%s)\n" , + name,desc_to_gpio (desc), np->name); +else + desc->name = name;This will introduce a regression. As I mentioned in a comment on an earlier patch,
of_get_gpio_hogfalls back to using the node name and those need not be unique.
Yes, will fix this as well.
Thanks,
Markus
Quoted 3 lines
Johan
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/28df365e/attachment-0001.sig>
PATCH 7/9 gpiolib-sysfs: Add gpio name parsing for sysfs export
In reply to Markus Pargmann--- drivers/gpio/gpiolib-sysfs.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-)
drivers/gpio/gpiolib-sysfs.c +28 -13
@@ -443,18 +443,25 @@ static ssize_t export_store(struct class *class, const char *buf, size_t len) { long gpio; -struct gpio_desc *desc; +struct gpio_desc *desc =NULL ;int status; status =kstrtol (buf,0 , &gpio); -if (status <0 ) - goto done; +if (!status) + desc =gpio_to_desc (gpio); - desc =gpio_to_desc (gpio); -/* reject invalid GPIOs */ +/* Fall back on detection by name */ if (!desc) { -pr_warn ("%s: invalid GPIO %ld\n" , __func__, gpio); -return -EINVAL ; +char *gpio_name =kstrdup (buf,GFP_KERNEL ); + + desc =gpio_name_to_desc (strim (gpio_name)); +kfree (gpio_name); + +/* reject invalid GPIOs */ +if (!desc) { +pr_warn ("%s: invalid GPIO %s\n" , __func__, buf); +return -EINVAL ; + } }/* No extra locking here; FLAG_SYSFS just signifies that the
@@ -485,17 +492,25 @@ static ssize_t unexport_store(struct class *class, const char *buf, size_t len) { long gpio; -struct gpio_desc *desc; +struct gpio_desc *desc =NULL ;int status; status =kstrtol (buf,0 , &gpio); -if (status <0 ) - goto done; +if (!status) + desc =gpio_to_desc (gpio); + +/* Fall back on detection by name */ +if (!desc) { +char *gpio_name =kstrdup (buf,GFP_KERNEL ); + + desc =gpio_name_to_desc (strim (gpio_name)); +kfree (gpio_name); + } + - desc =gpio_to_desc (gpio);/* reject bogus commands (gpio_unexport ignores them) */ if (!desc) { -pr_warn ("%s: invalid GPIO %ld\n" , __func__, gpio); +pr_warn ("%s: invalid GPIO %s\n" , __func__, buf);return -EINVAL ; }
@@ -509,7 +524,7 @@ static ssize_t unexport_store(struct class *class, status =0 ;gpiod_free (desc); } -done: +if (status)pr_debug ("%s: status %d\n" , __func__, status);return status ? : len;
Signature · 1 line
2.1.4
PATCH 7/9 gpiolib-sysfs: Add gpio name parsing for sysfs export
Quoted 20 lines · Markus Pargmann
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib-sysfs.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-)drivers/gpio/gpiolib-sysfs.c +1
-3@@ -443,18 +443,25 @@ static ssize_t export_store(struct class *class, const char *buf, size_t len) { long gpio; -struct gpio_desc *desc; +struct gpio_desc *desc =NULL ;int status; status =kstrtol (buf,0 , &gpio); -if (status <0 ) - goto done;
drivers/gpio/gpiolib-sysfs.c static ssize_t export_store(struct class *class, · line 455
Quoted 10 lines
drivers/gpio/gpiolib-sysfs.c +4
-4+if (!status) + desc =gpio_to_desc (gpio); - desc =gpio_to_desc (gpio); -/* reject invalid GPIOs */ +/* Fall back on detection by name */ if (!desc) { -pr_warn ("%s: invalid GPIO %ld\n" , __func__, gpio); -return -EINVAL ; +char *gpio_name =kstrdup (buf,GFP_KERNEL );
Must check for allocation failures, but why use kstrdup at all?
Quoted 19 lines
drivers/gpio/gpiolib-sysfs.c +10
-1+ + desc =gpio_name_to_desc (strim (gpio_name)); +kfree (gpio_name); + +/* reject invalid GPIOs */ +if (!desc) { +pr_warn ("%s: invalid GPIO %s\n" , __func__, buf); +return -EINVAL ; + } }/* No extra locking here; FLAG_SYSFS just signifies that the @@ -485,17 +492,25 @@ static ssize_t unexport_store(struct class *class, const char *buf, size_t len) { long gpio; -struct gpio_desc *desc; +struct gpio_desc *desc =NULL ;int status;
drivers/gpio/gpiolib-sysfs.c static ssize_t unexport_store(struct class *class, · line 504
Quoted 10 lines
drivers/gpio/gpiolib-sysfs.c +6
-2status =kstrtol (buf,0 , &gpio); -if (status <0 ) - goto done; +if (!status) + desc =gpio_to_desc (gpio); + +/* Fall back on detection by name */ +if (!desc) { +char *gpio_name =kstrdup (buf,GFP_KERNEL );
Same here.
drivers/gpio/gpiolib-sysfs.c static ssize_t unexport_store(struct class *class, · line 509
Quoted 5 lines
drivers/gpio/gpiolib-sysfs.c +5
-0+ + desc =gpio_name_to_desc (strim (gpio_name)); +kfree (gpio_name); + } +
Random whitespace change.
drivers/gpio/gpiolib-sysfs.c static ssize_t unexport_store(struct class *class, · line 515
Quoted 8 lines
drivers/gpio/gpiolib-sysfs.c +1
-2- desc =gpio_to_desc (gpio);/* reject bogus commands (gpio_unexport ignores them) */ if (!desc) { -pr_warn ("%s: invalid GPIO %ld\n" , __func__, gpio); +pr_warn ("%s: invalid GPIO %s\n" , __func__, buf);return -EINVAL ; }
Johan
PATCH 7/9 gpiolib-sysfs: Add gpio name parsing for sysfs export
Quoted 24 lines · Johan Hovold
Quoted 23 lines · Markus Pargmann
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib-sysfs.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-)drivers/gpio/gpiolib-sysfs.c +3
-3@@ -443,18 +443,25 @@ static ssize_t export_store(struct class *class, const char *buf, size_t len) { long gpio; -struct gpio_desc *desc; +struct gpio_desc *desc =NULL ;int status; status =kstrtol (buf,0 , &gpio); -if (status <0 ) - goto done; +if (!status) + desc =gpio_to_desc (gpio);
Quoted 9 lines
drivers/gpio/gpiolib-sysfs.c static ssize_t export_store(struct class *class, · line 455
Quoted 7 lines
drivers/gpio/gpiolib-sysfs.c +2
-4- desc =gpio_to_desc (gpio); -/* reject invalid GPIOs */ +/* Fall back on detection by name */ if (!desc) { -pr_warn ("%s: invalid GPIO %ld\n" , __func__, gpio); -return -EINVAL ; +char *gpio_name =kstrdup (buf,GFP_KERNEL );Must check for allocation failures, but why use kstrdup at all?
Yes have to check that.strim() may modify the buf variable which is constant.
Best regards,
Markus
Quoted 52 lines
Quoted 20 lines
drivers/gpio/gpiolib-sysfs.c +10
-1+ + desc =gpio_name_to_desc (strim (gpio_name)); +kfree (gpio_name); + +/* reject invalid GPIOs */ +if (!desc) { +pr_warn ("%s: invalid GPIO %s\n" , __func__, buf); +return -EINVAL ; + } }/* No extra locking here; FLAG_SYSFS just signifies that the @@ -485,17 +492,25 @@ static ssize_t unexport_store(struct class *class, const char *buf, size_t len) { long gpio; -struct gpio_desc *desc; +struct gpio_desc *desc =NULL ;int status;drivers/gpio/gpiolib-sysfs.c static ssize_t unexport_store(struct class *class, · line 504
Quoted 10 lines
drivers/gpio/gpiolib-sysfs.c +6
-2status =kstrtol (buf,0 , &gpio); -if (status <0 ) - goto done; +if (!status) + desc =gpio_to_desc (gpio); + +/* Fall back on detection by name */ +if (!desc) { +char *gpio_name =kstrdup (buf,GFP_KERNEL );Same here.
drivers/gpio/gpiolib-sysfs.c static ssize_t unexport_store(struct class *class, · line 509
Quoted 5 lines
drivers/gpio/gpiolib-sysfs.c +5
-0+ + desc =gpio_name_to_desc (strim (gpio_name)); +kfree (gpio_name); + } +Random whitespace change.
drivers/gpio/gpiolib-sysfs.c static ssize_t unexport_store(struct class *class, · line 515
Quoted 8 lines
drivers/gpio/gpiolib-sysfs.c +1
-2- desc =gpio_to_desc (gpio);/* reject bogus commands (gpio_unexport ignores them) */ if (!desc) { -pr_warn ("%s: invalid GPIO %ld\n" , __func__, gpio); +pr_warn ("%s: invalid GPIO %s\n" , __func__, buf);return -EINVAL ; }Johan
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/d8e01ab4/attachment-0001.sig>
PATCH 7/9 gpiolib-sysfs: Add gpio name parsing for sysfs export
Quoted 7 lines · Markus Pargmann
Quoted 5 lines · Johan Hovold
Review
Quoted 1 line · Markus Pargmann
(quoted diff) +1
-0+ char *gpio_name = kstrdup(buf, GFP_KERNEL);Must check for allocation failures, but why use kstrdup at all?
Quoted 5 lines
Yes have to check that.
strim()may modify the buf variable which is constant.Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + desc = gpio_name_to_desc(strim(gpio_name));
Ah, I missed that strim.
Johan
PATCH 8/9 gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name
In reply to Markus PargmannThis patch adds a sysfs attribute 'name' to gpios that were exported. It exposes the newly added name property of gpio descriptors.
--- drivers/gpio/gpiolib-sysfs.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
drivers/gpio/gpiolib-sysfs.c +20 -0
@@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev, }static DEVICE_ATTR_RW (value); +static ssize_t gpio_name_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct gpio_desc *desc =dev_get_drvdata (dev); +ssize_t status; + +mutex_lock (&sysfs_lock); + +if (!test_bit (FLAG_EXPORT , &desc->flags)) + status = -EIO ; +else + status =sprintf (buf,"%s\n" , desc->name); + +mutex_unlock (&sysfs_lock); +return status; +} + +static DEVICE_ATTR(name, 0444, gpio_name_show, NULL); +static irqreturn_t gpio_sysfs_irq (int irq,void *priv) {struct gpiod_data *data = priv;
@@ -377,6 +396,7 @@ static struct attribute *gpio_attrs[] = { &dev_attr_edge.attr, &dev_attr_value.attr, &dev_attr_active_low.attr, + &dev_attr_name.attr, NULL, };
Signature · 1 line
2.1.4
PATCH 8/9 gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name
Quoted 15 lines · Markus Pargmann
This patch adds a sysfs attribute 'name' to gpios that were exported. It exposes the newly added name property of gpio descriptors.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib-sysfs.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)drivers/gpio/gpiolib-sysfs.c +0
-0@@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev, }static DEVICE_ATTR_RW (value);
drivers/gpio/gpiolib-sysfs.c static ssize_t value_store(struct device *dev, · line 150
Quoted 10 lines
drivers/gpio/gpiolib-sysfs.c +9
-0+static ssize_t gpio_name_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct gpio_desc *desc =dev_get_drvdata (dev); +ssize_t status; + +mutex_lock (&sysfs_lock); + +if (!test_bit (FLAG_EXPORT , &desc->flags))
No need to check flag export, and no need for the mutex either.
drivers/gpio/gpiolib-sysfs.c static ssize_t value_store(struct device *dev, · line 153
Quoted 3 lines
drivers/gpio/gpiolib-sysfs.c +3
-0+ status = -EIO ; + else + status =sprintf (buf,"%s\n" , desc->name);
Depending on sprintfs representation of NULL is not a good idea here.
drivers/gpio/gpiolib-sysfs.c static ssize_t value_store(struct device *dev, · line 159
Quoted 6 lines
drivers/gpio/gpiolib-sysfs.c +6
-0+ +mutex_unlock (&sysfs_lock); +return status; +} + +static DEVICE_ATTR(name, 0444, gpio_name_show, NULL);
DEVICE_ATTR_RO
drivers/gpio/gpiolib-sysfs.c static struct attribute *gpio_attrs[] = { · line 401
Quoted 11 lines
drivers/gpio/gpiolib-sysfs.c +2
-0+static irqreturn_t gpio_sysfs_irq (int irq,void *priv) {struct gpiod_data *data = priv;@@ -377,6 +396,7 @@ static struct attribute *gpio_attrs[] = { &dev_attr_edge.attr, &dev_attr_value.attr, &dev_attr_active_low.attr, + &dev_attr_name.attr, NULL, };
Johan
PATCH 8/9 gpiolib-sysfs: Show gpio-name in /sys/class/gpio/gpio*/name
Quoted 44 lines · Johan Hovold
Quoted 15 lines · Markus Pargmann
This patch adds a sysfs attribute 'name' to gpios that were exported. It exposes the newly added name property of gpio descriptors.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib-sysfs.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)drivers/gpio/gpiolib-sysfs.c +0
-0@@ -139,6 +139,25 @@ static ssize_t value_store(struct device *dev, }static DEVICE_ATTR_RW (value);drivers/gpio/gpiolib-sysfs.c static ssize_t value_store(struct device *dev, · line 150
Quoted 10 lines
drivers/gpio/gpiolib-sysfs.c +9
-0+static ssize_t gpio_name_show (struct device *dev, +struct device_attribute *attr,char *buf) +{ +struct gpio_desc *desc =dev_get_drvdata (dev); +ssize_t status; + +mutex_lock (&sysfs_lock); + +if (!test_bit (FLAG_EXPORT , &desc->flags))No need to check flag export, and no need for the mutex either.
drivers/gpio/gpiolib-sysfs.c static ssize_t value_store(struct device *dev, · line 153
Quoted 3 lines
drivers/gpio/gpiolib-sysfs.c +3
-0+ status = -EIO ; + else + status =sprintf (buf,"%s\n" , desc->name);Depending on sprintfs representation of NULL is not a good idea here.
drivers/gpio/gpiolib-sysfs.c static ssize_t value_store(struct device *dev, · line 159
Quoted 6 lines
drivers/gpio/gpiolib-sysfs.c +6
-0+ +mutex_unlock (&sysfs_lock); +return status; +} + +static DEVICE_ATTR(name, 0444, gpio_name_show, NULL);
DEVICE_ATTR_RO
Quoted 11 lines
drivers/gpio/gpiolib-sysfs.c static struct attribute *gpio_attrs[] = { · line 401
Quoted 11 lines
drivers/gpio/gpiolib-sysfs.c +2
-0+static irqreturn_t gpio_sysfs_irq (int irq,void *priv) {struct gpiod_data *data = priv;@@ -377,6 +396,7 @@ static struct attribute *gpio_attrs[] = { &dev_attr_edge.attr, &dev_attr_value.attr, &dev_attr_active_low.attr, + &dev_attr_name.attr, NULL, };
Thanks, will fix all of them.
Best regards,
Markus
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/f7122bb0/attachment-0001.sig>
PATCH 9/9 gpiolib: Add gpio name information to /sys/kernel/debug/gpio
In reply to Markus PargmannAdd some information about gpio names to the debugfs gpio file. name and label of a GPIO are then displayed next to each other. This way it is easy to see what the real name of GPIO is and what the driver requested it for.
--- drivers/gpio/gpiolib.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/gpio/gpiolib.c +8 -3
@@ -2308,14 +2308,19 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)int is_irq;for (i =0 ; i < chip->ngpio; i++, gpio++, gdesc++) { -if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) +if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) { +if (gdesc->name) { +seq_printf (s," gpio-%-3d (%-20.20s)\n" , + gpio, gdesc->name); + }continue ; + }gpiod_get_direction (gdesc); is_out =test_bit (FLAG_IS_OUT , &gdesc->flags); is_irq =test_bit (FLAG_USED_AS_IRQ , &gdesc->flags); - seq_printf(s, " gpio-%-3d (%-20.20s) %s %s %s", - gpio, gdesc->label, + seq_printf(s, " gpio-%-3d (%-20.20s|%-20.20s) %s %s %s", + gpio, gdesc->name ? gdesc->name : "", gdesc->label, is_out ? "out" : "in ", chip->get ? (chip->get(chip, i) ? "hi" : "lo")
Signature · 1 line
2.1.4
PATCH 9/9 gpiolib: Add gpio name information to /sys/kernel/debug/gpio
Quoted 15 lines · Markus Pargmann
Add some information about gpio names to the debugfs gpio file. name and label of a GPIO are then displayed next to each other. This way it is easy to see what the real name of GPIO is and what the driver requested it for.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/gpio/gpiolib.c +0
-0
drivers/gpio/gpiolib.c static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) · line 2315
Quoted 9 lines
drivers/gpio/gpiolib.c +5
-1@@ -2308,14 +2308,19 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)int is_irq;for (i =0 ; i < chip->ngpio; i++, gpio++, gdesc++) { -if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) +if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) { +if (gdesc->name) { +seq_printf (s," gpio-%-3d (%-20.20s)\n" , + gpio, gdesc->name); + }
So now we'd no longer just be listing requested gpios, but on a similar format to how requested ones used to be represented.
Then there's the debugfs as ABI discussion...
Johan
PATCH 9/9 gpiolib: Add gpio name information to /sys/kernel/debug/gpio
Quoted 18 lines · Johan Hovold
Quoted 17 lines · Markus Pargmann
Add some information about gpio names to the debugfs gpio file. name and label of a GPIO are then displayed next to each other. This way it is easy to see what the real name of GPIO is and what the driver requested it for.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/gpio/gpiolib.c +0
-0@@ -2308,14 +2308,19 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)int is_irq;
Quoted 10 lines
drivers/gpio/gpiolib.c static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) · line 2315
Quoted 7 lines
drivers/gpio/gpiolib.c +5
-1for (i =0 ; i < chip->ngpio; i++, gpio++, gdesc++) { -if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) +if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) { +if (gdesc->name) { +seq_printf (s," gpio-%-3d (%-20.20s)\n" , + gpio, gdesc->name); + }So now we'd no longer just be listing requested gpios, but on a similar format to how requested ones used to be represented.
Better suggestions on how to display those extra information in debugfs?
Quoted 2 lines
Then there's the debugfs as ABI discussion...
I didn't consider debugfs as ABI as I thought it is just for debugging purposes?
Best regards,
Markus
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150729/f78dab89/attachment.sig>
PATCH 9/9 gpiolib: Add gpio name information to /sys/kernel/debug/gpio
Quoted 30 lines · Markus Pargmann
Quoted 18 lines · Johan Hovold
Quoted 17 lines · Markus Pargmann
Add some information about gpio names to the debugfs gpio file. name and label of a GPIO are then displayed next to each other. This way it is easy to see what the real name of GPIO is and what the driver requested it for.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/gpio/gpiolib.c +0
-0@@ -2308,14 +2308,19 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)int is_irq;Quoted 10 lines
drivers/gpio/gpiolib.c static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) · line 2315
Quoted 7 lines
drivers/gpio/gpiolib.c +5
-1for (i =0 ; i < chip->ngpio; i++, gpio++, gdesc++) { -if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) +if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) { +if (gdesc->name) { +seq_printf (s," gpio-%-3d (%-20.20s)\n" , + gpio, gdesc->name); + }So now we'd no longer just be listing requested gpios, but on a similar format to how requested ones used to be represented.
Quoted 1 line
Better suggestions on how to display those extra information in debugfs?
Perhaps a new file only listing the line names.
Quoted 4 lines
Quoted 1 line
Then there's the debugfs as ABI discussion...
I didn't consider debugfs as ABI as I thought it is just for debugging purposes?
Some people seem to have expressed a different position:
"The fact that something is documented (whether correctly or not) has absolutely _zero_ impact on anything at all. What makes something an ABI is that it's useful and available. The only way something isn't an ABI is by _explicitly_ making sure that it's not available even by mistake in a stable form for binary use.
Example: kernel internal data structures and function calls. We make sure that you simply _cannot_ make a binary that works across kernel versions. That is the only way for an ABI to not form."
https://lwn.net/Articles/309298/
Johan
PATCH 9/9 gpiolib: Add gpio name information to /sys/kernel/debug/gpio
Quoted 31 lines · Johan Hovold
Quoted 30 lines · Markus Pargmann
Quoted 28 lines · Johan Hovold
Quoted 15 lines · Markus Pargmann
Add some information about gpio names to the debugfs gpio file. name and label of a GPIO are then displayed next to each other. This way it is easy to see what the real name of GPIO is and what the driver requested it for.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/gpio/gpiolib.c +0
-0drivers/gpio/gpiolib.c static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) · line 2315
Quoted 9 lines
drivers/gpio/gpiolib.c +5
-1@@ -2308,14 +2308,19 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)int is_irq;for (i =0 ; i < chip->ngpio; i++, gpio++, gdesc++) { -if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) +if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) { +if (gdesc->name) { +seq_printf (s," gpio-%-3d (%-20.20s)\n" , + gpio, gdesc->name); + }So now we'd no longer just be listing requested gpios, but on a similar format to how requested ones used to be represented.
Quoted 3 lines
Quoted 1 line
Better suggestions on how to display those extra information in debugfs?
Perhaps a new file only listing the line names.
Yes that's a possibility. On the other side having it all in one place is nice. As it helps you to debug faster, you can directly see which line name was requested for what purpose and so on.
Quoted 14 lines
Quoted 5 lines
Quoted 2 lines
Then there's the debugfs as ABI discussion...
I didn't consider debugfs as ABI as I thought it is just for debugging purposes?
Some people seem to have expressed a different position:
"The fact that something is documented (whether correctly or not) has absolutely _zero_ impact on anything at all. What makes something an ABI is that it's useful and available. The only way something isn't an ABI is by _explicitly_ making sure that it's not available even by mistake in a stable form for binary use.
Quoted 7 lines
Example: kernel internal data structures and function calls. We make sure that you simply _cannot_ make a binary that works across kernel versions. That is the only way for an ABI to not form."
Oh, I see. But I think that debugfs should help us to debug issues. If we start to care about breaking userspace tools it is not as helpful anymore. If I develop some tool against information that are clearly marked for debugging, I would somehow expect that my tool breaks at some point. But thats just my opinion.
Best regards,
Markus
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150731/8596c7fb/attachment-0001.sig>
PATCH 9/9 gpiolib: Add gpio name information to /sys/kernel/debug/gpio
Quoted 32 lines · Markus Pargmann
Quoted 31 lines · Johan Hovold
Quoted 30 lines · Markus Pargmann
Quoted 28 lines · Johan Hovold
Quoted 15 lines · Markus Pargmann
Add some information about gpio names to the debugfs gpio file. name and label of a GPIO are then displayed next to each other. This way it is easy to see what the real name of GPIO is and what the driver requested it for.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/gpio/gpiolib.c +0
-0drivers/gpio/gpiolib.c static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) · line 2315
Quoted 9 lines
drivers/gpio/gpiolib.c +5
-1@@ -2308,14 +2308,19 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)int is_irq;for (i =0 ; i < chip->ngpio; i++, gpio++, gdesc++) { -if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) +if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) { +if (gdesc->name) { +seq_printf (s," gpio-%-3d (%-20.20s)\n" , + gpio, gdesc->name); + }So now we'd no longer just be listing requested gpios, but on a similar format to how requested ones used to be represented.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Better suggestions on how to display those extra information in debugfs?
Perhaps a new file only listing the line names.
Yes that's a possibility. On the other side having it all in one place is nice. As it helps you to debug faster, you can directly see which line name was requested for what purpose and so on.
Certainly, but you'd change the meaning of debug/gpio which used to contain only requested gpios.
Not saying we shouldn't, but yeah, let's were this discussion goes first.
Quoted 18 lines
Quoted 18 lines
Quoted 4 lines
Quoted 1 line
Then there's the debugfs as ABI discussion...
I didn't consider debugfs as ABI as I thought it is just for debugging purposes?
Some people seem to have expressed a different position:
"The fact that something is documented (whether correctly or not) has absolutely _zero_ impact on anything at all. What makes something an ABI is that it's useful and available. The only way something isn't an ABI is by _explicitly_ making sure that it's not available even by mistake in a stable form for binary use.
Example: kernel internal data structures and function calls. We make sure that you simply _cannot_ make a binary that works across kernel versions. That is the only way for an ABI to not form."
Quoted 8 lines
Quoted 2 lines
https://lwn.net/Articles/309298/Oh, I see. But I think that debugfs should help us to debug issues. If we start to care about breaking userspace tools it is not as helpful anymore. If I develop some tool against information that are clearly marked for debugging, I would somehow expect that my tool breaks at some point. But thats just my opinion.
I tend to agree, and wish debugfs was never to be considered ABI, but it's Torvald's words above...
Johan
PATCH 9/9 gpiolib: Add gpio name information to /sys/kernel/debug/gpio
In reply to Johan HovoldAm Freitag, den 31.07.2015, 10:54 +0200 schrieb Johan Hovold:
Quoted 48 lines · Markus Pargmann
Quoted 30 lines · Johan Hovold
Quoted 18 lines · Markus Pargmann
Quoted 17 lines
Add some information about gpio names to the debugfs gpio file. name and label of a GPIO are then displayed next to each other. This way it is easy to see what the real name of GPIO is and what the driver requested it for.
Signed-off-by Markus Pargmann <[email protected]>--- drivers/gpio/gpiolib.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/gpio/gpiolib.c +0
-0@@ -2308,14 +2308,19 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip)int is_irq;Quoted 10 lines
drivers/gpio/gpiolib.c static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) · line 2315
Quoted 7 lines
drivers/gpio/gpiolib.c +5
-1for (i =0 ; i < chip->ngpio; i++, gpio++, gdesc++) { -if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) +if (!test_bit (FLAG_REQUESTED , &gdesc->flags)) { +if (gdesc->name) { +seq_printf (s," gpio-%-3d (%-20.20s)\n" , + gpio, gdesc->name); + }So now we'd no longer just be listing requested gpios, but on a similar format to how requested ones used to be represented.
Quoted 1 line
Better suggestions on how to display those extra information in debugfs?
Perhaps a new file only listing the line names.
Quoted 4 lines
Quoted 1 line
Then there's the debugfs as ABI discussion...
I didn't consider debugfs as ABI as I thought it is just for debugging purposes?
Some people seem to have expressed a different position:
"The fact that something is documented (whether correctly or not) has absolutely _zero_ impact on anything at all. What makes something an ABI is that it's useful and available. The only way something isn't an ABI is by _explicitly_ making sure that it's not available even by mistake in a stable form for binary use.
Quoted 6 lines
Example: kernel internal data structures and function calls. We make sure that you simply _cannot_ make a binary that works across kernel versions. That is the only way for an ABI to not form."
Assuming debugfs is ABI is completely backwards. debugfs makes no ABI guarantees, that's why stuff that needs a stable ABI like tracing is actively moving out of debugfs into other places, where ABI stability can be assumed.
Regards,
Lucas
Signature · 2 lines
Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ |
cover gpiolib: Add GPIO name support
In reply to Markus PargmannQuoted 19 lines · Markus Pargmann
This is a proposal to add GPIO names to the kernel based on devicetree descriptions.
This series adds GPIO name support. Until now it is only possible to use names for already requested GPIOs (for example what they are used for). It is not possible to identify GPIOs by a name although most of them have a name for example in the schematics of the board. This makes it difficult to identify a specific GPIO from userspace.
As the GPIO name information is a hardware description this series uses the devicetree bindings introduced by the GPIO hogging mechanism, specifically 'line-name', to identify GPIOs. The sysfs 'export' file is changed to accept names as fallback. The gpio numbers still have a higher priority to ensure backwards compatibility.
Exported GPIOs are still using their number as directory name (gpio<ID>). But the directories now contain a 'name' file which is '(null)' for non-existent names and the name otherwise.
Quoted 7 lines
This series can be used to have an easy name mapping for udev with a quite simple rule similar to this:
SUBSYSTEM=="gpio", KERNEL=="gpio*", ATTR{name}!="(null)", ACTION=="add", \ PROGRAM+="/bin/sh -c 'mkdir -p /dev/gpios; rm -f /dev/gpios/$attr{name}; ln -s /sys%p/ /dev/gpios/$attr{name}"With this rule udev adds a link for each exported GPIO with a name into
/dev/gpios/. This way it is not necessary to know the number of a GPIO to use it.
I must say I like this series.
Because it makes the GPIO sysfs ABI *less* insane. Especially I like the patch that makes a distinction between "label" (use) and "name" (the name of the actual line). That illustrates clearly that this is thought-through.
However I still have some doubts as it will expand the sysfs ABI, and we have discussed a char device for GPIO chips as a better alternative, for example since these can do get/set multiple GPIOs with a single context switch (ioctl), whereas sysfs is "one value per file" and would be able to expose some flags better.
Still this is very tempting. Adding some more people to To: to get some opinions.
Yours,
Linus Walleij
cover gpiolib: Add GPIO name support
Quoted 36 lines · Linus Walleij
Quoted 19 lines · Markus Pargmann
This is a proposal to add GPIO names to the kernel based on devicetree descriptions.
This series adds GPIO name support. Until now it is only possible to use names for already requested GPIOs (for example what they are used for). It is not possible to identify GPIOs by a name although most of them have a name for example in the schematics of the board. This makes it difficult to identify a specific GPIO from userspace.
As the GPIO name information is a hardware description this series uses the devicetree bindings introduced by the GPIO hogging mechanism, specifically 'line-name', to identify GPIOs. The sysfs 'export' file is changed to accept names as fallback. The gpio numbers still have a higher priority to ensure backwards compatibility.
Exported GPIOs are still using their number as directory name (gpio<ID>). But the directories now contain a 'name' file which is '(null)' for non-existent names and the name otherwise.
Quoted 7 lines
This series can be used to have an easy name mapping for udev with a quite simple rule similar to this:
SUBSYSTEM=="gpio", KERNEL=="gpio*", ATTR{name}!="(null)", ACTION=="add", \ PROGRAM+="/bin/sh -c 'mkdir -p /dev/gpios; rm -f /dev/gpios/$attr{name}; ln -s /sys%p/ /dev/gpios/$attr{name}"With this rule udev adds a link for each exported GPIO with a name into
/dev/gpios/. This way it is not necessary to know the number of a GPIO to use it.I must say I like this series.
Because it makes the GPIO sysfs ABI *less* insane. Especially I like the patch that makes a distinction between "label" (use) and "name" (the name of the actual line). That illustrates clearly that this is thought-through.
Quoted 5 lines
However I still have some doubts as it will expand the sysfs ABI, and we have discussed a char device for GPIO chips as a better alternative, for example since these can do get/set multiple GPIOs with a single context switch (ioctl), whereas sysfs is "one value per file" and would be able to expose some flags better.
Even if we introduce a new interface (and I don't see traction towards this yet), sysfs is just too convenient for 90% of the cases to leave it borderline-unusable as it is currently. The obligation to use global numbers that potentially change every boot is getting us complaints every week or so - this series sounds like a good way to reduce our inbox volume, and if only for that, I encourage it being pursued. :)
It will also not get in the way of a new user-space interface, so why refrain from these extra ~90 lines of code?
Alex.
cover gpiolib: Add GPIO name support
Hello,
Quoted 2 lines · Alexandre Courbot
Quoted 1 line · Linus Walleij
I must say I like this series.
I think apart from patch 3 which I'd like to have discussed a bit more, I like it, too. I didn't try, but the series should work fine without patch 3 such that this patch can be discussed separately and the remaining 8 patches can go in?
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
cover gpiolib: Add GPIO name support
Hi,
Quoted 9 lines · Uwe Kleine-K?nig
Hello,
Quoted 2 lines · Alexandre Courbot
Quoted 1 line · Linus Walleij
I must say I like this series.
I think apart from patch 3 which I'd like to have discussed a bit more, I like it, too. I didn't try, but the series should work fine without patch 3 such that this patch can be discussed separately and the remaining 8 patches can go in?
This series should work without patch 3, yes. Just remove the hogging keyword from the gpios defined in the devicetree.
However some test coverage for this series would be good.
Best regards,
Markus
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150721/93f5e420/attachment.sig>
A message that is not in the archive (sent off-list or not yet received): CAGmoSHt0Kg-cxe3U6uV40=ttmFbDruRcJZNxtmSZ=gmZQN5fTw@mail.gmail.com
cover gpiolib: Add GPIO name support
Quoted 4 lines · Arun Bharadwaj
2) Some times I get a kernel dump when I try to unexport the gpio or even when I try to read the "name" file. This is not very repeatable and seems to happen at random. I haven't had the time to look into this in more detail yet, but here's the dump FWIW: http://pastebin.com/xq9cpCQE
This is probably because of patch 8/9 erroneously taking the sysfs_lock in gpio_name_show.
Johan
cover gpiolib: Add GPIO name support
Quoted 9 lines · Johan Hovold
Quoted 4 lines · Arun Bharadwaj
2) Some times I get a kernel dump when I try to unexport the gpio or even when I try to read the "name" file. This is not very repeatable and seems to happen at random. I haven't had the time to look into this in more detail yet, but here's the dump FWIW: http://pastebin.com/xq9cpCQE
This is probably because of patch 8/9 erroneously taking the
sysfs_lockingpio_name_show.
Arun, thank you for testing. I will send a fixed series at the beginning of next week.
Best regards,
Markus
Signature · 11 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150731/409a0439/attachment.sig>
cover gpiolib: Add GPIO name support
In reply to Linus WalleijQuoted 36 lines · Linus Walleij
Quoted 19 lines · Markus Pargmann
This is a proposal to add GPIO names to the kernel based on devicetree descriptions.
This series adds GPIO name support. Until now it is only possible to use names for already requested GPIOs (for example what they are used for). It is not possible to identify GPIOs by a name although most of them have a name for example in the schematics of the board. This makes it difficult to identify a specific GPIO from userspace.
As the GPIO name information is a hardware description this series uses the devicetree bindings introduced by the GPIO hogging mechanism, specifically 'line-name', to identify GPIOs. The sysfs 'export' file is changed to accept names as fallback. The gpio numbers still have a higher priority to ensure backwards compatibility.
Exported GPIOs are still using their number as directory name (gpio<ID>). But the directories now contain a 'name' file which is '(null)' for non-existent names and the name otherwise.
Quoted 7 lines
This series can be used to have an easy name mapping for udev with a quite simple rule similar to this:
SUBSYSTEM=="gpio", KERNEL=="gpio*", ATTR{name}!="(null)", ACTION=="add", \ PROGRAM+="/bin/sh -c 'mkdir -p /dev/gpios; rm -f /dev/gpios/$attr{name}; ln -s /sys%p/ /dev/gpios/$attr{name}"With this rule udev adds a link for each exported GPIO with a name into
/dev/gpios/. This way it is not necessary to know the number of a GPIO to use it.I must say I like this series.
Because it makes the GPIO sysfs ABI *less* insane. Especially I like the patch that makes a distinction between "label" (use) and "name" (the name of the actual line). That illustrates clearly that this is thought-through.
Quoted 5 lines
However I still have some doubts as it will expand the sysfs ABI, and we have discussed a char device for GPIO chips as a better alternative, for example since these can do get/set multiple GPIOs with a single context switch (ioctl), whereas sysfs is "one value per file" and would be able to expose some flags better.
I'm also reluctant to band-aiding the sysfs interface with more ABI that we need to maintain forever. Making it easier to use also reduces the incentives to come up with a saner interface.
As I mentioned in a comment to patch 6/9, this series introduces an incompatibility with current DT since it no longer allows two hogs to use the same name, while the current hog implementation is documented to fall back to using the (not necessarily unique) node name when a line-name property isn't specified.
Even though this looks like it could be worked around, it's an example of the kind of issues we risk running into if we keep on incrementally patching this interface.
That said, this is a step along the lines that we have discussed in the past. Adding pin functions (line names) to generic pin nodes in DT makes sense, but we need to think through the details first. For example:
- Should we allow duplicate pin functions (line names)? We need to consider not just on-chip controllers, but also hot-pluggable controllers and device-tree overlays.
- Should we allow initial configurations to be specified and still allow (some) pins to be requested later ("soft" hogging)?
- Should we specify pin directionality? I've suggested elsewhere that adding such limitations (e.g. as a mask) may make sense in cases were changing pin direction is known to cause damage.
- What would a new gpio interface look like?
Johan
cover gpiolib: Add GPIO name support
Quoted 7 lines · Johan Hovold
That said, this is a step along the lines that we have discussed in the past. Adding pin functions (line names) to generic pin nodes in DT makes sense, but we need to think through the details first. For example:
- Should we allow duplicate pin functions (line names)? We need to consider not just on-chip controllers, but also hot-pluggable controllers and device-tree overlays.
I guess the typical example is if we have two identical GPIO expanders. We have this with arch/arm/boot/dts/ste-nomadik-nhk15.dts for example.
With this scheme, they will simply have to name their lines uniquely for things to work since /sys/class/gpio is a flat space, not hierarchic per-gpiochip (as would be preferred, should this be sane to maintain).
Quoted 2 lines
- Should we allow initial configurations to be specified and still allow (some) pins to be requested later ("soft" hogging)?
That is a key idea in this patch set.
Maybe we need a "userspace hog" to mark these specifically as not used by the operating system. (Bringing me back to my tirade about people doing stupid things in userspace just because they don't know about all the nice GPIO stuff in the kernel.)
Quoted 3 lines
- Should we specify pin directionality? I've suggested elsewhere that adding such limitations (e.g. as a mask) may make sense in cases were changing pin direction is known to cause damage.
We have the flag jamming IRQ lines to be input, but input-only and output-only seems reasonable for hogs in reaction to the current "input", "output-low" and "output-high" properties.
Could be a separate patch though.
Quoted 1 line
- What would a new gpio interface look like?
Yeah :/ people are scared of adding chardevs it seems.
I think they are kind of nice, the biggest problem seems to be totally confused userspace with udev and mdev and none of them ever knows about new stuff and people get problems becuase of old rootfs:es.
Yours,
Linus Walleij
cover gpiolib: Add GPIO name support
Quoted 10 lines · Linus Walleij
Quoted 7 lines · Johan Hovold
That said, this is a step along the lines that we have discussed in the past. Adding pin functions (line names) to generic pin nodes in DT makes sense, but we need to think through the details first. For example:
- Should we allow duplicate pin functions (line names)? We need to consider not just on-chip controllers, but also hot-pluggable controllers and device-tree overlays.
Quoted 8 lines
I guess the typical example is if we have two identical GPIO expanders. We have this with
arch/arm/boot/dts/ste-nomadik-nhk15.dtsfor example.With this scheme, they will simply have to name their lines uniquely for things to work since
/sys/class/gpiois a flat space, not hierarchic per-gpiochip (as would be preferred, should this be sane to maintain).
/sys/class/gpio and the pin numbers is a flat space, but the line names must not necessarily be unique. With a new userspace interface duplicate line names could be handled.
Consider also DT overlays and hot-pluggable buses (e.g. USB when it gets a DT representation). A driver for a device on such a bus, be it USB or Greybus, could even be providing line names through some other means when registering the controller.
Quoted 9 lines
Quoted 2 lines
- Should we allow initial configurations to be specified and still allow (some) pins to be requested later ("soft" hogging)?
That is a key idea in this patch set.
Maybe we need a "userspace hog" to mark these specifically as not used by the operating system. (Bringing me back to my tirade about people doing stupid things in userspace just because they don't know about all the nice GPIO stuff in the kernel.)
Should we then refuse such "userspace hogs" from being requested from the kernel? This can't really be considered hardware description...
Quoted 9 lines
Quoted 3 lines
- Should we specify pin directionality? I've suggested elsewhere that adding such limitations (e.g. as a mask) may make sense in cases were changing pin direction is known to cause damage.
We have the flag jamming IRQ lines to be input, but input-only and output-only seems reasonable for hogs in reaction to the current "input", "output-low" and "output-high" properties.
Could be a separate patch though.
Indeed. I'm just suggesting we not rush into adding more ABI, before really determining what these interfaces should look like.
Quoted 8 lines
Quoted 1 line
- What would a new gpio interface look like?
Yeah :/ people are scared of adding chardevs it seems.
I think they are kind of nice, the biggest problem seems to be totally confused userspace with udev and mdev and none of them ever knows about new stuff and people get problems becuase of old rootfs:es.
Really? But that's not our problem, right?
I keep rejecting vendor-specific gpio ioctl-interfaces for usb-serial devices, while the current sysfs interface really isn't a good replacement (but we must use standard interfaces of course). It's not just about the line names.
Johan