Messages in this thread | | | Date | Fri, 19 Dec 2008 11:18:52 +0000 | From | Jonathan Cameron <> | Subject | Re: [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
| |