From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lars-Peter Clausen Subject: Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver Date: Mon, 24 Jun 2013 18:41:22 +0200 Message-ID: <51C876B2.1050103@metafoo.de> References: <1371754675-12242-1-git-send-email-alexandre.belloni@free-electrons.com> <1371754675-12242-2-git-send-email-alexandre.belloni@free-electrons.com> <51C7EA23.1040707@metafoo.de> <51C82169.1010203@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51C82169.1010203-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Sender: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Alexandre Belloni Cc: Shawn Guo , Grant Likely , jimwall@q.com, brian-ZKiFAVwZFM2FeswfMrDH8w@public.gmane.org, Maxime Ripard , linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Jonathan Cameron , Rob Landley , Rob Herring List-Id: devicetree@vger.kernel.org On 06/24/2013 12:37 PM, Alexandre Belloni wrote: > 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 >>> Signed-off-by: Maxime Ripard >>> --- >>> .../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. > Ah, ok I missed that it is an internally generated voltage. It might makes sense to add that to the properties documentation. I guess ideally you'd also register a regulator for the internal regulator and then use that. But I think that will unnecessarily complicate the code, so I guess the current solution is fine. There is one bug in probe though, if nuvoton,vldo is not set tmp will remain uninitialized. >>> +}; >> [...] >>> 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 ! Ah, ok, makes sense.