lkml.org 
[lkml]   [2008]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [lm-sensors] Accelerometer etc subsystem (Update on progress)
    From
    Date

    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.



    \
     
     \ /
      Last update: 2008-06-28 10:37    [W:0.033 / U:1.864 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site