lkml.org 
[lkml]   [2008]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2 2/7] mfd: PCF50633 adc driver
Hi Balaji,

Nice driver. I particularly like the queueing structure. In the IIO
design I hadn't come across any devices that operate in this request
then wait for interrupt fashion. May need to rethink a few bits of
that.

Couple of minor suggestions below.

> This patch adds basic support for the PCF50633 ADC. The subtractive mode
> is not supported yet.
>
> Since we don't have adc subsystem, it currently lives in drivers/mfd.
>
> Signed-off-by: Balaji Rao <balajirrao@openmoko.org>
> Cc: Andy Green <andy@openmoko.com>
...
> --- /dev/null
> +++ b/drivers/mfd/pcf50633-adc.c
> @@ -0,0 +1,277 @@
> +/* NXP PCF50633 ADC Driver
> + *
> + * (C) 2006-2008 by Openmoko, Inc.
> + * Author: Balaji Rao <balajirrao@openmoko.org>
> + * All rights reserved.
> + *
> + * Broken down from monstrous PCF50633 driver mainly by
> + * Harald Welte, Andy Green and Werner Almesberger
> + *
> + * This program is free software; you can redistribute it and/or
modify it
> + * under the terms of the GNU General Public License as published
by the
> + * Free Software Foundation; either version 2 of the License, or
(at your
> + * option) any later version.
> + *
> + * NOTE: This driver does not yet support subtractive ADC mode, which
means
> + * you can do only one measurement per read request.
> + */
...

This is confusingly named. To my mind it is writing the setup
to the device not reading it.

> +static void adc_read_setup(struct pcf50633 *pcf, int channel, int avg)
> +{
> + channel &= PCF50633_ADCC1_ADCMUX_MASK;
This needs a bit more explanation. Particularly as the data
sheet describes that accsw as 'for rationmetric measurement'.
Also, seeing as I assume this is the only driver that can touch
these registers and you don't change them else where, why can't
they be in initial setup code rather than here? (probably a good
reason, but be nice to have it document here!)
> + /* kill ratiometric, but enable ACCSW biasing */
> + pcf50633_reg_write(pcf, PCF50633_REG_ADCC2, 0x00);
> + pcf50633_reg_write(pcf, PCF50633_REG_ADCC3, 0x01);
> +
> + /* start ADC conversion on selected channel */
> + pcf50633_reg_write(pcf, PCF50633_REG_ADCC1, channel | avg |
...
> +
> +static void pcf50633_adc_irq(int irq, void *data)
> +{
> + struct pcf50633_adc *adc = data;
> + struct pcf50633 *pcf = adc->pcf;
> + struct pcf50633_adc_request *req;
> + int head;
> + mutex_lock(&adc->queue_mutex);
> + head = adc->queue_head;
> +
> + req = adc->queue[head];
> + if (WARN_ON(!req)) {
> + dev_err(pcf->dev, "pcf50633-adc irq: ADC queue empty!\n");
> + mutex_unlock(&adc->queue_mutex);
> + return;
> + }
> + adc->queue[head] = NULL;

Weird formatting?

> + adc->queue_head = (head + 1) &
> + (PCF50633_MAX_ADC_FIFO_DEPTH - 1);
> +
> + mutex_unlock(&adc->queue_mutex);
> + req->callback(pcf, req->callback_param, adc_result(pcf));
> + kfree(req);
> +
> + trigger_next_adc_job_if_any(pcf);
> +}
> +
Rest looks good to me.

Acked-by: Jonathan Cameron



\
 
 \ /
  Last update: 2008-12-19 12:49    [W:2.502 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site