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

2013-06-24 Thread Lars-Peter Clausen 
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.bell...@free-electrons.com
 Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
 ---
  .../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 000..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.

 +};
[...]
 diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
 new file mode 100644
 index 000..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

 + 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?

 +
 + nau7802_sync(st);
 +
 + /* read registers to ensure we flush everything */
 + ret = nau7802_read_conversion(st);
 + if (ret  0)
 + goto read_chan_info_failure;
 +
 + /* Wait for a conversion to finish */
 + ret = wait_for_completion_interruptible_timeout(st-value_ok,
 + msecs_to_jiffies(1000));
 + if (ret == 0)
 + ret = -ETIMEDOUT;
 +
 + if (ret  0)
 + goto read_chan_info_failure;
 +
 + disable_irq(st-client-irq);
 +
 + *val = st-last_value;
 +
 + return IIO_VAL_INT;
 +
 +read_chan_info_failure:
 + disable_irq(st-client-irq);
 +
 + return ret;
 +}
[...]

[...]
 +static int nau7802_probe(struct i2c_client *client,
 + const struct i2c_device_id *id)
 +{
 + struct iio_dev *indio_dev;
 + struct nau7802_state *st;
 + struct device_node *np = client-dev.of_node;
 + int i, ret;
 + u8 data;
 + u32 tmp;
 +
 + if (!client-dev.of_node) {
 + dev_err(client-dev, No device tree node available.\n);
 + return -EINVAL;
 + }

Except for getting the vref the is no direct dependency on devicetree, if
you switch to the regulator framework for the vref this check can be removed.

[...]
 + /* Setup the ADC channels available on the board */
 + indio_dev-num_channels = 2;

ARRAY_SIZE(nau7802_chan_array)

 + indio_dev-channels = nau7802_chan_array;
 +
 + init_completion(st-value_ok);

You need to initialize the completion before requesting the IRQ handler.

[...]
 +}
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

2013-06-24 Thread Lars-Peter Clausen 
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 alexandre.bell...@free-electrons.com
 Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
 ---
  .../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 000..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 000..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.

___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

2013-06-23 Thread Lars-Peter Clausen 
On 06/22/2013 03:28 PM, Alexandre Belloni wrote:
 On 22/06/2013 15:20, Lars-Peter Clausen wrote:
 On 06/22/2013 03:07 PM, Alexandre Belloni wrote:
 On 22/06/2013 14:02, Lars-Peter Clausen wrote:
 On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
 On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
 The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
 gain and sampling rates.

 Sorry, somewhat low on time today so only a quick review.

 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
a little vague?  Not that I have a better idea!
 I really don't like the name min_conversions either. Isn't this effectively
 a decimation filter?
 Yeah, it could be seen like that but it is only relevant and only
 happens when switching between channels. I'm open to any ideas.

 I see. Is there anything about this in the datasheet on how many conversions
 you usually need? Is this really something you need to change at runtime or
 does moving this to platform data work?


 
 There is actually nothing in the datasheet. The default value (6
 conversions) was found experimentally. What I did was saturating the ADC
 with the higher value on one channel and the lower value on the other
 one and I tried to find when reading both channel sequentially was
 resulting in a correct value.
 
 You may not need to change it at runtime. And that value mainly depend
 on the precision versus speed balance you want to achieve. If you know
 that the values on both channels will not be to far apart, then you may
 not need to wait at all.
 
 Would you think that is something I should hide in the DT ? Or maybe I
 can drop that knob for now and see if it is needed in the future.
 

It is always a good idea to be conservative when introducing new ABI, so if
you think we can get away with hardcoding this in the driver I think that's
a good idea.

- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

2013-06-23 Thread Jonathan Cameron 
On 06/23/2013 02:54 PM, Lars-Peter Clausen wrote:
 On 06/22/2013 03:28 PM, Alexandre Belloni wrote:
 On 22/06/2013 15:20, Lars-Peter Clausen wrote:
 On 06/22/2013 03:07 PM, Alexandre Belloni wrote:
 On 22/06/2013 14:02, Lars-Peter Clausen wrote:
 On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
 On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
 The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
 gain and sampling rates.

 Sorry, somewhat low on time today so only a quick review.

 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
a little vague?  Not that I have a better idea!
 I really don't like the name min_conversions either. Isn't this 
 effectively
 a decimation filter?
 Yeah, it could be seen like that but it is only relevant and only
 happens when switching between channels. I'm open to any ideas.

 I see. Is there anything about this in the datasheet on how many conversions
 you usually need? Is this really something you need to change at runtime or
 does moving this to platform data work?



 There is actually nothing in the datasheet. The default value (6
 conversions) was found experimentally. What I did was saturating the ADC
 with the higher value on one channel and the lower value on the other
 one and I tried to find when reading both channel sequentially was
 resulting in a correct value.

 You may not need to change it at runtime. And that value mainly depend
 on the precision versus speed balance you want to achieve. If you know
 that the values on both channels will not be to far apart, then you may
 not need to wait at all.

 Would you think that is something I should hide in the DT ? Or maybe I
 can drop that knob for now and see if it is needed in the future.

 
 It is always a good idea to be conservative when introducing new ABI, so if
 you think we can get away with hardcoding this in the driver I think that's
 a good idea.
 
I agree entirely.  We can always make it controllable later as long
as we keep the default the same as the value you hard code in now.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

2013-06-22 Thread Lars-Peter Clausen 
On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
 On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
 The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
 gain and sampling rates.

 Sorry, somewhat low on time today so only a quick review.
 
 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
a little vague?  Not that I have a better idea!

I really don't like the name min_conversions either. Isn't this effectively
a decimation filter?

- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

2013-06-22 Thread Jonathan Cameron 
On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
 The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
 gain and sampling rates.

Sorry, somewhat low on time today so only a quick review.

1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
   a little vague?  Not that I have a better idea!

2) A lot of i2c calls could do with error handling.


 Signed-off-by: Alexandre Belloni alexandre.bell...@free-electrons.com
 Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
 ---
  .../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 000..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)
 +  - 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 ab0767e6..d7f9ed8 100644
 --- a/drivers/iio/adc/Kconfig
 +++ b/drivers/iio/adc/Kconfig
 @@ -133,6 +133,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 0a825be..d426081 100644
 --- a/drivers/iio/adc/Makefile
 +++ b/drivers/iio/adc/Makefile
 @@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
  obj-$(CONFIG_EXYNOS_ADC) += exynos_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 000..e1b6981
 --- /dev/null
 +++ b/drivers/iio/adc/nau7802.c
 @@ -0,0 +1,603 @@
 +/*
 + * 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/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_BITNAU7802_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_BITNAU7802_PUCTRL_CS(1)
 +#define NAU7802_PUCTRL_CR(x) (x  5)
 +#define NAU7802_PUCTRL_CR_BITNAU7802_PUCTRL_CR(1)
 +#define NAU7802_PUCTRL_AVDDS(x)  (x  7)
 +#define NAU7802_PUCTRL_AVDDS_BIT NAU7802_PUCTRL_AVDDS(1)
 +#define NAU7802_REG_CTRL10x01
 +#define NAU7802_CTRL1_VLDO(x)(x  3)
 +#define NAU7802_CTRL1_GAINS(x)   (x)
 +#define NAU7802_CTRL1_GAINS_BITS 0x07
 +#define NAU7802_REG_CTRL20x02
 +#define NAU7802_CTRL2_CHS(x) (x  7)
 +#define NAU7802_CTRL2_CRS(x) (x  4)
 +#define NAU7802_SAMP_FREQ_3200x07
 +#define NAU7802_CTRL2_CHS_BITNAU7802_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
 +
 +#define NAU7802_DEFAULT_CONVERSIONS 6
 +
 +struct nau7802_state {
 + struct i2c_client   *client;
 +

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

2013-06-22 Thread Lars-Peter Clausen 
On 06/22/2013 03:07 PM, Alexandre Belloni wrote:
 On 22/06/2013 14:02, Lars-Peter Clausen wrote:
 On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
 On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
 The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
 gain and sampling rates.

 Sorry, somewhat low on time today so only a quick review.

 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
a little vague?  Not that I have a better idea!
 I really don't like the name min_conversions either. Isn't this effectively
 a decimation filter?
 
 Yeah, it could be seen like that but it is only relevant and only
 happens when switching between channels. I'm open to any ideas.
 

I see. Is there anything about this in the datasheet on how many conversions
you usually need? Is this really something you need to change at runtime or
does moving this to platform data work?

- Lars
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss