On 20/03/15 17:43, Peter Meerwald wrote:
> Hello Antoine,
>
>> This patch adds the support of the Berlin ADC, available on Berlin SoCs.
>> This ADC has 8 channels available, with one connected to a temperature
>> sensor.
>>
>> The particularity here, is that the temperature sensor connected to the
>> ADC has its own registers, and both the ADC and the temperature sensor
>> must be configured when using it.
>
> some quick comments inline below;
> sometimes this refers to berlin, sometimes to berlin2?
>
> probably these regmap_read() / _write() pairs could be MACRO()'d away
> somehow
>
> regards, p.
>
>> Signed-off-by: Antoine Tenart <antoine.tenart@xxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/iio/adc/Kconfig | 7 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/berlin-adc.c | 397 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 405 insertions(+)
>> create mode 100644 drivers/iio/adc/berlin-adc.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 202daf889be2..2f10a6d8d442 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -135,6 +135,13 @@ config AXP288_ADC
>> device. Depending on platform configuration, this general purpose ADC can
>> be used for sampling sensors such as thermal resistors.
>>
>> +config BERLIN_ADC
>> + tristate "Marvell Berlin ADC driver"
>> + depends on ARCH_BERLIN
>> + help
>> + Marvell Berlin ADC driver. This ADC has 8 channels, with one used for
>> + temparature measurement.
>
> temperature
>
>> +
>> config CC10001_ADC
>> tristate "Cosmic Circuits 10001 ADC driver"
>> depends on HAS_IOMEM || HAVE_CLK || REGULATOR
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 0315af640866..d7b2d1df2353 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
>> obj-$(CONFIG_AD799X) += ad799x.o
>> obj-$(CONFIG_AT91_ADC) += at91_adc.o
>> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>> +obj-$(CONFIG_BERLIN_ADC) += berlin-adc.o
>> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>> diff --git a/drivers/iio/adc/berlin-adc.c b/drivers/iio/adc/berlin-adc.c
>> new file mode 100644
>> index 000000000000..8cb0f5800511
>> --- /dev/null
>> +++ b/drivers/iio/adc/berlin-adc.c
>> @@ -0,0 +1,397 @@
>> +/*
>> + * Marvell Berlin ADC driver
>> + *
>> + * Copyright (C) 2015 Marvell Technology Group Ltd.
>> + *
>> + * Antoine Tenart <antoine.tenart@xxxxxxxxxxxxxxxxxx>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +#include <linux/sched.h>
>> +#include <linux/wait.h>
>> +
>
> the prefix SYSCTL prefix is unexpected, couldn't you use BERLIN_ or
> something?
>
>> +#define SYSCTL_SM_CTRL 0x14
>> +#define SYSCTL_SM_CTRL_SM_SOC_INT BIT(1)
>
> whitespace issue?
>
>> +#define SYSCTL_SM_CTRL_SOC_SM_INT BIT(2)
>> +#define SYSCTL_SM_CTRL_ADC_SEL(x) (BIT(x) << 5) /* 0-15 */
>> +#define SYSCTL_SM_CTRL_ADC_SEL_MASK (0xf << 5)
>> +#define SYSCTL_SM_CTRL_ADC_POWER BIT(9)
>> +#define SYSCTL_SM_CTRL_ADC_CLKSEL_DIV2 (0x0 << 10)
>> +#define SYSCTL_SM_CTRL_ADC_CLKSEL_DIV3 (0x1 << 10)
>> +#define SYSCTL_SM_CTRL_ADC_CLKSEL_DIV4 (0x2 << 10)
>> +#define SYSCTL_SM_CTRL_ADC_CLKSEL_DIV8 (0x3 << 10)
>> +#define SYSCTL_SM_CTRL_ADC_CLKSEL_MASK (0x3 << 10)
>> +#define SYSCTL_SM_CTRL_ADC_START BIT(12)
>> +#define SYSCTL_SM_CTRL_ADC_RESET BIT(13)
>> +#define SYSCTL_SM_CTRL_ADC_BANDGAP_RDY BIT(14)
>> +#define SYSCTL_SM_CTRL_ADC_CONT_SINGLE (0x0 << 15)
>> +#define SYSCTL_SM_CTRL_ADC_CONT_CONTINUOUS (0x1 << 15)
>> +#define SYSCTL_SM_CTRL_ADC_BUFFER_EN BIT(16)
>> +#define SYSCTL_SM_CTRL_ADC_VREF_EXT (0x0 << 17)
>> +#define SYSCTL_SM_CTRL_ADC_VREF_INT (0x1 << 17)
>> +#define SYSCTL_SM_CTRL_ADC_ROTATE BIT(19)
>> +#define SYSCTL_SM_CTRL_TSEN_EN BIT(20)
>> +#define SYSCTL_SM_CTRL_TSEN_CLK_SEL_125 (0x0 << 21) /* 1.25 MHz */
>> +#define SYSCTL_SM_CTRL_TSEN_CLK_SEL_250 (0x1 << 21) /* 2.5 MHz */
>> +#define SYSCTL_SM_CTRL_TSEN_MODE_0_125 (0x0 << 22) /* 0-125 C */
>> +#define SYSCTL_SM_CTRL_TSEN_MODE_10_50 (0x1 << 22) /* 10-50 C */
>> +#define SYSCTL_SM_CTRL_TSEN_RESET BIT(29)
>> +#define SYSCTL_SM_ADC_DATA 0x20
>> +#define SYSCTL_SM_ADC_MASK 0x3ff
>> +#define SYSCTL_SM_ADC_STATUS 0x1c
>> +#define SYSCTL_SM_ADC_STATUS_DATA_RDY(x) BIT(x) /* 0-15 */
>> +#define SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK 0xf
>> +#define SYSCTL_SM_ADC_STATUS_INT_EN(x) (BIT(x) << 16) /* 0-15 */
>> +#define SYSCTL_SM_ADC_STATUS_INT_EN_MASK (0xf << 16)
>> +#define SYSCTL_SM_TSEN_STATUS 0x24
>> +#define SYSCTL_SM_TSEN_STATUS_DATA_RDY BIT(0)
>> +#define SYSCTL_SM_TSEN_STATUS_INT_EN BIT(1)
>> +#define SYSCTL_SM_TSEN_DATA 0x28
>> +#define SYSCTL_SM_TSEN_MASK 0x3ff
>> +#define SYSCTL_SM_TSEN_CTRL 0x74
>> +#define SYSCTL_SM_TSEN_CTRL_START BIT(8)
>> +#define SYSCTL_SM_TSEN_CTRL_SETTLING_4 (0x0 << 21) /* 4 us */
>> +#define SYSCTL_SM_TSEN_CTRL_SETTLING_12 (0x1 << 21) /* 12 us */
>> +#define SYSCTL_SM_TSEN_CTRL_TRIM(x) ((x) << 22)
>> +#define SYSCTL_SM_TSEN_CTRL_TRIM_MASK (0xf << 22)
>> +
>> +struct berlin2_adc_priv {
>> + struct regmap *regmap;
>> + struct mutex lock;
>> + wait_queue_head_t wq;
>> + u32 irq;
>> + u32 tsen_irq;
>
> int rather
>
>> + bool data_available;
>> + int data;
>> +};
>> +
>> +#define BERLIN2_ADC_CHANNEL(n, t) \
>> + { \
>> + .channel = n, \
>> + .datasheet_name = "channel"#n, \
>> + .type = t, \
>> + .indexed = 1, \
>> + .scan_index = n, \
>> + .scan_type = { \
>> + .sign = 'u', \
>> + .realbits = 64, \
>> + .storagebits = 64, \
>> + }, \
>
> the data read is not 64 bit I think
>
> the driver does not seem to support buffered reads, so scan_type and
> scan_index can be removed
>
>
>> + .info_mask_shared_by_type = 0, \
>
> .info_mask_shared_by_type = 0 is unnecessary
>
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> + }
>> +
>> +#define N_CHANNELS 8
>
> not prefixed; would be good if you could do with ARRAY_SIZE over
> _adc_channels instead
>
>> +static struct iio_chan_spec berlin2_adc_channels[] = {
>
> why berlin2?
>
>> + BERLIN2_ADC_CHANNEL(0, IIO_VOLTAGE), /* external input */
>> + BERLIN2_ADC_CHANNEL(1, IIO_VOLTAGE), /* external input */
>> + BERLIN2_ADC_CHANNEL(2, IIO_VOLTAGE), /* external input */
>> + BERLIN2_ADC_CHANNEL(3, IIO_VOLTAGE), /* external input */
>> + BERLIN2_ADC_CHANNEL(4, IIO_VOLTAGE), /* reserved */
>> + BERLIN2_ADC_CHANNEL(5, IIO_VOLTAGE), /* reserved */
>> + BERLIN2_ADC_CHANNEL(6, IIO_TEMP), /* temperature sensor */
>
> the temperature channel is not indexed (there is only one)
>
>> + BERLIN2_ADC_CHANNEL(7, IIO_VOLTAGE), /* reserved */
>> + { /* timestamp */
>
>
> use IIO_CHAN_SOFT_TIMESTAMP(N_CHANNELS) here
>
>> + .channel = -1,
>> + .type = IIO_TIMESTAMP,
>> + .scan_index = N_CHANNELS,
>> + .scan_type = {
>> + .sign = 's',
>> + .realbits = 64,
>> + .storagebits = 64,
>> + },
>> + },
>> +};
>> +
>> +static int berlin2_adc_read(struct iio_dev *idev, int channel)
>> +{
>> + struct berlin2_adc_priv *priv = iio_priv(idev);
>> + int val, data, ret;
>> +
>> + mutex_lock(&priv->lock);
>> +
>> + /* Configure the ADC */
>> + regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> + val &= ~(SYSCTL_SM_CTRL_ADC_RESET | SYSCTL_SM_CTRL_ADC_SEL_MASK);
>> + val |= SYSCTL_SM_CTRL_ADC_SEL(channel) | SYSCTL_SM_CTRL_ADC_START;
>> + regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> + /* Enable the interrupts */
>> + regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS,
>> + SYSCTL_SM_ADC_STATUS_INT_EN(channel));
>> +
>> + ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
>> + msecs_to_jiffies(1000));
>> +
>> + /* Disable the interrupts */
>> + regmap_read(priv->regmap, SYSCTL_SM_ADC_STATUS, &val);
>> + val &= ~SYSCTL_SM_ADC_STATUS_INT_EN(channel);
>> + regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS, val);
>> +
>> + if (ret == 0)
>> + ret = -ETIMEDOUT;
>> + if (ret < 0) {
>> + mutex_unlock(&priv->lock);
>> + return ret;
>> + }
>> +
>> + regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> + val &= ~SYSCTL_SM_CTRL_ADC_START;
>> + regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> + data = priv->data;
>> + priv->data = -1;
>> + priv->data_available = false;
>> +
>> + mutex_unlock(&priv->lock);
>> +
>> + return data;
>> +}
>> +
>> +static int berlin2_adc_tsen_read(struct iio_dev *idev)
>> +{
>> + struct berlin2_adc_priv *priv = iio_priv(idev);
>> + int val, data, ret;
>> +
>> + mutex_lock(&priv->lock);
>> +
>> + /* Configure the ADC */
>> + regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> + val &= ~SYSCTL_SM_CTRL_TSEN_RESET;
>> + val |= SYSCTL_SM_CTRL_ADC_ROTATE;
>> + regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> + /* Configure the temperature sensor */
>> + regmap_read(priv->regmap, SYSCTL_SM_TSEN_CTRL, &val);
>> + val &= ~SYSCTL_SM_TSEN_CTRL_TRIM_MASK;
>> + val |= SYSCTL_SM_TSEN_CTRL_TRIM(3) | SYSCTL_SM_TSEN_CTRL_SETTLING_12
>> + | SYSCTL_SM_TSEN_CTRL_START;
>> + regmap_write(priv->regmap, SYSCTL_SM_TSEN_CTRL, val);
>> +
>> + /* Enable interrupts */
>> + regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS,
>> + SYSCTL_SM_TSEN_STATUS_INT_EN);
>> +
>> + ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
>> + msecs_to_jiffies(1000));
>> +
>> + /* Disable interrupts */
>> + regmap_read(priv->regmap, SYSCTL_SM_TSEN_STATUS, &val);
>> + val &= ~SYSCTL_SM_TSEN_STATUS_INT_EN;
>> + regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS, val);
>> +
>> + if (ret == 0)
>> + ret = -ETIMEDOUT;
>> + if (ret < 0) {
>> + mutex_unlock(&priv->lock);
>> + return ret;
>> + }
>> +
>> + regmap_read(priv->regmap, SYSCTL_SM_TSEN_CTRL, &val);
>> + val &= ~SYSCTL_SM_TSEN_CTRL_START;
>> + regmap_write(priv->regmap, SYSCTL_SM_TSEN_CTRL, val);
>> +
>> + data = priv->data;
>> + priv->data = -1;
>> + priv->data_available = false;
>> +
>> + mutex_unlock(&priv->lock);
>> +
>> + return data;
>> +}
>> +
>> +static int berlin2_adc_read_raw(struct iio_dev *idev,
>> + struct iio_chan_spec const *chan, int *val, int *val2,
>> + long mask)
>> +{
>> + int temp;
>
> maybe named it ret and use instead of *val and temp
>
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + if (chan->type == IIO_VOLTAGE) {
>> + *val = berlin2_adc_read(idev, chan->channel);
>> + if (*val < 0)
>> + return *val;
>
> is this milli-Volts?
>
>> +
>> + return IIO_VAL_INT;
>> + } else if (chan->type == IIO_TEMP) {
>> + temp = berlin2_adc_tsen_read(idev);
>> + if (temp < 0)
>> + return temp;
>> +
>> + if (temp > 2047)
>> + temp = -(4096 - temp);
>> +
>> + /* Convert to Celsius */
>> + *val = (temp * 100) / 264 - 270;
>
> use SCALE and OFFSET IIO_CHAN_INFO_s to that the temperature can be
> interpreted as milli-Celsius (or use _PROCESSED, not _RAW)
>
>> + return IIO_VAL_INT;
>> + }
>> + default:
>> + break;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static irqreturn_t berlin2_adc_irq(int irq, void *private)
>> +{
>> + struct iio_dev *idev = private;
>> + struct berlin2_adc_priv *priv = iio_priv(idev);
>> + unsigned val;
>> +
>> + regmap_read(priv->regmap, SYSCTL_SM_ADC_STATUS, &val);
>> + if (val & SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK) {
>> + regmap_read(priv->regmap, SYSCTL_SM_ADC_DATA, &priv->data);
>> + priv->data &= SYSCTL_SM_ADC_MASK;
>> +
>> + val &= ~SYSCTL_SM_ADC_STATUS_DATA_RDY_MASK;
>> + regmap_write(priv->regmap, SYSCTL_SM_ADC_STATUS, val);
>> +
>> + priv->data_available = true;
>> + wake_up_interruptible(&priv->wq);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t berlin2_adc_tsen_irq(int irq, void *private)
>> +{
>> + struct iio_dev *idev = private;
>> + struct berlin2_adc_priv *priv = iio_priv(idev);
>> + unsigned val;
>> +
>> + regmap_read(priv->regmap, SYSCTL_SM_TSEN_STATUS, &val);
>> + if (val & SYSCTL_SM_TSEN_STATUS_DATA_RDY) {
>> + regmap_read(priv->regmap, SYSCTL_SM_TSEN_DATA, &priv->data);
>> + priv->data &= SYSCTL_SM_TSEN_MASK;
>> +
>> + val &= ~SYSCTL_SM_TSEN_STATUS_DATA_RDY;
>> + regmap_write(priv->regmap, SYSCTL_SM_TSEN_STATUS, val);
>> +
>> + priv->data_available = true;
>> + wake_up_interruptible(&priv->wq);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static const struct iio_info berlin2_adc_info = {
>> + .driver_module = THIS_MODULE,
>> + .read_raw = berlin2_adc_read_raw,
>> +};
>> +
>> +static int berlin2_adc_probe(struct platform_device *pdev)
>> +{
>> + struct iio_dev *idev;
>
> people prefer indio_dev instead of idev
>
>> + struct berlin2_adc_priv *priv;
>> + struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
>> + int ret, val;
>> +
>> + idev = iio_device_alloc(sizeof(struct berlin2_adc_priv));
>
> devm_iio_device_alloc?
>
>> + if (!idev)
>> + return -ENOMEM;
>> +
>> + priv = iio_priv(idev);
>> + platform_set_drvdata(pdev, idev);
>> +
>> + priv->regmap = syscon_node_to_regmap(parent_np);
>> + of_node_put(parent_np);
>> + if (IS_ERR(priv->regmap))
>> + return PTR_ERR(priv->regmap);
>> +
>> + priv->irq = platform_get_irq_byname(pdev, "adc");
>> + if (priv->irq < 0)
>
> you have irq as u32, should be int otherwise the check does not make sense
>
>> + return -ENODEV;
>> +
>> + priv->tsen_irq = platform_get_irq_byname(pdev, "tsen");
>> + if (priv->tsen_irq < 0)
>> + return -ENODEV;
>> +
>> + ret = devm_request_irq(&pdev->dev, priv->irq, berlin2_adc_irq, 0,
>> + pdev->dev.driver->name, idev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = devm_request_irq(&pdev->dev, priv->tsen_irq, berlin2_adc_tsen_irq,
>> + 0, pdev->dev.driver->name, idev);
>> + if (ret)
>> + return ret;
>> +
>> + init_waitqueue_head(&priv->wq);
>> + mutex_init(&priv->lock);
>> +
>> + idev->dev.parent = &pdev->dev;
>> + idev->name = dev_name(&pdev->dev);
>> + idev->modes = INDIO_DIRECT_MODE;
>> + idev->info = &berlin2_adc_info;
>> +
>> + idev->num_channels = N_CHANNELS;
>> + idev->channels = berlin2_adc_channels;
>> +
>> + /* Power up the ADC */
>> + regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> + val |= SYSCTL_SM_CTRL_ADC_POWER;
>> + regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> + ret = iio_device_register(idev);
>
> devm_iio_device_register?
>
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to register the IIO device: %d\n",
>> + ret);
>
> probably not the most useful error msg and about the only logging; drop
> it?
>
> and just do
> return devm_iio_device_register(idev);
There is an ordering issue here that makes that look sensible, but it isn't.
Rule of thumb. If there is anything else in the remove, you shouldn't
use devm_iio_device_register (same is often true of the interrupt equivalent
though occasionally the ordering is such that using the devm versions
of those is fine.)
>
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int berlin2_adc_remove(struct platform_device *pdev)
>> +{
>> + struct iio_dev *idev = platform_get_drvdata(pdev);
>> + struct berlin2_adc_priv *priv = iio_priv(idev);
>> + int val;
>> +
>> + /* Power down the ADC */
>> + regmap_read(priv->regmap, SYSCTL_SM_CTRL, &val);
>> + val &= ~SYSCTL_SM_CTRL_ADC_POWER;
>> + regmap_write(priv->regmap, SYSCTL_SM_CTRL, val);
>> +
>> + free_irq(priv->irq, idev);
>> + free_irq(priv->tsen_irq, idev);
>> +
>> + iio_device_unregister(idev);
You are removing the userspace interface only after you have
turned the device off and disabled the irq's etc. Not a good
plan. There is a reason why most remove functions are the
reverse of the probe (in what they do and in the order they do
it in). Leads to far fewer issues like this and makes them much
easier to review!
>> + iio_device_free(idev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id berlin2_adc_match[] = {
>> + { .compatible = "marvell,berlin2-adc", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, berlin2q_adc_match);
>> +
>> +static struct platform_driver berlin2_adc_driver = {
>> + .driver = {
>> + .name = "berlin2-adc",
>> + .of_match_table = berlin2_adc_match,
>> + },
>> + .probe = berlin2_adc_probe,
>> + .remove = berlin2_adc_remove,
>> +};
>> +module_platform_driver(berlin2_adc_driver);
>> +
>> +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@xxxxxxxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Marvell Berlin2 ADC driver");
>> +MODULE_LICENSE("GPL");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
