[PATCH 05/15] staging: comedi: add new device-global config interface
Ian Abbott
abbotti at mev.co.uk
Thu Nov 10 18:14:35 UTC 2016
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 <olsonse at 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: <abbotti at mev.co.uk> )=-
-=( Web: http://www.mev.co.uk/ )=-
More information about the devel
mailing list