Messages in this thread | | | Date | Mon, 22 Dec 2008 21:53:01 +0530 | From | Balaji Rao <> | Subject | Re: [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
| |