| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <51726541.6060304@kernel.org>
Date: Sat, 20 Apr 2013 10:52:01 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
CC: Shawn Guo <shawn.guo@...aro.org>,
Grant Likely <grant.likely@...retlab.ca>, jimwall@...om,
brian@...stalfontz.com,
Maxime Ripard <maxime.ripard@...e-electrons.com>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
devicetree-discuss@...ts.ozlabs.org,
Jonathan Cameron <jic23@....ac.uk>,
Rob Landley <rob@...dley.net>,
Rob Herring <rob.herring@...xeda.com>
Subject: Re: [PATCH 1/3] iio: Add Nuvoton NAU7802 ADC driver
On 04/18/2013 04:38 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@...e-electrons.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
A few bits and pieces inline.
Ouch, that interrupt handling is annoyingly complex. Pesky hardware
designers...
> ---
> .../bindings/iio/adc/nuvoton-nau7802.txt | 17 +
> drivers/iio/adc/Kconfig | 9 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/nau7802.c | 644 ++++++++++++++++++++
> 4 files changed, 671 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)
> +
> +Required properties:
> + - compatible: Should be "nuvoton,nau7802"
> + - reg: Should contain the ADC I2C address
> +
> +Optional properties:
> + - nuvoton,vldo: Reference voltage in millivolts (integer)
As this is external I'd personally prefer using the regulator subsystem
to provide it. That gives a more flexible way of supplying it.
If fixed you can always use a fixed regulator...
> + - 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>;
> +};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index e372257..ed3059a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -113,6 +113,15 @@ config MAX1363
> max11646, max11647) Provides direct access via sysfs and buffered
> data via the iio dev interface.
>
> +config NAU7802
> + tristate "Nuvoton NAU7802 ADC driver"
> + depends on I2C
> + help
> + Say yes here to build support for Nuvoton NAU7802 ADC.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called nau7802.
> +
> config TI_ADC081C
> tristate "Texas Instruments ADC081C021/027"
> depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 2d5f100..a02150d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
> obj-$(CONFIG_AT91_ADC) += at91_adc.o
> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_MAX1363) += max1363.o
> +obj-$(CONFIG_NAU7802) += nau7802.o
> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
> new file mode 100644
> index 0000000..0148fd8
> --- /dev/null
> +++ b/drivers/iio/adc/nau7802.c
> @@ -0,0 +1,644 @@
> +/*
> + * Driver for the Nuvoton NAU7802 ADC
> + *
> + * Copyright 2013 Free Electrons
> + *
> + * Licensed under the GPLv2 or later.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/wait.h>
> +#include <linux/of_irq.h>
> +#include <linux/log2.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define NAU7802_REG_PUCTRL 0x00
> +#define NAU7802_PUCTRL_RR(x) (x << 0)
> +#define NAU7802_PUCTRL_RR_BIT NAU7802_PUCTRL_RR(1)
> +#define NAU7802_PUCTRL_PUD(x) (x << 1)
> +#define NAU7802_PUCTRL_PUD_BIT NAU7802_PUCTRL_PUD(1)
> +#define NAU7802_PUCTRL_PUA(x) (x << 2)
> +#define NAU7802_PUCTRL_PUA_BIT NAU7802_PUCTRL_PUA(1)
> +#define NAU7802_PUCTRL_PUR(x) (x << 3)
> +#define NAU7802_PUCTRL_PUR_BIT NAU7802_PUCTRL_PUR(1)
> +#define NAU7802_PUCTRL_CS(x) (x << 4)
> +#define NAU7802_PUCTRL_CS_BIT NAU7802_PUCTRL_CS(1)
> +#define NAU7802_PUCTRL_CR(x) (x << 5)
> +#define NAU7802_PUCTRL_CR_BIT NAU7802_PUCTRL_CR(1)
> +#define NAU7802_PUCTRL_AVDDS(x) (x << 7)
> +#define NAU7802_PUCTRL_AVDDS_BIT NAU7802_PUCTRL_AVDDS(1)
> +#define NAU7802_REG_CTRL1 0x01
> +#define NAU7802_CTRL1_VLDO(x) (x << 3)
> +#define NAU7802_CTRL1_GAINS(x) (x)
> +#define NAU7802_CTRL1_GAINS_BITS 0x07
> +#define NAU7802_REG_CTRL2 0x02
> +#define NAU7802_CTRL2_CHS(x) (x << 7)
> +#define NAU7802_CTRL2_CRS(x) (x << 4)
> +#define NAU7802_CTRL2_CHS_BIT NAU7802_CTRL2_CHS(1)
> +#define NAU7802_REG_ADC_B2 0x12
> +#define NAU7802_REG_ADC_B1 0x13
> +#define NAU7802_REG_ADC_B0 0x14
> +#define NAU7802_REG_ADC_CTRL 0x15
> +
> +struct nau7802_state {
> + struct i2c_client *client;
> + s32 last_value;
I can't immediately see why you need two locks...
I think the datalock is only ever taken when the
lock is already held (be it in a threaded irq).
> + struct mutex lock;
> + struct mutex data_lock;
> + u32 vref_mv;
> + u32 conversion_count;
> + u32 min_conversions;
> + u8 sample_rate;
> + struct completion value_ok;
> +};
> +
> +static int nau7802_i2c_read(struct nau7802_state *st, u8 reg, u8 *data)
> +{
> + int ret = 0;
> +
> + ret = i2c_smbus_read_byte_data(st->client, reg);
> + if (ret < 0) {
> + dev_err(&st->client->dev, "failed to read from I2C\n");
> + return ret;
> + }
> +
> + *data = ret;
> +
> + return 0;
> +}
> +
Don't wrap standard functions like this. Call them directly. It's not
worth the added complexity to get a not particularly informative error message
from the read.
> +static int nau7802_i2c_write(struct nau7802_state *st, u8 reg, u8 data)
> +{
> + return i2c_smbus_write_byte_data(st->client, reg, data);
> +}
> +
> +static const u16 nau7802_sample_freq_avail[] = {10, 20, 40, 80,
> + 10, 10, 10, 320};
> +
> +static ssize_t nau7802_sysfs_set_sampling_frequency(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
> + struct iio_dev *idev = dev_to_iio_dev(dev);
> + struct nau7802_state *st = iio_priv(idev);
> + u32 val;
> + int ret, i;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + ret = -EINVAL;
> + for (i = 0; i < 8; i++)
> + if (val == nau7802_sample_freq_avail[i]) {
> + mutex_lock(&st->lock);
> + st->sample_rate = i;
> + st->conversion_count = 0;
> + nau7802_i2c_write(st, NAU7802_REG_CTRL2,
> + NAU7802_CTRL2_CRS(st->sample_rate));
> + mutex_unlock(&st->lock);
> + ret = len;
> + break;
> + }
> + return ret;
> +}
> +
> +static ssize_t nau7802_sysfs_get_sampling_frequency(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *idev = dev_to_iio_dev(dev);
> + struct nau7802_state *st = iio_priv(idev);
> +
> + return sprintf(buf, "%d\n",
> + nau7802_sample_freq_avail[st->sample_rate]);
> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
> + nau7802_sysfs_get_sampling_frequency,
> + nau7802_sysfs_set_sampling_frequency);
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("10 40 80 320");
> +
> +static IIO_CONST_ATTR(gain_available, "1 2 4 8 16 32 64 128");
> +
> +static ssize_t nau7802_sysfs_set_gain(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
> + struct iio_dev *idev = dev_to_iio_dev(dev);
> + struct nau7802_state *st = iio_priv(idev);
> + u32 val;
> + u8 data;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val < 1 || val > 128 || !is_power_of_2(val))
> + return -EINVAL;
> +
> + mutex_lock(&st->lock);
> + st->conversion_count = 0;
> +
> + ret = nau7802_i2c_read(st, NAU7802_REG_CTRL1, &data);
> + if (ret < 0)
> + goto nau7802_sysfs_set_gain_out;
> + ret = nau7802_i2c_write(st, NAU7802_REG_CTRL1,
> + (data & (~NAU7802_CTRL1_GAINS_BITS)) | ilog2(val));
> +nau7802_sysfs_set_gain_out:
> + mutex_unlock(&st->lock);
> + return ret ? ret : len;
> +}
> +
> +static ssize_t nau7802_sysfs_get_gain(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *idev = dev_to_iio_dev(dev);
> + struct nau7802_state *st = iio_priv(idev);
> + u8 data;
> + int ret;
> +
> + ret = nau7802_i2c_read(st, NAU7802_REG_CTRL1, &data);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", 1 << (data & NAU7802_CTRL1_GAINS_BITS));
> +}
> +
> +static IIO_DEVICE_ATTR(gain, S_IWUSR | S_IRUGO,
> + nau7802_sysfs_get_gain,
> + nau7802_sysfs_set_gain, 0);
> +
> +static ssize_t nau7802_sysfs_set_min_conversions(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
> + struct iio_dev *idev = dev_to_iio_dev(dev);
> + struct nau7802_state *st = iio_priv(idev);
> + u32 val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&st->lock);
> + st->min_conversions = val;
> + st->conversion_count = 0;
> + mutex_unlock(&st->lock);
> + return len;
> +}
> +
> +static ssize_t nau7802_sysfs_get_min_conversions(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *idev = dev_to_iio_dev(dev);
> + struct nau7802_state *st = iio_priv(idev);
> +
> + return sprintf(buf, "%d\n", st->min_conversions);
> +}
> +
> +static IIO_DEVICE_ATTR(min_conversions, S_IWUSR | S_IRUGO,
> + nau7802_sysfs_get_min_conversions,
> + nau7802_sysfs_set_min_conversions, 0);
> +
> +static struct attribute *nau7802_attributes[] = {
Given you have only adc channels you could just use the relevant
info_mask element for and have this as a shared attribute of them.
> + &iio_dev_attr_sampling_frequency.dev_attr.attr,
> + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
Just have a range of scale options available and figure out which
gain they correspond to. Obviously you'll need a write_raw callback
to implement that, but it's pretty standard and conforms to the iio
abi which this does not. I've been meaning to clean up the way
'available' is handled but for now you will probaby want to have
a suitable gain here.
> + &iio_dev_attr_gain.dev_attr.attr,
> + &iio_const_attr_gain_available.dev_attr.attr,
> + &iio_dev_attr_min_conversions.dev_attr.attr,
What governs this? Looks to me more like it should be hidden away
in the device tree than be here.
> + NULL
> +};
> +
> +static const struct attribute_group nau7802_attribute_group = {
> + .attrs = nau7802_attributes,
> +};
> +
> +static int nau7802_read_conversion(struct nau7802_state *st)
> +{
> + int ret;
> + u8 data;
> +
> + mutex_lock(&st->data_lock);
> + ret = nau7802_i2c_read(st, NAU7802_REG_ADC_B2, &data);
> + if (ret)
> + goto nau7802_read_conversion_out;
> + st->last_value = data << 24;
> +
> + ret = nau7802_i2c_read(st, NAU7802_REG_ADC_B1, &data);
> + if (ret)
> + goto nau7802_read_conversion_out;
> + st->last_value |= data << 16;
> +
> + ret = nau7802_i2c_read(st, NAU7802_REG_ADC_B0, &data);
> + if (ret)
> + goto nau7802_read_conversion_out;
> + st->last_value |= data << 8;
> +
> + st->last_value >>= 8;
umm. Why shift everything 8 to the left then 8 to the right?
> +
> +nau7802_read_conversion_out:
> + mutex_unlock(&st->data_lock);
> + return ret;
> +}
> +
What does this do? Perhaps a comment?
> +static int nau7802_sync(struct nau7802_state *st)
> +{
> + int ret;
> + u8 data;
> +
> + ret = nau7802_i2c_read(st, NAU7802_REG_PUCTRL, &data);
> + if (ret)
> + return ret;
> + ret = nau7802_i2c_write(st, NAU7802_REG_PUCTRL,
> + data | NAU7802_PUCTRL_CS_BIT);
> + return ret;
> +}
> +
> +static irqreturn_t nau7802_eoc_trigger(int irq, void *private)
> +{
> + struct iio_dev *idev = private;
> + struct nau7802_state *st = iio_priv(idev);
> + u8 status;
> + int ret;
> +
> + ret = nau7802_i2c_read(st, NAU7802_REG_PUCTRL, &status);
> + if (ret)
> + return IRQ_HANDLED;
> +
> + if (!(status & NAU7802_PUCTRL_CR_BIT))
> + return IRQ_NONE;
> +
> + if (nau7802_read_conversion(st))
> + return IRQ_HANDLED;
> +
> + /* wait for enough conversions before getting a stable value when
> + * changing channels */
> + if (st->conversion_count < st->min_conversions)
> + st->conversion_count++;
> + if (st->conversion_count >= st->min_conversions)
> + complete_all(&st->value_ok);
pretty much always stick in a blank line before return statements.
Makes it easier to read...
> + return IRQ_HANDLED;
> +}
> +
> +static int nau7802_read_irq(struct iio_dev *idev,
> + struct iio_chan_spec const *chan,
> + int *val)
> +{
> + struct nau7802_state *st = iio_priv(idev);
> + int ret;
> +
> + INIT_COMPLETION(st->value_ok);
So let me see if I have this correct.
> + enable_irq(st->client->irq);
This will probably fire immediately as it is a level irq and we have had
it masked.
> +
> + nau7802_sync(st);
This flushes the device?
> +
> + /* read registers to ensure we flush everything */
> + ret = nau7802_read_conversion(st);
> + if (ret)
> + goto read_chan_info_failure;
> +
This then ensures we have scrubbed any left over data? Might scrub
some extra but that isn't a problem...
> + /* Wait for a conversion to finish */
> + ret = wait_for_completion_interruptible_timeout(&st->value_ok,
> + msecs_to_jiffies(1000));
This will fire next time we have new data?
> + if (ret == 0)
> + ret = -ETIMEDOUT;
> +
> + if (ret < 0)
> + goto read_chan_info_failure;
> +
> + disable_irq(st->client->irq);
> +
> + *val = st->last_value;
nitpick - blank line here.
> + return IIO_VAL_INT;
> +
> +read_chan_info_failure:
> + disable_irq(st->client->irq);
> + return ret;
> +}
> +
> +static int nau7802_read_poll(struct iio_dev *idev,
> + struct iio_chan_spec const *chan,
> + int *val)
> +{
> + struct nau7802_state *st = iio_priv(idev);
> + int ret;
> + u8 data;
> +
> + nau7802_sync(st);
> +
> + /* read registers to ensure we flush everything */
> + ret = nau7802_read_conversion(st);
> + if (ret)
> + return ret;
> +
> + do {
> + nau7802_i2c_read(st, NAU7802_REG_PUCTRL, &data);
> + if (ret)
> + return ret;
> + while (!(data & NAU7802_PUCTRL_CR_BIT)) {
> + if (st->sample_rate != 0x07)
> + msleep(20);
> + else
> + mdelay(4);
> + nau7802_i2c_read(st, NAU7802_REG_PUCTRL, &data);
> + if (ret)
> + return ret;
> + }
> +
> + nau7802_read_conversion(st);
> + if (ret)
> + return ret;
> + if (st->conversion_count < st->min_conversions)
> + st->conversion_count++;
> + } while (st->conversion_count < st->min_conversions);
> +
> + *val = st->last_value;
nitpick - nicer with a blank line here.
> + return IIO_VAL_INT;
> +}
> +
> +static int nau7802_read_raw(struct iio_dev *idev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct nau7802_state *st = iio_priv(idev);
> + u8 data;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> + /*
> + * Select the channel to use
> + * - Channel 1 is value 0 in the CHS register
> + * - Channel 2 is value 1 in the CHS register
> + */
> + ret = nau7802_i2c_read(st, NAU7802_REG_CTRL2, &data);
> + if (ret < 0)
> + return ret;
> + if (((data & NAU7802_CTRL2_CHS_BIT) && !chan->channel) ||
> + (!(data & NAU7802_CTRL2_CHS_BIT) &&
> + chan->channel)) {
> + st->conversion_count = 0;
> + ret = nau7802_i2c_write(st, NAU7802_REG_CTRL2,
> + NAU7802_CTRL2_CHS(chan->channel) |
> + NAU7802_CTRL2_CRS(st->sample_rate));
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (st->client->irq)
> + ret = nau7802_read_irq(idev, chan, val);
> + else
> + ret = nau7802_read_poll(idev, chan, val);
> +
> + mutex_unlock(&st->lock);
> + return ret;
> + case IIO_CHAN_INFO_SCALE:
> + ret = nau7802_i2c_read(st, NAU7802_REG_CTRL1, &data);
> + if (ret < 0)
> + return ret;
> +
> + *val = 0;
> + *val2 = (((u64)st->vref_mv) * 1000000000ULL) >>
> + (chan->scan_type.realbits +
> + (data & NAU7802_CTRL1_GAINS_BITS));
It's saturday morning and I'm half asleep so I'll take your word for
this being the nicest way of computing this. However with a 23 bit
adc measuring around 5V? I would have thought to get a decent number
of significant figures you'd want to do IIO_VAL_INT_PLUS_NANO?
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + break;
> + }
> + return -EINVAL;
> +}
> +
> +static const struct iio_info nau7802_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = &nau7802_read_raw,
> + .attrs = &nau7802_attribute_group,
> +};
> +
> +static int nau7802_channel_init(struct iio_dev *idev)
> +{
> + struct iio_chan_spec *chan_array;
> + int i;
> +
> + idev->num_channels = 2;
> +
> + chan_array = devm_kzalloc(&idev->dev,
> + sizeof(*chan_array) * idev->num_channels,
> + GFP_KERNEL);
> +
There are only two of them and that is pretty much fixed. Do this with
a const static array rather than dynamically. If you really want the
flexibility on number of channels, then just set idev->num_channels
appropriately and it'll ignore later entries in the table.
Dynamic allocation only makes sense where we have a huge and complex
set of possible channel combinations.
> + for (i = 0; i < idev->num_channels; i++) {
> + struct iio_chan_spec *chan = chan_array + i;
> +
> + chan->type = IIO_VOLTAGE;
> + chan->indexed = 1;
> + chan->channel = i;
> + chan->scan_index = i;
> + chan->scan_type.sign = 's';
> + chan->scan_type.realbits = 23;
As I read it this part is a 24bit adc where they are committing to 23bits of good
precision. As such realbits should be 24.
Not that it's used for anything unless you are doing buffering!
(so feel free to drop the scan_index and san_type entirely.
> + chan->scan_type.storagebits = 24;
> + chan->info_mask = IIO_CHAN_INFO_SCALE_SHARED_BIT |
> + IIO_CHAN_INFO_RAW_SEPARATE_BIT;
This has changed, take a look at the latest staging-next.
> + }
> + idev->channels = chan_array;
> +
> + return idev->num_channels;
> +}
> +
> +static int nau7802_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct iio_dev *idev;
> + struct nau7802_state *st;
> + struct device_node *np = client->dev.of_node;
> + int ret;
> + u8 data;
> + u32 tmp;
> +
> + if (!client->dev.of_node) {
> + dev_err(&client->dev, "No device tree node available.\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * If we have some interrupts declared, use them, if not, fall back
> + * on polling.
> + */
> + if (of_find_property(np, "interrupts", NULL)) {
> + if (client->irq <= 0) {
> + client->irq = irq_of_parse_and_map(np, 0);
> + if (client->irq <= 0)
> + return -EPROBE_DEFER;
> + }
> + } else
> + client->irq = 0;
> +
> + idev = iio_device_alloc(sizeof(struct nau7802_state));
sizeof(*st) is a little cleaner.
Also we have pretty much standardized on calling the iio_dev
indio_dev (no idea why but it's how it turned out ;) Whilst
it doesn't really matter it does make reviewers jobs marginally
easier ;)
> + if (idev == NULL)
> + return -ENOMEM;
> +
> + st = iio_priv(idev);
> +
> + i2c_set_clientdata(client, idev);
> +
> + idev->dev.parent = &client->dev;
> + idev->name = dev_name(&client->dev);
> + idev->modes = INDIO_DIRECT_MODE;
> + idev->info = &nau7802_info;
> +
> + st->client = client;
> +
> + /* Reset the device */
> + nau7802_i2c_write(st, NAU7802_REG_PUCTRL, NAU7802_PUCTRL_RR_BIT);
> +
> + /* Enter normal operation mode */
> + nau7802_i2c_write(st, NAU7802_REG_PUCTRL, NAU7802_PUCTRL_PUD_BIT);
> +
> + /*
> + * After about 200 usecs, the device should be ready and then
> + * the Power Up bit will be set to 1. If not, wait for it.
> + */
> + do {
> + udelay(200);
> + ret = nau7802_i2c_read(st, NAU7802_REG_PUCTRL, &data);
> + if (ret)
> + return -ENODEV;
You want some sort of timeout and fail here. Perasonally if the device
should be ready in 200 usec, I'd only let it have one go at doing so
before failing (or give 250 usec if it's not all that precise).
> + } while (!(data & NAU7802_PUCTRL_PUR_BIT));
> +
> + of_property_read_u32(np, "nuvoton,vldo", &tmp);
> + st->vref_mv = tmp;
> +
> + data = NAU7802_PUCTRL_PUD_BIT | NAU7802_PUCTRL_PUA_BIT |
> + NAU7802_PUCTRL_CS_BIT;
> + if (tmp >= 2400)
> + data |= NAU7802_PUCTRL_AVDDS_BIT;
> +
> + nau7802_i2c_write(st, NAU7802_REG_PUCTRL, data);
> + nau7802_i2c_write(st, NAU7802_REG_ADC_CTRL, 0x30);
> +
> + if (tmp >= 2400) {
> + data = NAU7802_CTRL1_VLDO((4500 - tmp) / 300);
> + nau7802_i2c_write(st, NAU7802_REG_CTRL1, data);
> + }
> +
> + st->min_conversions = 6;
> +
> + /*
> + * The ADC fires continuously and we can't do anything about
> + * it. So we need to have the IRQ disabled by default, and we
> + * will enable them back when we will need them..
> + */
> + if (client->irq) {
> + irq_set_status_flags(client->irq, IRQ_NOAUTOEN);
> + ret = request_threaded_irq(client->irq,
> + NULL,
> + nau7802_eoc_trigger,
> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> + client->dev.driver->name,
> + idev);
> + if (ret) {
> + /*
> + * What may happen here is that our IRQ controller is
> + * not able to get level interrupt but this is required
> + * by this ADC as when going over 40 sample per second,
> + * the interrupt line may stay high between conversions.
> + * So, we continue no matter what but we switch to
> + * polling mode.
> + */
Good plan. Pesky hardware designers and not guaranteeing transitions. ;)
> + dev_info(&client->dev,
> + "Failed to allocate IRQ, using polling mode\n");
> + client->irq = 0;
> + /*
> + * We are polling, use the fastest sample rate by
> + * default
> + */
> + st->sample_rate = 0x7;
> + nau7802_i2c_write(st, NAU7802_REG_CTRL2,
> + NAU7802_CTRL2_CRS(st->sample_rate));
> + }
> + }
> +
> + /* Setup the ADC channels available on the board */
> + ret = nau7802_channel_init(idev);
> + if (ret < 0) {
> + dev_err(&client->dev, "Couldn't initialize the channels.\n");
> + goto error_channel_init;
> + }
> +
> + init_completion(&st->value_ok);
> + mutex_init(&st->lock);
> + mutex_init(&st->data_lock);
> +
> + ret = iio_device_register(idev);
> + if (ret < 0) {
> + dev_err(&client->dev, "Couldn't register the device.\n");
> + goto error_device_register;
> + }
> +
> + return 0;
> +
> +error_device_register:
> + mutex_destroy(&st->lock);
datalock?
> +error_channel_init:
> + if (client->irq)
> + free_irq(client->irq, idev);
> + iio_device_free(idev);
> + return ret;
> +}
> +
> +static int nau7802_remove(struct i2c_client *client)
> +{
> + struct iio_dev *idev = i2c_get_clientdata(client);
> + struct nau7802_state *st = iio_priv(idev);
> +
> + iio_device_unregister(idev);
> + mutex_destroy(&st->lock);
> + mutex_destroy(&st->data_lock);
> + if (client->irq)
> + free_irq(client->irq, idev);
> + iio_device_free(idev);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id nau7802_i2c_id[] = {
> + { "nau7802", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, nau7802_i2c_id);
> +
> +static const struct of_device_id nau7802_dt_ids[] = {
> + { .compatible = "nuvoton,nau7802" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, nau7802_dt_ids);
> +
> +static struct i2c_driver nau7802_driver = {
> + .probe = nau7802_probe,
> + .remove = nau7802_remove,
> + .id_table = nau7802_i2c_id,
> + .driver = {
> + .name = "nau7802",
> + .of_match_table = of_match_ptr(nau7802_dt_ids),
> + },
> +};
> +
> +module_i2c_driver(nau7802_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Nuvoton NAU7802 ADC Driver");
> +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@...e-electrons.com>");
> +MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@...e-electrons.com>");
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists