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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);

> +		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);
> +	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");
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)
--
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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux