Re: [PATCH V2 2/7] mfd: PCF50633 adc driver

[Jonathan Cameron <Jonathan.Cameron@gmail.com>]

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