Range_table and range_table_list
Alexis Berlemont
Please, find attached two patches which try to manage subdevice's ranges
lists in a unique field (inside the subdevice structure) instead of two
(range_table and range_table_list).
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:
- the first patch 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 not that clean nor perfect, they are just some ideas I
would like to share.
Best regards.
Alexis.
Alexis Berlemont
Here is a mail I sent last week. It contains some proposals related with
the ranges declarations into subdevices.
Alexis.
Alexis Berlemont <alexis.b...@free.fr> writes:
> diff -uNrp comedi-0.7.76/comedi/comedi_fops.c comedi-0.7.76.modif/comedi/comedi_fops.c
> --- comedi-0.7.76/comedi/comedi_fops.c 2009-04-07 23:33:05.000000000 +0200
> +++ comedi-0.7.76.modif/comedi/comedi_fops.c 2009-04-16 01:17:13.000000000 +0200
> @@ -410,8 +410,8 @@ static int do_subdinfo_ioctl(comedi_devi
> if (s->range_table) {
> us->range_type =
> (dev->
> - minor << 28) | (i << 24) | (0 << 16) | (s->
> - range_table->length);
> + minor << 28) | (i << 24) | (0 << 16) |
> + get_range_table(s, 0)->length;
> } else {
> us->range_type = 0; /* XXX */
> }
> @@ -429,7 +429,8 @@ static int do_subdinfo_ioctl(comedi_devi
> 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;
> @@ -490,13 +491,16 @@ static int do_chaninfo_ioctl(comedi_devi
> 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(copy_to_user(it.rangelist,s->range_type_list,s->n_chan*sizeof(unsigned int)))
> diff -uNrp comedi-0.7.76/comedi/drivers.c comedi-0.7.76.modif/comedi/drivers.c
> --- comedi-0.7.76/comedi/drivers.c 2007-12-13 20:25:17.000000000 +0100
> +++ comedi-0.7.76.modif/comedi/drivers.c 2009-04-16 22:58:50.000000000 +0200
> @@ -296,8 +296,9 @@ static int postconfig(comedi_device * de
> dev->minor, i);
> }
>
> - if (!s->range_table && !s->range_table_list)
> - s->range_table = &range_unknown;
> + if (!s->range_table)
> + setup_global_range_table
> + (s, (comedi_lrange *)&range_unknown);
>
> if (!s->insn_read && s->insn_bits)
> s->insn_read = insn_rw_emulate_bits;
> diff -uNrp comedi-0.7.76/comedi/range.c comedi-0.7.76.modif/comedi/range.c
> --- comedi-0.7.76/comedi/range.c 2007-11-06 20:28:25.000000000 +0100
> +++ comedi-0.7.76.modif/comedi/range.c 2009-04-16 01:36:09.000000000 +0200
> @@ -68,16 +68,12 @@ int do_rangeinfo_ioctl(comedi_device * d
> if (subd >= query_dev->n_subdevices)
> return -EINVAL;
> s = query_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",
> RANGE_LENGTH(it.range_type), lr->length, it.range_type);
> @@ -132,37 +128,24 @@ int check_chanlist(comedi_subdevice * s,
> 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 {
> + 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;
> }
> diff -uNrp comedi-0.7.76/include/linux/comedidev.h comedi-0.7.76.modif/include/linux/comedidev.h
> --- comedi-0.7.76/include/linux/comedidev.h 2007-12-13 20:25:22.000000000 +0100
> +++ comedi-0.7.76.modif/include/linux/comedidev.h 2009-04-16 01:38:43.000000000 +0200
> @@ -119,8 +119,7 @@ struct comedi_subdevice_struct {
>
> unsigned int settling_time_0;
>
> - const comedi_lrange *range_table;
> - const comedi_lrange *const *range_table_list;
> + struct comedi_crange_struct *range_table;
>
> unsigned int *chanlist; /* driver-owned chanlist (not used) */
>
> @@ -426,6 +425,77 @@ struct comedi_lrange_struct {
> comedi_krange range[GCC_ZERO_LENGTH_ARRAY];
> };
>
> +#define COMEDI_PERCHAN_LRANGE 0x80000000
> +
> +struct comedi_crange_struct {
> + unsigned int mode;
> + struct comedi_lrange_struct *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(comedi_subdevice * subd,
> + comedi_lrange *lrng)
> +{
> + subd->range_table = kmalloc(sizeof(struct comedi_crange_struct) +
> + sizeof(struct comedi_lrange_struct *),
> + 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(comedi_subdevice * subd)
> +{
> + subd->range_table = kmalloc(sizeof(struct comedi_crange_struct) +
> + sizeof(struct comedi_lrange_struct *) *
> + subd->n_chan,
> + GFP_KERNEL);
> +
> + if(subd->range_table == NULL)
> + return -ENOMEM;
> +
> + memset(subd->range_table, 0,
> + sizeof(struct comedi_crange_struct) +
> + sizeof(struct comedi_lrange_struct *) * subd->n_chan);
> +
> + subd->range_table->mode = COMEDI_PERCHAN_LRANGE;
> +
> + return 0;
> +}
> +
> +static inline int set_range_table(comedi_subdevice *subd,
> + unsigned int chan_idx,
> + 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 comedi_lrange *get_range_table(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(comedi_device * dev,
>
>
> diff -uNrp comedi-0.7.76/comedi/comedi_fops.c comedi-0.7.76.modif/comedi/comedi_fops.c
> --- comedi-0.7.76/comedi/comedi_fops.c 2009-04-07 23:33:05.000000000 +0200
> +++ comedi-0.7.76.modif/comedi/comedi_fops.c 2009-04-15 00:46:27.000000000 +0200
> @@ -410,8 +410,8 @@ static int do_subdinfo_ioctl(comedi_devi
> if (s->range_table) {
> us->range_type =
> (dev->
> - minor << 28) | (i << 24) | (0 << 16) | (s->
> - range_table->length);
> + minor << 28) | (i << 24) | (0 << 16) |
> + (s->range_table->length & ~COMEDI_PERCHAN_LRANGE);
> } else {
> us->range_type = 0; /* XXX */
> }
> @@ -429,7 +429,8 @@ static int do_subdinfo_ioctl(comedi_devi
> 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;
> @@ -489,14 +490,20 @@ static int do_chaninfo_ioctl(comedi_devi
>
> if (it.rangelist) {
> int i;
> + struct comedi_trange_struct *trng;
>
> - if (!s->range_table_list)
> + if(s->range_table == NULL ||
> + (s->range_table->length & COMEDI_PERCHAN_LRANGE) == 0)
> return -EINVAL;
> +
> + trng = (struct comedi_trange_struct *) 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(copy_to_user(it.rangelist,s->range_type_list,s->n_chan*sizeof(unsigned int)))
> diff -uNrp comedi-0.7.76/comedi/drivers.c comedi-0.7.76.modif/comedi/drivers.c
> --- comedi-0.7.76/comedi/drivers.c 2007-12-13 20:25:17.000000000 +0100
> +++ comedi-0.7.76.modif/comedi/drivers.c 2009-04-15 01:02:15.000000000 +0200
> @@ -296,7 +296,7 @@ static int postconfig(comedi_device * de
> dev->minor, i);
> }
>
> - 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 -uNrp comedi-0.7.76/comedi/range.c comedi-0.7.76.modif/comedi/range.c
> --- comedi-0.7.76/comedi/range.c 2007-11-06 20:28:25.000000000 +0100
> +++ comedi-0.7.76.modif/comedi/range.c 2009-04-15 01:01:54.000000000 +0200
> @@ -68,16 +68,20 @@ int do_rangeinfo_ioctl(comedi_device * d
> if (subd >= query_dev->n_subdevices)
> return -EINVAL;
> s = query_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_struct *trng;
> if (chan >= s->n_chan)
> return -EINVAL;
> - lr = s->range_table_list[chan];
> +
> + trng = (struct comedi_trange_struct *) s->range_table;
> + lr = trng->list[chan];
> } else {
> - return -EINVAL;
> + lr = s->range_table;
> }
> -
> +
> if (RANGE_LENGTH(it.range_type) != lr->length) {
> DPRINTK("wrong length %d should be %d (0x%08x)\n",
> RANGE_LENGTH(it.range_type), lr->length, it.range_type);
> @@ -132,7 +136,27 @@ int check_chanlist(comedi_subdevice * s,
> 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_struct *trng =
> + (struct comedi_trange_struct *) 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
> @@ -148,21 +172,7 @@ int check_chanlist(comedi_subdevice * s,
> #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;
> }
> diff -uNrp comedi-0.7.76/include/linux/comedidev.h comedi-0.7.76.modif/include/linux/comedidev.h
> --- comedi-0.7.76/include/linux/comedidev.h 2007-12-13 20:25:22.000000000 +0100
> +++ comedi-0.7.76.modif/include/linux/comedidev.h 2009-04-16 00:13:57.000000000 +0200
> @@ -120,7 +120,6 @@ struct comedi_subdevice_struct {
> unsigned int settling_time_0;
>
> const comedi_lrange *range_table;
> - const comedi_lrange *const *range_table_list;
>
> unsigned int *chanlist; /* driver-owned chanlist (not used) */
>
> @@ -421,11 +420,18 @@ extern const comedi_lrange range_unknown
> #define GCC_ZERO_LENGTH_ARRAY 0
> #endif
>
> +#define COMEDI_PERCHAN_LRANGE 0x80000000
> +
> struct comedi_lrange_struct {
> int length;
> comedi_krange range[GCC_ZERO_LENGTH_ARRAY];
> };
>
> +struct comedi_trange_struct {
> + int length;
> + struct comedi_lrange_struct *list[0];
> +};
> +
> /* some silly little inline functions */
>
> static inline int alloc_subdevices(comedi_device * dev,
> @@ -454,6 +460,24 @@ static inline int alloc_private(comedi_d
> return 0;
> }
>
> +static inline int alloc_range_table_list(comedi_subdevice * subd)
> +{
> + struct comedi_trange_struct * trng =
> + kmalloc(sizeof(struct comedi_trange_struct) +
> + subd->n_chan * sizeof(struct comedi_lrange_struct *),
> + GFP_KERNEL);
> +
> + if (trng == NULL)
> + return -ENOMEM;
> +
> + memset(trng, 0, sizeof(struct comedi_trange_struct) +
> + subd->n_chan * sizeof(struct comedi_lrange_struct *));
> +
> + trng->length = COMEDI_PERCHAN_LRANGE | subd->n_chan;
> +
> + return 0;
> +}
> +
> static inline unsigned int bytes_per_sample(const comedi_subdevice * subd)
> {
> if (subd->subdev_flags & SDF_LSAMPL)
Ian Abbott
> Hi,
>
> Here is a mail I sent last week. It contains some proposals related with
> the ranges declarations into subdevices.
>
> Alexis.
I don't think we'll use this patch in CVS, but there are people busy
migrating the Comedi code into the Linux kernel sources and cleaning it
up. This is happening in the staging area of the "linux-next" tree. I
think any clean up effort should take place on that version of the
Comedi code.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abb...@mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
Alexis Berlemont
Many thanks for the answer.
Ian Abbott <abb...@mev.co.uk> writes:
> Alexis Berlemont wrote:
>> Hi,
>>
>> Here is a mail I sent last week. It contains some proposals related with
>> the ranges declarations into subdevices.
>>
>> Alexis.
>
> I don't think we'll use this patch in CVS, but there are people busy
> migrating the Comedi code into the Linux kernel sources and cleaning it
> up. This is happening in the staging area of the "linux-next" tree. I
> think any clean up effort should take place on that version of the
> Comedi code.
Does that mean that the CVS tree will be left unused once Comedi will be
fully integrated into Linux?
In such a case, the co-kernel code (RTLinux, RTAI) is bound to be
dropped from the linux-next version. I am afraid this feature will not
be tolerated in mainline.
Do you consider giving up RTLinux and RTAI support ?
Best regards.
Alexis.
Ian Abbott
> Ian Abbott <abb...@mev.co.uk> writes:
>
>> Alexis Berlemont wrote:
>>> Hi,
>>>
>>> Here is a mail I sent last week. It contains some proposals related with
>>> the ranges declarations into subdevices.
>>>
>>> Alexis.
>> I don't think we'll use this patch in CVS, but there are people busy
>> migrating the Comedi code into the Linux kernel sources and cleaning it
>> up. This is happening in the staging area of the "linux-next" tree. I
>> think any clean up effort should take place on that version of the
>> Comedi code.
>
> Does that mean that the CVS tree will be left unused once Comedi will be
> fully integrated into Linux?
I guess it will be left around to support older kernels, but probably
won't get much new stuff added apart from bug fixes and perhaps the odd
backport.
> In such a case, the co-kernel code (RTLinux, RTAI) is bound to be
> dropped from the linux-next version. I am afraid this feature will not
> be tolerated in mainline.
I think you're right, but as long as the abstractions that Comedi uses
are left behind by the mainline changes (things like
comedi_request_irq(), comedi_spin_lock_irqsave(), comedi_switch_to_rt(),
etc.) then it would be easy enough for those abstractions to be filled
in by future RTAI or RTLinux patches.