lkml.org 
[lkml]   [2008]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2 2/7] mfd: PCF50633 adc driver
    On Fri, Dec 19, 2008 at 11:18:52AM +0000, Jonathan Cameron wrote:
    Hi Jonathan,

    >
    > 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;

    Yes, right. Will change.

    > 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!)

    Yes, the for killing ratiometric measurement can be in _probe. But we
    need to enable accsw everytime because it's turned off automatically
    once a conversion is complete - to save power.

    > > + /* 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?
    >

    Oops! Will fix.

    Thank you for the review.

    Balaji Rao


    \
     
     \ /
      Last update: 2008-12-22 17:25    [W:0.020 / U:122.216 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site