[PATCHv2 1/2] ARM: sunxi: iio: Adds support for Allwinner LRADC

[vinic...@gmail.com]

From: Vinicius Freire <vinic...@gmail.com>

This patch adds support for the built-in LRADC in Allwinner A10 and
A13 SoCs. It has DT-support and uses the new iio framework.

Signed-off-by: Vinicius Freire <vinic...@gmail.com>
---
drivers/iio/adc/Kconfig | 8 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/sunxi_lradc.c | 462 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 471 insertions(+)
create mode 100644 drivers/iio/adc/sunxi_lradc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 93129ec..5ffb17e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -103,6 +103,14 @@ config AT91_ADC
help
Say yes here to build support for Atmel AT91 ADC.

+config SUNXI_LRADC
+ tristate "Allwinner A1X LRADC"
+ depends on ARCH_SUNXI
+ select SYSFS
+ help
+ Say yes here to build support for built-in Allwinner
+ A10/A13 LRADC.
+
config EXYNOS_ADC
bool "Exynos ADC driver support"
depends on OF
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 8f475d3..6a7f1a8 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
obj-$(CONFIG_AD7793) += ad7793.o
obj-$(CONFIG_AD7887) += ad7887.o
obj-$(CONFIG_AT91_ADC) += at91_adc.o
+obj-$(CONFIG_SUNXI_LRADC) += sunxi_lradc.o
obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
diff --git a/drivers/iio/adc/sunxi_lradc.c b/drivers/iio/adc/sunxi_lradc.c
new file mode 100644
index 0000000..0088e3d
--- /dev/null
+++ b/drivers/iio/adc/sunxi_lradc.c
@@ -0,0 +1,462 @@
+/*
+ * Support for Allwinner LRADC in A1X SoCs
+ *
+ * based on linux/drivers/iio/adc/at91_adc.c
+ *
+ * Copyright 2011 Free Electrons
+ *
+ * Copyright 2013 Vinicius Freire <vinic...@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitmap.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/wait.h>
+
+#include <linux/iio/iio.h>
+
+/* A10 LRADC registers definitions */
+/* LRADC Control Register */
+#define SUNXI_LRADC_CTRL (0x00)
+/* LRADC Interrupt Control Register */
+#define SUNXI_LRADC_INTC (0x04)
+/* LRADC Interrupt Status Register */
+#define SUNXI_LRADC_INTS (0x08)
+/* LRADC Data 0 Register */
+#define SUNXI_LRADC_DATA0 (0x0C)
+/* LRADC Data 1 Register */
+#define SUNXI_LRADC_DATA1 (0x10)
+
+/* Bit definitions for LRADC_CTRL */
+#define SUNXI_LRADC_FST_DLY(x) ((x & 0x7F) << 24)
+#define SUNXI_LRADC_CHAN_SEL(x) ((x & 0x02) << 22)
+#define SUNXI_LRADC_TIME_SEL(x) ((x & 0x07) << 16)
+#define SUNXI_LRADC_KEY_MODE(x) ((x & 0x02) << 12)
+#define SUNXI_LRADC_LVLA_B(x) ((x & 0x0F) << 8)
+#define SUNXI_LRADC_HOLD_EN(x) ((x) << 6)
+#define SUNXI_LRADC_LVLB(x) ((x & 0x02) << 4)
+#define SUNXI_LRADC_S_RATE(x) ((x & 0x02) << 2)
+#define SUNXI_LRADC_EN(x) ((x) << 0)
+
+/* Bit definitions for LRADC_INTC */
+#define SUNXI_LRADC1_KEYUP_IRQ(x) ((x) << 12)
+#define SUNXI_LRADC1_ALR_HOLD_IRQ(x) ((x) << 11)
+#define SUNXI_LRADC1_HOLD_IRQ(x) ((x) << 10)
+#define SUNXI_LRADC1_KEY_IRQ(x) ((x) << 9)
+#define SUNXI_LRADC1_DATA_IRQ(x) ((x) << 8)
+
+#define SUNXI_LRADC0_KEYUP_IRQ(x) ((x) << 4)
+#define SUNXI_LRADC0_ALR_HOLD_IRQ(x) ((x) << 3)
+#define SUNXI_LRADC0_HOLD_IRQ(x) ((x) << 2)
+#define SUNXI_LRADC0_KEYDOWN(x) ((x) << 1)
+#define SUNXI_LRADC0_DATA_IRQ(x) ((x) << 0)
+
+/* Bit definitions for LRADC_INTS */
+/* Masks */
+#define SUNXI_LRADC1_KEYUP_PEN 0x0C
+#define SUNXI_LRADC1_ALR_HOLD_PEN 0x0B
+#define SUNXI_LRADC1_HOLDKEY_PEN 0x0A
+#define SUNXI_LRADC1_KEYDOWN_IRQ_PEN 0x09
+#define SUNXI_LRADC1_DATA_IRQ_EN 0x08
+
+#define SUNXI_LRADC0_KEYUP_PEN 0x04
+#define SUNXI_LRADC0_ALR_HOLD_PEN 0x03
+#define SUNXI_LRADC0_HOLDKEY_PEN 0x02
+#define SUNXI_LRADC0_KEYDOWN_IRQ_PEN 0x01
+#define SUNXI_LRADC0_DATA_IRQ_EN 0x00
+
+#define sunxi_adc_readl(info, reg) \
+ (readl(info->reg_base + reg))
+#define sunxi_adc_writel(info, reg, val) \
+ (writel(val, info->reg_base + reg))
+
+struct sunxi_adc_reg {
+ u32 lradc_ctrl;
+ u32 lradc_intc;
+ u32 lradc_ints;
+};
+
+struct sunxi_adc_data {
+ unsigned long channels_used;
+ u8 num_channels;
+ struct sunxi_adc_reg *registers;
+ u8 startup_time;
+ u16 vref;
+ u32 inter_mask;
+};
+
+struct sunxi_adc_state {
+ u16 *buffer;
+ unsigned long channels_mask;
+ u8 channel_cur;
+ bool done;
+ int irq;
+ u16 last_value;
+ struct mutex lock;
+ u8 num_channels;
+ void __iomem *reg_base;
+ struct sunxi_adc_reg *registers;
+ u8 startup_time;
+ u8 sample_hold_time;
+ u32 vref_mv;
+ wait_queue_head_t wq_data_avail;
+ bool sample_hold_en;
+ u8 sample_rate;
+ u32 inter_mask;
+};
+
+static irqreturn_t sunxi_lradc_irq(int irq, void *private)
+{
+ struct iio_dev *idev = private;
+ struct sunxi_adc_state *st = iio_priv(idev);
+ u32 status = sunxi_adc_readl(st, st->registers->lradc_ints);
+
+ if (!(status & st->inter_mask))
+ return IRQ_HANDLED;
+ st->last_value = sunxi_adc_readl(st, st->channel_cur);
+ st->done = true;
+ wake_up_interruptible(&st->wq_data_avail);
+
+ return IRQ_HANDLED;
+}
+
+static int sunxi_adc_channel_init(struct iio_dev *idev)
+{
+ struct sunxi_adc_state *st = iio_priv(idev);
+ struct iio_chan_spec *chan_array;
+ int bit, idx = 0;
+
+ idev->num_channels = bitmap_weight(&st->channels_mask,
+ st->num_channels) + 1;
+
+ chan_array = devm_kzalloc(&idev->dev,
+ ((idev->num_channels + 1) *
+ sizeof(struct iio_chan_spec)),
+ GFP_KERNEL);
+
+ if (!chan_array)
+ return -ENOMEM;
+
+ for_each_set_bit(bit, &st->channels_mask, st->num_channels) {
+ struct iio_chan_spec *chan = chan_array + idx;
+
+ chan->type = IIO_VOLTAGE;
+ chan->indexed = 1;
+ chan->channel = bit;
+ chan->scan_index = idx;
+ chan->scan_type.sign = 'u';
+ chan->scan_type.realbits = 6;
+ chan->scan_type.storagebits = 32;
+ chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+ chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+ idx++;
+ }
+
+ idev->channels = chan_array;
+ return idev->num_channels;
+}
+
+static int sunxi_adc_read_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct sunxi_adc_state *st = iio_priv(idev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ mutex_lock(&st->lock);
+
+ sunxi_adc_writel(st, SUNXI_LRADC_CTRL,
+ SUNXI_LRADC_CHAN_SEL(chan->channel));
+ st->channel_cur = chan->channel;
+ sunxi_adc_writel(st, SUNXI_LRADC_INTC, st->inter_mask);
+ sunxi_adc_writel(st, SUNXI_LRADC_CTRL, SUNXI_LRADC_EN(1));
+
+ ret = wait_event_interruptible_timeout(st->wq_data_avail,
+ st->done,
+ msecs_to_jiffies(1000));
+ if (ret == 0)
+ ret = -ETIMEDOUT;
+ if (ret < 0) {
+ mutex_unlock(&st->lock);
+ return ret;
+ }
+
+ *val = st->last_value;
+
+ sunxi_adc_writel(st, SUNXI_LRADC_CTRL, SUNXI_LRADC_EN(0));
+ sunxi_adc_writel(st, SUNXI_LRADC_INTC, ~(st->inter_mask));
+
+ st->last_value = 0;
+ st->done = false;
+ mutex_unlock(&st->lock);
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ *val = (st->vref_mv * 1000) >> chan->scan_type.realbits;
+ *val2 = 0;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ break;
+ }
+ return -EINVAL;
+}
+
+static int sunxi_adc_probe_dt(struct sunxi_adc_state *st,
+ struct platform_device *pdev)
+{
+ struct iio_dev *idev = iio_priv_to_dev(st);
+ struct device_node *node = pdev->dev.of_node;
+ int ret;
+ u32 prop;
+ u8 prop8;
+
+ if (!node)
+ return -EINVAL;
+
+ if (of_property_read_u32(node, "sunxi,adc-channels-used", &prop)) {
+ dev_err(&idev->dev, "Missing adc-channels-used property in the DT.\n");
+ ret = -EINVAL;
+ goto error_ret;
+ }
+ st->channels_mask = prop;
+
+ if (of_property_read_u32(node, "sunxi,adc-num-channels", &prop)) {
+ dev_err(&idev->dev, "Missing adc-num-channels property in the DT.\n");
+ ret = -EINVAL;
+ goto error_ret;
+ }
+ st->num_channels = prop;
+
+ if (of_property_read_u32(node, "sunxi,adc-startup-time", &prop)) {
+ dev_err(&idev->dev, "Missing adc-startup-time property in the DT.\n");
+ ret = -EINVAL;
+ goto error_ret;
+ }
+ st->startup_time = prop;
+
+ if (of_property_read_u32(node, "sunxi,adc-interrupt-mask", &prop)) {
+ dev_err(&idev->dev, "Missing adc-interrupt-mask property in the DT.\n");
+ ret = -EINVAL;
+ goto error_ret;
+ }
+ st->inter_mask = prop;
+
+ if (of_property_read_u32(node, "sunxi,adc-vref", &prop)) {
+ dev_err(&idev->dev, "Missing adc-vref property in the DT.\n");
+ ret = -EINVAL;
+ goto error_ret;
+ }
+ st->vref_mv = prop;
+
+ if (of_property_read_u8(node, "sunxi,adc-sample-rate", &prop8)) {
+ /* if it is not defined, uses 250Hz */
+ prop8 = 0x00;
+ }
+ st->sample_rate = prop8;
+
+ st->sample_hold_en =
+ of_property_read_bool(node, "sunxi,adc-sample-hold-enable");
+
+ st->registers = devm_kzalloc(&idev->dev,
+ sizeof(struct sunxi_adc_reg),
+ GFP_KERNEL);
+ if (!st->registers) {
+ dev_err(&idev->dev, "Could not allocate register memory.\n");
+ ret = -ENOMEM;
+ goto error_ret;
+ }
+
+ if (of_property_read_u32(node, "sunxi,adc-interrupt-mask", &prop)) {
+ dev_err(&idev->dev, "Missing adc-interrupt-mask property in the DT.\n");
+ ret = -EINVAL;
+ goto error_ret;
+ }
+ st->inter_mask = prop;
+
+ return 0;
+
+error_ret:
+ return ret;
+}
+
+static int sunxi_adc_probe_pdata(struct sunxi_adc_state *st,
+ struct platform_device *pdev)
+{
+ struct sunxi_adc_data *pdata = pdev->dev.platform_data;
+
+ if (!pdata)
+ return -EINVAL;
+
+ st->vref_mv = pdata->vref;
+ st->channels_mask = pdata->channels_used;
+ st->num_channels = pdata->num_channels;
+ st->startup_time = pdata->startup_time;
+ st->inter_mask = pdata->inter_mask;
+ st->registers = pdata->registers;
+
+ return 0;
+}
+
+static const struct iio_info sunxi_adc_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = &sunxi_adc_read_raw,
+};
+
+static int sunxi_adc_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct iio_dev *idev;
+ struct sunxi_adc_state *st;
+ struct resource *res;
+ u32 reg;
+
+ idev = iio_device_alloc(sizeof(struct sunxi_adc_state));
+ if (idev == NULL) {
+ ret = -ENOMEM;
+ goto error_ret;
+ }
+
+ st = iio_priv(idev);
+
+ if (pdev->dev.of_node)
+ ret = sunxi_adc_probe_dt(st, pdev);
+ else
+ ret = sunxi_adc_probe_pdata(st, pdev);
+
+ if (ret) {
+ dev_err(&pdev->dev, "No platform data available.\n");
+ ret = -EINVAL;
+ goto error_free_device;
+ }
+
+ platform_set_drvdata(pdev, idev);
+
+ idev->dev.parent = &pdev->dev;
+ idev->name = dev_name(&pdev->dev);
+ idev->modes = INDIO_DIRECT_MODE;
+ idev->info = &sunxi_adc_info;
+
+ st->irq = platform_get_irq(pdev, 0);
+ if (st->irq < 0) {
+ dev_err(&pdev->dev, "No IRQ ID is designated\n");
+ ret = -ENODEV;
+ goto error_free_device;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ st->reg_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(st->reg_base)) {
+ ret = PTR_ERR(st->reg_base);
+ goto error_free_device;
+ }
+
+ /* Disable all IRQs */
+ sunxi_adc_writel(st, SUNXI_LRADC_INTC, 0x00);
+ ret = request_irq(st->irq,
+ sunxi_lradc_irq,
+ 0,
+ pdev->dev.driver->name,
+ idev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to allocate IRQ.\n");
+ goto error_free_irq;
+ }
+
+ if (!st->startup_time) {
+ dev_err(&pdev->dev, "No startup time available.\n");
+ ret = -EINVAL;
+ goto error_free_irq;
+ }
+ reg = SUNXI_LRADC_FST_DLY(st->startup_time);
+
+ if (st->sample_hold_en)
+ reg |= SUNXI_LRADC_HOLD_EN(st->sample_hold_en);
+ if (st->sample_rate)
+ reg |= SUNXI_LRADC_S_RATE(st->sample_rate);
+ sunxi_adc_writel(st, SUNXI_LRADC_CTRL, reg);
+
+ /* Setup the ADC channels available on the board */
+ ret = sunxi_adc_channel_init(idev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Couldn't initialize the channels.\n");
+ goto error_free_irq;
+ }
+
+ init_waitqueue_head(&st->wq_data_avail);
+ mutex_init(&st->lock);
+
+ ret = iio_device_register(idev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Couldn't register the device.\n");
+ goto error_free_irq;
+ }
+
+ return 0;
+
+error_free_irq:
+ free_irq(st->irq, idev);
+error_free_device:
+ iio_device_free(idev);
+error_ret:
+ return ret;
+}
+
+static int sunxi_adc_remove(struct platform_device *pdev)
+{
+ struct iio_dev *idev = platform_get_drvdata(pdev);
+ struct sunxi_adc_state *st = iio_priv(idev);
+
+ iio_device_unregister(idev);
+ free_irq(st->irq, idev);
+ iio_device_free(idev);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id sunxi_adc_dt_ids[] = {
+ {.compatible = "allwinner,sun4i-lradc"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, sunxi_adc_dt_ids);
+#endif
+
+static struct platform_driver sunxi_adc_driver = {
+ .probe = sunxi_adc_probe,
+ .remove = sunxi_adc_remove,
+ .driver = {
+ .name = "sunxi_lradc",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(sunxi_adc_dt_ids),
+ },
+};
+
+module_platform_driver(sunxi_adc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Sunxi A1X LRADC Driver");
+MODULE_AUTHOR("Vinicius Freire <vinic...@gmail.com>");
--
1.7.10.4

[vinic...@gmail.com]

From: Vinicius Freire <vinic...@gmail.com>

Adds entries for built-in LRADC Allwinner in A10 and A13.

Signed-off-by: Vinicius Freire <vinic...@gmail.com>
---

arch/arm/boot/dts/sun4i-a10.dtsi | 12 ++++++++++++
arch/arm/boot/dts/sun5i-a13.dtsi | 12 ++++++++++++
2 files changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index c32770a..91916a6 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -261,6 +261,18 @@
clocks = <&osc24M>;
};

+ lradc0: lradc@01c22800 {
+ compatible = "allwinner,sun4i-lradc";
+ reg = <0x01c22800 0x400>;
+ interrupts = <31>;
+ sunxi,adc-channels-used = <0x02>;
+ sunxi,adc-vref = <2000>;
+ sunxi,adc-num-channels = <2>;
+ sunxi,adc-sample-rate = <0x00>;
+ sunxi,adc-startup-time = <15>;
+ sunxi,adc-interrupt-mask = <0x101>;
+ };
+
wdt: watchdog@01c20c90 {
compatible = "allwinner,sun4i-wdt";
reg = <0x01c20c90 0x10>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index f6091dc..904635e 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -217,6 +217,18 @@
clocks = <&osc24M>;
};

+ lradc0: lradc@01c22800 {
+ compatible = "allwinner,sun4i-lradc";
+ reg = <0x01c22800 0x400>;
+ interrupts = <31>;
+ sunxi,adc-channels-used = <0x02>;
+ sunxi,adc-vref = <2000>;
+ sunxi,adc-num-channels = <2>;
+ sunxi,adc-sample-rate = <0x00>;
+ sunxi,adc-startup-time = <15>;
+ sunxi,adc-interrupt-mask = <0x101>;
+ };
+
wdt: watchdog@01c20c90 {
compatible = "allwinner,sun4i-wdt";
reg = <0x01c20c90 0x10>;
--
1.7.10.4

[vinic...@gmail.com]

This is a preliminary patch set for internal review in linux-sunxi mailing.

Vinifr

[vinic...@gmail.com]

From: Vinicius Freire <vinic...@gmail.com>

Adds entries for built-in LRADC Allwinner in A10 and A13.

Signed-off-by: Vinicius Freire <vinic...@gmail.com>
---

arch/arm/boot/dts/sun4i-a10.dtsi | 12 ++++++++++++
arch/arm/boot/dts/sun5i-a13.dtsi | 12 ++++++++++++
2 files changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi

index c32770a..b11ce9d 100644

--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -261,6 +261,18 @@
clocks = <&osc24M>;
};

+ lradc0: lradc@01c22800 {
+ compatible = "allwinner,sun4i-lradc";
+ reg = <0x01c22800 0x400>;
+ interrupts = <31>;

+ sunxi,adc-channels-used = <0x03>;

+ sunxi,adc-vref = <2000>;
+ sunxi,adc-num-channels = <2>;
+ sunxi,adc-sample-rate = <0x00>;
+ sunxi,adc-startup-time = <15>;
+ sunxi,adc-interrupt-mask = <0x101>;
+ };
+
wdt: watchdog@01c20c90 {
compatible = "allwinner,sun4i-wdt";
reg = <0x01c20c90 0x10>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi

index f6091dc..ac22fa6 100644

--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -217,6 +217,18 @@
clocks = <&osc24M>;
};

+ lradc0: lradc@01c22800 {
+ compatible = "allwinner,sun4i-lradc";
+ reg = <0x01c22800 0x400>;
+ interrupts = <31>;

+ sunxi,adc-channels-used = <0x03>;

[Maxime Ripard]

Hi Vinicius,

On Tue, Sep 03, 2013 at 11:23:31PM -0300, vinic...@gmail.com wrote:
> From: Vinicius Freire <vinic...@gmail.com>
>
> This patch adds support for the built-in LRADC in Allwinner A10 and
> A13 SoCs. It has DT-support and uses the new iio framework.

It's not that new, the first patches were merged in 2.6.32 :)

> Signed-off-by: Vinicius Freire <vinic...@gmail.com>
> ---
> drivers/iio/adc/Kconfig | 8 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/sunxi_lradc.c | 462 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 471 insertions(+)
> create mode 100644 drivers/iio/adc/sunxi_lradc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 93129ec..5ffb17e 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -103,6 +103,14 @@ config AT91_ADC
> help
> Say yes here to build support for Atmel AT91 ADC.
>
> +config SUNXI_LRADC
> + tristate "Allwinner A1X LRADC"
> + depends on ARCH_SUNXI
> + select SYSFS

Do you really need sysfs here? I don't see you using it later in your
driver (and the IIO config option should take care of depending on that)

I'd prefer to have the name of the register before the name of the bits.

That would give something like SUNXI_LRADC_CTRL_FST_DLY.

> +#define SUNXI_LRADC_CHAN_SEL(x) ((x & 0x02) << 22)

Are you sure the mask is right here? it's 0b10, which also means that
it's 1 << 23, and that if you use this macro for the channel 1, it will
return 0.

It doesn't look very right.

> +#define SUNXI_LRADC_TIME_SEL(x) ((x & 0x07) << 16)
> +#define SUNXI_LRADC_KEY_MODE(x) ((x & 0x02) << 12)

Ditto.

> +#define SUNXI_LRADC_LVLA_B(x) ((x & 0x0F) << 8)
> +#define SUNXI_LRADC_HOLD_EN(x) ((x) << 6)
> +#define SUNXI_LRADC_LVLB(x) ((x & 0x02) << 4)
> +#define SUNXI_LRADC_S_RATE(x) ((x & 0x02) << 2)
> +#define SUNXI_LRADC_EN(x) ((x) << 0)
> +
> +/* Bit definitions for LRADC_INTC */
> +#define SUNXI_LRADC1_KEYUP_IRQ(x) ((x) << 12)
> +#define SUNXI_LRADC1_ALR_HOLD_IRQ(x) ((x) << 11)
> +#define SUNXI_LRADC1_HOLD_IRQ(x) ((x) << 10)
> +#define SUNXI_LRADC1_KEY_IRQ(x) ((x) << 9)
> +#define SUNXI_LRADC1_DATA_IRQ(x) ((x) << 8)

Use BIT() here.

Also, why do you need to make it a macro ? you just need a mask here.

> +
> +#define SUNXI_LRADC0_KEYUP_IRQ(x) ((x) << 4)
> +#define SUNXI_LRADC0_ALR_HOLD_IRQ(x) ((x) << 3)
> +#define SUNXI_LRADC0_HOLD_IRQ(x) ((x) << 2)
> +#define SUNXI_LRADC0_KEYDOWN(x) ((x) << 1)
> +#define SUNXI_LRADC0_DATA_IRQ(x) ((x) << 0)

So basically, we have the same layout for IRQs repeated every 8 bits?

Why do you use a macro like

#define SUNXI_LRADC_KEYUP_IRQ(val) BIT((val * 8) + 4)

> +/* Bit definitions for LRADC_INTS */
> +/* Masks */
> +#define SUNXI_LRADC1_KEYUP_PEN 0x0C
> +#define SUNXI_LRADC1_ALR_HOLD_PEN 0x0B
> +#define SUNXI_LRADC1_HOLDKEY_PEN 0x0A
> +#define SUNXI_LRADC1_KEYDOWN_IRQ_PEN 0x09
> +#define SUNXI_LRADC1_DATA_IRQ_EN 0x08
> +
> +#define SUNXI_LRADC0_KEYUP_PEN 0x04
> +#define SUNXI_LRADC0_ALR_HOLD_PEN 0x03
> +#define SUNXI_LRADC0_HOLDKEY_PEN 0x02
> +#define SUNXI_LRADC0_KEYDOWN_IRQ_PEN 0x01
> +#define SUNXI_LRADC0_DATA_IRQ_EN 0x00

Ditto.

> +#define sunxi_adc_readl(info, reg) \
> + (readl(info->reg_base + reg))
> +#define sunxi_adc_writel(info, reg, val) \
> + (writel(val, info->reg_base + reg))

You'd better use an inline function here.

What's the difference between what you store in sunxi_adc_data and
sunxi_adc_state ? You seem to duplicate the information here.

> +
> +static irqreturn_t sunxi_lradc_irq(int irq, void *private)
> +{
> + struct iio_dev *idev = private;
> + struct sunxi_adc_state *st = iio_priv(idev);
> + u32 status = sunxi_adc_readl(st, st->registers->lradc_ints);

Why don't you just use the defines here?

The channel_mask stuff isn't really needed in that case, you can just
register the two channels.

What is this interrupt mask?

I think it's wrong here. val should be 0, and val2 should be what you
just computed.

pdata ?!

we don't use platform datas, and we never will.

You can use devm_request_irq here to trim down the cleanup path.

Thanks!

Do you have any work going on to register against the input layer and
use the lradc for the buttons?

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Maxime Ripard]

Hi,

What's the difference with the v2 you sent earlier?

In general, when you issue a new version, please make a changelog, and
resend the whole serie.

On Sat, Sep 07, 2013 at 03:17:56PM -0300, vinic...@gmail.com wrote:
> From: Vinicius Freire <vinic...@gmail.com>
>
> Adds entries for built-in LRADC Allwinner in A10 and A13.
>
> Signed-off-by: Vinicius Freire <vinic...@gmail.com>
> ---
> arch/arm/boot/dts/sun4i-a10.dtsi | 12 ++++++++++++
> arch/arm/boot/dts/sun5i-a13.dtsi | 12 ++++++++++++
> 2 files changed, 24 insertions(+)

You should document these bindings in Documentation/devicetree/bindings.

> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> index c32770a..b11ce9d 100644
> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> @@ -261,6 +261,18 @@
> clocks = <&osc24M>;
> };
>
> + lradc0: lradc@01c22800 {
> + compatible = "allwinner,sun4i-lradc";
> + reg = <0x01c22800 0x400>;
> + interrupts = <31>;
> + sunxi,adc-channels-used = <0x03>;
> + sunxi,adc-vref = <2000>;
> + sunxi,adc-num-channels = <2>;
> + sunxi,adc-sample-rate = <0x00>;
> + sunxi,adc-startup-time = <15>;
> + sunxi,adc-interrupt-mask = <0x101>;

Hmmm, that's not right.

Keep in mind that the device tree is here for the hardware description
only.

So, in that list, should be removed:
- sunxi,adc-channels-used
- sunxi,adc-num-channels
- sunxi,adc-interrupt-mask

Should be coming from another mechanism (possibly sysfs):
- sunxi,adc-sample-rate

And you could probably use a regulator for the adc-vref property.

Also, remember that DT properties should be using the pattern
<vendor>,<property> so it should be allwinner,adc-vref and not sunxi,...

Thanks!

[vinic...@gmail.com]

You are right!

>
>
> > +#define SUNXI_LRADC_LVLA_B(x) ((x & 0x0F) << 8)
>
> > +#define SUNXI_LRADC_HOLD_EN(x) ((x) << 6)
>
> > +#define SUNXI_LRADC_LVLB(x) ((x & 0x02) << 4)
>
> > +#define SUNXI_LRADC_S_RATE(x) ((x & 0x02) << 2)
>
> > +#define SUNXI_LRADC_EN(x) ((x) << 0)
>
> > +
>
> > +/* Bit definitions for LRADC_INTC */
>
> > +#define SUNXI_LRADC1_KEYUP_IRQ(x) ((x) << 12)
>
> > +#define SUNXI_LRADC1_ALR_HOLD_IRQ(x) ((x) << 11)
>
> > +#define SUNXI_LRADC1_HOLD_IRQ(x) ((x) << 10)
>
> > +#define SUNXI_LRADC1_KEY_IRQ(x) ((x) << 9)
>
> > +#define SUNXI_LRADC1_DATA_IRQ(x) ((x) << 8)
>
>
>
> Use BIT() here.
>
>

Ok.

But if the user wants to use only one channel?

It is the mask used in SUNXI_LRADC_INTC to define which interrupts will be enabled. SUNXI_LRADC_INTC = 0x101 = ADC1_DATA_IRQ_EN and ADC0_DATA_IRQ_EN.
I do not enabled interrupts button because it was not implemented.

Sorry, I did not understand it.

For now not. :)

[vinic...@gmail.com]

Em domingo, 8 de setembro de 2013 06h30min20s UTC-3, Maxime Ripard escreveu:
> Hi,
>
>
>
> What's the difference with the v2 you sent earlier?
>
>
>
> In general, when you issue a new version, please make a changelog, and
>
> resend the whole serie.
>

Hm, sorry. But I did not understand the purpose of these logs, although seeing them in several patches.

And considering that this patch would be sent to iio list, it would be [PATCH] or [PATCHv3] with log?

>
>
> On Sat, Sep 07, 2013 at 03:17:56PM -0300, vinic...@gmail.com wrote:
>
> > From: Vinicius Freire <vinic...@gmail.com>
>
> >
>
> > Adds entries for built-in LRADC Allwinner in A10 and A13.
>
> >
>
> > Signed-off-by: Vinicius Freire <vinic...@gmail.com>
>
> > ---
>
> > arch/arm/boot/dts/sun4i-a10.dtsi | 12 ++++++++++++
>
> > arch/arm/boot/dts/sun5i-a13.dtsi | 12 ++++++++++++
>
> > 2 files changed, 24 insertions(+)
>
>
>
> You should document these bindings in Documentation/devicetree/bindings.
>
>

Ok.

Oops! I'll fix it.

[Maxime Ripard]

Hi Vinicius,

Please keep in CC the person you're responding to, this mail almost went
unnoticed :)

On Sun, Sep 08, 2013 at 01:39:56PM -0700, vinic...@gmail.com wrote:
> Em domingo, 8 de setembro de 2013 06h17min47s UTC-3, Maxime Ripard escreveu:
> > > + for_each_set_bit(bit, &st->channels_mask, st->num_channels) {
> >
> > The channel_mask stuff isn't really needed in that case, you can just
> > register the two channels.
> >
> But if the user wants to use only one channel?

Well, he will just read the channel_raw file he cares about.

What's the point of restricting access to a few channels here?

> > > + sunxi_adc_writel(st, SUNXI_LRADC_INTC, st->inter_mask);
> > What is this interrupt mask?
> It is the mask used in SUNXI_LRADC_INTC to define which interrupts
> will be enabled.
> SUNXI_LRADC_INTC = 0x101 = ADC1_DATA_IRQ_EN and ADC0_DATA_IRQ_EN.
> I do not enabled interrupts button because it was not implemented.

My question was more about why is it configurable (and especially, why
is this configuration done in the DT)? if the user wants to read
channel0, enable channel 0 data irq, if he wants to read channel1,
enable channel1 data irq.

In which case would we want to have a different behaviour?

> > > + case IIO_CHAN_INFO_SCALE:
> > > + *val = (st->vref_mv * 1000) >> chan->scan_type.realbits;
> > > + *val2 = 0;
> > > + return IIO_VAL_INT_PLUS_MICRO;
> >
> > I think it's wrong here. val should be 0, and val2 should be what you
> > just computed.
> >
> Sorry, I did not understand it.

*val = 0;
*val2 = (st->vref_mv * 1000) >> chan->scan_type.realbits;

Thanks,

[Maxime Ripard]

On Sun, Sep 08, 2013 at 01:56:23PM -0700, vinic...@gmail.com wrote:
> Em domingo, 8 de setembro de 2013 06h30min20s UTC-3, Maxime Ripard escreveu:
> > Hi,
> >
> >
> >
> > What's the difference with the v2 you sent earlier?
> >
> >
> >
> > In general, when you issue a new version, please make a changelog, and
> >
> > resend the whole serie.
> >
>
> Hm, sorry. But I did not understand the purpose of these logs,
> although seeing them in several patches.

It's *very* useful for the one reviewing your patches, for several
reasons:
- It allows to spot easily the differences between versions.
Basically, I have not been able to spot the difference between your
v2 and v3, so it just adds confusion
- It allows to put more context around your patch. That way, we can
easily remember the comments we had, and see if those were fixed or
not.
- And since we can easily spot what has changed, we can decide if just
a quick review is needed, for the few rough edges that were
remaining, or if it needs a deeper review.

> And considering that this patch would be sent to iio list, it would be
> [PATCH] or [PATCHv3] with log?

You should send it at least to Jonathan Cameron (the iio maintainer) and
myself, and to the linux-arm-kernel, linux-iio and devicetree mailing
lists. But since you never posted these patches there, you should send
it as the first version.

Honestly, I don't really see the point of those "private" reviews on
linux-sunxi, especially in your case where your driver is in pretty good
shape already.

Whenever it's ready, send it, that's it.