On 12/10/16 12:05, Spencer E. Olson wrote:
Adds interface for configuring options that are global to all sub-devices.
For now, only options to configure device-globally identified signal routes
have been defined.

Signed-off-by: Spencer E. Olson <olso...@umich.edu>
---
 drivers/staging/comedi/comedi.h      | 49 +++++++++++++++++++++
 drivers/staging/comedi/comedi_fops.c | 84 ++++++++++++++++++++++++++++++++++++
 drivers/staging/comedi/comedidev.h   | 13 ++++++
 drivers/staging/comedi/drivers.c     | 18 ++++++++
 4 files changed, 164 insertions(+)

diff --git a/drivers/staging/comedi/comedi.h b/drivers/staging/comedi/comedi.h
index c80d0d6..a0ff6c1 100644
--- a/drivers/staging/comedi/comedi.h
+++ b/drivers/staging/comedi/comedi.h
@@ -116,6 +116,7 @@
 #define INSN_WRITE             (1 | INSN_MASK_WRITE)
 #define INSN_BITS              (2 | INSN_MASK_READ | INSN_MASK_WRITE)
 #define INSN_CONFIG            (3 | INSN_MASK_READ | INSN_MASK_WRITE)
+#define INSN_DEVICE_CONFIG     (INSN_CONFIG | INSN_MASK_SPECIAL)
 #define INSN_GTOD              (4 | INSN_MASK_READ | INSN_MASK_SPECIAL)
 #define INSN_WAIT              (5 | INSN_MASK_WRITE | INSN_MASK_SPECIAL)
 #define INSN_INTTRIG           (6 | INSN_MASK_WRITE | INSN_MASK_SPECIAL)
@@ -357,6 +358,23 @@ enum configuration_ids {
 };

 /**
+ * enum device_configuration_ids - COMEDI configuration instruction codes 
global
+ * to an entire device.
+ * @INSN_DEVICE_CONFIG_TEST_ROUTE:     Validate the possibility of a
+ *                                     globally-named route
+ * @INSN_DEVICE_CONFIG_CONNECT_ROUTE:  Connect a globally-named route
+ * @INSN_DEVICE_CONFIG_DISCONNECT_ROUTE:Disconnect a globally-named route
+ * @INSN_DEVICE_CONFIG_GET_ROUTES:     Get a list of all globally-named routes
+ *                                     that are valid for a particular device.
+ */
+enum device_config_route_ids {
+       INSN_DEVICE_CONFIG_TEST_ROUTE = 0,
+       INSN_DEVICE_CONFIG_CONNECT_ROUTE = 1,
+       INSN_DEVICE_CONFIG_DISCONNECT_ROUTE = 2,
+       INSN_DEVICE_CONFIG_GET_ROUTES = 3,
+};
+
+/**
  * enum comedi_digital_trig_op - operations for configuring a digital trigger
  * @COMEDI_DIGITAL_TRIG_DISABLE:       Return digital trigger to its default,
  *                                     inactive, unconfigured state.
@@ -886,6 +904,37 @@ struct comedi_bufinfo {
        unsigned int unused[4];
 };

+/**
+ * Globally-named route pair to contain values returned by comedi_get_routes.
+ * @source:            Globally-identified source of route.
+ * @destination:       Globally-identified destination of route.
+ */
+struct comedi_route_pair {
+       unsigned int source;
+       unsigned int destination;
+};
+
+/**
+ * Structure for arguments of INSN_DEVICE_CONFIG_GET_ROUTES.
+ * @config_id: The first element of comedi_insn->data must contain an insn
+ *             op-code.
+ * @n: On input, represents the length of the route_list array.
+ *             On output, represents the number of route pairs copied to user.
+ * @route_list:        Pointer to user array into which to copy route 
information.  If
+ *             route_list is NULL, the number of valid routes for the given
+ *             device is returned.
+ *
+ * The arguments of INSN commands are actually an array of unsigned int values.
+ * For INSN_DEVICE_CONFIG_GET_ROUTES, the array must actually contain data for
+ * this structure.  In short, a user can define this structure, cast it to the
+ * data component of struct comedi_insn (unsigned int[]).
+ */
+struct comedi_get_routes_data {
+       unsigned int config_id;
+       unsigned int n;
+       struct comedi_route_pair __user *route_list;
+};
+
 /* range stuff */

 #define __RANGE(a, b)  ((((a) & 0xffff) << 16) | ((b) & 0xffff))
diff --git a/drivers/staging/comedi/comedi_fops.c 
b/drivers/staging/comedi/comedi_fops.c
index 64b3966..d087a61 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -1243,6 +1243,77 @@ static int check_insn_config_length(struct comedi_insn 
*insn,
        return -EINVAL;
 }

+static int check_insn_device_config_length(struct comedi_insn *insn,
+                                          unsigned int *data)
+{
+       if (insn->n < 1)
+               return -EINVAL;
+
+       switch (data[0]) {
+       case INSN_DEVICE_CONFIG_TEST_ROUTE:
+       case INSN_DEVICE_CONFIG_CONNECT_ROUTE:
+       case INSN_DEVICE_CONFIG_DISCONNECT_ROUTE:
+               if (insn->n == 3)
+                       return 0;
+               break;
+       case INSN_DEVICE_CONFIG_GET_ROUTES:
+               /*
+                * big enough for config_id, a pointer to routelist userland
+                * memory, and the length of the userland memory buffer.
+                */
+               if (insn->n == (sizeof(struct comedi_get_routes_data) /
+                               sizeof(unsigned int)))
+                       return 0;
+               break;
+       }
+       return -EINVAL;
+}

INSN_DEVICE_CONFIG_GET_ROUTES is incompatible with the handling of the COMEDI_INSN and COMEDI_INSNLIST ioctls in "comedi_compat32.c", and I don't really want ugly INSN-code-specific special cases in that code.

Perhaps the data for the INSN_DEVICE_CONFIG_GET_ROUTES can be flattened so that config_id appears in data[1], n appears in data[2], and the source and destination pairs appear in data[3+i] and data[4+i]. Then if insn->n == 3, the user just wants to get the number of routes. If insn->n == 3 + 2 * data[2], the user wants to get the actual route data too.

+
+/*
+ * Calls low-level driver get_valid_routes function to either return a count of
+ * valid routes to user, or copy of list of all valid device routes to buffer 
in
+ * userspace.
+ *
+ * Return: -EINVAL if low-level driver does not allocate and return routes as
+ *        expected.  Returns 0 otherwise.
+ */
+static int get_valid_routes(struct comedi_device *dev, unsigned int *data)
+{
+       struct comedi_get_routes_data *packed =
+               (struct comedi_get_routes_data *)data;
+       struct comedi_route_pair *klr = NULL;
+       unsigned int n;
+
+       n = dev->get_valid_routes(dev, !packed->route_list ? NULL : &klr);
+
+       if (!packed->route_list || n == 0) {
+               /* either user requested count only, or we have no data */
+               if (klr) {
+                       dev_warn(dev->class_dev,
+                                "low-level driver allocated 0-length array\n");
+                       kfree(klr);
+               }
+               packed->n = n;
+               return 0;
+       }
+
+       if (!klr) {
+               dev_err(dev->class_dev,
+                       "driver returned zero routes, but indicated %u\n", n);
+               return -EINVAL;
+       }
+
+       /* by this point, we know that there is data and also a return buffer */
+       packed->n = min(n, packed->n);
+       if (packed->n > 0) {
+               copy_to_user(packed->route_list, klr,
+                            packed->n * sizeof(struct comedi_route_pair));

The return value of copy_to_user() should be checked here, but if the interface is changed to return the routes in insn->data[3] onwards, it's a non-issue.

+       }
+
+       kfree(klr);
+       return 0;
+}
+
 static int parse_insn(struct comedi_device *dev, struct comedi_insn *insn,
                      unsigned int *data, void *file)
 {
@@ -1306,6 +1377,19 @@ static int parse_insn(struct comedi_device *dev, struct 
comedi_insn *insn,
                        if (ret >= 0)
                                ret = 1;
                        break;
+               case INSN_DEVICE_CONFIG:
+                       ret = check_insn_device_config_length(insn, data);
+                       if (ret)
+                               break;
+
+                       if (data[0] == INSN_DEVICE_CONFIG_GET_ROUTES) {
+                               ret = get_valid_routes(dev, data);
+                               break;
+                       }
+
+                       /* other global device config instructions. */
+                       ret = dev->insn_device_config(dev, insn, data);
+                       break;
                default:
                        dev_dbg(dev->class_dev, "invalid insn\n");
                        ret = -EINVAL;
diff --git a/drivers/staging/comedi/comedidev.h 
b/drivers/staging/comedi/comedidev.h
index dcb6376..37fe2e4 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -509,6 +509,15 @@ struct comedi_driver {
  *     called when @use_count changes from 0 to 1.
  * @close: Optional pointer to a function set by the low-level driver to be
  *     called when @use_count changed from 1 to 0.
+ * @insn_device_config: Optional pointer to a handler for all sub-instructions
+ *     except %INSN_DEVICE_CONFIG_GET_ROUTES of the %INSN_DEVICE_CONFIG
+ *     instruction.  If this is not initialized by the low-level driver, a
+ *     default handler will be set during post-configuration.
+ * @get_valid_routes: Optional pointer to a handler for the
+ *     %INSN_DEVICE_CONFIG_GET_ROUTES sub-instruction of the
+ *     %INSN_DEVICE_CONFIG instruction set.  If this is not initialized by the
+ *     low-level driver, a default handler that copies zero routes back to the
+ *     user will be used.
  *
  * This is the main control data structure for a COMEDI device (as far as the
  * COMEDI core is concerned).  There are two groups of COMEDI devices -
@@ -558,6 +567,10 @@ struct comedi_device {

        int (*open)(struct comedi_device *dev);
        void (*close)(struct comedi_device *dev);
+       int (*insn_device_config)(struct comedi_device *, struct comedi_insn *,
+                                 unsigned int *);
+       unsigned int (*get_valid_routes)(struct comedi_device *dev,
+                                        struct comedi_route_pair **pptr);
 };

 /*
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index a5bf2cc..57fc608 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -220,6 +220,18 @@ static int poll_invalid(struct comedi_device *dev, struct 
comedi_subdevice *s)
        return -EINVAL;
 }

+int insn_device_inval(struct comedi_device *dev, struct comedi_insn *insn,
+                     unsigned int *data)
+{
+       return -EINVAL;
+}
+
+unsigned int get_zero_valid_routes(struct comedi_device *dev,
+                                  struct comedi_route_pair **pptr)
+{
+       return 0;
+}
+

Do those two functions need to be global?

 int insn_inval(struct comedi_device *dev, struct comedi_subdevice *s,
               struct comedi_insn *insn, unsigned int *data)
 {
@@ -662,6 +674,12 @@ static int __comedi_device_postconfig(struct comedi_device 
*dev)
        int ret;
        int i;

+       if (!dev->insn_device_config)
+               dev->insn_device_config = insn_device_inval;
+
+       if (!dev->get_valid_routes)
+               dev->get_valid_routes = get_zero_valid_routes;
+
        for (i = 0; i < dev->n_subdevices; i++) {
                s = &dev->subdevices[i];




--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbo...@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to