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

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

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