From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from am1ehsobe002.messaging.microsoft.com ([213.199.154.205]:33280 "EHLO am1outboundpool.messaging.microsoft.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756877Ab2EGPAJ (ORCPT ); Mon, 7 May 2012 11:00:09 -0400 Message-ID: <4FA7E373.2080704@analog.com> Date: Mon, 7 May 2012 17:00:03 +0200 From: Michael Hennerich Reply-To: MIME-Version: 1.0 To: Jonathan Cameron CC: Jonathan Cameron , "linux-iio@vger.kernel.org" , "device-drivers-devel@blackfin.uclinux.org" , Drivers , Mark Brown Subject: Re: [RFC] iio: amplifiers: New driver for AD8366 Dual-Digital Variable Gain Amplifier References: <1329914182-5428-1-git-send-email-michael.hennerich@analog.com> <4F6A2D2B.9050400@kernel.org> <4F6AE844.7020102@analog.com> <4F6AEC83.8010009@cam.ac.uk> In-Reply-To: <4F6AEC83.8010009@cam.ac.uk> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org 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 >>> 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 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 --- 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 >>>> --- >>>> 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 >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +#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"); >>>> +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