| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 19 Sep 2013 06:41:22 +0100
From: Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To: "Zubair Lutfullah :" <zubair.lutfullah@...il.com>
CC: jic23@....ac.uk, dmitry.torokhov@...il.com,
linux-iio@...r.kernel.org, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, bigeasy@...utronix.de,
gregkh@...uxfoundation.org
Subject: Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
"Zubair Lutfullah :" <zubair.lutfullah@...il.com> wrote:
>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.
As far as I can see you pull one set of channels into data[] then push that out to the kfifo.
That is repeated lots of times but only up to 8 entries are ever used in this array. If not what is the maximum scanbytes can be?
>
>
>...
>
>> >+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;
i is only ever up to scanbytes / 4.
Hence max of 8 I think?
Now there is a good argument for adding some bulk filling code for the buffers but that is not happening here.
>> //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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists