Date: Thu, 19 Sep 2013 10:24:34 +0500
From: "Zubair Lutfullah :" <zubair.lutfullah@gmail.com>
To: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
Cc: Zubair Lutfullah <zubair.lutfullah@gmail.com>, jic23@cam.ac.uk,
        dmitry.torokhov@gmail.com, linux-iio@vger.kernel.org,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        bigeasy@linutronix.de, gregkh@linuxfoundation.org
Subject: Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
Message-ID: <20130919052419.GB4363@gmail.com>
References: <1379503383-17086-1-git-send-email-zubair.lutfullah@gmail.com>
 <1379503383-17086-3-git-send-email-zubair.lutfullah@gmail.com>
 <5239B197.2040807@jic23.retrosnub.co.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5239B197.2040807@jic23.retrosnub.co.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4094
Lines: 131

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/