Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Alexandre Belloni]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/