Re: [PATCH v2 1/4] iio: adc: add support for Berlin

From: Jonathan Cameron
Date: Thu Apr 09 2015 - 09:58:36 EST

Next message: Jonathan Cameron: "Re: [PATCH v2 2/4] Documentation: bindings: document the Berlin ADC driver"
Previous message: Steven Rostedt: "Re: [PATCH v5 2/3] sched/rt: Fix wrong SMP scheduler behavior for equal prio cases"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 03/04/15 14:06, Antoine Tenart wrote:
> 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.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@xxxxxxxxxxxxxxxxxx>
Few bits and bobs inline. However, big one is that you've gotten
confused on how the devm managed allocations work....
Also a lot of care is needed with using devm_iio_device_register.
It only makes sense in really simple devices where there really is nothing
to do on device removal...

> ---
> drivers/iio/adc/Kconfig | 7 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/berlin2-adc.c | 379 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 387 insertions(+)
> create mode 100644 drivers/iio/adc/berlin2-adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 202daf889be2..92cc40fd5e71 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 BERLIN2_ADC
> + tristate "Marvell Berlin2 ADC driver"
> + depends on ARCH_BERLIN
> + help
> + Marvell Berlin2 ADC driver. This ADC has 8 channels, with one used for
> + temperature measurement.
> +
> 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..2b6dadaf7714 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_BERLIN2_ADC) += berlin2-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/berlin2-adc.c b/drivers/iio/adc/berlin2-adc.c
> new file mode 100644
> index 000000000000..cd21fefc403e
> --- /dev/null
> +++ b/drivers/iio/adc/berlin2-adc.c
> @@ -0,0 +1,379 @@
> +/*
> + * Marvell Berlin2 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>
> +
> +#define BERLIN2_SM_CTRL 0x14
> +#define BERLIN2_SM_CTRL_SM_SOC_INT BIT(1)
> +#define BERLIN2_SM_CTRL_SOC_SM_INT BIT(2)
> +#define BERLIN2_SM_CTRL_ADC_SEL(x) (BIT(x) << 5) /* 0-15 */
> +#define BERLIN2_SM_CTRL_ADC_SEL_MASK (0xf << 5)
> +#define BERLIN2_SM_CTRL_ADC_POWER BIT(9)
> +#define BERLIN2_SM_CTRL_ADC_CLKSEL_DIV2 (0x0 << 10)
> +#define BERLIN2_SM_CTRL_ADC_CLKSEL_DIV3 (0x1 << 10)
> +#define BERLIN2_SM_CTRL_ADC_CLKSEL_DIV4 (0x2 << 10)
> +#define BERLIN2_SM_CTRL_ADC_CLKSEL_DIV8 (0x3 << 10)
> +#define BERLIN2_SM_CTRL_ADC_CLKSEL_MASK (0x3 << 10)
> +#define BERLIN2_SM_CTRL_ADC_START BIT(12)
> +#define BERLIN2_SM_CTRL_ADC_RESET BIT(13)
> +#define BERLIN2_SM_CTRL_ADC_BANDGAP_RDY BIT(14)
> +#define BERLIN2_SM_CTRL_ADC_CONT_SINGLE (0x0 << 15)
> +#define BERLIN2_SM_CTRL_ADC_CONT_CONTINUOUS (0x1 << 15)
> +#define BERLIN2_SM_CTRL_ADC_BUFFER_EN BIT(16)
> +#define BERLIN2_SM_CTRL_ADC_VREF_EXT (0x0 << 17)
> +#define BERLIN2_SM_CTRL_ADC_VREF_INT (0x1 << 17)
> +#define BERLIN2_SM_CTRL_ADC_ROTATE BIT(19)
> +#define BERLIN2_SM_CTRL_TSEN_EN BIT(20)
> +#define BERLIN2_SM_CTRL_TSEN_CLK_SEL_125 (0x0 << 21) /* 1.25 MHz */
> +#define BERLIN2_SM_CTRL_TSEN_CLK_SEL_250 (0x1 << 21) /* 2.5 MHz */
> +#define BERLIN2_SM_CTRL_TSEN_MODE_0_125 (0x0 << 22) /* 0-125 C */
> +#define BERLIN2_SM_CTRL_TSEN_MODE_10_50 (0x1 << 22) /* 10-50 C */
> +#define BERLIN2_SM_CTRL_TSEN_RESET BIT(29)
> +#define BERLIN2_SM_ADC_DATA 0x20
> +#define BERLIN2_SM_ADC_MASK 0x3ff
> +#define BERLIN2_SM_ADC_STATUS 0x1c
> +#define BERLIN2_SM_ADC_STATUS_DATA_RDY(x) BIT(x) /* 0-15 */
> +#define BERLIN2_SM_ADC_STATUS_DATA_RDY_MASK 0xf
> +#define BERLIN2_SM_ADC_STATUS_INT_EN(x) (BIT(x) << 16) /* 0-15 */
> +#define BERLIN2_SM_ADC_STATUS_INT_EN_MASK (0xf << 16)
> +#define BERLIN2_SM_TSEN_STATUS 0x24
> +#define BERLIN2_SM_TSEN_STATUS_DATA_RDY BIT(0)
> +#define BERLIN2_SM_TSEN_STATUS_INT_EN BIT(1)
> +#define BERLIN2_SM_TSEN_DATA 0x28
> +#define BERLIN2_SM_TSEN_MASK 0xfff
> +#define BERLIN2_SM_TSEN_CTRL 0x74
> +#define BERLIN2_SM_TSEN_CTRL_START BIT(8)
> +#define BERLIN2_SM_TSEN_CTRL_SETTLING_4 (0x0 << 21) /* 4 us */
> +#define BERLIN2_SM_TSEN_CTRL_SETTLING_12 (0x1 << 21) /* 12 us */
> +#define BERLIN2_SM_TSEN_CTRL_SETTLING_MASK (0x1 << 21)
> +#define BERLIN2_SM_TSEN_CTRL_TRIM(x) ((x) << 22)
> +#define BERLIN2_SM_TSEN_CTRL_TRIM_MASK (0xf << 22)
> +
> +struct berlin2_adc_priv {
> + struct regmap *regmap;
> + struct mutex lock;
> + wait_queue_head_t wq;
> + int irq;
> + int tsen_irq;
> + bool data_available;
> + int data;
> +};
> +
> +#define BERLIN2_ADC_CHANNEL(n, t) \
> + { \
> + .channel = n, \
> + .datasheet_name = "channel"#n, \
> + .type = t, \
> + .indexed = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + }
> +
> +static struct iio_chan_spec berlin2_adc_channels[] = {
> + 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 */
> + { /* temperature sensor */
> + .channel = 6,
> + .datasheet_name = "channel6",
> + .type = IIO_TEMP,
> + .indexed = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + },
> + BERLIN2_ADC_CHANNEL(7, IIO_VOLTAGE), /* reserved */
> + IIO_CHAN_SOFT_TIMESTAMP(8), /* timestamp */
> +};
> +#define BERLIN2_N_CHANNELS ARRAY_SIZE(berlin2_adc_channels)
> +
> +static int berlin2_adc_read(struct iio_dev *indio_dev, int channel)
> +{
> + struct berlin2_adc_priv *priv = iio_priv(indio_dev);
> + int data, ret;
> +
> + mutex_lock(&priv->lock);
> +
> + /* Configure the ADC */
> + regmap_update_bits(priv->regmap, BERLIN2_SM_CTRL,
> + BERLIN2_SM_CTRL_ADC_RESET | BERLIN2_SM_CTRL_ADC_SEL_MASK
> + | BERLIN2_SM_CTRL_ADC_START,
> + BERLIN2_SM_CTRL_ADC_SEL(channel) | BERLIN2_SM_CTRL_ADC_START);
> +
> + /* Enable the interrupts */
> + regmap_write(priv->regmap, BERLIN2_SM_ADC_STATUS,
> + BERLIN2_SM_ADC_STATUS_INT_EN(channel));
> +
> + ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
> + msecs_to_jiffies(1000));
> +
> + /* Disable the interrupts */
> + regmap_update_bits(priv->regmap, BERLIN2_SM_ADC_STATUS,
> + BERLIN2_SM_ADC_STATUS_INT_EN(channel), 0);
> +
> + if (ret == 0)
> + ret = -ETIMEDOUT;
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> +
> + regmap_update_bits(priv->regmap, BERLIN2_SM_CTRL,
> + BERLIN2_SM_CTRL_ADC_START, 0);
> +
> + 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 *indio_dev)
> +{
> + struct berlin2_adc_priv *priv = iio_priv(indio_dev);
> + int data, ret;
> +
> + mutex_lock(&priv->lock);
> +
> + /* Configure the ADC */
> + regmap_update_bits(priv->regmap, BERLIN2_SM_CTRL,
> + BERLIN2_SM_CTRL_TSEN_RESET | BERLIN2_SM_CTRL_ADC_ROTATE,
> + BERLIN2_SM_CTRL_ADC_ROTATE);
> +
> + /* Configure the temperature sensor */
> + regmap_update_bits(priv->regmap, BERLIN2_SM_TSEN_CTRL,
> + BERLIN2_SM_TSEN_CTRL_TRIM_MASK | BERLIN2_SM_TSEN_CTRL_SETTLING_MASK
> + | BERLIN2_SM_TSEN_CTRL_START,
> + BERLIN2_SM_TSEN_CTRL_TRIM(3) | BERLIN2_SM_TSEN_CTRL_SETTLING_12
> + | BERLIN2_SM_TSEN_CTRL_START);
> +
> + /* Enable interrupts */
> + regmap_write(priv->regmap, BERLIN2_SM_TSEN_STATUS,
> + BERLIN2_SM_TSEN_STATUS_INT_EN);
Is there a possible race here? Depends on whether this is a level interrupt
or an edge triggered one... Would probably expect this just before the
call to start the read.
> +
> + ret = wait_event_interruptible_timeout(priv->wq, priv->data_available,
> + msecs_to_jiffies(1000));
> +
> + /* Disable interrupts */
> + regmap_update_bits(priv->regmap, BERLIN2_SM_TSEN_STATUS,
> + BERLIN2_SM_TSEN_STATUS_INT_EN, 0);
> +
> + if (ret == 0)
> + ret = -ETIMEDOUT;
> + if (ret < 0) {
> + mutex_unlock(&priv->lock);
> + return ret;
> + }
> +
> + regmap_update_bits(priv->regmap, BERLIN2_SM_TSEN_CTRL,
> + BERLIN2_SM_TSEN_CTRL_START, 0);
> +
> + 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 *indio_dev,
> + struct iio_chan_spec const *chan, int *val, int *val2,
> + long mask)
> +{
> + int temp;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (chan->type != IIO_VOLTAGE)
> + return -EINVAL;
> +
> + *val = berlin2_adc_read(indio_dev, chan->channel);
> + if (*val < 0)
> + return *val;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_PROCESSED:
> + if (chan->type != IIO_TEMP)
> + return -EINVAL;
> +
> + temp = berlin2_adc_tsen_read(indio_dev);
> + if (temp < 0)
> + return temp;
> +
> + if (temp > 2047)
> + temp = -(4096 - temp);
> +
> + /* Convert to Celsius */
> + *val = (temp * 100) / 264 - 270;
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static irqreturn_t berlin2_adc_irq(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct berlin2_adc_priv *priv = iio_priv(indio_dev);
> + unsigned val;
> +
> + regmap_read(priv->regmap, BERLIN2_SM_ADC_STATUS, &val);
> + if (val & BERLIN2_SM_ADC_STATUS_DATA_RDY_MASK) {
> + regmap_read(priv->regmap, BERLIN2_SM_ADC_DATA, &priv->data);
> + priv->data &= BERLIN2_SM_ADC_MASK;
> +
> + val &= ~BERLIN2_SM_ADC_STATUS_DATA_RDY_MASK;
> + regmap_write(priv->regmap, BERLIN2_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 *indio_dev = private;
> + struct berlin2_adc_priv *priv = iio_priv(indio_dev);
I'd roll the above two lines into one.
struct berlin2_adc_priv *priv = iio_priv(private);

> + unsigned val;
> +
> + regmap_read(priv->regmap, BERLIN2_SM_TSEN_STATUS, &val);
> + if (val & BERLIN2_SM_TSEN_STATUS_DATA_RDY) {
> + regmap_read(priv->regmap, BERLIN2_SM_TSEN_DATA, &priv->data);
> + priv->data &= BERLIN2_SM_TSEN_MASK;
> +
> + val &= ~BERLIN2_SM_TSEN_STATUS_DATA_RDY;
> + regmap_write(priv->regmap, BERLIN2_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 *indio_dev;
> + struct berlin2_adc_priv *priv;
> + struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev,
> + sizeof(struct berlin2_adc_priv));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + priv = iio_priv(indio_dev);
> + platform_set_drvdata(pdev, indio_dev);
> +
> + 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)
> + 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, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_request_irq(&pdev->dev, priv->tsen_irq, berlin2_adc_tsen_irq,
> + 0, pdev->dev.driver->name, indio_dev);
> + if (ret)
> + return ret;
> +
> + init_waitqueue_head(&priv->wq);
> + mutex_init(&priv->lock);
> +
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &berlin2_adc_info;
> +
> + indio_dev->num_channels = BERLIN2_N_CHANNELS;
> + indio_dev->channels = berlin2_adc_channels;
> +
> + /* Power up the ADC */
> + regmap_update_bits(priv->regmap, BERLIN2_SM_CTRL,
> + BERLIN2_SM_CTRL_ADC_POWER, BERLIN2_SM_CTRL_ADC_POWER);
> +
> + return devm_iio_device_register(&pdev->dev, indio_dev);
you are mixing devm requests with explicit removes in the remove.
You are going to get double frees.

You'll also want to catch any errors from iio_device_register and power down
the device (via the regmap call) if the register fails.

> +}
> +
> +static int berlin2_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct berlin2_adc_priv *priv = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
As you used devm_iio_device_register you don't need to explicitly unregister.
However...
> +
> + /* Power down the ADC */
> + regmap_update_bits(priv->regmap, BERLIN2_SM_CTRL,
> + BERLIN2_SM_CTRL_ADC_POWER, 0);
If you do use the devm register above then the userspace access to the
hardware will only be removed AFTER the regmap_update_bits here. Hence
there will be a nasty window. I would suggest not using the
devm_iio_device_register but rather iio_device_register and then keep
the explicit unregister here.

> +
> + free_irq(priv->irq, indio_dev);
> + free_irq(priv->tsen_irq, indio_dev);
> +
> + iio_device_free(indio_dev);
You used devm versions of the irq requests and iio_device alloc. These
frees should not be here and will result in a double free.
> +
> + 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 v2");
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Jonathan Cameron: "Re: [PATCH v2 2/4] Documentation: bindings: document the Berlin ADC driver"
Previous message: Steven Rostedt: "Re: [PATCH v5 2/3] sched/rt: Fix wrong SMP scheduler behavior for equal prio cases"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]