On Fri, 2008-06-27 at 10:45 +0100, Jonathan Cameron wrote:
> Ben Nizette wrote:
> > On Thu, 2008-06-26 at 19:01 +0100, Jonathan Cameron wrote:
> >
> >> Sysfs - Parameter Control - gain / offsets etc
> >> State control, turn interrupts on and off etc.
> >
> > As in turn userspace [interrupt] event notification on and off? I would
> > have thought it'd be the kernel driver's responsibility to turn the
> > device's interrupt generation on and off according to needs for
> > data/events etc.
> Ok, there is a division here between interrupt handling on the host side
> which indeed should be turned on and off transparently by the driver
> and actually telling the sensor which interrupts to generate. In a sense
> this is simply a case of terminology and what is actually being requested
> is event notifications (which then match with those sent up to userspace).
Righteo, I suspected as much :-)
< snip good replies :-) >
>
> > also:
> >
> > +/* As the ring buffer contents are device dependent this functionality
> > + * must remain part of the driver and not the ring buffer subsystem */
> > +static ssize_t
> > +lis3l02dq_read_accel_from_ring(struct industrialio_ring_buffer *ring,
> > + int element, char *buf)
> > +{
> > + int val, ret, len;
> > + uint16_t temp;
> > + char *data, *datalock;
> > +
> > + data = kmalloc(8, GFP_KERNEL);
> > + if (data == NULL) {
> > + ret = -ENOMEM;
> > + goto error_ret;
> > + }
> > + ret = industrialio_read_last_from_ring(ring, data);
> > + datalock = data + 2*element;
> >
> > I haven't looked deeply at the ringbuffer code but can you guarantee
> > that later elements are at higher addresses than the lower ones? As in,
> > can one datum in the the ring buffer wrap to the beginning again?
> At the moment the ring buffer has to be a whole number of datums big.
> I'm inclined to keep it way and move more towards dynamic allocation
> such that it is true than to try handling split data reading sets.
Yup, agreed. Just so long as your code thinks about :-)
>
> > + kfree(data);
> >
> > You free the data before you use it? Though you are using it through a
> > different pointer below. I wouldn't be scared of allocating 8 bytes on
> > the stack rather than kmalloc'ing (unless you expect this to be called
> > in a deep callchain)
> Ouch. That's an out and out bug!
> >
> > + temp = (((uint16_t)((datalock[1]))) << 8)
> > + | (uint16_t)(datalock[0]);
> > + val = *((int16_t *)(&temp));
> >
> > All this data/datalock/bitshuffle nonsense would be nicer if you just
> > used structs and unions, yeah?
> >
> > union channel {
> > char data[2];
> > int16_t val;
> > }
> Good point, I'd forgotten you could do that with unions.
Cool, just watch endianness of course :-)
> >
> > struct datum {
> > union channel elements[3];
> > }
> >
> > or something.
> >
> > + len = sprintf(buf, "ring %d\n", val);
> > +
> > + return len;
> > +error_ret:
> > + return ret;
> > +}
> Good approach, I'll switch to that.
>
> >
> > Incidentally, is there much that your ringbuffer can do which kfifo
> > can't? Apart from having a bunch of extra nice accessor-helpers sitting
> > on the top.
> Not sure, I'll look into it.
kfifo won't be a drop in replacement, it's just a very simple ring fifo.
I suspect your higher level ring buffer accessors and allocators could
live on top of it though.
> >
> > Overall looking good and useful, can't wait 'till it's done :-)
> Thanks for the comments.
>
> Jonathan
np,
--Ben.
