Messages in this thread | | | Date | Thu, 19 Sep 2013 10:24:34 +0500 | From | "Zubair Lutfullah :" <> | Subject | Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support |
| |
On Wed, Sep 18, 2013 at 02:58:47PM +0100, Jonathan Cameron wrote: > On 18/09/13 12:23, Zubair Lutfullah wrote: > >Previously the driver had only one-shot reading functionality. > >This patch adds continuous sampling support to the driver. > > ... > > Very very nearly there. Couple of suggestions in-line. > (about 30 seconds work + testing ;)
Thank-you so much. Makes me want to put in more work and finish it :)
> > I'm still unsure of why we need 32bit storage in the fifo > for the data. I've proposed the changes I'd make to put it in 16 bit > storage inline. The fact that the device is working in 32bits > is irrelevant given we only have a 12 bit number coming out and > it is in 12 least significant bits. I guess there might be a tiny > cost in doing the conversion, but you kfifo will be half the size > as a result and that seems more likely to a worthwhile gain! >
I understand and will make the changes.
> Out of interest, are you testing this with generic_buffer.c? > If so, what changes were necessary? Given this driver will not > have a trigger it would be nice to update that example code to handle > that case if it doesn't already.
I simply remove the lines like goto err_trigger etc. :p Sneaky but works. I wasn't sure how to make the code understand its a INDIO_BUFFER_HARDWARE.. But this is a separate discussion..
> > > >--- > > drivers/iio/adc/ti_am335x_adc.c | 216 +++++++++++++++++++++++++++++++++- > > include/linux/mfd/ti_am335x_tscadc.h | 9 ++ > > 2 files changed, 220 insertions(+), 5 deletions(-) > > > >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c ... > > > > struct tiadc_device { > > struct ti_tscadc_dev *mfd_tscadc; > > int channels; > > u8 channel_line[8]; > > u8 channel_step[8]; > >+ int buffer_en_ch_steps; > >+ u32 *data; > u16 *data; > > Also it might actually save memory to simply have a fixed size array > of the maximum size used here and avoid the extra allocations for > the cost > of 16 bytes in here. > > Hence, > > u16 data[8]; > > };
Why data[8]? The length is dynamic. This would be valid for the usual one sample per trigger case. But here its continuous sampling and the hardware pushes samples *quickly* Dynamic allocation is needed.
...
> >+static irqreturn_t tiadc_worker_h(int irq, void *private) > >+{ > >+ struct iio_dev *indio_dev = private; > >+ struct tiadc_device *adc_dev = iio_priv(indio_dev); > >+ int i, k, fifo1count, read; > >+ u32 *data = adc_dev->data; > u16* data = adc_dev->data; > >+ > >+ fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT); > >+ for (k = 0; k < fifo1count; k = k + i) { > >+ for (i = 0; i < (indio_dev->scan_bytes)/4; i++) { > >+ read = tiadc_readl(adc_dev, REG_FIFO1); > >+ data[i] = read & FIFOREAD_DATA_MASK; > //This is a 12 bit number after the mask so will fit just fine into 16 bits.
Indeed ... > >+ > >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev) > >+{ > >+ struct tiadc_device *adc_dev = iio_priv(indio_dev); > >+ struct iio_buffer *buffer = indio_dev->buffer; > >+ unsigned int enb = 0; > >+ u8 bit; > >+ > (can drop this if doing the array with adc_dev as suggested above) > >+ adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); > >+ if (adc_dev->data == NULL) > >+ return -ENOMEM;
As explained previously. Large array.. This is needed..
... > >+{ > >+ struct tiadc_device *adc_dev = iio_priv(indio_dev); > >+ > >+ tiadc_step_config(indio_dev); > (can drop this if doing the array withing adc_dev as suggested above) > >+ kfree(adc_dev->data); > This is also missbalanced with the preenable (which is true of quite > a few drivers - one day I'll clean those up!)
Misbalanced? :s
> >+ > >+ return 0; > >+} >
Thanks for the review and feedback. I'll resend the patches with 16 bit everything and dynamic allocation.
Zubair >
| |