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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081222162300.GC3073@fedora.yogi>
Date:	Mon, 22 Dec 2008 21:53:01 +0530
From:	Balaji Rao <balajirrao@...nmoko.org>
To:	Jonathan Cameron <Jonathan.Cameron@...il.com>
Cc:	linux-kernel@...r.kernel.org, Andy Green <andy@...nmoko.com>,
	Samuel Ortiz <sameo@...nedhand.com>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/