[PATCH v2 0/4] add support for Allwinner SoCs ADC

[PATCH v2 0/4] add support for Allwinner SoCs ADC

[Quentin Schulz]

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. The first four channels can be used either
for the ADC or the touchscreen and the fifth channel is used for the
thermal sensor. We currently have a driver for the two latter functions in
drivers/input/touchscreen/sun4i-ts.c but we don't have access to the ADC
feature at all. It is meant to replace the current driver by using MFD and
subdrivers.

This adds initial support for Allwinner SoCs ADC with all features. Yet,
the touchscreen is not implemented but will be added later. To switch
between touchscreen and ADC modes, you need to poke a few bits in registers
and (de)activate an interrupt (pen-up).
An MFD is provided to let the input driver activate the pen-up interrupt
through a virtual interrupt, poke a few bits via regmap and read data from
the ADC driver while both (and iio_hwmon) are probed by the MFD.

There are slight variations between the different SoCs ADC like the address
of some registers and the scale and offset to apply to raw thermal sensor
values. These variations are handled by using different platform_device_id,
passed to the sub-drivers when they are probed by the MFD.

This also modifies iio-hwmon to allow probe deferring when no iio channel
is found. Currently when no iio channel is found, the probing of iio-hwmon
fails. This is problematic when iio-hwmon probes before the iio driver
could register iio channels to share.
It exposes labels for channels read by iio_hwmon. Until now, there were no
way to identify what the exposed channels are representing. Now, if a
channel has its extend_name field set, this value will be exposed in a
sysfs file suffixed by _label. If the field is empty, no file will be
created.

Quentin Schulz (4):
  hwmon: iio_hwmon: defer probe when no channel is found
  iio: adc: add support for Allwinner SoCs ADC
  mfd: add support for Allwinner SoCs ADC
  hwmon: iio: add label for channels read by iio_hwmon

 drivers/hwmon/iio_hwmon.c           |  82 ++++++-
 drivers/iio/adc/Kconfig             |  12 ++
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/sunxi-gpadc-iio.c   | 417 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/Kconfig                 |  14 ++
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/sunxi-gpadc-mfd.c       | 197 +++++++++++++++++
 include/linux/mfd/sunxi-gpadc-mfd.h |  23 ++
 8 files changed, 738 insertions(+), 10 deletions(-)
 create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
 create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
 create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h

-- 
2.5.0

[PATCH v2 1/4] hwmon: iio_hwmon: defer probe when no channel is found

[Quentin Schulz]

iio_channel_get_all returns -ENODEV when it cannot find either phandles and
properties in the Device Tree or channels whose consumer_dev_name matches
iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
which might be probed after iio_hwmon.

It is better to defer the probe of iio_hwmon if such error is returned by
iio_channel_get_all in order to let a chance to iio drivers to expose
channels in iio_map_list.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

No modifications for this patch since we did not settled for a solution.
What should we do?

 drivers/hwmon/iio_hwmon.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index b550ba5..c0da4d9 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		name = dev->of_node->name;
 
 	channels = iio_channel_get_all(dev);
-	if (IS_ERR(channels))
+	if (IS_ERR(channels)) {
+		if (PTR_ERR(channels) == -ENODEV)
+			return -EPROBE_DEFER;
 		return PTR_ERR(channels);
+	}
 
 	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
 	if (st == NULL) {
-- 
2.5.0

[v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found

[Guenter Roeck]

On Fri, Jul 15, 2016 at 11:59:11AM +0200, Quentin Schulz wrote:
> iio_channel_get_all returns -ENODEV when it cannot find either phandles and
> properties in the Device Tree or channels whose consumer_dev_name matches
> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> which might be probed after iio_hwmon.
> 
> It is better to defer the probe of iio_hwmon if such error is returned by
> iio_channel_get_all in order to let a chance to iio drivers to expose
> channels in iio_map_list.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> No modifications for this patch since we did not settled for a solution.
> What should we do?
> 
AFAICS the only thing we can do is to replace module_platform_driver() with
its explicitly coded variant, and to use use late_initcall() instead of
module_init(). Anything else would result in endless probe deferrals if
there are no channels.

Guenter

>  drivers/hwmon/iio_hwmon.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index b550ba5..c0da4d9 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		name = dev->of_node->name;
>  
>  	channels = iio_channel_get_all(dev);
> -	if (IS_ERR(channels))
> +	if (IS_ERR(channels)) {
> +		if (PTR_ERR(channels) == -ENODEV)
> +			return -EPROBE_DEFER;
>  		return PTR_ERR(channels);
> +	}
>  
>  	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>  	if (st == NULL) {

[v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found

[Maxime Ripard]

Hi Guenter,

On Sat, Jul 16, 2016 at 10:00:13AM -0700, Guenter Roeck wrote:
> On Fri, Jul 15, 2016 at 11:59:11AM +0200, Quentin Schulz wrote:
> > iio_channel_get_all returns -ENODEV when it cannot find either phandles and
> > properties in the Device Tree or channels whose consumer_dev_name matches
> > iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> > which might be probed after iio_hwmon.
> > 
> > It is better to defer the probe of iio_hwmon if such error is returned by
> > iio_channel_get_all in order to let a chance to iio drivers to expose
> > channels in iio_map_list.
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > ---
> > 
> > No modifications for this patch since we did not settled for a solution.
> > What should we do?
> > 
> AFAICS the only thing we can do is to replace module_platform_driver() with
> its explicitly coded variant, and to use use late_initcall() instead of
> module_init().

I thought this kind of changes to the driver init time were
discouraged these days?

> Anything else would result in endless probe deferrals if there are
> no channels.

Well, technically, not endless. AFAIK, the kernel only retries when a
new driver is probed, which should hopefully settle down rather
quickly.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel 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/20160718/058276cc/attachment.sig>

[v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found

[Guenter Roeck]

On 07/18/2016 03:02 AM, Maxime Ripard wrote:
> Hi Guenter,
>
> On Sat, Jul 16, 2016 at 10:00:13AM -0700, Guenter Roeck wrote:
>> On Fri, Jul 15, 2016 at 11:59:11AM +0200, Quentin Schulz wrote:
>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles and
>>> properties in the Device Tree or channels whose consumer_dev_name matches
>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>>> which might be probed after iio_hwmon.
>>>
>>> It is better to defer the probe of iio_hwmon if such error is returned by
>>> iio_channel_get_all in order to let a chance to iio drivers to expose
>>> channels in iio_map_list.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>> ---
>>>
>>> No modifications for this patch since we did not settled for a solution.
>>> What should we do?
>>>
>> AFAICS the only thing we can do is to replace module_platform_driver() with
>> its explicitly coded variant, and to use use late_initcall() instead of
>> module_init().
>
> I thought this kind of changes to the driver init time were
> discouraged these days?
>

Blindly converting -ENODEV to -EPROBEDEFER doesn't sound like a good idea
either.

>> Anything else would result in endless probe deferrals if there are
>> no channels.
>
> Well, technically, not endless. AFAIK, the kernel only retries when a
> new driver is probed, which should hopefully settle down rather
> quickly.
>

Plus each time a new driver is loaded for whatever reason.

Also, are you sure that leaving a device in deferred probe state doesn't
have undesirable side effects ? For example, would driver_probe_done()
ever return true ?

Guenter

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. This patch adds the ADC driver which is
based on the MFD for the same SoCs ADC.

This also registers the thermal adc channel in the iio map array so
iio_hwmon could use it without modifying the Device Tree.

This driver probes on three different platform_device_id to take into
account slight differences between Allwinner SoCs ADCs.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

v2:
 - add SUNXI_GPADC_ prefixes for defines,
 - correct typo in Kconfig,
 - reorder alphabetically includes, makefile,
 - add license header,
 - fix architecture variations not being handled in interrupt handlers or
   read raw functions,
 - fix unability to return negative values from thermal sensor,
 - add gotos to reduce code repetition,
 - fix irq variable being unsigned int instead of int,
 - remove useless dev_err and dev_info,
 - deactivate all interrupts if probe fails,
 - fix iio_device_register on NULL variable,
 - deactivate ADC in the IP when probe fails or when removing driver,

 drivers/iio/adc/Kconfig           |  12 ++
 drivers/iio/adc/Makefile          |   1 +
 drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 430 insertions(+)
 create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 25378c5..184856f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -338,6 +338,18 @@ config NAU7802
 	  To compile this driver as a module, choose M here: the
 	  module will be called nau7802.
 
+config SUNXI_ADC
+	tristate "ADC driver for sunxi platforms"
+	depends on IIO
+	depends on MFD_SUNXI_ADC
+	help
+	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
+	  ADC. This ADC provides 4 channels which can be used as an ADC or as a
+	  touchscreen input and one channel for thermal sensor.
+
+          To compile this driver as a module, choose M here: the module will be
+	  called sunxi-gpadc-iio.
+
 config PALMAS_GPADC
 	tristate "TI Palmas General Purpose ADC"
 	depends on MFD_PALMAS
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 38638d4..3e60a1d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -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_SUNXI_ADC) += sunxi-gpadc-iio.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/sunxi-gpadc-iio.c b/drivers/iio/adc/sunxi-gpadc-iio.c
new file mode 100644
index 0000000..87cc913
--- /dev/null
+++ b/drivers/iio/adc/sunxi-gpadc-iio.c
@@ -0,0 +1,417 @@
+/* ADC driver for sunxi platforms
+ *
+ * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/driver.h>
+#include <linux/iio/machine.h>
+#include <linux/mfd/sunxi-gpadc-mfd.h>
+
+#define SUNXI_GPADC_TP_CTRL0			0x00
+#define SUNXI_GPADC_TP_CTRL1			0x04
+#define SUNXI_GPADC_TP_CTRL2			0x08
+#define SUNXI_GPADC_TP_CTRL3			0x0c
+#define SUNXI_GPADC_TP_TPR			0x18
+#define SUNXI_GPADC_TP_CDAT			0x1c
+#define SUNXI_GPADC_TEMP_DATA			0x20
+#define SUNXI_GPADC_TP_DATA			0x24
+
+/* TP_CTRL0 bits */
+#define SUNXI_GPADC_ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
+#define SUNXI_GPADC_ADC_FIRST_DLY_MODE		BIT(23)
+#define SUNXI_GPADC_ADC_CLK_SELECT		BIT(22)
+#define SUNXI_GPADC_ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
+#define SUNXI_GPADC_FS_DIV(x)			((x) << 16) /* 4 bits */
+#define SUNXI_GPADC_T_ACQ(x)			((x) << 0)  /* 16 bits */
+
+/* TP_CTRL1 bits */
+#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x)	((x) << 12) /* 8 bits */
+#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN	BIT(9)
+#define SUNXI_GPADC_TOUCH_PAN_CALI_EN		BIT(6)
+#define SUNXI_GPADC_TP_DUAL_EN			BIT(5)
+#define SUNXI_GPADC_TP_MODE_EN			BIT(4)
+#define SUNXI_GPADC_TP_ADC_SELECT		BIT(3)
+#define SUNXI_GPADC_ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
+
+/* TP_CTRL1 bits for sun6i SOCs */
+#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN	BIT(7)
+#define SUNXI_GPADC_SUN6I_TP_DUAL_EN		BIT(6)
+#define SUNXI_GPADC_SUN6I_TP_MODE_EN		BIT(5)
+#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT		BIT(4)
+#define SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(x)	BIT(x)  /* 4 bits */
+
+/* TP_CTRL2 bits */
+#define SUNXI_GPADC_TP_SENSITIVE_ADJUST(x)	((x) << 28) /* 4 bits */
+#define SUNXI_GPADC_TP_MODE_SELECT(x)		((x) << 26) /* 2 bits */
+#define SUNXI_GPADC_PRE_MEA_EN			BIT(24)
+#define SUNXI_GPADC_PRE_MEA_THRE_CNT(x)		((x) << 0)  /* 24 bits */
+
+/* TP_CTRL3 bits */
+#define SUNXI_GPADC_FILTER_EN			BIT(2)
+#define SUNXI_GPADC_FILTER_TYPE(x)		((x) << 0)  /* 2 bits */
+
+/* TP_INT_FIFOC irq and fifo mask / control bits */
+#define SUNXI_GPADC_TEMP_IRQ_EN			BIT(18)
+#define SUNXI_GPADC_TP_OVERRUN_IRQ_EN		BIT(17)
+#define SUNXI_GPADC_TP_DATA_IRQ_EN		BIT(16)
+#define SUNXI_GPADC_TP_DATA_XY_CHANGE		BIT(13)
+#define SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(x)	((x) << 8)  /* 5 bits */
+#define SUNXI_GPADC_TP_DATA_DRQ_EN		BIT(7)
+#define SUNXI_GPADC_TP_FIFO_FLUSH		BIT(4)
+#define SUNXI_GPADC_TP_UP_IRQ_EN		BIT(1)
+#define SUNXI_GPADC_TP_DOWN_IRQ_EN		BIT(0)
+
+/* TP_INT_FIFOS irq and fifo status bits */
+#define SUNXI_GPADC_TEMP_DATA_PENDING		BIT(18)
+#define SUNXI_GPADC_FIFO_OVERRUN_PENDING	BIT(17)
+#define SUNXI_GPADC_FIFO_DATA_PENDING		BIT(16)
+#define SUNXI_GPADC_TP_IDLE_FLG			BIT(2)
+#define SUNXI_GPADC_TP_UP_PENDING		BIT(1)
+#define SUNXI_GPADC_TP_DOWN_PENDING		BIT(0)
+
+/* TP_TPR bits */
+#define SUNXI_GPADC_TEMP_ENABLE(x)		((x) << 16)
+/* t = x * 256 * 16 / clkin */
+#define SUNXI_GPADC_TEMP_PERIOD(x)		((x) << 0)
+
+#define SUNXI_GPADC_ARCH_SUN4I			BIT(0)
+#define SUNXI_GPADC_ARCH_SUN5I			BIT(1)
+#define SUNXI_GPADC_ARCH_SUN6I			BIT(2)
+
+struct sunxi_gpadc_dev {
+	void __iomem			*regs;
+	struct completion		completion;
+	int				temp_data;
+	u32				adc_data;
+	struct regmap			*regmap;
+	unsigned int			fifo_data_irq;
+	unsigned int			temp_data_irq;
+	unsigned int			flags;
+};
+
+#define SUNXI_GPADC_ADC_CHANNEL(_channel, _name) {		\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.datasheet_name = _name,				\
+}
+
+static struct iio_map sunxi_gpadc_hwmon_maps[] = {
+	{
+		.adc_channel_label = "temp_adc",
+		.consumer_dev_name = "iio_hwmon.0",
+	},
+	{ /* sentinel */ },
+};
+
+static const struct iio_chan_spec sunxi_gpadc_channels[] = {
+	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
+	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
+	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
+	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.datasheet_name = "temp_adc",
+		.extend_name = "SoC temperature",
+	},
+	{ /* sentinel */ },
+};
+
+static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
+				int *val)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
+	int ret = 0;
+
+	mutex_lock(&indio_dev->mlock);
+
+	reinit_completion(&info->completion);
+	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
+		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+			     SUNXI_GPADC_SUN6I_TP_MODE_EN |
+			     SUNXI_GPADC_SUN6I_TP_ADC_SELECT |
+			     SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel));
+	else
+		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+			     SUNXI_GPADC_TP_MODE_EN |
+			     SUNXI_GPADC_TP_ADC_SELECT |
+			     SUNXI_GPADC_ADC_CHAN_SELECT(channel));
+	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
+		     SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) |
+		     SUNXI_GPADC_TP_FIFO_FLUSH);
+	enable_irq(info->fifo_data_irq);
+
+	if (!wait_for_completion_timeout(&info->completion,
+					 msecs_to_jiffies(100))) {
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	*val = info->adc_data;
+
+out:
+	disable_irq(info->fifo_data_irq);
+	mutex_unlock(&indio_dev->mlock);
+
+	return ret;
+}
+
+static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
+	int ret = 0;
+
+	mutex_lock(&indio_dev->mlock);
+
+	reinit_completion(&info->completion);
+
+	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
+		     SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) |
+		     SUNXI_GPADC_TP_FIFO_FLUSH);
+	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
+		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+			     SUNXI_GPADC_SUN6I_TP_MODE_EN);
+	else
+		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+			     SUNXI_GPADC_TP_MODE_EN);
+	enable_irq(info->temp_data_irq);
+
+	if (!wait_for_completion_timeout(&info->completion,
+					 msecs_to_jiffies(100))) {
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
+		*val = info->temp_data * 133 - 257000;
+	else if (info->flags & SUNXI_GPADC_ARCH_SUN5I)
+		*val = info->temp_data * 100 - 144700;
+	else if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
+		*val = info->temp_data * 167 - 271000;
+
+out:
+	disable_irq(info->temp_data_irq);
+	mutex_unlock(&indio_dev->mlock);
+
+	return ret;
+
+}
+
+static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = sunxi_gpadc_temp_read(indio_dev, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_RAW:
+		ret = sunxi_gpadc_adc_read(indio_dev, chan->channel, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info sunxi_gpadc_iio_info = {
+	.read_raw = sunxi_gpadc_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void *dev_id)
+{
+	struct sunxi_gpadc_dev *info = dev_id;
+	int ret;
+
+	ret = regmap_read(info->regmap, SUNXI_GPADC_TEMP_DATA,
+			  &info->temp_data);
+	if (ret == 0)
+		complete(&info->completion);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
+{
+	struct sunxi_gpadc_dev *info = dev_id;
+	int ret;
+
+	ret = regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data);
+	if (ret == 0)
+		complete(&info->completion);
+
+	return IRQ_HANDLED;
+}
+
+static int sunxi_gpadc_probe(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_dev *info;
+	struct iio_dev *indio_dev;
+	int ret, irq;
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
+
+	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+
+	info->regmap = sunxi_gpadc_mfd_dev->regmap;
+	init_completion(&info->completion);
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->info = &sunxi_gpadc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
+	indio_dev->channels = sunxi_gpadc_channels;
+
+	info->flags = platform_get_device_id(pdev)->driver_data;
+
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL0, SUNXI_GPADC_FS_DIV(7) |
+		     SUNXI_GPADC_ADC_CLK_DIVIDER(2) | SUNXI_GPADC_T_ACQ(63));
+	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
+		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+			     SUNXI_GPADC_SUN6I_TP_MODE_EN);
+	else
+		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+			     SUNXI_GPADC_TP_MODE_EN);
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3, SUNXI_GPADC_FILTER_EN |
+		     SUNXI_GPADC_FILTER_TYPE(1));
+	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR,
+		     SUNXI_GPADC_TEMP_ENABLE(1) |
+		     SUNXI_GPADC_TEMP_PERIOD(1953));
+
+	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
+	if (irq < 0) {
+		dev_err(&pdev->dev,
+			"no TEMP_DATA_PENDING interrupt registered\n");
+		ret = irq;
+		goto err;
+	}
+
+	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
+	ret = devm_request_any_context_irq(&pdev->dev, irq,
+					   sunxi_gpadc_temp_data_irq_handler, 0,
+					   "temp_data", info);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"could not request TEMP_DATA_PENDING interrupt: %d\n",
+			ret);
+		goto err;
+	}
+
+	info->temp_data_irq = irq;
+	disable_irq(irq);
+
+	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
+	if (irq < 0) {
+		dev_err(&pdev->dev,
+			"no FIFO_DATA_PENDING interrupt registered\n");
+		ret = irq;
+		goto err;
+	}
+
+	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
+	ret = devm_request_any_context_irq(&pdev->dev, irq,
+					   sunxi_gpadc_fifo_data_irq_handler,
+					   0, "fifo_data", info);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"could not request FIFO_DATA_PENDING interrupt: %d\n",
+			ret);
+		goto err;
+	}
+
+	info->fifo_data_irq = irq;
+	disable_irq(irq);
+
+	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register iio map array\n");
+		goto err;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not register the device\n");
+		iio_map_array_unregister(indio_dev);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
+	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
+
+	return ret;
+}
+
+static int sunxi_gpadc_remove(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_dev *info;
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+	info = iio_priv(indio_dev);
+	iio_device_unregister(indio_dev);
+	iio_map_array_unregister(indio_dev);
+	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
+	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
+
+	return 0;
+}
+
+static const struct platform_device_id sunxi_gpadc_id[] = {
+	{ "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I },
+	{ "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I },
+	{ "sun6i-a31-gpadc-iio", SUNXI_GPADC_ARCH_SUN6I },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver sunxi_gpadc_driver = {
+	.driver = {
+		.name = "sunxi-gpadc-iio",
+	},
+	.id_table = sunxi_gpadc_id,
+	.probe = sunxi_gpadc_probe,
+	.remove = sunxi_gpadc_remove,
+};
+
+module_platform_driver(sunxi_gpadc_driver);
+
+MODULE_DESCRIPTION("ADC driver for sunxi platforms");
+MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Maxime Ripard]

On Fri, Jul 15, 2016 at 11:59:12AM +0200, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>    read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig           |  12 ++
>  drivers/iio/adc/Makefile          |   1 +
>  drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 430 insertions(+)
>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..184856f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -338,6 +338,18 @@ config NAU7802
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called nau7802.
>  
> +config SUNXI_ADC

We try to avoid the SUNXI prefix usually, otherwise this driver will
have a generic name (or at least is implicitly saying that it supports
all the sunxi SoCs), while it supports only a subset of those SoCs.

> +	tristate "ADC driver for sunxi platforms"

And you should also mention which ADC is supported, since we usually
have several of them.

Something like "Support for the Allwinner SoCs GPADC"

> +	depends on IIO
> +	depends on MFD_SUNXI_ADC

The order of your patches is quite weird. You depend on an option that
is not present yet?

> +	help
> +	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +	  ADC. This ADC provides 4 channels which can be used as an ADC or as a
> +	  touchscreen input and one channel for thermal sensor.
> +
> +          To compile this driver as a module, choose M here: the module will be

Your indentation is weird here, and the wrapping is likely to be wrong
too.

> +	  called sunxi-gpadc-iio.
> +
>  config PALMAS_GPADC
>  	tristate "TI Palmas General Purpose ADC"
>  	depends on MFD_PALMAS
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..3e60a1d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -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_SUNXI_ADC) += sunxi-gpadc-iio.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/sunxi-gpadc-iio.c b/drivers/iio/adc/sunxi-gpadc-iio.c
> new file mode 100644
> index 0000000..87cc913
> --- /dev/null
> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
> @@ -0,0 +1,417 @@
> +/* ADC driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.

Your wrapping is wrong.

> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +
> +#define SUNXI_GPADC_TP_CTRL0			0x00
> +#define SUNXI_GPADC_TP_CTRL1			0x04
> +#define SUNXI_GPADC_TP_CTRL2			0x08
> +#define SUNXI_GPADC_TP_CTRL3			0x0c
> +#define SUNXI_GPADC_TP_TPR			0x18
> +#define SUNXI_GPADC_TP_CDAT			0x1c
> +#define SUNXI_GPADC_TEMP_DATA			0x20
> +#define SUNXI_GPADC_TP_DATA			0x24
> +
> +/* TP_CTRL0 bits */
> +#define SUNXI_GPADC_ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
> +#define SUNXI_GPADC_ADC_FIRST_DLY_MODE		BIT(23)
> +#define SUNXI_GPADC_ADC_CLK_SELECT		BIT(22)
> +#define SUNXI_GPADC_ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
> +#define SUNXI_GPADC_FS_DIV(x)			((x) << 16) /* 4 bits */
> +#define SUNXI_GPADC_T_ACQ(x)			((x) << 0)  /* 16 bits */

We usually prefer to have the bits defined directly after the
registers, and prefixed with the name of the register they belong to.

Something like SUNXI_GPADC_TP_CTRL_T_ACQ in this case

> +
> +/* TP_CTRL1 bits */
> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x)	((x) << 12) /* 8 bits */
> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN	BIT(9)
> +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN		BIT(6)
> +#define SUNXI_GPADC_TP_DUAL_EN			BIT(5)
> +#define SUNXI_GPADC_TP_MODE_EN			BIT(4)
> +#define SUNXI_GPADC_TP_ADC_SELECT		BIT(3)
> +#define SUNXI_GPADC_ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */

Usually the comments are on the line above. However, if you really
want to enforce something, you should rather mask the
value. Otherwise, that comment is pretty useless.

> +
> +/* TP_CTRL1 bits for sun6i SOCs */
> +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN	BIT(7)
> +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN		BIT(6)
> +#define SUNXI_GPADC_SUN6I_TP_MODE_EN		BIT(5)
> +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT		BIT(4)

Shouldn't that go in either a common define or the touchscreen driver?

> +#define SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(x)	BIT(x)  /* 4 bits */



> +
> +/* TP_CTRL2 bits */
> +#define SUNXI_GPADC_TP_SENSITIVE_ADJUST(x)	((x) << 28) /* 4 bits */
> +#define SUNXI_GPADC_TP_MODE_SELECT(x)		((x) << 26) /* 2 bits */
> +#define SUNXI_GPADC_PRE_MEA_EN			BIT(24)
> +#define SUNXI_GPADC_PRE_MEA_THRE_CNT(x)		((x) << 0)  /* 24 bits */
> +
> +/* TP_CTRL3 bits */
> +#define SUNXI_GPADC_FILTER_EN			BIT(2)
> +#define SUNXI_GPADC_FILTER_TYPE(x)		((x) << 0)  /* 2 bits */
> +
> +/* TP_INT_FIFOC irq and fifo mask / control bits */
> +#define SUNXI_GPADC_TEMP_IRQ_EN			BIT(18)
> +#define SUNXI_GPADC_TP_OVERRUN_IRQ_EN		BIT(17)
> +#define SUNXI_GPADC_TP_DATA_IRQ_EN		BIT(16)
> +#define SUNXI_GPADC_TP_DATA_XY_CHANGE		BIT(13)
> +#define SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(x)	((x) << 8)  /* 5 bits */
> +#define SUNXI_GPADC_TP_DATA_DRQ_EN		BIT(7)
> +#define SUNXI_GPADC_TP_FIFO_FLUSH		BIT(4)
> +#define SUNXI_GPADC_TP_UP_IRQ_EN		BIT(1)
> +#define SUNXI_GPADC_TP_DOWN_IRQ_EN		BIT(0)
> +
> +/* TP_INT_FIFOS irq and fifo status bits */
> +#define SUNXI_GPADC_TEMP_DATA_PENDING		BIT(18)
> +#define SUNXI_GPADC_FIFO_OVERRUN_PENDING	BIT(17)
> +#define SUNXI_GPADC_FIFO_DATA_PENDING		BIT(16)
> +#define SUNXI_GPADC_TP_IDLE_FLG			BIT(2)
> +#define SUNXI_GPADC_TP_UP_PENDING		BIT(1)
> +#define SUNXI_GPADC_TP_DOWN_PENDING		BIT(0)
> +
> +/* TP_TPR bits */
> +#define SUNXI_GPADC_TEMP_ENABLE(x)		((x) << 16)
> +/* t = x * 256 * 16 / clkin */

That comment would be better next to the code that does that
computation.

> +#define SUNXI_GPADC_TEMP_PERIOD(x)		((x) << 0)
> +
> +#define SUNXI_GPADC_ARCH_SUN4I			BIT(0)
> +#define SUNXI_GPADC_ARCH_SUN5I			BIT(1)
> +#define SUNXI_GPADC_ARCH_SUN6I			BIT(2)
> +
> +struct sunxi_gpadc_dev {
> +	void __iomem			*regs;
> +	struct completion		completion;
> +	int				temp_data;
> +	u32				adc_data;
> +	struct regmap			*regmap;
> +	unsigned int			fifo_data_irq;
> +	unsigned int			temp_data_irq;
> +	unsigned int			flags;
> +};
> +
> +#define SUNXI_GPADC_ADC_CHANNEL(_channel, _name) {		\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.datasheet_name = _name,				\
> +}
> +
> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
> +	{
> +		.adc_channel_label = "temp_adc",
> +		.consumer_dev_name = "iio_hwmon.0",
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
> +	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
> +	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
> +	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
> +	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.datasheet_name = "temp_adc",
> +		.extend_name = "SoC temperature",
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
> +				int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	reinit_completion(&info->completion);
> +	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_SUN6I_TP_MODE_EN |
> +			     SUNXI_GPADC_SUN6I_TP_ADC_SELECT |
> +			     SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel));
> +	else
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_TP_MODE_EN |
> +			     SUNXI_GPADC_TP_ADC_SELECT |
> +			     SUNXI_GPADC_ADC_CHAN_SELECT(channel));

Using a function pointer that would compute this, or some fields in a
structure to store this would be better.

> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_FIFO_FLUSH);
> +	enable_irq(info->fifo_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*val = info->adc_data;
> +
> +out:
> +	disable_irq(info->fifo_data_irq);
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	reinit_completion(&info->completion);
> +
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_FIFO_FLUSH);
> +	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_SUN6I_TP_MODE_EN);
> +	else
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_TP_MODE_EN);
> +	enable_irq(info->temp_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
> +		*val = info->temp_data * 133 - 257000;
> +	else if (info->flags & SUNXI_GPADC_ARCH_SUN5I)
> +		*val = info->temp_data * 100 - 144700;
> +	else if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
> +		*val = info->temp_data * 167 - 271000;

Ditto, having functions to comptue this and just store the function
pointer would be better.

> +
> +out:
> +	disable_irq(info->temp_data_irq);
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret;
> +
> +}
> +
> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = sunxi_gpadc_temp_read(indio_dev, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_RAW:
> +		ret = sunxi_gpadc_adc_read(indio_dev, chan->channel, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info sunxi_gpadc_iio_info = {
> +	.read_raw = sunxi_gpadc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void *dev_id)
> +{
> +	struct sunxi_gpadc_dev *info = dev_id;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, SUNXI_GPADC_TEMP_DATA,
> +			  &info->temp_data);
> +	if (ret == 0)
> +		complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
> +{
> +	struct sunxi_gpadc_dev *info = dev_id;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data);
> +	if (ret == 0)
> +		complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sunxi_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info;
> +	struct iio_dev *indio_dev;
> +	int ret, irq;
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	info->regmap = sunxi_gpadc_mfd_dev->regmap;
> +	init_completion(&info->completion);
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &sunxi_gpadc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
> +	indio_dev->channels = sunxi_gpadc_channels;
> +
> +	info->flags = platform_get_device_id(pdev)->driver_data;
> +
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL0, SUNXI_GPADC_FS_DIV(7) |
> +		     SUNXI_GPADC_ADC_CLK_DIVIDER(2) | SUNXI_GPADC_T_ACQ(63));
> +	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_SUN6I_TP_MODE_EN);
> +	else
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_TP_MODE_EN);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3, SUNXI_GPADC_FILTER_EN |
> +		     SUNXI_GPADC_FILTER_TYPE(1));
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR,
> +		     SUNXI_GPADC_TEMP_ENABLE(1) |
> +		     SUNXI_GPADC_TEMP_PERIOD(1953));
> +
> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no TEMP_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
> +					   sunxi_gpadc_temp_data_irq_handler, 0,
> +					   "temp_data", info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"could not request TEMP_DATA_PENDING interrupt: %d\n",
> +			ret);
> +		goto err;
> +	}
> +
> +	info->temp_data_irq = irq;
> +	disable_irq(irq);
> +
> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no FIFO_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
> +					   sunxi_gpadc_fifo_data_irq_handler,
> +					   0, "fifo_data", info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
> +			ret);
> +		goto err;
> +	}
> +
> +	info->fifo_data_irq = irq;
> +	disable_irq(irq);

request_irq starts with irq enabled, which means that you can have an
interrupt showing up between your call to request_irq and the
disable_irq. 

How would that work?

> +
> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register iio map array\n");
> +		goto err;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not register the device\n");
> +		iio_map_array_unregister(indio_dev);

That should go in a separate label.

> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);

Why is that needed?

> +	return ret;
> +}
> +
> +static int sunxi_gpadc_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info;
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	info = iio_priv(indio_dev);
> +	iio_device_unregister(indio_dev);
> +	iio_map_array_unregister(indio_dev);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id sunxi_gpadc_id[] = {
> +	{ "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I },
> +	{ "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I },
> +	{ "sun6i-a31-gpadc-iio", SUNXI_GPADC_ARCH_SUN6I },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver sunxi_gpadc_driver = {
> +	.driver = {
> +		.name = "sunxi-gpadc-iio",
> +	},
> +	.id_table = sunxi_gpadc_id,
> +	.probe = sunxi_gpadc_probe,
> +	.remove = sunxi_gpadc_remove,
> +};

Having some runtime_pm support for this would be great too.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel 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/20160718/1cea435a/attachment-0001.sig>

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 18/07/2016 14:57, Maxime Ripard wrote:
> On Fri, Jul 15, 2016 at 11:59:12AM +0200, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> ---
>>
>> v2:
>>  - add SUNXI_GPADC_ prefixes for defines,
>>  - correct typo in Kconfig,
>>  - reorder alphabetically includes, makefile,
>>  - add license header,
>>  - fix architecture variations not being handled in interrupt handlers or
>>    read raw functions,
>>  - fix unability to return negative values from thermal sensor,
>>  - add gotos to reduce code repetition,
>>  - fix irq variable being unsigned int instead of int,
>>  - remove useless dev_err and dev_info,
>>  - deactivate all interrupts if probe fails,
>>  - fix iio_device_register on NULL variable,
>>  - deactivate ADC in the IP when probe fails or when removing driver,
>>
>>  drivers/iio/adc/Kconfig           |  12 ++
>>  drivers/iio/adc/Makefile          |   1 +
>>  drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 430 insertions(+)
>>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 25378c5..184856f 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -338,6 +338,18 @@ config NAU7802
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called nau7802.
>>  
>> +config SUNXI_ADC
> 
> We try to avoid the SUNXI prefix usually, otherwise this driver will
> have a generic name (or at least is implicitly saying that it supports
> all the sunxi SoCs), while it supports only a subset of those SoCs.
> 

ACK. Will be replaced by SUN4I_GPADC.

>> +	tristate "ADC driver for sunxi platforms"
> 
> And you should also mention which ADC is supported, since we usually
> have several of them.
> 
> Something like "Support for the Allwinner SoCs GPADC"
> 

ACK.

>> +	depends on IIO
>> +	depends on MFD_SUNXI_ADC
> 
> The order of your patches is quite weird. You depend on an option that
> is not present yet?
> 

ACK. Will modify the order of patches to reflect the real order.

>> +	help
>> +	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
>> +	  ADC. This ADC provides 4 channels which can be used as an ADC or as a
>> +	  touchscreen input and one channel for thermal sensor.
>> +
>> +          To compile this driver as a module, choose M here: the module will be
> 
> Your indentation is weird here, and the wrapping is likely to be wrong
> too.
> 

ACK.

[...]
>> @@ -0,0 +1,417 @@
>> +/* ADC driver for sunxi platforms
>> + *
>> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
> 
> Your wrapping is wrong.
> 

ACK.

>> + */
>> +
>> +#include <linux/completion.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/mfd/sunxi-gpadc-mfd.h>
>> +
>> +#define SUNXI_GPADC_TP_CTRL0			0x00
>> +#define SUNXI_GPADC_TP_CTRL1			0x04
>> +#define SUNXI_GPADC_TP_CTRL2			0x08
>> +#define SUNXI_GPADC_TP_CTRL3			0x0c
>> +#define SUNXI_GPADC_TP_TPR			0x18
>> +#define SUNXI_GPADC_TP_CDAT			0x1c
>> +#define SUNXI_GPADC_TEMP_DATA			0x20
>> +#define SUNXI_GPADC_TP_DATA			0x24
>> +
>> +/* TP_CTRL0 bits */
>> +#define SUNXI_GPADC_ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
>> +#define SUNXI_GPADC_ADC_FIRST_DLY_MODE		BIT(23)
>> +#define SUNXI_GPADC_ADC_CLK_SELECT		BIT(22)
>> +#define SUNXI_GPADC_ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
>> +#define SUNXI_GPADC_FS_DIV(x)			((x) << 16) /* 4 bits */
>> +#define SUNXI_GPADC_T_ACQ(x)			((x) << 0)  /* 16 bits */
> 
> We usually prefer to have the bits defined directly after the
> registers, and prefixed with the name of the register they belong to.
> 
> Something like SUNXI_GPADC_TP_CTRL_T_ACQ in this case
> 

This modification induces the name of the bits to be really long:
SUNXI_GPADC_TP_CTRL1_SUN6I_TOUCH_PAN_CALI_EN for example. ACK anyway.

>> +
>> +/* TP_CTRL1 bits */
>> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x)	((x) << 12) /* 8 bits */
>> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN	BIT(9)
>> +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN		BIT(6)
>> +#define SUNXI_GPADC_TP_DUAL_EN			BIT(5)
>> +#define SUNXI_GPADC_TP_MODE_EN			BIT(4)
>> +#define SUNXI_GPADC_TP_ADC_SELECT		BIT(3)
>> +#define SUNXI_GPADC_ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
> 
> Usually the comments are on the line above. However, if you really
> want to enforce something, you should rather mask the
> value. Otherwise, that comment is pretty useless.
> 

Do you mean something like that:
#define SUNXI_GPADC_ADC_CHAN_SELECT(x)		(GENMASK(2,0) & x) ?

>> +
>> +/* TP_CTRL1 bits for sun6i SOCs */
>> +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN	BIT(7)
>> +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN		BIT(6)
>> +#define SUNXI_GPADC_SUN6I_TP_MODE_EN		BIT(5)
>> +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT		BIT(4)
> 
> Shouldn't that go in either a common define or the touchscreen driver?
> 

Then shouldn't I put all defines in a common header? (sunxi-gpadc-mfd.h)

[...]
>> +/* TP_TPR bits */
>> +#define SUNXI_GPADC_TEMP_ENABLE(x)		((x) << 16)
>> +/* t = x * 256 * 16 / clkin */
> 
> That comment would be better next to the code that does that
> computation.
> 

ACK.

[...]
>> +	reinit_completion(&info->completion);
>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
>> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
>> +			     SUNXI_GPADC_SUN6I_TP_MODE_EN |
>> +			     SUNXI_GPADC_SUN6I_TP_ADC_SELECT |
>> +			     SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel));
>> +	else
>> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
>> +			     SUNXI_GPADC_TP_MODE_EN |
>> +			     SUNXI_GPADC_TP_ADC_SELECT |
>> +			     SUNXI_GPADC_ADC_CHAN_SELECT(channel));
> 
> Using a function pointer that would compute this, or some fields in a
> structure to store this would be better.
> 

ACK.

[...]
>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>> +		*val = info->temp_data * 133 - 257000;
>> +	else if (info->flags & SUNXI_GPADC_ARCH_SUN5I)
>> +		*val = info->temp_data * 100 - 144700;
>> +	else if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
>> +		*val = info->temp_data * 167 - 271000;
> 
> Ditto, having functions to comptue this and just store the function
> pointer would be better.
> 

As Jonathan suggests, we should better go with separate read_raws
(IIO_CHAN_RAW returns info->temp_data, INFO_CHAN_SCALE and
INFO_CHAN_OFFSET return a different value depending on the
architecture). So this would split the above code in separate functions
as you wanted.

[...]
>> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no TEMP_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_temp_data_irq_handler, 0,
>> +					   "temp_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request TEMP_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		goto err;
>> +	}
>> +
>> +	info->temp_data_irq = irq;
>> +	disable_irq(irq);
>> +
>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_fifo_data_irq_handler,
>> +					   0, "fifo_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		goto err;
>> +	}
>> +
>> +	info->fifo_data_irq = irq;
>> +	disable_irq(irq);
> 
> request_irq starts with irq enabled, which means that you can have an
> interrupt showing up between your call to request_irq and the
> disable_irq. 
> 
> How would that work?
> 

Same as what I answered in Jonathan's mail:

"Once the interrupt is activated, the IP performs continuous conversions
(temp_data_irq only periodically). I want these interrupts to be enabled
only when I read the sysfs file or we get useless interrupts.
In the current state of this driver's irq handlers, I only set values in
structures and all the needed structures are already initialized before
requesting irqs. So it does not look like a race. I can prevent races in
future versions by adding an atomic flag if wanted."

>> +
>> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to register iio map array\n");
>> +		goto err;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "could not register the device\n");
>> +		iio_map_array_unregister(indio_dev);
> 
> That should go in a separate label.
> 

ACK.

>> +		goto err;
>> +	}
>> +
>> +	return 0;
>> +
>> +err:
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
> 
> Why is that needed?
> 

This disables ADC and Temperature on the hardware side of the IP.
(mainly a shortcut to SUNXI_GPADC_TP_MODE_EN (or its architecture
variant) and SUNXI_GPADC_TEMP_ENABLE set to 0.

>> +	return ret;
>> +}
>> +
>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>> +{
>> +	struct sunxi_gpadc_dev *info;
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +	info = iio_priv(indio_dev);
>> +	iio_device_unregister(indio_dev);
>> +	iio_map_array_unregister(indio_dev);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id sunxi_gpadc_id[] = {
>> +	{ "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I },
>> +	{ "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I },
>> +	{ "sun6i-a31-gpadc-iio", SUNXI_GPADC_ARCH_SUN6I },
>> +	{ /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver sunxi_gpadc_driver = {
>> +	.driver = {
>> +		.name = "sunxi-gpadc-iio",
>> +	},
>> +	.id_table = sunxi_gpadc_id,
>> +	.probe = sunxi_gpadc_probe,
>> +	.remove = sunxi_gpadc_remove,
>> +};
> 
> Having some runtime_pm support for this would be great too.
> 

Basically disabling the ADC and interrupts (as in the remove) in
_suspend and _idle and reenabling everything in "before _suspend"-state
in _resume I guess?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160719/c5f00a2e/attachment-0001.sig>

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Maxime Ripard]

On Tue, Jul 19, 2016 at 11:04:23AM +0200, Quentin Schulz wrote:
> >> +/* TP_CTRL1 bits */
> >> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x)	((x) << 12) /* 8 bits */
> >> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN	BIT(9)
> >> +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN		BIT(6)
> >> +#define SUNXI_GPADC_TP_DUAL_EN			BIT(5)
> >> +#define SUNXI_GPADC_TP_MODE_EN			BIT(4)
> >> +#define SUNXI_GPADC_TP_ADC_SELECT		BIT(3)
> >> +#define SUNXI_GPADC_ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
> > 
> > Usually the comments are on the line above. However, if you really
> > want to enforce something, you should rather mask the
> > value. Otherwise, that comment is pretty useless.
> > 
> 
> Do you mean something like that:
> #define SUNXI_GPADC_ADC_CHAN_SELECT(x)		(GENMASK(2,0) & x) ?

Yes.

> >> +/* TP_CTRL1 bits for sun6i SOCs */
> >> +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN	BIT(7)
> >> +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN		BIT(6)
> >> +#define SUNXI_GPADC_SUN6I_TP_MODE_EN		BIT(5)
> >> +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT		BIT(4)
> > 
> > Shouldn't that go in either a common define or the touchscreen driver?
> > 
> 
> Then shouldn't I put all defines in a common header? (sunxi-gpadc-mfd.h)

You should :)

> >> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
> >> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
> >> +					   sunxi_gpadc_fifo_data_irq_handler,
> >> +					   0, "fifo_data", info);
> >> +	if (ret < 0) {
> >> +		dev_err(&pdev->dev,
> >> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
> >> +			ret);
> >> +		goto err;
> >> +	}
> >> +
> >> +	info->fifo_data_irq = irq;
> >> +	disable_irq(irq);
> > 
> > request_irq starts with irq enabled, which means that you can have an
> > interrupt showing up between your call to request_irq and the
> > disable_irq. 
> > 
> > How would that work?
> > 
> 
> Same as what I answered in Jonathan's mail:
> 
> "Once the interrupt is activated, the IP performs continuous conversions
> (temp_data_irq only periodically). I want these interrupts to be enabled
> only when I read the sysfs file or we get useless interrupts.
> In the current state of this driver's irq handlers, I only set values in
> structures and all the needed structures are already initialized before
> requesting irqs. So it does not look like a race. I can prevent races in
> future versions by adding an atomic flag if wanted."

Then please have a nice comment explaining that.

> >> +static struct platform_driver sunxi_gpadc_driver = {
> >> +	.driver = {
> >> +		.name = "sunxi-gpadc-iio",
> >> +	},
> >> +	.id_table = sunxi_gpadc_id,
> >> +	.probe = sunxi_gpadc_probe,
> >> +	.remove = sunxi_gpadc_remove,
> >> +};
> > 
> > Having some runtime_pm support for this would be great too.
> > 
> 
> Basically disabling the ADC and interrupts (as in the remove) in
> _suspend and _idle and reenabling everything in "before _suspend"-state
> in _resume I guess?

Well, yes and no. Your probe would not do any hardware
initialisation. You can pm_runtime_get_sync before using it, which
will call the suspend hook.

Once you're done, call pm_runtime_put, and that will disable the
hardware.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel 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/20160719/9f70c297/attachment.sig>

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 15/07/16 10:59, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

Hi Quentin,

Various bits inline.  In particular the irq handling looks flakey / racey
to me.  Definitely need some explanatory comments.

Also another note on the craziness that using extended_name to provide
the hwmon labels will cause :)

Jonathan
> ---
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>    read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig           |  12 ++
>  drivers/iio/adc/Makefile          |   1 +
>  drivers/iio/adc/sunxi-gpadc-iio.c | 417 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 430 insertions(+)
>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..184856f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -338,6 +338,18 @@ config NAU7802
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called nau7802.
>  
> +config SUNXI_ADC
> +	tristate "ADC driver for sunxi platforms"
> +	depends on IIO
> +	depends on MFD_SUNXI_ADC
> +	help
> +	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +	  ADC. This ADC provides 4 channels which can be used as an ADC or as a
> +	  touchscreen input and one channel for thermal sensor.
> +
> +          To compile this driver as a module, choose M here: the module will be
> +	  called sunxi-gpadc-iio.
> +
>  config PALMAS_GPADC
>  	tristate "TI Palmas General Purpose ADC"
>  	depends on MFD_PALMAS
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..3e60a1d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -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_SUNXI_ADC) += sunxi-gpadc-iio.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/sunxi-gpadc-iio.c b/drivers/iio/adc/sunxi-gpadc-iio.c
> new file mode 100644
> index 0000000..87cc913
> --- /dev/null
> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
> @@ -0,0 +1,417 @@
> +/* ADC driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +
> +#define SUNXI_GPADC_TP_CTRL0			0x00
> +#define SUNXI_GPADC_TP_CTRL1			0x04
> +#define SUNXI_GPADC_TP_CTRL2			0x08
> +#define SUNXI_GPADC_TP_CTRL3			0x0c
> +#define SUNXI_GPADC_TP_TPR			0x18
> +#define SUNXI_GPADC_TP_CDAT			0x1c
> +#define SUNXI_GPADC_TEMP_DATA			0x20
> +#define SUNXI_GPADC_TP_DATA			0x24
> +
> +/* TP_CTRL0 bits */
> +#define SUNXI_GPADC_ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
> +#define SUNXI_GPADC_ADC_FIRST_DLY_MODE		BIT(23)
> +#define SUNXI_GPADC_ADC_CLK_SELECT		BIT(22)
> +#define SUNXI_GPADC_ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
> +#define SUNXI_GPADC_FS_DIV(x)			((x) << 16) /* 4 bits */
> +#define SUNXI_GPADC_T_ACQ(x)			((x) << 0)  /* 16 bits */
> +
> +/* TP_CTRL1 bits */
> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE(x)	((x) << 12) /* 8 bits */
> +#define SUNXI_GPADC_STYLUS_UP_DEBOUNCE_EN	BIT(9)
> +#define SUNXI_GPADC_TOUCH_PAN_CALI_EN		BIT(6)
> +#define SUNXI_GPADC_TP_DUAL_EN			BIT(5)
> +#define SUNXI_GPADC_TP_MODE_EN			BIT(4)
> +#define SUNXI_GPADC_TP_ADC_SELECT		BIT(3)
> +#define SUNXI_GPADC_ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
> +
> +/* TP_CTRL1 bits for sun6i SOCs */
> +#define SUNXI_GPADC_SUN6I_TOUCH_PAN_CALI_EN	BIT(7)
> +#define SUNXI_GPADC_SUN6I_TP_DUAL_EN		BIT(6)
> +#define SUNXI_GPADC_SUN6I_TP_MODE_EN		BIT(5)
> +#define SUNXI_GPADC_SUN6I_TP_ADC_SELECT		BIT(4)
> +#define SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(x)	BIT(x)  /* 4 bits */
> +
> +/* TP_CTRL2 bits */
> +#define SUNXI_GPADC_TP_SENSITIVE_ADJUST(x)	((x) << 28) /* 4 bits */
> +#define SUNXI_GPADC_TP_MODE_SELECT(x)		((x) << 26) /* 2 bits */
> +#define SUNXI_GPADC_PRE_MEA_EN			BIT(24)
> +#define SUNXI_GPADC_PRE_MEA_THRE_CNT(x)		((x) << 0)  /* 24 bits */
> +
> +/* TP_CTRL3 bits */
> +#define SUNXI_GPADC_FILTER_EN			BIT(2)
> +#define SUNXI_GPADC_FILTER_TYPE(x)		((x) << 0)  /* 2 bits */
> +
> +/* TP_INT_FIFOC irq and fifo mask / control bits */
> +#define SUNXI_GPADC_TEMP_IRQ_EN			BIT(18)
> +#define SUNXI_GPADC_TP_OVERRUN_IRQ_EN		BIT(17)
> +#define SUNXI_GPADC_TP_DATA_IRQ_EN		BIT(16)
> +#define SUNXI_GPADC_TP_DATA_XY_CHANGE		BIT(13)
> +#define SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(x)	((x) << 8)  /* 5 bits */
> +#define SUNXI_GPADC_TP_DATA_DRQ_EN		BIT(7)
> +#define SUNXI_GPADC_TP_FIFO_FLUSH		BIT(4)
> +#define SUNXI_GPADC_TP_UP_IRQ_EN		BIT(1)
> +#define SUNXI_GPADC_TP_DOWN_IRQ_EN		BIT(0)
> +
> +/* TP_INT_FIFOS irq and fifo status bits */
> +#define SUNXI_GPADC_TEMP_DATA_PENDING		BIT(18)
> +#define SUNXI_GPADC_FIFO_OVERRUN_PENDING	BIT(17)
> +#define SUNXI_GPADC_FIFO_DATA_PENDING		BIT(16)
> +#define SUNXI_GPADC_TP_IDLE_FLG			BIT(2)
> +#define SUNXI_GPADC_TP_UP_PENDING		BIT(1)
> +#define SUNXI_GPADC_TP_DOWN_PENDING		BIT(0)
> +
> +/* TP_TPR bits */
> +#define SUNXI_GPADC_TEMP_ENABLE(x)		((x) << 16)
> +/* t = x * 256 * 16 / clkin */
> +#define SUNXI_GPADC_TEMP_PERIOD(x)		((x) << 0)
> +
> +#define SUNXI_GPADC_ARCH_SUN4I			BIT(0)
> +#define SUNXI_GPADC_ARCH_SUN5I			BIT(1)
> +#define SUNXI_GPADC_ARCH_SUN6I			BIT(2)
> +
> +struct sunxi_gpadc_dev {
> +	void __iomem			*regs;
> +	struct completion		completion;
> +	int				temp_data;
> +	u32				adc_data;
> +	struct regmap			*regmap;
> +	unsigned int			fifo_data_irq;
> +	unsigned int			temp_data_irq;
> +	unsigned int			flags;
I'd prefer something more explicit than this.  Right now only one
bit can ever be set - indicating a particular chip family.  Why not
just have an enum instead?
> +};
> +
> +#define SUNXI_GPADC_ADC_CHANNEL(_channel, _name) {		\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.datasheet_name = _name,				\
> +}
> +
> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
> +	{
> +		.adc_channel_label = "temp_adc",
> +		.consumer_dev_name = "iio_hwmon.0",
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
> +	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
> +	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
> +	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
> +	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.datasheet_name = "temp_adc",
> +		.extend_name = "SoC temperature",
Just curious, did you look at the resultling sysfs entries?
Going to be something like
in_temp_SoC\ Temperature_input... Not good.
If there is a strong enough reason (and there may be) to add a 'help string'
type label to struct iio_chan_spec then my only real worries would be that
we would be adding a whole pile of ABI that would be in the, near impossible
to change in future, category...
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
> +				int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	reinit_completion(&info->completion);
> +	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_SUN6I_TP_MODE_EN |
> +			     SUNXI_GPADC_SUN6I_TP_ADC_SELECT |
> +			     SUNXI_GPADC_SUN6I_ADC_CHAN_SELECT(channel));
> +	else
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_TP_MODE_EN |
> +			     SUNXI_GPADC_TP_ADC_SELECT |
> +			     SUNXI_GPADC_ADC_CHAN_SELECT(channel));
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_FIFO_FLUSH);
> +	enable_irq(info->fifo_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*val = info->adc_data;
> +
> +out:
> +	disable_irq(info->fifo_data_irq);
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	reinit_completion(&info->completion);
> +
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_FIFO_FLUSH);
> +	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_SUN6I_TP_MODE_EN);
> +	else
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_TP_MODE_EN);
> +	enable_irq(info->temp_data_irq);
Is this hardware spitting out extra irqs?  If not, much better to just
leave it enabled all the time and control whether it can occur or not
by controlling the device state..
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
> +		*val = info->temp_data * 133 - 257000;
Why report as processed?  I'd just report them as raw with the scale
and offset provided.  It's not a big thing, but if we can leave it so
that the conversion only occurs when desired, why not?

For in kernel users, this all happen 'automagically' anyway ;)

> +	else if (info->flags & SUNXI_GPADC_ARCH_SUN5I)
> +		*val = info->temp_data * 100 - 144700;
> +	else if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
> +		*val = info->temp_data * 167 - 271000;
> +
> +out:
> +	disable_irq(info->temp_data_irq);
> +	mutex_unlock(&indio_dev->mlock);
mlock has a very specific purpose - locking the state changes of
between 'buffered' (push) and poll modes. Don't use it for anything else.
In fact it's almost always a bug to access it directly at all.  We have
nice wrappers now for checking and locking the access mode.
Just have a local lock in your iio_priv structure.
> +
> +	return ret;
> +
> +}
> +
> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = sunxi_gpadc_temp_read(indio_dev, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_RAW:
> +		ret = sunxi_gpadc_adc_read(indio_dev, chan->channel, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info sunxi_gpadc_iio_info = {
> +	.read_raw = sunxi_gpadc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void *dev_id)
> +{
> +	struct sunxi_gpadc_dev *info = dev_id;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, SUNXI_GPADC_TEMP_DATA,
> +			  &info->temp_data);
> +	if (ret == 0)
> +		complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
> +{
> +	struct sunxi_gpadc_dev *info = dev_id;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data);
> +	if (ret == 0)
> +		complete(&info->completion);
if (!regmap_read(...))?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sunxi_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info;
> +	struct iio_dev *indio_dev;
> +	int ret, irq;
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	info->regmap = sunxi_gpadc_mfd_dev->regmap;
> +	init_completion(&info->completion);
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &sunxi_gpadc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
> +	indio_dev->channels = sunxi_gpadc_channels;
> +
> +	info->flags = platform_get_device_id(pdev)->driver_data;
> +
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL0, SUNXI_GPADC_FS_DIV(7) |
> +		     SUNXI_GPADC_ADC_CLK_DIVIDER(2) | SUNXI_GPADC_T_ACQ(63));
> +	if (info->flags & SUNXI_GPADC_ARCH_SUN6I)
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_SUN6I_TP_MODE_EN);
> +	else
> +		regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +			     SUNXI_GPADC_TP_MODE_EN);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3, SUNXI_GPADC_FILTER_EN |
> +		     SUNXI_GPADC_FILTER_TYPE(1));
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR,
> +		     SUNXI_GPADC_TEMP_ENABLE(1) |
> +		     SUNXI_GPADC_TEMP_PERIOD(1953));
> +
> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no TEMP_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
> +					   sunxi_gpadc_temp_data_irq_handler, 0,
> +					   "temp_data", info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"could not request TEMP_DATA_PENDING interrupt: %d\n",
> +			ret);
> +		goto err;
> +	}
> +
> +	info->temp_data_irq = irq;
> +	disable_irq(irq);
As below, I want a comment explaining why you are disabling the irq here.
This is clearly racey..
> +
> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no FIFO_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
> +					   sunxi_gpadc_fifo_data_irq_handler,
> +					   0, "fifo_data", info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
> +			ret);
> +		goto err;
> +	}
> +
> +	info->fifo_data_irq = irq;
There's a race here - this may be the only way of doing it though.

However, I want a comment explaining why here... Are we looking at a hardware
bug?
> +	disable_irq(irq);
> +
> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register iio map array\n");
> +		goto err;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not register the device\n");
> +		iio_map_array_unregister(indio_dev);
Once you have an unwinding path (which is sensible here) put everything in
that block - it makes it easier to review (initially I compared that block
with the remove and thought you'd missed this error path).

> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info;
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	info = iio_priv(indio_dev);
> +	iio_device_unregister(indio_dev);
> +	iio_map_array_unregister(indio_dev);
Slight disrepancy here between the cleanup in the error path in
probe vs what we have in remove (next line doesn't exist in probe cleanup)
Perhaps a comment here would make it clear why...
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id sunxi_gpadc_id[] = {
> +	{ "sun4i-a10-gpadc-iio", SUNXI_GPADC_ARCH_SUN4I },
> +	{ "sun5i-a13-gpadc-iio", SUNXI_GPADC_ARCH_SUN5I },
> +	{ "sun6i-a31-gpadc-iio", SUNXI_GPADC_ARCH_SUN6I },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver sunxi_gpadc_driver = {
> +	.driver = {
> +		.name = "sunxi-gpadc-iio",
> +	},
> +	.id_table = sunxi_gpadc_id,
> +	.probe = sunxi_gpadc_probe,
> +	.remove = sunxi_gpadc_remove,
> +};
> +
> +module_platform_driver(sunxi_gpadc_driver);
> +
> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_LICENSE("GPL v2");
> 

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 18/07/2016 15:18, Jonathan Cameron wrote:
> On 15/07/16 10:59, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> 
> Hi Quentin,
> 
> Various bits inline.  In particular the irq handling looks flakey / racey
> to me.  Definitely need some explanatory comments.
> 
> Also another note on the craziness that using extended_name to provide
> the hwmon labels will cause :)
> 
> Jonathan
[...]
>> +struct sunxi_gpadc_dev {
>> +	void __iomem			*regs;
>> +	struct completion		completion;
>> +	int				temp_data;
>> +	u32				adc_data;
>> +	struct regmap			*regmap;
>> +	unsigned int			fifo_data_irq;
>> +	unsigned int			temp_data_irq;
>> +	unsigned int			flags;
> I'd prefer something more explicit than this.  Right now only one
> bit can ever be set - indicating a particular chip family.  Why not
> just have an enum instead?

ACK.

[...]
>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>> +	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
>> +	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
>> +	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
>> +	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>> +	{
>> +		.type = IIO_TEMP,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +		.datasheet_name = "temp_adc",
>> +		.extend_name = "SoC temperature",
> Just curious, did you look at the resultling sysfs entries?
> Going to be something like
> in_temp_SoC\ Temperature_input... Not good.

Just encountered this after further testing, not good as you said.

> If there is a strong enough reason (and there may be) to add a 'help string'
> type label to struct iio_chan_spec then my only real worries would be that
> we would be adding a whole pile of ABI that would be in the, near impossible
> to change in future, category...

I don't understand your "adding a whole pile of ABI" thing. Same as
datasheet_name variable: const char* in struct iio_chan_spec and used
whenever needed with some NULL checks. This is the easiest way to do it.
Or maybe you're thinking of adding an item to iio_chan_info_enum and all
its underlying modifications? This could allow us to expose a _label
sysfs file in the iio_device per type/channel.

I understand the "near impossible to change in future" concern though.

[...]
>> +	enable_irq(info->temp_data_irq);
> Is this hardware spitting out extra irqs?  If not, much better to just
> leave it enabled all the time and control whether it can occur or not
> by controlling the device state..

The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in
the current state of the driver: 2s). What do you mean by controlling
the device state? Enabling or disabling the hardware part of the IP
responsible of getting the temperature
(SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)?
Once the interrupt is activated, the IP periodically performs
conversions. We don't really want interrupts to be activated when not
needed.

[...]
>> +
>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>> +		*val = info->temp_data * 133 - 257000;
> Why report as processed?  I'd just report them as raw with the scale
> and offset provided.  It's not a big thing, but if we can leave it so
> that the conversion only occurs when desired, why not?
> 
> For in kernel users, this all happen 'automagically' anyway ;)
> 

ACK.

[...]
>> +	mutex_unlock(&indio_dev->mlock);
> mlock has a very specific purpose - locking the state changes of
> between 'buffered' (push) and poll modes. Don't use it for anything else.
> In fact it's almost always a bug to access it directly at all.  We have
> nice wrappers now for checking and locking the access mode.
> Just have a local lock in your iio_priv structure.

ACK. There is still a lot of drivers using iio_dev mlock though.

[...]
>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct sunxi_gpadc_dev *info = dev_id;
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data);
>> +	if (ret == 0)
>> +		complete(&info->completion);
> if (!regmap_read(...))?
> 

ACK.

[...]
>> +
>> +	info->temp_data_irq = irq;
>> +	disable_irq(irq);
> As below, I want a comment explaining why you are disabling the irq here.
> This is clearly racey..

Once the interrupt is activated, the IP performs continuous conversions
(temp_data_irq only periodically). I want these interrupts to be enabled
only when I read the sysfs file or we get useless interrupts.
In the current state of this driver's irq handlers, I only set values in
structures and all the needed structures are already initialized before
requesting irqs. So it does not look like a race. I can prevent races in
future versions by adding an atomic flag if wanted.

>> +
>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_fifo_data_irq_handler,
>> +					   0, "fifo_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		goto err;
>> +	}
>> +
>> +	info->fifo_data_irq = irq;
> There's a race here - this may be the only way of doing it though.
> 
> However, I want a comment explaining why here... Are we looking at a hardware
> bug?
>> +	disable_irq(irq);
>> +
>> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to register iio map array\n");
>> +		goto err;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "could not register the device\n");
>> +		iio_map_array_unregister(indio_dev);
> Once you have an unwinding path (which is sensible here) put everything in
> that block - it makes it easier to review (initially I compared that block
> with the remove and thought you'd missed this error path).
> 

ACK. As suggested by Maxime, I will put that in a different goto label.

>> +		goto err;
>> +	}
>> +
>> +	return 0;
>> +
>> +err:
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>> +{
>> +	struct sunxi_gpadc_dev *info;
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +	info = iio_priv(indio_dev);
>> +	iio_device_unregister(indio_dev);
>> +	iio_map_array_unregister(indio_dev);
> Slight disrepancy here between the cleanup in the error path in
> probe vs what we have in remove (next line doesn't exist in probe cleanup)
> Perhaps a comment here would make it clear why...
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);

SUNXI_GPADC_TP_INT_FIFO is the register in charge of activating hardware
interrupts. I disable all interrupts. In the probe, the interrupts are
already disabled (with disable_irq(irq)). This is actually just a
shortcut compared to "disable_irq(info->fifo_data_irq);
disable_irq(inf->temp_data_irq);".

[...]

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 19/07/16 09:33, Quentin Schulz wrote:
> On 18/07/2016 15:18, Jonathan Cameron wrote:
>> On 15/07/16 10:59, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. This patch adds the ADC driver which is
>>> based on the MFD for the same SoCs ADC.
>>>
>>> This also registers the thermal adc channel in the iio map array so
>>> iio_hwmon could use it without modifying the Device Tree.
>>>
>>> This driver probes on three different platform_device_id to take into
>>> account slight differences between Allwinner SoCs ADCs.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>
>> Hi Quentin,
>>
>> Various bits inline.  In particular the irq handling looks flakey / racey
>> to me.  Definitely need some explanatory comments.
>>
>> Also another note on the craziness that using extended_name to provide
>> the hwmon labels will cause :)
>>
>> Jonathan
> [...]
>>> +struct sunxi_gpadc_dev {
>>> +	void __iomem			*regs;
>>> +	struct completion		completion;
>>> +	int				temp_data;
>>> +	u32				adc_data;
>>> +	struct regmap			*regmap;
>>> +	unsigned int			fifo_data_irq;
>>> +	unsigned int			temp_data_irq;
>>> +	unsigned int			flags;
>> I'd prefer something more explicit than this.  Right now only one
>> bit can ever be set - indicating a particular chip family.  Why not
>> just have an enum instead?
> 
> ACK.
> 
> [...]
>>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>>> +	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
>>> +	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
>>> +	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
>>> +	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>>> +	{
>>> +		.type = IIO_TEMP,
>>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> +		.datasheet_name = "temp_adc",
>>> +		.extend_name = "SoC temperature",
>> Just curious, did you look at the resultling sysfs entries?
>> Going to be something like
>> in_temp_SoC\ Temperature_input... Not good.
> 
> Just encountered this after further testing, not good as you said.
> 
>> If there is a strong enough reason (and there may be) to add a 'help string'
>> type label to struct iio_chan_spec then my only real worries would be that
>> we would be adding a whole pile of ABI that would be in the, near impossible
>> to change in future, category...
> 
> I don't understand your "adding a whole pile of ABI" thing. 
Adding a 'free form' description field to the sysfs ABI of IIO is
nasty in that such things tend not to be as reviewed as they should
be and the moment an unclear / wrong statement is there, it's in
the kernel ABI and chances are someone will go and write a script
using it.  Userspace breakage ensues so we get stuck with garbage. 
>Same as
> datasheet_name variable: const char* in struct iio_chan_spec and used
> whenever needed with some NULL checks. This is the easiest way to do it.
> Or maybe you're thinking of adding an item to iio_chan_info_enum and all
> its underlying modifications? This could allow us to expose a _label
> sysfs file in the iio_device per type/channel.
As you suggest - new field in iio_chan_spec.
> 
> I understand the "near impossible to change in future" concern though.
> 
> [...]
>>> +	enable_irq(info->temp_data_irq);
>> Is this hardware spitting out extra irqs?  If not, much better to just
>> leave it enabled all the time and control whether it can occur or not
>> by controlling the device state..
> 
> The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in
> the current state of the driver: 2s). What do you mean by controlling
> the device state? Enabling or disabling the hardware part of the IP
> responsible of getting the temperature
> (SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)?
Yes, or something along those lines if it wakes up fast enough.
> Once the interrupt is activated, the IP periodically performs
> conversions. We don't really want interrupts to be activated when not
> needed.
> 
> [...]
>>> +
>>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>>> +		*val = info->temp_data * 133 - 257000;
>> Why report as processed?  I'd just report them as raw with the scale
>> and offset provided.  It's not a big thing, but if we can leave it so
>> that the conversion only occurs when desired, why not?
>>
>> For in kernel users, this all happen 'automagically' anyway ;)
>>
> 
> ACK.
> 
> [...]
>>> +	mutex_unlock(&indio_dev->mlock);
>> mlock has a very specific purpose - locking the state changes of
>> between 'buffered' (push) and poll modes. Don't use it for anything else.
>> In fact it's almost always a bug to access it directly at all.  We have
>> nice wrappers now for checking and locking the access mode.
>> Just have a local lock in your iio_priv structure.
> 
> ACK. There is still a lot of drivers using iio_dev mlock though.
> 
> [...]
>>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
>>> +{
>>> +	struct sunxi_gpadc_dev *info = dev_id;
>>> +	int ret;
>>> +
>>> +	ret = regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data);
>>> +	if (ret == 0)
>>> +		complete(&info->completion);
>> if (!regmap_read(...))?
>>
> 
> ACK.
> 
> [...]
>>> +
>>> +	info->temp_data_irq = irq;
>>> +	disable_irq(irq);
>> As below, I want a comment explaining why you are disabling the irq here.
>> This is clearly racey..
> 
> Once the interrupt is activated, the IP performs continuous conversions
> (temp_data_irq only periodically). I want these interrupts to be enabled
> only when I read the sysfs file or we get useless interrupts.
> In the current state of this driver's irq handlers, I only set values in
> structures and all the needed structures are already initialized before
> requesting irqs. So it does not look like a race. I can prevent races in
> future versions by adding an atomic flag if wanted.
> 
>>> +
>>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>>> +	if (irq < 0) {
>>> +		dev_err(&pdev->dev,
>>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>>> +		ret = irq;
>>> +		goto err;
>>> +	}
>>> +
>>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>>> +					   sunxi_gpadc_fifo_data_irq_handler,
>>> +					   0, "fifo_data", info);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev,
>>> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
>>> +			ret);
>>> +		goto err;
>>> +	}
>>> +
>>> +	info->fifo_data_irq = irq;
>> There's a race here - this may be the only way of doing it though.
>>
>> However, I want a comment explaining why here... Are we looking at a hardware
>> bug?
>>> +	disable_irq(irq);
>>> +
>>> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev, "failed to register iio map array\n");
>>> +		goto err;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, indio_dev);
>>> +
>>> +	ret = iio_device_register(indio_dev);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev, "could not register the device\n");
>>> +		iio_map_array_unregister(indio_dev);
>> Once you have an unwinding path (which is sensible here) put everything in
>> that block - it makes it easier to review (initially I compared that block
>> with the remove and thought you'd missed this error path).
>>
> 
> ACK. As suggested by Maxime, I will put that in a different goto label.
> 
>>> +		goto err;
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +err:
>>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
>>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>>> +{
>>> +	struct sunxi_gpadc_dev *info;
>>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +
>>> +	info = iio_priv(indio_dev);
>>> +	iio_device_unregister(indio_dev);
>>> +	iio_map_array_unregister(indio_dev);
>> Slight disrepancy here between the cleanup in the error path in
>> probe vs what we have in remove (next line doesn't exist in probe cleanup)
>> Perhaps a comment here would make it clear why...
>>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> 
> SUNXI_GPADC_TP_INT_FIFO is the register in charge of activating hardware
> interrupts. I disable all interrupts. In the probe, the interrupts are
> already disabled (with disable_irq(irq)). This is actually just a
> shortcut compared to "disable_irq(info->fifo_data_irq);
> disable_irq(inf->temp_data_irq);".
That disables reception of interrupts, I'd suggest it would be cleaner
to ensure you also disable their generation if an error occurs.
Basically it looks nicer that way ;)
> 
> [...]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 20/07/2016 16:57, Jonathan Cameron wrote:
> On 19/07/16 09:33, Quentin Schulz wrote:
>> On 18/07/2016 15:18, Jonathan Cameron wrote:
>>> On 15/07/16 10:59, Quentin Schulz wrote:
[...]
>>>> +	enable_irq(info->temp_data_irq);
>>> Is this hardware spitting out extra irqs?  If not, much better to just
>>> leave it enabled all the time and control whether it can occur or not
>>> by controlling the device state..
>>
>> The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in
>> the current state of the driver: 2s). What do you mean by controlling
>> the device state? Enabling or disabling the hardware part of the IP
>> responsible of getting the temperature
>> (SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)?
> Yes, or something along those lines if it wakes up fast enough.

The ADC wakes up fast enough but resets its internal time clock (I don't
know if it's the right term to use). Note that the temperature interrupt
occurs by period of X seconds in this IP.

This means that each time we disable the ADC on the hardware side, no
temperature interrupt will occur within the first X seconds. I don't
think this is what we want.

[...]

Quentin

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 21/07/16 14:15, Quentin Schulz wrote:
> On 20/07/2016 16:57, Jonathan Cameron wrote:
>> On 19/07/16 09:33, Quentin Schulz wrote:
>>> On 18/07/2016 15:18, Jonathan Cameron wrote:
>>>> On 15/07/16 10:59, Quentin Schulz wrote:
> [...]
>>>>> +	enable_irq(info->temp_data_irq);
>>>> Is this hardware spitting out extra irqs?  If not, much better to just
>>>> leave it enabled all the time and control whether it can occur or not
>>>> by controlling the device state..
>>>
>>> The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in
>>> the current state of the driver: 2s). What do you mean by controlling
>>> the device state? Enabling or disabling the hardware part of the IP
>>> responsible of getting the temperature
>>> (SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)?
>> Yes, or something along those lines if it wakes up fast enough.
> 
> The ADC wakes up fast enough but resets its internal time clock (I don't
> know if it's the right term to use). Note that the temperature interrupt
> occurs by period of X seconds in this IP.
> 
> This means that each time we disable the ADC on the hardware side, no
> temperature interrupt will occur within the first X seconds. I don't
> think this is what we want.
I'm guessing X is non trivial ;)

So fair enough.  Could you add this justification as a comment in the
driver somewhere so that people coming back to this in a few years time
will know what the justification for this 'unusual' handling is.

Thanks,

Jonathan
> 
> [...]
> 
> Quentin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 18/07/2016 15:18, Jonathan Cameron wrote:
[...]
>> +
>> +	if (!wait_for_completion_timeout(&info->completion,
>> +					 msecs_to_jiffies(100))) {
>> +		ret = -ETIMEDOUT;
>> +		goto out;
>> +	}
>> +
>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>> +		*val = info->temp_data * 133 - 257000;
> Why report as processed?  I'd just report them as raw with the scale
> and offset provided.  It's not a big thing, but if we can leave it so
> that the conversion only occurs when desired, why not?
> 
> For in kernel users, this all happen 'automagically' anyway ;)
> 

Mmmmh, in the code above we apply the scale on the raw value and then
the offset. While in iio_convert_raw_to_processed
(http://lxr.free-electrons.com/source/drivers/iio/inkern.c#L507), the
offset is applied before the scale.

The way would be to factorize the computation by scale:
Now: *val = raw * scale + offset
Then: *val = (raw + offset/scale) * scale

But the offset is an integer and offset/scale is therefore rounded.
Currently, we have the following values:
sun4i: -257000/133 = -1932.3308270676691
sun5i: -144700/100 = -1447
sun6i: -271000/167 = -1622.754491017964

Do we accept such rounding?

If not, we either stay with the processed value in read_raw or patch
inkern to add an offset to apply after having applied the scale to the
raw value (val2 from iio_channel_read is yet unused with
IIO_CHAN_INFO_OFFSET for example, we could use that to specify an
offset2 to apply after the switch(scale_type)-case).

[...]
Quentin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160720/b66e41b3/attachment.sig>

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Crt Mori]

On 20 July 2016 at 14:37, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> On 18/07/2016 15:18, Jonathan Cameron wrote:
> [...]
>>> +
>>> +    if (!wait_for_completion_timeout(&info->completion,
>>> +                                     msecs_to_jiffies(100))) {
>>> +            ret = -ETIMEDOUT;
>>> +            goto out;
>>> +    }
>>> +
>>> +    if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>>> +            *val = info->temp_data * 133 - 257000;
>> Why report as processed?  I'd just report them as raw with the scale
>> and offset provided.  It's not a big thing, but if we can leave it so
>> that the conversion only occurs when desired, why not?
>>
>> For in kernel users, this all happen 'automagically' anyway ;)
>>
>
> Mmmmh, in the code above we apply the scale on the raw value and then
> the offset. While in iio_convert_raw_to_processed
> (http://lxr.free-electrons.com/source/drivers/iio/inkern.c#L507), the
> offset is applied before the scale.
>
> The way would be to factorize the computation by scale:
> Now: *val = raw * scale + offset
> Then: *val = (raw + offset/scale) * scale
>
> But the offset is an integer and offset/scale is therefore rounded.
> Currently, we have the following values:
> sun4i: -257000/133 = -1932.3308270676691
> sun5i: -144700/100 = -1447
> sun6i: -271000/167 = -1622.754491017964
>
> Do we accept such rounding?
How much does this rounding bring as mistake on end result and is this
acceptable for you? You can always multiply it by 1000 or more to get
the precision you want out.
>
> If not, we either stay with the processed value in read_raw or patch
> inkern to add an offset to apply after having applied the scale to the
> raw value (val2 from iio_channel_read is yet unused with
> IIO_CHAN_INFO_OFFSET for example, we could use that to specify an
> offset2 to apply after the switch(scale_type)-case).
>
> [...]
> Quentin
>

[PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 20/07/16 13:37, Quentin Schulz wrote:
> On 18/07/2016 15:18, Jonathan Cameron wrote:
> [...]
>>> +
>>> +	if (!wait_for_completion_timeout(&info->completion,
>>> +					 msecs_to_jiffies(100))) {
>>> +		ret = -ETIMEDOUT;
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>>> +		*val = info->temp_data * 133 - 257000;
>> Why report as processed?  I'd just report them as raw with the scale
>> and offset provided.  It's not a big thing, but if we can leave it so
>> that the conversion only occurs when desired, why not?
>>
>> For in kernel users, this all happen 'automagically' anyway ;)
>>
> 
> Mmmmh, in the code above we apply the scale on the raw value and then
> the offset. While in iio_convert_raw_to_processed
> (http://lxr.free-electrons.com/source/drivers/iio/inkern.c#L507), the
> offset is applied before the scale.
> 
> The way would be to factorize the computation by scale:
> Now: *val = raw * scale + offset
> Then: *val = (raw + offset/scale) * scale
> 
> But the offset is an integer and offset/scale is therefore rounded.
> Currently, we have the following values:
> sun4i: -257000/133 = -1932.3308270676691
> sun5i: -144700/100 = -1447
> sun6i: -271000/167 = -1622.754491017964
> 
> Do we accept such rounding?
Yes - how accurate do you think a temp sensor is - really not a problem.
> 
> If not, we either stay with the processed value in read_raw or patch
> inkern to add an offset to apply after having applied the scale to the
> raw value (val2 from iio_channel_read is yet unused with
> IIO_CHAN_INFO_OFFSET for example, we could use that to specify an
> offset2 to apply after the switch(scale_type)-case).
I don't really care that much if you just want to keep them as
processed values.

J
> 
> [...]
> Quentin
> 

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Quentin Schulz]

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. For now, only the ADC and the thermal
sensor drivers are probed by the MFD, the touchscreen controller support
will be added later.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

v2:
 - add license headers,
 - reorder alphabetically includes,
 - add SUNXI_GPADC_ prefixes for defines,

 drivers/mfd/Kconfig                 |  14 +++
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
 4 files changed, 236 insertions(+)
 create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
 create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1bcf601..67b55d0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
 	  by the probe function of this MFD driver according to a device tree
 	  property.
 
+config MFD_SUNXI_ADC
+	tristate "ADC MFD core driver for sunxi platforms"
+	select MFD_CORE
+	select REGMAP_MMIO
+	help
+	  Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
+	  This driver will only map the hardware interrupt and registers, you
+	  have to select individual drivers based on this MFD to be able to use
+	  the ADC or the thermal sensor. This will try to probe the ADC driver
+	  sunxi-gpadc-iio and the hwmon driver iio_hwmon.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sunxi-gpadc-mfd.
+
 config MFD_ATMEL_HLCDC
 	tristate "Atmel HLCDC (High-end LCD Controller)"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 42a66e1..dcf43cd 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -201,6 +201,8 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
 obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
 obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
 
+obj-$(CONFIG_MFD_SUNXI_ADC)	+= sunxi-gpadc-mfd.o
+
 intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)	+= intel_soc_pmic_bxtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
new file mode 100644
index 0000000..f0005a6
--- /dev/null
+++ b/drivers/mfd/sunxi-gpadc-mfd.c
@@ -0,0 +1,197 @@
+/* ADC MFD core driver for sunxi platforms
+ *
+ * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/sunxi-gpadc-mfd.h>
+
+#define SUNXI_IRQ_FIFO_DATA	0
+#define SUNXI_IRQ_TEMP_DATA	1
+
+static struct resource adc_resources[] = {
+	{
+		.name	= "FIFO_DATA_PENDING",
+		.start	= SUNXI_IRQ_FIFO_DATA,
+		.end	= SUNXI_IRQ_FIFO_DATA,
+		.flags	= IORESOURCE_IRQ,
+	}, {
+		.name	= "TEMP_DATA_PENDING",
+		.start	= SUNXI_IRQ_TEMP_DATA,
+		.end	= SUNXI_IRQ_TEMP_DATA,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
+	REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
+	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
+};
+
+static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
+	.name = "sunxi_gpadc_mfd_irq_chip",
+	.status_base = SUNXI_GPADC_TP_INT_FIFOS,
+	.ack_base = SUNXI_GPADC_TP_INT_FIFOS,
+	.mask_base = SUNXI_GPADC_TP_INT_FIFOC,
+	.init_ack_masked = true,
+	.mask_invert = true,
+	.irqs = sunxi_gpadc_mfd_regmap_irq,
+	.num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
+	.num_regs = 1,
+};
+
+static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
+	{
+		.name	= "sun4i-a10-gpadc-iio",
+		.resources = adc_resources,
+		.num_resources = ARRAY_SIZE(adc_resources),
+	}, {
+		.name = "iio_hwmon",
+	}
+};
+
+static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
+	{
+		.name	= "sun5i-a13-gpadc-iio",
+		.resources = adc_resources,
+		.num_resources = ARRAY_SIZE(adc_resources),
+	}, {
+		.name = "iio_hwmon",
+	},
+};
+
+static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
+	{
+		.name	= "sun6i-a31-gpadc-iio",
+		.resources = adc_resources,
+		.num_resources = ARRAY_SIZE(adc_resources),
+	}, {
+		.name = "iio_hwmon",
+	},
+};
+
+static const struct regmap_config sunxi_gpadc_mfd_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
+	struct resource *mem = NULL;
+	unsigned int irq;
+	int ret;
+
+	sunxi_gpadc_mfd_dev = devm_kzalloc(&pdev->dev,
+					   sizeof(*sunxi_gpadc_mfd_dev),
+					   GFP_KERNEL);
+	if (!sunxi_gpadc_mfd_dev)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sunxi_gpadc_mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(sunxi_gpadc_mfd_dev->regs))
+		return PTR_ERR(sunxi_gpadc_mfd_dev->regs);
+
+	sunxi_gpadc_mfd_dev->dev = &pdev->dev;
+	dev_set_drvdata(sunxi_gpadc_mfd_dev->dev, sunxi_gpadc_mfd_dev);
+
+	sunxi_gpadc_mfd_dev->regmap =
+		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
+				      sunxi_gpadc_mfd_dev->regs,
+				      &sunxi_gpadc_mfd_regmap_config);
+	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
+		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
+		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
+		return ret;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
+				  IRQF_ONESHOT, 0,
+				  &sunxi_gpadc_mfd_regmap_irq_chip,
+				  &sunxi_gpadc_mfd_dev->regmap_irqc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
+		return ret;
+	}
+
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "allwinner,sun4i-a10-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun4i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+	else if (of_device_is_compatible(pdev->dev.of_node,
+					 "allwinner,sun5i-a13-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun5i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+	else if (of_device_is_compatible(pdev->dev.of_node,
+					 "allwinner,sun6i-a31-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun6i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
+		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "successfully loaded\n");
+
+	return 0;
+}
+
+static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
+	unsigned int irq;
+
+	irq = platform_get_irq(pdev, 0);
+	mfd_remove_devices(&pdev->dev);
+	sunxi_gpadc_mfd_dev = dev_get_drvdata(&pdev->dev);
+	regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-ts" },
+	{ .compatible = "allwinner,sun5i-a13-ts" },
+	{ .compatible = "allwinner,sun6i-a31-ts" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, sunxi_gpadc_mfd_of_match);
+
+static struct platform_driver sunxi_gpadc_mfd_driver = {
+	.driver = {
+		.name = "sunxi-adc-mfd",
+		.of_match_table = of_match_ptr(sunxi_gpadc_mfd_of_match),
+	},
+	.probe = sunxi_gpadc_mfd_probe,
+	.remove = sunxi_gpadc_mfd_remove,
+};
+
+module_platform_driver(sunxi_gpadc_mfd_driver);
+
+MODULE_DESCRIPTION("ADC MFD core driver for sunxi platforms");
+MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
new file mode 100644
index 0000000..7155845
--- /dev/null
+++ b/include/linux/mfd/sunxi-gpadc-mfd.h
@@ -0,0 +1,23 @@
+/* Header of ADC MFD core driver for sunxi platforms
+ *
+ * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#ifndef __SUNXI_GPADC_MFD__H__
+#define __SUNXI_GPADC_MFD__H__
+
+#define SUNXI_GPADC_TP_INT_FIFOC            0x10
+#define SUNXI_GPADC_TP_INT_FIFOS            0x14
+
+struct sunxi_gpadc_mfd_dev {
+	struct device			*dev;
+	struct regmap			*regmap;
+	struct regmap_irq_chip_data	*regmap_irqc;
+	void __iomem			*regs;
+};
+
+#endif
-- 
2.5.0

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Maxime Ripard]

On Fri, Jul 15, 2016 at 11:59:13AM +0200, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. For now, only the ADC and the thermal
> sensor drivers are probed by the MFD, the touchscreen controller support
> will be added later.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> v2:
>  - add license headers,
>  - reorder alphabetically includes,
>  - add SUNXI_GPADC_ prefixes for defines,
> 
>  drivers/mfd/Kconfig                 |  14 +++
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..67b55d0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
>  	  by the probe function of this MFD driver according to a device tree
>  	  property.
>  
> +config MFD_SUNXI_ADC
> +	tristate "ADC MFD core driver for sunxi platforms"
> +	select MFD_CORE
> +	select REGMAP_MMIO

It should also depends on the architectures supported (and probably COMPILE_TEST)

> +	help
> +	  Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
> +	  This driver will only map the hardware interrupt and registers, you
> +	  have to select individual drivers based on this MFD to be able to use
> +	  the ADC or the thermal sensor. This will try to probe the ADC driver
> +	  sunxi-gpadc-iio and the hwmon driver iio_hwmon.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sunxi-gpadc-mfd.
> +
>  config MFD_ATMEL_HLCDC
>  	tristate "Atmel HLCDC (High-end LCD Controller)"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..dcf43cd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -201,6 +201,8 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  
> +obj-$(CONFIG_MFD_SUNXI_ADC)	+= sunxi-gpadc-mfd.o
> +
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)	+= intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 0000000..f0005a6
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,197 @@
> +/* ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +
> +#define SUNXI_IRQ_FIFO_DATA	0
> +#define SUNXI_IRQ_TEMP_DATA	1
> +
> +static struct resource adc_resources[] = {
> +	{
> +		.name	= "FIFO_DATA_PENDING",
> +		.start	= SUNXI_IRQ_FIFO_DATA,
> +		.end	= SUNXI_IRQ_FIFO_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	}, {
> +		.name	= "TEMP_DATA_PENDING",
> +		.start	= SUNXI_IRQ_TEMP_DATA,
> +		.end	= SUNXI_IRQ_TEMP_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
> +	REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
> +	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> +	.name = "sunxi_gpadc_mfd_irq_chip",
> +	.status_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.ack_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.mask_base = SUNXI_GPADC_TP_INT_FIFOC,
> +	.init_ack_masked = true,
> +	.mask_invert = true,
> +	.irqs = sunxi_gpadc_mfd_regmap_irq,
> +	.num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
> +	.num_regs = 1,
> +};
> +
> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun4i-a10-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
> +	}
> +};
> +
> +static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun5i-a13-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
> +	},
> +};
> +
> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun6i-a31-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
> +	},
> +};
> +
> +static const struct regmap_config sunxi_gpadc_mfd_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
> +	struct resource *mem = NULL;
> +	unsigned int irq;
> +	int ret;
> +
> +	sunxi_gpadc_mfd_dev = devm_kzalloc(&pdev->dev,
> +					   sizeof(*sunxi_gpadc_mfd_dev),
> +					   GFP_KERNEL);
> +	if (!sunxi_gpadc_mfd_dev)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sunxi_gpadc_mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regs))
> +		return PTR_ERR(sunxi_gpadc_mfd_dev->regs);
> +
> +	sunxi_gpadc_mfd_dev->dev = &pdev->dev;
> +	dev_set_drvdata(sunxi_gpadc_mfd_dev->dev, sunxi_gpadc_mfd_dev);
> +
> +	sunxi_gpadc_mfd_dev->regmap =
> +		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
> +				      sunxi_gpadc_mfd_dev->regs,
> +				      &sunxi_gpadc_mfd_regmap_config);

This is usually on a single line (even if it exceeds 80 chars). Or
maybe you can use a shorter variable name (like dev, or mfd).

> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
> +		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
> +				  IRQF_ONESHOT, 0,
> +				  &sunxi_gpadc_mfd_regmap_irq_chip,
> +				  &sunxi_gpadc_mfd_dev->regmap_irqc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
> +		return ret;
> +	}

You should probably make sure that you clear all the interrupts before
enabling them.

> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "allwinner,sun4i-a10-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun4i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun5i-a13-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun5i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun6i-a31-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun6i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
> +				      0, NULL);

This huge if / else can be removed by putting those structures in the
data pointer of of_device_id.

> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "successfully loaded\n");
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +	unsigned int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	mfd_remove_devices(&pdev->dev);
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(&pdev->dev);
> +	regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-a10-ts" },
> +	{ .compatible = "allwinner,sun5i-a13-ts" },
> +	{ .compatible = "allwinner,sun6i-a31-ts" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxi_gpadc_mfd_of_match);
> +
> +static struct platform_driver sunxi_gpadc_mfd_driver = {
> +	.driver = {
> +		.name = "sunxi-adc-mfd",
> +		.of_match_table = of_match_ptr(sunxi_gpadc_mfd_of_match),
> +	},
> +	.probe = sunxi_gpadc_mfd_probe,
> +	.remove = sunxi_gpadc_mfd_remove,
> +};
> +
> +module_platform_driver(sunxi_gpadc_mfd_driver);
> +
> +MODULE_DESCRIPTION("ADC MFD core driver for sunxi platforms");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
> new file mode 100644
> index 0000000..7155845
> --- /dev/null
> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
> @@ -0,0 +1,23 @@
> +/* Header of ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#ifndef __SUNXI_GPADC_MFD__H__
> +#define __SUNXI_GPADC_MFD__H__
> +
> +#define SUNXI_GPADC_TP_INT_FIFOC            0x10
> +#define SUNXI_GPADC_TP_INT_FIFOS            0x14

Why do you declare only these two registers there?

> +
> +struct sunxi_gpadc_mfd_dev {
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regmap_irq_chip_data	*regmap_irqc;
> +	void __iomem			*regs;
> +};
> +
> +#endif
> -- 
> 2.5.0

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel 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/20160718/73343381/attachment.sig>

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 18/07/2016 15:02, Maxime Ripard wrote:
> On Fri, Jul 15, 2016 at 11:59:13AM +0200, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. For now, only the ADC and the thermal
>> sensor drivers are probed by the MFD, the touchscreen controller support
>> will be added later.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> ---
[...]
>> +config MFD_SUNXI_ADC
>> +	tristate "ADC MFD core driver for sunxi platforms"
>> +	select MFD_CORE
>> +	select REGMAP_MMIO
> 
> It should also depends on the architectures supported (and probably COMPILE_TEST)
> 

ACK.

[...]
>> +
>> +	sunxi_gpadc_mfd_dev->regmap =
>> +		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
>> +				      sunxi_gpadc_mfd_dev->regs,
>> +				      &sunxi_gpadc_mfd_regmap_config);
> 
> This is usually on a single line (even if it exceeds 80 chars). Or
> maybe you can use a shorter variable name (like dev, or mfd).
> 

I'll go with a shorter name.

>> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
>> +		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
>> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
>> +				  IRQF_ONESHOT, 0,
>> +				  &sunxi_gpadc_mfd_regmap_irq_chip,
>> +				  &sunxi_gpadc_mfd_dev->regmap_irqc);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
>> +		return ret;
>> +	}
> 
> You should probably make sure that you clear all the interrupts before
> enabling them.
> 

ACK. Thanks, didn't think of that.

>> +	if (of_device_is_compatible(pdev->dev.of_node,
>> +				    "allwinner,sun4i-a10-ts"))
>> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> +				      sun4i_gpadc_mfd_cells,
>> +				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
>> +				      0, NULL);
>> +	else if (of_device_is_compatible(pdev->dev.of_node,
>> +					 "allwinner,sun5i-a13-ts"))
>> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> +				      sun5i_gpadc_mfd_cells,
>> +				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
>> +				      0, NULL);
>> +	else if (of_device_is_compatible(pdev->dev.of_node,
>> +					 "allwinner,sun6i-a31-ts"))
>> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> +				      sun6i_gpadc_mfd_cells,
>> +				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
>> +				      0, NULL);
> 
> This huge if / else can be removed by putting those structures in the
> data pointer of of_device_id.
> 

Indeed. It is what I am using for the ADC driver, don't know why I
didn't think of this for the MFD as well.

[...]
>> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
>> new file mode 100644
>> index 0000000..7155845
>> --- /dev/null
>> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
>> @@ -0,0 +1,23 @@
>> +/* Header of ADC MFD core driver for sunxi platforms
>> + *
>> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#ifndef __SUNXI_GPADC_MFD__H__
>> +#define __SUNXI_GPADC_MFD__H__
>> +
>> +#define SUNXI_GPADC_TP_INT_FIFOC            0x10
>> +#define SUNXI_GPADC_TP_INT_FIFOS            0x14
> 
> Why do you declare only these two registers there?
> 

Because these are used by the MFD while the others not. Maybe it's
better to put all register and bit defines in sunxi-gpadc-mfd.h? Anyway,
just found out it would be clearer to use the defines for the interrupts
rather than directly "BIT(x)". So that makes two more defines here.
Should we put everything in sunxi-gpadc-mfd.h since the MFD is needed
for the ADC, (future) touchscreen and iio_hwmon drivers?


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160719/e8f07835/attachment.sig>

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 15/07/16 10:59, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. For now, only the ADC and the thermal
> sensor drivers are probed by the MFD, the touchscreen controller support
> will be added later.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Hmm. Previous patch includes the header this one creates.  Ordering issue?
The depends kind of prevents build failures by ensuring that can't be built
until this one is in place, but it is certainly an ugly way to do it.

Few little bits innline.
> ---
> 
> v2:
>  - add license headers,
>  - reorder alphabetically includes,
>  - add SUNXI_GPADC_ prefixes for defines,
> 
>  drivers/mfd/Kconfig                 |  14 +++
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..67b55d0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
>  	  by the probe function of this MFD driver according to a device tree
>  	  property.
>  
> +config MFD_SUNXI_ADC
> +	tristate "ADC MFD core driver for sunxi platforms"
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	help
> +	  Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
> +	  This driver will only map the hardware interrupt and registers, you
> +	  have to select individual drivers based on this MFD to be able to use
> +	  the ADC or the thermal sensor. This will try to probe the ADC driver
> +	  sunxi-gpadc-iio and the hwmon driver iio_hwmon.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sunxi-gpadc-mfd.
> +
>  config MFD_ATMEL_HLCDC
>  	tristate "Atmel HLCDC (High-end LCD Controller)"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..dcf43cd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -201,6 +201,8 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  
> +obj-$(CONFIG_MFD_SUNXI_ADC)	+= sunxi-gpadc-mfd.o
> +
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)	+= intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 0000000..f0005a6
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,197 @@
> +/* ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +
> +#define SUNXI_IRQ_FIFO_DATA	0
> +#define SUNXI_IRQ_TEMP_DATA	1
> +
> +static struct resource adc_resources[] = {
> +	{
> +		.name	= "FIFO_DATA_PENDING",
> +		.start	= SUNXI_IRQ_FIFO_DATA,
> +		.end	= SUNXI_IRQ_FIFO_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	}, {
> +		.name	= "TEMP_DATA_PENDING",
> +		.start	= SUNXI_IRQ_TEMP_DATA,
> +		.end	= SUNXI_IRQ_TEMP_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
> +	REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
> +	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> +	.name = "sunxi_gpadc_mfd_irq_chip",
> +	.status_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.ack_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.mask_base = SUNXI_GPADC_TP_INT_FIFOC,
> +	.init_ack_masked = true,
> +	.mask_invert = true,
> +	.irqs = sunxi_gpadc_mfd_regmap_irq,
> +	.num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
> +	.num_regs = 1,
> +};
> +
> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun4i-a10-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
> +	}
> +};
> +
> +static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun5i-a13-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
> +	},
> +};
> +
> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun6i-a31-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
I still really dislike using this to force the probe of that driver but
kind of up to the hwmon / mfd guys on this.

I don't have any better suggestions though..
> +	},
> +};
> +
> +static const struct regmap_config sunxi_gpadc_mfd_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
> +	struct resource *mem = NULL;
Neither of the above assignments is necessary as both will be explicitly
assigned before they are otherwise used.
> +	unsigned int irq;
> +	int ret;
> +
> +	sunxi_gpadc_mfd_dev = devm_kzalloc(&pdev->dev,
> +					   sizeof(*sunxi_gpadc_mfd_dev),
> +					   GFP_KERNEL);
> +	if (!sunxi_gpadc_mfd_dev)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sunxi_gpadc_mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regs))
> +		return PTR_ERR(sunxi_gpadc_mfd_dev->regs);
> +
> +	sunxi_gpadc_mfd_dev->dev = &pdev->dev;
> +	dev_set_drvdata(sunxi_gpadc_mfd_dev->dev, sunxi_gpadc_mfd_dev);
> +
> +	sunxi_gpadc_mfd_dev->regmap =
> +		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
> +				      sunxi_gpadc_mfd_dev->regs,
> +				      &sunxi_gpadc_mfd_regmap_config);
> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
> +		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
> +				  IRQF_ONESHOT, 0,
> +				  &sunxi_gpadc_mfd_regmap_irq_chip,
> +				  &sunxi_gpadc_mfd_dev->regmap_irqc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "allwinner,sun4i-a10-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun4i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun5i-a13-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun5i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun6i-a31-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun6i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "successfully loaded\n");
Seems like noise to me, but not my subsystem :)
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +	unsigned int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	mfd_remove_devices(&pdev->dev);
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(&pdev->dev);
> +	regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-a10-ts" },
> +	{ .compatible = "allwinner,sun5i-a13-ts" },
> +	{ .compatible = "allwinner,sun6i-a31-ts" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxi_gpadc_mfd_of_match);
> +
> +static struct platform_driver sunxi_gpadc_mfd_driver = {
> +	.driver = {
> +		.name = "sunxi-adc-mfd",
> +		.of_match_table = of_match_ptr(sunxi_gpadc_mfd_of_match),
> +	},
> +	.probe = sunxi_gpadc_mfd_probe,
> +	.remove = sunxi_gpadc_mfd_remove,
> +};
> +
> +module_platform_driver(sunxi_gpadc_mfd_driver);
> +
> +MODULE_DESCRIPTION("ADC MFD core driver for sunxi platforms");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
> new file mode 100644
> index 0000000..7155845
> --- /dev/null
> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
> @@ -0,0 +1,23 @@
> +/* Header of ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#ifndef __SUNXI_GPADC_MFD__H__
> +#define __SUNXI_GPADC_MFD__H__
> +
> +#define SUNXI_GPADC_TP_INT_FIFOC            0x10
> +#define SUNXI_GPADC_TP_INT_FIFOS            0x14
> +
> +struct sunxi_gpadc_mfd_dev {
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regmap_irq_chip_data	*regmap_irqc;
> +	void __iomem			*regs;
> +};
> +
> +#endif
> 

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Lee Jones]

On Mon, 18 Jul 2016, Jonathan Cameron wrote:

> On 15/07/16 10:59, Quentin Schulz wrote:
> > The Allwinner SoCs all have an ADC that can also act as a touchscreen
> > controller and a thermal sensor. For now, only the ADC and the thermal
> > sensor drivers are probed by the MFD, the touchscreen controller support
> > will be added later.
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> The depends kind of prevents build failures by ensuring that can't be built
> until this one is in place, but it is certainly an ugly way to do it.
> 
> Few little bits innline.
> > ---
> > 
> > v2:
> >  - add license headers,
> >  - reorder alphabetically includes,
> >  - add SUNXI_GPADC_ prefixes for defines,
> > 
> >  drivers/mfd/Kconfig                 |  14 +++
> >  drivers/mfd/Makefile                |   2 +
> >  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> >  4 files changed, 236 insertions(+)
> >  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> >  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h

[...]

> > +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> > +	{
> > +		.name	= "sun6i-a31-gpadc-iio",
> > +		.resources = adc_resources,
> > +		.num_resources = ARRAY_SIZE(adc_resources),
> > +	}, {
> > +		.name = "iio_hwmon",
> I still really dislike using this to force the probe of that driver but
> kind of up to the hwmon / mfd guys on this.

Can you at least say *why* you don't like it?

How else would it get probed?

> I don't have any better suggestions though..
> > +	},
> > +};

[...]

> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> > +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> > +		return ret;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "successfully loaded\n");
> Seems like noise to me, but not my subsystem :)

Agreed, I don't allow this either.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 19/07/16 08:31, Lee Jones wrote:
> On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> 
>> On 15/07/16 10:59, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. For now, only the ADC and the thermal
>>> sensor drivers are probed by the MFD, the touchscreen controller support
>>> will be added later.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> Hmm. Previous patch includes the header this one creates.  Ordering issue?
>> The depends kind of prevents build failures by ensuring that can't be built
>> until this one is in place, but it is certainly an ugly way to do it.
>>
>> Few little bits innline.
>>> ---
>>>
>>> v2:
>>>  - add license headers,
>>>  - reorder alphabetically includes,
>>>  - add SUNXI_GPADC_ prefixes for defines,
>>>
>>>  drivers/mfd/Kconfig                 |  14 +++
>>>  drivers/mfd/Makefile                |   2 +
>>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
>>>  4 files changed, 236 insertions(+)
>>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> 
> [...]
> 
>>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
>>> +	{
>>> +		.name	= "sun6i-a31-gpadc-iio",
>>> +		.resources = adc_resources,
>>> +		.num_resources = ARRAY_SIZE(adc_resources),
>>> +	}, {
>>> +		.name = "iio_hwmon",
>> I still really dislike using this to force the probe of that driver but
>> kind of up to the hwmon / mfd guys on this.
> 
> Can you at least say *why* you don't like it?
It just feels odd to have an mfd child that isn't really dependent
on the mfd hardware itself.

Still if you are happy, mfd is your domain and my objections were
as you probably noticed not that strong - or well described!)

So I'm fine with this.
> 
> How else would it get probed?
> 
>> I don't have any better suggestions though..
>>> +	},
>>> +};
> 
> [...]
> 
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
>>> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
>>> +		return ret;
>>> +	}
>>> +
>>> +	dev_info(&pdev->dev, "successfully loaded\n");
>> Seems like noise to me, but not my subsystem :)
> 
> Agreed, I don't allow this either.
> 
> [...]
> 

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Lee Jones]

On Wed, 20 Jul 2016, Jonathan Cameron wrote:

> On 19/07/16 08:31, Lee Jones wrote:
> > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > 
> >> On 15/07/16 10:59, Quentin Schulz wrote:
> >>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> >>> controller and a thermal sensor. For now, only the ADC and the thermal
> >>> sensor drivers are probed by the MFD, the touchscreen controller support
> >>> will be added later.
> >>>
> >>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> >> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> >> The depends kind of prevents build failures by ensuring that can't be built
> >> until this one is in place, but it is certainly an ugly way to do it.
> >>
> >> Few little bits innline.
> >>> ---
> >>>
> >>> v2:
> >>>  - add license headers,
> >>>  - reorder alphabetically includes,
> >>>  - add SUNXI_GPADC_ prefixes for defines,
> >>>
> >>>  drivers/mfd/Kconfig                 |  14 +++
> >>>  drivers/mfd/Makefile                |   2 +
> >>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> >>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> >>>  4 files changed, 236 insertions(+)
> >>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> >>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > 
> > [...]
> > 
> >>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> >>> +	{
> >>> +		.name	= "sun6i-a31-gpadc-iio",
> >>> +		.resources = adc_resources,
> >>> +		.num_resources = ARRAY_SIZE(adc_resources),
> >>> +	}, {
> >>> +		.name = "iio_hwmon",
> >> I still really dislike using this to force the probe of that driver but
> >> kind of up to the hwmon / mfd guys on this.
> > 
> > Can you at least say *why* you don't like it?
> It just feels odd to have an mfd child that isn't really dependent
> on the mfd hardware itself.
> 
> Still if you are happy, mfd is your domain and my objections were
> as you probably noticed not that strong - or well described!)
> 
> So I'm fine with this.

I see.  So it's not actually part of the same IP/chip?

> > How else would it get probed?
> > 
> >> I don't have any better suggestions though..
> >>> +	},
> >>> +};
> > 
> > [...]
> > 
> >>> +	if (ret) {
> >>> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> >>> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	dev_info(&pdev->dev, "successfully loaded\n");
> >> Seems like noise to me, but not my subsystem :)
> > 
> > Agreed, I don't allow this either.
> > 
> > [...]
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Maxime Ripard]

1;4205;0c
On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> 
> > On 19/07/16 08:31, Lee Jones wrote:
> > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > > 
> > >> On 15/07/16 10:59, Quentin Schulz wrote:
> > >>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> > >>> controller and a thermal sensor. For now, only the ADC and the thermal
> > >>> sensor drivers are probed by the MFD, the touchscreen controller support
> > >>> will be added later.
> > >>>
> > >>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > >> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> > >> The depends kind of prevents build failures by ensuring that can't be built
> > >> until this one is in place, but it is certainly an ugly way to do it.
> > >>
> > >> Few little bits innline.
> > >>> ---
> > >>>
> > >>> v2:
> > >>>  - add license headers,
> > >>>  - reorder alphabetically includes,
> > >>>  - add SUNXI_GPADC_ prefixes for defines,
> > >>>
> > >>>  drivers/mfd/Kconfig                 |  14 +++
> > >>>  drivers/mfd/Makefile                |   2 +
> > >>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> > >>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> > >>>  4 files changed, 236 insertions(+)
> > >>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > >>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > > 
> > > [...]
> > > 
> > >>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> > >>> +	{
> > >>> +		.name	= "sun6i-a31-gpadc-iio",
> > >>> +		.resources = adc_resources,
> > >>> +		.num_resources = ARRAY_SIZE(adc_resources),
> > >>> +	}, {
> > >>> +		.name = "iio_hwmon",
> > >> I still really dislike using this to force the probe of that driver but
> > >> kind of up to the hwmon / mfd guys on this.
> > > 
> > > Can you at least say *why* you don't like it?
> > It just feels odd to have an mfd child that isn't really dependent
> > on the mfd hardware itself.
> > 
> > Still if you are happy, mfd is your domain and my objections were
> > as you probably noticed not that strong - or well described!)
> > 
> > So I'm fine with this.
> 
> I see.  So it's not actually part of the same IP/chip?

The chip has a temperature sensor, and we want to expose that
temperature through IIO.

But we don't really have the choice on how we probe iio-hwmon
here. The binding was already there, and we have to keep it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel 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/20160721/22b492d8/attachment.sig>

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Lee Jones]

On Thu, 21 Jul 2016, Maxime Ripard wrote:

> 1;4205;0c
> On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> > On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> > 
> > > On 19/07/16 08:31, Lee Jones wrote:
> > > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > > > 
> > > >> On 15/07/16 10:59, Quentin Schulz wrote:
> > > >>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> > > >>> controller and a thermal sensor. For now, only the ADC and the thermal
> > > >>> sensor drivers are probed by the MFD, the touchscreen controller support
> > > >>> will be added later.
> > > >>>
> > > >>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > > >> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> > > >> The depends kind of prevents build failures by ensuring that can't be built
> > > >> until this one is in place, but it is certainly an ugly way to do it.
> > > >>
> > > >> Few little bits innline.
> > > >>> ---
> > > >>>
> > > >>> v2:
> > > >>>  - add license headers,
> > > >>>  - reorder alphabetically includes,
> > > >>>  - add SUNXI_GPADC_ prefixes for defines,
> > > >>>
> > > >>>  drivers/mfd/Kconfig                 |  14 +++
> > > >>>  drivers/mfd/Makefile                |   2 +
> > > >>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> > > >>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> > > >>>  4 files changed, 236 insertions(+)
> > > >>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > > >>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > > > 
> > > > [...]
> > > > 
> > > >>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> > > >>> +	{
> > > >>> +		.name	= "sun6i-a31-gpadc-iio",
> > > >>> +		.resources = adc_resources,
> > > >>> +		.num_resources = ARRAY_SIZE(adc_resources),
> > > >>> +	}, {
> > > >>> +		.name = "iio_hwmon",
> > > >> I still really dislike using this to force the probe of that driver but
> > > >> kind of up to the hwmon / mfd guys on this.
> > > > 
> > > > Can you at least say *why* you don't like it?
> > > It just feels odd to have an mfd child that isn't really dependent
> > > on the mfd hardware itself.
> > > 
> > > Still if you are happy, mfd is your domain and my objections were
> > > as you probably noticed not that strong - or well described!)
> > > 
> > > So I'm fine with this.
> > 
> > I see.  So it's not actually part of the same IP/chip?
> 
> The chip has a temperature sensor, and we want to expose that
> temperature through IIO.
> 
> But we don't really have the choice on how we probe iio-hwmon
> here. The binding was already there, and we have to keep it.

What binding?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 22/07/16 15:55, Lee Jones wrote:
> On Thu, 21 Jul 2016, Maxime Ripard wrote:
> 
>> 1;4205;0c
>> On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
>>> On Wed, 20 Jul 2016, Jonathan Cameron wrote:
>>>
>>>> On 19/07/16 08:31, Lee Jones wrote:
>>>>> On Mon, 18 Jul 2016, Jonathan Cameron wrote:
>>>>>
>>>>>> On 15/07/16 10:59, Quentin Schulz wrote:
>>>>>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>>>>>> controller and a thermal sensor. For now, only the ADC and the thermal
>>>>>>> sensor drivers are probed by the MFD, the touchscreen controller support
>>>>>>> will be added later.
>>>>>>>
>>>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>>>> Hmm. Previous patch includes the header this one creates.  Ordering issue?
>>>>>> The depends kind of prevents build failures by ensuring that can't be built
>>>>>> until this one is in place, but it is certainly an ugly way to do it.
>>>>>>
>>>>>> Few little bits innline.
>>>>>>> ---
>>>>>>>
>>>>>>> v2:
>>>>>>>  - add license headers,
>>>>>>>  - reorder alphabetically includes,
>>>>>>>  - add SUNXI_GPADC_ prefixes for defines,
>>>>>>>
>>>>>>>  drivers/mfd/Kconfig                 |  14 +++
>>>>>>>  drivers/mfd/Makefile                |   2 +
>>>>>>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
>>>>>>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
>>>>>>>  4 files changed, 236 insertions(+)
>>>>>>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>>>>>>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
>>>>>
>>>>> [...]
>>>>>
>>>>>>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
>>>>>>> +	{
>>>>>>> +		.name	= "sun6i-a31-gpadc-iio",
>>>>>>> +		.resources = adc_resources,
>>>>>>> +		.num_resources = ARRAY_SIZE(adc_resources),
>>>>>>> +	}, {
>>>>>>> +		.name = "iio_hwmon",
>>>>>> I still really dislike using this to force the probe of that driver but
>>>>>> kind of up to the hwmon / mfd guys on this.
>>>>>
>>>>> Can you at least say *why* you don't like it?
>>>> It just feels odd to have an mfd child that isn't really dependent
>>>> on the mfd hardware itself.
>>>>
>>>> Still if you are happy, mfd is your domain and my objections were
>>>> as you probably noticed not that strong - or well described!)
>>>>
>>>> So I'm fine with this.
>>>
>>> I see.  So it's not actually part of the same IP/chip?
>>
>> The chip has a temperature sensor, and we want to expose that
>> temperature through IIO.
>>
>> But we don't really have the choice on how we probe iio-hwmon
>> here. The binding was already there, and we have to keep it.
> 
> What binding?
> 
The somewhat 'interesting' one for iio-hwmon I would guess is what
Maxime is referring to.  Here they are using what was effectively
the old 'board file' methods inside the driver itself.  Which is
fine and done in quite a few places - though we have some issues
with deferred probing that we can improve upon.

Anyhow, it's real hardware and as this doesn't have anything to
do with device tree there is no need to get around the objections
of lack of generality that bothers the dt maintainers :)

Lets go with it as it is. Sorry for confusing everyone!

p.s. I'm pitching a tech session for the KS at the moment
on ksummit-discuss to hit the fiddlier question of bindings
/ cross subsystem interactions if anyone is interested.

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Maxime Ripard]

On Fri, Jul 22, 2016 at 02:55:56PM +0100, Lee Jones wrote:
> On Thu, 21 Jul 2016, Maxime Ripard wrote:
> 
> > 1;4205;0c
> > On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> > > On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> > > 
> > > > On 19/07/16 08:31, Lee Jones wrote:
> > > > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > > > > 
> > > > >> On 15/07/16 10:59, Quentin Schulz wrote:
> > > > >>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> > > > >>> controller and a thermal sensor. For now, only the ADC and the thermal
> > > > >>> sensor drivers are probed by the MFD, the touchscreen controller support
> > > > >>> will be added later.
> > > > >>>
> > > > >>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > > > >> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> > > > >> The depends kind of prevents build failures by ensuring that can't be built
> > > > >> until this one is in place, but it is certainly an ugly way to do it.
> > > > >>
> > > > >> Few little bits innline.
> > > > >>> ---
> > > > >>>
> > > > >>> v2:
> > > > >>>  - add license headers,
> > > > >>>  - reorder alphabetically includes,
> > > > >>>  - add SUNXI_GPADC_ prefixes for defines,
> > > > >>>
> > > > >>>  drivers/mfd/Kconfig                 |  14 +++
> > > > >>>  drivers/mfd/Makefile                |   2 +
> > > > >>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> > > > >>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> > > > >>>  4 files changed, 236 insertions(+)
> > > > >>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > > > >>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > > > > 
> > > > > [...]
> > > > > 
> > > > >>> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> > > > >>> +	{
> > > > >>> +		.name	= "sun6i-a31-gpadc-iio",
> > > > >>> +		.resources = adc_resources,
> > > > >>> +		.num_resources = ARRAY_SIZE(adc_resources),
> > > > >>> +	}, {
> > > > >>> +		.name = "iio_hwmon",
> > > > >> I still really dislike using this to force the probe of that driver but
> > > > >> kind of up to the hwmon / mfd guys on this.
> > > > > 
> > > > > Can you at least say *why* you don't like it?
> > > > It just feels odd to have an mfd child that isn't really dependent
> > > > on the mfd hardware itself.
> > > > 
> > > > Still if you are happy, mfd is your domain and my objections were
> > > > as you probably noticed not that strong - or well described!)
> > > > 
> > > > So I'm fine with this.
> > > 
> > > I see.  So it's not actually part of the same IP/chip?
> > 
> > The chip has a temperature sensor, and we want to expose that
> > temperature through IIO.
> > 
> > But we don't really have the choice on how we probe iio-hwmon
> > here. The binding was already there, and we have to keep it.
> 
> What binding?

The binding introduced for the original driver this serie aims to
replace:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/input/touchscreen/sun4i.txt

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel 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/20160725/e0612716/attachment.sig>

[PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 18/07/2016 15:25, Jonathan Cameron wrote:
> On 15/07/16 10:59, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. For now, only the ADC and the thermal
>> sensor drivers are probed by the MFD, the touchscreen controller support
>> will be added later.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> The depends kind of prevents build failures by ensuring that can't be built
> until this one is in place, but it is certainly an ugly way to do it.
> 
> Few little bits innline.
[...]
>> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
>> +{
>> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
>> +	struct resource *mem = NULL;
> Neither of the above assignments is necessary as both will be explicitly
> assigned before they are otherwise used.

ACK.

[...]
>> +	dev_info(&pdev->dev, "successfully loaded\n");
> Seems like noise to me, but not my subsystem :)

ACK.
[...]

[PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Quentin Schulz]

Currently, iio_hwmon only exposes values of the IIO channels it can read
but no label by channel is exposed.

This adds exposition of sysfs files containing label for IIO channels it
can read based on extended_name field of the iio_chan_spec of the channel.
If the extended_name field is empty, the sysfs file is not created by
iio_hwmon.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

patch added in v2

 drivers/hwmon/iio_hwmon.c | 77 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 68 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index c0da4d9..28d15b2 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
 #include <linux/iio/types.h>
 
 /**
@@ -57,12 +58,26 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
 	return sprintf(buf, "%d\n", result);
 }
 
+static ssize_t iio_hwmon_read_label(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
+	struct iio_hwmon_state *state = dev_get_drvdata(dev);
+	const char *label = state->channels[sattr->index].channel->extend_name;
+
+	if (label)
+		return sprintf(buf, "%s\n", label);
+
+	return 0;
+}
+
 static int iio_hwmon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct iio_hwmon_state *st;
-	struct sensor_device_attribute *a;
-	int ret, i;
+	struct sensor_device_attribute *a, *b;
+	int ret, i, j = 0;
 	int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1;
 	enum iio_chan_type type;
 	struct iio_channel *channels;
@@ -92,7 +107,8 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		st->num_channels++;
 
 	st->attrs = devm_kzalloc(dev,
-				 sizeof(*st->attrs) * (st->num_channels + 1),
+				 sizeof(*st->attrs) *
+				 (2 * st->num_channels + 1),
 				 GFP_KERNEL);
 	if (st->attrs == NULL) {
 		ret = -ENOMEM;
@@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		}
 
 		sysfs_attr_init(&a->dev_attr.attr);
+
+		b = NULL;
+		if (st->channels[i].channel->extend_name) {
+			b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
+			if (b == NULL) {
+				ret = -ENOMEM;
+				goto error_release_channels;
+			}
+
+			sysfs_attr_init(&b->dev_attr.attr);
+		}
+
 		ret = iio_get_channel_type(&st->channels[i], &type);
 		if (ret < 0)
 			goto error_release_channels;
@@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		case IIO_VOLTAGE:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
 							  "in%d_input",
-							  in_i++);
+							  in_i);
+			if (b)
+				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+								  "in%d_label",
+								  in_i);
+			in_i++;
 			break;
 		case IIO_TEMP:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
 							  "temp%d_input",
-							  temp_i++);
+							  temp_i);
+
+			if (b)
+				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+								  "temp%d_label",
+								  temp_i);
+			temp_i++;
 			break;
 		case IIO_CURRENT:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
 							  "curr%d_input",
-							  curr_i++);
+							  curr_i);
+
+			if (b)
+				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+								  "curr%d_label",
+								  curr_i);
+			curr_i++;
 			break;
 		case IIO_HUMIDITYRELATIVE:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
 							  "humidity%d_input",
-							  humidity_i++);
+							  humidity_i);
+
+			if (b)
+				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+								  "humidity%d_label",
+								  humidity_i);
+			humidity_i++;
 			break;
 		default:
 			ret = -EINVAL;
 			goto error_release_channels;
 		}
-		if (a->dev_attr.attr.name == NULL) {
+		if (a->dev_attr.attr.name == NULL ||
+		    (b && b->dev_attr.attr.name == NULL)) {
 			ret = -ENOMEM;
 			goto error_release_channels;
 		}
 		a->dev_attr.show = iio_hwmon_read_val;
 		a->dev_attr.attr.mode = S_IRUGO;
 		a->index = i;
-		st->attrs[i] = &a->dev_attr.attr;
+		st->attrs[j++] = &a->dev_attr.attr;
+
+		if (b) {
+			b->dev_attr.show = iio_hwmon_read_label;
+			b->dev_attr.attr.mode = S_IRUGO;
+			b->index = i;
+			st->attrs[j++] = &b->dev_attr.attr;
+		}
 	}
 
 	st->attr_group.attrs = st->attrs;
-- 
2.5.0

[PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Guenter Roeck]

On 07/15/2016 02:59 AM, Quentin Schulz wrote:
> Currently, iio_hwmon only exposes values of the IIO channels it can read
> but no label by channel is exposed.
>
> This adds exposition of sysfs files containing label for IIO channels it
> can read based on extended_name field of the iio_chan_spec of the channel.
> If the extended_name field is empty, the sysfs file is not created by
> iio_hwmon.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>
> patch added in v2
>
>   drivers/hwmon/iio_hwmon.c | 77 +++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index c0da4d9..28d15b2 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -16,6 +16,7 @@
>   #include <linux/of.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
>   #include <linux/iio/types.h>
>
>   /**
> @@ -57,12 +58,26 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
>   	return sprintf(buf, "%d\n", result);
>   }
>
> +static ssize_t iio_hwmon_read_label(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
> +	const char *label = state->channels[sattr->index].channel->extend_name;
> +
> +	if (label)
> +		return sprintf(buf, "%s\n", label);
> +
Can the name disappear on the fly, or be changed on the fly ?
Then this is unusable. We should and can only provide labels
if a name exists and is permanent. Otherwise all we do is
to confuse user space.

> +	return 0;
> +}
> +
>   static int iio_hwmon_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct iio_hwmon_state *st;
> -	struct sensor_device_attribute *a;
> -	int ret, i;
> +	struct sensor_device_attribute *a, *b;
> +	int ret, i, j = 0;
>   	int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1;
>   	enum iio_chan_type type;
>   	struct iio_channel *channels;
> @@ -92,7 +107,8 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		st->num_channels++;
>
>   	st->attrs = devm_kzalloc(dev,
> -				 sizeof(*st->attrs) * (st->num_channels + 1),
> +				 sizeof(*st->attrs) *
> +				 (2 * st->num_channels + 1),
>   				 GFP_KERNEL);
>   	if (st->attrs == NULL) {
>   		ret = -ENOMEM;
> @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		}
>
>   		sysfs_attr_init(&a->dev_attr.attr);
> +
> +		b = NULL;
> +		if (st->channels[i].channel->extend_name) {
> +			b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
> +			if (b == NULL) {
> +				ret = -ENOMEM;
> +				goto error_release_channels;
> +			}
> +
> +			sysfs_attr_init(&b->dev_attr.attr);

Why is this initialization here and not with the rest of the initialization
of this attribute ?

> +		}
> +
>   		ret = iio_get_channel_type(&st->channels[i], &type);
>   		if (ret < 0)
>   			goto error_release_channels;
> @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		case IIO_VOLTAGE:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "in%d_input",
> -							  in_i++);
> +							  in_i);
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "in%d_label",
> +								  in_i);
> +			in_i++;
>   			break;
>   		case IIO_TEMP:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "temp%d_input",
> -							  temp_i++);
> +							  temp_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "temp%d_label",
> +								  temp_i);
> +			temp_i++;
>   			break;
>   		case IIO_CURRENT:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "curr%d_input",
> -							  curr_i++);
> +							  curr_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "curr%d_label",
> +								  curr_i);
> +			curr_i++;
>   			break;
>   		case IIO_HUMIDITYRELATIVE:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "humidity%d_input",
> -							  humidity_i++);
> +							  humidity_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "humidity%d_label",
> +								  humidity_i);
> +			humidity_i++;
>   			break;
>   		default:
>   			ret = -EINVAL;
>   			goto error_release_channels;
>   		}
> -		if (a->dev_attr.attr.name == NULL) {
> +		if (a->dev_attr.attr.name == NULL ||
> +		    (b && b->dev_attr.attr.name == NULL)) {
>   			ret = -ENOMEM;
>   			goto error_release_channels;
>   		}

Just realized that we have a memory leak here. The 'name' memory is never released.

>   		a->dev_attr.show = iio_hwmon_read_val;
>   		a->dev_attr.attr.mode = S_IRUGO;
>   		a->index = i;
> -		st->attrs[i] = &a->dev_attr.attr;
> +		st->attrs[j++] = &a->dev_attr.attr;
> +
> +		if (b) {
> +			b->dev_attr.show = iio_hwmon_read_label;
> +			b->dev_attr.attr.mode = S_IRUGO;
> +			b->index = i;
> +			st->attrs[j++] = &b->dev_attr.attr;
> +		}
>   	}
>
>   	st->attr_group.attrs = st->attrs;
>

[PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Quentin Schulz]

On 15/07/2016 16:03, Guenter Roeck wrote:
> On 07/15/2016 02:59 AM, Quentin Schulz wrote:
[...]
>> +static ssize_t iio_hwmon_read_label(struct device *dev,
>> +                    struct device_attribute *attr,
>> +                    char *buf)
>> +{
>> +    struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>> +    struct iio_hwmon_state *state = dev_get_drvdata(dev);
>> +    const char *label =
>> state->channels[sattr->index].channel->extend_name;
>> +
>> +    if (label)
>> +        return sprintf(buf, "%s\n", label);
>> +
> Can the name disappear on the fly, or be changed on the fly ?
> Then this is unusable. We should and can only provide labels
> if a name exists and is permanent. Otherwise all we do is
> to confuse user space.

It cannot, the extend_name field is const char* in the struct:
http://lxr.free-electrons.com/source/include/linux/iio/iio.h#L247

[...]
>> @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device
>> *pdev)
>>           }
>>
>>           sysfs_attr_init(&a->dev_attr.attr);
>> +
>> +        b = NULL;
>> +        if (st->channels[i].channel->extend_name) {
>> +            b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
>> +            if (b == NULL) {
>> +                ret = -ENOMEM;
>> +                goto error_release_channels;
>> +            }
>> +
>> +            sysfs_attr_init(&b->dev_attr.attr);
> 
> Why is this initialization here and not with the rest of the initialization
> of this attribute ?

I don't get your question. I've followed the exact same pattern as for
"a" variable's initialization.
The initialization is before the switch case because the name of the
exposed sysfs file depends on the type of the IIO channel. If I move the
initialization in the switch case, I'll have duplicated code.

>> +        }
>> +
>>           ret = iio_get_channel_type(&st->channels[i], &type);
>>           if (ret < 0)
>>               goto error_release_channels;
>> @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct
>> platform_device *pdev)
>>           case IIO_VOLTAGE:
>>               a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>                                 "in%d_input",
>> -                              in_i++);
>> +                              in_i);
>> +            if (b)
>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> +                                  "in%d_label",
>> +                                  in_i);
>> +            in_i++;
>>               break;
>>           case IIO_TEMP:
>>               a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>                                 "temp%d_input",
>> -                              temp_i++);
>> +                              temp_i);
>> +
>> +            if (b)
>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> +                                  "temp%d_label",
>> +                                  temp_i);
>> +            temp_i++;
>>               break;
>>           case IIO_CURRENT:
>>               a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>                                 "curr%d_input",
>> -                              curr_i++);
>> +                              curr_i);
>> +
>> +            if (b)
>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> +                                  "curr%d_label",
>> +                                  curr_i);
>> +            curr_i++;
>>               break;
>>           case IIO_HUMIDITYRELATIVE:
>>               a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>                                 "humidity%d_input",
>> -                              humidity_i++);
>> +                              humidity_i);
>> +
>> +            if (b)
>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> +                                  "humidity%d_label",
>> +                                  humidity_i);
>> +            humidity_i++;
>>               break;
>>           default:
>>               ret = -EINVAL;
>>               goto error_release_channels;
>>           }
>> -        if (a->dev_attr.attr.name == NULL) {
>> +        if (a->dev_attr.attr.name == NULL ||
>> +            (b && b->dev_attr.attr.name == NULL)) {
>>               ret = -ENOMEM;
>>               goto error_release_channels;
>>           }
> 
> Just realized that we have a memory leak here. The 'name' memory is
> never released.
> 

I don't know if we have to do something to revert the effects of
sysfs_attr_init but you sure are right that the a and b's attribute's
name is never freed. This case would be handled with devm_kasprintf, I
guess? Thanks.

>>           a->dev_attr.show = iio_hwmon_read_val;
>>           a->dev_attr.attr.mode = S_IRUGO;
>>           a->index = i;
>> -        st->attrs[i] = &a->dev_attr.attr;
>> +        st->attrs[j++] = &a->dev_attr.attr;
>> +
>> +        if (b) {
>> +            b->dev_attr.show = iio_hwmon_read_label;
>> +            b->dev_attr.attr.mode = S_IRUGO;
>> +            b->index = i;
>> +            st->attrs[j++] = &b->dev_attr.attr;
>> +        }
>>       }
>>
>>       st->attr_group.attrs = st->attrs;
>>
> 

Quentin

[PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Guenter Roeck]

On 07/15/2016 07:36 AM, Quentin Schulz wrote:
> On 15/07/2016 16:03, Guenter Roeck wrote:
>> On 07/15/2016 02:59 AM, Quentin Schulz wrote:
> [...]
>>> +static ssize_t iio_hwmon_read_label(struct device *dev,
>>> +                    struct device_attribute *attr,
>>> +                    char *buf)
>>> +{
>>> +    struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>>> +    struct iio_hwmon_state *state = dev_get_drvdata(dev);
>>> +    const char *label =
>>> state->channels[sattr->index].channel->extend_name;
>>> +
>>> +    if (label)
>>> +        return sprintf(buf, "%s\n", label);
>>> +
>> Can the name disappear on the fly, or be changed on the fly ?
>> Then this is unusable. We should and can only provide labels
>> if a name exists and is permanent. Otherwise all we do is
>> to confuse user space.
>
> It cannot, the extend_name field is const char* in the struct:
> http://lxr.free-electrons.com/source/include/linux/iio/iio.h#L247
>

Then why "if(label)" ?

> [...]
>>> @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device
>>> *pdev)
>>>            }
>>>
>>>            sysfs_attr_init(&a->dev_attr.attr);
>>> +
>>> +        b = NULL;
>>> +        if (st->channels[i].channel->extend_name) {
>>> +            b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
>>> +            if (b == NULL) {
>>> +                ret = -ENOMEM;
>>> +                goto error_release_channels;
>>> +            }
>>> +
>>> +            sysfs_attr_init(&b->dev_attr.attr);
>>
>> Why is this initialization here and not with the rest of the initialization
>> of this attribute ?
>
> I don't get your question. I've followed the exact same pattern as for
> "a" variable's initialization.
> The initialization is before the switch case because the name of the
> exposed sysfs file depends on the type of the IIO channel. If I move the
> initialization in the switch case, I'll have duplicated code.
>

Yes, but that didn't require all those if statements.

>>> +        }
>>> +
>>>            ret = iio_get_channel_type(&st->channels[i], &type);
>>>            if (ret < 0)
>>>                goto error_release_channels;
>>> @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct
>>> platform_device *pdev)
>>>            case IIO_VOLTAGE:
>>>                a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>>                                  "in%d_input",
>>> -                              in_i++);
>>> +                              in_i);
>>> +            if (b)
>>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>> +                                  "in%d_label",
>>> +                                  in_i);
>>> +            in_i++;
>>>                break;
>>>            case IIO_TEMP:
>>>                a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>>                                  "temp%d_input",
>>> -                              temp_i++);
>>> +                              temp_i);
>>> +
>>> +            if (b)
>>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>> +                                  "temp%d_label",
>>> +                                  temp_i);
>>> +            temp_i++;
>>>                break;
>>>            case IIO_CURRENT:
>>>                a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>>                                  "curr%d_input",
>>> -                              curr_i++);
>>> +                              curr_i);
>>> +
>>> +            if (b)
>>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>> +                                  "curr%d_label",
>>> +                                  curr_i);
>>> +            curr_i++;
>>>                break;
>>>            case IIO_HUMIDITYRELATIVE:
>>>                a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>>                                  "humidity%d_input",
>>> -                              humidity_i++);
>>> +                              humidity_i);
>>> +
>>> +            if (b)
>>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>> +                                  "humidity%d_label",
>>> +                                  humidity_i);
>>> +            humidity_i++;
>>>                break;
>>>            default:
>>>                ret = -EINVAL;
>>>                goto error_release_channels;
>>>            }
>>> -        if (a->dev_attr.attr.name == NULL) {
>>> +        if (a->dev_attr.attr.name == NULL ||
>>> +            (b && b->dev_attr.attr.name == NULL)) {
>>>                ret = -ENOMEM;
>>>                goto error_release_channels;
>>>            }
>>
>> Just realized that we have a memory leak here. The 'name' memory is
>> never released.
>>
>
> I don't know if we have to do something to revert the effects of
> sysfs_attr_init but you sure are right that the a and b's attribute's
> name is never freed. This case would be handled with devm_kasprintf, I
> guess? Thanks.
>
>>>            a->dev_attr.show = iio_hwmon_read_val;
>>>            a->dev_attr.attr.mode = S_IRUGO;
>>>            a->index = i;
>>> -        st->attrs[i] = &a->dev_attr.attr;
>>> +        st->attrs[j++] = &a->dev_attr.attr;
>>> +
>>> +        if (b) {

sysfs_attr_init() might be better here to keep the initialization of '*b'
as close together as possible.

Guenetr

>>> +            b->dev_attr.show = iio_hwmon_read_label;
>>> +            b->dev_attr.attr.mode = S_IRUGO;
>>> +            b->index = i;
>>> +            st->attrs[j++] = &b->dev_attr.attr;
>>> +        }
>>>        }
>>>
>>>        st->attr_group.attrs = st->attrs;
>>>
>>
>
> Quentin
>

[PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Jonathan Cameron]

On 15/07/16 10:59, Quentin Schulz wrote:
> Currently, iio_hwmon only exposes values of the IIO channels it can read
> but no label by channel is exposed.
> 
> This adds exposition of sysfs files containing label for IIO channels it
> can read based on extended_name field of the iio_chan_spec of the channel.
> If the extended_name field is empty, the sysfs file is not created by
> iio_hwmon.
Hmm. This is not the intent of extended name at all.  That exists to add
a small amount of information to an constructed IIO channel name.
Typically it's used to indicate physically wired stuff like:

in_voltage0_vdd_raw for cases where that channel of the ADC is hard wired
to the vdd.  In this particular case the use might actually work as the
vdd makes it clear it's a voltage - in general that's not the case.

Use of extended_name at all in IIO is only done after extensive review.
It adds nasty custom ABI to a device, so the gain has to be considerable
to use it.

When I read your original suggestion of adding labels, I was expecting the
use of datasheet_name.  That has the advantage of being well defined by
the datasheet (if not it should not be provided) + not being used in
the construction of the IIO channel related attributes.  However, that
may still not correspond well to the expected labelling in hwmon.
Thinking more on this, the label is going to often be a function of how
the board is wired up...  Perhaps it should be a characteristic of the
channel_map (hence from DT or similar) rather than part of the IIO driver
itself?

At first glance hwmon labels appear to be pretty freeform...  However
we need to be very careful here as this is effectively defining a large
chunk of new ABI.

This isn't a thing that I have a particularly clear view on (as you
might be able to tell ;).  Other opinions sought!

Jonathan
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> patch added in v2
> 
>  drivers/hwmon/iio_hwmon.c | 77 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index c0da4d9..28d15b2 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
>  #include <linux/iio/types.h>
>  
>  /**
> @@ -57,12 +58,26 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
>  	return sprintf(buf, "%d\n", result);
>  }
>  
> +static ssize_t iio_hwmon_read_label(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
> +	const char *label = state->channels[sattr->index].channel->extend_name;
> +
> +	if (label)
> +		return sprintf(buf, "%s\n", label);
> +
> +	return 0;
> +}
> +
>  static int iio_hwmon_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct iio_hwmon_state *st;
> -	struct sensor_device_attribute *a;
> -	int ret, i;
> +	struct sensor_device_attribute *a, *b;
> +	int ret, i, j = 0;
>  	int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1;
>  	enum iio_chan_type type;
>  	struct iio_channel *channels;
> @@ -92,7 +107,8 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		st->num_channels++;
>  
>  	st->attrs = devm_kzalloc(dev,
> -				 sizeof(*st->attrs) * (st->num_channels + 1),
> +				 sizeof(*st->attrs) *
> +				 (2 * st->num_channels + 1),
>  				 GFP_KERNEL);
>  	if (st->attrs == NULL) {
>  		ret = -ENOMEM;
> @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		}
>  
>  		sysfs_attr_init(&a->dev_attr.attr);
> +
> +		b = NULL;
> +		if (st->channels[i].channel->extend_name) {
> +			b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
> +			if (b == NULL) {
> +				ret = -ENOMEM;
> +				goto error_release_channels;
> +			}
> +
> +			sysfs_attr_init(&b->dev_attr.attr);
> +		}
> +
>  		ret = iio_get_channel_type(&st->channels[i], &type);
>  		if (ret < 0)
>  			goto error_release_channels;
> @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		case IIO_VOLTAGE:
>  			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>  							  "in%d_input",
> -							  in_i++);
> +							  in_i);
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "in%d_label",
> +								  in_i);
> +			in_i++;
>  			break;
>  		case IIO_TEMP:
>  			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>  							  "temp%d_input",
> -							  temp_i++);
> +							  temp_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "temp%d_label",
> +								  temp_i);
> +			temp_i++;
>  			break;
>  		case IIO_CURRENT:
>  			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>  							  "curr%d_input",
> -							  curr_i++);
> +							  curr_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "curr%d_label",
> +								  curr_i);
> +			curr_i++;
>  			break;
>  		case IIO_HUMIDITYRELATIVE:
>  			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>  							  "humidity%d_input",
> -							  humidity_i++);
> +							  humidity_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "humidity%d_label",
> +								  humidity_i);
> +			humidity_i++;
>  			break;
>  		default:
>  			ret = -EINVAL;
>  			goto error_release_channels;
>  		}
> -		if (a->dev_attr.attr.name == NULL) {
> +		if (a->dev_attr.attr.name == NULL ||
> +		    (b && b->dev_attr.attr.name == NULL)) {
>  			ret = -ENOMEM;
>  			goto error_release_channels;
>  		}
>  		a->dev_attr.show = iio_hwmon_read_val;
>  		a->dev_attr.attr.mode = S_IRUGO;
>  		a->index = i;
> -		st->attrs[i] = &a->dev_attr.attr;
> +		st->attrs[j++] = &a->dev_attr.attr;
> +
> +		if (b) {
> +			b->dev_attr.show = iio_hwmon_read_label;
> +			b->dev_attr.attr.mode = S_IRUGO;
> +			b->index = i;
> +			st->attrs[j++] = &b->dev_attr.attr;
> +		}
>  	}
>  
>  	st->attr_group.attrs = st->attrs;
> 

[PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Quentin Schulz]

On 18/07/2016 14:24, Jonathan Cameron wrote:
> On 15/07/16 10:59, Quentin Schulz wrote:
>> Currently, iio_hwmon only exposes values of the IIO channels it can read
>> but no label by channel is exposed.
>>
>> This adds exposition of sysfs files containing label for IIO channels it
>> can read based on extended_name field of the iio_chan_spec of the channel.
>> If the extended_name field is empty, the sysfs file is not created by
>> iio_hwmon.
> Hmm. This is not the intent of extended name at all.  That exists to add
> a small amount of information to an constructed IIO channel name.
> Typically it's used to indicate physically wired stuff like:
> 
> in_voltage0_vdd_raw for cases where that channel of the ADC is hard wired
> to the vdd.  In this particular case the use might actually work as the
> vdd makes it clear it's a voltage - in general that's not the case.
> 
> Use of extended_name at all in IIO is only done after extensive review.
> It adds nasty custom ABI to a device, so the gain has to be considerable
> to use it.
> 
> When I read your original suggestion of adding labels, I was expecting the
> use of datasheet_name.  That has the advantage of being well defined by
> the datasheet (if not it should not be provided) + not being used in
> the construction of the IIO channel related attributes.  However, that
> may still not correspond well to the expected labelling in hwmon.

Good to know for extend_name use cases. While doing further testing, I
noticed the extend_name is also appended to the sysfs filename.. which
is definitely not what we want.

I've checked for datasheet_name and it is only used to be compared to
adc_channel_label from iio_map structure. Same for adc_channel_label
(which has to be the same as datasheet_name of the iio_chan_spec it is
linked to). So I could use this instead of extend_name to put a label on
a channel.
However, I thought of it to be more a way to identify the hardware in
the datasheet more than giving users a hint on what it is. That's what
"git grep adc_channel_label" told me. It's definitely better to use
datasheet_name over extend_name for channel labeling but I don't know if
it's really the good variable to use for labeling? In my understanding
of datasheet_name, in my case it would be more "temp_gpadc" than "SoC
temperature", that's what I mean.

> Thinking more on this, the label is going to often be a function of how
> the board is wired up...  Perhaps it should be a characteristic of the
> channel_map (hence from DT or similar) rather than part of the IIO driver
> itself?

Hmm.. I would not put a property in the DT only for labeling.

[...]

[PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Jonathan Cameron]

On 19/07/16 07:55, Quentin Schulz wrote:
> On 18/07/2016 14:24, Jonathan Cameron wrote:
>> On 15/07/16 10:59, Quentin Schulz wrote:
>>> Currently, iio_hwmon only exposes values of the IIO channels it can read
>>> but no label by channel is exposed.
>>>
>>> This adds exposition of sysfs files containing label for IIO channels it
>>> can read based on extended_name field of the iio_chan_spec of the channel.
>>> If the extended_name field is empty, the sysfs file is not created by
>>> iio_hwmon.
>> Hmm. This is not the intent of extended name at all.  That exists to add
>> a small amount of information to an constructed IIO channel name.
>> Typically it's used to indicate physically wired stuff like:
>>
>> in_voltage0_vdd_raw for cases where that channel of the ADC is hard wired
>> to the vdd.  In this particular case the use might actually work as the
>> vdd makes it clear it's a voltage - in general that's not the case.
>>
>> Use of extended_name at all in IIO is only done after extensive review.
>> It adds nasty custom ABI to a device, so the gain has to be considerable
>> to use it.
>>
>> When I read your original suggestion of adding labels, I was expecting the
>> use of datasheet_name.  That has the advantage of being well defined by
>> the datasheet (if not it should not be provided) + not being used in
>> the construction of the IIO channel related attributes.  However, that
>> may still not correspond well to the expected labelling in hwmon.
> 
> Good to know for extend_name use cases. While doing further testing, I
> noticed the extend_name is also appended to the sysfs filename.. which
> is definitely not what we want.
> 
> I've checked for datasheet_name and it is only used to be compared to
> adc_channel_label from iio_map structure. Same for adc_channel_label
> (which has to be the same as datasheet_name of the iio_chan_spec it is
> linked to). So I could use this instead of extend_name to put a label on
> a channel.
> However, I thought of it to be more a way to identify the hardware in
> the datasheet more than giving users a hint on what it is. That's what
> "git grep adc_channel_label" told me. It's definitely better to use
> datasheet_name over extend_name for channel labeling but I don't know if
> it's really the good variable to use for labeling? In my understanding
> of datasheet_name, in my case it would be more "temp_gpadc" than "SoC
> temperature", that's what I mean.
If we are going to do this I think we need a new field in iio_chan_spec
for it.  The problem as ever is going to be that we'll end up with
'fuzzy' ABI which we can't change - even if we end up with spelling
mistakes sneaking through review - or entirely incorrect labels.
> 
>> Thinking more on this, the label is going to often be a function of how
>> the board is wired up...  Perhaps it should be a characteristic of the
>> channel_map (hence from DT or similar) rather than part of the IIO driver
>> itself?
> 
> Hmm.. I would not put a property in the DT only for labeling.
It's an odd one because in many cases the map (which is effectively
representing a wire) is the only relevant place to have such a label.

Can see what you mean about not putting things in DT which are basically
'help'!
> 
> [...]
>