[PATCH] mfd: ab8500-gpadc Add new GPADC driver

[PATCH] mfd: ab8500-gpadc Add new GPADC driver

[Arun Murthy]

AB8500 GPADC driver used to convert Acc and battery/ac/usb voltage

Signed-off-by: Arun Murthy <arun.murthy@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/mfd/Kconfig              |    7 +
 drivers/mfd/Makefile             |    1 +
 drivers/mfd/ab8500-gpadc.c       |  277 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/ab8500-gpadc.h |   47 +++++++
 include/linux/mfd/ab8500.h       |    6 +
 5 files changed, 338 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/ab8500-gpadc.c
 create mode 100644 include/linux/mfd/ab8500-gpadc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index fd01836..5309534 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -523,6 +523,13 @@ config AB8500_DEBUG
          Select this option if you want debug information using the debug
          filesystem, debugfs.
 
+config AB8500_GPADC
+	bool "AB8500 GPADC driver"
+	depends on AB8500_CORE && REGULATOR_AB8500
+	default y
+	help
+	  AB8500 GPADC driver used to convert Acc and battery/ac/usb voltage
+
 config AB3550_CORE
         bool "ST-Ericsson AB3550 Mixed Signal Circuit core functions"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a54e2c7..c17ab3f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_AB3550_CORE)	+= ab3550-core.o
 obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o
 obj-$(CONFIG_AB8500_I2C_CORE)	+= ab8500-i2c.o
 obj-$(CONFIG_AB8500_DEBUG)	+= ab8500-debugfs.o
+obj-$(CONFIG_AB8500_GPADC)	+= ab8500-gpadc.o
 obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
 obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
 obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
diff --git a/drivers/mfd/ab8500-gpadc.c b/drivers/mfd/ab8500-gpadc.c
new file mode 100644
index 0000000..b2113db
--- /dev/null
+++ b/drivers/mfd/ab8500-gpadc.c
@@ -0,0 +1,277 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * License Terms: GNU General Public License v2
+ * Author: Arun R Murthy <arun.murthy@stericsson.com>
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/completion.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/mfd/ab8500.h>
+#include <linux/mfd/abx500.h>
+#include <linux/mfd/ab8500-gpadc.h>
+
+/*
+ * GPADC register offsets
+ * Bank : 0x0A
+ */
+#define AB8500_GPADC_CTRL1_REG		0x00
+#define AB8500_GPADC_CTRL2_REG		0x01
+#define AB8500_GPADC_CTRL3_REG		0x02
+#define AB8500_GPADC_AUTO_TIMER_REG	0x03
+#define AB8500_GPADC_STAT_REG		0x04
+#define AB8500_GPADC_MANDATAL_REG	0x05
+#define AB8500_GPADC_MANDATAH_REG	0x06
+#define AB8500_GPADC_AUTODATAL_REG	0x07
+#define AB8500_GPADC_AUTODATAH_REG	0x08
+#define AB8500_GPADC_MUX_CTRL_REG	0x09
+
+/* gpadc constants */
+#define EN_VINTCORE12			0x04
+#define EN_VTVOUT			0x02
+#define EN_GPADC			0x01
+#define DIS_GPADC			0x00
+#define SW_AVG_16			0x60
+#define ADC_SW_CONV			0x04
+#define EN_BUF				0x40
+#define DIS_ZERO			0x00
+#define GPADC_BUSY			0x01
+
+/**
+ * ab8500_gpadc_convert() - gpadc conversion
+ * @input:	analog input to be converted to digital data
+ *
+ * This function converts the selected analog i/p to digital
+ * data. Thereafter calibration has to be made to obtain the
+ * data in the required quantity measurement.
+ */
+int ab8500_gpadc_convert(struct ab8500_gpadc *di, u8 input)
+{
+	int ret;
+	u16 data = 0;
+	int looplimit = 0;
+	u8 val, low_data, high_data;
+
+	if (!di)
+		return -ENODEV;
+
+	mutex_lock(&di->ab8500_gpadc_lock);
+	/* Enable VTVout LDO this is required for GPADC */
+	regulator_enable(di->regu);
+
+	/* Check if ADC is not busy, lock and proceed */
+	do {
+		ret = abx500_get_register_interruptible(di->dev, AB8500_GPADC,
+			AB8500_GPADC_STAT_REG, &val);
+		if (ret < 0)
+			goto out;
+		if (!(val & GPADC_BUSY))
+			break;
+		msleep(10);
+	} while (++looplimit < 10);
+	if (looplimit >= 10 && (val & GPADC_BUSY)) {
+		dev_err(di->dev, "gpadc_conversion: GPADC busy");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Enable GPADC */
+	ret = abx500_mask_and_set_register_interruptible(di->dev, AB8500_GPADC,
+		AB8500_GPADC_CTRL1_REG, EN_GPADC, EN_GPADC);
+	if (ret < 0) {
+		dev_err(di->dev, "gpadc_conversion: enable gpadc failed\n");
+		goto out;
+	}
+	/* Select the input source and set average samples to 16 */
+	ret = abx500_set_register_interruptible(di->dev, AB8500_GPADC,
+		AB8500_GPADC_CTRL2_REG, (input | SW_AVG_16));
+	if (ret < 0) {
+		dev_err(di->dev,
+			"gpadc_conversion: set avg samples failed\n");
+		goto out;
+	}
+	/* Enable ADC, Buffering and select rising edge, start Conversion */
+	ret = abx500_mask_and_set_register_interruptible(di->dev, AB8500_GPADC,
+		AB8500_GPADC_CTRL1_REG, EN_BUF, EN_BUF);
+	if (ret < 0) {
+		dev_err(di->dev,
+			"gpadc_conversion: select falling edge failed\n");
+		goto out;
+	}
+	ret = abx500_mask_and_set_register_interruptible(di->dev, AB8500_GPADC,
+		AB8500_GPADC_CTRL1_REG, ADC_SW_CONV, ADC_SW_CONV);
+	if (ret < 0) {
+		dev_err(di->dev,
+			"gpadc_conversion: start s/w conversion failed\n");
+		goto out;
+	}
+	/* wait for completion of conversion */
+	if (!wait_for_completion_timeout(&di->ab8500_gpadc_complete, 2*HZ)) {
+		dev_err(di->dev,
+			"timeout: didnt recieve GPADC conversion interrupt\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Read the converted RAW data */
+	ret = abx500_get_register_interruptible(di->dev, AB8500_GPADC,
+		AB8500_GPADC_MANDATAL_REG, &low_data);
+	if (ret < 0) {
+		dev_err(di->dev, "gpadc_conversion: read low data failed\n");
+		goto out;
+	}
+
+	ret = abx500_get_register_interruptible(di->dev, AB8500_GPADC,
+		AB8500_GPADC_MANDATAH_REG, &high_data);
+	if (ret < 0) {
+		dev_err(di->dev, "gpadc_conversion: read high data failed\n");
+		goto out;
+	}
+
+	data = (high_data << 8) | low_data;
+	/* Disable GPADC */
+	ret = abx500_set_register_interruptible(di->dev, AB8500_GPADC,
+		AB8500_GPADC_CTRL1_REG, DIS_GPADC);
+	if (ret < 0) {
+		dev_err(di->dev, "gpadc_conversion: disable gpadc failed\n");
+		goto out;
+	}
+	/* Disable VTVout LDO this is required for GPADC */
+	regulator_disable(di->regu);
+	mutex_unlock(&di->ab8500_gpadc_lock);
+	return data;
+
+out:
+	/*
+	 * It has shown to be needed to turn off the GPADC if an error occurs,
+	 * otherwise we might have problem when waiting for the busy bit in the
+	 * GPADC status register to go low. In V1.1 there wait_for_completion
+	 * seems to timeout when waiting for an interrupt.. Not seen in V2.0
+	 */
+	(void) abx500_set_register_interruptible(di->dev, AB8500_GPADC,
+		AB8500_GPADC_CTRL1_REG, DIS_GPADC);
+	regulator_disable(di->regu);
+	mutex_unlock(&di->ab8500_gpadc_lock);
+	dev_err(di->dev, "gpadc_conversion: Failed to AD convert channel %d\n",
+		input);
+	return ret;
+}
+EXPORT_SYMBOL(ab8500_gpadc_convert);
+
+/**
+ * ab8500_bm_gpswadcconvend_handler() - isr for s/w gpadc conversion completion
+ * @irq:	irq number
+ * @data:	pointer to the data passed during request irq
+ *
+ * This is a interrupt service routine for s/w gpadc conversion completion.
+ * Notifies the gpadc completion is completed and the converted raw value
+ * can be read from the registers.
+ * Returns IRQ status(IRQ_HANDLED)
+ */
+static irqreturn_t ab8500_bm_gpswadcconvend_handler(int irq, void *_di)
+{
+	struct ab8500_gpadc *di = _di;
+
+	complete(&di->ab8500_gpadc_complete);
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit ab8500_gpadc_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct ab8500_gpadc *di;
+
+	di = kzalloc(sizeof(struct ab8500_gpadc), GFP_KERNEL);
+	if (!di) {
+		dev_err(&pdev->dev, "Error: No memory\n");
+		return -ENOMEM;
+	}
+
+	di->parent = dev_get_drvdata(pdev->dev.parent);
+	di->irq = platform_get_irq_byname(pdev, "SW_CONV_END");
+	if (di->irq < 0) {
+		dev_err(di->dev, "failed to get platform irq-%d\n", di->irq);
+		ret = di->irq;
+		goto fail;
+	}
+
+	di->dev = &pdev->dev;
+	mutex_init(&di->ab8500_gpadc_lock);
+
+	/* Initialize completion used to notify completion of conversion */
+	init_completion(&di->ab8500_gpadc_complete);
+
+	/* Register interrupt  - SwAdcComplete */
+	ret = request_threaded_irq(di->irq, NULL,
+		ab8500_bm_gpswadcconvend_handler,
+		IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc", di);
+	if (ret < 0) {
+		dev_err(di->dev, "Failed to register interrupt, irq: %d\n",
+			di->irq);
+		goto fail;
+	}
+
+	/* VTVout LDO used to power up ab8500-GPADC */
+	di->regu = regulator_get(&pdev->dev, "vddadc");
+	if (IS_ERR(di->regu)) {
+		ret = PTR_ERR(di->regu);
+		dev_err(di->dev, "failed to get vtvout LDO\n");
+		goto fail;
+	}
+	di->parent->gpadc = di;
+	dev_dbg(di->dev, "probe success\n");
+	return 0;
+fail:
+	kfree(di);
+	di = NULL;
+	return ret;
+}
+
+static int __devexit ab8500_gpadc_remove(struct platform_device *pdev)
+{
+	struct ab8500_gpadc *di = platform_get_drvdata(pdev);
+
+	/* remove interrupt  - completion of Sw ADC conversion */
+	free_irq(di->irq, di);
+	/* disable VTVout LDO that is being used by GPADC */
+	regulator_put(di->regu);
+	kfree(di);
+	di = NULL;
+	return 0;
+}
+
+static struct platform_driver ab8500_gpadc_driver = {
+	.probe = ab8500_gpadc_probe,
+	.remove = __devexit_p(ab8500_gpadc_remove),
+	.driver = {
+		.name = "ab8500-gpadc",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init ab8500_gpadc_init(void)
+{
+	return platform_driver_register(&ab8500_gpadc_driver);
+}
+
+static void __exit ab8500_gpadc_exit(void)
+{
+	platform_driver_unregister(&ab8500_gpadc_driver);
+}
+
+subsys_initcall_sync(ab8500_gpadc_init);
+module_exit(ab8500_gpadc_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Arun R Murthy");
+MODULE_ALIAS("platform:ab8500_gpadc");
+MODULE_DESCRIPTION("AB8500 GPADC driver");
diff --git a/include/linux/mfd/ab8500-gpadc.h b/include/linux/mfd/ab8500-gpadc.h
new file mode 100644
index 0000000..1b0adbb
--- /dev/null
+++ b/include/linux/mfd/ab8500-gpadc.h
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2010 ST-Ericsson SA
+ * Licensed under GPLv2.
+ *
+ * Author: Arun R Murthy <arun.murthy@stericsson.com>
+ */
+
+#ifndef	_AB8500_GPADC_H
+#define _AB8500_GPADC_H
+
+/* GPADC source: From datasheet(ADCSwSel[4:0] in GPADCCtrl2) */
+#define BAT_CTRL        0x01
+#define BTEMP_BALL      0x02
+#define MAIN_CHARGER_V  0x03
+#define ACC_DETECT1     0x04
+#define ACC_DETECT2     0x05
+#define ADC_AUX1	0x06
+#define ADC_AUX2	0x07
+#define MAIN_BAT_V      0x08
+#define VBUS_V          0x09
+#define MAIN_CHARGER_C  0x0A
+#define USB_CHARGER_C   0x0B
+#define BK_BAT_V        0x0C
+#define DIE_TEMP        0x0D
+
+/**
+ * struct ab8500_gpadc - ab8500 GPADC device information
+ * @dev:			pointer to the struct device
+ * @parent:			pointer to the parent device structure ab8500
+ * @ab8500_gpadc_complete:	pointer to the struct completion, to indicate
+ *				the completion of gpadc conversion
+ * @ab8500_gpadc_lock:		structure of type mutex
+ * @regu:			pointer to the struct regulator
+ * @irq:			interrupt number that is used by gpadc
+ */
+struct ab8500_gpadc {
+	struct device *dev;
+	struct ab8500 *parent;
+	struct completion ab8500_gpadc_complete;
+	struct mutex ab8500_gpadc_lock;
+	struct regulator *regu;
+	int irq;
+};
+
+int ab8500_gpadc_convert(struct ab8500_gpadc *di, u8 input);
+
+#endif /* _AB8500_GPADC_H */
diff --git a/include/linux/mfd/ab8500.h b/include/linux/mfd/ab8500.h
index 37f56b7..8ebc4d8 100644
--- a/include/linux/mfd/ab8500.h
+++ b/include/linux/mfd/ab8500.h
@@ -106,6 +106,9 @@
 #define AB8500_NR_IRQS			112
 #define AB8500_NUM_IRQ_REGS		14
 
+/* Forward Declaration */
+struct ab8500_gpadc;
+
 /**
  * struct ab8500 - ab8500 internal structure
  * @dev: parent device
@@ -119,6 +122,7 @@
  * @tx_buf: tx buf for SPI
  * @mask: cache of IRQ regs for bus lock
  * @oldmask: cache of previous IRQ regs for bus lock
+ * @gpadc: pointer to the ab8500 gpadc device information
  */
 struct ab8500 {
 	struct device	*dev;
@@ -137,6 +141,8 @@ struct ab8500 {
 
 	u8 mask[AB8500_NUM_IRQ_REGS];
 	u8 oldmask[AB8500_NUM_IRQ_REGS];
+
+	struct ab8500_gpadc *gpadc;
 };
 
 struct regulator_init_data;
-- 
1.7.2.dirty

Re: [PATCH] mfd: ab8500-gpadc Add new GPADC driver

[Mattias Wallin]

On 01/20/2011 11:28 AM, Arun MURTHY wrote:
> AB8500 GPADC driver used to convert Acc and battery/ac/usb voltage
> 
> Signed-off-by: Arun Murthy <arun.murthy@stericsson.com>
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> ---
>  drivers/mfd/Kconfig              |    7 +
>  drivers/mfd/Makefile             |    1 +
>  drivers/mfd/ab8500-gpadc.c       |  277 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/ab8500-gpadc.h |   47 +++++++
>  include/linux/mfd/ab8500.h       |    6 +
>  5 files changed, 338 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/mfd/ab8500-gpadc.c
>  create mode 100644 include/linux/mfd/ab8500-gpadc.h
> 
...
> +/**
> + * ab8500_gpadc_convert() - gpadc conversion
> + * @input:	analog input to be converted to digital data
> + *
> + * This function converts the selected analog i/p to digital
> + * data. Thereafter calibration has to be made to obtain the
> + * data in the required quantity measurement.
> + */
> +int ab8500_gpadc_convert(struct ab8500_gpadc *di, u8 input)
I would like this interface to change in order to remove the struct ab8500_gpadc
from the struct ab8500. I.e not restrict the users to subdriver of ab8500.
> +{
> +	int ret;
> +	u16 data = 0;
> +	int looplimit = 0;
> +	u8 val, low_data, high_data;
> +
> +	if (!di)
> +		return -ENODEV;
> +
> +	mutex_lock(&di->ab8500_gpadc_lock);
> +	/* Enable VTVout LDO this is required for GPADC */
> +	regulator_enable(di->regu);
...
> diff --git a/include/linux/mfd/ab8500.h b/include/linux/mfd/ab8500.h
> index 37f56b7..8ebc4d8 100644
> --- a/include/linux/mfd/ab8500.h
> +++ b/include/linux/mfd/ab8500.h
> @@ -106,6 +106,9 @@
>  #define AB8500_NR_IRQS			112
>  #define AB8500_NUM_IRQ_REGS		14
>  
> +/* Forward Declaration */
> +struct ab8500_gpadc;
> +
>  /**
>   * struct ab8500 - ab8500 internal structure
>   * @dev: parent device
> @@ -119,6 +122,7 @@
>   * @tx_buf: tx buf for SPI
>   * @mask: cache of IRQ regs for bus lock
>   * @oldmask: cache of previous IRQ regs for bus lock
> + * @gpadc: pointer to the ab8500 gpadc device information
>   */
>  struct ab8500 {
>  	struct device	*dev;
> @@ -137,6 +141,8 @@ struct ab8500 {
>  
>  	u8 mask[AB8500_NUM_IRQ_REGS];
>  	u8 oldmask[AB8500_NUM_IRQ_REGS];
> +
> +	struct ab8500_gpadc *gpadc;
Please remove this gpadc dependency from the struct ab8500.
>  };
>  
>  struct regulator_init_data;

Thanks and Regards,
Mattias Wallin

RE: [PATCH] mfd: ab8500-gpadc Add new GPADC driver

[Arun MURTHY]

> > + * ab8500_gpadc_convert() - gpadc conversion
> > + * @input:	analog input to be converted to digital data
> > + *
> > + * This function converts the selected analog i/p to digital
> > + * data. Thereafter calibration has to be made to obtain the
> > + * data in the required quantity measurement.
> > + */
> > +int ab8500_gpadc_convert(struct ab8500_gpadc *di, u8 input)
> I would like this interface to change in order to remove the struct
> ab8500_gpadc
> from the struct ab8500. I.e not restrict the users to subdriver of
> ab8500.

The only clients for GPADC is the battery driver and the Audio Acc
detection. Both of these are sub-modules/clients of ab8500.
None other than these can use the GPADC.
Inputs to GPADC can only be one among the following:
/* GPADC source: From datasheet(ADCSwSel[4:0] in GPADCCtrl2) */
#define BAT_CTRL        0x01
#define BTEMP_BALL      0x02
#define MAIN_CHARGER_V  0x03
#define ACC_DETECT1     0x04
#define ACC_DETECT2     0x05
#define ADC_AUX1		0x06
#define ADC_AUX2		0x07
#define MAIN_BAT_V      0x08
#define VBUS_V          0x09
#define MAIN_CHARGER_C  0x0A
#define USB_CHARGER_C   0x0B
#define BK_BAT_V        0x0C
#define DIE_TEMP        0x0D

Henceforth in order to secure the usage of GPADC, and in order to
restrict it to only EM and AUDIO sub-module, the gpadc device struct
was added to ab8500 struct. Also that the exported function
ab8500_gpadc_convert has an argument struct ab8500_gpadc, which can
be obtained be dereferencing the struct ab8500. This is possible only
with the ab8500 and its clients, thereby securing the usage to
battery driver and audio acc detect.

Thanks and Regards,
Arun R Murthy
-------------

Re: [PATCH] mfd: ab8500-gpadc Add new GPADC driver

[Mattias Wallin]

On 01/21/2011 11:31 AM, Arun MURTHY wrote:
> The only clients for GPADC is the battery driver and the Audio Acc
> detection. Both of these are sub-modules/clients of ab8500.
> None other than these can use the GPADC.
> Inputs to GPADC can only be one among the following:
> /* GPADC source: From datasheet(ADCSwSel[4:0] in GPADCCtrl2) */
> #define BAT_CTRL        0x01
> #define BTEMP_BALL      0x02
> #define MAIN_CHARGER_V  0x03
> #define ACC_DETECT1     0x04
> #define ACC_DETECT2     0x05
> #define ADC_AUX1		0x06
> #define ADC_AUX2		0x07
> #define MAIN_BAT_V      0x08
> #define VBUS_V          0x09
> #define MAIN_CHARGER_C  0x0A
> #define USB_CHARGER_C   0x0B
> #define BK_BAT_V        0x0C
> #define DIE_TEMP        0x0D
> 
> Henceforth in order to secure the usage of GPADC, and in order to
> restrict it to only EM and AUDIO sub-module, the gpadc device struct
> was added to ab8500 struct. Also that the exported function
> ab8500_gpadc_convert has an argument struct ab8500_gpadc, which can
> be obtained be dereferencing the struct ab8500. This is possible only
> with the ab8500 and its clients, thereby securing the usage to
> battery driver and audio acc detect.
Yes and I would like to remove this restriction and have a simpler more open api.
First I don't like that users needs to do a lot of pointer dereferencing in their call like ab8500_gpadc_convert(dev->parent->ab8500->gpadc, USB_CHARGER) (an example).
I prefer a get function that returns a handle that should be used as first argument in the convert calls.
It also makes sense if this driver will be extended to use more channels.
Second this driver will be used by for example accessories which likely will be called by 3 party drivers
and to make their life easier I don't want to force them to this ab8500-core dependency.
I agree however that the users must be in above list and most of them are ab8500 devices already.

Best Regards,
/Mattias Wallin

RE: [PATCH] mfd: ab8500-gpadc Add new GPADC driver

[Arun MURTHY]

> > Henceforth in order to secure the usage of GPADC, and in order to
> > restrict it to only EM and AUDIO sub-module, the gpadc device struct
> > was added to ab8500 struct. Also that the exported function
> > ab8500_gpadc_convert has an argument struct ab8500_gpadc, which can
> > be obtained be dereferencing the struct ab8500. This is possible only
> > with the ab8500 and its clients, thereby securing the usage to
> > battery driver and audio acc detect.
> Yes and I would like to remove this restriction and have a simpler more
> open api.
> First I don't like that users needs to do a lot of pointer
> dereferencing in their call like ab8500_gpadc_convert(dev->parent-
> >ab8500->gpadc, USB_CHARGER) (an example).

It's very much simple. All clients of ab500 will have a pointer to the
struct ab8500. Pointer to GPADC struct is an element in ab8500.
Hence it is something like  ab8500_gpadc_convert(di->ab8500->gpadc,
BAT_CTRL); I don't find anything complicated in this.

> I prefer a get function that returns a handle that should be used as
> first argument in the convert calls.
> It also makes sense if this driver will be extended to use more
> channels.

This is not possible. This is the limitation/the feature of the hardware.
It's not the software that limits the channel.
In the GPADC hardware, there are dedicated channel and also dedicatedly
allocated to the clients which are nothing but the battery parameters(
voltage, current, battery resistance, ac voltage, usb voltage) and the
audio acc. Apart from these no other peripheral or channel can use this.
It's not the restriction because of this implementation, but are the features
supported by the GPPDC hardware. Even if implemented considering your
comments, none other apart from the above mentioned can use(No extra
channels can be added and the existing channel are already hardwired hence
can't be modified/changed)  

> Second this driver will be used by for example accessories which likely
> will be called by 3 party drivers
This is not at all the 3rd part driver. Acc is part of audio block which
is a module in AB8500. Hence Audio will be a client of ab8500 mfd.

> and to make their life easier I don't want to force them to this
> ab8500-core dependency.
Its not that software is forcing or limiting the use to only ab8500
clients, but it's more the hardware feature.
Please refer the ab8500 datasheet for further details.

> I agree however that the users must be in above list and most of them
> are ab8500 devices already.
In that case there comes no instance where others apart from the ab8500
clients using this drivers.

Thanks and Regards,
Arun R Murthy
-------------

Re: [PATCH] mfd: ab8500-gpadc Add new GPADC driver

[Samuel Ortiz]

Hi Mattias,

On Fri, Jan 21, 2011 at 01:07:40PM +0100, Mattias Wallin wrote:
> On 01/21/2011 11:31 AM, Arun MURTHY wrote:
> > The only clients for GPADC is the battery driver and the Audio Acc
> > detection. Both of these are sub-modules/clients of ab8500.
> > None other than these can use the GPADC.
> > Inputs to GPADC can only be one among the following:
> > /* GPADC source: From datasheet(ADCSwSel[4:0] in GPADCCtrl2) */
> > #define BAT_CTRL        0x01
> > #define BTEMP_BALL      0x02
> > #define MAIN_CHARGER_V  0x03
> > #define ACC_DETECT1     0x04
> > #define ACC_DETECT2     0x05
> > #define ADC_AUX1		0x06
> > #define ADC_AUX2		0x07
> > #define MAIN_BAT_V      0x08
> > #define VBUS_V          0x09
> > #define MAIN_CHARGER_C  0x0A
> > #define USB_CHARGER_C   0x0B
> > #define BK_BAT_V        0x0C
> > #define DIE_TEMP        0x0D
> > 
> > Henceforth in order to secure the usage of GPADC, and in order to
> > restrict it to only EM and AUDIO sub-module, the gpadc device struct
> > was added to ab8500 struct. Also that the exported function
> > ab8500_gpadc_convert has an argument struct ab8500_gpadc, which can
> > be obtained be dereferencing the struct ab8500. This is possible only
> > with the ab8500 and its clients, thereby securing the usage to
> > battery driver and audio acc detect.
> Yes and I would like to remove this restriction and have a simpler more open api.
> First I don't like that users needs to do a lot of pointer dereferencing in their call like ab8500_gpadc_convert(dev->parent->ab8500->gpadc, USB_CHARGER) (an example).
>
As subdevices, I expect users to have an ab8500 pointer. So it would just be
one dereference.


> I prefer a get function that returns a handle that should be used as first argument in the convert calls.
> It also makes sense if this driver will be extended to use more channels.
> Second this driver will be used by for example accessories which likely will be called by 3 party drivers
> and to make their life easier I don't want to force them to this ab8500-core dependency.
>
As I'm not familiar with your HW architecture, could you please describe how
those accessories would wire into the ab8500 core ?
If those devices really are independent drivers (i.e. not subdevices) needing
to get an A/D conversion from the ab8500 adc (I don't see how that can happen,
hence my above question), then it might make sense to use a conversion API
independent from any ab8500 pointer. But otherwise, I prefer this API rather
than the one in v2 of this patch.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH] mfd: ab8500-gpadc Add new GPADC driver

[Mattias Wallin]

On 02/01/2011 12:36 PM, Samuel Ortiz wrote:
> As subdevices, I expect users to have an ab8500 pointer. So it would just be
> one dereference.
I do not want users to have or use the ab8500 pointer at all. I would 
like to move it from the .h file into the ab8500-core.c eventually.

> As I'm not familiar with your HW architecture, could you please describe how
> those accessories would wire into the ab8500 core ?
The accessories can for example be a simple phone headset, a carkit and 
so on. A headset wire into the 3.5mm plug and gpadc can be used to 
understand whats plugged in. Our analog baseband chip ab8500 is a 
container of subfunctionality like audio codec, digital encoder, voltage 
regulators and so on.
The idea behind ab8500-core driver is to provide register access and 
interrupt management to the subdrivers implementing the 
subfunctionality. The gpadc driver is one these subdrivers. A headset 
driver becomes a subdriver of the gpadc wich is a subdriver to 
ab8500-core so the question is how far we should enforce these hierarchy 
of drivers.
In my opinion the line goes here. The gpadc provides a service to 
convert. Open for not only subdrivers and the rational is to reduce 
complexity.
> If those devices really are independent drivers (i.e. not subdevices) needing
> to get an A/D conversion from the ab8500 adc (I don't see how that can happen,
> hence my above question), then it might make sense to use a conversion API
> independent from any ab8500 pointer. But otherwise, I prefer this API rather
> than the one in v2 of this patch.
You are absolutely right that really independent drivers of ab8500 will 
probably not be found and I understand your argument. But many parts in 
our platform have connections to ab8500 via regulators, clocks or other 
wires. The decision is instead based on design to reduce complexity. If 
a driver uses direct register access to ab8500 then it should be a 
subdriver (to enforce startup order for example) otherwise is is not 
required (in my oppinion). An accessory driver should easily be ported 
from other platforms and not be tied to ab8500.

Me and Arun got some feedback to keep our discussion internal first so 
sorry for keeping you out the last mails but the result is patch v2.

Thanks and regards,
Mattias Wallin