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: <494B98CC.4050700@gmail.com>
Date:	Fri, 19 Dec 2008 12:51:24 +0000
From:	Jonathan Cameron <Jonathan.Cameron@...il.com>
To:	Andy Green <andy@...nmoko.com>
CC:	Balaji Rao <balajirrao@...nmoko.org>, linux-kernel@...r.kernel.org,
	Samuel Ortiz <sameo@...nedhand.com>
Subject: Re: [PATCH V2 2/7] mfd: PCF50633 adc driver

Andy Green wrote:
> Somebody in the thread at some point said:
>
> Hi Johnathan -
>
> | This needs a bit more explanation. Particularly as the data
> | sheet describes that accsw as 'for rationmetric measurement'.
>
> What's going on here is that ACCSW can be driven from the PMU to bias a
> resistor divider to scale what's being measured... we use it to detect
> resistor to 0V on ID pin of USB on our charger.
>
> You can power accsw / that divider "by hand" which is what we're doing
> here, or you can have the ratiometric state machine change the mux /
> bias automatically between the two measurements it takes, which we're
> not doing.
That makes sense. Thanks for the explanation.
>
> | 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!)
>
> ADCC2 setting concerned with ratiometric disable could indeed be moved
> to init, it was done like this before I did the queuing stuff and we
> took ADC measurement in two different places in the code.  But in fact
> for the best we should only enable accsw until the measurement completes
> and save a tiny bit of power.
Either way sounds fine to me as I'd be surprised if it is a significant
power
drain and anyway it would come at the cost of a couple more register writes
which, if the bus is slow, would be more of an issue.

>
> |> +    /* 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);
>
> This would save the power
>
> ~    pcf50633_reg_write(pcf, PCF50633_REG_ADCC3, 0x0);
>
> |> +    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
> |
>
> -Andy

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