Hi,
Please, find attached two patches related with the Comedi layer.
Each patch is a proposal to change the way the ranges tables are managed
within the subdevice structure.
As you may know, the range structure allows the translation of freshly
acquired logical values into real values (thanks to the unit, the
minimal and the maximal value)
An acquisition subdevice can work with various ranges. That is why
some tab field is necessary to hold them all in the subdevice
descriptor structure. However, what I do not understand is why are
there two fields to handle them (and we cannot use both at the same
time).
Currently, according to the ranges' type:
- either same ranges for all channels
- or a specific ranges table per channel,
we have to use the field range_table or the field range_table_list.
The range management could be made easier or at least more generic:
- the first proposal introduces a container structure. Thanks to its
first field, we could define the range's type for a specific
subdevice; and the second field is devoted to store comedi_lrange
pointers. Some inline helper functions were added so as to manage
the range without having to get through the structures.
- the second patch is the closest one from the original API. An internal
structure is added (comedi_trange_struct) so as to handle per channel
ranges tables. The internal range management code just has to detect
which structure type is hidden behind the subdevice field range_table.
These patches are neither that clean nor perfect, they are just some
ideas I would like to share. By the way, these patches modify only the
core files (kcomedilib and the drivers have been left unchanged).
Best regards.
Alexis.
>From 12b57a9ece86c658fc84db4c635d69d2f7e9638f Mon Sep 17 00:00:00 2001
From: Alexis Berlemont <alexis.berlemont@xxxxxxxxx>
Date: Sun, 26 Apr 2009 00:51:43 +0200
Subject: [PATCH] Introduce a container structure for ranges so as to handle global
range tables and per channel range tables
---
drivers/staging/comedi/comedi_fops.c | 11 +++--
drivers/staging/comedi/comedidev.h | 75 +++++++++++++++++++++++++++++++++-
drivers/staging/comedi/drivers.c | 5 +-
drivers/staging/comedi/range.c | 64 +++++++++++------------------
4 files changed, 107 insertions(+), 48 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index bb4a289..c71ddf6 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -419,7 +419,7 @@ static int do_subdinfo_ioctl(struct comedi_device *dev, struct comedi_subdinfo *
us->maxdata = s->maxdata;
if (s->range_table) {
us->range_type =
- (i << 24) | (0 << 16) | (s->range_table->length);
+ (i << 24) | (0 << 16) | get_range_table(s, 0)->length;
} else {
us->range_type = 0; /* XXX */
}
@@ -437,7 +437,8 @@ static int do_subdinfo_ioctl(struct comedi_device *dev, struct comedi_subdinfo *
us->subd_flags |= SDF_MAXDATA;
if (s->flaglist)
us->subd_flags |= SDF_FLAGS;
- if (s->range_table_list)
+ if (s->range_table &&
+ (s->range_table->mode & COMEDI_PERCHAN_LRANGE) != 0)
us->subd_flags |= SDF_RANGETYPE;
if (s->do_cmd)
us->subd_flags |= SDF_CMD;
@@ -503,13 +504,15 @@ static int do_chaninfo_ioctl(struct comedi_device *dev, struct comedi_chaninfo *
if (it.rangelist) {
int i;
- if (!s->range_table_list)
+ if(s->range_table == NULL ||
+ (s->range_table->mode & COMEDI_PERCHAN_LRANGE) == 0)
return -EINVAL;
+
for (i = 0; i < s->n_chan; i++) {
int x;
x = (dev->minor << 28) | (it.subdev << 24) | (i << 16) |
- (s->range_table_list[i]->length);
+ get_range_table(s, i)->length;
put_user(x, it.rangelist + i);
}
#if 0
diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index 753d3ab..4c67058 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -149,8 +149,7 @@ struct comedi_subdevice {
unsigned int settling_time_0;
- const struct comedi_lrange *range_table;
- const struct comedi_lrange *const *range_table_list;
+ struct comedi_crange *range_table;
unsigned int *chanlist; /* driver-owned chanlist (not used) */
@@ -411,6 +410,78 @@ struct comedi_lrange {
struct comedi_krange range[GCC_ZERO_LENGTH_ARRAY];
};
+#define COMEDI_PERCHAN_LRANGE 0x80000000
+
+struct comedi_crange {
+ unsigned int mode;
+ struct comedi_lrange *list[0];
+};
+
+/* For static configuration (global declaration) */
+#define COMEDI_GLOBAL_RNG(x) { \
+ .mode = 0, \
+ .list = {&(x)}, }
+
+/* For dynamic confiugration */
+static inline int setup_global_range_table(struct comedi_subdevice * subd,
+ struct comedi_lrange *lrng)
+{
+ subd->range_table = kmalloc(sizeof(struct comedi_crange) +
+ sizeof(struct comedi_lrange *),
+ GFP_KERNEL);
+
+ if(subd->range_table == NULL)
+ return -ENOMEM;
+
+ subd->range_table->mode = 0;
+ subd->range_table->list[0] = lrng;
+
+ return 0;
+}
+
+static inline int alloc_perchan_range_table(struct comedi_subdevice * subd)
+{
+ subd->range_table = kmalloc(sizeof(struct comedi_crange) +
+ sizeof(struct comedi_lrange *) *
+ subd->n_chan,
+ GFP_KERNEL);
+
+ if(subd->range_table == NULL)
+ return -ENOMEM;
+
+ memset(subd->range_table, 0,
+ sizeof(struct comedi_crange) +
+ sizeof(struct comedi_lrange *) * subd->n_chan);
+
+ subd->range_table->mode = COMEDI_PERCHAN_LRANGE;
+
+ return 0;
+}
+
+static inline int set_range_table(struct comedi_subdevice *subd,
+ unsigned int chan_idx,
+ struct comedi_lrange *lrng)
+{
+ if(subd->range_table == NULL)
+ return -EINVAL;
+
+ chan_idx = (subd->range_table->mode & COMEDI_PERCHAN_LRANGE) ?
+ chan_idx : 0;
+ subd->range_table->list[chan_idx] = lrng;
+
+ return 0;
+}
+
+static inline
+struct comedi_lrange *get_range_table(struct comedi_subdevice *subd,
+ unsigned int chan_idx)
+{
+ chan_idx = (subd->range_table->mode & COMEDI_PERCHAN_LRANGE) ?
+ chan_idx : 0;
+ return subd->range_table->list[chan_idx];
+}
+
+
/* some silly little inline functions */
static inline int alloc_subdevices(struct comedi_device *dev,
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index cabbf09..c0bcff3 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -273,8 +273,9 @@ static int postconfig(struct comedi_device *dev)
comedi_alloc_subdevice_minor(dev, s);
}
- if (!s->range_table && !s->range_table_list)
- s->range_table = &range_unknown;
+ if (!s->range_table)
+ setup_global_range_table
+ (s, (struct comedi_lrange *)&range_unknown);
if (!s->insn_read && s->insn_bits)
s->insn_read = insn_rw_emulate_bits;
diff --git a/drivers/staging/comedi/range.c b/drivers/staging/comedi/range.c
index d09f0bb..04f7b89 100644
--- a/drivers/staging/comedi/range.c
+++ b/drivers/staging/comedi/range.c
@@ -61,15 +61,11 @@ int do_rangeinfo_ioctl(struct comedi_device *dev, struct comedi_rangeinfo *arg)
if (subd >= dev->n_subdevices)
return -EINVAL;
s = dev->subdevices + subd;
- if (s->range_table) {
- lr = s->range_table;
- } else if (s->range_table_list) {
- if (chan >= s->n_chan)
- return -EINVAL;
- lr = s->range_table_list[chan];
- } else {
- return -EINVAL;
- }
+
+ if (s->range_table == NULL || chan >= s->n_chan)
+ return -EINVAL;
+ else
+ lr = get_range_table(s, chan);
if (RANGE_LENGTH(it.range_type) != lr->length) {
DPRINTK("wrong length %d should be %d (0x%08x)\n",
@@ -125,36 +121,24 @@ int check_chanlist(struct comedi_subdevice *s, int n, unsigned int *chanlist)
int i;
int chan;
- if (s->range_table) {
- for (i = 0; i < n; i++)
- if (CR_CHAN(chanlist[i]) >= s->n_chan ||
- CR_RANGE(chanlist[i]) >= s->range_table->length
- || aref_invalid(s, chanlist[i])) {
- rt_printk
- ("bad chanlist[%d]=0x%08x n_chan=%d range length=%d\n",
- i, chanlist[i], s->n_chan,
- s->range_table->length);
-#if 0
- for (i = 0; i < n; i++)
- printk("[%d]=0x%08x\n", i, chanlist[i]);
-#endif
- return -EINVAL;
- }
- } else if (s->range_table_list) {
- for (i = 0; i < n; i++) {
- chan = CR_CHAN(chanlist[i]);
- if (chan >= s->n_chan ||
- CR_RANGE(chanlist[i]) >=
- s->range_table_list[chan]->length
- || aref_invalid(s, chanlist[i])) {
- rt_printk("bad chanlist[%d]=0x%08x\n", i,
- chanlist[i]);
- return -EINVAL;
- }
- }
- } else {
- rt_printk("comedi: (bug) no range type list!\n");
- return -EINVAL;
- }
+ if (s->range_table == NULL) {
+ rt_printk("comedi: (bug) no range type list!\n");
+ return -EINVAL;
+ }
+
+ for (i = 0; i < n; i++) {
+
+ chan = CR_CHAN(chanlist[i]);
+
+ if (chan >= s->n_chan ||
+ CR_RANGE(chanlist[i]) >= get_range_table(s, chan)->length ||
+ aref_invalid(s, chanlist[i])) {
+
+ rt_printk("bad chanlist[%d]=0x%08x\n",
+ i, chanlist[i]);
+ return -EINVAL;
+ }
+ }
+
return 0;
}
--
1.6.2.4
>From 56980bc984730de6aaa65e14e595a2c8c34f3781 Mon Sep 17 00:00:00 2001
From: Alexis Berlemont <alexis.berlemont@xxxxxxxxx>
Date: Sun, 26 Apr 2009 15:50:05 +0200
Subject: [PATCH] Add an internal structure to handle per channel ranges tables.
---
drivers/staging/comedi/comedi_fops.c | 18 ++++++++---
drivers/staging/comedi/comedidev.h | 26 ++++++++++++++++-
drivers/staging/comedi/drivers.c | 2 +-
drivers/staging/comedi/range.c | 53 ++++++++++++++++++++-------------
4 files changed, 71 insertions(+), 28 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index bb4a289..0b9c8e9 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -419,7 +419,8 @@ static int do_subdinfo_ioctl(struct comedi_device *dev, struct comedi_subdinfo *
us->maxdata = s->maxdata;
if (s->range_table) {
us->range_type =
- (i << 24) | (0 << 16) | (s->range_table->length);
+ (i << 24) | (0 << 16) |
+ (s->range_table->length & ~COMEDI_PERCHAN_LRANGE);
} else {
us->range_type = 0; /* XXX */
}
@@ -437,7 +438,8 @@ static int do_subdinfo_ioctl(struct comedi_device *dev, struct comedi_subdinfo *
us->subd_flags |= SDF_MAXDATA;
if (s->flaglist)
us->subd_flags |= SDF_FLAGS;
- if (s->range_table_list)
+ if (s->range_table &&
+ (s->range_table->length & COMEDI_PERCHAN_LRANGE) != 0)
us->subd_flags |= SDF_RANGETYPE;
if (s->do_cmd)
us->subd_flags |= SDF_CMD;
@@ -503,13 +505,19 @@ static int do_chaninfo_ioctl(struct comedi_device *dev, struct comedi_chaninfo *
if (it.rangelist) {
int i;
- if (!s->range_table_list)
- return -EINVAL;
+ struct comedi_trange *trng;
+
+ if(s->range_table == NULL ||
+ (s->range_table->length & COMEDI_PERCHAN_LRANGE) == 0)
+ return -EINVAL;
+
+ trng = (struct comedi_trange *) s->range_table;
+
for (i = 0; i < s->n_chan; i++) {
int x;
x = (dev->minor << 28) | (it.subdev << 24) | (i << 16) |
- (s->range_table_list[i]->length);
+ (trng->list[i]->length & ~COMEDI_PERCHAN_LRANGE);
put_user(x, it.rangelist + i);
}
#if 0
diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index 753d3ab..cc70048 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -150,7 +150,6 @@ struct comedi_subdevice {
unsigned int settling_time_0;
const struct comedi_lrange *range_table;
- const struct comedi_lrange *const *range_table_list;
unsigned int *chanlist; /* driver-owned chanlist (not used) */
@@ -406,11 +405,18 @@ extern const struct comedi_lrange range_unknown;
#define GCC_ZERO_LENGTH_ARRAY 0
#endif
+#define COMEDI_PERCHAN_LRANGE 0x80000000
+
struct comedi_lrange {
int length;
struct comedi_krange range[GCC_ZERO_LENGTH_ARRAY];
};
+ struct comedi_trange {
+ int length;
+ struct comedi_lrange *list[0];
+ };
+
/* some silly little inline functions */
static inline int alloc_subdevices(struct comedi_device *dev,
@@ -440,6 +446,24 @@ static inline int alloc_private(struct comedi_device *dev, int size)
return 0;
}
+static inline int alloc_range_table_list(struct comedi_subdevice * subd)
+{
+ struct comedi_trange * trng =
+ kmalloc(sizeof(struct comedi_trange) +
+ subd->n_chan * sizeof(struct comedi_lrange *),
+ GFP_KERNEL);
+
+ if (trng == NULL)
+ return -ENOMEM;
+
+ memset(trng, 0, sizeof(struct comedi_trange) +
+ subd->n_chan * sizeof(struct comedi_lrange *));
+
+ trng->length = COMEDI_PERCHAN_LRANGE | subd->n_chan;
+
+ return 0;
+}
+
static inline unsigned int bytes_per_sample(const struct comedi_subdevice *subd)
{
if (subd->subdev_flags & SDF_LSAMPL)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index cabbf09..f368548 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -273,7 +273,7 @@ static int postconfig(struct comedi_device *dev)
comedi_alloc_subdevice_minor(dev, s);
}
- if (!s->range_table && !s->range_table_list)
+ if (!s->range_table)
s->range_table = &range_unknown;
if (!s->insn_read && s->insn_bits)
diff --git a/drivers/staging/comedi/range.c b/drivers/staging/comedi/range.c
index d09f0bb..1953c25 100644
--- a/drivers/staging/comedi/range.c
+++ b/drivers/staging/comedi/range.c
@@ -61,14 +61,19 @@ int do_rangeinfo_ioctl(struct comedi_device *dev, struct comedi_rangeinfo *arg)
if (subd >= dev->n_subdevices)
return -EINVAL;
s = dev->subdevices + subd;
- if (s->range_table) {
- lr = s->range_table;
- } else if (s->range_table_list) {
+
+ if (s->range_table == NULL) {
+ return -EINVAL;
+ } else if (s->range_table->length & COMEDI_PERCHAN_LRANGE) {
+ struct comedi_trange *trng;
+
if (chan >= s->n_chan)
return -EINVAL;
- lr = s->range_table_list[chan];
+
+ trng = (struct comedi_trange *) s->range_table;
+ lr = trng->list[chan];
} else {
- return -EINVAL;
+ lr = s->range_table;
}
if (RANGE_LENGTH(it.range_type) != lr->length) {
@@ -125,7 +130,27 @@ int check_chanlist(struct comedi_subdevice *s, int n, unsigned int *chanlist)
int i;
int chan;
- if (s->range_table) {
+ if (s->range_table == NULL) {
+ rt_printk("comedi: (bug) no range type list!\n");
+ return -EINVAL;
+ }
+
+ if (s->range_table->length & COMEDI_PERCHAN_LRANGE) {
+ struct comedi_trange *trng =
+ (struct comedi_trange *) s->range_table;
+
+ for (i = 0; i < n; i++) {
+ chan = CR_CHAN(chanlist[i]);
+ if (chan >= s->n_chan ||
+ CR_RANGE(chanlist[i]) >=
+ trng->list[chan]->length
+ || aref_invalid(s, chanlist[i])) {
+ rt_printk("bad chanlist[%d]=0x%08x\n", i,
+ chanlist[i]);
+ return -EINVAL;
+ }
+ }
+ } else {
for (i = 0; i < n; i++)
if (CR_CHAN(chanlist[i]) >= s->n_chan ||
CR_RANGE(chanlist[i]) >= s->range_table->length
@@ -140,21 +165,7 @@ int check_chanlist(struct comedi_subdevice *s, int n, unsigned int *chanlist)
#endif
return -EINVAL;
}
- } else if (s->range_table_list) {
- for (i = 0; i < n; i++) {
- chan = CR_CHAN(chanlist[i]);
- if (chan >= s->n_chan ||
- CR_RANGE(chanlist[i]) >=
- s->range_table_list[chan]->length
- || aref_invalid(s, chanlist[i])) {
- rt_printk("bad chanlist[%d]=0x%08x\n", i,
- chanlist[i]);
- return -EINVAL;
- }
- }
- } else {
- rt_printk("comedi: (bug) no range type list!\n");
- return -EINVAL;
}
+
return 0;
}
--
1.6.2.4
