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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
