* [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier @ 2012-02-22 12:36 michael.hennerich 2012-03-21 19:34 ` Jonathan Cameron 0 siblings, 1 reply; 16+ messages in thread From: michael.hennerich @ 2012-02-22 12:36 UTC (permalink / raw) To: jic23; +Cc: linux-iio, device-drivers-devel, drivers, Michael Hennerich From: Michael Hennerich <michael.hennerich@analog.com> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> --- drivers/staging/iio/Kconfig | 1 + drivers/staging/iio/Makefile | 1 + drivers/staging/iio/amplifiers/Kconfig | 17 +++ drivers/staging/iio/amplifiers/Makefile | 5 + drivers/staging/iio/amplifiers/ad8366.c | 228 +++++++++++++++++++++++++++++++ 5 files changed, 252 insertions(+), 0 deletions(-) create mode 100644 drivers/staging/iio/amplifiers/Kconfig create mode 100644 drivers/staging/iio/amplifiers/Makefile create mode 100644 drivers/staging/iio/amplifiers/ad8366.c diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig index 2b56823..fd7a164 100644 --- a/drivers/staging/iio/Kconfig +++ b/drivers/staging/iio/Kconfig @@ -59,6 +59,7 @@ config IIO_CONSUMERS_PER_TRIGGER source "drivers/staging/iio/accel/Kconfig" source "drivers/staging/iio/adc/Kconfig" source "drivers/staging/iio/addac/Kconfig" +source "drivers/staging/iio/amplifiers/Kconfig" source "drivers/staging/iio/cdc/Kconfig" source "drivers/staging/iio/dac/Kconfig" source "drivers/staging/iio/frequency/Kconfig" diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile index 0161095..c52cde3 100644 --- a/drivers/staging/iio/Makefile +++ b/drivers/staging/iio/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_IIO_DUMMY_EVGEN) += iio_dummy_evgen.o obj-y += accel/ obj-y += adc/ obj-y += addac/ +obj-y += amplifiers/ obj-y += cdc/ obj-y += dac/ obj-y += frequency/ diff --git a/drivers/staging/iio/amplifiers/Kconfig b/drivers/staging/iio/amplifiers/Kconfig new file mode 100644 index 0000000..05d707e --- /dev/null +++ b/drivers/staging/iio/amplifiers/Kconfig @@ -0,0 +1,17 @@ +# +# Gain Amplifiers, etc. +# +menu "Amplifiers" + +config AD8366 + tristate "Analog Devices AD8366 VGA" + depends on SPI + select BITREVERSE + help + Say yes here to build support for Analog Devices AD8366 + SPI Dual-Digital Variable Gain Amplifier (VGA). + + To compile this driver as a module, choose M here: the + module will be called ad8366. + +endmenu diff --git a/drivers/staging/iio/amplifiers/Makefile b/drivers/staging/iio/amplifiers/Makefile new file mode 100644 index 0000000..a6ca366 --- /dev/null +++ b/drivers/staging/iio/amplifiers/Makefile @@ -0,0 +1,5 @@ +# +# Makefile iio/amplifiers +# + +obj-$(CONFIG_AD8366) += ad8366.o diff --git a/drivers/staging/iio/amplifiers/ad8366.c b/drivers/staging/iio/amplifiers/ad8366.c new file mode 100644 index 0000000..187606f --- /dev/null +++ b/drivers/staging/iio/amplifiers/ad8366.c @@ -0,0 +1,228 @@ +/* + * AD8366 SPI Dual-Digital Variable Gain Amplifier (VGA) + * + * Copyright 2012 Analog Devices Inc. + * + * Licensed under the GPL-2. + */ + +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/sysfs.h> +#include <linux/spi/spi.h> +#include <linux/regulator/consumer.h> +#include <linux/err.h> +#include <linux/module.h> +#include <linux/bitrev.h> + +#include "../iio.h" +#include "../sysfs.h" + +struct ad8366_state { + struct spi_device *spi; + struct regulator *reg; + unsigned char ch_a; + unsigned char ch_b; + /* + * DMA (thus cache coherency maintenance) requires the + * transfer buffers to live in their own cache lines. + */ + unsigned char data[2] ____cacheline_aligned; +}; + +static int ad8366_write(struct iio_dev *indio_dev, + unsigned char ch_a, char unsigned ch_b) +{ + struct ad8366_state *st = iio_priv(indio_dev); + int ret; + + ch_a = bitrev8(ch_a & 0x3F); + ch_b = bitrev8(ch_b & 0x3F); + + st->data[0] = ch_b >> 4; + st->data[1] = (ch_b << 4) | (ch_a >> 2); + + ret = spi_write(st->spi, st->data, ARRAY_SIZE(st->data)); + if (ret < 0) + dev_err(&indio_dev->dev, "write failed (%d)", ret); + + return ret; +} + +static int ad8366_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, + int *val2, + long m) +{ + struct ad8366_state *st = iio_priv(indio_dev); + int ret; + unsigned code; + + mutex_lock(&indio_dev->mlock); + switch (m) { + case 0: + if (chan->channel == 0) + code = st->ch_a; + else + code = st->ch_b; + + /* Values in dB */ + code = code * 253 + 4500; + *val = code / 1000; + *val2 = (code % 1000) * 1000; + + ret = IIO_VAL_INT_PLUS_MICRO; + break; + default: + ret = -EINVAL; + } + mutex_unlock(&indio_dev->mlock); + + return ret; +}; + +static int ad8366_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, + int val2, + long mask) +{ + struct ad8366_state *st = iio_priv(indio_dev); + unsigned code; + int ret; + + /* Values in dB */ + code = (((u8)val * 1000) + ((u32)val2 / 1000)); + + if (code > 20500 || code < 4500) + return -EINVAL; + + code = (code - 4500) / 253; + + mutex_lock(&indio_dev->mlock); + switch (mask) { + case 0: + if (chan->channel == 0) + st->ch_a = code; + else + st->ch_b = code; + + ret = ad8366_write(indio_dev, st->ch_a, st->ch_b); + break; + default: + ret = -EINVAL; + } + mutex_unlock(&indio_dev->mlock); + + return ret; +} + +static const struct iio_info ad8366_info = { + .read_raw = &ad8366_read_raw, + .write_raw = &ad8366_write_raw, + .driver_module = THIS_MODULE, +}; + +#define AD8366_CHAN(_channel, _name) { \ + .type = IIO_ALTVOLTAGE, \ + .output = 1, \ + .indexed = 1, \ + .processed_val = 1, \ + .channel = _channel, \ + .extend_name = _name, \ +} + +static const struct iio_chan_spec ad8366_channels[] = { + AD8366_CHAN(0, "gain"), + AD8366_CHAN(1, "gain"), +}; + +static int __devinit ad8366_probe(struct spi_device *spi) +{ + struct iio_dev *indio_dev; + struct ad8366_state *st; + int ret; + + indio_dev = iio_allocate_device(sizeof(*st)); + if (indio_dev == NULL) + return -ENOMEM; + + st = iio_priv(indio_dev); + + st->reg = regulator_get(&spi->dev, "vcc"); + if (!IS_ERR(st->reg)) { + ret = regulator_enable(st->reg); + if (ret) + goto error_put_reg; + } + + spi_set_drvdata(spi, indio_dev); + st->spi = spi; + + indio_dev->dev.parent = &spi->dev; + indio_dev->name = spi_get_device_id(spi)->name; + indio_dev->info = &ad8366_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = ad8366_channels; + indio_dev->num_channels = ARRAY_SIZE(ad8366_channels); + + ret = iio_device_register(indio_dev); + if (ret) + goto error_disable_reg; + + ad8366_write(indio_dev, 0 , 0); + + return 0; + +error_disable_reg: + if (!IS_ERR(st->reg)) + regulator_disable(st->reg); +error_put_reg: + if (!IS_ERR(st->reg)) + regulator_put(st->reg); + + iio_free_device(indio_dev); + + return ret; +} + +static int __devexit ad8366_remove(struct spi_device *spi) +{ + struct iio_dev *indio_dev = spi_get_drvdata(spi); + struct ad8366_state *st = iio_priv(indio_dev); + struct regulator *reg = st->reg; + + iio_device_unregister(indio_dev); + + if (!IS_ERR(reg)) { + regulator_disable(reg); + regulator_put(reg); + } + + iio_free_device(indio_dev); + + return 0; +} + +static const struct spi_device_id ad8366_id[] = { + {"ad8366", 0}, + {} +}; + +static struct spi_driver ad8366_driver = { + .driver = { + .name = KBUILD_MODNAME, + .owner = THIS_MODULE, + }, + .probe = ad8366_probe, + .remove = __devexit_p(ad8366_remove), + .id_table = ad8366_id, +}; + +module_spi_driver(ad8366_driver); + +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>"); +MODULE_DESCRIPTION("Analog Devices AD8366 VGA"); +MODULE_LICENSE("GPL v2"); -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-02-22 12:36 [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier michael.hennerich @ 2012-03-21 19:34 ` Jonathan Cameron 2012-03-21 19:59 ` Mark Brown 2012-03-22 8:52 ` Michael Hennerich 0 siblings, 2 replies; 16+ messages in thread From: Jonathan Cameron @ 2012-03-21 19:34 UTC (permalink / raw) To: michael.hennerich; +Cc: linux-iio, device-drivers-devel, drivers, Mark Brown On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: > From: Michael Hennerich <michael.hennerich@analog.com> Sorry for the slow response on this one. Been off sick... Anyhow, I'm still not sure what the right interface for this type of device is. The obvious options are: 1) Make gain an IIO type (doesn't make much sense as gain is only going to be of one particular existing type). 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use extend name. Any real reason for picking altvoltage rather than voltage? Clearly gain has the same meaning in either case (assuming it's linear). 3) Make a change to core to allow a channel to have elements in info_mask but not actually to have a raw access. Not entirely sure how we will do that cleanly. Also it's not clear whether the gain would be an IN or an OUT channel type! All thoughts on this welcome. Maybe we have to decide to leave it for now and get back to the question at a later date? The driver is clean and short so it's only really the above question of approach that needs answering to my mind... I guess you feel that what you have here is the best plan but I'd like a few more views on this before we set the precedence.... I've pulled Mark into the cc as he might have come across similar devices from the audio side of things and hence have some experience around this question... > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > --- > drivers/staging/iio/Kconfig | 1 + > drivers/staging/iio/Makefile | 1 + > drivers/staging/iio/amplifiers/Kconfig | 17 +++ > drivers/staging/iio/amplifiers/Makefile | 5 + > drivers/staging/iio/amplifiers/ad8366.c | 228 +++++++++++++++++++++++++++++++ > 5 files changed, 252 insertions(+), 0 deletions(-) > create mode 100644 drivers/staging/iio/amplifiers/Kconfig > create mode 100644 drivers/staging/iio/amplifiers/Makefile > create mode 100644 drivers/staging/iio/amplifiers/ad8366.c > > diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig > index 2b56823..fd7a164 100644 > --- a/drivers/staging/iio/Kconfig > +++ b/drivers/staging/iio/Kconfig > @@ -59,6 +59,7 @@ config IIO_CONSUMERS_PER_TRIGGER > source "drivers/staging/iio/accel/Kconfig" > source "drivers/staging/iio/adc/Kconfig" > source "drivers/staging/iio/addac/Kconfig" > +source "drivers/staging/iio/amplifiers/Kconfig" > source "drivers/staging/iio/cdc/Kconfig" > source "drivers/staging/iio/dac/Kconfig" > source "drivers/staging/iio/frequency/Kconfig" > diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile > index 0161095..c52cde3 100644 > --- a/drivers/staging/iio/Makefile > +++ b/drivers/staging/iio/Makefile > @@ -20,6 +20,7 @@ obj-$(CONFIG_IIO_DUMMY_EVGEN) += iio_dummy_evgen.o > obj-y += accel/ > obj-y += adc/ > obj-y += addac/ > +obj-y += amplifiers/ > obj-y += cdc/ > obj-y += dac/ > obj-y += frequency/ > diff --git a/drivers/staging/iio/amplifiers/Kconfig b/drivers/staging/iio/amplifiers/Kconfig > new file mode 100644 > index 0000000..05d707e > --- /dev/null > +++ b/drivers/staging/iio/amplifiers/Kconfig > @@ -0,0 +1,17 @@ > +# > +# Gain Amplifiers, etc. > +# > +menu "Amplifiers" > + > +config AD8366 > + tristate "Analog Devices AD8366 VGA" > + depends on SPI > + select BITREVERSE > + help > + Say yes here to build support for Analog Devices AD8366 > + SPI Dual-Digital Variable Gain Amplifier (VGA). Hmm.. interesting choice of acronym! Just for the unknowledgable amongst us, what's the difference between a vga and a pga? > + > + To compile this driver as a module, choose M here: the > + module will be called ad8366. > + > +endmenu > diff --git a/drivers/staging/iio/amplifiers/Makefile b/drivers/staging/iio/amplifiers/Makefile > new file mode 100644 > index 0000000..a6ca366 > --- /dev/null > +++ b/drivers/staging/iio/amplifiers/Makefile > @@ -0,0 +1,5 @@ > +# > +# Makefile iio/amplifiers > +# > + > +obj-$(CONFIG_AD8366) += ad8366.o > diff --git a/drivers/staging/iio/amplifiers/ad8366.c b/drivers/staging/iio/amplifiers/ad8366.c > new file mode 100644 > index 0000000..187606f > --- /dev/null > +++ b/drivers/staging/iio/amplifiers/ad8366.c > @@ -0,0 +1,228 @@ > +/* > + * AD8366 SPI Dual-Digital Variable Gain Amplifier (VGA) > + * > + * Copyright 2012 Analog Devices Inc. > + * > + * Licensed under the GPL-2. > + */ > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/sysfs.h> > +#include <linux/spi/spi.h> > +#include <linux/regulator/consumer.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/bitrev.h> > + > +#include "../iio.h" > +#include "../sysfs.h" > + > +struct ad8366_state { > + struct spi_device *spi; > + struct regulator *reg; > + unsigned char ch_a; > + unsigned char ch_b; > + /* > + * DMA (thus cache coherency maintenance) requires the > + * transfer buffers to live in their own cache lines. > + */ > + unsigned char data[2] ____cacheline_aligned; > +}; > + > +static int ad8366_write(struct iio_dev *indio_dev, > + unsigned char ch_a, char unsigned ch_b) > +{ > + struct ad8366_state *st = iio_priv(indio_dev); > + int ret; > + > + ch_a = bitrev8(ch_a & 0x3F); > + ch_b = bitrev8(ch_b & 0x3F); > + > + st->data[0] = ch_b >> 4; > + st->data[1] = (ch_b << 4) | (ch_a >> 2); > + > + ret = spi_write(st->spi, st->data, ARRAY_SIZE(st->data)); > + if (ret < 0) > + dev_err(&indio_dev->dev, "write failed (%d)", ret); > + > + return ret; > +} > + > +static int ad8366_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, > + int *val2, > + long m) > +{ > + struct ad8366_state *st = iio_priv(indio_dev); > + int ret; > + unsigned code; > + > + mutex_lock(&indio_dev->mlock); > + switch (m) { > + case 0: > + if (chan->channel == 0) > + code = st->ch_a; > + else > + code = st->ch_b; > + > + /* Values in dB */ > + code = code * 253 + 4500; > + *val = code / 1000; > + *val2 = (code % 1000) * 1000; > + > + ret = IIO_VAL_INT_PLUS_MICRO; > + break; > + default: > + ret = -EINVAL; > + } > + mutex_unlock(&indio_dev->mlock); > + > + return ret; > +}; > + > +static int ad8366_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, > + int val2, > + long mask) > +{ > + struct ad8366_state *st = iio_priv(indio_dev); > + unsigned code; > + int ret; > + > + /* Values in dB */ > + code = (((u8)val * 1000) + ((u32)val2 / 1000)); > + > + if (code > 20500 || code < 4500) > + return -EINVAL; > + > + code = (code - 4500) / 253; > + > + mutex_lock(&indio_dev->mlock); > + switch (mask) { > + case 0: > + if (chan->channel == 0) > + st->ch_a = code; > + else > + st->ch_b = code; > + > + ret = ad8366_write(indio_dev, st->ch_a, st->ch_b); > + break; > + default: > + ret = -EINVAL; > + } > + mutex_unlock(&indio_dev->mlock); > + > + return ret; > +} > + > +static const struct iio_info ad8366_info = { > + .read_raw = &ad8366_read_raw, > + .write_raw = &ad8366_write_raw, > + .driver_module = THIS_MODULE, > +}; > + > +#define AD8366_CHAN(_channel, _name) { \ > + .type = IIO_ALTVOLTAGE, \ > + .output = 1, \ > + .indexed = 1, \ > + .processed_val = 1, \ > + .channel = _channel, \ > + .extend_name = _name, \ > +} > + > +static const struct iio_chan_spec ad8366_channels[] = { > + AD8366_CHAN(0, "gain"), > + AD8366_CHAN(1, "gain"), > +}; > + > +static int __devinit ad8366_probe(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev; > + struct ad8366_state *st; > + int ret; > + > + indio_dev = iio_allocate_device(sizeof(*st)); > + if (indio_dev == NULL) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + > + st->reg = regulator_get(&spi->dev, "vcc"); > + if (!IS_ERR(st->reg)) { > + ret = regulator_enable(st->reg); > + if (ret) > + goto error_put_reg; > + } > + > + spi_set_drvdata(spi, indio_dev); > + st->spi = spi; > + > + indio_dev->dev.parent = &spi->dev; > + indio_dev->name = spi_get_device_id(spi)->name; > + indio_dev->info = &ad8366_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = ad8366_channels; > + indio_dev->num_channels = ARRAY_SIZE(ad8366_channels); > + > + ret = iio_device_register(indio_dev); > + if (ret) > + goto error_disable_reg; > + > + ad8366_write(indio_dev, 0 , 0); > + > + return 0; > + > +error_disable_reg: > + if (!IS_ERR(st->reg)) > + regulator_disable(st->reg); > +error_put_reg: > + if (!IS_ERR(st->reg)) > + regulator_put(st->reg); > + > + iio_free_device(indio_dev); > + > + return ret; > +} > + > +static int __devexit ad8366_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct ad8366_state *st = iio_priv(indio_dev); > + struct regulator *reg = st->reg; > + > + iio_device_unregister(indio_dev); > + > + if (!IS_ERR(reg)) { > + regulator_disable(reg); > + regulator_put(reg); > + } > + > + iio_free_device(indio_dev); > + > + return 0; > +} > + > +static const struct spi_device_id ad8366_id[] = { > + {"ad8366", 0}, > + {} > +}; > + > +static struct spi_driver ad8366_driver = { > + .driver = { > + .name = KBUILD_MODNAME, > + .owner = THIS_MODULE, > + }, > + .probe = ad8366_probe, > + .remove = __devexit_p(ad8366_remove), > + .id_table = ad8366_id, > +}; > + > +module_spi_driver(ad8366_driver); > + > +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>"); > +MODULE_DESCRIPTION("Analog Devices AD8366 VGA"); > +MODULE_LICENSE("GPL v2"); > -- > 1.7.0.4 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-03-21 19:34 ` Jonathan Cameron @ 2012-03-21 19:59 ` Mark Brown 2012-03-22 9:05 ` Hennerich, Michael 2012-03-22 8:52 ` Michael Hennerich 1 sibling, 1 reply; 16+ messages in thread From: Mark Brown @ 2012-03-21 19:59 UTC (permalink / raw) To: Jonathan Cameron Cc: michael.hennerich, linux-iio, device-drivers-devel, drivers [-- Attachment #1: Type: text/plain, Size: 414 bytes --] On Wed, Mar 21, 2012 at 07:34:03PM +0000, Jonathan Cameron wrote: > I've pulled Mark into the cc as he might have come across similar > devices from the audio side of things and hence have some experience > around this question... How to represent gains from amplifiers? Audio specifies gains as dB between input and output; generic stuff like IIO may want to do that using a linear scale (or have the option). [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-03-21 19:59 ` Mark Brown @ 2012-03-22 9:05 ` Hennerich, Michael 0 siblings, 0 replies; 16+ messages in thread From: Hennerich, Michael @ 2012-03-22 9:05 UTC (permalink / raw) To: Mark Brown, Jonathan Cameron Cc: linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers Mark Brown wrote on 2012-03-21: > On Wed, Mar 21, 2012 at 07:34:03PM +0000, Jonathan Cameron wrote: > >> I've pulled Mark into the cc as he might have come across similar >> devices from the audio side of things and hence have some experience >> around this question... > > How to represent gains from amplifiers? Audio specifies gains as dB > between input and output; generic stuff like IIO may want to do that > using a linear scale (or have the option). All VGAs I know describe the gain in dB. Greetings, Michael -- Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-03-21 19:34 ` Jonathan Cameron 2012-03-21 19:59 ` Mark Brown @ 2012-03-22 8:52 ` Michael Hennerich 2012-03-22 9:10 ` Jonathan Cameron 1 sibling, 1 reply; 16+ messages in thread From: Michael Hennerich @ 2012-03-22 8:52 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 03/21/2012 08:34 PM, Jonathan Cameron wrote: > On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >> From: Michael Hennerich<michael.hennerich@analog.com> > Sorry for the slow response on this one. Been off sick... > > Anyhow, I'm still not sure what the right interface for this type > of device is. > > The obvious options are: > > 1) Make gain an IIO type (doesn't make much sense as gain is only going > to be of one particular existing type). > 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use extend > name. Any real reason for picking altvoltage rather than voltage? I'm open for advice. Since I made the amplifier being an OUT type device I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. Some VGAs/PGAs work from DC, but typically VGAs are HF devices. > Clearly gain has the same meaning in either case (assuming it's linear). > 3) Make a change to core to allow a channel to have elements in > info_mask but not actually to have a raw access. Not entirely sure > how we will do that cleanly. Also it's not clear whether the gain > would be an IN or an OUT channel type! Well - having the ability for channels without raw access element would be of interest. > All thoughts on this welcome. Maybe we have to decide to leave it > for now and get back to the question at a later date? > > The driver is clean and short so it's only really the above question of > approach that needs answering to my mind... I guess you feel that > what you have here is the best plan but I'd like a few more views on > this before we set the precedence.... > > I've pulled Mark into the cc as he might have come across similar > devices from the audio side of things and hence have some experience > around this question... >> Signed-off-by: Michael Hennerich<michael.hennerich@analog.com> >> --- >> drivers/staging/iio/Kconfig | 1 + >> drivers/staging/iio/Makefile | 1 + >> drivers/staging/iio/amplifiers/Kconfig | 17 +++ >> drivers/staging/iio/amplifiers/Makefile | 5 + >> drivers/staging/iio/amplifiers/ad8366.c | 228 +++++++++++++++++++++++++++++++ >> 5 files changed, 252 insertions(+), 0 deletions(-) >> create mode 100644 drivers/staging/iio/amplifiers/Kconfig >> create mode 100644 drivers/staging/iio/amplifiers/Makefile >> create mode 100644 drivers/staging/iio/amplifiers/ad8366.c >> >> diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig >> index 2b56823..fd7a164 100644 >> --- a/drivers/staging/iio/Kconfig >> +++ b/drivers/staging/iio/Kconfig >> @@ -59,6 +59,7 @@ config IIO_CONSUMERS_PER_TRIGGER >> source "drivers/staging/iio/accel/Kconfig" >> source "drivers/staging/iio/adc/Kconfig" >> source "drivers/staging/iio/addac/Kconfig" >> +source "drivers/staging/iio/amplifiers/Kconfig" >> source "drivers/staging/iio/cdc/Kconfig" >> source "drivers/staging/iio/dac/Kconfig" >> source "drivers/staging/iio/frequency/Kconfig" >> diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile >> index 0161095..c52cde3 100644 >> --- a/drivers/staging/iio/Makefile >> +++ b/drivers/staging/iio/Makefile >> @@ -20,6 +20,7 @@ obj-$(CONFIG_IIO_DUMMY_EVGEN) += iio_dummy_evgen.o >> obj-y += accel/ >> obj-y += adc/ >> obj-y += addac/ >> +obj-y += amplifiers/ >> obj-y += cdc/ >> obj-y += dac/ >> obj-y += frequency/ >> diff --git a/drivers/staging/iio/amplifiers/Kconfig b/drivers/staging/iio/amplifiers/Kconfig >> new file mode 100644 >> index 0000000..05d707e >> --- /dev/null >> +++ b/drivers/staging/iio/amplifiers/Kconfig >> @@ -0,0 +1,17 @@ >> +# >> +# Gain Amplifiers, etc. >> +# >> +menu "Amplifiers" >> + >> +config AD8366 >> + tristate "Analog Devices AD8366 VGA" >> + depends on SPI >> + select BITREVERSE >> + help >> + Say yes here to build support for Analog Devices AD8366 >> + SPI Dual-Digital Variable Gain Amplifier (VGA). > Hmm.. interesting choice of acronym! Just for the unknowledgable > amongst us, what's the difference between a vga and a pga? PGA = Programmable Gain Amplifier VGA = Variable Gain Amplifier. That's a good question.IMHO VGA is the more widely use acronym. But I guess it depends who ever decided the title of the datasheet. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called ad8366. >> + >> +endmenu >> diff --git a/drivers/staging/iio/amplifiers/Makefile b/drivers/staging/iio/amplifiers/Makefile >> new file mode 100644 >> index 0000000..a6ca366 >> --- /dev/null >> +++ b/drivers/staging/iio/amplifiers/Makefile >> @@ -0,0 +1,5 @@ >> +# >> +# Makefile iio/amplifiers >> +# >> + >> +obj-$(CONFIG_AD8366) += ad8366.o >> diff --git a/drivers/staging/iio/amplifiers/ad8366.c b/drivers/staging/iio/amplifiers/ad8366.c >> new file mode 100644 >> index 0000000..187606f >> --- /dev/null >> +++ b/drivers/staging/iio/amplifiers/ad8366.c >> @@ -0,0 +1,228 @@ >> +/* >> + * AD8366 SPI Dual-Digital Variable Gain Amplifier (VGA) >> + * >> + * Copyright 2012 Analog Devices Inc. >> + * >> + * Licensed under the GPL-2. >> + */ >> + >> +#include<linux/device.h> >> +#include<linux/kernel.h> >> +#include<linux/slab.h> >> +#include<linux/sysfs.h> >> +#include<linux/spi/spi.h> >> +#include<linux/regulator/consumer.h> >> +#include<linux/err.h> >> +#include<linux/module.h> >> +#include<linux/bitrev.h> >> + >> +#include "../iio.h" >> +#include "../sysfs.h" >> + >> +struct ad8366_state { >> + struct spi_device *spi; >> + struct regulator *reg; >> + unsigned char ch_a; >> + unsigned char ch_b; >> + /* >> + * DMA (thus cache coherency maintenance) requires the >> + * transfer buffers to live in their own cache lines. >> + */ >> + unsigned char data[2] ____cacheline_aligned; >> +}; >> + >> +static int ad8366_write(struct iio_dev *indio_dev, >> + unsigned char ch_a, char unsigned ch_b) >> +{ >> + struct ad8366_state *st = iio_priv(indio_dev); >> + int ret; >> + >> + ch_a = bitrev8(ch_a& 0x3F); >> + ch_b = bitrev8(ch_b& 0x3F); >> + >> + st->data[0] = ch_b>> 4; >> + st->data[1] = (ch_b<< 4) | (ch_a>> 2); >> + >> + ret = spi_write(st->spi, st->data, ARRAY_SIZE(st->data)); >> + if (ret< 0) >> + dev_err(&indio_dev->dev, "write failed (%d)", ret); >> + >> + return ret; >> +} >> + >> +static int ad8366_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int *val, >> + int *val2, >> + long m) >> +{ >> + struct ad8366_state *st = iio_priv(indio_dev); >> + int ret; >> + unsigned code; >> + >> + mutex_lock(&indio_dev->mlock); >> + switch (m) { >> + case 0: >> + if (chan->channel == 0) >> + code = st->ch_a; >> + else >> + code = st->ch_b; >> + >> + /* Values in dB */ >> + code = code * 253 + 4500; >> + *val = code / 1000; >> + *val2 = (code % 1000) * 1000; >> + >> + ret = IIO_VAL_INT_PLUS_MICRO; >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + mutex_unlock(&indio_dev->mlock); >> + >> + return ret; >> +}; >> + >> +static int ad8366_write_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int val, >> + int val2, >> + long mask) >> +{ >> + struct ad8366_state *st = iio_priv(indio_dev); >> + unsigned code; >> + int ret; >> + >> + /* Values in dB */ >> + code = (((u8)val * 1000) + ((u32)val2 / 1000)); >> + >> + if (code> 20500 || code< 4500) >> + return -EINVAL; >> + >> + code = (code - 4500) / 253; >> + >> + mutex_lock(&indio_dev->mlock); >> + switch (mask) { >> + case 0: >> + if (chan->channel == 0) >> + st->ch_a = code; >> + else >> + st->ch_b = code; >> + >> + ret = ad8366_write(indio_dev, st->ch_a, st->ch_b); >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + mutex_unlock(&indio_dev->mlock); >> + >> + return ret; >> +} >> + >> +static const struct iio_info ad8366_info = { >> + .read_raw =&ad8366_read_raw, >> + .write_raw =&ad8366_write_raw, >> + .driver_module = THIS_MODULE, >> +}; >> + >> +#define AD8366_CHAN(_channel, _name) { \ >> + .type = IIO_ALTVOLTAGE, \ >> + .output = 1, \ >> + .indexed = 1, \ >> + .processed_val = 1, \ >> + .channel = _channel, \ >> + .extend_name = _name, \ >> +} >> + >> +static const struct iio_chan_spec ad8366_channels[] = { >> + AD8366_CHAN(0, "gain"), >> + AD8366_CHAN(1, "gain"), >> +}; >> + >> +static int __devinit ad8366_probe(struct spi_device *spi) >> +{ >> + struct iio_dev *indio_dev; >> + struct ad8366_state *st; >> + int ret; >> + >> + indio_dev = iio_allocate_device(sizeof(*st)); >> + if (indio_dev == NULL) >> + return -ENOMEM; >> + >> + st = iio_priv(indio_dev); >> + >> + st->reg = regulator_get(&spi->dev, "vcc"); >> + if (!IS_ERR(st->reg)) { >> + ret = regulator_enable(st->reg); >> + if (ret) >> + goto error_put_reg; >> + } >> + >> + spi_set_drvdata(spi, indio_dev); >> + st->spi = spi; >> + >> + indio_dev->dev.parent =&spi->dev; >> + indio_dev->name = spi_get_device_id(spi)->name; >> + indio_dev->info =&ad8366_info; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->channels = ad8366_channels; >> + indio_dev->num_channels = ARRAY_SIZE(ad8366_channels); >> + >> + ret = iio_device_register(indio_dev); >> + if (ret) >> + goto error_disable_reg; >> + >> + ad8366_write(indio_dev, 0 , 0); >> + >> + return 0; >> + >> +error_disable_reg: >> + if (!IS_ERR(st->reg)) >> + regulator_disable(st->reg); >> +error_put_reg: >> + if (!IS_ERR(st->reg)) >> + regulator_put(st->reg); >> + >> + iio_free_device(indio_dev); >> + >> + return ret; >> +} >> + >> +static int __devexit ad8366_remove(struct spi_device *spi) >> +{ >> + struct iio_dev *indio_dev = spi_get_drvdata(spi); >> + struct ad8366_state *st = iio_priv(indio_dev); >> + struct regulator *reg = st->reg; >> + >> + iio_device_unregister(indio_dev); >> + >> + if (!IS_ERR(reg)) { >> + regulator_disable(reg); >> + regulator_put(reg); >> + } >> + >> + iio_free_device(indio_dev); >> + >> + return 0; >> +} >> + >> +static const struct spi_device_id ad8366_id[] = { >> + {"ad8366", 0}, >> + {} >> +}; >> + >> +static struct spi_driver ad8366_driver = { >> + .driver = { >> + .name = KBUILD_MODNAME, >> + .owner = THIS_MODULE, >> + }, >> + .probe = ad8366_probe, >> + .remove = __devexit_p(ad8366_remove), >> + .id_table = ad8366_id, >> +}; >> + >> +module_spi_driver(ad8366_driver); >> + >> +MODULE_AUTHOR("Michael Hennerich<hennerich@blackfin.uclinux.org>"); >> +MODULE_DESCRIPTION("Analog Devices AD8366 VGA"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 1.7.0.4 >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Greetings, Michael -- Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-03-22 8:52 ` Michael Hennerich @ 2012-03-22 9:10 ` Jonathan Cameron 2012-03-22 9:53 ` Michael Hennerich 2012-05-07 15:00 ` Michael Hennerich 0 siblings, 2 replies; 16+ messages in thread From: Jonathan Cameron @ 2012-03-22 9:10 UTC (permalink / raw) To: michael.hennerich Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 3/22/2012 8:52 AM, Michael Hennerich wrote: > On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>> From: Michael Hennerich<michael.hennerich@analog.com> >> Sorry for the slow response on this one. Been off sick... >> >> Anyhow, I'm still not sure what the right interface for this type >> of device is. >> >> The obvious options are: >> >> 1) Make gain an IIO type (doesn't make much sense as gain is only going >> to be of one particular existing type). >> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use extend >> name. Any real reason for picking altvoltage rather than voltage? > I'm open for advice. Since I made the amplifier being an OUT type device > I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. > Some VGAs/PGAs work from DC, but typically VGAs are HF devices. Hmm.. Don't suppose it really matters but we ought to aim for consistency (by review) at least. This particular part is DC through to 600MHz. > >> Clearly gain has the same meaning in either case (assuming it's linear). >> 3) Make a change to core to allow a channel to have elements in >> info_mask but not actually to have a raw access. Not entirely sure >> how we will do that cleanly. Also it's not clear whether the gain >> would be an IN or an OUT channel type! > Well - having the ability for channels without raw access element > would be of interest. True enough. Cleanest way to do this that I can think of is to make a tree wide change to add the raw element to the info_mask. We could allow for a zero info_mask value actually being the equivalent of having only a raw channel. It's invasive but if we agreee it should be done now is probably the best time to do it (just post merge window etc). Whilst here, we clearly need way of destinguishing values in DB from linear gains. Could add a new return type for read_raw callbacks? >> All thoughts on this welcome. Maybe we have to decide to leave it >> for now and get back to the question at a later date? >> >> The driver is clean and short so it's only really the above question of >> approach that needs answering to my mind... I guess you feel that >> what you have here is the best plan but I'd like a few more views on >> this before we set the precedence.... >> >> I've pulled Mark into the cc as he might have come across similar >> devices from the audio side of things and hence have some experience >> around this question... >>> Signed-off-by: Michael Hennerich<michael.hennerich@analog.com> >>> --- >>> drivers/staging/iio/Kconfig | 1 + >>> drivers/staging/iio/Makefile | 1 + >>> drivers/staging/iio/amplifiers/Kconfig | 17 +++ >>> drivers/staging/iio/amplifiers/Makefile | 5 + >>> drivers/staging/iio/amplifiers/ad8366.c | 228 >>> +++++++++++++++++++++++++++++++ >>> 5 files changed, 252 insertions(+), 0 deletions(-) >>> create mode 100644 drivers/staging/iio/amplifiers/Kconfig >>> create mode 100644 drivers/staging/iio/amplifiers/Makefile >>> create mode 100644 drivers/staging/iio/amplifiers/ad8366.c >>> >>> diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig >>> index 2b56823..fd7a164 100644 >>> --- a/drivers/staging/iio/Kconfig >>> +++ b/drivers/staging/iio/Kconfig >>> @@ -59,6 +59,7 @@ config IIO_CONSUMERS_PER_TRIGGER >>> source "drivers/staging/iio/accel/Kconfig" >>> source "drivers/staging/iio/adc/Kconfig" >>> source "drivers/staging/iio/addac/Kconfig" >>> +source "drivers/staging/iio/amplifiers/Kconfig" >>> source "drivers/staging/iio/cdc/Kconfig" >>> source "drivers/staging/iio/dac/Kconfig" >>> source "drivers/staging/iio/frequency/Kconfig" >>> diff --git a/drivers/staging/iio/Makefile >>> b/drivers/staging/iio/Makefile >>> index 0161095..c52cde3 100644 >>> --- a/drivers/staging/iio/Makefile >>> +++ b/drivers/staging/iio/Makefile >>> @@ -20,6 +20,7 @@ obj-$(CONFIG_IIO_DUMMY_EVGEN) += iio_dummy_evgen.o >>> obj-y += accel/ >>> obj-y += adc/ >>> obj-y += addac/ >>> +obj-y += amplifiers/ >>> obj-y += cdc/ >>> obj-y += dac/ >>> obj-y += frequency/ >>> diff --git a/drivers/staging/iio/amplifiers/Kconfig >>> b/drivers/staging/iio/amplifiers/Kconfig >>> new file mode 100644 >>> index 0000000..05d707e >>> --- /dev/null >>> +++ b/drivers/staging/iio/amplifiers/Kconfig >>> @@ -0,0 +1,17 @@ >>> +# >>> +# Gain Amplifiers, etc. >>> +# >>> +menu "Amplifiers" >>> + >>> +config AD8366 >>> + tristate "Analog Devices AD8366 VGA" >>> + depends on SPI >>> + select BITREVERSE >>> + help >>> + Say yes here to build support for Analog Devices AD8366 >>> + SPI Dual-Digital Variable Gain Amplifier (VGA). >> Hmm.. interesting choice of acronym! Just for the unknowledgable >> amongst us, what's the difference between a vga and a pga? > > PGA = Programmable Gain Amplifier > VGA = Variable Gain Amplifier. > That's a good question.IMHO VGA is the more widely > use acronym. But I guess it depends who ever decided the title > of the datasheet. Fair enough. I've curiously hit a lot of PGAs but VGA was a new one to me ;) > >>> + >>> + To compile this driver as a module, choose M here: the >>> + module will be called ad8366. >>> + >>> +endmenu >>> diff --git a/drivers/staging/iio/amplifiers/Makefile >>> b/drivers/staging/iio/amplifiers/Makefile >>> new file mode 100644 >>> index 0000000..a6ca366 >>> --- /dev/null >>> +++ b/drivers/staging/iio/amplifiers/Makefile >>> @@ -0,0 +1,5 @@ >>> +# >>> +# Makefile iio/amplifiers >>> +# >>> + >>> +obj-$(CONFIG_AD8366) += ad8366.o >>> diff --git a/drivers/staging/iio/amplifiers/ad8366.c >>> b/drivers/staging/iio/amplifiers/ad8366.c >>> new file mode 100644 >>> index 0000000..187606f >>> --- /dev/null >>> +++ b/drivers/staging/iio/amplifiers/ad8366.c >>> @@ -0,0 +1,228 @@ >>> +/* >>> + * AD8366 SPI Dual-Digital Variable Gain Amplifier (VGA) >>> + * >>> + * Copyright 2012 Analog Devices Inc. >>> + * >>> + * Licensed under the GPL-2. >>> + */ >>> + >>> +#include<linux/device.h> >>> +#include<linux/kernel.h> >>> +#include<linux/slab.h> >>> +#include<linux/sysfs.h> >>> +#include<linux/spi/spi.h> >>> +#include<linux/regulator/consumer.h> >>> +#include<linux/err.h> >>> +#include<linux/module.h> >>> +#include<linux/bitrev.h> >>> + >>> +#include "../iio.h" >>> +#include "../sysfs.h" >>> + >>> +struct ad8366_state { >>> + struct spi_device *spi; >>> + struct regulator *reg; >>> + unsigned char ch_a; >>> + unsigned char ch_b; >>> + /* >>> + * DMA (thus cache coherency maintenance) requires the >>> + * transfer buffers to live in their own cache lines. >>> + */ >>> + unsigned char data[2] ____cacheline_aligned; >>> +}; >>> + >>> +static int ad8366_write(struct iio_dev *indio_dev, >>> + unsigned char ch_a, char unsigned ch_b) >>> +{ >>> + struct ad8366_state *st = iio_priv(indio_dev); >>> + int ret; >>> + >>> + ch_a = bitrev8(ch_a& 0x3F); >>> + ch_b = bitrev8(ch_b& 0x3F); >>> + >>> + st->data[0] = ch_b>> 4; >>> + st->data[1] = (ch_b<< 4) | (ch_a>> 2); >>> + >>> + ret = spi_write(st->spi, st->data, ARRAY_SIZE(st->data)); >>> + if (ret< 0) >>> + dev_err(&indio_dev->dev, "write failed (%d)", ret); >>> + >>> + return ret; >>> +} >>> + >>> +static int ad8366_read_raw(struct iio_dev *indio_dev, >>> + struct iio_chan_spec const *chan, >>> + int *val, >>> + int *val2, >>> + long m) >>> +{ >>> + struct ad8366_state *st = iio_priv(indio_dev); >>> + int ret; >>> + unsigned code; >>> + >>> + mutex_lock(&indio_dev->mlock); >>> + switch (m) { >>> + case 0: >>> + if (chan->channel == 0) >>> + code = st->ch_a; >>> + else >>> + code = st->ch_b; >>> + >>> + /* Values in dB */ >>> + code = code * 253 + 4500; >>> + *val = code / 1000; >>> + *val2 = (code % 1000) * 1000; >>> + >>> + ret = IIO_VAL_INT_PLUS_MICRO; >>> + break; >>> + default: >>> + ret = -EINVAL; >>> + } >>> + mutex_unlock(&indio_dev->mlock); >>> + >>> + return ret; >>> +}; >>> + >>> +static int ad8366_write_raw(struct iio_dev *indio_dev, >>> + struct iio_chan_spec const *chan, >>> + int val, >>> + int val2, >>> + long mask) >>> +{ >>> + struct ad8366_state *st = iio_priv(indio_dev); >>> + unsigned code; >>> + int ret; >>> + >>> + /* Values in dB */ >>> + code = (((u8)val * 1000) + ((u32)val2 / 1000)); >>> + >>> + if (code> 20500 || code< 4500) >>> + return -EINVAL; >>> + >>> + code = (code - 4500) / 253; >>> + >>> + mutex_lock(&indio_dev->mlock); >>> + switch (mask) { >>> + case 0: >>> + if (chan->channel == 0) >>> + st->ch_a = code; >>> + else >>> + st->ch_b = code; >>> + >>> + ret = ad8366_write(indio_dev, st->ch_a, st->ch_b); >>> + break; >>> + default: >>> + ret = -EINVAL; >>> + } >>> + mutex_unlock(&indio_dev->mlock); >>> + >>> + return ret; >>> +} >>> + >>> +static const struct iio_info ad8366_info = { >>> + .read_raw =&ad8366_read_raw, >>> + .write_raw =&ad8366_write_raw, >>> + .driver_module = THIS_MODULE, >>> +}; >>> + >>> +#define AD8366_CHAN(_channel, _name) { \ >>> + .type = IIO_ALTVOLTAGE, \ >>> + .output = 1, \ >>> + .indexed = 1, \ >>> + .processed_val = 1, \ >>> + .channel = _channel, \ >>> + .extend_name = _name, \ >>> +} >>> + >>> +static const struct iio_chan_spec ad8366_channels[] = { >>> + AD8366_CHAN(0, "gain"), >>> + AD8366_CHAN(1, "gain"), >>> +}; >>> + >>> +static int __devinit ad8366_probe(struct spi_device *spi) >>> +{ >>> + struct iio_dev *indio_dev; >>> + struct ad8366_state *st; >>> + int ret; >>> + >>> + indio_dev = iio_allocate_device(sizeof(*st)); >>> + if (indio_dev == NULL) >>> + return -ENOMEM; >>> + >>> + st = iio_priv(indio_dev); >>> + >>> + st->reg = regulator_get(&spi->dev, "vcc"); >>> + if (!IS_ERR(st->reg)) { >>> + ret = regulator_enable(st->reg); >>> + if (ret) >>> + goto error_put_reg; >>> + } >>> + >>> + spi_set_drvdata(spi, indio_dev); >>> + st->spi = spi; >>> + >>> + indio_dev->dev.parent =&spi->dev; >>> + indio_dev->name = spi_get_device_id(spi)->name; >>> + indio_dev->info =&ad8366_info; >>> + indio_dev->modes = INDIO_DIRECT_MODE; >>> + indio_dev->channels = ad8366_channels; >>> + indio_dev->num_channels = ARRAY_SIZE(ad8366_channels); >>> + >>> + ret = iio_device_register(indio_dev); >>> + if (ret) >>> + goto error_disable_reg; >>> + >>> + ad8366_write(indio_dev, 0 , 0); >>> + >>> + return 0; >>> + >>> +error_disable_reg: >>> + if (!IS_ERR(st->reg)) >>> + regulator_disable(st->reg); >>> +error_put_reg: >>> + if (!IS_ERR(st->reg)) >>> + regulator_put(st->reg); >>> + >>> + iio_free_device(indio_dev); >>> + >>> + return ret; >>> +} >>> + >>> +static int __devexit ad8366_remove(struct spi_device *spi) >>> +{ >>> + struct iio_dev *indio_dev = spi_get_drvdata(spi); >>> + struct ad8366_state *st = iio_priv(indio_dev); >>> + struct regulator *reg = st->reg; >>> + >>> + iio_device_unregister(indio_dev); >>> + >>> + if (!IS_ERR(reg)) { >>> + regulator_disable(reg); >>> + regulator_put(reg); >>> + } >>> + >>> + iio_free_device(indio_dev); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct spi_device_id ad8366_id[] = { >>> + {"ad8366", 0}, >>> + {} >>> +}; >>> + >>> +static struct spi_driver ad8366_driver = { >>> + .driver = { >>> + .name = KBUILD_MODNAME, >>> + .owner = THIS_MODULE, >>> + }, >>> + .probe = ad8366_probe, >>> + .remove = __devexit_p(ad8366_remove), >>> + .id_table = ad8366_id, >>> +}; >>> + >>> +module_spi_driver(ad8366_driver); >>> + >>> +MODULE_AUTHOR("Michael Hennerich<hennerich@blackfin.uclinux.org>"); >>> +MODULE_DESCRIPTION("Analog Devices AD8366 VGA"); >>> +MODULE_LICENSE("GPL v2"); >>> -- >>> 1.7.0.4 >>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-03-22 9:10 ` Jonathan Cameron @ 2012-03-22 9:53 ` Michael Hennerich 2012-03-22 9:59 ` Jonathan Cameron 2012-05-07 15:00 ` Michael Hennerich 1 sibling, 1 reply; 16+ messages in thread From: Michael Hennerich @ 2012-03-22 9:53 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 03/22/2012 10:10 AM, Jonathan Cameron wrote: > On 3/22/2012 8:52 AM, Michael Hennerich wrote: >> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>> From: Michael Hennerich<michael.hennerich@analog.com> >>> Sorry for the slow response on this one. Been off sick... >>> >>> Anyhow, I'm still not sure what the right interface for this type >>> of device is. >>> >>> The obvious options are: >>> >>> 1) Make gain an IIO type (doesn't make much sense as gain is only going >>> to be of one particular existing type). >>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use extend >>> name. Any real reason for picking altvoltage rather than voltage? >> I'm open for advice. Since I made the amplifier being an OUT type device >> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. > Hmm.. Don't suppose it really matters but we ought to aim for consistency > (by review) at least. This particular part is DC through to 600MHz. >>> Clearly gain has the same meaning in either case (assuming it's linear). >>> 3) Make a change to core to allow a channel to have elements in >>> info_mask but not actually to have a raw access. Not entirely sure >>> how we will do that cleanly. Also it's not clear whether the gain >>> would be an IN or an OUT channel type! >> Well - having the ability for channels without raw access element >> would be of interest. > True enough. Cleanest way to do this that I can think of is to make a tree > wide change to add the raw element to the info_mask. We could allow for > a zero info_mask value actually being the equivalent of having only a raw > channel. It's invasive but if we agreee it should be done now is > probably the > best time to do it (just post merge window etc). Well - to make the change less invasive - we could use reversed logic add a new info_mask element IIO_CHAN_INFO_NO_RAW=0. We already reserve 0... > Whilst here, we clearly need way of destinguishing values in DB from linear > gains. Could add a new return type for read_raw callbacks? That should work. ... and the core adds dB to the string? >>> All thoughts on this welcome. Maybe we have to decide to leave it >>> for now and get back to the question at a later date? >>> >>> The driver is clean and short so it's only really the above question of >>> approach that needs answering to my mind... I guess you feel that >>> what you have here is the best plan but I'd like a few more views on >>> this before we set the precedence.... >>> >>> I've pulled Mark into the cc as he might have come across similar >>> devices from the audio side of things and hence have some experience >>> around this question... >>>> Signed-off-by: Michael Hennerich<michael.hennerich@analog.com> >>>> --- >>>> drivers/staging/iio/Kconfig | 1 + >>>> drivers/staging/iio/Makefile | 1 + >>>> drivers/staging/iio/amplifiers/Kconfig | 17 +++ >>>> drivers/staging/iio/amplifiers/Makefile | 5 + >>>> drivers/staging/iio/amplifiers/ad8366.c | 228 >>>> +++++++++++++++++++++++++++++++ >>>> 5 files changed, 252 insertions(+), 0 deletions(-) >>>> create mode 100644 drivers/staging/iio/amplifiers/Kconfig >>>> create mode 100644 drivers/staging/iio/amplifiers/Makefile >>>> create mode 100644 drivers/staging/iio/amplifiers/ad8366.c >>>> >>>> diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig >>>> index 2b56823..fd7a164 100644 >>>> --- a/drivers/staging/iio/Kconfig >>>> +++ b/drivers/staging/iio/Kconfig >>>> @@ -59,6 +59,7 @@ config IIO_CONSUMERS_PER_TRIGGER >>>> source "drivers/staging/iio/accel/Kconfig" >>>> source "drivers/staging/iio/adc/Kconfig" >>>> source "drivers/staging/iio/addac/Kconfig" >>>> +source "drivers/staging/iio/amplifiers/Kconfig" >>>> source "drivers/staging/iio/cdc/Kconfig" >>>> source "drivers/staging/iio/dac/Kconfig" >>>> source "drivers/staging/iio/frequency/Kconfig" >>>> diff --git a/drivers/staging/iio/Makefile >>>> b/drivers/staging/iio/Makefile >>>> index 0161095..c52cde3 100644 >>>> --- a/drivers/staging/iio/Makefile >>>> +++ b/drivers/staging/iio/Makefile >>>> @@ -20,6 +20,7 @@ obj-$(CONFIG_IIO_DUMMY_EVGEN) += iio_dummy_evgen.o >>>> obj-y += accel/ >>>> obj-y += adc/ >>>> obj-y += addac/ >>>> +obj-y += amplifiers/ >>>> obj-y += cdc/ >>>> obj-y += dac/ >>>> obj-y += frequency/ >>>> diff --git a/drivers/staging/iio/amplifiers/Kconfig >>>> b/drivers/staging/iio/amplifiers/Kconfig >>>> new file mode 100644 >>>> index 0000000..05d707e >>>> --- /dev/null >>>> +++ b/drivers/staging/iio/amplifiers/Kconfig >>>> @@ -0,0 +1,17 @@ >>>> +# >>>> +# Gain Amplifiers, etc. >>>> +# >>>> +menu "Amplifiers" >>>> + >>>> +config AD8366 >>>> + tristate "Analog Devices AD8366 VGA" >>>> + depends on SPI >>>> + select BITREVERSE >>>> + help >>>> + Say yes here to build support for Analog Devices AD8366 >>>> + SPI Dual-Digital Variable Gain Amplifier (VGA). >>> Hmm.. interesting choice of acronym! Just for the unknowledgable >>> amongst us, what's the difference between a vga and a pga? >> PGA = Programmable Gain Amplifier >> VGA = Variable Gain Amplifier. >> That's a good question.IMHO VGA is the more widely >> use acronym. But I guess it depends who ever decided the title >> of the datasheet. > Fair enough. I've curiously hit a lot of PGAs but VGA was a new one > to me ;) >>>> + >>>> + To compile this driver as a module, choose M here: the >>>> + module will be called ad8366. >>>> + >>>> +endmenu >>>> diff --git a/drivers/staging/iio/amplifiers/Makefile >>>> b/drivers/staging/iio/amplifiers/Makefile >>>> new file mode 100644 >>>> index 0000000..a6ca366 >>>> --- /dev/null >>>> +++ b/drivers/staging/iio/amplifiers/Makefile >>>> @@ -0,0 +1,5 @@ >>>> +# >>>> +# Makefile iio/amplifiers >>>> +# >>>> + >>>> +obj-$(CONFIG_AD8366) += ad8366.o >>>> diff --git a/drivers/staging/iio/amplifiers/ad8366.c >>>> b/drivers/staging/iio/amplifiers/ad8366.c >>>> new file mode 100644 >>>> index 0000000..187606f >>>> --- /dev/null >>>> +++ b/drivers/staging/iio/amplifiers/ad8366.c >>>> @@ -0,0 +1,228 @@ >>>> +/* >>>> + * AD8366 SPI Dual-Digital Variable Gain Amplifier (VGA) >>>> + * >>>> + * Copyright 2012 Analog Devices Inc. >>>> + * >>>> + * Licensed under the GPL-2. >>>> + */ >>>> + >>>> +#include<linux/device.h> >>>> +#include<linux/kernel.h> >>>> +#include<linux/slab.h> >>>> +#include<linux/sysfs.h> >>>> +#include<linux/spi/spi.h> >>>> +#include<linux/regulator/consumer.h> >>>> +#include<linux/err.h> >>>> +#include<linux/module.h> >>>> +#include<linux/bitrev.h> >>>> + >>>> +#include "../iio.h" >>>> +#include "../sysfs.h" >>>> + >>>> +struct ad8366_state { >>>> + struct spi_device *spi; >>>> + struct regulator *reg; >>>> + unsigned char ch_a; >>>> + unsigned char ch_b; >>>> + /* >>>> + * DMA (thus cache coherency maintenance) requires the >>>> + * transfer buffers to live in their own cache lines. >>>> + */ >>>> + unsigned char data[2] ____cacheline_aligned; >>>> +}; >>>> + >>>> +static int ad8366_write(struct iio_dev *indio_dev, >>>> + unsigned char ch_a, char unsigned ch_b) >>>> +{ >>>> + struct ad8366_state *st = iio_priv(indio_dev); >>>> + int ret; >>>> + >>>> + ch_a = bitrev8(ch_a& 0x3F); >>>> + ch_b = bitrev8(ch_b& 0x3F); >>>> + >>>> + st->data[0] = ch_b>> 4; >>>> + st->data[1] = (ch_b<< 4) | (ch_a>> 2); >>>> + >>>> + ret = spi_write(st->spi, st->data, ARRAY_SIZE(st->data)); >>>> + if (ret< 0) >>>> + dev_err(&indio_dev->dev, "write failed (%d)", ret); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int ad8366_read_raw(struct iio_dev *indio_dev, >>>> + struct iio_chan_spec const *chan, >>>> + int *val, >>>> + int *val2, >>>> + long m) >>>> +{ >>>> + struct ad8366_state *st = iio_priv(indio_dev); >>>> + int ret; >>>> + unsigned code; >>>> + >>>> + mutex_lock(&indio_dev->mlock); >>>> + switch (m) { >>>> + case 0: >>>> + if (chan->channel == 0) >>>> + code = st->ch_a; >>>> + else >>>> + code = st->ch_b; >>>> + >>>> + /* Values in dB */ >>>> + code = code * 253 + 4500; >>>> + *val = code / 1000; >>>> + *val2 = (code % 1000) * 1000; >>>> + >>>> + ret = IIO_VAL_INT_PLUS_MICRO; >>>> + break; >>>> + default: >>>> + ret = -EINVAL; >>>> + } >>>> + mutex_unlock(&indio_dev->mlock); >>>> + >>>> + return ret; >>>> +}; >>>> + >>>> +static int ad8366_write_raw(struct iio_dev *indio_dev, >>>> + struct iio_chan_spec const *chan, >>>> + int val, >>>> + int val2, >>>> + long mask) >>>> +{ >>>> + struct ad8366_state *st = iio_priv(indio_dev); >>>> + unsigned code; >>>> + int ret; >>>> + >>>> + /* Values in dB */ >>>> + code = (((u8)val * 1000) + ((u32)val2 / 1000)); >>>> + >>>> + if (code> 20500 || code< 4500) >>>> + return -EINVAL; >>>> + >>>> + code = (code - 4500) / 253; >>>> + >>>> + mutex_lock(&indio_dev->mlock); >>>> + switch (mask) { >>>> + case 0: >>>> + if (chan->channel == 0) >>>> + st->ch_a = code; >>>> + else >>>> + st->ch_b = code; >>>> + >>>> + ret = ad8366_write(indio_dev, st->ch_a, st->ch_b); >>>> + break; >>>> + default: >>>> + ret = -EINVAL; >>>> + } >>>> + mutex_unlock(&indio_dev->mlock); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static const struct iio_info ad8366_info = { >>>> + .read_raw =&ad8366_read_raw, >>>> + .write_raw =&ad8366_write_raw, >>>> + .driver_module = THIS_MODULE, >>>> +}; >>>> + >>>> +#define AD8366_CHAN(_channel, _name) { \ >>>> + .type = IIO_ALTVOLTAGE, \ >>>> + .output = 1, \ >>>> + .indexed = 1, \ >>>> + .processed_val = 1, \ >>>> + .channel = _channel, \ >>>> + .extend_name = _name, \ >>>> +} >>>> + >>>> +static const struct iio_chan_spec ad8366_channels[] = { >>>> + AD8366_CHAN(0, "gain"), >>>> + AD8366_CHAN(1, "gain"), >>>> +}; >>>> + >>>> +static int __devinit ad8366_probe(struct spi_device *spi) >>>> +{ >>>> + struct iio_dev *indio_dev; >>>> + struct ad8366_state *st; >>>> + int ret; >>>> + >>>> + indio_dev = iio_allocate_device(sizeof(*st)); >>>> + if (indio_dev == NULL) >>>> + return -ENOMEM; >>>> + >>>> + st = iio_priv(indio_dev); >>>> + >>>> + st->reg = regulator_get(&spi->dev, "vcc"); >>>> + if (!IS_ERR(st->reg)) { >>>> + ret = regulator_enable(st->reg); >>>> + if (ret) >>>> + goto error_put_reg; >>>> + } >>>> + >>>> + spi_set_drvdata(spi, indio_dev); >>>> + st->spi = spi; >>>> + >>>> + indio_dev->dev.parent =&spi->dev; >>>> + indio_dev->name = spi_get_device_id(spi)->name; >>>> + indio_dev->info =&ad8366_info; >>>> + indio_dev->modes = INDIO_DIRECT_MODE; >>>> + indio_dev->channels = ad8366_channels; >>>> + indio_dev->num_channels = ARRAY_SIZE(ad8366_channels); >>>> + >>>> + ret = iio_device_register(indio_dev); >>>> + if (ret) >>>> + goto error_disable_reg; >>>> + >>>> + ad8366_write(indio_dev, 0 , 0); >>>> + >>>> + return 0; >>>> + >>>> +error_disable_reg: >>>> + if (!IS_ERR(st->reg)) >>>> + regulator_disable(st->reg); >>>> +error_put_reg: >>>> + if (!IS_ERR(st->reg)) >>>> + regulator_put(st->reg); >>>> + >>>> + iio_free_device(indio_dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int __devexit ad8366_remove(struct spi_device *spi) >>>> +{ >>>> + struct iio_dev *indio_dev = spi_get_drvdata(spi); >>>> + struct ad8366_state *st = iio_priv(indio_dev); >>>> + struct regulator *reg = st->reg; >>>> + >>>> + iio_device_unregister(indio_dev); >>>> + >>>> + if (!IS_ERR(reg)) { >>>> + regulator_disable(reg); >>>> + regulator_put(reg); >>>> + } >>>> + >>>> + iio_free_device(indio_dev); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct spi_device_id ad8366_id[] = { >>>> + {"ad8366", 0}, >>>> + {} >>>> +}; >>>> + >>>> +static struct spi_driver ad8366_driver = { >>>> + .driver = { >>>> + .name = KBUILD_MODNAME, >>>> + .owner = THIS_MODULE, >>>> + }, >>>> + .probe = ad8366_probe, >>>> + .remove = __devexit_p(ad8366_remove), >>>> + .id_table = ad8366_id, >>>> +}; >>>> + >>>> +module_spi_driver(ad8366_driver); >>>> + >>>> +MODULE_AUTHOR("Michael Hennerich<hennerich@blackfin.uclinux.org>"); >>>> +MODULE_DESCRIPTION("Analog Devices AD8366 VGA"); >>>> +MODULE_LICENSE("GPL v2"); >>>> -- >>>> 1.7.0.4 >>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Greetings, Michael -- Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-03-22 9:53 ` Michael Hennerich @ 2012-03-22 9:59 ` Jonathan Cameron 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Cameron @ 2012-03-22 9:59 UTC (permalink / raw) To: michael.hennerich Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 3/22/2012 9:53 AM, Michael Hennerich wrote: > On 03/22/2012 10:10 AM, Jonathan Cameron wrote: >> On 3/22/2012 8:52 AM, Michael Hennerich wrote: >>> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>>> From: Michael Hennerich<michael.hennerich@analog.com> >>>> Sorry for the slow response on this one. Been off sick... >>>> >>>> Anyhow, I'm still not sure what the right interface for this type >>>> of device is. >>>> >>>> The obvious options are: >>>> >>>> 1) Make gain an IIO type (doesn't make much sense as gain is only >>>> going >>>> to be of one particular existing type). >>>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use >>>> extend >>>> name. Any real reason for picking altvoltage rather than voltage? >>> I'm open for advice. Since I made the amplifier being an OUT type >>> device >>> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >>> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. >> Hmm.. Don't suppose it really matters but we ought to aim for >> consistency >> (by review) at least. This particular part is DC through to 600MHz. >>>> Clearly gain has the same meaning in either case (assuming it's >>>> linear). >>>> 3) Make a change to core to allow a channel to have elements in >>>> info_mask but not actually to have a raw access. Not entirely sure >>>> how we will do that cleanly. Also it's not clear whether the gain >>>> would be an IN or an OUT channel type! >>> Well - having the ability for channels without raw access element >>> would be of interest. >> True enough. Cleanest way to do this that I can think of is to make >> a tree >> wide change to add the raw element to the info_mask. We could allow for >> a zero info_mask value actually being the equivalent of having only a >> raw >> channel. It's invasive but if we agreee it should be done now is >> probably the >> best time to do it (just post merge window etc). > Well - to make the change less invasive - we could use reversed logic > add a new info_mask element IIO_CHAN_INFO_NO_RAW=0. > We already reserve 0... I wondered about doing exactly that as well. Less invasive but then we have a rather illogical setup... If we were out of staging then we'd probably go with that hack, but given we still have scope to make wholesale changes, lets do it properly... Almost every channel should have an info_mask set anyway (only exception is processed channels where nothing is controllable) an there are very very few of them. I'll do this change if we go with it as I have some patches clearling out IIO_CHAN macro usage once and for all and this is sure as heck going to clash! > >> Whilst here, we clearly need way of destinguishing values in DB from >> linear >> gains. Could add a new return type for read_raw callbacks? > That should work. ... and the core adds dB to the string? That's what I was thinking. Not ideal, but it's still reasonably machine readable and should work fine with in kernel users as well as long as the reader / writer is expecting it (or the boiler plate code is). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-03-22 9:10 ` Jonathan Cameron 2012-03-22 9:53 ` Michael Hennerich @ 2012-05-07 15:00 ` Michael Hennerich 2012-05-07 15:17 ` Michael Hennerich 1 sibling, 1 reply; 16+ messages in thread From: Michael Hennerich @ 2012-05-07 15:00 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 03/22/2012 10:10 AM, Jonathan Cameron wrote: > On 3/22/2012 8:52 AM, Michael Hennerich wrote: >> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>> From: Michael Hennerich<michael.hennerich@analog.com> >>> Sorry for the slow response on this one. Been off sick... >>> >>> Anyhow, I'm still not sure what the right interface for this type >>> of device is. >>> >>> The obvious options are: >>> >>> 1) Make gain an IIO type (doesn't make much sense as gain is only going >>> to be of one particular existing type). >>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use extend >>> name. Any real reason for picking altvoltage rather than voltage? >> I'm open for advice. Since I made the amplifier being an OUT type device >> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. > Hmm.. Don't suppose it really matters but we ought to aim for consistency > (by review) at least. This particular part is DC through to 600MHz. >>> Clearly gain has the same meaning in either case (assuming it's linear). >>> 3) Make a change to core to allow a channel to have elements in >>> info_mask but not actually to have a raw access. Not entirely sure >>> how we will do that cleanly. Also it's not clear whether the gain >>> would be an IN or an OUT channel type! >> Well - having the ability for channels without raw access element >> would be of interest. > True enough. Cleanest way to do this that I can think of is to make a tree > wide change to add the raw element to the info_mask. We could allow for > a zero info_mask value actually being the equivalent of having only a raw > channel. It's invasive but if we agreee it should be done now is > probably the > best time to do it (just post merge window etc). Hi Jonathan, Thanks for getting this in place. > > Whilst here, we clearly need way of destinguishing values in DB from linear > gains. Could add a new return type for read_raw callbacks? Does something like this work for you? Also wondering if we should introduce IIO_CHAN_INFO_GAIN for amplifier type devices? From e62f3a3de86f2edde2137237531732cdf5fc2ed8 Mon Sep 17 00:00:00 2001 From: Michael Hennerich <michael.hennerich@analog.com> Date: Mon, 7 May 2012 16:53:45 +0200 Subject: [PATCH] iio: core: introduce dB scle: IIO_VAL_INT_PLUS_MICRO_DB Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> --- drivers/iio/industrialio-core.c | 19 +++++++++++++------ include/linux/iio/types.h | 1 + 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 72e33b8..e436f6f 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -306,26 +306,33 @@ static ssize_t iio_read_channel_info(struct device *dev, struct iio_dev *indio_dev = dev_get_drvdata(dev); struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); int val, val2; + bool scale_db = false; int ret = indio_dev->info->read_raw(indio_dev, this_attr->c, &val, &val2, this_attr->address); if (ret < 0) return ret; - if (ret == IIO_VAL_INT) + switch (ret) { + case IIO_VAL_INT: return sprintf(buf, "%d\n", val); - else if (ret == IIO_VAL_INT_PLUS_MICRO) { + case IIO_VAL_INT_PLUS_MICRO_DB: + scale_db = true; + case IIO_VAL_INT_PLUS_MICRO: if (val2 < 0) - return sprintf(buf, "-%d.%06u\n", val, -val2); + return sprintf(buf, "-%d.%06u%s\n", val, -val2, + scale_db ? " db" : ""); else - return sprintf(buf, "%d.%06u\n", val, val2); - } else if (ret == IIO_VAL_INT_PLUS_NANO) { + return sprintf(buf, "%d.%06u%s\n", val, val2, + scale_db ? " db" : ""); + case IIO_VAL_INT_PLUS_NANO: if (val2 < 0) return sprintf(buf, "-%d.%09u\n", val, -val2); else return sprintf(buf, "%d.%09u\n", val, val2); - } else + default: return 0; + } } static ssize_t iio_write_channel_info(struct device *dev, diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h index a471fd5..1b073b1 100644 --- a/include/linux/iio/types.h +++ b/include/linux/iio/types.h @@ -50,5 +50,6 @@ enum iio_modifier { #define IIO_VAL_INT 1 #define IIO_VAL_INT_PLUS_MICRO 2 #define IIO_VAL_INT_PLUS_NANO 3 +#define IIO_VAL_INT_PLUS_MICRO_DB 4 #endif /* _IIO_TYPES_H_ */ -- 1.7.0.4 >>> All thoughts on this welcome. Maybe we have to decide to leave it >>> for now and get back to the question at a later date? >>> >>> The driver is clean and short so it's only really the above question of >>> approach that needs answering to my mind... I guess you feel that >>> what you have here is the best plan but I'd like a few more views on >>> this before we set the precedence.... >>> >>> I've pulled Mark into the cc as he might have come across similar >>> devices from the audio side of things and hence have some experience >>> around this question... >>>> Signed-off-by: Michael Hennerich<michael.hennerich@analog.com> >>>> --- >>>> drivers/staging/iio/Kconfig | 1 + >>>> drivers/staging/iio/Makefile | 1 + >>>> drivers/staging/iio/amplifiers/Kconfig | 17 +++ >>>> drivers/staging/iio/amplifiers/Makefile | 5 + >>>> drivers/staging/iio/amplifiers/ad8366.c | 228 >>>> +++++++++++++++++++++++++++++++ >>>> 5 files changed, 252 insertions(+), 0 deletions(-) >>>> create mode 100644 drivers/staging/iio/amplifiers/Kconfig >>>> create mode 100644 drivers/staging/iio/amplifiers/Makefile >>>> create mode 100644 drivers/staging/iio/amplifiers/ad8366.c >>>> >>>> diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig >>>> index 2b56823..fd7a164 100644 >>>> --- a/drivers/staging/iio/Kconfig >>>> +++ b/drivers/staging/iio/Kconfig >>>> @@ -59,6 +59,7 @@ config IIO_CONSUMERS_PER_TRIGGER >>>> source "drivers/staging/iio/accel/Kconfig" >>>> source "drivers/staging/iio/adc/Kconfig" >>>> source "drivers/staging/iio/addac/Kconfig" >>>> +source "drivers/staging/iio/amplifiers/Kconfig" >>>> source "drivers/staging/iio/cdc/Kconfig" >>>> source "drivers/staging/iio/dac/Kconfig" >>>> source "drivers/staging/iio/frequency/Kconfig" >>>> diff --git a/drivers/staging/iio/Makefile >>>> b/drivers/staging/iio/Makefile >>>> index 0161095..c52cde3 100644 >>>> --- a/drivers/staging/iio/Makefile >>>> +++ b/drivers/staging/iio/Makefile >>>> @@ -20,6 +20,7 @@ obj-$(CONFIG_IIO_DUMMY_EVGEN) += iio_dummy_evgen.o >>>> obj-y += accel/ >>>> obj-y += adc/ >>>> obj-y += addac/ >>>> +obj-y += amplifiers/ >>>> obj-y += cdc/ >>>> obj-y += dac/ >>>> obj-y += frequency/ >>>> diff --git a/drivers/staging/iio/amplifiers/Kconfig >>>> b/drivers/staging/iio/amplifiers/Kconfig >>>> new file mode 100644 >>>> index 0000000..05d707e >>>> --- /dev/null >>>> +++ b/drivers/staging/iio/amplifiers/Kconfig >>>> @@ -0,0 +1,17 @@ >>>> +# >>>> +# Gain Amplifiers, etc. >>>> +# >>>> +menu "Amplifiers" >>>> + >>>> +config AD8366 >>>> + tristate "Analog Devices AD8366 VGA" >>>> + depends on SPI >>>> + select BITREVERSE >>>> + help >>>> + Say yes here to build support for Analog Devices AD8366 >>>> + SPI Dual-Digital Variable Gain Amplifier (VGA). >>> Hmm.. interesting choice of acronym! Just for the unknowledgable >>> amongst us, what's the difference between a vga and a pga? >> PGA = Programmable Gain Amplifier >> VGA = Variable Gain Amplifier. >> That's a good question.IMHO VGA is the more widely >> use acronym. But I guess it depends who ever decided the title >> of the datasheet. > Fair enough. I've curiously hit a lot of PGAs but VGA was a new one > to me ;) >>>> + >>>> + To compile this driver as a module, choose M here: the >>>> + module will be called ad8366. >>>> + >>>> +endmenu >>>> diff --git a/drivers/staging/iio/amplifiers/Makefile >>>> b/drivers/staging/iio/amplifiers/Makefile >>>> new file mode 100644 >>>> index 0000000..a6ca366 >>>> --- /dev/null >>>> +++ b/drivers/staging/iio/amplifiers/Makefile >>>> @@ -0,0 +1,5 @@ >>>> +# >>>> +# Makefile iio/amplifiers >>>> +# >>>> + >>>> +obj-$(CONFIG_AD8366) += ad8366.o >>>> diff --git a/drivers/staging/iio/amplifiers/ad8366.c >>>> b/drivers/staging/iio/amplifiers/ad8366.c >>>> new file mode 100644 >>>> index 0000000..187606f >>>> --- /dev/null >>>> +++ b/drivers/staging/iio/amplifiers/ad8366.c >>>> @@ -0,0 +1,228 @@ >>>> +/* >>>> + * AD8366 SPI Dual-Digital Variable Gain Amplifier (VGA) >>>> + * >>>> + * Copyright 2012 Analog Devices Inc. >>>> + * >>>> + * Licensed under the GPL-2. >>>> + */ >>>> + >>>> +#include<linux/device.h> >>>> +#include<linux/kernel.h> >>>> +#include<linux/slab.h> >>>> +#include<linux/sysfs.h> >>>> +#include<linux/spi/spi.h> >>>> +#include<linux/regulator/consumer.h> >>>> +#include<linux/err.h> >>>> +#include<linux/module.h> >>>> +#include<linux/bitrev.h> >>>> + >>>> +#include "../iio.h" >>>> +#include "../sysfs.h" >>>> + >>>> +struct ad8366_state { >>>> + struct spi_device *spi; >>>> + struct regulator *reg; >>>> + unsigned char ch_a; >>>> + unsigned char ch_b; >>>> + /* >>>> + * DMA (thus cache coherency maintenance) requires the >>>> + * transfer buffers to live in their own cache lines. >>>> + */ >>>> + unsigned char data[2] ____cacheline_aligned; >>>> +}; >>>> + >>>> +static int ad8366_write(struct iio_dev *indio_dev, >>>> + unsigned char ch_a, char unsigned ch_b) >>>> +{ >>>> + struct ad8366_state *st = iio_priv(indio_dev); >>>> + int ret; >>>> + >>>> + ch_a = bitrev8(ch_a& 0x3F); >>>> + ch_b = bitrev8(ch_b& 0x3F); >>>> + >>>> + st->data[0] = ch_b>> 4; >>>> + st->data[1] = (ch_b<< 4) | (ch_a>> 2); >>>> + >>>> + ret = spi_write(st->spi, st->data, ARRAY_SIZE(st->data)); >>>> + if (ret< 0) >>>> + dev_err(&indio_dev->dev, "write failed (%d)", ret); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int ad8366_read_raw(struct iio_dev *indio_dev, >>>> + struct iio_chan_spec const *chan, >>>> + int *val, >>>> + int *val2, >>>> + long m) >>>> +{ >>>> + struct ad8366_state *st = iio_priv(indio_dev); >>>> + int ret; >>>> + unsigned code; >>>> + >>>> + mutex_lock(&indio_dev->mlock); >>>> + switch (m) { >>>> + case 0: >>>> + if (chan->channel == 0) >>>> + code = st->ch_a; >>>> + else >>>> + code = st->ch_b; >>>> + >>>> + /* Values in dB */ >>>> + code = code * 253 + 4500; >>>> + *val = code / 1000; >>>> + *val2 = (code % 1000) * 1000; >>>> + >>>> + ret = IIO_VAL_INT_PLUS_MICRO; >>>> + break; >>>> + default: >>>> + ret = -EINVAL; >>>> + } >>>> + mutex_unlock(&indio_dev->mlock); >>>> + >>>> + return ret; >>>> +}; >>>> + >>>> +static int ad8366_write_raw(struct iio_dev *indio_dev, >>>> + struct iio_chan_spec const *chan, >>>> + int val, >>>> + int val2, >>>> + long mask) >>>> +{ >>>> + struct ad8366_state *st = iio_priv(indio_dev); >>>> + unsigned code; >>>> + int ret; >>>> + >>>> + /* Values in dB */ >>>> + code = (((u8)val * 1000) + ((u32)val2 / 1000)); >>>> + >>>> + if (code> 20500 || code< 4500) >>>> + return -EINVAL; >>>> + >>>> + code = (code - 4500) / 253; >>>> + >>>> + mutex_lock(&indio_dev->mlock); >>>> + switch (mask) { >>>> + case 0: >>>> + if (chan->channel == 0) >>>> + st->ch_a = code; >>>> + else >>>> + st->ch_b = code; >>>> + >>>> + ret = ad8366_write(indio_dev, st->ch_a, st->ch_b); >>>> + break; >>>> + default: >>>> + ret = -EINVAL; >>>> + } >>>> + mutex_unlock(&indio_dev->mlock); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static const struct iio_info ad8366_info = { >>>> + .read_raw =&ad8366_read_raw, >>>> + .write_raw =&ad8366_write_raw, >>>> + .driver_module = THIS_MODULE, >>>> +}; >>>> + >>>> +#define AD8366_CHAN(_channel, _name) { \ >>>> + .type = IIO_ALTVOLTAGE, \ >>>> + .output = 1, \ >>>> + .indexed = 1, \ >>>> + .processed_val = 1, \ >>>> + .channel = _channel, \ >>>> + .extend_name = _name, \ >>>> +} >>>> + >>>> +static const struct iio_chan_spec ad8366_channels[] = { >>>> + AD8366_CHAN(0, "gain"), >>>> + AD8366_CHAN(1, "gain"), >>>> +}; >>>> + >>>> +static int __devinit ad8366_probe(struct spi_device *spi) >>>> +{ >>>> + struct iio_dev *indio_dev; >>>> + struct ad8366_state *st; >>>> + int ret; >>>> + >>>> + indio_dev = iio_allocate_device(sizeof(*st)); >>>> + if (indio_dev == NULL) >>>> + return -ENOMEM; >>>> + >>>> + st = iio_priv(indio_dev); >>>> + >>>> + st->reg = regulator_get(&spi->dev, "vcc"); >>>> + if (!IS_ERR(st->reg)) { >>>> + ret = regulator_enable(st->reg); >>>> + if (ret) >>>> + goto error_put_reg; >>>> + } >>>> + >>>> + spi_set_drvdata(spi, indio_dev); >>>> + st->spi = spi; >>>> + >>>> + indio_dev->dev.parent =&spi->dev; >>>> + indio_dev->name = spi_get_device_id(spi)->name; >>>> + indio_dev->info =&ad8366_info; >>>> + indio_dev->modes = INDIO_DIRECT_MODE; >>>> + indio_dev->channels = ad8366_channels; >>>> + indio_dev->num_channels = ARRAY_SIZE(ad8366_channels); >>>> + >>>> + ret = iio_device_register(indio_dev); >>>> + if (ret) >>>> + goto error_disable_reg; >>>> + >>>> + ad8366_write(indio_dev, 0 , 0); >>>> + >>>> + return 0; >>>> + >>>> +error_disable_reg: >>>> + if (!IS_ERR(st->reg)) >>>> + regulator_disable(st->reg); >>>> +error_put_reg: >>>> + if (!IS_ERR(st->reg)) >>>> + regulator_put(st->reg); >>>> + >>>> + iio_free_device(indio_dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int __devexit ad8366_remove(struct spi_device *spi) >>>> +{ >>>> + struct iio_dev *indio_dev = spi_get_drvdata(spi); >>>> + struct ad8366_state *st = iio_priv(indio_dev); >>>> + struct regulator *reg = st->reg; >>>> + >>>> + iio_device_unregister(indio_dev); >>>> + >>>> + if (!IS_ERR(reg)) { >>>> + regulator_disable(reg); >>>> + regulator_put(reg); >>>> + } >>>> + >>>> + iio_free_device(indio_dev); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct spi_device_id ad8366_id[] = { >>>> + {"ad8366", 0}, >>>> + {} >>>> +}; >>>> + >>>> +static struct spi_driver ad8366_driver = { >>>> + .driver = { >>>> + .name = KBUILD_MODNAME, >>>> + .owner = THIS_MODULE, >>>> + }, >>>> + .probe = ad8366_probe, >>>> + .remove = __devexit_p(ad8366_remove), >>>> + .id_table = ad8366_id, >>>> +}; >>>> + >>>> +module_spi_driver(ad8366_driver); >>>> + >>>> +MODULE_AUTHOR("Michael Hennerich<hennerich@blackfin.uclinux.org>"); >>>> +MODULE_DESCRIPTION("Analog Devices AD8366 VGA"); >>>> +MODULE_LICENSE("GPL v2"); >>>> -- >>>> 1.7.0.4 >>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Greetings, Michael -- Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-05-07 15:00 ` Michael Hennerich @ 2012-05-07 15:17 ` Michael Hennerich 2012-05-08 12:53 ` Jonathan Cameron 0 siblings, 1 reply; 16+ messages in thread From: Michael Hennerich @ 2012-05-07 15:17 UTC (permalink / raw) To: michael.hennerich Cc: Jonathan Cameron, Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 05/07/2012 05:00 PM, Michael Hennerich wrote: > On 03/22/2012 10:10 AM, Jonathan Cameron wrote: >> On 3/22/2012 8:52 AM, Michael Hennerich wrote: >>> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>>> From: Michael Hennerich<michael.hennerich@analog.com> >>>> Sorry for the slow response on this one. Been off sick... >>>> >>>> Anyhow, I'm still not sure what the right interface for this type >>>> of device is. >>>> >>>> The obvious options are: >>>> >>>> 1) Make gain an IIO type (doesn't make much sense as gain is only >>>> going >>>> to be of one particular existing type). >>>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use >>>> extend >>>> name. Any real reason for picking altvoltage rather than voltage? >>> I'm open for advice. Since I made the amplifier being an OUT type >>> device >>> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >>> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. >> Hmm.. Don't suppose it really matters but we ought to aim for >> consistency >> (by review) at least. This particular part is DC through to 600MHz. >>>> Clearly gain has the same meaning in either case (assuming it's >>>> linear). >>>> 3) Make a change to core to allow a channel to have elements in >>>> info_mask but not actually to have a raw access. Not entirely sure >>>> how we will do that cleanly. Also it's not clear whether the gain >>>> would be an IN or an OUT channel type! >>> Well - having the ability for channels without raw access element >>> would be of interest. >> True enough. Cleanest way to do this that I can think of is to make >> a tree >> wide change to add the raw element to the info_mask. We could allow for >> a zero info_mask value actually being the equivalent of having only a >> raw >> channel. It's invasive but if we agreee it should be done now is >> probably the >> best time to do it (just post merge window etc). > Hi Jonathan, > > Thanks for getting this in place. > >> >> Whilst here, we clearly need way of destinguishing values in DB from >> linear >> gains. Could add a new return type for read_raw callbacks? > > Does something like this work for you? > Also wondering if we should introduce IIO_CHAN_INFO_GAIN > for amplifier type devices? Thinking about it a bit more - why not have iio_chan_type:IIO_GAIN? > > From e62f3a3de86f2edde2137237531732cdf5fc2ed8 Mon Sep 17 00:00:00 2001 > From: Michael Hennerich <michael.hennerich@analog.com> > Date: Mon, 7 May 2012 16:53:45 +0200 > Subject: [PATCH] iio: core: introduce dB scle: IIO_VAL_INT_PLUS_MICRO_DB > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com> > --- > drivers/iio/industrialio-core.c | 19 +++++++++++++------ > include/linux/iio/types.h | 1 + > 2 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c > b/drivers/iio/industrialio-core.c > index 72e33b8..e436f6f 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -306,26 +306,33 @@ static ssize_t iio_read_channel_info(struct > device *dev, > struct iio_dev *indio_dev = dev_get_drvdata(dev); > struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); > int val, val2; > + bool scale_db = false; > int ret = indio_dev->info->read_raw(indio_dev, this_attr->c, > &val, &val2, this_attr->address); > > if (ret < 0) > return ret; > > - if (ret == IIO_VAL_INT) > + switch (ret) { > + case IIO_VAL_INT: > return sprintf(buf, "%d\n", val); > - else if (ret == IIO_VAL_INT_PLUS_MICRO) { > + case IIO_VAL_INT_PLUS_MICRO_DB: > + scale_db = true; > + case IIO_VAL_INT_PLUS_MICRO: > if (val2 < 0) > - return sprintf(buf, "-%d.%06u\n", val, -val2); > + return sprintf(buf, "-%d.%06u%s\n", val, -val2, > + scale_db ? " db" : ""); > else > - return sprintf(buf, "%d.%06u\n", val, val2); > - } else if (ret == IIO_VAL_INT_PLUS_NANO) { > + return sprintf(buf, "%d.%06u%s\n", val, val2, > + scale_db ? " db" : ""); > + case IIO_VAL_INT_PLUS_NANO: > if (val2 < 0) > return sprintf(buf, "-%d.%09u\n", val, -val2); > else > return sprintf(buf, "%d.%09u\n", val, val2); > - } else > + default: > return 0; > + } > } > > static ssize_t iio_write_channel_info(struct device *dev, > diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h > index a471fd5..1b073b1 100644 > --- a/include/linux/iio/types.h > +++ b/include/linux/iio/types.h > @@ -50,5 +50,6 @@ enum iio_modifier { > #define IIO_VAL_INT 1 > #define IIO_VAL_INT_PLUS_MICRO 2 > #define IIO_VAL_INT_PLUS_NANO 3 > +#define IIO_VAL_INT_PLUS_MICRO_DB 4 > > #endif /* _IIO_TYPES_H_ */ -- Greetings, Michael -- Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-05-07 15:17 ` Michael Hennerich @ 2012-05-08 12:53 ` Jonathan Cameron 2012-05-08 13:26 ` Hennerich, Michael 0 siblings, 1 reply; 16+ messages in thread From: Jonathan Cameron @ 2012-05-08 12:53 UTC (permalink / raw) To: michael.hennerich Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 5/7/2012 4:17 PM, Michael Hennerich wrote: > On 05/07/2012 05:00 PM, Michael Hennerich wrote: >> On 03/22/2012 10:10 AM, Jonathan Cameron wrote: >>> On 3/22/2012 8:52 AM, Michael Hennerich wrote: >>>> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>>>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>>>> From: Michael Hennerich<michael.hennerich@analog.com> >>>>> Sorry for the slow response on this one. Been off sick... >>>>> >>>>> Anyhow, I'm still not sure what the right interface for this type >>>>> of device is. >>>>> >>>>> The obvious options are: >>>>> >>>>> 1) Make gain an IIO type (doesn't make much sense as gain is only >>>>> going >>>>> to be of one particular existing type). >>>>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use >>>>> extend >>>>> name. Any real reason for picking altvoltage rather than voltage? >>>> I'm open for advice. Since I made the amplifier being an OUT type >>>> device >>>> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >>>> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. >>> Hmm.. Don't suppose it really matters but we ought to aim for >>> consistency >>> (by review) at least. This particular part is DC through to 600MHz. >>>>> Clearly gain has the same meaning in either case (assuming it's >>>>> linear). >>>>> 3) Make a change to core to allow a channel to have elements in >>>>> info_mask but not actually to have a raw access. Not entirely sure >>>>> how we will do that cleanly. Also it's not clear whether the gain >>>>> would be an IN or an OUT channel type! >>>> Well - having the ability for channels without raw access element >>>> would be of interest. >>> True enough. Cleanest way to do this that I can think of is to make >>> a tree >>> wide change to add the raw element to the info_mask. We could allow >>> for >>> a zero info_mask value actually being the equivalent of having only >>> a raw >>> channel. It's invasive but if we agreee it should be done now is >>> probably the >>> best time to do it (just post merge window etc). >> Hi Jonathan, >> >> Thanks for getting this in place. >> >>> >>> Whilst here, we clearly need way of destinguishing values in DB from >>> linear >>> gains. Could add a new return type for read_raw callbacks? >> >> Does something like this work for you? >> Also wondering if we should introduce IIO_CHAN_INFO_GAIN >> for amplifier type devices? > > Thinking about it a bit more - why not have iio_chan_type:IIO_GAIN? Lack of consistency with other devices. If we have a pga on the front of an adc then the type is voltage and control is done via relevant info element. How is this any different? ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-05-08 12:53 ` Jonathan Cameron @ 2012-05-08 13:26 ` Hennerich, Michael 2012-05-08 13:32 ` Jonathan Cameron 0 siblings, 1 reply; 16+ messages in thread From: Hennerich, Michael @ 2012-05-08 13:26 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown Jonathan Cameron wrote on 2012-05-08: > On 5/7/2012 4:17 PM, Michael Hennerich wrote: >> On 05/07/2012 05:00 PM, Michael Hennerich wrote: >>> On 03/22/2012 10:10 AM, Jonathan Cameron wrote: >>>> On 3/22/2012 8:52 AM, Michael Hennerich wrote: >>>>> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>>>>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>>>>> From: Michael Hennerich<michael.hennerich@analog.com> >>>>>> Sorry for the slow response on this one. Been off sick... >>>>>> >>>>>> Anyhow, I'm still not sure what the right interface for this type >>>>>> of device is. >>>>>> >>>>>> The obvious options are: >>>>>> >>>>>> 1) Make gain an IIO type (doesn't make much sense as gain is only >>>>>> going >>>>>> to be of one particular existing type). >>>>>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use >>>>>> extend >>>>>> name. Any real reason for picking altvoltage rather than > voltage? >>>>> I'm open for advice. Since I made the amplifier being an OUT type >>>>> device >>>>> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >>>>> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. >>>> Hmm.. Don't suppose it really matters but we ought to aim for >>>> consistency >>>> (by review) at least. This particular part is DC through to > 600MHz. >>>>>> Clearly gain has the same meaning in either case (assuming it's >>>>>> linear). 3) Make a change to core to allow a channel to have >>>>>> elements in info_mask but not actually to have a raw access. Not >>>>>> entirely sure how we will do that cleanly. Also it's not clear >>>>>> whether the gain would be an IN or an OUT channel type! >>>>> Well - having the ability for channels without raw access element >>>>> would be of interest. >>>> True enough. Cleanest way to do this that I can think of is to make >>>> a tree wide change to add the raw element to the info_mask. We could >>>> allow for a zero info_mask value actually being the equivalent of >>>> having only a raw channel. It's invasive but if we agreee it should >>>> be done now is probably the best time to do it (just post merge >>>> window etc). >>> Hi Jonathan, >>> >>> Thanks for getting this in place. >>> >>>> >>>> Whilst here, we clearly need way of destinguishing values in DB from >>>> linear gains. Could add a new return type for read_raw callbacks? >>> >>> Does something like this work for you? >>> Also wondering if we should introduce IIO_CHAN_INFO_GAIN >>> for amplifier type devices? >> >> Thinking about it a bit more - why not have iio_chan_type:IIO_GAIN? > Lack of consistency with other devices. If we have a pga on the front > of an adc then the type is voltage and control is done via relevant info > element. How is this any different? Well there can be several types of amplifiers voltage, current or none-elec= tronic such as optical amplifiers. The key element GAIN of an amplifier is only the ratio between input and ou= tput. The unit cancels out, so why bother with voltage or current? That's why I proposed IIO_GAIN. Anyways I'm fine with IIO_VOLTAGE for now, but I would like to use IIO_CHAN= _INFO_GAIN and not IIO_CHAN_INFO_SCALE? Greetings, Michael -- Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Mar= garet Seif ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-05-08 13:26 ` Hennerich, Michael @ 2012-05-08 13:32 ` Jonathan Cameron 2012-05-08 14:48 ` Michael Hennerich 0 siblings, 1 reply; 16+ messages in thread From: Jonathan Cameron @ 2012-05-08 13:32 UTC (permalink / raw) To: Hennerich, Michael Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 5/8/2012 2:26 PM, Hennerich, Michael wrote: > Jonathan Cameron wrote on 2012-05-08: >> On 5/7/2012 4:17 PM, Michael Hennerich wrote: >>> On 05/07/2012 05:00 PM, Michael Hennerich wrote: >>>> On 03/22/2012 10:10 AM, Jonathan Cameron wrote: >>>>> On 3/22/2012 8:52 AM, Michael Hennerich wrote: >>>>>> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>>>>>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>>>>>> From: Michael Hennerich<michael.hennerich@analog.com> >>>>>>> Sorry for the slow response on this one. Been off sick... >>>>>>> >>>>>>> Anyhow, I'm still not sure what the right interface for this type >>>>>>> of device is. >>>>>>> >>>>>>> The obvious options are: >>>>>>> >>>>>>> 1) Make gain an IIO type (doesn't make much sense as gain is only >>>>>>> going >>>>>>> to be of one particular existing type). >>>>>>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use >>>>>>> extend >>>>>>> name. Any real reason for picking altvoltage rather than >> voltage? >>>>>> I'm open for advice. Since I made the amplifier being an OUT type >>>>>> device >>>>>> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >>>>>> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. >>>>> Hmm.. Don't suppose it really matters but we ought to aim for >>>>> consistency >>>>> (by review) at least. This particular part is DC through to >> 600MHz. >>>>>>> Clearly gain has the same meaning in either case (assuming it's >>>>>>> linear). 3) Make a change to core to allow a channel to have >>>>>>> elements in info_mask but not actually to have a raw access. Not >>>>>>> entirely sure how we will do that cleanly. Also it's not clear >>>>>>> whether the gain would be an IN or an OUT channel type! >>>>>> Well - having the ability for channels without raw access element >>>>>> would be of interest. >>>>> True enough. Cleanest way to do this that I can think of is to make >>>>> a tree wide change to add the raw element to the info_mask. We could >>>>> allow for a zero info_mask value actually being the equivalent of >>>>> having only a raw channel. It's invasive but if we agreee it should >>>>> be done now is probably the best time to do it (just post merge >>>>> window etc). >>>> Hi Jonathan, >>>> >>>> Thanks for getting this in place. >>>> >>>>> Whilst here, we clearly need way of destinguishing values in DB from >>>>> linear gains. Could add a new return type for read_raw callbacks? >>>> Does something like this work for you? >>>> Also wondering if we should introduce IIO_CHAN_INFO_GAIN >>>> for amplifier type devices? >>> Thinking about it a bit more - why not have iio_chan_type:IIO_GAIN? >> Lack of consistency with other devices. If we have a pga on the front >> of an adc then the type is voltage and control is done via relevant info >> element. How is this any different? > Well there can be several types of amplifiers voltage, current or none-electronic > such as optical amplifiers. Quite. But a given amplier only does one type. Hence arguement still stands... The only exception is an analog device such as an accelerometer wired up to a conventional adc, but then we don't handle that anyway... > > The key element GAIN of an amplifier is only the ratio between input and output. > The unit cancels out, so why bother with voltage or current? Far as I know you can't use a voltage amplifier for current amplificiation or the other way around, so still makes sence to keep them alongside the type. The voltage vs altvoltage is a bit of a pain but I guess no cunning plan is perfect.. > That's why I proposed IIO_GAIN. > Anyways I'm fine with IIO_VOLTAGE for now, but I would like to use IIO_CHAN_INFO_GAIN > and not IIO_CHAN_INFO_SCALE? Its internally applied so actually IIO_CHAN_INFO_CALIBSCALE is the relevant one. I'm really keen to stick to the existing ones purely because this does look exactly like a pga front end as found on numerous adcs. Why treat it differently just because it is in a different package? We may need to be clever about routing between parts sometime in the future but would be best to keep it simple for now? > > Greetings, > Michael > > -- > Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen > Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; > Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-05-08 13:32 ` Jonathan Cameron @ 2012-05-08 14:48 ` Michael Hennerich 2012-05-08 14:53 ` Jonathan Cameron 0 siblings, 1 reply; 16+ messages in thread From: Michael Hennerich @ 2012-05-08 14:48 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 05/08/2012 03:32 PM, Jonathan Cameron wrote: > On 5/8/2012 2:26 PM, Hennerich, Michael wrote: >> Jonathan Cameron wrote on 2012-05-08: >>> On 5/7/2012 4:17 PM, Michael Hennerich wrote: >>>> On 05/07/2012 05:00 PM, Michael Hennerich wrote: >>>>> On 03/22/2012 10:10 AM, Jonathan Cameron wrote: >>>>>> On 3/22/2012 8:52 AM, Michael Hennerich wrote: >>>>>>> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>>>>>>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>>>>>>> From: Michael Hennerich<michael.hennerich@analog.com> >>>>>>>> Sorry for the slow response on this one. Been off sick... >>>>>>>> >>>>>>>> Anyhow, I'm still not sure what the right interface for this type >>>>>>>> of device is. >>>>>>>> >>>>>>>> The obvious options are: >>>>>>>> >>>>>>>> 1) Make gain an IIO type (doesn't make much sense as gain is only >>>>>>>> going >>>>>>>> to be of one particular existing type). >>>>>>>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use >>>>>>>> extend >>>>>>>> name. Any real reason for picking altvoltage rather than >>> voltage? >>>>>>> I'm open for advice. Since I made the amplifier being an OUT type >>>>>>> device >>>>>>> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >>>>>>> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. >>>>>> Hmm.. Don't suppose it really matters but we ought to aim for >>>>>> consistency >>>>>> (by review) at least. This particular part is DC through to >>> 600MHz. >>>>>>>> Clearly gain has the same meaning in either case (assuming it's >>>>>>>> linear). 3) Make a change to core to allow a channel to have >>>>>>>> elements in info_mask but not actually to have a raw access. Not >>>>>>>> entirely sure how we will do that cleanly. Also it's not clear >>>>>>>> whether the gain would be an IN or an OUT channel type! >>>>>>> Well - having the ability for channels without raw access element >>>>>>> would be of interest. >>>>>> True enough. Cleanest way to do this that I can think of is to make >>>>>> a tree wide change to add the raw element to the info_mask. We could >>>>>> allow for a zero info_mask value actually being the equivalent of >>>>>> having only a raw channel. It's invasive but if we agreee it should >>>>>> be done now is probably the best time to do it (just post merge >>>>>> window etc). >>>>> Hi Jonathan, >>>>> >>>>> Thanks for getting this in place. >>>>> >>>>>> Whilst here, we clearly need way of destinguishing values in DB from >>>>>> linear gains. Could add a new return type for read_raw callbacks? >>>>> Does something like this work for you? >>>>> Also wondering if we should introduce IIO_CHAN_INFO_GAIN >>>>> for amplifier type devices? >>>> Thinking about it a bit more - why not have iio_chan_type:IIO_GAIN? >>> Lack of consistency with other devices. If we have a pga on the front >>> of an adc then the type is voltage and control is done via relevant info >>> element. How is this any different? >> Well there can be several types of amplifiers voltage, current or none-electronic >> such as optical amplifiers. > Quite. But a given amplier only does one type. Hence arguement still > stands... The only > exception is an analog device such as an accelerometer wired up to a > conventional adc, > but then we don't handle that anyway... >> The key element GAIN of an amplifier is only the ratio between input and output. >> The unit cancels out, so why bother with voltage or current? > Far as I know you can't use a voltage amplifier for current > amplificiation or the other way around, > so still makes sence to keep them alongside the type. The voltage vs > altvoltage is a bit of > a pain but I guess no cunning plan is perfect.. It really depends on the external circuit. But I agree programmable current amplifiers are rare. More overlap is between IIO_VOLTAGE and IIO_POWER. >> That's why I proposed IIO_GAIN. >> Anyways I'm fine with IIO_VOLTAGE for now, but I would like to use IIO_CHAN_INFO_GAIN >> and not IIO_CHAN_INFO_SCALE? > Its internally applied so actually IIO_CHAN_INFO_CALIBSCALE is the > relevant one. > I'm really keen to stick to the existing ones purely because this does > look exactly like a > pga front end as found on numerous adcs. Why treat it differently just > because it is > in a different package? We may need to be clever about routing between > parts sometime > in the future but would be best to keep it simple for now? I know it's important to stay consistent. But when talking about characteristics of an amplifier - CALIBSCALE doesn't really sound well and SCALE is for sure not applicable. What /sys/bus/iio/devices/iio:deviceX/in_voltageY_calibscale Description: Hardware applied calibration scale factor. (assumed to fix production inaccuracies). If shared across all channels, <type>_calibscale is used. Apart from 'calibration' and 'production inaccuracies' - Technically seen it does the right thing. But you need some imagination to use CALIBSCALE for gain. -- Greetings, Michael -- Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-05-08 14:48 ` Michael Hennerich @ 2012-05-08 14:53 ` Jonathan Cameron 2012-05-08 15:57 ` Michael Hennerich 0 siblings, 1 reply; 16+ messages in thread From: Jonathan Cameron @ 2012-05-08 14:53 UTC (permalink / raw) To: michael.hennerich Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 5/8/2012 3:48 PM, Michael Hennerich wrote: > On 05/08/2012 03:32 PM, Jonathan Cameron wrote: >> On 5/8/2012 2:26 PM, Hennerich, Michael wrote: >>> Jonathan Cameron wrote on 2012-05-08: >>>> On 5/7/2012 4:17 PM, Michael Hennerich wrote: >>>>> On 05/07/2012 05:00 PM, Michael Hennerich wrote: >>>>>> On 03/22/2012 10:10 AM, Jonathan Cameron wrote: >>>>>>> On 3/22/2012 8:52 AM, Michael Hennerich wrote: >>>>>>>> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>>>>>>>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>>>>>>>> From: Michael Hennerich<michael.hennerich@analog.com> >>>>>>>>> Sorry for the slow response on this one. Been off sick... >>>>>>>>> >>>>>>>>> Anyhow, I'm still not sure what the right interface for this type >>>>>>>>> of device is. >>>>>>>>> >>>>>>>>> The obvious options are: >>>>>>>>> >>>>>>>>> 1) Make gain an IIO type (doesn't make much sense as gain is only >>>>>>>>> going >>>>>>>>> to be of one particular existing type). >>>>>>>>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use >>>>>>>>> extend >>>>>>>>> name. Any real reason for picking altvoltage rather than >>>> voltage? >>>>>>>> I'm open for advice. Since I made the amplifier being an OUT type >>>>>>>> device >>>>>>>> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >>>>>>>> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. >>>>>>> Hmm.. Don't suppose it really matters but we ought to aim for >>>>>>> consistency >>>>>>> (by review) at least. This particular part is DC through to >>>> 600MHz. >>>>>>>>> Clearly gain has the same meaning in either case (assuming it's >>>>>>>>> linear). 3) Make a change to core to allow a channel to have >>>>>>>>> elements in info_mask but not actually to have a raw access. Not >>>>>>>>> entirely sure how we will do that cleanly. Also it's not clear >>>>>>>>> whether the gain would be an IN or an OUT channel type! >>>>>>>> Well - having the ability for channels without raw access element >>>>>>>> would be of interest. >>>>>>> True enough. Cleanest way to do this that I can think of is to >>>>>>> make >>>>>>> a tree wide change to add the raw element to the info_mask. We >>>>>>> could >>>>>>> allow for a zero info_mask value actually being the equivalent of >>>>>>> having only a raw channel. It's invasive but if we agreee it >>>>>>> should >>>>>>> be done now is probably the best time to do it (just post merge >>>>>>> window etc). >>>>>> Hi Jonathan, >>>>>> >>>>>> Thanks for getting this in place. >>>>>> >>>>>>> Whilst here, we clearly need way of destinguishing values in DB >>>>>>> from >>>>>>> linear gains. Could add a new return type for read_raw callbacks? >>>>>> Does something like this work for you? >>>>>> Also wondering if we should introduce IIO_CHAN_INFO_GAIN >>>>>> for amplifier type devices? >>>>> Thinking about it a bit more - why not have iio_chan_type:IIO_GAIN? >>>> Lack of consistency with other devices. If we have a pga on the front >>>> of an adc then the type is voltage and control is done via relevant >>>> info >>>> element. How is this any different? >>> Well there can be several types of amplifiers voltage, current or >>> none-electronic >>> such as optical amplifiers. >> Quite. But a given amplier only does one type. Hence arguement still >> stands... The only >> exception is an analog device such as an accelerometer wired up to a >> conventional adc, >> but then we don't handle that anyway... >>> The key element GAIN of an amplifier is only the ratio between input >>> and output. >>> The unit cancels out, so why bother with voltage or current? >> Far as I know you can't use a voltage amplifier for current >> amplificiation or the other way around, >> so still makes sence to keep them alongside the type. The voltage vs >> altvoltage is a bit of >> a pain but I guess no cunning plan is perfect.. > It really depends on the external circuit. > But I agree programmable current amplifiers are rare. > More overlap is between IIO_VOLTAGE and IIO_POWER. Fair point... Whilst there is an issue here, i'm still unconvinced that an IIO_GAIN type makes sense.. > >>> That's why I proposed IIO_GAIN. >>> Anyways I'm fine with IIO_VOLTAGE for now, but I would like to use >>> IIO_CHAN_INFO_GAIN >>> and not IIO_CHAN_INFO_SCALE? >> Its internally applied so actually IIO_CHAN_INFO_CALIBSCALE is the >> relevant one. >> I'm really keen to stick to the existing ones purely because this does >> look exactly like a >> pga front end as found on numerous adcs. Why treat it differently just >> because it is >> in a different package? We may need to be clever about routing between >> parts sometime >> in the future but would be best to keep it simple for now? > > I know it's important to stay consistent. > But when talking about characteristics of an amplifier - > CALIBSCALE doesn't really sound well and SCALE is for sure not > applicable. > > > What /sys/bus/iio/devices/iio:deviceX/in_voltageY_calibscale > Description: > Hardware applied calibration scale factor. (assumed to fix > production inaccuracies). If shared across all channels, > <type>_calibscale is used. > > Apart from 'calibration' and 'production inaccuracies' - > Technically seen it does the right thing. > But you need some imagination to use CALIBSCALE for gain. Perhaps we can rename this? Maybe hardwaregain or something like that? Can always allow both for a bit and do a smooth transition over to the new choice? Jonathan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier 2012-05-08 14:53 ` Jonathan Cameron @ 2012-05-08 15:57 ` Michael Hennerich 0 siblings, 0 replies; 16+ messages in thread From: Michael Hennerich @ 2012-05-08 15:57 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, linux-iio@vger.kernel.org, device-drivers-devel@blackfin.uclinux.org, Drivers, Mark Brown On 05/08/2012 04:53 PM, Jonathan Cameron wrote: > On 5/8/2012 3:48 PM, Michael Hennerich wrote: >> On 05/08/2012 03:32 PM, Jonathan Cameron wrote: >>> On 5/8/2012 2:26 PM, Hennerich, Michael wrote: >>>> Jonathan Cameron wrote on 2012-05-08: >>>>> On 5/7/2012 4:17 PM, Michael Hennerich wrote: >>>>>> On 05/07/2012 05:00 PM, Michael Hennerich wrote: >>>>>>> On 03/22/2012 10:10 AM, Jonathan Cameron wrote: >>>>>>>> On 3/22/2012 8:52 AM, Michael Hennerich wrote: >>>>>>>>> On 03/21/2012 08:34 PM, Jonathan Cameron wrote: >>>>>>>>>> On 02/22/2012 12:36 PM, michael.hennerich@analog.com wrote: >>>>>>>>>>> From: Michael Hennerich<michael.hennerich@analog.com> >>>>>>>>>> Sorry for the slow response on this one. Been off sick... >>>>>>>>>> >>>>>>>>>> Anyhow, I'm still not sure what the right interface for this type >>>>>>>>>> of device is. >>>>>>>>>> >>>>>>>>>> The obvious options are: >>>>>>>>>> >>>>>>>>>> 1) Make gain an IIO type (doesn't make much sense as gain is only >>>>>>>>>> going >>>>>>>>>> to be of one particular existing type). >>>>>>>>>> 2) Have it as an IIO_ALTVOLTAGE channel as you have here and use >>>>>>>>>> extend >>>>>>>>>> name. Any real reason for picking altvoltage rather than >>>>> voltage? >>>>>>>>> I'm open for advice. Since I made the amplifier being an OUT type >>>>>>>>> device >>>>>>>>> I chose IIO_ALTVOLTAGE analogous to our DDS/PLL drivers. >>>>>>>>> Some VGAs/PGAs work from DC, but typically VGAs are HF devices. >>>>>>>> Hmm.. Don't suppose it really matters but we ought to aim for >>>>>>>> consistency >>>>>>>> (by review) at least. This particular part is DC through to >>>>> 600MHz. >>>>>>>>>> Clearly gain has the same meaning in either case (assuming it's >>>>>>>>>> linear). 3) Make a change to core to allow a channel to have >>>>>>>>>> elements in info_mask but not actually to have a raw access. Not >>>>>>>>>> entirely sure how we will do that cleanly. Also it's not clear >>>>>>>>>> whether the gain would be an IN or an OUT channel type! >>>>>>>>> Well - having the ability for channels without raw access element >>>>>>>>> would be of interest. >>>>>>>> True enough. Cleanest way to do this that I can think of is to >>>>>>>> make >>>>>>>> a tree wide change to add the raw element to the info_mask. We >>>>>>>> could >>>>>>>> allow for a zero info_mask value actually being the equivalent of >>>>>>>> having only a raw channel. It's invasive but if we agreee it >>>>>>>> should >>>>>>>> be done now is probably the best time to do it (just post merge >>>>>>>> window etc). >>>>>>> Hi Jonathan, >>>>>>> >>>>>>> Thanks for getting this in place. >>>>>>> >>>>>>>> Whilst here, we clearly need way of destinguishing values in DB >>>>>>>> from >>>>>>>> linear gains. Could add a new return type for read_raw callbacks? >>>>>>> Does something like this work for you? >>>>>>> Also wondering if we should introduce IIO_CHAN_INFO_GAIN >>>>>>> for amplifier type devices? >>>>>> Thinking about it a bit more - why not have iio_chan_type:IIO_GAIN? >>>>> Lack of consistency with other devices. If we have a pga on the front >>>>> of an adc then the type is voltage and control is done via relevant >>>>> info >>>>> element. How is this any different? >>>> Well there can be several types of amplifiers voltage, current or >>>> none-electronic >>>> such as optical amplifiers. >>> Quite. But a given amplier only does one type. Hence arguement still >>> stands... The only >>> exception is an analog device such as an accelerometer wired up to a >>> conventional adc, >>> but then we don't handle that anyway... >>>> The key element GAIN of an amplifier is only the ratio between input >>>> and output. >>>> The unit cancels out, so why bother with voltage or current? >>> Far as I know you can't use a voltage amplifier for current >>> amplificiation or the other way around, >>> so still makes sence to keep them alongside the type. The voltage vs >>> altvoltage is a bit of >>> a pain but I guess no cunning plan is perfect.. >> It really depends on the external circuit. >> But I agree programmable current amplifiers are rare. >> More overlap is between IIO_VOLTAGE and IIO_POWER. > Fair point... Whilst there is an issue here, i'm still unconvinced that > an IIO_GAIN type makes sense.. >>>> That's why I proposed IIO_GAIN. >>>> Anyways I'm fine with IIO_VOLTAGE for now, but I would like to use >>>> IIO_CHAN_INFO_GAIN >>>> and not IIO_CHAN_INFO_SCALE? >>> Its internally applied so actually IIO_CHAN_INFO_CALIBSCALE is the >>> relevant one. >>> I'm really keen to stick to the existing ones purely because this does >>> look exactly like a >>> pga front end as found on numerous adcs. Why treat it differently just >>> because it is >>> in a different package? We may need to be clever about routing between >>> parts sometime >>> in the future but would be best to keep it simple for now? >> I know it's important to stay consistent. >> But when talking about characteristics of an amplifier - >> CALIBSCALE doesn't really sound well and SCALE is for sure not >> applicable. >> >> What /sys/bus/iio/devices/iio:deviceX/in_voltageY_calibscale >> Description: >> Hardware applied calibration scale factor. (assumed to fix >> production inaccuracies). If shared across all channels, >> <type>_calibscale is used. >> >> Apart from 'calibration' and 'production inaccuracies' - >> Technically seen it does the right thing. >> But you need some imagination to use CALIBSCALE for gain. > Perhaps we can rename this? Maybe hardwaregain or > something like that? Can always allow both for a bit and > do a smooth transition over to the new choice? Hardwargain sound good to me. I've no problem keeping CALIBSCALE where it's applicable and matches the purpose. -Michael > > Jonathan > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Greetings, Michael -- Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-05-08 15:57 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-22 12:36 [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier michael.hennerich 2012-03-21 19:34 ` Jonathan Cameron 2012-03-21 19:59 ` Mark Brown 2012-03-22 9:05 ` Hennerich, Michael 2012-03-22 8:52 ` Michael Hennerich 2012-03-22 9:10 ` Jonathan Cameron 2012-03-22 9:53 ` Michael Hennerich 2012-03-22 9:59 ` Jonathan Cameron 2012-05-07 15:00 ` Michael Hennerich 2012-05-07 15:17 ` Michael Hennerich 2012-05-08 12:53 ` Jonathan Cameron 2012-05-08 13:26 ` Hennerich, Michael 2012-05-08 13:32 ` Jonathan Cameron 2012-05-08 14:48 ` Michael Hennerich 2012-05-08 14:53 ` Jonathan Cameron 2012-05-08 15:57 ` Michael Hennerich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).