| Message ID | 20210304154205.1918124-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: event_monitor: Enable events before monitoring | expand |
On Thu, 2021-03-04 at 16:42 +0100, Linus Walleij wrote: > After some painful sessions with a driver that register an > enable/disable sysfs knob (gp2ap002) and manually going > in and enabling the event before monitoring it: > > cd /sys/bus/iio/devices/iio\:device2/events > # ls > in_proximity_thresh_either_en > # echo 1 > in_proximity_thresh_either_en > > I realized that it's better if the iio_event_monitor is > smart enough to enable all events by itself and disable them > after use. > > I didn't add a command line option for this, to me it > seems pretty intuitive and mostly what you want the tool > to do for you. > > Cc: Bastien Nocera <hadess@hadess.net> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> If I'm not mistaken, "-a" does that for the iio_generic_buffer tool. Maybe moving enable_disable_all_channels() to a common location and using that would cut down on the duplicated code? > --- > Bastien: added you on CC FYI because I suppose maybe > iio-sensor-proxy isn't yet doing this and one day people > will wonder why their events aren't arriving. > --- > tools/iio/iio_event_monitor.c | 51 > +++++++++++++++++++++++++++++++++++ > tools/iio/iio_utils.h | 1 + > 2 files changed, 52 insertions(+) > > diff --git a/tools/iio/iio_event_monitor.c > b/tools/iio/iio_event_monitor.c > index bb03859db89d..27778fe28f82 100644 > --- a/tools/iio/iio_event_monitor.c > +++ b/tools/iio/iio_event_monitor.c > @@ -14,6 +14,7 @@ > > #include <unistd.h> > #include <stdlib.h> > +#include <dirent.h> > #include <stdbool.h> > #include <stdio.h> > #include <errno.h> > @@ -280,10 +281,49 @@ static void print_event(struct iio_event_data > *event) > printf("\n"); > } > > +/* Enable or disable events in sysfs if the knob is available */ > +static void enable_events(char *dev_dir, int enable) > +{ > + const struct dirent *ent; > + char evdir[256]; > + int ret; > + DIR *dp; > + > + snprintf(evdir, sizeof(evdir), FORMAT_EVENTS_DIR, dev_dir); > + evdir[sizeof(evdir)-1] = '\0'; > + > + dp = opendir(evdir); > + if (!dp) { > + fprintf(stderr, "Enabling/disabling events: can't > open %s\n", > + evdir); > + return; > + } > + > + while (ent = readdir(dp), ent) { > + if (iioutils_check_suffix(ent->d_name, "_en")) { > + printf("%sabling: %s\n", > + enable ? "En" : "Dis", > + ent->d_name); > + ret = write_sysfs_int(ent->d_name, evdir, > + enable); > + if (ret < 0) > + fprintf(stderr, "Failed to > enable/disable %s\n", > + ent->d_name); > + } > + } > + > + if (closedir(dp) == -1) { > + perror("Enabling/disabling channels: " > + "Failed to close directory"); > + return; > + } > +} > + > int main(int argc, char **argv) > { > struct iio_event_data event; > const char *device_name; > + char *dev_dir_name = NULL; > char *chrdev_name; > int ret; > int dev_num; > @@ -303,6 +343,10 @@ int main(int argc, char **argv) > ret = asprintf(&chrdev_name, "/dev/iio:device%d", > dev_num); > if (ret < 0) > return -ENOMEM; > + /* Look up sysfs dir as well if we can */ > + ret = asprintf(&dev_dir_name, "%siio:device%d", > iio_dir, dev_num); > + if (ret < 0) > + return -ENOMEM; > } else { > /* > * If we can't find an IIO device by name assume > device_name is > @@ -313,6 +357,9 @@ int main(int argc, char **argv) > return -ENOMEM; > } > > + if (dev_dir_name) > + enable_events(dev_dir_name, 1); > + > fd = open(chrdev_name, 0); > if (fd == -1) { > ret = -errno; > @@ -365,6 +412,10 @@ int main(int argc, char **argv) > perror("Failed to close event file"); > > error_free_chrdev_name: > + /* Disable events after use */ > + if (dev_dir_name) > + enable_events(dev_dir_name, 0); > + > free(chrdev_name); > > return ret; > diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h > index 74bde4fde2c8..c01695049739 100644 > --- a/tools/iio/iio_utils.h > +++ b/tools/iio/iio_utils.h > @@ -13,6 +13,7 @@ > #define IIO_MAX_NAME_LENGTH 64 > > #define FORMAT_SCAN_ELEMENTS_DIR "%s/scan_elements" > +#define FORMAT_EVENTS_DIR "%s/events" > #define FORMAT_TYPE_FILE "%s_type" > > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
On Thu, Mar 4, 2021 at 5:00 PM Bastien Nocera <hadess@hadess.net> wrote: > If I'm not mistaken, "-a" does that for the iio_generic_buffer tool. Yeah I implemented that, and I thought about doing the same here but ... the name of the tool sort of announce that one want to listen to all events so I thought it should just default-enable all of them in this case. > Maybe moving enable_disable_all_channels() to a common location and > using that would cut down on the duplicated code? The event enablement is slightly different, the generic buffer turns on various channels, which is conceptually different from various events, but let's see what Jonathan says. We are sharing most of the code already in the iio-utils.c but I can try to break out more if it doesn't get to abstract. Thanks Bastien, Linus Walleij
On Thu, 4 Mar 2021 21:48:25 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Mar 4, 2021 at 5:00 PM Bastien Nocera <hadess@hadess.net> wrote: > > > If I'm not mistaken, "-a" does that for the iio_generic_buffer tool. > > Yeah I implemented that, and I thought about doing the same here > but ... the name of the tool sort of announce that one want to > listen to all events so I thought it should just default-enable > all of them in this case. > > > Maybe moving enable_disable_all_channels() to a common location and > > using that would cut down on the duplicated code? > > The event enablement is slightly different, the generic buffer > turns on various channels, which is conceptually different > from various events, but let's see what Jonathan says. > > We are sharing most of the code already in the iio-utils.c > but I can try to break out more if it doesn't get to abstract. Sadly this doesn't work for many devices. It is a common thing for hardware to only support a much smaller set of event monitoring registers / threshold detectors than the number of channels. In many cases we handle that by working on a fifo basis. So what this will do is enable a bunch of events which will then be replaced by later events - end result some random event will be enabled (or maybe 2 of them across N channels) Not intuitive at all :( I'm fine with it being controlled by a parameter though if that works for you. Docs should explain it doesn't always result in all events being enabled however if the hardware is not capable of doing that. Jonathan > > Thanks Bastien, > Linus Walleij
On Sat, Mar 6, 2021 at 4:39 PM Jonathan Cameron <jic23@kernel.org> wrote: > Sadly this doesn't work for many devices. > It is a common thing for hardware to only support a much smaller > set of event monitoring registers / threshold detectors than the > number of channels. In many cases we handle that by working on > a fifo basis. So what this will do is enable a bunch of events > which will then be replaced by later events - end result some > random event will be enabled (or maybe 2 of them across N channels) I understand. What about augmenting the heuristics like this: 1. Count the available events. 2. If they are just one, then enable that event and disable after use. This will make all proximity sensors and other things that just provide a single event work out of the box. Yours, Linus Walleij
On Sat, 6 Mar 2021 22:29:41 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > On Sat, Mar 6, 2021 at 4:39 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > Sadly this doesn't work for many devices. > > It is a common thing for hardware to only support a much smaller > > set of event monitoring registers / threshold detectors than the > > number of channels. In many cases we handle that by working on > > a fifo basis. So what this will do is enable a bunch of events > > which will then be replaced by later events - end result some > > random event will be enabled (or maybe 2 of them across N channels) > > I understand. > > What about augmenting the heuristics like this: > > 1. Count the available events. > 2. If they are just one, then enable that event and disable after use. > > This will make all proximity sensors and other things that just > provide a single event work out of the box. Rather unintuitive interface. I think we are better off just adding a -a parameter like we have for the buffer example. If people get used to it enabling sensible events for a simple device then move on to a more complex one where the heuristic breaks down then they will be very confused. J > > Yours, > Linus Walleij
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c index bb03859db89d..27778fe28f82 100644 --- a/tools/iio/iio_event_monitor.c +++ b/tools/iio/iio_event_monitor.c @@ -14,6 +14,7 @@ #include <unistd.h> #include <stdlib.h> +#include <dirent.h> #include <stdbool.h> #include <stdio.h> #include <errno.h> @@ -280,10 +281,49 @@ static void print_event(struct iio_event_data *event) printf("\n"); } +/* Enable or disable events in sysfs if the knob is available */ +static void enable_events(char *dev_dir, int enable) +{ + const struct dirent *ent; + char evdir[256]; + int ret; + DIR *dp; + + snprintf(evdir, sizeof(evdir), FORMAT_EVENTS_DIR, dev_dir); + evdir[sizeof(evdir)-1] = '\0'; + + dp = opendir(evdir); + if (!dp) { + fprintf(stderr, "Enabling/disabling events: can't open %s\n", + evdir); + return; + } + + while (ent = readdir(dp), ent) { + if (iioutils_check_suffix(ent->d_name, "_en")) { + printf("%sabling: %s\n", + enable ? "En" : "Dis", + ent->d_name); + ret = write_sysfs_int(ent->d_name, evdir, + enable); + if (ret < 0) + fprintf(stderr, "Failed to enable/disable %s\n", + ent->d_name); + } + } + + if (closedir(dp) == -1) { + perror("Enabling/disabling channels: " + "Failed to close directory"); + return; + } +} + int main(int argc, char **argv) { struct iio_event_data event; const char *device_name; + char *dev_dir_name = NULL; char *chrdev_name; int ret; int dev_num; @@ -303,6 +343,10 @@ int main(int argc, char **argv) ret = asprintf(&chrdev_name, "/dev/iio:device%d", dev_num); if (ret < 0) return -ENOMEM; + /* Look up sysfs dir as well if we can */ + ret = asprintf(&dev_dir_name, "%siio:device%d", iio_dir, dev_num); + if (ret < 0) + return -ENOMEM; } else { /* * If we can't find an IIO device by name assume device_name is @@ -313,6 +357,9 @@ int main(int argc, char **argv) return -ENOMEM; } + if (dev_dir_name) + enable_events(dev_dir_name, 1); + fd = open(chrdev_name, 0); if (fd == -1) { ret = -errno; @@ -365,6 +412,10 @@ int main(int argc, char **argv) perror("Failed to close event file"); error_free_chrdev_name: + /* Disable events after use */ + if (dev_dir_name) + enable_events(dev_dir_name, 0); + free(chrdev_name); return ret; diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h index 74bde4fde2c8..c01695049739 100644 --- a/tools/iio/iio_utils.h +++ b/tools/iio/iio_utils.h @@ -13,6 +13,7 @@ #define IIO_MAX_NAME_LENGTH 64 #define FORMAT_SCAN_ELEMENTS_DIR "%s/scan_elements" +#define FORMAT_EVENTS_DIR "%s/events" #define FORMAT_TYPE_FILE "%s_type" #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
After some painful sessions with a driver that register an enable/disable sysfs knob (gp2ap002) and manually going in and enabling the event before monitoring it: cd /sys/bus/iio/devices/iio\:device2/events # ls in_proximity_thresh_either_en # echo 1 > in_proximity_thresh_either_en I realized that it's better if the iio_event_monitor is smart enough to enable all events by itself and disable them after use. I didn't add a command line option for this, to me it seems pretty intuitive and mostly what you want the tool to do for you. Cc: Bastien Nocera <hadess@hadess.net> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Bastien: added you on CC FYI because I suppose maybe iio-sensor-proxy isn't yet doing this and one day people will wonder why their events aren't arriving. --- tools/iio/iio_event_monitor.c | 51 +++++++++++++++++++++++++++++++++++ tools/iio/iio_utils.h | 1 + 2 files changed, 52 insertions(+)