daniel-santos / mcp2210-linux

MCP2210 driver for linux
http://danielthesantos.blogspot.com/search/label/mcp2210
51 stars 31 forks source link

GPIO interrupts advertised to gpio subsystem but not implemented. #21

Open ToBeReplaced opened 8 years ago

ToBeReplaced commented 8 years ago

If I set config according to settings-example.h, I am able to export pin 255 (corresponding to 8), creating /sys/class/gpio/unused255. The direction is in, but there is no edge setting, and I am unable to select on the value file for POLLPRI events.

I recompiled and reinstalled the module with #define CONFIG_MCP2210_IRQ 1, then recompiled the user project with the final pin in settings-example.h set to have .has_irq = 1, .irq = 1, .irq_type = 1, but nothing changed.

Does this drive provide what I am looking for, and if so, how do I get there?

ToBeReplaced commented 8 years ago

If I drop .irq_type, then perform the above, I get an edge file set to none. Even if I set the edge to rising, no POLLPRI events are generated. I also tried setting poll_gpio_usecs and the like, to no effect.

daniel-santos commented 8 years ago

First off, my memory is a little hazy as to where I was with the IRQ support, but I think that you misunderstand the aim of this part of the driver. One might say that your misunderstanding stems from a complete lack of any sort of documentation or even code comments about it, but I say that it's just your shortcoming! :) -- Actually, maybe I should try to explain that somewhere.....

Due to the nature of USB being a strictly master/slave protocol, this we have to poll the device periodically to discover if anything changes. However, a peripheral may do a very fast pulse to signal that it needs servicing and we can easily miss that if our polling interval is greater than the peripheral's interrupt pulse width.

Therefore, the idea of the IRQ functionality was that the driver would exploit the MCP2210's own interrupt monitoring functionality which can catch a pulse of a very small width and the driver would poll the MCP2210 at some interval. The MCP2210 provides this functionality when its CS6 pin (pin 11 on QFN package or pin 14 on the SOIC/SSOP packages) is configured in its "dedicated" mode. Thus, we would only poll the MCP2210 when pin 6 .mode == MCP2210_PIN_DEDICATED and .has_irq == 1. Then you set the polling interval with .poll_intr_usecs (I think). Finally, the .irq setting was to specify which software IRQ of the local machine that you want to to be triggered! As far as the edge/pulse mode of the pin is described in section 1.6.1.6 of the datasheet.

However, I don't recall what all worked and didn't work so I'll need to study this a bit more closely and get back to you. Also, if I understand you correctly, you aren't using another driver on top of the mcp2210 driver, rather you are interacting with it from user space. Please correct me if I'm wrong here.

I've never tried to use select on a gpio dev node, but I suppose that might work. If you don't get any change events but you see the gpio value change when you cat the file, then I'm wrong or something else is broken in the driver. I'll look into this a bit further.

EDIT: By the way, can you please post your settings.h file?

daniel-santos commented 8 years ago

OK, first of all if you are exporting your gpios to user space via /sys/class/gpio/export and the direction is set to in, then you should verify that you can see changes to the line in the value file of that gpio. We need to be certain that this part is working before troubleshooting anything related to IRQ edge levels or select/poll.

Next, note that this interface is available when CONFIG_GPIO_SYSFS is set in your .config (which I'm sure you have) and is defined in the kernel tree in drivers/gpio/gpiolib-sysfs.c. Understand that when you read the file /sys/class/gpio/<yourpin>/value file that the kernel's gpiolib-sysfs library is calling my gpio get() function. If I've read this correctly, the call sequence goes like this:

drivers/gpio/gpiolib-sysfs.c: gpio_value_show() ->
drivers/gpio/gpiolib.c: gpiod_get_value_cansleep() ->
drivers/gpio/gpiolib.c: _gpiod_get_raw_value() ->
mcp2210-gpio.c: get()

When you call my gpio get function, the driver queries the mcp2210 based upon the rules around stale_gpio_usecs. The way that works is that if stale_gpio_usecs is zero, then any time get() is called, it will query the MCP2210 for an update. If stale_gpio_usecs is non-zero then it will only query the device if the cached data about the known state is older than that value (in microseconds) -- this provides a means for sanity in cases where get may be called in rapid succession, but doing a query for each call would generate way too much overhead. Remember that this driver isn't just written for use from user-space, but also for use by other drivers in kernel-space. Most such drivers presume that a GPIO and/or SPI bus is local and very low latency where as using a USB-to-SPI/GPIO bridge has a very high latency in comparison.

So anyway, if you can see the changes when reading the value file, then you may not get changes when using select or poll because it's not the same as reading a file. Thus, you need the poll_gpio_usecs set to cause the driver to poll the gpio state even when nobody is calling the gpio's get function! I would suggest a value of something like 20000 (20 milliseconds or 50 times per second), but if you get spam in the kernel log about "poll interval collapse" then increase the interval (I need to change this to make it not spam the kernel log) because this is not a "delay" in-between polls, but a fixed interval that it will attempt to maintain.

ToBeReplaced commented 8 years ago

First, thank you so much for the work you already put in to the driver, and thanks for taking the time to help me out. I checked out 452fdc50d359f836c9a281b8f1e2cbf0674ae55a (HEAD) and did a quick session to demonstrate what I am seeing, copied below.

You are correct that I am trying to interact with the gpio pins from userspace: https://www.kernel.org/doc/Documentation/gpio/sysfs.txt . Judging by the USB_QUIRKS comment, it sounds like you have a Raspberry Pi to play with. If you run the python command given at the bottom of my session below on one of your RPi pins, you should see the expected behavior.

I've never worked with a kernel driver before, so much of my confusion comes from unfamiliarity with the domain rather than anything in your code. It all feels quite readable, and much of the configuration is understandable from the mcp2210 manual. For example, changing the pin directions and initial values with .gpio_value and .gpio_direction are explained well by the mcp2210 docs.

[root@localhost mcp2210-linux]# uname -r
3.10.0-327.4.4.el7.centos.plus.x86_64
[root@localhost mcp2210-linux]# git diff
diff --git a/out-of-tree-autoconf.h.template b/out-of-tree-autoconf.h.template
index a931ed1..e75dd18 100644
--- a/out-of-tree-autoconf.h.template
+++ b/out-of-tree-autoconf.h.template
@@ -28,14 +28,14 @@
 #define CONFIG_MCP2210_GPIO            1
 #define CONFIG_MCP2210_EEPROM          1
 #define CONFIG_MCP2210_CREEK           1
-//#define CONFIG_MCP2210_IRQ           1
+#define CONFIG_MCP2210_IRQ             1
 #define CONFIG_MCP2210_DEBUG           1
 #define CONFIG_MCP2210_DEBUG_VERBOSE   1
 #define CONFIG_MCP2210_DEBUG_INITIAL   9
 //#define CONFIG_MCP2210_AUTOPM                1

 /* basically, the RPi */
-#define CONFIG_MCP2210_USB_QUIRKS      1
+//#define CONFIG_MCP2210_USB_QUIRKS    1

 /* this option will eventually get a better name or be removed */
 #define CONFIG_MCP2210_DONT_SUCK_THE_CPU_DRY 1
diff --git a/user/settings-example.h b/user/settings-example.h
index 12f010a..abc04f2 100644
--- a/user/settings-example.h
+++ b/user/settings-example.h
@@ -123,7 +123,7 @@ static const struct mcp2210_board_config my_board_config = {
                        .name = "unused%d",
                }
        },
-       .poll_gpio_usecs              = 0, //10000 * 1000,
+       .poll_gpio_usecs              = 20000, //10000 * 1000,
        .stale_gpio_usecs             = 0, //10000 * 1000,
        .poll_intr_usecs              = 0, //10000 * 1000,
        .stale_intr_usecs             = 0, //10000 * 1000,
[root@localhost mcp2210-linux]# rm -f out-of-tree-autoconf.h && make clean && make all && make modules_install
rm -rf *.o *~ core .depend .*.cmd *.ko *.mod.c mcp2210.s .tmp_versions \
       modules.order Module.symvers
make -C user clean
make[1]: Entering directory `/root/mcp2210-linux/user'
rm -f mcp2210-lib.c *.o mcp2210 libmcp2210.so
make[1]: Leaving directory `/root/mcp2210-linux/user'
cp -n out-of-tree-autoconf.h.template out-of-tree-autoconf.h
make -C /lib/modules/3.10.0-327.4.4.el7.centos.plus.x86_64/build M=/root/mcp2210-linux modules
make[1]: Entering directory `/usr/src/kernels/3.10.0-327.4.4.el7.centos.plus.x86_64'
  CC [M]  /root/mcp2210-linux/mcp2210-core.o
  CC [M]  /root/mcp2210-linux/mcp2210-ioctl.o
  CC [M]  /root/mcp2210-linux/mcp2210-ctl.o
  CC [M]  /root/mcp2210-linux/mcp2210-spi.o
  CC [M]  /root/mcp2210-linux/mcp2210-eeprom.o
  CC [M]  /root/mcp2210-linux/mcp2210-lib.o
  CC [M]  /root/mcp2210-linux/mcp2210-gpio.o
  CC [M]  /root/mcp2210-linux/mcp2210-irq.o
  LD [M]  /root/mcp2210-linux/mcp2210.o
  Building modules, stage 2.
  MODPOST 1 modules
  CC      /root/mcp2210-linux/mcp2210.mod.o
  LD [M]  /root/mcp2210-linux/mcp2210.ko
make[1]: Leaving directory `/usr/src/kernels/3.10.0-327.4.4.el7.centos.plus.x86_64'
make -C user
make[1]: Entering directory `/root/mcp2210-linux/user'
ln -fs ../mcp2210-lib.c .
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef -I.. -D__USER__ -include out-of-tree-autoconf.h  -c -o mcp2210-lib.o mcp2210-lib.c
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef -I.. -D__USER__ -include out-of-tree-autoconf.h  -c -o mcp2210-user.o mcp2210-user.c
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef mcp2210-lib.o mcp2210-user.o -shared -o libmcp2210.so
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef -I.. -D__USER__ -include out-of-tree-autoconf.h  -c -o mcp2210.o mcp2210.c
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef -I.. -D__USER__ -include out-of-tree-autoconf.h  -c -o tests.o tests.c
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef mcp2210.o tests.o -lmcp2210 -L. -o mcp2210-util
make[1]: Leaving directory `/root/mcp2210-linux/user'
make -C /lib/modules/3.10.0-327.4.4.el7.centos.plus.x86_64/build M=/root/mcp2210-linux modules_install
make[1]: Entering directory `/usr/src/kernels/3.10.0-327.4.4.el7.centos.plus.x86_64'
  INSTALL /root/mcp2210-linux/mcp2210.ko
Can't read private key
  DEPMOD  3.10.0-327.4.4.el7.centos.plus.x86_64
make[1]: Leaving directory `/usr/src/kernels/3.10.0-327.4.4.el7.centos.plus.x86_64'
[root@localhost user]# cd /root/mcp2210-linux/user/ && rm -f settings.h && make && modprobe mcp2210
cp -n settings-example.h settings.h
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef -I.. -D__USER__ -include out-of-tree-autoconf.h  -c -o mcp2210-lib.o mcp2210-lib.c
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef -I.. -D__USER__ -include out-of-tree-autoconf.h  -c -o mcp2210-user.o mcp2210-user.c
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef mcp2210-lib.o mcp2210-user.o -shared -o libmcp2210.so
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef -I.. -D__USER__ -include out-of-tree-autoconf.h  -c -o mcp2210.o mcp2210.c
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef -I.. -D__USER__ -include out-of-tree-autoconf.h  -c -o tests.o tests.c
gcc -O2 -pipe -g3 -fPIC -Wall -Wextra -Werror -Wcast-align -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror-implicit-function-declaration -Wundef mcp2210.o tests.o -lmcp2210 -L. -o mcp2210-util
[root@localhost user]# ./mcp2210_bind.sh 
/sys/bus/usb/devices ~/mcp2210-linux/user
[root@localhost user]# LD_LIBRARY_PATH=/root/mcp2210-linux/user/ /root/mcp2210-linux/user/mcp2210-util set config 63
src->pins[0].name = 0x4047d3 ("L6470")
src->pins[0].modalias = 0x40474c ("spidev")
src->pins[1].name = 0x4047d9 ("unused%d")
src->pins[1].modalias = (nil) ("")
src->pins[2].name = 0x4047e2 ("SSPND")
src->pins[2].modalias = (nil) ("")
src->pins[3].name = 0x4047e8 ("LED")
src->pins[3].modalias = (nil) ("")
src->pins[4].name = 0x4047ec ("LOWPWR")
src->pins[4].modalias = (nil) ("")
src->pins[5].name = 0x4047f3 ("USBCFG")
src->pins[5].modalias = (nil) ("")
src->pins[6].name = 0x4047fa ("MOTION")
src->pins[6].modalias = (nil) ("")
src->pins[7].name = 0x404801 ("ADNS-9800")
src->pins[7].modalias = 0x40474c ("spidev")
src->pins[8].name = 0x4047d9 ("unused%d")
src->pins[8].modalias = (nil) ("")
state = 0x241a020 struct mcp2210_state {
  .have_chip_settings          = 1
  .have_power_up_chip_settings = 1
  .have_spi_settings           = 1
  .have_power_up_spi_settings  = 1
  .have_usb_key_params         = 1
  .have_config                 = 1
  .is_spi_probed               = 0
  .is_gpio_probed              = 0
  .chip_settings = 0x241a022 struct mcp2210_chip_settings {
    .pin_mode {
      [0] = spi
      [1] = gpio
      [2] = dedicated
      [3] = dedicated
      [4] = dedicated
      [5] = dedicated
      [6] = dedicated
      [7] = spi
      [8] = gpio
    }
    .gpio_value           = 0x0002
    .gpio_direction       = 0x0140
    .other_settings       = 0x01
    .nvram_access_control = 0x00
    .password             = 00000000 00000000
  }
  .power_up_chip_settings = 0x241a039 struct mcp2210_chip_settings {
    .pin_mode {
      [0] = spi
      [1] = gpio
      [2] = dedicated
      [3] = dedicated
      [4] = dedicated
      [5] = dedicated
      [6] = dedicated
      [7] = spi
      [8] = gpio
    }
    .gpio_value           = 0x0002
    .gpio_direction       = 0x0140
    .other_settings       = 0x01
    .nvram_access_control = 0x00
    .password             = 00000000 00000000
  }
  .spi_settings = 0x241a050 struct mcp2210_spi_xfer_setting {
    .bitrate               = 12000000
    .idle_cs               = 0x01ff
    .active_cs             = 0x0000
    .cs_to_data_delay      = 0x0001
    .last_byte_to_cs_delay = 0x0001
    .delay_between_bytes   = 0x0001
    .bytes_per_trans       = 0x0004
    .mode                  = 0x03
  }
  .power_up_spi_settings = 0x241a061 struct mcp2210_spi_xfer_setting {
    .bitrate               = 12000000
    .idle_cs               = 0x01ff
    .active_cs             = 0x0000
    .cs_to_data_delay      = 0x0001
    .last_byte_to_cs_delay = 0x0001
    .delay_between_bytes   = 0x0001
    .bytes_per_trans       = 0x0004
    .mode                  = 0x03
  }
  .usb_key_params = 0x241a072 struct mcp2210_spi_xfer_setting {
    .vid               = 0x04d8
    .pid               = 0x00de
    .chip_power_option = 0x80
    .requested_power   = 50 (100mA)
  }
  .cur_spi_config              = 0
  .idle_cs                     = 0x0000
  .active_cs                   = 0x0000
  .spi_delay_per_kb            = 0
  .last_poll_gpio              = 0
  .last_poll_intr              = 0
  .interrupt_event_counter     = 0x0000
  }
.config = 0x241a0a0 struct mcp2210_board_config {
  .pins {
    [0] = 0x241a0a8 struct mcp2210_pin_config {
      .mode     = 0x01 (spi)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .spi {
        .max_speed_hz          = 20000
        .min_speed_hz          = 2000
        .mode                  = 0x03
        .bits_per_word         = 8
        .cs_to_data_delay      = 1
        .last_byte_to_cs_delay = 1
        .delay_between_bytes   = 1
        .delay_between_xfers   = 1
      }
      .modalias = spidev
      .name     = L6470
    }
    [1] = 0x241a0d0 struct mcp2210_pin_config {
      .mode     = 0x00 (gpio)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .modalias = (null)
      .name     = unused%d
    }
    [2] = 0x241a0f8 struct mcp2210_pin_config {
      .mode     = 0x02 (dedicated)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .modalias = (null)
      .name     = SSPND
    }
    [3] = 0x241a120 struct mcp2210_pin_config {
      .mode     = 0x02 (dedicated)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .modalias = (null)
      .name     = LED
    }
    [4] = 0x241a148 struct mcp2210_pin_config {
      .mode     = 0x02 (dedicated)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .modalias = (null)
      .name     = LOWPWR
    }
    [5] = 0x241a170 struct mcp2210_pin_config {
      .mode     = 0x02 (dedicated)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .modalias = (null)
      .name     = USBCFG
    }
    [6] = 0x241a198 struct mcp2210_pin_config {
      .mode     = 0x02 (dedicated)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .modalias = (null)
      .name     = MOTION
    }
    [7] = 0x241a1c0 struct mcp2210_pin_config {
      .mode     = 0x01 (spi)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .spi {
        .max_speed_hz          = 20000
        .min_speed_hz          = 5000
        .mode                  = 0x03
        .bits_per_word         = 8
        .cs_to_data_delay      = 2
        .last_byte_to_cs_delay = 2
        .delay_between_bytes   = 4
        .delay_between_xfers   = 10
      }
      .modalias = spidev
      .name     = ADNS-9800
    }
    [8] = 0x241a1e8 struct mcp2210_pin_config {
      .mode     = 0x00 (gpio)
      .has_irq  = 0x00
      .irq      = 0x00
      .irq_type = 0x00
      .modalias = (null)
      .name     = unused%d
    }
  }
  .poll_gpio_usecs  = 20000
  .stale_gpio_usecs = 0
  .poll_intr_usecs  = 0
  .stale_intr_usecs = 0
  .strings_size     = 79
  .strings          = 0x241a230 "L6470"
}
[root@localhost user]# echo 255 > /sys/class/gpio/export && cat /sys/class/gpio/unused255/direction && cat /sys/class/gpio/unused255/edge
in
none
[root@localhost user]# python -c 'import select; p = select.poll(); f = open("/sys/class/gpio/unused255/value"); p.register(f, select.POLLPRI | select.POLLERR); print p.poll(5000); print f.read().rstrip(); print "Externally set the pin high, you should see [] and 1 because edge is set to \"none\""; print p.poll(5000); f.seek(0); print f.read().rstrip();'
[(3, 10)]
0
Externally set the pin high, you should see [] and 1 because edge is set to "none"
[]
1
[root@localhost user]# echo 'rising' > /sys/class/gpio/unused255/edge && cat /sys/class/gpio/unused255/edge
rising
[root@localhost user]# python -c 'import select; p = select.poll(); f = open("/sys/class/gpio/unused255/value"); p.register(f, select.POLLPRI | select.POLLERR); print p.poll(5000); print f.read().rstrip(); print "Externally set the pin high, you should see [(3, 2)] or [(3, 10)] and 1 because edge is set to \"rising\""; print p.poll(5000); f.seek(0); print f.read().rstrip();'
[(3, 10)]
0
Externally set the pin high, you should see [(3, 2)] or [(3, 10)] and 1 because edge is set to "rising"
[]
1

For clarification, the python command says "poll for a maximum of 5 seconds and write out the pin value, then repeat". select.POLLPRI is 2 and select.POLLERR is 8. [(3, 10)] means that poll saw both POLLPRI and POLLERR for file descriptor 3 (the gpio pin) . [] means that neither POLLPRI nor POLLERR was seen.

As far as I can tell, everything looks good until that last command. If I cat the value file in another shell while the python command is blocking, I see the correct value 1, but the python command remains unchanged. This suggests that POLLPRI is not being set even though the value file is being updated. Is this something where the .irq you are talking about is relevant?

daniel-santos commented 8 years ago

Thank you for all of this information, it's very helpful!

I found the problem, this is indeed part of the code I never completed. And since I started the work, my driver is lying to gpiolib-sysfs by telling it that I'm doing all of these things for it. :( On the bright side, I may not be that far away. I even see some commented out code that I copied and pasted from another driver as I was experimenting with it. These are in mcp2210-irq.c, the functions mcp2210_irq_mask, mcp2210_irq_unmask and mcp2210_irq_set_type.

I don't have all of my toys up and running yet -- I recently moved and just unpacked them yesterday, but I don't have it all working yet (I need another network cable in my computer room :). In the mean time, you can read the value and process interrupts that way, but it is a far more CPU-expensive way to ask if a pin has changed.

ToBeReplaced commented 8 years ago

I was looking at https://github.com/raspberrypi/linux/blob/rpi-4.1.y/arch/arm/mach-bcm2708/bcm2708_gpio.c for inspiration, but a literal translation proved too challenging for me. In particular, I wasn't sure how to translate macros like GPIOREN, whether we really want to work relative to 32, and coping with irq_base being an int in your struct (looks like it needs to be a pointer?).

I'm not a C developer, and kernel sources got me lost. Do you know of an example struct definition for a device that includes (irq_)base, rise, fall, high low so I can maybe try again?

daniel-santos commented 8 years ago

I don't follow your question. I have no idea what GPIOREN would be aside from something specific to some device.

Interrupts in Linux are described by an int and irq_base is the starting IRQ number of the first IRQ the driver controls, so that's why it is an int. I actually have interrupts working, although I haven't committed them yet. Honestly, this functionality is for far higher level stuff, like loading spi protocol drivers that are interrupt driven (all in kernel space).

Also, I haven't tested my current irq code using the mechanism that you described yet. Where is this python program that you are using?

I will commit these changes once I clean up the patches.

EDIT: Oh, nevermind, I see the python code now. :)

ToBeReplaced commented 8 years ago

I clearly don't understand irq or really anything that happens in kernel space ;). My goal would be to have gpio interrupts exposed in userspace via setting POLLPRI in the sysfs gpio interface. It sounds like you're probably already there, so that's great!

On the SPI-front, there are plenty of devices that are worth reading/writing from userspace that would benefit from interrupting gpio. For example, you could configure an accelerometer with spidev, map its interrupt pin back to one of the gpio pins, use the sysfs gpio system to wait on the pin, then read the accelerometer with spidev, clearing the pin.

daniel-santos commented 8 years ago

Well, that does make sense.

As far as IRQs, think of it in terms of when some peripheral on your motherboard creates an interrupt, like your ethernet controller. It gets a packet addressed to its MAC address and I'm sure it waits until it has the whole packet queued up in it's little buffer, then it asserts an interrupt, probably by pulling one of the pins on its chip low. This is usually connected to a dedicated interrupt controller chip. Now days computers use an "advanced interrupt controller chip" or APIC. This cute little device has a bunch of input pins, but just one output line that connects to the CPU's interrupt pin. The CPU gets that and then talks to the APIC via IO memory to find out who created the interrupt. You can see all of your computer's interrupts (or at least the ones that either have been fired or that a driver has registered to consume) with cat /proc/interrupts.

My point is that you don't really need an interrupt to do what you are trying to do, you can repeatedly read the value of the gpio to determine when your userspace driver needs to be run. You could put a little sleep in-between each read so that you don't burn the CPU too badly. However, that is far less efficient than if you could just select like you were doing.

As to that part I don't think that will currently work because I haven't finished the glue for the Linux gpio subsystem. Specifically, I haven't yet implemented the to_irq() function. I'll look into that today.

daniel-santos commented 8 years ago

Now this is interesting. In order to have to_irq(), and thus to be able to poll a sysfs gpio for an interrupt, you need to have your kernel configured with GPIOLIB_IRQCHIP. Since I'm using qemu for testing & debugging now, I'm using an x86_64 platform and on this platform, I can only get this with PINCTRL which I can only get if I define X86_INTEL_LPSS or X86_AMD_PLATFORM_DEVICE. I wonder if this is a flaw (I'm using 4.1.15 right now). I guess I'll have to document this later.

daniel-santos commented 8 years ago

Hey, I don't really understand python, so I don't know what's wrong, maybe there's some package I don't have installed? I run Gentoo. Anyway, I get a syntax error with your example:

/sys/class/gpio/GP510
(root@loudmouth-guest)# python -c 'import select; p = select.poll(); f = open("value"); p.register(f, select.POLLPRI | select.POLLERR); print p.poll(5000); print f.read().rstrip(); print "Externally set the pin high, you should see [] and 1 because edge is set to \"none\""; print p.poll(5000); f.seek(0); print f.read().rstrip();'
  File "<string>", line 1
    import select; p = select.poll(); f = open("value"); p.register(f, select.POLLPRI | select.POLLERR); print p.poll(5000); print f.read().rstrip(); print "Externally set the pin high, you should see [] and 1 because edge is set to \"none\""; print p.poll(5000); f.seek(0); print f.read().rstrip();
                                                                                                               ^
SyntaxError: invalid syntax
daniel-santos commented 8 years ago

OK, well in lieu of that, I used a little C program to test it and it's now working. I guess I'll clean up the patches now. Just in case anybody cares, here's the test program:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <poll.h>
#include <fcntl.h>

static void usage(const char *argv0) {
    fprintf(stderr, "USAGE: %s <file_name>\n\n", argv0);
    fprintf(stderr, "    Where file_name is the name of a sysfs gpio value file.\n");
    fprintf(stderr, "    Example: %s /sys/class/gpio/GP510/value\n", argv0);
    exit(1);
}

static void poll_pin(const char *fn) {
    struct pollfd fdlist[1];
    int fd;

    fd = open(fn, O_RDONLY);
    if (fd == -1) {
        perror("open");
        fprintf(stderr, "Failed to open file %s\n", fn);
        return;
    }
    fdlist[0].fd = fd;
    fdlist[0].events = POLLPRI;

    while (1) {
        int ret;
        char buf[3];

        if (poll(fdlist, 1, -1) == -1) {
            perror("poll");
            return;
        }

        ret = read(fdlist[0].fd, buf, 2);
        if (ret == -1) {
            perror("read");
            return;
        }
        buf[ret] = 0;
        printf("event on pin %s, value = %c\n", fn, buf[0]);

        if (lseek(fd, 0, SEEK_SET) == -1) {
            perror("lseek");
            return;
        }
    }
}

int main(int argc, char *argv[]) {
    if (argc != 2)
        usage(argv[0]);

    poll_pin(argv[1]);

    return 0;
}
ToBeReplaced commented 8 years ago

Quick guess... python --version is Python 3.x.x not Python 2.x.x? If so, try:

python -c 'import select; p = select.poll(); f = open("/sys/class/gpio/unused255/value"); p.register(f, select.POLLPRI | select.POLLERR); print(p.poll(5000)); print(f.read().rstrip()); print("Externally set the pin high, you should see [(3, 2)] or [(3, 10)] and 1 because edge is set to \"rising\""); print(p.poll(5000)); f.seek(0); print(f.read().rstrip());'

print was incompatibly upgraded in python 3, leading to a syntax error

daniel-santos commented 8 years ago

hehe, yes :) 3.4.3 :) Thanks for that!

I've committed the changes, so when you get a chance pull them and try it out. I haven't updated the README yet, so here's the instructions in a nutshell. For the gpio pin you want to use as an interrupt, set the following.

.has_irq=1,
.irq=0,
irq_type = IRQ_TYPE_EDGE_RISING,

For IRQ_TYPE_EDGE_RISING, please copy the enum from the new settings-example.h. Now I believe that you can omit irq_type and just echo rising > /sys/class/gpio/asdf/edge, which you have to do either way to tell gpiolib-sysfs that you want interrupts. When you do this, it should be telling the mcp2210 what the trigger type is anyway, but I haven't tested that just yet.

Another thing is that the mcp2210 doesn't have a way to ACK interrupts. An interrupt will be generated once and my struct irq_chip doesn't define an ack function. This is because we aren't doing a real hardware interrupt on the host machine, so there's nothing that needs to be done prior to exiting the ISR (and thus, re-enabling interrupts). However, you'll need to do whatever you need to do so that your peripheral de-asserts the interrupt line so that you can be notified of the next interrupt.

ToBeReplaced commented 8 years ago

I pulled down master and had to omit some code due to being on an older kernel (3.10). Unfortunately, mask didn't show up in irq_data until 3.11.

diff --git a/mcp2210-irq.c b/mcp2210-irq.c
index c2b961d..b67b1c8 100644
--- a/mcp2210-irq.c
+++ b/mcp2210-irq.c
@@ -28,13 +28,17 @@
 static int complete_poll(struct mcp2210_cmd *cmd, void *context);

 static void mcp2210_irq_mask(struct irq_data *data) {
-       struct mcp2210_device *dev = irq_data_get_irq_chip_data(data);
-       dev->irq_mask |= data->mask;
+       #if LINUX_VERSION_CODE > KERNEL_VERSION(3,10,0)
+               struct mcp2210_device *dev = irq_data_get_irq_chip_data(data);
+               dev->irq_mask |= data->mask;
+       #endif
 }

 static void mcp2210_irq_unmask(struct irq_data *data) {
-       struct mcp2210_device *dev = irq_data_get_irq_chip_data(data);
-       dev->irq_mask &= ~data->mask;
+       #if LINUX_VERSION_CODE > KERNEL_VERSION(3,10,0)
+               struct mcp2210_device *dev = irq_data_get_irq_chip_data(data);
+               dev->irq_mask &= ~data->mask;
+       #endif
 }

 static int mcp2210_irq_set_type(struct irq_data *data, unsigned int flow_type) {

With everything else left alone, I observed the following: 1) After export, the edge file still read none, and the value file was not selectable (as before, expected). 2) After setting the edge file to rising, the value file was selectable only on rising events (as desired, different from before). 3) After setting the edge file to 'falling', the value file was still selectable only on rising events (unexpected).

More playing around showed that whatever is set as the irq_type is used regardless of the contents of the edge file as long as the edge file does not read none. This may be what you expect given the commented out code above, I'm not sure.

The irq_type is mandatory on my side; omitting it and setting the edge file did not result in a selectable value file.

This behavior is entirely sufficient for my use case, as I do not need to change the edge file in realtime. I can set the edge to match the irq_type and everything appears to work as desired.

The only possibly odd behavior is that I see the POLLERR bit set in addition to the POLLPRI bit. This is not a problem for me, and I don't know what is supposed to occur. Perhaps this just has to do with debounce?

Finally, is there a way for me to compensate you for your work or is there a charity you would be interested in me making a small donation to? I really appreciate the effort you've put in and I understand how this kind of work is often a thankless job.

daniel-santos commented 8 years ago

I apologize for the delay in getting back to you. Firstly, I can't say that I'm in any position to turn down compensation at the moment, I'll send you an email. Thanks! :)

As for the patch, I greatly appreciate this. Generally, I would prefer if you forked the project on github and submited a pull request or even run git send-email and post the output so that you can get proper credit. In this case it looks like my mask function is broken anyway (doesn't work at all), so I'll have to re-write it either way.

As for the irq_type, it is a bug that you cannot change it dynamically via sysfs. I did intend (at least for now) on limiting the gpios that can produce interrupts to those that are configured for it in the settings. I had expected that trying to set /sys/class/gpio/n/edge_type on a pin not configured with .irq = 1 to anything other than none would return ENOSYS and print the warning mcp2210_to_irq: pin %u not configured as irq producer to the kernel log, but I also intended for users to be able to change the edge type dynamically.

Finally, I don't yet know why POLERR would be set, but I suspect it's something that I did wrong. I'll find out! :)

g5pw commented 5 years ago

Hello @daniel-santos, what's the status on this? I wanted to use interrupts on some GPIOs (not GPIO6) but apparently, if I write "falling" or "rising", or anything else than "none" in the edge file, I get EINVAL. I'm not really a kernel hacker, but I dabble, so I might be able to help!

daniel-santos commented 5 years ago

Hello g5pw! I really need to through my issues! This should be finished. The code for GPIOs and interrupts has been revised a few times. I have been using it to supply interrupts to spi protocol drivers, but I haven't tested it from userspace (via sysfs) in a while.

g5pw commented 5 years ago

I can help you with that, since I use the module for a work-related project (thank you, by the way!). I'm using the master, but it looks like it's not working for me? Moreover, I can't seem to find the exact place where EINVAL is returned when writing to the edge file...

daniel-santos commented 5 years ago

I got sick last week, so sorry for my delay here. Can you please post your settings.h file? Remember that there are two mechanisms for producing interrupts: using GP6 in dedicated mode for edge-triggered and then just using any GPIO for level-triggered interrupts. You don't want to use a GPIO for edge-triggered because it is unreliable.