Re: [PATCH v3] iio: adc: add ADC12130/ADC12132/ADC12138 ADC driver

[Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On 20/08/16 03:54, Akinobu Mita wrote:
> Hi Rob,
> 
> 2016-08-16 1:42 GMT+09:00 Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>> On 07/08/16 16:22, Akinobu Mita wrote:
>>> This adds Texas Instruments' ADC12130/ADC12132/ADC12138 12-bit plus
>>> sign ADC driver.  I have tested with the ADC12138.  The ADC12130 and
>>> ADC12132 are not tested but these are similar to ADC12138 except that
>>> the mode programming instruction is a bit different.
>>>
>>> Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Hi,
>>
>> Now had time for a closer look.  A few minor bits and bobs to fix
>> up highlighted inline.
>>
>> Looks pretty good.
>>
>> Jonathan
>>> Cc: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>
>>> Cc: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
>>> Cc: Peter Meerwald <pmeerw-jW+XmwGofnusTnJN9+BGXg@public.gmane.org>
>>> ---
>>> * Changes from v2
>>> - improve error label names
>>>
>>>  .../devicetree/bindings/iio/adc/ti-adc12138.txt    |  32 ++
>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/ti-adc12138.c                      | 542 +++++++++++++++++++++
>>>  4 files changed, 587 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc12138.txt
>>>  create mode 100644 drivers/iio/adc/ti-adc12138.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc12138.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc12138.txt
>>> new file mode 100644
>>> index 0000000..3a11d2a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc12138.txt
>>> @@ -0,0 +1,32 @@
>>> +* Texas Instruments' ADC12130/ADC12132/ADC12138
>>> +
>>> +Required properties:
>>> + - compatible: Should be one of
>>> +     * "ti,adc12130"
>>> +     * "ti,adc12132"
>>> +     * "ti,adc12138"
>>> + - reg: SPI chip select number for the device
>>> + - interrupts: Should contain interrupt for EOC (end of conversion)
>>> + - clocks: phandle to conversion clock input
>>> + - spi-max-frequency: Definision as per
>>> +     Documentation/devicetree/bindings/spi/spi-bus.txt
>>> + - vref-p-supply: The regulator supply for positive analog voltage reference
>>> +
>>> +Optional properties:
>>> + - vref-n-supply: The regulator supply for negative analog voltage reference
>>> +     If not specified, this is assumed to be analog ground.
>> This is novel. Documented in the datasheet as a negative reference which
>> must be positive (greater than 0) for it to work well.
>>
>> Intersting question on whether this should be optional. Easy enough to
>> provided a fixed regulator at 0V afterall.
>>
>>
>>> + - acquisition-time: The number of conversion clock periods for the S/H's
>>> +     acquisition time.  Should be one of 6, 10, 18, 34.  If not specified,
>>> +     default value of 10 is used.
>> Should this be a ti specific parameter.  Not sure.
> 
> Is it better to change this parameter to "ti,acquisition-time" ?
I think so. If not it needs to go in generic docs rather than this file then
be appropriately cross referenced from here. I'd keep it as ti specific for now.
We can generalize later if it makes sense (though will have to support this
as well which isn't too difficult).
> 
>> I'd also add a note here about why one would set this to other than the
>> default (source impedance etc).
>>
>>
>>> +
>>> +Example:
>>> +adc@0 {
>>> +     compatible = "ti,adc12138";
>>> +     reg = <0>;
>>> +     interrupts = <28 IRQ_TYPE_EDGE_RISING>;
>>> +     interrupt-parent = <&gpio1>;
>>> +     clocks = <&cclk>;
>>> +     vref-p-supply = <&ldo4_reg>;
>>> +     spi-max-frequency = <5000000>;
>>> +     acquisition-time = <6>;
>>> +};
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>