Re: [PATCH RESEND] x86, intel_mid: ADC management

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

On Tue, Apr 10, 2012 at 02:20:05PM +0100, Alan Cox wrote:
> This ended in a discussion of where shall we put it and then silence so
> resending this because we still need it and we've got patches that depend
> upon it getting backlogged.

The decision seems fairly settled that general purpose ADCs like this
should go into IIO, the trick is going to be getting IIO out of staging.

[PATCH RESEND] x86, intel_mid: ADC management

[Alan Cox]

This ended in a discussion of where shall we put it and then silence so
resending this because we still need it and we've got patches that depend
upon it getting backlogged.

From: Alan Cox <alan@linux.intel.com>

This is a fold up of the following ported to 3.3


commit f81040398e4b45323d4144fb05c126245e86866d
Author: Mika Westerberg <mika.westerberg@linux.intel.com>
Date:   Thu Jan 12 11:51:18 2012 +0200

    x86, mrst: add platform data for gpadc driver

    Now that the driver is converted to use intel_msic interfaces we can add
    the platform data back.

    Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
    Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

commit 6d9d2d390f6e18ce1d419b9e2762c9bf139a80ea
Author: Mika Westerberg <mika.westerberg@linux.intel.com>
Date:   Thu Jan 12 11:51:17 2012 +0200

    gpadc: convert to use intel_msic

    In Medfield all the subdevices in MSIC chip are handled by intel_msic MFD
    driver. This patch converts gpadc driver to use interfaces provided by
    intel_msic driver.

    This will enable all the users (including sn95031 audio codec) to use gpadc
    again.

    Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
    Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>


commit f1532c8454b8958aee933a9915862345d62ee3f8
Author: Mika Westerberg <mika.westerberg@linux.intel.com>
Date:   Thu Jan 12 11:51:16 2012 +0200

    x86, mrst: remove gpadc platform data

    Nowadays these devices are handled by intel_msic MFD driver which has
    slightly different way of passing platform data to the drivers.

    So as a first step we remove the old platform data for gpadc driver. In
    subsequent patches we add it back in such format suitable for intel_msic
    driver.

    Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
    Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

commit 8444512429fd93786f114824bb5c258272aa1d2d
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:31 2011 +0100

    gpadc: add reg dump after timeout

    Sometimes, it is very hard to duplicate ADC timeout issue.
    Some issue only can be duplicated on a small quantity of boards.
    To add the registers dump after timeout will help debugging.

    Signed-off-by: Bin Yang <bin.yang@intel.com>

commit 26fc6e4e72d1cac189064b9c85290e3c8a907cf0
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:30 2011 +0100

    gpadc: fix battery temp accuracy

    From oscilloscope result, BPTHERM takes <1ms to stabilize.
    So it needs to add 1ms delay after VBUSREF is enabled.

    Signed-off-by: Bin Yang <bin.yang@intel.com>

commit 9dc5019e570c59e70349b34ec864bfc447a0d0c5
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:30 2011 +0100

    gpadc: optimize driver initialization

    ADC trimming cost a long time. It blocks the kernel boot sequence.
    Change to do trimming in a single workqueue.

    Signed-off-by: Bin Yang <bin.yang@intel.com>

commit 16149d8e3dfe0d663851318390ebafdede760281
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:29 2011 +0100

    gpadc: read sample result in one loop

    RR bit is used to inform HW for sample result read.
    it needs to hold the RR bit for all channels result read.

    this patch moves the lines to set/clear RR bit outside the loop.

    Signed-off-by: Bin Yang <bin.yang@intel.com>

commit d910dd3b07b6e0766c96c59e48de5bcce1175f0c
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:29 2011 +0100

    gpadc: fix logic err of last addr checking

    it has logic error to check last addr which is used.

    Signed-off-by: Bin Yang <bin.yang@intel.com>

commit 1b688193bd96331da73482399925d51cbf535780
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:28 2011 +0100

    gpadc: update the calibration algorithm

    gpadc document had updated this algorithm in new version.

    This patch updates the driver base on new  document.

    Signed-off-by: Bin Yang <bin.yang@intel.com>

commit baa74900a34ad17de1619a8f009702e66eb4fbd4
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:28 2011 +0100

    gpadc: fix sample result error

    During stress test, ADC results have some bad valules.
    Driver has a race condition issue. It initializes the output data to 0 first
    without any lock. And the caller may access the output pointer before function
    return.

    This patch moves the output data initializing code inside mutex protected period.
    And it also adds a limitation which forbid caller to access output pointer
    before function return.

    Signed-off-by: Bin Yang <bin.yang@intel.com>

commit 9c53a361051541a7cb24a8fde90d5c75c5d52d91
Author: He Bo <bo.he@intel.com>
Date:   Tue Oct 18 14:16:26 2011 +0100

    gpadc: system does not enter S3 after incoming call

    intel_mid_gpadc driver causes deadlock in S3, fix it.

    Signed-off-by: He Bo <bo.he@intel.com>

commit f25e645c187d3b9943ad0180e52984f51c77e64b
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:25 2011 +0100

    gpadc: prevent CPU from S0i3 when ADC is active

    Under the following scenario:
    	1. IPC1 request sent to SCU
    	2. PM_CMD s0ix entry request sent to SCU
    	3. MWAIT C6 abort (or no attempt at MWAIT C6)
    	If steps #1 an #2 are sufficiently close (maybe within 200us) there is
    a possibility that SCU will handle the PM_CMD first and begin waiting on an
    ack_c6 response from the MWAIT that is expected. Since the MWAIT never comes
    (or is aborted) SCU will have to eventually time out before being able to move
    on and handle the IPC1 request.

    Signed-off-by: Bin Yang <bin.yang@intel.com>
    [ port to new pm_qos_add_request() interface -Guanqun ]
    Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>

commit fdd0adef95e6b5836cc4ed5764a398b81841945d
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:23 2011 +0100

    The coulomb count interrupt is not used by kernel driver. It needs to be
    handled inside firmware.

    Signed-off-by: Bin Yang <bin.yang@intel.com>

commit 1f4f11af853a74c015a9ffb5bc22d3f9140ce6d5
Author: Bin Yang <bin.yang@intel.com>
Date:   Tue Oct 18 14:16:19 2011 +0100

    gpadc: add gpadc driver support

    The general purpose ADC inside MSIC is used by battery and thermal.  This
    driver provides the channel allocation and basic ADC functions.

    Signed-off-by: Bin Yang <bin.yang@intel.com>
    Signed-off-by: Hao Wu <hao.wu@intel.com>



Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 arch/x86/include/asm/intel_mid_gpadc.h   |   13 +
 arch/x86/platform/mrst/Makefile          |    1 
 arch/x86/platform/mrst/intel_mid_gpadc.c |  645 ++++++++++++++++++++++++++++++
 arch/x86/platform/mrst/mrst.c            |    6 
 4 files changed, 665 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/include/asm/intel_mid_gpadc.h
 create mode 100644 arch/x86/platform/mrst/intel_mid_gpadc.c


diff --git a/arch/x86/include/asm/intel_mid_gpadc.h b/arch/x86/include/asm/intel_mid_gpadc.h
new file mode 100644
index 0000000..ae79309
--- /dev/null
+++ b/arch/x86/include/asm/intel_mid_gpadc.h
@@ -0,0 +1,13 @@
+#ifndef __INTEL_MID_GPADC_H__
+#define __INTEL_MID_GPADC_H__
+
+#define CH_NEED_VREF		(1 << 8)
+#define CH_NEED_VCALIB		(1 << 9)
+#define CH_NEED_ICALIB		(1 << 10)
+
+int intel_mid_gpadc_gsmpulse_sample(int *vol, int *cur);
+int intel_mid_gpadc_sample(void *handle, int sample_count, ...);
+void intel_mid_gpadc_free(void *handle);
+void *intel_mid_gpadc_alloc(int count, ...);
+#endif
+
diff --git a/arch/x86/platform/mrst/Makefile b/arch/x86/platform/mrst/Makefile
index af1da7e..66e006c 100644
--- a/arch/x86/platform/mrst/Makefile
+++ b/arch/x86/platform/mrst/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_X86_INTEL_MID)	+= mrst.o
 obj-$(CONFIG_X86_INTEL_MID)	+= vrtc.o
 obj-$(CONFIG_EARLY_PRINTK_INTEL_MID)	+= early_printk_mrst.o
+obj-$(CONFIG_X86_INTEL_MID)	+= intel_mid_gpadc.o
diff --git a/arch/x86/platform/mrst/intel_mid_gpadc.c b/arch/x86/platform/mrst/intel_mid_gpadc.c
new file mode 100644
index 0000000..6927cae
--- /dev/null
+++ b/arch/x86/platform/mrst/intel_mid_gpadc.c
@@ -0,0 +1,645 @@
+/*
+ * intel_msic_gpadc.c - Intel Medfield MSIC GPADC Driver
+ *
+ * Copyright (C) 2010 Intel Corporation
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * Author: Jenny TC <jenny.tc@intel.com>
+ * Author: Bin Yang <bin.yang@intel.com>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/sched.h>
+#include <linux/platform_device.h>
+#include <linux/pm_qos.h>
+#include <linux/workqueue.h>
+#include <linux/mfd/intel_msic.h>
+#include <asm/intel_mid_gpadc.h>
+
+#define EEPROMCAL1		0x317
+#define EEPROMCAL2		0x318
+
+#define MCCINT_MCCCAL		(1 << 1)
+#define MCCINT_MOVERFLOW	(1 << 0)
+
+#define IRQLVL1MSK_ADCM		(1 << 1)
+
+#define ADC1CNTL1_AD1OFFSETEN	(1 << 6)
+#define ADC1CNTL1_AD1CALEN	(1 << 5)
+#define ADC1CNTL1_ADEN		(1 << 4)
+#define ADC1CNTL1_ADSTRT	(1 << 3)
+#define ADC1CNTL1_ADSLP		7
+#define ADC1CNTL1_ADSLP_DEF	1
+
+#define ADC1INT_ADC1CAL		(1 << 2)
+#define ADC1INT_GSM		(1 << 1)
+#define ADC1INT_RND		(1 << 0)
+
+#define ADC1CNTL3_ADCTHERM	(1 << 2)
+#define ADC1CNTL3_GSMDATARD	(1 << 1)
+#define ADC1CNTL3_RRDATARD	(1 << 0)
+
+#define ADC1CNTL2_DEF		0x7
+#define ADC1CNTL2_ADCGSMEN	(1 << 7)
+
+#define MSIC_STOPCH		(1 << 4)
+
+#define GPADC_CH_MAX		15
+
+#define PM_QOS_ADC_DRV_VALUE	4999
+
+#define GPADC_POWERON_DELAY	1
+
+struct gpadc_info {
+	int initialized;
+
+	struct workqueue_struct *workq;
+	wait_queue_head_t trimming_wait;
+	struct work_struct trimming_work;
+	int trimming_start;
+
+	/* This mutex protects gpadc sample/config from concurrent conflict.
+	   Any function, which does the sample or config, needs to
+	   hold this lock.
+	   If it is locked, it also means the gpadc is in active mode.
+	   GSM mode sample does not need to hold this lock. It can be used with
+	   normal sample concurrent without poweron.
+	*/
+	struct mutex lock;
+	struct device *dev;
+	int irq;
+	u8 irq_status;
+
+	int vzse;
+	int vge;
+	int izse;
+	int ige;
+	int addr_mask;
+
+	wait_queue_head_t wait;
+	int rnd_done;
+	int conv_done;
+	int gsmpulse_done;
+
+	struct pm_qos_request pm_qos_request;
+};
+
+struct gpadc_request {
+	int count;
+	int vref;
+	int ch[GPADC_CH_MAX];
+	int addr[GPADC_CH_MAX];
+};
+
+static struct gpadc_info gpadc_info;
+
+static inline int gpadc_clear_bits(u16 addr, u8 mask)
+{
+	return intel_msic_reg_update(addr, 0, mask);
+}
+
+static inline int gpadc_set_bits(u16 addr, u8 mask)
+{
+	return intel_msic_reg_update(addr, 0xff, mask);
+}
+
+static inline int gpadc_write(u16 addr, u8 data)
+{
+	return intel_msic_reg_write(addr, data);
+}
+
+static inline int gpadc_read(u16 addr, u8 *data)
+{
+	return intel_msic_reg_read(addr, data);
+}
+
+static void gpadc_dump(struct gpadc_info *mgi)
+{
+	u8 data;
+	int i;
+
+	gpadc_read(INTEL_MSIC_VAUDACNT, &data);
+	dev_err(mgi->dev, "VAUDACNT: 0x%x\n", data);
+	gpadc_read(INTEL_MSIC_IRQLVL1MSK, &data);
+	dev_err(mgi->dev, "IRQLVL1MSK: 0x%x\n", data);
+	gpadc_read(INTEL_MSIC_IRQLVL1, &data);
+	dev_err(mgi->dev, "IRQLVL1: 0x%x\n", data);
+	gpadc_read(INTEL_MSIC_ADC1INT, &data);
+	dev_err(mgi->dev, "ADC1INT: 0x%x\n", data);
+	gpadc_read(INTEL_MSIC_ADC1CNTL1, &data);
+	dev_err(mgi->dev, "ADC1CNTL1: 0x%x\n", data);
+	gpadc_read(INTEL_MSIC_ADC1CNTL2, &data);
+	dev_err(mgi->dev, "ADC1CNTL2: 0x%x\n", data);
+	gpadc_read(INTEL_MSIC_ADC1CNTL3, &data);
+	dev_err(mgi->dev, "ADC1CNTL3: 0x%x\n", data);
+	for (i = 0; i < GPADC_CH_MAX; i++) {
+		gpadc_read(INTEL_MSIC_ADC1ADDR0+i, &data);
+		dev_err(mgi->dev, "ADC1ADDR[%d]: 0x%x\n", i, data);
+	}
+}
+
+static int gpadc_poweron(struct gpadc_info *mgi, int vref)
+{
+	if (gpadc_set_bits(INTEL_MSIC_ADC1CNTL1, ADC1CNTL1_ADEN) != 0)
+		return -EIO;
+	msleep(GPADC_POWERON_DELAY);
+	if (vref) {
+		if (gpadc_set_bits(INTEL_MSIC_ADC1CNTL3,
+				   ADC1CNTL3_ADCTHERM) != 0)
+			return -EIO;
+		msleep(GPADC_POWERON_DELAY);
+	}
+	return 0;
+}
+
+static int gpadc_poweroff(struct gpadc_info *mgi)
+{
+	if (gpadc_clear_bits(INTEL_MSIC_ADC1CNTL1, ADC1CNTL1_ADEN) != 0)
+		return -EIO;
+	if (gpadc_clear_bits(INTEL_MSIC_ADC1CNTL3, ADC1CNTL3_ADCTHERM) != 0)
+		return -EIO;
+	return 0;
+}
+
+static void gpadc_trimming(struct work_struct *work)
+{
+	u8 data;
+	int fse, zse, fse_sign, zse_sign;
+	struct gpadc_info *mgi =
+		container_of(work, struct gpadc_info, trimming_work);
+
+	mutex_lock(&mgi->lock);
+	mgi->trimming_start = 1;
+	wake_up(&mgi->trimming_wait);
+	if (gpadc_poweron(mgi, 1)) {
+		dev_err(mgi->dev, "power on failed\n");
+		goto failed;
+	}
+	/* calibration */
+	gpadc_read(INTEL_MSIC_ADC1CNTL1, &data);
+	data &= ~ADC1CNTL1_AD1OFFSETEN;
+	data |= ADC1CNTL1_AD1CALEN;
+	gpadc_write(INTEL_MSIC_ADC1CNTL1, data);
+	gpadc_read(INTEL_MSIC_ADC1INT, &data);
+
+	/*workaround: no calib int */
+	msleep(300);
+	gpadc_set_bits(INTEL_MSIC_ADC1INT, ADC1INT_ADC1CAL);
+	gpadc_clear_bits(INTEL_MSIC_ADC1CNTL1, ADC1CNTL1_AD1CALEN);
+
+	/* voltage trim */
+	gpadc_read(EEPROMCAL1, &data);
+	zse = (data & 0xf)/2;
+	fse = ((data >> 4) & 0xf)/2;
+	gpadc_read(EEPROMCAL2, &data);
+	zse_sign = (data & (1 << 6)) ? 1 : 0;
+	fse_sign = (data & (1 << 7)) ? 1 : 0;
+	zse *= zse_sign;
+	fse *= fse_sign;
+	mgi->vzse = zse;
+	mgi->vge = fse - zse;
+
+	/* current trim */
+	fse = (data & 0xf)/2;
+	fse_sign = (data & (1 << 5)) ? 1 : 0;
+	fse = ~(fse_sign * fse) + 1;
+	gpadc_read(INTEL_MSIC_ADC1OFFSETH, &data);
+	zse = data << 2;
+	gpadc_read(INTEL_MSIC_ADC1OFFSETL, &data);
+	zse += data & 0x3;
+	mgi->izse = zse;
+	mgi->ige = fse + zse;
+	if (gpadc_poweroff(mgi)) {
+		dev_err(mgi->dev, "power off failed\n");
+		goto failed;
+	}
+
+failed:
+	mutex_unlock(&mgi->lock);
+}
+
+static irqreturn_t msic_gpadc_isr(int irq, void *data)
+{
+	struct gpadc_info *mgi = data;
+	struct intel_msic *msic = dev_get_drvdata(mgi->dev->parent);
+	int ret;
+
+	ret = intel_msic_irq_read(msic, INTEL_MSIC_ADC1INT, &mgi->irq_status);
+	if (ret < 0)
+		dev_warn(mgi->dev, "failed to read IRQ status\n");
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t msic_gpadc_irq(int irq, void *data)
+{
+	struct gpadc_info *mgi = data;
+
+	if (mgi->irq_status & ADC1INT_GSM) {
+		mgi->gsmpulse_done = 1;
+		wake_up(&mgi->wait);
+	} else if (mgi->irq_status & ADC1INT_RND) {
+		mgi->rnd_done = 1;
+		wake_up(&mgi->wait);
+	} else if (mgi->irq_status & ADC1INT_ADC1CAL) {
+		mgi->conv_done = 1;
+		wake_up(&mgi->wait);
+	} else {
+		/* coulomb counter should be handled by firmware. Ignore it */
+		dev_dbg(mgi->dev, "coulomb counter is not support\n");
+	}
+	return IRQ_HANDLED;
+}
+
+static int alloc_channel_addr(struct gpadc_info *mgi, int ch)
+{
+	int i;
+	int addr = -EBUSY;
+	int last = 0;
+
+	for (i = 0; i < GPADC_CH_MAX; i++)
+		if (mgi->addr_mask & (1 << i))
+			last = i;
+
+	for (i = 0; i < GPADC_CH_MAX; i++) {
+		if (!(mgi->addr_mask & (1 << i))) {
+			addr = i;
+			mgi->addr_mask |= 1 << i;
+			if (addr > last) {
+				gpadc_clear_bits(INTEL_MSIC_ADC1ADDR0+last,
+						 MSIC_STOPCH);
+				gpadc_write(INTEL_MSIC_ADC1ADDR0+addr,
+					    ch|MSIC_STOPCH);
+			} else {
+				gpadc_write(INTEL_MSIC_ADC1ADDR0+addr, ch);
+			}
+			break;
+		}
+	}
+	return addr;
+}
+
+static void free_channel_addr(struct gpadc_info *mgi, int addr)
+{
+	int last = 0;
+	int i;
+
+	mgi->addr_mask &= ~(1 << addr);
+	for (i = 0; i < GPADC_CH_MAX; i++)
+		if (mgi->addr_mask & (1 << i))
+			last = i;
+	if (addr > last)
+		gpadc_set_bits(INTEL_MSIC_ADC1ADDR0+last, MSIC_STOPCH);
+}
+
+/**
+ * intel_mid_gpadc_gsmpulse_sample - do gpadc sample during gsm pulse.
+ * @val: return the voltage value. caller should not access it before return.
+ * @cur: return the current value. caller should not access it before return.
+ *
+ * Returns 0 on success or an error code.
+ *
+ * This function may sleep.
+ */
+int intel_mid_gpadc_gsmpulse_sample(int *vol, int *cur)
+{
+	struct gpadc_info *mgi = &gpadc_info;
+	int i;
+	u8 data;
+	int tmp;
+	int ret = 0;
+
+	if (!mgi->initialized)
+		return -ENODEV;
+
+	mutex_lock(&mgi->lock);
+	pm_qos_add_request(&mgi->pm_qos_request,
+			   PM_QOS_CPU_DMA_LATENCY, PM_QOS_ADC_DRV_VALUE);
+	gpadc_write(INTEL_MSIC_ADC1CNTL2, ADC1CNTL2_DEF);
+	gpadc_set_bits(INTEL_MSIC_ADC1CNTL2, ADC1CNTL2_ADCGSMEN);
+
+	if (wait_event_timeout(mgi->wait, mgi->gsmpulse_done, HZ) == 0) {
+		gpadc_dump(mgi);
+		dev_err(mgi->dev, "gsmpulse sample timeout\n");
+		ret = -ETIMEDOUT;
+		goto fail;
+	}
+	gpadc_clear_bits(INTEL_MSIC_ADC1CNTL1, ADC1CNTL1_ADEN);
+	gpadc_set_bits(INTEL_MSIC_ADC1CNTL3, ADC1CNTL3_GSMDATARD);
+
+	*vol = 0;
+	*cur = 0;
+	for (i = 0; i < 4; i++) {
+		gpadc_read(INTEL_MSIC_ADC1BV0H + i * 2, &data);
+		tmp = data << 2;
+		gpadc_read(INTEL_MSIC_ADC1BV0H + i * 2 + 1, &data);
+		tmp += data & 0x3;
+		if (tmp > *vol)
+			*vol = tmp;
+
+		gpadc_read(INTEL_MSIC_ADC1BI0H + i * 2, &data);
+		tmp = data << 2;
+		gpadc_read(INTEL_MSIC_ADC1BI0H + i * 2 + 1, &data);
+		tmp += data & 0x3;
+		if (tmp > *cur)
+			*cur = tmp;
+	}
+
+	/**
+	 * Using the calibration data, we have the voltage and current
+	 * after calibration correction as below:
+	 * V_CAL_CODE = V_RAW_CODE - (VZSE + (VGE)* VRAW_CODE/1023)
+	 * I_CAL_CODE = I_RAW_CODE - (IZSE + (IGE)* IRAW_CODE/1023)
+	*/
+	*vol -= mgi->vzse + mgi->vge * (*vol) / 1023;
+	*cur -= mgi->izse + mgi->ige * (*cur) / 1023;
+
+	gpadc_set_bits(INTEL_MSIC_ADC1INT, ADC1INT_GSM);
+	gpadc_clear_bits(INTEL_MSIC_ADC1CNTL3, ADC1CNTL3_GSMDATARD);
+fail:
+	gpadc_clear_bits(INTEL_MSIC_ADC1CNTL2, ADC1CNTL2_ADCGSMEN);
+	pm_qos_remove_request(&mgi->pm_qos_request);
+	mutex_unlock(&mgi->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(intel_mid_gpadc_gsmpulse_sample);
+
+/**
+ * intel_mid_gpadc_sample - do gpadc sample.
+ * @handle: the gpadc handle
+ * @sample_count: do sample serveral times and get the average value.
+ * @...: sampling resulting arguments of all channels. Caller should not
+ *	 access it before return.
+ *
+ * Returns 0 on success or an error code.
+ *
+ * This function may sleep.
+ */
+int intel_mid_gpadc_sample(void *handle, int sample_count, ...)
+{
+
+	struct gpadc_request *rq = handle;
+	struct gpadc_info *mgi = &gpadc_info;
+	int i;
+	u8 data;
+	int ret = 0;
+	int count;
+	int tmp;
+	int *val[GPADC_CH_MAX];
+	va_list args;
+
+	if (!mgi->initialized)
+		return -ENODEV;
+
+	mutex_lock(&mgi->lock);
+	va_start(args, sample_count);
+	for (i = 0; i < rq->count; i++) {
+		val[i] = va_arg(args, int*);
+		*val[i] = 0;
+	}
+	va_end(args);
+
+	pm_qos_add_request(&mgi->pm_qos_request,
+			   PM_QOS_CPU_DMA_LATENCY, PM_QOS_ADC_DRV_VALUE);
+	gpadc_poweron(mgi, rq->vref);
+	gpadc_clear_bits(INTEL_MSIC_ADC1CNTL1, ADC1CNTL1_AD1OFFSETEN);
+	gpadc_read(INTEL_MSIC_ADC1CNTL1, &data);
+	data = (data & ~ADC1CNTL1_ADSLP) + ADC1CNTL1_ADSLP_DEF;
+	gpadc_write(INTEL_MSIC_ADC1CNTL1, data);
+	mgi->rnd_done = 0;
+	gpadc_set_bits(INTEL_MSIC_ADC1CNTL1, ADC1CNTL1_ADSTRT);
+	for (count = 0; count < sample_count; count++) {
+		if (wait_event_timeout(mgi->wait, mgi->rnd_done, HZ) == 0) {
+			gpadc_dump(mgi);
+			dev_err(mgi->dev, "sample timeout\n");
+			ret = -ETIMEDOUT;
+			goto fail;
+		}
+		gpadc_set_bits(INTEL_MSIC_ADC1CNTL3, ADC1CNTL3_RRDATARD);
+		for (i = 0; i < rq->count; ++i) {
+			tmp = 0;
+			gpadc_read(INTEL_MSIC_ADC1SNS0H + 2 * rq->addr[i],
+				   &data);
+			tmp += data << 2;
+			gpadc_read(INTEL_MSIC_ADC1SNS0H + 2 * rq->addr[i] + 1,
+				   &data);
+			tmp += data & 0x3;
+
+			/**
+			 * Using the calibration data, we have the voltage and
+			 * current after calibration correction as below:
+			 * V_CAL_CODE = V_RAW_CODE - (VZSE+(VGE)*VRAW_CODE/1023)
+			 * I_CAL_CODE = I_RAW_CODE - (IZSE+(IGE)*IRAW_CODE/1023)
+			 */
+			if (rq->ch[i] & CH_NEED_VCALIB)
+				tmp -= mgi->vzse + mgi->vge * tmp / 1023;
+			if (rq->ch[i] & CH_NEED_ICALIB)
+				tmp -= mgi->izse + mgi->ige * tmp / 1023;
+			*val[i] += tmp;
+		}
+		gpadc_clear_bits(INTEL_MSIC_ADC1CNTL3, ADC1CNTL3_RRDATARD);
+		mgi->rnd_done = 0;
+	}
+
+	for (i = 0; i < rq->count; ++i)
+		*val[i] /= sample_count;
+
+fail:
+	gpadc_clear_bits(INTEL_MSIC_ADC1CNTL1, ADC1CNTL1_ADSTRT);
+	gpadc_poweroff(mgi);
+	pm_qos_remove_request(&mgi->pm_qos_request);
+	mutex_unlock(&mgi->lock);
+	return ret;
+}
+EXPORT_SYMBOL(intel_mid_gpadc_sample);
+
+/**
+ * intel_mid_gpadc_free - free gpadc
+ * @handle: the gpadc handle
+ *
+ * This function may sleep.
+ */
+void intel_mid_gpadc_free(void *handle)
+{
+	struct gpadc_request *rq = handle;
+	struct gpadc_info *mgi = &gpadc_info;
+	int i;
+
+	mutex_lock(&mgi->lock);
+	for (i = 0; i < rq->count; i++)
+		free_channel_addr(mgi, rq->addr[i]);
+	mutex_unlock(&mgi->lock);
+	kfree(rq);
+}
+EXPORT_SYMBOL(intel_mid_gpadc_free);
+
+/**
+ * intel_mid_gpadc_alloc - allocate gpadc for channels
+ * @count: the count of channels
+ * @...: the channel parameters. (channel idx | flags)
+ *       flags:
+ *             CH_NEED_VCALIB   it needs voltage calibration
+ *             CH_NEED_ICALIB   it needs current calibration
+ *
+ * Returns gpadc handle on success or NULL on fail.
+ *
+ * This function may sleep.
+ */
+void *intel_mid_gpadc_alloc(int count, ...)
+{
+	struct gpadc_request *rq;
+	struct gpadc_info *mgi = &gpadc_info;
+	va_list args;
+	int ch;
+	int i;
+
+	if (!mgi->initialized)
+		return NULL;
+
+	rq = kzalloc(sizeof(struct gpadc_request), GFP_KERNEL);
+	if (rq == NULL)
+		return NULL;
+
+	va_start(args, count);
+	mutex_lock(&mgi->lock);
+	rq->count = count;
+	for (i = 0; i < count; i++) {
+		ch = va_arg(args, int);
+		rq->ch[i] = ch;
+		if (ch & CH_NEED_VREF)
+			rq->vref = 1;
+		ch &= 0xf;
+		rq->addr[i] = alloc_channel_addr(mgi, ch);
+		if (rq->addr[i] < 0) {
+			dev_err(mgi->dev, "alloc addr failed\n");
+			while (i-- > 0)
+				free_channel_addr(mgi, rq->addr[i]);
+			kfree(rq);
+			rq = NULL;
+			break;
+		}
+	}
+	mutex_unlock(&mgi->lock);
+	va_end(args);
+
+	return rq;
+}
+EXPORT_SYMBOL(intel_mid_gpadc_alloc);
+
+static int __devinit msic_gpadc_probe(struct platform_device *pdev)
+{
+	struct gpadc_info *mgi = &gpadc_info;
+
+	mutex_init(&mgi->lock);
+	init_waitqueue_head(&mgi->wait);
+	init_waitqueue_head(&mgi->trimming_wait);
+	mgi->workq = create_singlethread_workqueue(dev_name(&pdev->dev));
+	if (mgi->workq == NULL)
+		return -ENOMEM;
+
+	mgi->dev = &pdev->dev;
+	mgi->irq = platform_get_irq(pdev, 0);
+
+	gpadc_clear_bits(INTEL_MSIC_IRQLVL1MSK, IRQLVL1MSK_ADCM);
+	if (request_threaded_irq(mgi->irq, msic_gpadc_isr, msic_gpadc_irq,
+					IRQF_ONESHOT, "msic_adc", mgi)) {
+		dev_err(&pdev->dev, "unable to register irq %d\n", mgi->irq);
+		return -ENODEV;
+	}
+
+	gpadc_write(INTEL_MSIC_ADC1ADDR0, MSIC_STOPCH);
+	INIT_WORK(&mgi->trimming_work, gpadc_trimming);
+	queue_work(mgi->workq, &mgi->trimming_work);
+	wait_event(mgi->trimming_wait, mgi->trimming_start);
+	mgi->initialized = 1;
+
+	return 0;
+}
+
+static int __devexit msic_gpadc_remove(struct platform_device *pdev)
+{
+	struct gpadc_info *mgi = &gpadc_info;
+
+	free_irq(mgi->irq, mgi);
+	flush_workqueue(mgi->workq);
+	destroy_workqueue(mgi->workq);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int msic_gpadc_suspend_noirq(struct device *dev)
+{
+	struct gpadc_info *mgi = &gpadc_info;
+
+	/* If the gpadc is locked, it means gpadc is still in active mode. */
+	if (mutex_trylock(&mgi->lock))
+		return 0;
+	else
+		return -EBUSY;
+}
+
+static int msic_gpadc_resume_noirq(struct device *dev)
+{
+	struct gpadc_info *mgi = &gpadc_info;
+
+	mutex_unlock(&mgi->lock);
+	return 0;
+}
+#else
+#define msic_gpadc_suspend_noirq    NULL
+#define msic_gpadc_resume_noirq     NULL
+#endif
+
+static const struct dev_pm_ops msic_gpadc_driver_pm_ops = {
+	.suspend_noirq	= msic_gpadc_suspend_noirq,
+	.resume_noirq	= msic_gpadc_resume_noirq,
+};
+
+static struct platform_driver msic_gpadc_driver = {
+	.driver = {
+		   .name = "msic_adc",
+		   .owner = THIS_MODULE,
+		   .pm = &msic_gpadc_driver_pm_ops,
+		   },
+	.probe = msic_gpadc_probe,
+	.remove = __devexit_p(msic_gpadc_remove),
+};
+
+static int __init msic_gpadc_module_init(void)
+{
+	return platform_driver_register(&msic_gpadc_driver);
+}
+
+static void __exit msic_gpadc_module_exit(void)
+{
+	platform_driver_unregister(&msic_gpadc_driver);
+}
+
+module_init(msic_gpadc_module_init);
+module_exit(msic_gpadc_module_exit);
+
+MODULE_AUTHOR("Jenny TC <jenny.tc@intel.com>");
+MODULE_DESCRIPTION("Intel Medfield MSIC GPADC Driver");
+MODULE_LICENSE("GPL");
diff --git a/arch/x86/platform/mrst/mrst.c b/arch/x86/platform/mrst/mrst.c
index a810696..8cefd1b 100644
--- a/arch/x86/platform/mrst/mrst.c
+++ b/arch/x86/platform/mrst/mrst.c
@@ -792,6 +792,11 @@ static void *msic_generic_platform_data(void *info, enum intel_msic_block block)
 	return no_platform_data(info);
 }
 
+static void *msic_adc_platform_data(void *info)
+{
+	return msic_generic_platform_data(info, INTEL_MSIC_BLOCK_ADC);
+}
+
 static void *msic_battery_platform_data(void *info)
 {
 	return msic_generic_platform_data(info, INTEL_MSIC_BLOCK_BATTERY);
@@ -880,6 +885,7 @@ static const struct devs_id __initconst device_ids[] = {
 	{"smb347", SFI_DEV_TYPE_I2C, 0, &smb347_platform_data},
 
 	/* MSIC subdevices */
+	{"msic_adc", SFI_DEV_TYPE_IPC, 1, &msic_adc_platform_data},
 	{"msic_battery", SFI_DEV_TYPE_IPC, 1, &msic_battery_platform_data},
 	{"msic_gpio", SFI_DEV_TYPE_IPC, 1, &msic_gpio_platform_data},
 	{"msic_audio", SFI_DEV_TYPE_IPC, 1, &msic_audio_platform_data},

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Alan Cox]

On Tue, 10 Apr 2012 14:12:06 +0100
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Tue, Apr 10, 2012 at 02:20:05PM +0100, Alan Cox wrote:
> > This ended in a discussion of where shall we put it and then silence so
> > resending this because we still need it and we've got patches that depend
> > upon it getting backlogged.
> 
> The decision seems fairly settled that general purpose ADCs like this

It's far lower level than things like IIO. It provides very low level
services to other drivers not to user space. You could in theory write an
IIO driver to use the interface but at the moment all the consumers are
low level hardware drivers and likely to remain so.

Alan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

On Tue, Apr 10, 2012 at 02:25:01PM +0100, Alan Cox wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> > The decision seems fairly settled that general purpose ADCs like this

> It's far lower level than things like IIO. It provides very low level
> services to other drivers not to user space. You could in theory write an
> IIO driver to use the interface but at the moment all the consumers are
> low level hardware drivers and likely to remain so.

Right, but fundamentally it's just a general purpose ADC which looks
just the same as all the other SoC/PMIC ADCs.  Like I say the decision
for that hardware was to push it in via IIO.  If we don't do that we're
just going to be stuck with two subsystems doing very similar things.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Alan Cox]

> Right, but fundamentally it's just a general purpose ADC which looks
> just the same as all the other SoC/PMIC ADCs.  Like I say the decision
> for that hardware was to push it in via IIO. 

Your decision maybe. And it's one that makes no sense.

As I said this is a low level interface for driver enabling including
early boot time stuff, and its on a platform that's unlikely to want or
ever use and suck in the big blob of IIO code.

If you want an IIO interface to it you can write one, it's just a low
level resource manager.

Alan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

On Tue, Apr 10, 2012 at 02:42:35PM +0100, Alan Cox wrote:
> > Right, but fundamentally it's just a general purpose ADC which looks
> > just the same as all the other SoC/PMIC ADCs.  Like I say the decision
> > for that hardware was to push it in via IIO. 

> Your decision maybe. And it's one that makes no sense.

> As I said this is a low level interface for driver enabling including

It's not just me, it's where all the ARM SoCs and their PMICs are going.
If you look on the IIO list you'll see patches for at least the AT91 

> early boot time stuff, and its on a platform that's unlikely to want or
> ever use and suck in the big blob of IIO code.

Could you be more specific about what this early boot time stuff is?
Looking at the changelogs in there it all looks like the standard
battery monitoring and power supply stuff that these ADCs get used for -
just based on the changelogs there doesn't appear to be anything
remarkable here.

There's already a standard adaptor to map IIO into at least hwmon, not
sure about the power supply subsystem.  There were also some other
things people were doing in areas like thermal management that were
using this sort of hardware in generic ways.

> If you want an IIO interface to it you can write one, it's just a low
> level resource manager.

We can't just keep on going round adding new custom interfaces every
time someone supports a new SoC - it means we end up having to sit and
write lots of per-device adaptor code to integrate things and it's
hard for us to factor common code out of drivers when we've not got any
kind of framework for them.

If IIO is totally unsuitable then someone needs to make the case for a
new subsystem for this class of hardware and provide that subsystem but
it seems entirely obvious that there is a general class of hardware here
which is widely implemented.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Alan Cox]

> Could you be more specific about what this early boot time stuff is?
> Looking at the changelogs in there it all looks like the standard
> battery monitoring and power supply stuff that these ADCs get used for -
> just based on the changelogs there doesn't appear to be anything
> remarkable here.

It depends on the actual device but things the like battery management
are a key one.

> We can't just keep on going round adding new custom interfaces every
> time someone supports a new SoC - it means we end up having to sit and

We can't go around blocking entire platforms because of the IIO blob. I
raised this point with the whole previous *generation* of Intel SoC
devices about IIO and nothing has been done about it.

Get IIO out of staing and we can look at it, until then IIO is staging
code, it's not part of the kernel, it may never be part of the kernel,
and it should never block actual kernel code.

Alan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

On Tue, Apr 10, 2012 at 03:15:29PM +0100, Alan Cox wrote:
> > Could you be more specific about what this early boot time stuff is?
> > Looking at the changelogs in there it all looks like the standard
> > battery monitoring and power supply stuff that these ADCs get used for -
> > just based on the changelogs there doesn't appear to be anything
> > remarkable here.

> It depends on the actual device but things the like battery management
> are a key one.

Right, like I say that all sounds totally standard and unremarkable.

> > We can't just keep on going round adding new custom interfaces every
> > time someone supports a new SoC - it means we end up having to sit and

> We can't go around blocking entire platforms because of the IIO blob. I
> raised this point with the whole previous *generation* of Intel SoC
> devices about IIO and nothing has been done about it.

Including by Intel, of course.

> Get IIO out of staing and we can look at it, until then IIO is staging
> code, it's not part of the kernel, it may never be part of the kernel,
> and it should never block actual kernel code.

That's not where the rest of the embedded community has been coming from
on this stuff and from a deployment point of view staging isn't really
that big a blocker to users.  We've had a lot of experience with trying
to follow that approach and the results haven't been great thus far.

Frankly at this point I don't understand why we can't just lift IIO out
of staging as-is, perhaps with the userspace ABI nobbled or moved into
debugfs for the time being if that's still a concern.  Alternatively
there is the option of you proposing some other generic framework.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Alan Cox]

> > We can't go around blocking entire platforms because of the IIO blob. I
> > raised this point with the whole previous *generation* of Intel SoC
> > devices about IIO and nothing has been done about it.
> 
> Including by Intel, of course.

Given the responses I got to moving it out of staging were of the "not
yet" and "no" variety it hardly got a request to be worked upon.

> > Get IIO out of staing and we can look at it, until then IIO is staging
> > code, it's not part of the kernel, it may never be part of the kernel,
> > and it should never block actual kernel code.
> 
> That's not where the rest of the embedded community has been coming from
> on this stuff and from a deployment point of view staging isn't really
> that big a blocker to users

Non-staging code cannot depend upon staging code. That is the rule GregKH
laid down. The Intel drivers involved are established non staging drivers
and the gpadc layer is basically cleaning up the fact they all do this
themselves in private right now without any central co-ordination or
abstraction.

> Frankly at this point I don't understand why we can't just lift IIO out
> of staging as-is, perhaps with the userspace ABI nobbled or moved into
> debugfs for the time being if that's still a concern.  Alternatively
> there is the option of you proposing some other generic framework.

I've been saying this for over a year. It's still a huge blob of code
that isn't needed for pure low level stuff, but that's fixable and
something which can eventually be worked upon.

You are trying to make the tail wag the dog. Staging is basically out of
kernel. The x86 stuff is *in kernel*, this driver is the basis of
cleaning it up. We can sort out making it talk to IIO later. Right now
all you are doing is blocking a pile of much needed code cleanups, and
without them you *won't* have an API for the gpadc on Intel, it'll be
hardcoded in each driver still. That will mean you have *zero* change of
getting IIO support at all.

Whether gpadc then adopts an IIO interface is a question to ask later,
after (if) IIO is ever merged. If the platform ever starts using gpadc
for non internal hardware stuff then I'm pretty sure it'll end up adopting
whatever generic GPADC layer eventually emerges simply because we'll want
to share drivers.

Alan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4766 bytes --]

On Tue, Apr 10, 2012 at 05:56:32PM +0100, Alan Cox wrote:
> > > We can't go around blocking entire platforms because of the IIO blob. I
> > > raised this point with the whole previous *generation* of Intel SoC
> > > devices about IIO and nothing has been done about it.

> > Including by Intel, of course.

> Given the responses I got to moving it out of staging were of the "not
> yet" and "no" variety it hardly got a request to be worked upon.

The "not yet" bit of it certainly seems like something that could be
worked on, presumably "not yet" included a "we need to do X first"
element.

> > That's not where the rest of the embedded community has been coming from
> > on this stuff and from a deployment point of view staging isn't really
> > that big a blocker to users

> Non-staging code cannot depend upon staging code. That is the rule GregKH
> laid down. The Intel drivers involved are established non staging drivers
> and the gpadc layer is basically cleaning up the fact they all do this
> themselves in private right now without any central co-ordination or
> abstraction.

So, that's a bit different and not at all obvious from your e-mail - the
diffstat shows only the code you're adding, not the code you've factored
out of the existing mainline drivers.  The diffstat you posted was:

| arch/x86/include/asm/intel_mid_gpadc.h   |   13 +
| arch/x86/platform/mrst/Makefile          |    1
| arch/x86/platform/mrst/intel_mid_gpadc.c |  645 ++++++++++++++++++++++++++++++
| arch/x86/platform/mrst/mrst.c            |    6
| 4 files changed, 665 insertions(+), 0 deletions(-)

which is a pure addition of code and I'm not seeing anything in the
changelog about this either.

> > Frankly at this point I don't understand why we can't just lift IIO out
> > of staging as-is, perhaps with the userspace ABI nobbled or moved into
> > debugfs for the time being if that's still a concern.  Alternatively
> > there is the option of you proposing some other generic framework.

> I've been saying this for over a year. It's still a huge blob of code

I've CCed in Greg and Jonathan here - guys, what is happening with
getting IIO out of staging?  It's been there since 2009 and from an
outside point of view it's really difficult to see progress here, the
time taken to merge the subsystem seems really long.

To me it really feels like the subsystem is pretty mature at this point
and that if anything staging is blocking things rather than helping
them, the subsystem feels like it's getting normal development rather
than work to integrate it into the rest of the kernel and being in
staging is discouraging users who don't absolutely need to use it.

For example, when we get extcon (or whatever it ends up being called)
merged we'll either have to start writing a raft of auxadc specific
drivers for it or we'll have to come up with *some* kind of subsystem to
use to abstract out the standard DC measurement pattern.

If the code was moved out of staging today what would go wrong?

> that isn't needed for pure low level stuff, but that's fixable and
> something which can eventually be worked upon.

We do have to be careful about this sort of "this is a little bit of low
level code" thing - it (along with "our hardware is unique") comes up
rather a lot and it's often missing a good chunk of the picture.

> You are trying to make the tail wag the dog. Staging is basically out of

You keep saying "you" here.  To repeat once more, this is pretty much
what the general embedded community has been pushing towards, this is
not particularly my personal opinion.

> kernel. The x86 stuff is *in kernel*, this driver is the basis of
> cleaning it up. We can sort out making it talk to IIO later. Right now
> all you are doing is blocking a pile of much needed code cleanups, and
> without them you *won't* have an API for the gpadc on Intel, it'll be
> hardcoded in each driver still. That will mean you have *zero* change of
> getting IIO support at all.

Like I said above you'd not posted the patches which factor the code out
of the existing users, if that's what you're trying to do that's a bit
different to what your patch looked like.

> Whether gpadc then adopts an IIO interface is a question to ask later,
> after (if) IIO is ever merged. If the platform ever starts using gpadc
> for non internal hardware stuff then I'm pretty sure it'll end up adopting
> whatever generic GPADC layer eventually emerges simply because we'll want
> to share drivers.

Again, this is the sort of thing that SoC vendors routinely say but
don't end up doing quite so routinely as one would hope.  This is
understandable as this sort of thing is more of a problem for people
working in off-SoC devices and system integration than the SoC vendors
themselves.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/10/2012 06:58 PM, Mark Brown wrote:
> On Tue, Apr 10, 2012 at 05:56:32PM +0100, Alan Cox wrote:
>>>> We can't go around blocking entire platforms because of the
>>>> IIO blob. I raised this point with the whole previous
>>>> *generation* of Intel SoC devices about IIO and nothing has
>>>> been done about it.
> 
>>> Including by Intel, of course.
> 
>> Given the responses I got to moving it out of staging were of the
>> "not yet" and "no" variety it hardly got a request to be worked
>> upon.
> 
> The "not yet" bit of it certainly seems like something that could
> be worked on, presumably "not yet" included a "we need to do X
> first" element.
> 
Entirely possibly I gave my usual evasive set of a few things that
were top of the pile to knock over!  I've been promising a 'state of
iio email' for a while now. Sorry about that.

I was anxious to not move anything out of staging that we 'knew' needed
a major refactoring.  There is one more such refactoring going through
revisions at the moment. (Push in kernel interfaces to allow for
interrupt driven users such as a client iio driver that pushes data to
input).  Last attempt to move out of staging unfortunately fell foul of
some issues Greg KH raised in a review (correctly!)  These were with the
interfaces for requesting in kernel access to IIO device channels and we
really needed to get that right so I pulled back from sending a pull
request to Linus.  Since then time has been tight on my side. Some
others have been very helpful in picking up the slack.

Anyhow to repeat my whats left comments.

1)  Review of code.  This is crucial. If people have a little time
ripping holes in the core IIO code is what we need.  Arnd did a good job
of this a while back. Others have done bits of it since.

2) Getting the push code tidied up and pushed out.  I'll post it as an
updated rfc to linux-iio shortly.  All I had left that definitely
wanted doing here was cleaning up the example iio to input bridge
driver.  That can happen later.

Other bits that are less than ideal.

* Event passing to consumers else where in the kernel. Right now an
input driver can readings from a sensor, but there is no way of
requesting threshold interrupts.

* Interaction between consumer drivers (e.g. hwmon or input) where some
are requesting data by polling when they want it and others want a
stream of data driven by interrupts. I can give details of one
approach to this to anyone interested but it's basically a simply one
element buffer and some changes to channel requesting (entirely in the
IIO core
- - external drivers will just see requests that would previously be
refused start working).

>>> That's not where the rest of the embedded community has been
>>> coming from on this stuff and from a deployment point of view
>>> staging isn't really that big a blocker to users
> 
>> Non-staging code cannot depend upon staging code. That is the
>> rule GregKH laid down. The Intel drivers involved are established
>> non staging drivers and the gpadc layer is basically cleaning up
>> the fact they all do this themselves in private right now without
>> any central co-ordination or abstraction.
> 
> So, that's a bit different and not at all obvious from your e-mail
> - the diffstat shows only the code you're adding, not the code
> you've factored out of the existing mainline drivers.  The diffstat
> you posted was:
> 
> | arch/x86/include/asm/intel_mid_gpadc.h   |   13 + |
> arch/x86/platform/mrst/Makefile          |    1 |
> arch/x86/platform/mrst/intel_mid_gpadc.c |  645
> ++++++++++++++++++++++++++++++ | arch/x86/platform/mrst/mrst.c
> |    6 | 4 files changed, 665 insertions(+), 0 deletions(-)
> 
> which is a pure addition of code and I'm not seeing anything in
> the changelog about this either.
> 
>>> Frankly at this point I don't understand why we can't just lift
>>> IIO out of staging as-is, perhaps with the userspace ABI
>>> nobbled or moved into debugfs for the time being if that's
>>> still a concern.  Alternatively there is the option of you
>>> proposing some other generic framework.
> 
>> I've been saying this for over a year. It's still a huge blob of
>> code
> 
> I've CCed in Greg and Jonathan here - guys, what is happening with 
> getting IIO out of staging?  It's been there since 2009 and from
> an outside point of view it's really difficult to see progress
> here, the time taken to merge the subsystem seems really long.
Agreed :(

> 
> To me it really feels like the subsystem is pretty mature at this
> point and that if anything staging is blocking things rather than
> helping them, the subsystem feels like it's getting normal
> development rather than work to integrate it into the rest of the
> kernel and being in staging is discouraging users who don't
> absolutely need to use it.
> 
> For example, when we get extcon (or whatever it ends up being
> called) merged we'll either have to start writing a raft of auxadc
> specific drivers for it or we'll have to come up with *some* kind
> of subsystem to use to abstract out the standard DC measurement
> pattern.
> 
> If the code was moved out of staging today what would go wrong?
Churn in interfaces is probably about it.  Maybe a good use of any time
would be for people to take their non IIO drivers that they think might
fit (or data sheets!) and see whether there are things that they would
like to be different.

An initial move would just take (possibly with some file name tidying up!)

industrialio-core.c -> core.c
industrialio-trigger.c -> trigger.c
industrialio-event.c -> event.c
inkern.c -> inkern.c
kfifo-buf.c  -> buffer_kfifo.c

iio.h
buffer.h
driver.h
machine.h
consumer.h
trigger.h
trigger_consumer.h
types.h
iio_core.h
kfifo_buf.h
sysfs.h (stripped of a few legacy bits at the end that can go into a
sysfs-staging.h until the users are all cleaned up).

Some name stripping etc needed and a wholesale switch of headers.

Doing it this way would be a lot less painful than the build up approach
we suggested last time, but more controversial as this is large complex
code drop (to anyone who hasn't followed it in staging).

> 
>> that isn't needed for pure low level stuff, but that's fixable
>> and something which can eventually be worked upon.
> 
> We do have to be careful about this sort of "this is a little bit
> of low level code" thing - it (along with "our hardware is unique")
> comes up rather a lot and it's often missing a good chunk of the
> picture.
> 
>> You are trying to make the tail wag the dog. Staging is basically
>> out of
> 
> You keep saying "you" here.  To repeat once more, this is pretty
> much what the general embedded community has been pushing towards,
> this is not particularly my personal opinion.
> 
>> kernel. The x86 stuff is *in kernel*, this driver is the basis
>> of cleaning it up. We can sort out making it talk to IIO later.
>> Right now all you are doing is blocking a pile of much needed
>> code cleanups, and without them you *won't* have an API for the
>> gpadc on Intel, it'll be hardcoded in each driver still. That
>> will mean you have *zero* change of getting IIO support at all.
> 
> Like I said above you'd not posted the patches which factor the
> code out of the existing users, if that's what you're trying to do
> that's a bit different to what your patch looked like.
> 
>> Whether gpadc then adopts an IIO interface is a question to ask
>> later, after (if) IIO is ever merged. If the platform ever starts
>> using gpadc for non internal hardware stuff then I'm pretty sure
>> it'll end up adopting whatever generic GPADC layer eventually
>> emerges simply because we'll want to share drivers.
> 
> Again, this is the sort of thing that SoC vendors routinely say
> but don't end up doing quite so routinely as one would hope.  This
> is understandable as this sort of thing is more of a problem for
> people working in off-SoC devices and system integration than the
> SoC vendors themselves.

The 'fun' bit for most IIO developers is that we weren't originally
interested in SoCs at all.  The support we've added for in kernel
users of IIO has all come out of Mark and other's comments on how
it would be useful to them.  It's been interesting and there have
been considerable gains for our other usecases, but it has take time
to do.

Anyhow, if people have spare cycles, then as ever review time is what
we need, even better if people are willing to dive in and propose
fixes to stuff they don't like.

Now that was probably mostly irrelevant to the majority of the
the thread :)

Jonathan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPhIw4AAoJEFSFNJnE9BaIEmAP/2l/Ks11td0SGyU1uDz394V3
PuTPtzAxtGpHPApxafHnN7ci270N2YloSGefP3qZ5bcMwbj8XBOw4VQ8M2rWuSWa
xghMmc1IxfYa4BsazsvEEtlU1QZq6DTjrh51azXv6sgiJcFOttPCnIAxpJnnEQXG
EVb7guUnQhzzgNCAA11up2DHohvB4CGig/jx5d7JO2puhSw483+qEzY71bW0T6FA
NNXZF8zTsppcMg/YKYheCyJ2ihDUQXrhMUDc14Pz3UHiSx9q2sGrygOOnPNBoP5r
HTRe3sKCpEQrPjavX3f6l0BHGd4EW4UHwQ+ZELY2SYiXqcltleuw3EjGMQA9j/6z
p2opkGXMI2Zw7oI4C4XZ6ivpFQxUjpeAnNGhGHNDVQWUuXDsNmc/z6yYwW8CN1sn
BgAueu9hdoxQF7AoQyQ9cl6PC93aUvAkzf5TbNAfP+8Rsboo6xqxKAJGq2ddFkad
KA5Zpxe5VbalmNlMUeNmad6A1KSJe1XfPv+L1heAL+dE+w18pFUmaN8HYghDSOYV
IUqxGJ2VjEA+irYndYv+qjWfojpv8xrUc2WSJaYjclAWEm+CmTHTblzro5oDZE+C
tZjYGz1LobGSIO13hQqW3d1QrIDIt3Vx7Z6iROBSPbll44PaDPXV3DVZh8YiLn97
eLxK5n3x0jNJwVt8ACbO
=AbrL
-----END PGP SIGNATURE-----

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:

> 1)  Review of code.  This is crucial. If people have a little time
> ripping holes in the core IIO code is what we need.  Arnd did a good job
> of this a while back. Others have done bits of it since.

> 2) Getting the push code tidied up and pushed out.  I'll post it as an
> updated rfc to linux-iio shortly.  All I had left that definitely
> wanted doing here was cleaning up the example iio to input bridge
> driver.  That can happen later.

For these two can we refactor in place?  That's pretty much what seems
to have been happening anyway...

> * Event passing to consumers else where in the kernel. Right now an
> input driver can readings from a sensor, but there is no way of
> requesting threshold interrupts.

> * Interaction between consumer drivers (e.g. hwmon or input) where some
> are requesting data by polling when they want it and others want a

These sound like something that can be added incrementally?

> > If the code was moved out of staging today what would go wrong?

> Churn in interfaces is probably about it.  Maybe a good use of any time

I guess the big question is then if we can live with that.

> would be for people to take their non IIO drivers that they think might
> fit (or data sheets!) and see whether there are things that they would
> like to be different.

In tree there's a few auxadc and comparator drivers in drivers/mfd, plus
things like arch/arm/plat-samsung/adc.c in the arch direcories.  These
are all broadly similar to the at91 code that's been sent to IIO
already.  There's also the code Alan posted at the top of this thread.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

>On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
>
>> 1)  Review of code.  This is crucial. If people have a little time
>> ripping holes in the core IIO code is what we need.  Arnd did a good
>job
>> of this a while back. Others have done bits of it since.
>
>> 2) Getting the push code tidied up and pushed out.  I'll post it as
>an
>> updated rfc to linux-iio shortly.  All I had left that definitely
>> wanted doing here was cleaning up the example iio to input bridge
>> driver.  That can happen later.
>
>For these two can we refactor in place?  That's pretty much what seems
>to have been happening anyway...
>
I guess it comes down to whether Linus will pull.  2 should be there within a week or so anyway depending mostly on analog testing I haven't broken any of their drivers.
>> * Event passing to consumers else where in the kernel. Right now an
>> input driver can readings from a sensor, but there is no way of
>> requesting threshold interrupts.
>
>> * Interaction between consumer drivers (e.g. hwmon or input) where
>some
>> are requesting data by polling when they want it and others want a
>
>These sound like something that can be added incrementally?
>
>> > If the code was moved out of staging today what would go wrong?
>
>> Churn in interfaces is probably about it.  Maybe a good use of any
>time
>
>I guess the big question is then if we can live with that.

Agreed.  
>> would be for people to take their non IIO drivers that they think
>might
>> fit (or data sheets!) and see whether there are things that they
>would
>> like to be different.
>
>In tree there's a few auxadc and comparator drivers in drivers/mfd,
>plus
>things like arch/arm/plat-samsung/adc.c in the arch direcories.  These
>are all broadly similar to the at91 code that's been sent to IIO
>already.  There's also the code Alan posted at the top of this thread.
Quite a lot of things in miscellaneous as well to possibly pull in over time.



-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

>On Tue, Apr 10,=
 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
>
>> 1)  Review of code.=
  This is crucial. If people have a little time
>> ripping holes in the cor=
e IIO code is what we need.  Arnd did a good
>job
>> of this a while back. =
Others have done bits of it since.
>
>> 2) Getting the push code tidied up =
and pushed out.  I'll post it as
>an
>> updated rfc to linux-iio shortly.  =
All I had left that definitely
>> wanted doing here was cleaning up the exa=
mple iio to input bridge
>> driver.  That can happen later.
>
>For these tw=
o can we refactor in place?  That's pretty much what seems
>to have been ha=
ppening anyway...
>
I guess it comes down to whether Linus will pull.  2 sh=
ould be there within a week or so anyway depending mostly on analog testing=
 I haven't broken any of their drivers.
>> * Event passing to consumers els=
e where in the kernel. Right now an
>> input driver can readings from a sen=
sor, but there is no way of
>> requesting threshold interrupts.
>
>> * Inte=
raction between consumer drivers (e.g. hwmon or input) where
>some
>> are r=
equesting data by polling when they want it and others want a
>
>These soun=
d like something that can be added incrementally?
>
>> > If the code was mo=
ved out of staging today what would go wrong?
>
>> Churn in interfaces is p=
robably about it.  Maybe a good use of any
>time
>
>I guess the big questio=
n is then if we can live with that.

Agreed.  
>> would be for people to ta=
ke their non IIO drivers that they think
>might
>> fit (or data sheets!) an=
d see whether there are things that they
>would
>> like to be different.
>
=
>In tree there's a few auxadc and comparator drivers in drivers/mfd,
>plus
=
>things like arch/arm/plat-samsung/adc.c in the arch direcories.  These
>ar=
e all broadly similar to the at91 code that's been sent to IIO
>already.  T=
here's also the code Alan posted at the top of this thread.
Quite a lot of =
things in miscellaneous as well to possibly pull in over time.



-- 
Sent =
from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

This is a fairly stupid place to reply but I don't have any way of 
replying to the original
patch whilst at work.  Looking quickly at it in a web browser, looks 
like we fundamentally
have a couple of interfaces exported. An alloc and free pair and two 
types of sample?
The second, during gsmpulse sample is some sort of triggered capture?

Mapping to what is now in IIO (and hasn't been for that long!).
We handle alloc and free via a mapping table that registered with the 
core by the
consumer driver (often coming from platform data).

Raw channel reads are available via polling or you can register a 
callback function
(that's the bit that is going through review at the moment) if you have 
a buffered
situation.  So the missing bit is that we don't have a 'request n 
samples' call.
We also don't currently have the functions to switch the triggering 
mechanism from
consumers (though they should be easy to add).

Anyhow, those are the differences and looks like IIO needs a few small 
tweaks to
meet the interface requirements if you did decide to go that way.  The 
only one
I'm not immediately sure how to handle is the 'read n samples' bit.  A 
self unregistering
version of the callback buffer might do it reasonably cleanly....
What you have may correspond to an arbitary scan.  The question is do 
you ever do
uneven reading of different channels?  e.g. AABAAB.

Jonathan
>
> Mark Brown<broonie@opensource.wolfsonmicro.com>  wrote:
>
>> On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
>>
>>> 1)  Review of code.  This is crucial. If people have a little time
>>> ripping holes in the core IIO code is what we need.  Arnd did a good
>> job
>>> of this a while back. Others have done bits of it since.
>>> 2) Getting the push code tidied up and pushed out.  I'll post it as
>> an
>>> updated rfc to linux-iio shortly.  All I had left that definitely
>>> wanted doing here was cleaning up the example iio to input bridge
>>> driver.  That can happen later.
>> For these two can we refactor in place?  That's pretty much what seems
>> to have been happening anyway...
>>
> I guess it comes down to whether Linus will pull.  2 should be there within a week or so anyway depending mostly on analog testing I haven't broken any of their drivers.
>>> * Event passing to consumers else where in the kernel. Right now an
>>> input driver can readings from a sensor, but there is no way of
>>> requesting threshold interrupts.
>>> * Interaction between consumer drivers (e.g. hwmon or input) where
>> some
>>> are requesting data by polling when they want it and others want a
>> These sound like something that can be added incrementally?
>>
>>>> If the code was moved out of staging today what would go wrong?
>>> Churn in interfaces is probably about it.  Maybe a good use of any
>> time
>>
>> I guess the big question is then if we can live with that.
> Agreed.
>>> would be for people to take their non IIO drivers that they think
>> might
>>> fit (or data sheets!) and see whether there are things that they
>> would
>>> like to be different.
>> In tree there's a few auxadc and comparator drivers in drivers/mfd,
>> plus
>> things like arch/arm/plat-samsung/adc.c in the arch direcories.  These
>> are all broadly similar to the at91 code that's been sent to IIO
>> already.  There's also the code Alan posted at the top of this thread.
> Quite a lot of things in miscellaneous as well to possibly pull in over time.
>
>
>

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Alan Cox]

> > Non-staging code cannot depend upon staging code. That is the rule GregKH
> > laid down. The Intel drivers involved are established non staging drivers
> > and the gpadc layer is basically cleaning up the fact they all do this
> > themselves in private right now without any central co-ordination or
> > abstraction.
> 
> So, that's a bit different and not at all obvious from your e-mail - the
> diffstat shows only the code you're adding, not the code you've factored
> out of the existing mainline drivers.  The diffstat you posted was:
> 
> | arch/x86/include/asm/intel_mid_gpadc.h   |   13 +
> | arch/x86/platform/mrst/Makefile          |    1
> | arch/x86/platform/mrst/intel_mid_gpadc.c |  645 ++++++++++++++++++++++++++++++
> | arch/x86/platform/mrst/mrst.c            |    6
> | 4 files changed, 665 insertions(+), 0 deletions(-)
> 
> which is a pure addition of code and I'm not seeing anything in the
> changelog about this either.

True.. the code factoring out stuff is supposed to follow once this is
sorted.

> We do have to be careful about this sort of "this is a little bit of low
> level code" thing - it (along with "our hardware is unique") comes up
> rather a lot and it's often missing a good chunk of the picture.

Yes. But it also helps - there is no user space API here so the only bit
of IIO that needs importing is small, and if the API in kernel changes
nothing userspace will break.

I don't btw think that layer should depend upon IIO though - IIO should
depend upon that layer. There's no need to drag all the rest of IIO in
for this, just as it can depend upon the gpio layer etc.

If that code gets pulled out of IIO dumped into drivers/adc and has a bit
of a different API to the gpadc code then no problem, gpadc can follow it
happily enough. IIO can use it from staging and IIO can migrate whenever.

It does make sense to think hard about userspace APIs for IIO but for
kernel APIs its really being too conservative - we break kernel APIs
every release. They are not set into stone.

Alan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

On Wed, Apr 11, 2012 at 11:24:11AM +0100, Alan Cox wrote:

> If that code gets pulled out of IIO dumped into drivers/adc and has a bit
> of a different API to the gpadc code then no problem, gpadc can follow it
> happily enough. IIO can use it from staging and IIO can migrate whenever.

> It does make sense to think hard about userspace APIs for IIO but for
> kernel APIs its really being too conservative - we break kernel APIs
> every release. They are not set into stone.

The way I keep thinking about this (which I'm sure I've suggested
before) is that we should be refactoring IIO such that the userspace ABI
is just another in-kernel consumer of the device driver bit of IIO and
that driver bit should be small enough to just use.  I think that winds
up being roughly equivalent to your suggestion but means that there's
just one place to put the drivers which seems like a win.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

On 4/11/2012 11:38 AM, Mark Brown wrote:
> On Wed, Apr 11, 2012 at 11:24:11AM +0100, Alan Cox wrote:
>
>> If that code gets pulled out of IIO dumped into drivers/adc and has a bit
>> of a different API to the gpadc code then no problem, gpadc can follow it
>> happily enough. IIO can use it from staging and IIO can migrate whenever.
IIO is about a heck of a lot other than ADCs.  Keep that in mind. They 
are a substantial
corner but we handle a lot of output devices and other input devices 
(though these
might be adc's inside, that's not what your average users think of them as).
We 'have' to ensure anything we do works for the other device types as well.
>> It does make sense to think hard about userspace APIs for IIO but for
>> kernel APIs its really being too conservative - we break kernel APIs
>> every release. They are not set into stone.
> The way I keep thinking about this (which I'm sure I've suggested
> before) is that we should be refactoring IIO such that the userspace ABI
> is just another in-kernel consumer of the device driver bit of IIO and
> that driver bit should be small enough to just use.  I think that winds
> up being roughly equivalent to your suggestion but means that there's
> just one place to put the drivers which seems like a win.
The intent is there, but there are an awful lot of corner cases that 
need working out.
Mostly these don't relate to simple adc usage. Just take a look at those 
elements
of the drivers that don't pass through the read_raw and write_raw callbacks.

It might be possible to make the userspace interfaces optional without 
too much pain.

I  know it's not ideal, but at the end of the day IIO had a rather 
different target when
we wrote it from SoC ADCs. That target of consistent userspace 
interfaces and
brute force data capture still has to be met without introducing major 
regressions.

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Alan Cox]

> >> happily enough. IIO can use it from staging and IIO can migrate whenever.
> IIO is about a heck of a lot other than ADCs.  Keep that in mind. They 
> are a substantial
> corner but we handle a lot of output devices and other input devices 
> (though these
> might be adc's inside, that's not what your average users think of them as).
> We 'have' to ensure anything we do works for the other device types as well.

At the IIO layer, but an ADC layer itself needs very very little indeed.

You've got
	allocate
	deallocate
	read_samples (block/nonblock)
	setup
	->samples() callback

and devices are either polled, IRQ driven or DMA.

Now setup is a lot of different things but those can be abstracted and
added as needed (and much probably taken from the IIO bits).

A pure ADC abstraction ought to be a very very thin layer of code.

> I  know it's not ideal, but at the end of the day IIO had a rather 
> different target when
> we wrote it from SoC ADCs. That target of consistent userspace 
> interfaces and
> brute force data capture still has to be met without introducing major 
> regressions.

I don't see the two conflicting. At one level we have a need for a simple
abstraction for low level ADC access within devices (akin to gpio). At the
level above we have a need for a consistent, sensible interface to
userspace with a stable API.

Your simple IIO examples would just use the ADC abstraction, your complex
IIO examples would use the ADC abstraction *and* layer it with IIO level
code that is mixing it with all the other needed work.

Alan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

On 4/11/2012 12:13 PM, Alan Cox wrote:
>>>> happily enough. IIO can use it from staging and IIO can migrate whenever.
>> IIO is about a heck of a lot other than ADCs.  Keep that in mind. They
>> are a substantial
>> corner but we handle a lot of output devices and other input devices
>> (though these
>> might be adc's inside, that's not what your average users think of them as).
>> We 'have' to ensure anything we do works for the other device types as well.
> At the IIO layer, but an ADC layer itself needs very very little indeed.
>
> You've got
> 	allocate
> 	deallocate
> 	read_samples (block/nonblock)
> 	setup
> 	->samples() callback

To add a few more things that are common (there are others).

Read scale, read offset.
Hardware event interrupts,
Triggering control.
Filtering control.

Some adcs may only need what you specify, others need a whole lot more.
You might term these setup I suppose, but the consumer of the data often
needs to know about them.
>
> and devices are either polled, IRQ driven or DMA.
>
> Now setup is a lot of different things but those can be abstracted and
> added as needed (and much probably taken from the IIO bits).
>
> A pure ADC abstraction ought to be a very very thin layer of code.
Except that you then end up with simple_adc abstraction and a whole host 
of more
complex abstractions on top.
>
>> I  know it's not ideal, but at the end of the day IIO had a rather
>> different target when
>> we wrote it from SoC ADCs. That target of consistent userspace
>> interfaces and
>> brute force data capture still has to be met without introducing major
>> regressions.
> I don't see the two conflicting. At one level we have a need for a simple
> abstraction for low level ADC access within devices (akin to gpio). At the
> level above we have a need for a consistent, sensible interface to
> userspace with a stable API.
We have that simple abstraction.  Dumb polled or irq driven adc stuff 
can be done cleanly
in minimal code.
What I disagree on is that the bit you have grouped into setup is 
actually separate.  That
needs to be abstracted as well.  Consumers might not care that the gain 
just doubled
because someone else requested it, but I suspect many of them will.
>
> Your simple IIO examples would just use the ADC abstraction, your complex
> IIO examples would use the ADC abstraction *and* layer it with IIO level
> code that is mixing it with all the other needed work.
I suspect you'll end up adding more and more to your adc abstraction 
till you actually
end up with most of IIO.  That's effectively what we did...   It's big 
because there are
actually not that many 'simple' adc's out there.

Jonathan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

On 4/11/2012 11:24 AM, Alan Cox wrote:
>>> Non-staging code cannot depend upon staging code. That is the rule GregKH
>>> laid down. The Intel drivers involved are established non staging drivers
>>> and the gpadc layer is basically cleaning up the fact they all do this
>>> themselves in private right now without any central co-ordination or
>>> abstraction.
>> So, that's a bit different and not at all obvious from your e-mail - the
>> diffstat shows only the code you're adding, not the code you've factored
>> out of the existing mainline drivers.  The diffstat you posted was:
>>
>> | arch/x86/include/asm/intel_mid_gpadc.h   |   13 +
>> | arch/x86/platform/mrst/Makefile          |    1
>> | arch/x86/platform/mrst/intel_mid_gpadc.c |  645 ++++++++++++++++++++++++++++++
>> | arch/x86/platform/mrst/mrst.c            |    6
>> | 4 files changed, 665 insertions(+), 0 deletions(-)
>>
>> which is a pure addition of code and I'm not seeing anything in the
>> changelog about this either.
> True.. the code factoring out stuff is supposed to follow once this is
> sorted.
>
>> We do have to be careful about this sort of "this is a little bit of low
>> level code" thing - it (along with "our hardware is unique") comes up
>> rather a lot and it's often missing a good chunk of the picture.
> Yes. But it also helps - there is no user space API here so the only bit
> of IIO that needs importing is small, and if the API in kernel changes
> nothing userspace will break.
If only it separated that well.  There are numerous bits of iio that 
need to hack into
the core structures still. I wish there weren't but they are there.
>
> I don't btw think that layer should depend upon IIO though - IIO should
> depend upon that layer. There's no need to drag all the rest of IIO in
> for this, just as it can depend upon the gpio layer etc.
I have no problem with additional abstractions below. What I want to 
avoid is having
two equivalent interfaces.
>
> If that code gets pulled out of IIO dumped into drivers/adc and has a bit
> of a different API to the gpadc code then no problem, gpadc can follow it
> happily enough. IIO can use it from staging and IIO can migrate whenever.
Just to restate this. It doesn't go in drivers/adc. That would cover a 
tiny corner of the scope
of IIO and make the ripped out bit effectively useless for the rest.  We 
ended up with IIO because
we couldn't come up with a term that meant input / output devices that 
are analog (in some sense)
but not hwmon or human input related.  Other name suggestions welcome!

I'm not against this in principle but don't have the time and it will 
have to be carefully handled
to not end up with something so limited that it is useless for IIO's 
current drivers.
Now days there is actually relatively little bloat in the IIO core.

In the short term I'll look at making the sysfs interfaces a build time 
option for a driver or two.

As ever it comes down to man hours.
>
> It does make sense to think hard about userspace APIs for IIO but for
> kernel APIs its really being too conservative - we break kernel APIs
> every release. They are not set into stone.
No problem with that.   The recent core changes have all been about 
adding enough
support for the cases Mark raised (which are pretty much the same as here).
>
> Alan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Alan Cox]

> needs to be abstracted as well.  Consumers might not care that the gain 
> just doubled because someone else requested it, but I suspect many of them will.

At the bottom layer I'd expect a second consumer of the same data to get
-EBUSY, but you are then going to tell me there are ADCs with one gain
control for several channels no doubt 8)


> end up with most of IIO.  That's effectively what we did...   It's big 
> because there are
> actually not that many 'simple' adc's out there.

Fair enough - you would be the expert there.

Alan

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

On 4/11/2012 1:30 PM, Alan Cox wrote:
>> needs to be abstracted as well.  Consumers might not care that the gain
>> just doubled because someone else requested it, but I suspect many of them will.
> At the bottom layer I'd expect a second consumer of the same data to get
> -EBUSY, but you are then going to tell me there are ADCs with one gain
> control for several channels no doubt 8)
yup.  If you can think of it, someone hardware guy will have implemented 
it! In this case
often comes down to the fact that most adcs are actually a mux on the 
front of a single
channel and the pga may be before the mux or after it (or for a laugh, 
sometimes both).

A real 'fun' one is devices that do scan reads.  That is you can only 
read certain combinations
of channels at a time and sometimes those combinations are far from 
obvious...
>
>
>> end up with most of IIO.  That's effectively what we did...   It's big
>> because there are
>> actually not that many 'simple' adc's out there.
> Fair enough - you would be the expert there.
I just wish they were all simple!

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Greg Kroah-Hartman]

On Wed, Apr 11, 2012 at 07:19:01AM +0100, Jonathan Cameron wrote:
> 
> 
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> 
> >On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
> >
> >> 1)  Review of code.  This is crucial. If people have a little time
> >> ripping holes in the core IIO code is what we need.  Arnd did a good
> >job
> >> of this a while back. Others have done bits of it since.
> >
> >> 2) Getting the push code tidied up and pushed out.  I'll post it as
> >an
> >> updated rfc to linux-iio shortly.  All I had left that definitely
> >> wanted doing here was cleaning up the example iio to input bridge
> >> driver.  That can happen later.
> >
> >For these two can we refactor in place?  That's pretty much what seems
> >to have been happening anyway...
> >
> I guess it comes down to whether Linus will pull.  2 should be there
> within a week or so anyway depending mostly on analog testing I
> haven't broken any of their drivers.

Hm, shouldn't I be the one that moves this out of staging?  :)

Anyway, all I care about to get this code out of staging is that you
feel your userspace api is "sane" and not going to change.  Your
in-kernel stuff can radically change every kernel release with no
objection from me at all.

And from what I can tell, your userspace stuff looks pretty stable now,
right?  So, I don't mind moving this all out of staging for 3.5 as-is.

If so, I'll be glad to make the change to my repo so it starts to show
up in linux-next in the "correct" place whenever you want me to.

thanks,

greg k-h

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

>On Wed, Apr 11, 2012 at 07:19:01AM +0100, Jonathan Cameron wrote:
>> 
>> 
>> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
>> 
>> >On Tue, Apr 10, 2012 at 08:39:40PM +0100, Jonathan Cameron wrote:
>> >
>> >> 1)  Review of code.  This is crucial. If people have a little time
>> >> ripping holes in the core IIO code is what we need.  Arnd did a
>good
>> >job
>> >> of this a while back. Others have done bits of it since.
>> >
>> >> 2) Getting the push code tidied up and pushed out.  I'll post it
>as
>> >an
>> >> updated rfc to linux-iio shortly.  All I had left that definitely
>> >> wanted doing here was cleaning up the example iio to input bridge
>> >> driver.  That can happen later.
>> >
>> >For these two can we refactor in place?  That's pretty much what
>seems
>> >to have been happening anyway...
>> >
>> I guess it comes down to whether Linus will pull.  2 should be there
>> within a week or so anyway depending mostly on analog testing I
>> haven't broken any of their drivers.
>
>Hm, shouldn't I be the one that moves this out of staging?  :)

Sorry. Still in mind set of last try where we a parrallel version planned..

>Anyway, all I care about to get this code out of staging is that you
>feel your userspace api is "sane" and not going to change.  Your
>in-kernel stuff can radically change every kernel release with no
>objection from me at all.
>
>And from what I can tell, your userspace stuff looks pretty stable now,
>right?  So, I don't mind moving this all out of staging for 3.5 as-is.

There are corners of the userspace abi that aren't but they aren't related to the core so we can break the abi docs in two and just move the good stuff... 


>
>If so, I'll be glad to make the change to my repo so it starts to show
>up in linux-next in the "correct" place whenever you want me to.

Will clear current queue under review then that would be great.

Note we will need to leave a lot of drivers in staging for now so move may require a few sed scripts to link up headers etc...  quite a lot of drivers are way off.

Lets start a fresh thread on the move to make sure everyone agrees on what is ready!  

Thanks as ever for all your hard work!

>thanks,
>
>greg k-h

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Greg Kroah-Hartman]

On Wed, Apr 11, 2012 at 05:30:47PM +0100, Jonathan Cameron wrote:
> >Anyway, all I care about to get this code out of staging is that you
> >feel your userspace api is "sane" and not going to change.  Your
> >in-kernel stuff can radically change every kernel release with no
> >objection from me at all.
> >
> >And from what I can tell, your userspace stuff looks pretty stable now,
> >right?  So, I don't mind moving this all out of staging for 3.5 as-is.
> 
> There are corners of the userspace abi that aren't but they aren't
> related to the core so we can break the abi docs in two and just move
> the good stuff...

Ok, that sounds like a good first step.

> >If so, I'll be glad to make the change to my repo so it starts to show
> >up in linux-next in the "correct" place whenever you want me to.
> 
> Will clear current queue under review then that would be great.
> 
> Note we will need to leave a lot of drivers in staging for now so move
> may require a few sed scripts to link up headers etc...  quite a lot
> of drivers are way off.

Why would some drivers stay?  It should only be because of the userspace
api, not because they don't work properly, that's never stopped drivers
from entering the main part of the kernel before :)

> Lets start a fresh thread on the move to make sure everyone agrees on
> what is ready!

Sounds good.

greg k-h

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Jonathan Cameron]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

>On Wed, Apr 11, 2012 at 05:30:47PM +0100, Jonathan Cameron wrote:
>> >Anyway, all I care about to get this code out of staging is that you
>> >feel your userspace api is "sane" and not going to change.  Your
>> >in-kernel stuff can radically change every kernel release with no
>> >objection from me at all.
>> >
>> >And from what I can tell, your userspace stuff looks pretty stable
>now,
>> >right?  So, I don't mind moving this all out of staging for 3.5
>as-is.
>> 
>> There are corners of the userspace abi that aren't but they aren't
>> related to the core so we can break the abi docs in two and just move
>> the good stuff...
>
>Ok, that sounds like a good first step.
>
>> >If so, I'll be glad to make the change to my repo so it starts to
>show
>> >up in linux-next in the "correct" place whenever you want me to.
>> 
>> Will clear current queue under review then that would be great.
>> 
>> Note we will need to leave a lot of drivers in staging for now so
>move
>> may require a few sed scripts to link up headers etc...  quite a lot
>> of drivers are way off.
>
>Why would some drivers stay?  It should only be because of the
>userspace
>api, not because they don't work properly, that's never stopped drivers
>from entering the main part of the kernel before :)
>
You got it in one.  They have userspace interfaces that are miles from where they should be... 
>> Lets start a fresh thread on the move to make sure everyone agrees on
>> what is ready!
>
>Sounds good.
>
>greg k-h

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On Wed, Apr 11, 2012 at 01:30:35PM +0100, Alan Cox wrote:
> > needs to be abstracted as well.  Consumers might not care that the gain 
> > just doubled because someone else requested it, but I suspect many of them will.

> At the bottom layer I'd expect a second consumer of the same data to get
> -EBUSY, but you are then going to tell me there are ADCs with one gain
> control for several channels no doubt 8)

There's also some sensible use cases like the multiple system monitoring
subsystems we've got wanting to monitor the same power rail where it's
useful to have multiple consumers sharing the ADC, though with those
ones changing the gain probably doesn't make much sense.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND] x86, intel_mid: ADC management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Wed, Apr 11, 2012 at 12:19:04PM +0100, Jonathan Cameron wrote:
> On 4/11/2012 12:13 PM, Alan Cox wrote:

> >Your simple IIO examples would just use the ADC abstraction, your complex
> >IIO examples would use the ADC abstraction *and* layer it with IIO level
> >code that is mixing it with all the other needed work.

> I suspect you'll end up adding more and more to your adc abstraction
> till you actually
> end up with most of IIO.  That's effectively what we did...   It's
> big because there are
> actually not that many 'simple' adc's out there.

I tend to agree here - I think if we try to establish a strict
separation between the simple and complex abstractions it'd cause more
problems than it will solve trying to split things, and from a hardware
driver level it helps if there's just one upper layer.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]