[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Alexandre Belloni]

Document the bindings for the Allwinner LRADC.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree at vger.kernel.org

 .../devicetree/bindings/iio/adc/sun4i-lradc.txt       | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
new file mode 100644
index 000000000000..c75a6067b8a5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
@@ -0,0 +1,19 @@
+Allwinner sun4i Low Resolution ADC
+----------------------------------
+
+Required properties:
+ - compatible: "allwinner,sun4i-a10-lradc"
+ - reg: mmio address range of the chip
+ - interrupts: interrupt to which the chip is connected
+ - vref-supply: powersupply for the lradc reference voltage
+ - #io-channel-cells = <1>; As ADC has multiple outputs
+
+Example:
+
+	lradc: lradc at 01c22800 {
+		compatible = "allwinner,sun4i-a10-lradc";
+		reg = <0x01c22800 0x100>;
+		interrupts = <31>;
+		vref-supply = <&reg_vcc3v0>;
+		#io-channel-cells = <1>;
+	};
-- 
2.8.1

[PATCH 2/2] iio: adc: sun4i_lradc: new driver

[Alexandre Belloni]

Add an IIO driver for the Allwinner LRADC.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/iio/adc/Kconfig       |  10 ++
 drivers/iio/adc/Makefile      |   3 +-
 drivers/iio/adc/sun4i_lradc.c | 329 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 341 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/adc/sun4i_lradc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 25378c5882e2..55fb07bbe9a9 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -384,6 +384,16 @@ config ROCKCHIP_SARADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called rockchip_saradc.
 
+config SUN4I_LRADC
+	tristate "Allwinner LRADC driver"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	select IIO_TRIGGER
+	help
+	  Say yes here to build support for the LRADC found on Allwinner SoCs.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called sun4i_lradc.
+
 config TI_ADC081C
 	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 38638d46f972..a3b165af6bde 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -1,4 +1,4 @@
-#
+
 # Makefile for IIO ADC drivers
 #
 
@@ -37,6 +37,7 @@ obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
 obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
 obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
+obj-$(CONFIG_SUN4I_LRADC) += sun4i_lradc.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
diff --git a/drivers/iio/adc/sun4i_lradc.c b/drivers/iio/adc/sun4i_lradc.c
new file mode 100644
index 000000000000..9789dcc28621
--- /dev/null
+++ b/drivers/iio/adc/sun4i_lradc.c
@@ -0,0 +1,329 @@
+/*
+ * Driver for the LRADC present on the  Allwinner sun4i
+ *
+ * Copyright 2016 Free Electrons
+ *
+ * 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/bitops.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/interrupt.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/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/regulator/consumer.h>
+
+#define SUN4I_LRADC_CTRL		0x00
+#define SUN4I_LRADC_INTC		0x04
+#define SUN4I_LRADC_INTS		0x08
+#define SUN4I_LRADC_DATA0		0x0c
+#define SUN4I_LRADC_DATA1		0x10
+
+/* LRADC_CTRL bits */
+#define FIRST_CONVERT_DLY(x)	((x) << 24) /* 8 bits */
+#define CHAN_SELECT(x)		((x) << 22) /* 2 bits */
+#define CONTINUE_TIME_SEL(x)	((x) << 16) /* 4 bits */
+#define KEY_MODE_SEL(x)		((x) << 12) /* 2 bits */
+#define LEVELA_B_CNT(x)		((x) << 8)  /* 4 bits */
+#define LRADC_HOLD_EN		BIT(6)
+#define LEVELB_VOL(x)		((x) << 4)  /* 2 bits */
+#define LRADC_SAMPLE_RATE(x)	((x) << 2)  /* 2 bits */
+#define LRADC_EN		BIT(0)
+
+/* LRADC_INTC and LRADC_INTS bits */
+#define CHAN1_KEYUP_IRQ		BIT(12)
+#define CHAN1_ALRDY_HOLD_IRQ	BIT(11)
+#define CHAN1_HOLD_IRQ		BIT(10)
+#define	CHAN1_KEYDOWN_IRQ	BIT(9)
+#define CHAN1_DATA_IRQ		BIT(8)
+#define CHAN0_KEYUP_IRQ		BIT(4)
+#define CHAN0_ALRDY_HOLD_IRQ	BIT(3)
+#define CHAN0_HOLD_IRQ		BIT(2)
+#define	CHAN0_KEYDOWN_IRQ	BIT(1)
+#define CHAN0_DATA_IRQ		BIT(0)
+
+#define NUM_CHANS		2
+#define NUM_TRIGGERS		4
+
+struct sun4i_lradc_state {
+	void __iomem *base;
+	struct regulator *vref_supply;
+	u32 vref_mv;
+	struct completion data_ok[NUM_CHANS];
+	u32 last_event;
+	spinlock_t lock;
+};
+
+#define SUN4I_LRADC_CHANNEL(chan) {					\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = (chan),					\
+	.scan_index = (chan),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ)	\
+}
+
+static const struct iio_chan_spec sun4i_lradc_chan_array[] = {
+	SUN4I_LRADC_CHANNEL(0),
+	SUN4I_LRADC_CHANNEL(1),
+};
+
+static const struct {
+	int val;
+	int val2;
+} sun4i_lradc_sample_freq_avail[] = {
+	{250, 0},
+	{125, 0},
+	{62, 500000},
+	{32, 250000},
+};
+
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("32.25 62.5 125 250");
+
+static struct attribute *sun4i_lradc_attributes[] = {
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group sun4i_lradc_attribute_group = {
+	.attrs = sun4i_lradc_attributes,
+};
+
+static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct sun4i_lradc_state *st = iio_priv(indio_dev);
+	u32 ints, intc;
+
+	spin_lock(&st->lock);
+
+	ints = readl(st->base + SUN4I_LRADC_INTS);
+	intc = readl(st->base + SUN4I_LRADC_INTC);
+
+	if (ints & CHAN0_DATA_IRQ)
+		complete_all(&st->data_ok[0]);
+
+	if (ints & CHAN1_DATA_IRQ)
+		complete_all(&st->data_ok[1]);
+
+	st->last_event = ints & (CHAN1_KEYUP_IRQ | CHAN1_KEYDOWN_IRQ |
+				 CHAN0_KEYUP_IRQ | CHAN0_KEYDOWN_IRQ);
+
+	intc &= ~ints;
+	writel(intc, st->base + SUN4I_LRADC_INTC);
+	writel(ints, st->base + SUN4I_LRADC_INTS);
+
+	spin_unlock(&st->lock);
+
+	return IRQ_HANDLED;
+}
+
+static int sun4i_lradc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct sun4i_lradc_state *st = iio_priv(indio_dev);
+	int ret, tmp, idx;
+	unsigned long flags;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		reinit_completion(&st->data_ok[chan->channel]);
+		spin_lock_irqsave(&st->lock, flags);
+		tmp = readl(st->base + SUN4I_LRADC_INTC);
+
+		if (chan->channel)
+			tmp |= CHAN1_DATA_IRQ;
+		else
+			tmp |= CHAN0_DATA_IRQ;
+
+		writel(tmp, st->base + SUN4I_LRADC_INTC);
+		spin_unlock_irqrestore(&st->lock, flags);
+
+		ret = wait_for_completion_interruptible_timeout(
+			&st->data_ok[chan->channel],
+			msecs_to_jiffies(1000));
+		if (ret == 0)
+			return -ETIMEDOUT;
+
+		if (chan->channel)
+			*val = readl(st->base + SUN4I_LRADC_DATA1);
+		else
+			*val = readl(st->base + SUN4I_LRADC_DATA0);
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_mv;
+		*val2 = 6;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		tmp = readl(st->base + SUN4I_LRADC_CTRL);
+		idx = (tmp >> 2) & 0x3;
+		*val = sun4i_lradc_sample_freq_avail[idx].val;
+		*val2 = sun4i_lradc_sample_freq_avail[idx].val2;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int sun4i_lradc_write_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int val, int val2, long mask)
+{
+	struct sun4i_lradc_state *st = iio_priv(indio_dev);
+	u32 ctrl;
+	int i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		for (i = 0; i < ARRAY_SIZE(sun4i_lradc_sample_freq_avail); i++)
+			if (sun4i_lradc_sample_freq_avail[i].val == val &&
+			    sun4i_lradc_sample_freq_avail[i].val2 == val2)
+				break;
+		if (i == ARRAY_SIZE(sun4i_lradc_sample_freq_avail))
+			return -EINVAL;
+
+		ctrl = readl(st->base + SUN4I_LRADC_CTRL);
+		ctrl &= ~LRADC_SAMPLE_RATE(0x3);
+		writel(ctrl | LRADC_SAMPLE_RATE(i),
+		       st->base + SUN4I_LRADC_CTRL);
+
+		return 0;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int sun4i_lradc_write_raw_get_fmt(struct iio_dev *indio_dev,
+					 struct iio_chan_spec const *chan,
+					 long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		break;
+	}
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static const struct iio_info sun4i_lradc_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = sun4i_lradc_read_raw,
+	.write_raw = sun4i_lradc_write_raw,
+	.write_raw_get_fmt = sun4i_lradc_write_raw_get_fmt,
+	.attrs = &sun4i_lradc_attribute_group,
+};
+
+static int sun4i_lradc_probe(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev;
+	struct device *dev = &pdev->dev;
+	struct sun4i_lradc_state *st;
+	int err;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	indio_dev->dev.parent = dev;
+	indio_dev->name = dev_name(dev);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &sun4i_lradc_info;
+
+	st->vref_supply = devm_regulator_get(dev, "vref");
+	if (IS_ERR(st->vref_supply))
+		return PTR_ERR(st->vref_supply);
+
+	st->base = devm_ioremap_resource(dev,
+			platform_get_resource(pdev, IORESOURCE_MEM, 0));
+	if (IS_ERR(st->base))
+		return PTR_ERR(st->base);
+
+	writel(0, st->base + SUN4I_LRADC_INTC);
+	writel(0xffffffff, st->base + SUN4I_LRADC_INTS);
+
+	err = devm_request_irq(dev, platform_get_irq(pdev, 0),
+			       sun4i_lradc_irq, 0,
+			       "sun4i-a10-lradc", indio_dev);
+	if (err)
+		return err;
+
+	/* Setup the ADC channels available on the board */
+	indio_dev->num_channels = ARRAY_SIZE(sun4i_lradc_chan_array);
+	indio_dev->channels = sun4i_lradc_chan_array;
+
+	err = regulator_enable(st->vref_supply);
+	if (err)
+		return err;
+
+	err = devm_iio_device_register(dev, indio_dev);
+	if (err < 0) {
+		dev_err(dev, "Couldn't register the device.\n");
+		return err;
+	}
+
+	/* lradc Vref internally is divided by 2/3 */
+	st->vref_mv = regulator_get_voltage(st->vref_supply) * 2 / 3000;
+
+	init_completion(&st->data_ok[0]);
+	init_completion(&st->data_ok[1]);
+	spin_lock_init(&st->lock);
+
+	/* Continuous mode on both channels */
+	writel(CHAN_SELECT(0x3) | KEY_MODE_SEL(0x2) | LRADC_SAMPLE_RATE(0x00) |
+	       LRADC_EN, st->base + SUN4I_LRADC_CTRL);
+
+	return 0;
+}
+
+static const struct of_device_id sun4i_lradc_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-lradc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
+
+static struct platform_driver sun4i_lradc_driver = {
+	.probe	= sun4i_lradc_probe,
+	.driver = {
+		.name	= "sun4i-a10-lradc",
+		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
+	},
+};
+
+module_platform_driver(sun4i_lradc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Allwinner sun4i low resolution ADC driver");
+MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
-- 
2.8.1

[PATCH 2/2] iio: adc: sun4i_lradc: new driver

[Jonathan Cameron]

On 01/07/16 22:00, Alexandre Belloni wrote:
> Add an IIO driver for the Allwinner LRADC.
To avoid idiots (i.e. me) confusing this with the touch screen ADC
could you expand a little on the description in future patches.

A few general bits and bobs inline.

Jonathan
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  drivers/iio/adc/Kconfig       |  10 ++
>  drivers/iio/adc/Makefile      |   3 +-
>  drivers/iio/adc/sun4i_lradc.c | 329 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 341 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/iio/adc/sun4i_lradc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5882e2..55fb07bbe9a9 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,16 @@ config ROCKCHIP_SARADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called rockchip_saradc.
>  
> +config SUN4I_LRADC
> +	tristate "Allwinner LRADC driver"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	select IIO_TRIGGER
> +	help
> +	  Say yes here to build support for the LRADC found on Allwinner SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called sun4i_lradc.
> +
>  config TI_ADC081C
>  	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d46f972..a3b165af6bde 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -1,4 +1,4 @@
> -#
> +
>  # Makefile for IIO ADC drivers
>  #
>  
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>  obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_SUN4I_LRADC) += sun4i_lradc.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> diff --git a/drivers/iio/adc/sun4i_lradc.c b/drivers/iio/adc/sun4i_lradc.c
> new file mode 100644
> index 000000000000..9789dcc28621
> --- /dev/null
> +++ b/drivers/iio/adc/sun4i_lradc.c
> @@ -0,0 +1,329 @@
> +/*
> + * Driver for the LRADC present on the  Allwinner sun4i
> + *
> + * Copyright 2016 Free Electrons
> + *
> + * 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/bitops.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.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/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define SUN4I_LRADC_CTRL		0x00
> +#define SUN4I_LRADC_INTC		0x04
> +#define SUN4I_LRADC_INTS		0x08
> +#define SUN4I_LRADC_DATA0		0x0c
> +#define SUN4I_LRADC_DATA1		0x10
> +
Please prefix all defines (far too likely some of these might in future
turn up in a header this driver is including).
> +/* LRADC_CTRL bits */
> +#define FIRST_CONVERT_DLY(x)	((x) << 24) /* 8 bits */
> +#define CHAN_SELECT(x)		((x) << 22) /* 2 bits */
> +#define CONTINUE_TIME_SEL(x)	((x) << 16) /* 4 bits */
> +#define KEY_MODE_SEL(x)		((x) << 12) /* 2 bits */
> +#define LEVELA_B_CNT(x)		((x) << 8)  /* 4 bits */
> +#define LRADC_HOLD_EN		BIT(6)
> +#define LEVELB_VOL(x)		((x) << 4)  /* 2 bits */
> +#define LRADC_SAMPLE_RATE(x)	((x) << 2)  /* 2 bits */
> +#define LRADC_EN		BIT(0)
> +
> +/* LRADC_INTC and LRADC_INTS bits */
> +#define CHAN1_KEYUP_IRQ		BIT(12)
> +#define CHAN1_ALRDY_HOLD_IRQ	BIT(11)
> +#define CHAN1_HOLD_IRQ		BIT(10)
> +#define	CHAN1_KEYDOWN_IRQ	BIT(9)
> +#define CHAN1_DATA_IRQ		BIT(8)
> +#define CHAN0_KEYUP_IRQ		BIT(4)
> +#define CHAN0_ALRDY_HOLD_IRQ	BIT(3)
> +#define CHAN0_HOLD_IRQ		BIT(2)
> +#define	CHAN0_KEYDOWN_IRQ	BIT(1)
> +#define CHAN0_DATA_IRQ		BIT(0)
> +
> +#define NUM_CHANS		2
> +#define NUM_TRIGGERS		4
? Interesting, but not present here.
> +
> +struct sun4i_lradc_state {
> +	void __iomem *base;
> +	struct regulator *vref_supply;
> +	u32 vref_mv;
> +	struct completion data_ok[NUM_CHANS];
> +	u32 last_event;
> +	spinlock_t lock;
> +};
> +
> +#define SUN4I_LRADC_CHANNEL(chan) {					\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = (chan),					\
> +	.scan_index = (chan),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ)	\
> +}
> +
> +static const struct iio_chan_spec sun4i_lradc_chan_array[] = {
> +	SUN4I_LRADC_CHANNEL(0),
> +	SUN4I_LRADC_CHANNEL(1),
> +};
> +
> +static const struct {
> +	int val;
> +	int val2;
> +} sun4i_lradc_sample_freq_avail[] = {
> +	{250, 0},
> +	{125, 0},
> +	{62, 500000},
> +	{32, 250000},
> +};
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("32.25 62.5 125 250");
> +
> +static struct attribute *sun4i_lradc_attributes[] = {
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group sun4i_lradc_attribute_group = {
> +	.attrs = sun4i_lradc_attributes,
> +};
> +
> +static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> +{
> +	struct iio_dev *indio_dev = dev_id;
> +	struct sun4i_lradc_state *st = iio_priv(indio_dev);
> +	u32 ints, intc;
> +
> +	spin_lock(&st->lock);
> +
> +	ints = readl(st->base + SUN4I_LRADC_INTS);
> +	intc = readl(st->base + SUN4I_LRADC_INTC);
> +
> +	if (ints & CHAN0_DATA_IRQ)
> +		complete_all(&st->data_ok[0]);
> +
> +	if (ints & CHAN1_DATA_IRQ)
> +		complete_all(&st->data_ok[1]);
> +
> +	st->last_event = ints & (CHAN1_KEYUP_IRQ | CHAN1_KEYDOWN_IRQ |
> +				 CHAN0_KEYUP_IRQ | CHAN0_KEYDOWN_IRQ);
> +
> +	intc &= ~ints;
> +	writel(intc, st->base + SUN4I_LRADC_INTC);
> +	writel(ints, st->base + SUN4I_LRADC_INTS);
> +
> +	spin_unlock(&st->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sun4i_lradc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct sun4i_lradc_state *st = iio_priv(indio_dev);
> +	int ret, tmp, idx;
> +	unsigned long flags;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		reinit_completion(&st->data_ok[chan->channel]);
> +		spin_lock_irqsave(&st->lock, flags);
> +		tmp = readl(st->base + SUN4I_LRADC_INTC);
> +
> +		if (chan->channel)
> +			tmp |= CHAN1_DATA_IRQ;
> +		else
> +			tmp |= CHAN0_DATA_IRQ;
> +
> +		writel(tmp, st->base + SUN4I_LRADC_INTC);
> +		spin_unlock_irqrestore(&st->lock, flags);
> +
> +		ret = wait_for_completion_interruptible_timeout(
> +			&st->data_ok[chan->channel],
> +			msecs_to_jiffies(1000));
> +		if (ret == 0)
> +			return -ETIMEDOUT;
> +
> +		if (chan->channel)
> +			*val = readl(st->base + SUN4I_LRADC_DATA1);
> +		else
> +			*val = readl(st->base + SUN4I_LRADC_DATA0);
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mv;
> +		*val2 = 6;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		tmp = readl(st->base + SUN4I_LRADC_CTRL);
> +		idx = (tmp >> 2) & 0x3;
> +		*val = sun4i_lradc_sample_freq_avail[idx].val;
> +		*val2 = sun4i_lradc_sample_freq_avail[idx].val2;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int sun4i_lradc_write_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 int val, int val2, long mask)
> +{
> +	struct sun4i_lradc_state *st = iio_priv(indio_dev);
> +	u32 ctrl;
> +	int i;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		for (i = 0; i < ARRAY_SIZE(sun4i_lradc_sample_freq_avail); i++)
> +			if (sun4i_lradc_sample_freq_avail[i].val == val &&
> +			    sun4i_lradc_sample_freq_avail[i].val2 == val2)
> +				break;
> +		if (i == ARRAY_SIZE(sun4i_lradc_sample_freq_avail))
> +			return -EINVAL;
> +
> +		ctrl = readl(st->base + SUN4I_LRADC_CTRL);
> +		ctrl &= ~LRADC_SAMPLE_RATE(0x3);
> +		writel(ctrl | LRADC_SAMPLE_RATE(i),
> +		       st->base + SUN4I_LRADC_CTRL);
> +
> +		return 0;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int sun4i_lradc_write_raw_get_fmt(struct iio_dev *indio_dev,
> +					 struct iio_chan_spec const *chan,
> +					 long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		break;
> +	}
> +	return IIO_VAL_INT_PLUS_NANO;
Umm. You don't write anything other than samp_freq and the above
is the default so I don't think you need this function at all.
> +}
> +
> +static const struct iio_info sun4i_lradc_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = sun4i_lradc_read_raw,
> +	.write_raw = sun4i_lradc_write_raw,
> +	.write_raw_get_fmt = sun4i_lradc_write_raw_get_fmt,
> +	.attrs = &sun4i_lradc_attribute_group,
> +};
> +
> +static int sun4i_lradc_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &pdev->dev;
> +	struct sun4i_lradc_state *st;
> +	int err;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->dev.parent = dev;
Probably also want to point the of_node to the right place to allow
in kernel users to work if people want them.

> +	indio_dev->name = dev_name(dev);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &sun4i_lradc_info;
> +
> +	st->vref_supply = devm_regulator_get(dev, "vref");
> +	if (IS_ERR(st->vref_supply))
> +		return PTR_ERR(st->vref_supply);
> +
> +	st->base = devm_ioremap_resource(dev,
> +			platform_get_resource(pdev, IORESOURCE_MEM, 0));
> +	if (IS_ERR(st->base))
> +		return PTR_ERR(st->base);
> +
Perhaps a trivial comment on what this is doing... Presumably clearing all
inerrupts then turning them on?  If so you probably want to do that after
you have claimed the irqs.
> +	writel(0, st->base + SUN4I_LRADC_INTC);
> +	writel(0xffffffff, st->base + SUN4I_LRADC_INTS);
> +
> +	err = devm_request_irq(dev, platform_get_irq(pdev, 0),
> +			       sun4i_lradc_irq, 0,
> +			       "sun4i-a10-lradc", indio_dev);
> +	if (err)
> +		return err;
> +
> +	/* Setup the ADC channels available on the board */
> +	indio_dev->num_channels = ARRAY_SIZE(sun4i_lradc_chan_array);
> +	indio_dev->channels = sun4i_lradc_chan_array;
> +
> +	err = regulator_enable(st->vref_supply);
> +	if (err)
> +		return err;
Ever disable it again?  You need a remove function.  Note, be careful
with using devm_ form of iio_device_register when you add the remove
as it'll mean you turn the power off (possibly) before you remove the
interfaces to talk to the chip.
> +
> +	err = devm_iio_device_register(dev, indio_dev);
> +	if (err < 0) {
> +		dev_err(dev, "Couldn't register the device.\n");
> +		return err;
> +	}
At this point all userspace interfaces and in kernel interfaces are
exposed.  You really want the device to be ready to go by here.
Doesn't look like it is too me!
> +
> +	/* lradc Vref internally is divided by 2/3 */
> +	st->vref_mv = regulator_get_voltage(st->vref_supply) * 2 / 3000;
> +
> +	init_completion(&st->data_ok[0]);
> +	init_completion(&st->data_ok[1]);
> +	spin_lock_init(&st->lock);
> +
> +	/* Continuous mode on both channels */
> +	writel(CHAN_SELECT(0x3) | KEY_MODE_SEL(0x2) | LRADC_SAMPLE_RATE(0x00) |
> +	       LRADC_EN, st->base + SUN4I_LRADC_CTRL);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sun4i_lradc_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-a10-lradc", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
> +
> +static struct platform_driver sun4i_lradc_driver = {
> +	.probe	= sun4i_lradc_probe,
> +	.driver = {
> +		.name	= "sun4i-a10-lradc",
> +		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
> +	},
> +};
> +
> +module_platform_driver(sun4i_lradc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Allwinner sun4i low resolution ADC driver");
> +MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
> 

[PATCH 2/2] iio: adc: sun4i_lradc: new driver

[Alexandre Belloni]

On 03/07/2016 at 13:11:33 +0100, Jonathan Cameron wrote :
> On 01/07/16 22:00, Alexandre Belloni wrote:
> > Add an IIO driver for the Allwinner LRADC.
> To avoid idiots (i.e. me) confusing this with the touch screen ADC
> could you expand a little on the description in future patches.
> 

Sure, one is low resolution, the other one has a better resolution. I
pretty sure that is not completely helpful  :)

> > +/* LRADC_CTRL bits */
> > +#define FIRST_CONVERT_DLY(x)	((x) << 24) /* 8 bits */
> > +#define CHAN_SELECT(x)		((x) << 22) /* 2 bits */
> > +#define CONTINUE_TIME_SEL(x)	((x) << 16) /* 4 bits */
> > +#define KEY_MODE_SEL(x)		((x) << 12) /* 2 bits */
> > +#define LEVELA_B_CNT(x)		((x) << 8)  /* 4 bits */
> > +#define LRADC_HOLD_EN		BIT(6)
> > +#define LEVELB_VOL(x)		((x) << 4)  /* 2 bits */
> > +#define LRADC_SAMPLE_RATE(x)	((x) << 2)  /* 2 bits */
> > +#define LRADC_EN		BIT(0)
> > +
> > +/* LRADC_INTC and LRADC_INTS bits */
> > +#define CHAN1_KEYUP_IRQ		BIT(12)
> > +#define CHAN1_ALRDY_HOLD_IRQ	BIT(11)
> > +#define CHAN1_HOLD_IRQ		BIT(10)
> > +#define	CHAN1_KEYDOWN_IRQ	BIT(9)
> > +#define CHAN1_DATA_IRQ		BIT(8)
> > +#define CHAN0_KEYUP_IRQ		BIT(4)
> > +#define CHAN0_ALRDY_HOLD_IRQ	BIT(3)
> > +#define CHAN0_HOLD_IRQ		BIT(2)
> > +#define	CHAN0_KEYDOWN_IRQ	BIT(1)
> > +#define CHAN0_DATA_IRQ		BIT(0)
> > +
> > +#define NUM_CHANS		2
> > +#define NUM_TRIGGERS		4
> ? Interesting, but not present here.

Well, I toyed with trigger events and I forgot to remove that define.

> > +static int sun4i_lradc_write_raw_get_fmt(struct iio_dev *indio_dev,
> > +					 struct iio_chan_spec const *chan,
> > +					 long mask)
> > +{
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +
> > +	default:
> > +		break;
> > +	}
> > +	return IIO_VAL_INT_PLUS_NANO;
> Umm. You don't write anything other than samp_freq and the above
> is the default so I don't think you need this function at all.

I'll try again but I was under the impression that this was needed.
Maybe I got something else wrong when I first tested.

> > +	indio_dev->name = dev_name(dev);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &sun4i_lradc_info;
> > +
> > +	st->vref_supply = devm_regulator_get(dev, "vref");
> > +	if (IS_ERR(st->vref_supply))
> > +		return PTR_ERR(st->vref_supply);
> > +
> > +	st->base = devm_ioremap_resource(dev,
> > +			platform_get_resource(pdev, IORESOURCE_MEM, 0));
> > +	if (IS_ERR(st->base))
> > +		return PTR_ERR(st->base);
> > +
> Perhaps a trivial comment on what this is doing... Presumably clearing all
> inerrupts then turning them on?  If so you probably want to do that after
> you have claimed the irqs.

I'll add a comment. It is in fact disabling the interrupts and then
clearing them. It is not completely necessary until I add tirgger
support.

> > +	writel(0, st->base + SUN4I_LRADC_INTC);
> > +	writel(0xffffffff, st->base + SUN4I_LRADC_INTS);
> > +
> > +	err = devm_request_irq(dev, platform_get_irq(pdev, 0),
> > +			       sun4i_lradc_irq, 0,
> > +			       "sun4i-a10-lradc", indio_dev);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Setup the ADC channels available on the board */
> > +	indio_dev->num_channels = ARRAY_SIZE(sun4i_lradc_chan_array);
> > +	indio_dev->channels = sun4i_lradc_chan_array;
> > +
> > +	err = regulator_enable(st->vref_supply);
> > +	if (err)
> > +		return err;
> Ever disable it again?  You need a remove function.  Note, be careful
> with using devm_ form of iio_device_register when you add the remove
> as it'll mean you turn the power off (possibly) before you remove the
> interfaces to talk to the chip.

Sure, that shouldn't be an issue.

> > +
> > +	err = devm_iio_device_register(dev, indio_dev);
> > +	if (err < 0) {
> > +		dev_err(dev, "Couldn't register the device.\n");
> > +		return err;
> > +	}
> At this point all userspace interfaces and in kernel interfaces are
> exposed.  You really want the device to be ready to go by here.
> Doesn't look like it is too me!

Right, I'll move that at the end of the probe function.

> > +
> > +	/* lradc Vref internally is divided by 2/3 */
> > +	st->vref_mv = regulator_get_voltage(st->vref_supply) * 2 / 3000;
> > +
> > +	init_completion(&st->data_ok[0]);
> > +	init_completion(&st->data_ok[1]);
> > +	spin_lock_init(&st->lock);
> > +
> > +	/* Continuous mode on both channels */
> > +	writel(CHAN_SELECT(0x3) | KEY_MODE_SEL(0x2) | LRADC_SAMPLE_RATE(0x00) |
> > +	       LRADC_EN, st->base + SUN4I_LRADC_CTRL);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id sun4i_lradc_of_match[] = {
> > +	{ .compatible = "allwinner,sun4i-a10-lradc", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sun4i_lradc_of_match);
> > +
> > +static struct platform_driver sun4i_lradc_driver = {
> > +	.probe	= sun4i_lradc_probe,
> > +	.driver = {
> > +		.name	= "sun4i-a10-lradc",
> > +		.of_match_table = of_match_ptr(sun4i_lradc_of_match),
> > +	},
> > +};
> > +
> > +module_platform_driver(sun4i_lradc_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Allwinner sun4i low resolution ADC driver");
> > +MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
> > 
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Chen-Yu Tsai]

Hi,

On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> Document the bindings for the Allwinner LRADC.

We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
block.

Any plans to reconcile the different bindings?

Thanks
ChenYu

>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree at vger.kernel.org
>
>  .../devicetree/bindings/iio/adc/sun4i-lradc.txt       | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
> new file mode 100644
> index 000000000000..c75a6067b8a5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
> @@ -0,0 +1,19 @@
> +Allwinner sun4i Low Resolution ADC
> +----------------------------------
> +
> +Required properties:
> + - compatible: "allwinner,sun4i-a10-lradc"
> + - reg: mmio address range of the chip
> + - interrupts: interrupt to which the chip is connected
> + - vref-supply: powersupply for the lradc reference voltage
> + - #io-channel-cells = <1>; As ADC has multiple outputs
> +
> +Example:
> +
> +       lradc: lradc at 01c22800 {
> +               compatible = "allwinner,sun4i-a10-lradc";
> +               reg = <0x01c22800 0x100>;
> +               interrupts = <31>;
> +               vref-supply = <&reg_vcc3v0>;
> +               #io-channel-cells = <1>;
> +       };
> --
> 2.8.1
>

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Hans de Goede]

Hi,

On 02-07-16 11:12, Chen-Yu Tsai wrote:
> Hi,
>
> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
>> Document the bindings for the Allwinner LRADC.
>
> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> block.

Right, this block is used on many tablets and some dev boards to
provide buttons (as in the hid type) and the block is designed for
this purpose, giving an irq when the adc level crosses a certain
threshold.

Sure it can be used in a more generic way, but that is not its
primary goal.

So any generic purpose adc driver must not break the current
use-case (which is already used in mainline kernel dts files
in plenty of cases).

I believe that the best way to deal with this is to add an
"allwinner,general-purpose-mode" flag to the existing binding
(as well as document general purpose mode in the existing
binding rather then in a new binding doc).

That seems to be the right thing to do purely looking at this
from a dt binding pov.

For the implementation of this we can simpy have 2 drivers,
then both drivers can check the flag and if present return
-ENODEV from the existing input driver and likewise if not
present return -ENODEV from the iio driver.

We may actually use a similar solution for the touchscreen
controller which can also be alternatively used as a generic
purpose adc.

Regards,

Hans



> Any plans to reconcile the different bindings?
>
> Thanks
> ChenYu
>
>>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> ---
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree at vger.kernel.org
>>
>>  .../devicetree/bindings/iio/adc/sun4i-lradc.txt       | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
>> new file mode 100644
>> index 000000000000..c75a6067b8a5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/sun4i-lradc.txt
>> @@ -0,0 +1,19 @@
>> +Allwinner sun4i Low Resolution ADC
>> +----------------------------------
>> +
>> +Required properties:
>> + - compatible: "allwinner,sun4i-a10-lradc"
>> + - reg: mmio address range of the chip
>> + - interrupts: interrupt to which the chip is connected
>> + - vref-supply: powersupply for the lradc reference voltage
>> + - #io-channel-cells = <1>; As ADC has multiple outputs
>> +
>> +Example:
>> +
>> +       lradc: lradc at 01c22800 {
>> +               compatible = "allwinner,sun4i-a10-lradc";
>> +               reg = <0x01c22800 0x100>;
>> +               interrupts = <31>;
>> +               vref-supply = <&reg_vcc3v0>;
>> +               #io-channel-cells = <1>;
>> +       };
>> --
>> 2.8.1
>>

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Maxime Ripard]

On Sat, Jul 02, 2016 at 11:32:07AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 02-07-16 11:12, Chen-Yu Tsai wrote:
> >Hi,
> >
> >On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> ><alexandre.belloni@free-electrons.com> wrote:
> >>Document the bindings for the Allwinner LRADC.
> >
> >We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> >and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> >block.
> 
> Right, this block is used on many tablets and some dev boards to
> provide buttons (as in the hid type) and the block is designed for
> this purpose, giving an irq when the adc level crosses a certain
> threshold.
> 
> Sure it can be used in a more generic way, but that is not its
> primary goal.

We've always had a different view on this, but it's a detail :)

> So any generic purpose adc driver must not break the current
> use-case (which is already used in mainline kernel dts files
> in plenty of cases).

Yep.

> I believe that the best way to deal with this is to add an
> "allwinner,general-purpose-mode" flag to the existing binding
> (as well as document general purpose mode in the existing
> binding rather then in a new binding doc).
> 
> That seems to be the right thing to do purely looking at this
> from a dt binding pov.

There's a way simpler solution: if there's no child nodes, it's meant
to be used as an ADC, otherwise, as input.

The logic will have to be a bit more complex than that, since there's
two channels, and you could only require one for the buttons, leaving
the other one available as an ADC.

But that doesn't require any new property.

> For the implementation of this we can simpy have 2 drivers,
> then both drivers can check the flag and if present return
> -ENODEV from the existing input driver and likewise if not
> present return -ENODEV from the iio driver.
> 
> We may actually use a similar solution for the touchscreen
> controller which can also be alternatively used as a generic
> purpose adc.

There's no need to keep both drivers as long as we keep the features
and bindings.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160702/a68408f7/attachment.sig>

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Hans de Goede]

Hi,

On 02-07-16 13:02, Maxime Ripard wrote:
> On Sat, Jul 02, 2016 at 11:32:07AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 02-07-16 11:12, Chen-Yu Tsai wrote:
>>> Hi,
>>>
>>> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
>>> <alexandre.belloni@free-electrons.com> wrote:
>>>> Document the bindings for the Allwinner LRADC.
>>>
>>> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
>>> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
>>> block.
>>
>> Right, this block is used on many tablets and some dev boards to
>> provide buttons (as in the hid type) and the block is designed for
>> this purpose, giving an irq when the adc level crosses a certain
>> threshold.
>>
>> Sure it can be used in a more generic way, but that is not its
>> primary goal.
>
> We've always had a different view on this, but it's a detail :)
>
>> So any generic purpose adc driver must not break the current
>> use-case (which is already used in mainline kernel dts files
>> in plenty of cases).
>
> Yep.
>
>> I believe that the best way to deal with this is to add an
>> "allwinner,general-purpose-mode" flag to the existing binding
>> (as well as document general purpose mode in the existing
>> binding rather then in a new binding doc).
>>
>> That seems to be the right thing to do purely looking at this
>> from a dt binding pov.
>
> There's a way simpler solution: if there's no child nodes, it's meant
> to be used as an ADC, otherwise, as input.
>
> The logic will have to be a bit more complex than that, since there's
> two channels, and you could only require one for the buttons, leaving
> the other one available as an ADC.
>
> But that doesn't require any new property.

True, if there are no button nodes, then go general-purpose will
work too.

>> For the implementation of this we can simpy have 2 drivers,
>> then both drivers can check the flag and if present return
>> -ENODEV from the existing input driver and likewise if not
>> present return -ENODEV from the iio driver.
>>
>> We may actually use a similar solution for the touchscreen
>> controller which can also be alternatively used as a generic
>> purpose adc.
>
> There's no need to keep both drivers as long as we keep the features
> and bindings.

That is also true.

Regards,

Hans

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Alexandre Belloni]

On 02/07/2016 at 13:45:18 +0200, Hans de Goede wrote :
> Hi,
> 
> On 02-07-16 13:02, Maxime Ripard wrote:
> > On Sat, Jul 02, 2016 at 11:32:07AM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 02-07-16 11:12, Chen-Yu Tsai wrote:
> > > > Hi,
> > > > 
> > > > On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> > > > <alexandre.belloni@free-electrons.com> wrote:
> > > > > Document the bindings for the Allwinner LRADC.
> > > > 
> > > > We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> > > > and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> > > > block.
> > > 
> > > Right, this block is used on many tablets and some dev boards to
> > > provide buttons (as in the hid type) and the block is designed for
> > > this purpose, giving an irq when the adc level crosses a certain
> > > threshold.
> > > 
> > > Sure it can be used in a more generic way, but that is not its
> > > primary goal.
> > 
> > We've always had a different view on this, but it's a detail :)
> > 
> > > So any generic purpose adc driver must not break the current
> > > use-case (which is already used in mainline kernel dts files
> > > in plenty of cases).
> > 
> > Yep.
> > 
> > > I believe that the best way to deal with this is to add an
> > > "allwinner,general-purpose-mode" flag to the existing binding
> > > (as well as document general purpose mode in the existing
> > > binding rather then in a new binding doc).
> > > 
> > > That seems to be the right thing to do purely looking at this
> > > from a dt binding pov.
> > 
> > There's a way simpler solution: if there's no child nodes, it's meant
> > to be used as an ADC, otherwise, as input.
> > 
> > The logic will have to be a bit more complex than that, since there's
> > two channels, and you could only require one for the buttons, leaving
> > the other one available as an ADC.
> > 
> > But that doesn't require any new property.
> 
> True, if there are no button nodes, then go general-purpose will
> work too.
> 

Well, I'd still argue that we need two different compatibles because
encoding the usage of an IP in its compatible string is less than ideal.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Maxime Ripard]

On Sat, Jul 02, 2016 at 03:32:43PM +0200, Alexandre Belloni wrote:
> > > > I believe that the best way to deal with this is to add an
> > > > "allwinner,general-purpose-mode" flag to the existing binding
> > > > (as well as document general purpose mode in the existing
> > > > binding rather then in a new binding doc).
> > > > 
> > > > That seems to be the right thing to do purely looking at this
> > > > from a dt binding pov.
> > > 
> > > There's a way simpler solution: if there's no child nodes, it's meant
> > > to be used as an ADC, otherwise, as input.
> > > 
> > > The logic will have to be a bit more complex than that, since there's
> > > two channels, and you could only require one for the buttons, leaving
> > > the other one available as an ADC.
> > > 
> > > But that doesn't require any new property.
> > 
> > True, if there are no button nodes, then go general-purpose will
> > work too.
> 
> Well, I'd still argue that we need two different compatibles because
> encoding the usage of an IP in its compatible string is less than ideal.

That's true, but it's what we have.

So feel free to add a new compatible if you want, but we'll have to
support the old one anyway.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160702/a87c13a3/attachment.sig>

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Alexandre Belloni]

On 02/07/2016 at 17:12:55 +0800, Chen-Yu Tsai wrote :
> Hi,
> 
> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
> > Document the bindings for the Allwinner LRADC.
> 
> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> block.
> 
> Any plans to reconcile the different bindings?
> 

Yes, I already submitted an adc-keys driver that can work with any ADC:
https://lkml.org/lkml/2016/7/1/670

I agree that because it is not yet handling interrupts and is polling
the ADC, it is not as good as sun4i-lradc-keys yet. My plan is to solve
that but it require significant work in iio.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Hans de Goede]

Hi,

On 02-07-16 15:35, Alexandre Belloni wrote:
> On 02/07/2016 at 17:12:55 +0800, Chen-Yu Tsai wrote :
>> Hi,
>>
>> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
>> <alexandre.belloni@free-electrons.com> wrote:
>>> Document the bindings for the Allwinner LRADC.
>>
>> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
>> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
>> block.
>>
>> Any plans to reconcile the different bindings?
>>
>
> Yes, I already submitted an adc-keys driver that can work with any ADC:
> https://lkml.org/lkml/2016/7/1/670
>
> I agree that because it is not yet handling interrupts and is polling
> the ADC, it is not as good as sun4i-lradc-keys yet. My plan is to solve
> that but it require significant work in iio.

And it also seems to break dt compatibility. Note I'm not against
making an exception for this and breaking the dt compat, but until
the polling is fixed we should not replace sun4i-lradc-keys.

If I understand you correctly then you want to use a new generic
"sun4i-lradc" compatible. If you do that then we can just build both
drivers for now and use the right compatible depending on how the
board uses the lradc for now.

Regards,

Hans

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Alexandre Belloni]

On 02/07/2016 at 21:46:43 +0200, Hans de Goede wrote :
> Hi,
> 
> On 02-07-16 15:35, Alexandre Belloni wrote:
> > On 02/07/2016 at 17:12:55 +0800, Chen-Yu Tsai wrote :
> > > Hi,
> > > 
> > > On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
> > > <alexandre.belloni@free-electrons.com> wrote:
> > > > Document the bindings for the Allwinner LRADC.
> > > 
> > > We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
> > > and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
> > > block.
> > > 
> > > Any plans to reconcile the different bindings?
> > > 
> > 
> > Yes, I already submitted an adc-keys driver that can work with any ADC:
> > https://lkml.org/lkml/2016/7/1/670
> > 
> > I agree that because it is not yet handling interrupts and is polling
> > the ADC, it is not as good as sun4i-lradc-keys yet. My plan is to solve
> > that but it require significant work in iio.
> 
> And it also seems to break dt compatibility. Note I'm not against
> making an exception for this and breaking the dt compat, but until
> the polling is fixed we should not replace sun4i-lradc-keys.
> 
> If I understand you correctly then you want to use a new generic
> "sun4i-lradc" compatible. If you do that then we can just build both
> drivers for now and use the right compatible depending on how the
> board uses the lradc for now.
> 

Well, I never said we have to remove the previous compatible, just that
it was probably not the best one. I also didn't send a patch to remove
the previous driver and they can indeed coexist nicely for now.

Anyway, if we want to remove the sun4i-lradc-keys driver and keep DT
compatibility, we'll have to write a small stub driver. It isn't the
easiest task but it is doable.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 1/2] iio: sun4i-lradc: Add binding documentation

[Jonathan Cameron]

On 02/07/16 14:35, Alexandre Belloni wrote:
> On 02/07/2016 at 17:12:55 +0800, Chen-Yu Tsai wrote :
>> Hi,
>>
>> On Sat, Jul 2, 2016 at 5:00 AM, Alexandre Belloni
>> <alexandre.belloni@free-electrons.com> wrote:
>>> Document the bindings for the Allwinner LRADC.
>>
>> We already have Documentation/devicetree/bindings/input/sun4i-lradc-keys.txt
>> and I'm pretty sure Hans (CC-ed) argued that this is not a generic ADC
>> block.
>>
>> Any plans to reconcile the different bindings?
>>
> 
> Yes, I already submitted an adc-keys driver that can work with any ADC:
> https://lkml.org/lkml/2016/7/1/670
> 
> I agree that because it is not yet handling interrupts and is polling
> the ADC, it is not as good as sun4i-lradc-keys yet. My plan is to solve
> that but it require significant work in iio.
> 
As I'm having fun confusing the two drivers submitted for different
ADCs on the A10 this morning - here is the relevant bit of text I stuck
in my review for the other driver...

 For the key detection you have already observed that IIO needs some
 additions to be able to have consumers of what we term 'events' e.g. threshold
 interrupts.
 
 Looking at the lradc-keys driver in tree, it looks like we only really have
 really simple threshold interrups - configured to detect a very low voltage?
 + only one per channel.
 
 So not too nasty a case, but you are right some work is needed in IIO as
 we simply don't have a means of passing these on as yet or configuring them
 from in kernel consumers.
 If we take the easy route and don't demux incoming events then it shouldn't
 be too hard to add (demux can follow later).  Hence any client device can try
 to enable events it wants, but may get events that other client devices wanted
 as well.
 
 Config interface should be much the same as the write support for channels.
 Data flow marginally harder, but pretty much a list of callbacks within
 iio_push_event.
 
 Not trivial, but not too tricky either.
 
 The events subsystem has a few 'limitations' we need to address long term
 but as this is in kernel interface only, we can do this now and fix stuff
 up in future without any ABI breakage. (limitations are things like only
 one event of a given type and direction per channel - main challenge on
 that is finding a way of doing it without abi breakage).
 
 Anyhow, sounds fun - wish I had the time to do it myself!
Jonathan