On 24/06/2013 08:41, Lars-Peter Clausen wrote:
> On 06/20/2013 08:57 PM, Alexandre Belloni wrote:
>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>> gain and sampling rates.
>>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
>> ---
>> .../bindings/iio/adc/nuvoton-nau7802.txt | 17 +
>> drivers/iio/adc/Kconfig | 9 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/nau7802.c | 603 +++++++++++++++++++++
>> 4 files changed, 630 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>> create mode 100644 drivers/iio/adc/nau7802.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>> new file mode 100644
>> index 0000000..9bc4218
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>> @@ -0,0 +1,17 @@
>> +* Nuvoton NAU7802 Analog to Digital Converter (ADC)use
>> +
>> +Required properties:
>> + - compatible: Should be "nuvoton,nau7802"
>> + - reg: Should contain the ADC I2C address
>> +
>> +Optional properties:
>> + - nuvoton,vldo: Reference voltage in millivolts (integer)
>> + - interrupts: IRQ line for the ADC. If not used the driver will use
>> + polling.
>> +
>> +Example:
>> +adc2: nau7802@2a {
>> + compatible = "nuvoton,nau7802";
>> + reg = <0x2a>;
>> + nuvoton,vldo = <3000>;
> We usually use the regulator framework for specifying the reference voltage.
I followed what Jonathan said in his review of my first patch. Do we
want to use the regulator framework to set the internal reference
voltage of the ADC ? I agree that if you supply an external voltage, it
will be necessary to use the regulator framework. Unfortunately, I can't
test that here.
>> +};
> [...]
>> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
>> new file mode 100644
>> index 0000000..e1b6981
>> --- /dev/null
>> +++ b/drivers/iio/adc/nau7802.c
>> @@ -0,0 +1,603 @@
> [...]
>> +static int nau7802_set_gain(struct nau7802_state *st, int gain)
>> +{
>> + u8 data;
>> + int ret;
>> +
>> + mutex_lock(&st->lock);
>> + st->conversion_count = 0;
>> +
>> + data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
>> + if (data < 0)
>> + goto nau7802_sysfs_set_gain_out;
> ret will be uninitialized if the goto above is taken
Right, bigger issue, data is u8 so it will never be negative. I'm fixing
that !
>> + ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1,
>> + (data & (~NAU7802_CTRL1_GAINS_BITS)) |
>> + gain);
>> +
>> +nau7802_sysfs_set_gain_out:
>> + mutex_unlock(&st->lock);
>> +
>> + return ret ? ret : 0;
>> +}
> [...]
>> +static int nau7802_read_irq(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val)
>> +{
>> + struct nau7802_state *st = iio_priv(indio_dev);
>> + int ret;
>> +
>> + INIT_COMPLETION(st->value_ok);
>> + enable_irq(st->client->irq);
> Is it really necessary to enable/disable the IRQ or could you keep it
> enabled all the time?
Fact is that the ADC doesn't really care if you are reading data or not
so you will probably endd up in a situation were you will get 320 IRQ
per second but not caring about the result. We have 3 ADCs on the board.
so that amounts to 960 IRQ per second when we are only reading like once
par second !
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
