[PATCH v5] iio: adc: add new lp8788 adc driver

[PATCH v5] iio: adc: add new lp8788 adc driver

[Kim, Milo]

 TI LP8788 PMU provides regulators, battery charger, ADC,
 RTC, backlight driver and current sinks.

 This patch enables the LP8788 ADC functions.

 The LP8788 ADC has 13 ADC input selection and supports 12-bit resolution.
 Internal operation of getting ADC is access to registers of LP8788.
 The LP8788 ADC uses exported functions for accessing these registers.
 (exported by LP8788 MFD device driver)

 This driver supports IIO_CHAN_INFO_RAW and SCALE.
 The scale is micro unit.
 So the IIO consumer can get the result as following.

   result = ADC raw value * (scale_int + scale_part / 1000000)

 (The IIO map for the IIO consumer)

 The ADC input can be selective in the platform side.
 Even though this platform data is not defined,
 the default IIO map is created for supporting the power supply driver.
 The battery voltage and temperature are used inside this driver.

 (History)

 Patch v5.
 (a) Fix misuse of consumer_name_dev
   If the IIO consumer device is not defined in the platform side,
   the default consumer is the lp8788-charger.
   In this case, the consumer name should be defined as 'lp8788-charger'
   not driver name itself.

 (b) Add mutex for ADC read operation

 (c) Reorganization on lp8788_adc_read_raw()
   The LP8788 registers are accessed only when the mask is IIO_CHAN_INFO_RAW.
   Other cases, no need to read the registers.
   To make lp8788_adc_read_raw() simple, new lp8788_get_adc_result() is added.

 (d) make LP8788 specific names
   Add prefix LP8788_ for datasheet_name, channel and ADC channel label of
   the default IIO maps.

 Patch v4.
 Fix adc_raw function: support RAW and SCALE channel info.
 Change LP8788 ADC platform data - the IIO map.
 Enables the default IIO map.

 Patch v3.
 Fix wrong size of allocating the IIO private data.
 Fix coding styles.

 Patch v2.
 Support RAW and SCALE interface for IIO consumer.
 Clean up the IIO channel spec macro.

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/iio/adc/Kconfig      |    6 +
 drivers/iio/adc/Makefile     |    1 +
 drivers/iio/adc/lp8788_adc.c |  270 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 277 insertions(+)
 create mode 100644 drivers/iio/adc/lp8788_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8a78b4f..30c06ed 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -22,4 +22,10 @@ config AT91_ADC
 	help
 	  Say yes here to build support for Atmel AT91 ADC.
 
+config LP8788_ADC
+	bool "LP8788 ADC driver"
+	depends on MFD_LP8788
+	help
+	  Say yes here to build support for TI LP8788 ADC.
+
 endmenu
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 52eec25..72f9a76 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_AD7266) += ad7266.o
 obj-$(CONFIG_AT91_ADC) += at91_adc.o
+obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
new file mode 100644
index 0000000..6079f9e
--- /dev/null
+++ b/drivers/iio/adc/lp8788_adc.c
@@ -0,0 +1,270 @@
+/*
+ * TI LP8788 MFD - ADC driver
+ *
+ * Copyright 2012 Texas Instruments
+ *
+ * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/driver.h>
+#include <linux/iio/machine.h>
+#include <linux/mfd/lp8788.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* register address */
+#define LP8788_ADC_CONF			0x60
+#define LP8788_ADC_RAW			0x61
+#define LP8788_ADC_DONE			0x63
+
+#define ADC_CONV_START			1
+
+struct lp8788_adc {
+	struct lp8788 *lp;
+	struct iio_map *map;
+	struct mutex lock;
+};
+
+static const int lp8788_scale[LPADC_MAX] = {
+	[LPADC_VBATT_5P5] = 1343,
+	[LPADC_VIN_CHG]   = 3052,
+	[LPADC_IBATT]     = 610,
+	[LPADC_IC_TEMP]   = 610,
+	[LPADC_VBATT_6P0] = 1465,
+	[LPADC_VBATT_5P0] = 1221,
+	[LPADC_ADC1]      = 610,
+	[LPADC_ADC2]      = 610,
+	[LPADC_VDD]       = 1025,
+	[LPADC_VCOIN]     = 757,
+	[LPADC_VDD_LDO]   = 610,
+	[LPADC_ADC3]      = 610,
+	[LPADC_ADC4]      = 610,
+};
+
+static int lp8788_get_adc_result(struct lp8788_adc *adc, enum lp8788_adc_id id,
+				int *val)
+{
+	unsigned int msb;
+	unsigned int lsb;
+	unsigned int result;
+	u8 data;
+	u8 rawdata[2];
+	int size = ARRAY_SIZE(rawdata);
+	int retry = 5;
+	int ret;
+
+	data = (id << 1) | ADC_CONV_START;
+	ret = lp8788_write_byte(adc->lp, LP8788_ADC_CONF, data);
+	if (ret)
+		goto err_io;
+
+	/* retry until adc conversion is done */
+	data = 0;
+	while (retry--) {
+		usleep_range(100, 200);
+
+		ret = lp8788_read_byte(adc->lp, LP8788_ADC_DONE, &data);
+		if (ret)
+			goto err_io;
+
+		/* conversion done */
+		if (data)
+			break;
+	}
+
+	ret = lp8788_read_multi_bytes(adc->lp, LP8788_ADC_RAW, rawdata, size);
+	if (ret)
+		goto err_io;
+
+	msb = (rawdata[0] << 4) & 0x00000ff0;
+	lsb = (rawdata[1] >> 4) & 0x0000000f;
+	result = msb | lsb;
+	*val = result;
+
+	return 0;
+
+err_io:
+	return ret;
+}
+
+static int lp8788_adc_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan,
+			int *val, int *val2, long mask)
+{
+	struct lp8788_adc *adc = iio_priv(indio_dev);
+	enum lp8788_adc_id id = chan->channel;
+	int ret;
+
+	mutex_lock(&adc->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = lp8788_get_adc_result(adc, id, val) ? -EIO : IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		*val = (lp8788_scale[id] / 1000) * 1000;
+		*val2 = (lp8788_scale[id] % 1000) * 1000000;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&adc->lock);
+
+	return ret;
+}
+
+static const struct iio_info lp8788_adc_info = {
+	.read_raw = &lp8788_adc_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+#define LP8788_CHAN(_id, _type) {				\
+		.type = _type,					\
+		.indexed = 1,					\
+		.channel = LPADC_##_id,				\
+		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |	\
+			IIO_CHAN_INFO_SCALE_SEPARATE_BIT,	\
+		.address = LP8788_ADC_RAW,			\
+		.scan_type = IIO_ST('u', 12, 16, 0),		\
+		.scan_index = 1,				\
+		.datasheet_name = "LP8788_"#_id,			\
+}
+
+static const struct iio_chan_spec lp8788_adc_channels[] = {
+	[LPADC_VBATT_5P5] = LP8788_CHAN(VBATT_5P5, IIO_VOLTAGE),
+	[LPADC_VIN_CHG]   = LP8788_CHAN(VIN_CHG, IIO_VOLTAGE),
+	[LPADC_IBATT]     = LP8788_CHAN(IBATT, IIO_CURRENT),
+	[LPADC_IC_TEMP]   = LP8788_CHAN(IC_TEMP, IIO_TEMP),
+	[LPADC_VBATT_6P0] = LP8788_CHAN(VBATT_6P0, IIO_VOLTAGE),
+	[LPADC_VBATT_5P0] = LP8788_CHAN(VBATT_5P0, IIO_VOLTAGE),
+	[LPADC_ADC1]      = LP8788_CHAN(ADC1, IIO_VOLTAGE),
+	[LPADC_ADC2]      = LP8788_CHAN(ADC2, IIO_VOLTAGE),
+	[LPADC_VDD]       = LP8788_CHAN(VDD, IIO_VOLTAGE),
+	[LPADC_VCOIN]     = LP8788_CHAN(VCOIN, IIO_VOLTAGE),
+	[LPADC_VDD_LDO]   = LP8788_CHAN(VDD_LDO, IIO_VOLTAGE),
+	[LPADC_ADC3]      = LP8788_CHAN(ADC3, IIO_VOLTAGE),
+	[LPADC_ADC4]      = LP8788_CHAN(ADC4, IIO_VOLTAGE),
+};
+
+/* default maps used by iio consumer (lp8788-charger driver) */
+static struct iio_map lp8788_default_iio_maps[] = {
+	{
+		.consumer_dev_name = "lp8788-charger",
+		.consumer_channel = "lp8788_vbatt_5p0",
+		.adc_channel_label = "LP8788_VBATT_5P0",
+	},
+	{
+		.consumer_dev_name = "lp8788-charger",
+		.consumer_channel = "lp8788_adc1",
+		.adc_channel_label = "LP8788_ADC1",
+	},
+	{ }
+};
+
+static int lp8788_iio_map_register(struct iio_dev *indio_dev,
+				struct lp8788_platform_data *pdata,
+				struct lp8788_adc *adc)
+{
+	struct iio_map *map;
+	int ret;
+
+	map = !pdata || !pdata->adc_pdata ?
+		lp8788_default_iio_maps : pdata->adc_pdata;
+
+	ret = iio_map_array_register(indio_dev, map);
+	if (ret) {
+		dev_err(adc->lp->dev, "iio map err: %d\n", ret);
+		return ret;
+	}
+
+	adc->map = map;
+	return 0;
+}
+
+static void lp8788_iio_map_unregister(struct iio_dev *indio_dev,
+				struct lp8788_adc *adc)
+{
+	if (adc->map)
+		iio_map_array_unregister(indio_dev, adc->map);
+}
+
+static int __devinit lp8788_adc_probe(struct platform_device *pdev)
+{
+	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
+	struct iio_dev *indio_dev;
+	struct lp8788_adc *adc;
+	int ret;
+
+	indio_dev = iio_device_alloc(sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(indio_dev);
+	adc->lp = lp;
+	platform_set_drvdata(pdev, indio_dev);
+
+	ret = lp8788_iio_map_register(indio_dev, lp->pdata, adc);
+	if (ret)
+		goto err_iio_map;
+
+	mutex_init(&adc->lock);
+
+	indio_dev->dev.parent = lp->dev;
+	indio_dev->name = pdev->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &lp8788_adc_info;
+	indio_dev->channels = lp8788_adc_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lp8788_adc_channels);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(lp->dev, "iio dev register err: %d\n", ret);
+		goto err_iio_device;
+	}
+
+	return 0;
+
+err_iio_device:
+	lp8788_iio_map_unregister(indio_dev, adc);
+err_iio_map:
+	iio_device_free(indio_dev);
+	return ret;
+}
+
+static int __devexit lp8788_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev =  platform_get_drvdata(pdev);
+	struct lp8788_adc *adc = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	lp8788_iio_map_unregister(indio_dev, adc);
+	iio_device_free(indio_dev);
+
+	return 0;
+}
+
+static struct platform_driver lp8788_adc_driver = {
+	.probe = lp8788_adc_probe,
+	.remove = __devexit_p(lp8788_adc_remove),
+	.driver = {
+		.name = LP8788_DEV_ADC,
+		.owner = THIS_MODULE,
+	},
+};
+module_platform_driver(lp8788_adc_driver);
+
+MODULE_DESCRIPTION("Texas Instruments LP8788 ADC Driver");
+MODULE_AUTHOR("Milo Kim");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:lp8788-adc");
-- 
1.7.9.5


Best Regards,
Milo

Re: [PATCH v5] iio: adc: add new lp8788 adc driver

[Lars-Peter Clausen]

On 09/10/2012 10:02 AM, Kim, Milo wrote:
>  TI LP8788 PMU provides regulators, battery charger, ADC,
>  RTC, backlight driver and current sinks.
> 
>  This patch enables the LP8788 ADC functions.
> 
>  The LP8788 ADC has 13 ADC input selection and supports 12-bit resolution.
>  Internal operation of getting ADC is access to registers of LP8788.
>  The LP8788 ADC uses exported functions for accessing these registers.
>  (exported by LP8788 MFD device driver)
> 
>  This driver supports IIO_CHAN_INFO_RAW and SCALE.
>  The scale is micro unit.
>  So the IIO consumer can get the result as following.
> 
>    result = ADC raw value * (scale_int + scale_part / 1000000)
> 
>  (The IIO map for the IIO consumer)
> 
>  The ADC input can be selective in the platform side.
>  Even though this platform data is not defined,
>  the default IIO map is created for supporting the power supply driver.
>  The battery voltage and temperature are used inside this driver.
> 


Hi,

One issue and a couple of nitpicks inline.

> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
>  drivers/iio/adc/Kconfig      |    6 +
>  drivers/iio/adc/Makefile     |    1 +
>  drivers/iio/adc/lp8788_adc.c |  270 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 277 insertions(+)
[...]
> diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
> new file mode 100644
> index 0000000..6079f9e
> --- /dev/null
> +++ b/drivers/iio/adc/lp8788_adc.c
> @@ -0,0 +1,270 @@
[...]
> +
> +static const int lp8788_scale[LPADC_MAX] = {
> +	[LPADC_VBATT_5P5] = 1343,
> +	[LPADC_VIN_CHG]   = 3052,
> +	[LPADC_IBATT]     = 610,
> +	[LPADC_IC_TEMP]   = 610,
> +	[LPADC_VBATT_6P0] = 1465,
> +	[LPADC_VBATT_5P0] = 1221,
> +	[LPADC_ADC1]      = 610,
> +	[LPADC_ADC2]      = 610,
> +	[LPADC_VDD]       = 1025,
> +	[LPADC_VCOIN]     = 757,
> +	[LPADC_VDD_LDO]   = 610,
> +	[LPADC_ADC3]      = 610,
> +	[LPADC_ADC4]      = 610,
> +};
> +
[...]
> +static int lp8788_adc_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val, int *val2, long mask)
> +{
> +	struct lp8788_adc *adc = iio_priv(indio_dev);
> +	enum lp8788_adc_id id = chan->channel;
> +	int ret;
> +
> +	mutex_lock(&adc->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = lp8788_get_adc_result(adc, id, val) ? -EIO : IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = (lp8788_scale[id] / 1000) * 1000;
> +		*val2 = (lp8788_scale[id] % 1000) * 1000000;

This looks suspicious. E.g. if the entry in your table has the value "1234"
you'd end up with a scale factor of 1000.234000000. Which looks wrong for
two reasons, given that you return INT_PLUS_MICRO val2 should never exceed 6
decimal digits and secondly val is multiplied by a factor of thousand.

Can you tell us in what unit the values in lp8788_scale are, that would make
review easier.

> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info lp8788_adc_info = {
> +	.read_raw = &lp8788_adc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +#define LP8788_CHAN(_id, _type) {				\
> +		.type = _type,					\
> +		.indexed = 1,					\
> +		.channel = LPADC_##_id,				\
> +		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |	\
> +			IIO_CHAN_INFO_SCALE_SEPARATE_BIT,	\
> +		.address = LP8788_ADC_RAW,			\
> +		.scan_type = IIO_ST('u', 12, 16, 0),		\
> +		.scan_index = 1,				\
> +		.datasheet_name = "LP8788_"#_id,			\

I'd drop the "LP8788_" prefix.

> +}
> +
[...]
> +static int lp8788_iio_map_register(struct iio_dev *indio_dev,
> +				struct lp8788_platform_data *pdata,
> +				struct lp8788_adc *adc)
> +{
> +	struct iio_map *map;
> +	int ret;
> +
> +	map = !pdata || !pdata->adc_pdata ?

This could use a pair of parenthesis.

> +		lp8788_default_iio_maps : pdata->adc_pdata;
> +
> +	ret = iio_map_array_register(indio_dev, map);
> +	if (ret) {
> +		dev_err(adc->lp->dev, "iio map err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	adc->map = map;
> +	return 0;
> +}
> +

[...]
> +static int __devexit lp8788_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev =  platform_get_drvdata(pdev);

extra space.

> +	struct lp8788_adc *adc = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	lp8788_iio_map_unregister(indio_dev, adc);
> +	iio_device_free(indio_dev);
> +
> +	return 0;
> +}

Re: [PATCH v5] iio: adc: add new lp8788 adc driver

[Jonathan Cameron]

Couple of nitpicks in line to add to Lars-Peter's ones.

>   TI LP8788 PMU provides regulators, battery charger, ADC,
>   RTC, backlight driver and current sinks.
>
>   This patch enables the LP8788 ADC functions.
>
>   The LP8788 ADC has 13 ADC input selection and supports 12-bit resolution.
>   Internal operation of getting ADC is access to registers of LP8788.
>   The LP8788 ADC uses exported functions for accessing these registers.
>   (exported by LP8788 MFD device driver)
>
>   This driver supports IIO_CHAN_INFO_RAW and SCALE.
>   The scale is micro unit.
>   So the IIO consumer can get the result as following.
>
>     result = ADC raw value * (scale_int + scale_part / 1000000)
>
>   (The IIO map for the IIO consumer)
>
>   The ADC input can be selective in the platform side.
>   Even though this platform data is not defined,
>   the default IIO map is created for supporting the power supply driver.
>   The battery voltage and temperature are used inside this driver.
>
>   (History)
>
>   Patch v5.
>   (a) Fix misuse of consumer_name_dev
>     If the IIO consumer device is not defined in the platform side,
>     the default consumer is the lp8788-charger.
>     In this case, the consumer name should be defined as 'lp8788-charger'
>     not driver name itself.
>
>   (b) Add mutex for ADC read operation
>
>   (c) Reorganization on lp8788_adc_read_raw()
>     The LP8788 registers are accessed only when the mask is IIO_CHAN_INFO_RAW.
>     Other cases, no need to read the registers.
>     To make lp8788_adc_read_raw() simple, new lp8788_get_adc_result() is added.
>
>   (d) make LP8788 specific names
>     Add prefix LP8788_ for datasheet_name, channel and ADC channel label of
>     the default IIO maps.
>
>   Patch v4.
>   Fix adc_raw function: support RAW and SCALE channel info.
>   Change LP8788 ADC platform data - the IIO map.
>   Enables the default IIO map.
>
>   Patch v3.
>   Fix wrong size of allocating the IIO private data.
>   Fix coding styles.
>
>   Patch v2.
>   Support RAW and SCALE interface for IIO consumer.
>   Clean up the IIO channel spec macro.
>
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
>   drivers/iio/adc/Kconfig      |    6 +
>   drivers/iio/adc/Makefile     |    1 +
>   drivers/iio/adc/lp8788_adc.c |  270 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 277 insertions(+)
>   create mode 100644 drivers/iio/adc/lp8788_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8a78b4f..30c06ed 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -22,4 +22,10 @@ config AT91_ADC
>   	help
>   	  Say yes here to build support for Atmel AT91 ADC.
>
> +config LP8788_ADC
> +	bool "LP8788 ADC driver"
> +	depends on MFD_LP8788
> +	help
> +	  Say yes here to build support for TI LP8788 ADC.
> +
>   endmenu
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 52eec25..72f9a76 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -4,3 +4,4 @@
>
>   obj-$(CONFIG_AD7266) += ad7266.o
>   obj-$(CONFIG_AT91_ADC) += at91_adc.o
> +obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
> new file mode 100644
> index 0000000..6079f9e
> --- /dev/null
> +++ b/drivers/iio/adc/lp8788_adc.c
> @@ -0,0 +1,270 @@
> +/*
> + * TI LP8788 MFD - ADC driver
> + *
> + * Copyright 2012 Texas Instruments
> + *
> + * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/mfd/lp8788.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* register address */
> +#define LP8788_ADC_CONF			0x60
> +#define LP8788_ADC_RAW			0x61
> +#define LP8788_ADC_DONE			0x63
> +
> +#define ADC_CONV_START			1
> +
> +struct lp8788_adc {
> +	struct lp8788 *lp;
> +	struct iio_map *map;
> +	struct mutex lock;
> +};
> +
> +static const int lp8788_scale[LPADC_MAX] = {
> +	[LPADC_VBATT_5P5] = 1343,
> +	[LPADC_VIN_CHG]   = 3052,
> +	[LPADC_IBATT]     = 610,
> +	[LPADC_IC_TEMP]   = 610,
> +	[LPADC_VBATT_6P0] = 1465,
> +	[LPADC_VBATT_5P0] = 1221,
> +	[LPADC_ADC1]      = 610,
> +	[LPADC_ADC2]      = 610,
> +	[LPADC_VDD]       = 1025,
> +	[LPADC_VCOIN]     = 757,
> +	[LPADC_VDD_LDO]   = 610,
> +	[LPADC_ADC3]      = 610,
> +	[LPADC_ADC4]      = 610,
> +};
> +
> +static int lp8788_get_adc_result(struct lp8788_adc *adc, enum lp8788_adc_id id,
> +				int *val)
> +{
> +	unsigned int msb;
> +	unsigned int lsb;
> +	unsigned int result;
> +	u8 data;
> +	u8 rawdata[2];
> +	int size = ARRAY_SIZE(rawdata);
> +	int retry = 5;
> +	int ret;
> +
> +	data = (id << 1) | ADC_CONV_START;
> +	ret = lp8788_write_byte(adc->lp, LP8788_ADC_CONF, data);
> +	if (ret)
> +		goto err_io;
> +
> +	/* retry until adc conversion is done */
> +	data = 0;
> +	while (retry--) {
> +		usleep_range(100, 200);
> +
> +		ret = lp8788_read_byte(adc->lp, LP8788_ADC_DONE, &data);
> +		if (ret)
> +			goto err_io;
> +
> +		/* conversion done */
> +		if (data)
> +			break;
> +	}
> +
> +	ret = lp8788_read_multi_bytes(adc->lp, LP8788_ADC_RAW, rawdata, size);
> +	if (ret)
> +		goto err_io;
> +
> +	msb = (rawdata[0] << 4) & 0x00000ff0;
> +	lsb = (rawdata[1] >> 4) & 0x0000000f;
> +	result = msb | lsb;
> +	*val = result;
> +
> +	return 0;
> +
> +err_io:
> +	return ret;
> +}
> +
> +static int lp8788_adc_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val, int *val2, long mask)
> +{
> +	struct lp8788_adc *adc = iio_priv(indio_dev);
> +	enum lp8788_adc_id id = chan->channel;
> +	int ret;
> +
> +	mutex_lock(&adc->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = lp8788_get_adc_result(adc, id, val) ? -EIO : IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = (lp8788_scale[id] / 1000) * 1000;
> +		*val2 = (lp8788_scale[id] % 1000) * 1000000;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info lp8788_adc_info = {
> +	.read_raw = &lp8788_adc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +#define LP8788_CHAN(_id, _type) {				\
> +		.type = _type,					\
> +		.indexed = 1,					\
> +		.channel = LPADC_##_id,				\
> +		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |	\
> +			IIO_CHAN_INFO_SCALE_SEPARATE_BIT,	\
> +		.address = LP8788_ADC_RAW,			\
> +		.scan_type = IIO_ST('u', 12, 16, 0),		\
> +		.scan_index = 1,				\
scan_type and scan_index should only be relevant if you are doing 
buffered capture (unless I'm missing another use of them in here 
somewhere...)  Hence drop them for now...

> +		.datasheet_name = "LP8788_"#_id,			\
> +}
> +
> +static const struct iio_chan_spec lp8788_adc_channels[] = {
> +	[LPADC_VBATT_5P5] = LP8788_CHAN(VBATT_5P5, IIO_VOLTAGE),
> +	[LPADC_VIN_CHG]   = LP8788_CHAN(VIN_CHG, IIO_VOLTAGE),
> +	[LPADC_IBATT]     = LP8788_CHAN(IBATT, IIO_CURRENT),
> +	[LPADC_IC_TEMP]   = LP8788_CHAN(IC_TEMP, IIO_TEMP),
> +	[LPADC_VBATT_6P0] = LP8788_CHAN(VBATT_6P0, IIO_VOLTAGE),
> +	[LPADC_VBATT_5P0] = LP8788_CHAN(VBATT_5P0, IIO_VOLTAGE),
> +	[LPADC_ADC1]      = LP8788_CHAN(ADC1, IIO_VOLTAGE),
> +	[LPADC_ADC2]      = LP8788_CHAN(ADC2, IIO_VOLTAGE),
> +	[LPADC_VDD]       = LP8788_CHAN(VDD, IIO_VOLTAGE),
> +	[LPADC_VCOIN]     = LP8788_CHAN(VCOIN, IIO_VOLTAGE),
> +	[LPADC_VDD_LDO]   = LP8788_CHAN(VDD_LDO, IIO_VOLTAGE),
> +	[LPADC_ADC3]      = LP8788_CHAN(ADC3, IIO_VOLTAGE),
> +	[LPADC_ADC4]      = LP8788_CHAN(ADC4, IIO_VOLTAGE),
> +};
> +
> +/* default maps used by iio consumer (lp8788-charger driver) */
> +static struct iio_map lp8788_default_iio_maps[] = {
> +	{
> +		.consumer_dev_name = "lp8788-charger",
> +		.consumer_channel = "lp8788_vbatt_5p0",
> +		.adc_channel_label = "LP8788_VBATT_5P0",
> +	},
> +	{
> +		.consumer_dev_name = "lp8788-charger",
> +		.consumer_channel = "lp8788_adc1",
> +		.adc_channel_label = "LP8788_ADC1",
> +	},
> +	{ }
> +};
> +
> +static int lp8788_iio_map_register(struct iio_dev *indio_dev,
> +				struct lp8788_platform_data *pdata,
> +				struct lp8788_adc *adc)
> +{
> +	struct iio_map *map;
> +	int ret;
> +
> +	map = !pdata || !pdata->adc_pdata ?
> +		lp8788_default_iio_maps : pdata->adc_pdata;
> +
> +	ret = iio_map_array_register(indio_dev, map);
> +	if (ret) {
> +		dev_err(adc->lp->dev, "iio map err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	adc->map = map;
> +	return 0;
> +}
> +
> +static void lp8788_iio_map_unregister(struct iio_dev *indio_dev,
> +				struct lp8788_adc *adc)
> +{
Can't immediately see how this runs without adc->map set. If it
doesn't drop the check.
> +	if (adc->map)
> +		iio_map_array_unregister(indio_dev, adc->map);
> +}
> +
> +static int __devinit lp8788_adc_probe(struct platform_device *pdev)
> +{
> +	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
> +	struct iio_dev *indio_dev;
> +	struct lp8788_adc *adc;
> +	int ret;
> +
> +	indio_dev = iio_device_alloc(sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(indio_dev);
> +	adc->lp = lp;
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	ret = lp8788_iio_map_register(indio_dev, lp->pdata, adc);
> +	if (ret)
> +		goto err_iio_map;
> +
> +	mutex_init(&adc->lock);
> +
> +	indio_dev->dev.parent = lp->dev;
> +	indio_dev->name = pdev->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &lp8788_adc_info;
> +	indio_dev->channels = lp8788_adc_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(lp8788_adc_channels);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(lp->dev, "iio dev register err: %d\n", ret);
> +		goto err_iio_device;
> +	}
> +
> +	return 0;
> +
> +err_iio_device:
> +	lp8788_iio_map_unregister(indio_dev, adc);
> +err_iio_map:
> +	iio_device_free(indio_dev);
> +	return ret;
> +}
> +
> +static int __devexit lp8788_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev =  platform_get_drvdata(pdev);
> +	struct lp8788_adc *adc = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	lp8788_iio_map_unregister(indio_dev, adc);
> +	iio_device_free(indio_dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver lp8788_adc_driver = {
> +	.probe = lp8788_adc_probe,
> +	.remove = __devexit_p(lp8788_adc_remove),
> +	.driver = {
> +		.name = LP8788_DEV_ADC,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +module_platform_driver(lp8788_adc_driver);
> +
> +MODULE_DESCRIPTION("Texas Instruments LP8788 ADC Driver");
> +MODULE_AUTHOR("Milo Kim");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:lp8788-adc");
>

RE: [PATCH v5] iio: adc: add new lp8788 adc driver

[Kim, Milo]

> Hi,
> 
> One issue and a couple of nitpicks inline.

I really appreciate it.
Please see my questions below.

> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = lp8788_get_adc_result(adc, id, val) ? -EIO :
> IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = (lp8788_scale[id] / 1000) * 1000;
> > +		*val2 = (lp8788_scale[id] % 1000) * 1000000;
> 
> This looks suspicious. E.g. if the entry in your table has the value
> "1234"
> you'd end up with a scale factor of 1000.234000000. Which looks wrong
> for
> two reasons, given that you return INT_PLUS_MICRO val2 should never
> exceed 6
> decimal digits and secondly val is multiplied by a factor of thousand.
> 
> Can you tell us in what unit the values in lp8788_scale are, that would
> make
> review easier.

The LP8788 has 13 ADC input selection.

ADC selection:
Battery voltage, general ADC1 and so on.

ADC result:
Result = MAX_VALUE * (raw + 0.5) / 4095  except ADC is the charger voltage
If the ADC input is the charger voltage,
Result = MAX_VALUE * (raw + 0.5) / (4095 * 0.48)

The raw value is from the registers.
It has the range between 0 to 4095. (12bits)

MAX_VALUE is constant for each selection.
For the battery voltage, there are three ADC inputs. 5.0/5.5/6.0V
Battery voltage for Max 5.0V = 5.0
Battery voltage for Max 5.5V = 5.5
Battery voltage for Max 6.0V = 6.0
Charger = 6.0
ADC1 = 2.5 

I'm afraid I still misunderstand how IIO ADC works.
Could you me some guide how to setup the scale in the driver?

Thank you.

Best Regards,
Milo

RE: [PATCH v5] iio: adc: add new lp8788 adc driver

[Kim, Milo]

> 
> Couple of nitpicks in line to add to Lars-Peter's ones.

I really appreciate it.

> > +#define LP8788_CHAN(_id, _type) {				\
> > +		.type = _type,					\
> > +		.indexed = 1,					\
> > +		.channel = LPADC_##_id,				\
> > +		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |	\
> > +			IIO_CHAN_INFO_SCALE_SEPARATE_BIT,	\
> > +		.address = LP8788_ADC_RAW,			\
> > +		.scan_type = IIO_ST('u', 12, 16, 0),		\
> > +		.scan_index = 1,				\
> scan_type and scan_index should only be relevant if you are doing
> buffered capture (unless I'm missing another use of them in here
> somewhere...)  Hence drop them for now...

Thanks for clarifying this.

> > +static void lp8788_iio_map_unregister(struct iio_dev *indio_dev,
> > +				struct lp8788_adc *adc)
> > +{
> Can't immediately see how this runs without adc->map set. If it
> doesn't drop the check.
> > +	if (adc->map)
> > +		iio_map_array_unregister(indio_dev, adc->map);
> > +}

You're right. No need to check whether iio_map is valid or not.
That is already implemented in the iio_map_array_unregister().
It will be removed in the next patch.

Thank you.

Best Regards,
Milo

Re: [PATCH v5] iio: adc: add new lp8788 adc driver

[Lars-Peter Clausen]

On 09/14/2012 02:33 AM, Kim, Milo wrote:
>> Hi,
>>
>> One issue and a couple of nitpicks inline.
> 
> I really appreciate it.
> Please see my questions below.
> 
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_RAW:
>>> +		ret = lp8788_get_adc_result(adc, id, val) ? -EIO :
>> IIO_VAL_INT;
>>> +		break;
>>> +	case IIO_CHAN_INFO_SCALE:
>>> +		*val = (lp8788_scale[id] / 1000) * 1000;
>>> +		*val2 = (lp8788_scale[id] % 1000) * 1000000;
>>
>> This looks suspicious. E.g. if the entry in your table has the value
>> "1234"
>> you'd end up with a scale factor of 1000.234000000. Which looks wrong
>> for
>> two reasons, given that you return INT_PLUS_MICRO val2 should never
>> exceed 6
>> decimal digits and secondly val is multiplied by a factor of thousand.
>>
>> Can you tell us in what unit the values in lp8788_scale are, that would
>> make
>> review easier.
> 
> The LP8788 has 13 ADC input selection.
> 
> ADC selection:
> Battery voltage, general ADC1 and so on.
> 
> ADC result:
> Result = MAX_VALUE * (raw + 0.5) / 4095  except ADC is the charger voltage
> If the ADC input is the charger voltage,
> Result = MAX_VALUE * (raw + 0.5) / (4095 * 0.48)
> 
> The raw value is from the registers.
> It has the range between 0 to 4095. (12bits)
> 
> MAX_VALUE is constant for each selection.
> For the battery voltage, there are three ADC inputs. 5.0/5.5/6.0V
> Battery voltage for Max 5.0V = 5.0
> Battery voltage for Max 5.5V = 5.5
> Battery voltage for Max 6.0V = 6.0
> Charger = 6.0
> ADC1 = 2.5 
> 
> I'm afraid I still misunderstand how IIO ADC works.
> Could you me some guide how to setup the scale in the driver?

The scale is a fixpoint value, which should be multiplied with the raw value
to get the result in the proper unit. The unit depends on the channel type,
e.g. for voltage it is mV and for temperature it is C. The number of decimal
places for the fixed point value depends on whether you return
INT_PLUS_MICRO it's 6, if you return INT_PLUS_NANO it is 9. The digits
before the decimal point are stored in "val" the digits after the decimal
point are stored in "val2".

E.g. if you have
*val = 1;
*val2 = 1256;
return INT_PLUS_MICRO;

your scale factor is 1.001256, if you'd return INT_PLUS_NANO you scale
factor would be 1.0000001256 instead.

In your case you could for example calculate the voltage scales as:

tmp = MAX_VALUE * 1000000 / 4095;
*val = tmp / 1000000;
*val2 = tmp % 1000000;

This assumes that MAX_VALUE is in millivolt.

E.g. if MAX_VALUE is 5.0V you should get a scale of 1.220703 (val = 1, val2
= 220703). Since your MAX_VALUE is fixed you can probably just pre-calculate
the result of MAX_VALUE * 1000000 / 4095 for each channel, similar like you
already did with your lp8788_scale table.

- Lars

RE: [PATCH v5] iio: adc: add new lp8788 adc driver

[Kim, Milo]

> > The LP8788 has 13 ADC input selection.
> >
> > ADC selection:
> > Battery voltage, general ADC1 and so on.
> >
> > ADC result:
> > Result = MAX_VALUE * (raw + 0.5) / 4095  except ADC is the charger
> voltage
> > If the ADC input is the charger voltage,
> > Result = MAX_VALUE * (raw + 0.5) / (4095 * 0.48)
> >
> > The raw value is from the registers.
> > It has the range between 0 to 4095. (12bits)
> >
> > MAX_VALUE is constant for each selection.
> > For the battery voltage, there are three ADC inputs. 5.0/5.5/6.0V
> > Battery voltage for Max 5.0V = 5.0
> > Battery voltage for Max 5.5V = 5.5
> > Battery voltage for Max 6.0V = 6.0
> > Charger = 6.0
> > ADC1 = 2.5
> >
> > I'm afraid I still misunderstand how IIO ADC works.
> > Could you me some guide how to setup the scale in the driver?
> 
> The scale is a fixpoint value, which should be multiplied with the raw
> value
> to get the result in the proper unit. The unit depends on the channel
> type,
> e.g. for voltage it is mV and for temperature it is C. The number of
> decimal
> places for the fixed point value depends on whether you return
> INT_PLUS_MICRO it's 6, if you return INT_PLUS_NANO it is 9. The digits
> before the decimal point are stored in "val" the digits after the
> decimal
> point are stored in "val2".
> 
> E.g. if you have
> *val = 1;
> *val2 = 1256;
> return INT_PLUS_MICRO;
> 
> your scale factor is 1.001256, if you'd return INT_PLUS_NANO you scale
> factor would be 1.0000001256 instead.
> 
> In your case you could for example calculate the voltage scales as:
> 
> tmp = MAX_VALUE * 1000000 / 4095;
> *val = tmp / 1000000;
> *val2 = tmp % 1000000;
> 
> This assumes that MAX_VALUE is in millivolt.
> 
> E.g. if MAX_VALUE is 5.0V you should get a scale of 1.220703 (val = 1,
> val2
> = 220703). Since your MAX_VALUE is fixed you can probably just pre-
> calculate
> the result of MAX_VALUE * 1000000 / 4095 for each channel, similar like
> you
> already did with your lp8788_scale table.

Superb! Thanks a lot for your clear explanation.

I have one more question about the temperature ADC.

The LP8788 has no dedicated temperature ADC, but it provides four
general inputs - ADC1 to 4.
The IIO consumer can get the battery temperature among general ADC inputs.
I think the ADC driver should not assume that ADC1..4 is for the temperature,
because the selection is configurable in the platform side.

Then how can I set the unit of ADC MAX_VALUE?
I would set the unit of ADC MAX_VALUE to mili (same as voltage) and
the result is converted by the IIO consumer manually.
Does it make sense ?

Best Regards,
Milo

Re: [PATCH v5] iio: adc: add new lp8788 adc driver

[Jonathan Cameron]

On 09/15/2012 01:15 AM, Kim, Milo wrote:
>>> The LP8788 has 13 ADC input selection.
>>>
>>> ADC selection:
>>> Battery voltage, general ADC1 and so on.
>>>
>>> ADC result:
>>> Result = MAX_VALUE * (raw + 0.5) / 4095  except ADC is the charger
>> voltage
>>> If the ADC input is the charger voltage,
>>> Result = MAX_VALUE * (raw + 0.5) / (4095 * 0.48)
>>>
>>> The raw value is from the registers.
>>> It has the range between 0 to 4095. (12bits)
>>>
>>> MAX_VALUE is constant for each selection.
>>> For the battery voltage, there are three ADC inputs. 5.0/5.5/6.0V
>>> Battery voltage for Max 5.0V = 5.0
>>> Battery voltage for Max 5.5V = 5.5
>>> Battery voltage for Max 6.0V = 6.0
>>> Charger = 6.0
>>> ADC1 = 2.5
>>>
>>> I'm afraid I still misunderstand how IIO ADC works.
>>> Could you me some guide how to setup the scale in the driver?
>>
>> The scale is a fixpoint value, which should be multiplied with the raw
>> value
>> to get the result in the proper unit. The unit depends on the channel
>> type,
>> e.g. for voltage it is mV and for temperature it is C. The number of
>> decimal
>> places for the fixed point value depends on whether you return
>> INT_PLUS_MICRO it's 6, if you return INT_PLUS_NANO it is 9. The digits
>> before the decimal point are stored in "val" the digits after the
>> decimal
>> point are stored in "val2".
>>
>> E.g. if you have
>> *val = 1;
>> *val2 = 1256;
>> return INT_PLUS_MICRO;
>>
>> your scale factor is 1.001256, if you'd return INT_PLUS_NANO you scale
>> factor would be 1.0000001256 instead.
>>
>> In your case you could for example calculate the voltage scales as:
>>
>> tmp = MAX_VALUE * 1000000 / 4095;
>> *val = tmp / 1000000;
>> *val2 = tmp % 1000000;
>>
>> This assumes that MAX_VALUE is in millivolt.
>>
>> E.g. if MAX_VALUE is 5.0V you should get a scale of 1.220703 (val = 1,
>> val2
>> = 220703). Since your MAX_VALUE is fixed you can probably just pre-
>> calculate
>> the result of MAX_VALUE * 1000000 / 4095 for each channel, similar like
>> you
>> already did with your lp8788_scale table.
> 
> Superb! Thanks a lot for your clear explanation.
> 
> I have one more question about the temperature ADC.
> 
> The LP8788 has no dedicated temperature ADC, but it provides four
> general inputs - ADC1 to 4.
> The IIO consumer can get the battery temperature among general ADC inputs.
> I think the ADC driver should not assume that ADC1..4 is for the temperature,
> because the selection is configurable in the platform side.
> 
> Then how can I set the unit of ADC MAX_VALUE?
> I would set the unit of ADC MAX_VALUE to mili (same as voltage) and
> the result is converted by the IIO consumer manually.
> Does it make sense ?
Yes, if there is a temperature sensor attached to an ADC channel it
is not up to the ADC driver to know this, but rather to the consumer driver.
> 
> Best Regards,
> Milo
>