2013-07-19 09:28:17

by Oleksandr Kozaruk

[permalink] [raw]
Subject: [PATCH v6 0/2] TWL6030, TWL6032 GPADC driver

Hello,

v6 - addressed comments about trim bits, checkpatch clean up
v5 - gpadc DT node renamed from "gpadc" to generic "adc", added
temperature channels; raw code is corracted with calibration
data.
v4 - addressed comments: fixed style violation, bug in freeing memory,
added comments explaining calibration method, removed test network
channels from exposing to userspace, error handling for
wait_for_complition
v3 - fixed compiler warning
v2 - the driver put in drivers/iio, and
converted using iio facilities as suggested by Graeme.

TWL603[02] GPADC is used to measure battery voltage,
battery temperature, battery presence ID, and could
be used to measure twl603[02] die temperature.
This is used on TI blaze, blaze tablet platforms.

The TWL6030/TWL6032 is a PMIC that has a GPADC with 17/19
channels respectively. Some channels have current
source and are used for measuring voltage drop
on resistive load for detecting battery ID resistance,
or measuring voltage drop on NTC resistors for external
temperature measurements, other channels measure voltage,
(i.e. battery voltage), and have inbuilt voltage dividers,
thus, capable to scale voltage. Some channels are dedicated
for measuring die temperature.

Some channels could be calibrated in 2 points, having
offsets from ideal values in trim registers.

The difference between GPADC in TWL6030 and TWL6032:
- 10 bit vs 12 bit ADC;
- 17 vs 19 channels;
- channels have different purpose(i. e. battery voltage
channel 8 vs channel 18);
- trim values are interpreted differently.

The driver is derived from git://git.omapzoom.org/kernel/omap.git
The original driver's authors and contributors are Balaji T K,
Graeme Gregory, Ambresh K, Girish S Ghongdemath.

The changes to the original driver:
- device tree adaptation;
- drop ioctl support - never been used;
- unified measurement method for both devices;
- get rid of "if (device == X)" code style to data driven;
- drop polling end of conversion and use interrupt instead;
- iio framework is used

Tested with on blaze tablet 2 with OMAP4430(twl6030), and
OMAP4470(twl6032) SOMs.

The patches were tested against 3.10-rc7

Oleksandr Kozaruk (2):
ARM: dts: twl: Add GPADC data to device tree
iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

arch/arm/boot/dts/twl6030.dtsi | 6 +
drivers/iio/adc/Kconfig | 14 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/twl6030-gpadc.c | 991 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 1012 insertions(+)
create mode 100644 drivers/iio/adc/twl6030-gpadc.c

--
1.8.1.2


2013-07-19 09:28:22

by Oleksandr Kozaruk

[permalink] [raw]
Subject: [PATCH v6 1/2] ARM: dts: twl: Add GPADC data to device tree

GPADC is the general purpose ADC present on twl6030.
The dt data is interrupt used to trigger end of ADC
conversion.

Signed-off-by: Oleksandr Kozaruk <[email protected]>
---
arch/arm/boot/dts/twl6030.dtsi | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/twl6030.dtsi b/arch/arm/boot/dts/twl6030.dtsi
index 2e3bd31..d7d4c28 100644
--- a/arch/arm/boot/dts/twl6030.dtsi
+++ b/arch/arm/boot/dts/twl6030.dtsi
@@ -103,4 +103,10 @@
compatible = "ti,twl6030-pwmled";
#pwm-cells = <2>;
};
+
+ adc: gpadc {
+ compatible = "ti,twl6030-gpadc";
+ interrupts = <3>;
+ #io-channel-cells = <1>;
+ };
};
--
1.8.1.2

2013-07-19 09:28:31

by Oleksandr Kozaruk

[permalink] [raw]
Subject: [PATCH v6 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

The GPADC is general purpose ADC found on TWL6030, and TWL6032 PMIC,
known also as Phoenix and PhoenixLite.

The TWL6030 and TWL6032 have GPADC with 17 and 19 channels
respectively. Some channels have current source and are used for
measuring voltage drop on resistive load for detecting battery ID
resistance, or measuring voltage drop on NTC resistors for external
temperature measurements. Some channels measure voltage, (i.e. battery
voltage), and have voltage dividers, thus, capable to scale voltage.
Some channels are dedicated for measuring die temperature.

Some channels are calibrated in 2 points, having offsets from ideal
values kept in trim registers. This is used to correct measurements.

The differences between GPADC in TWL6030 and TWL6032:
- 10 bit vs 12 bit ADC;
- 17 vs 19 channels;
- channels have different purpose(i.e. battery voltage
channel 8 vs channel 18);
- trim values are interpreted differently.

Based on the driver patched from Balaji TK, Graeme Gregory, Ambresh K,
Girish S Ghongdemath.

Signed-off-by: Balaji T K <[email protected]>
Signed-off-by: Graeme Gregory <[email protected]>
Signed-off-by: Oleksandr Kozaruk <[email protected]>
---
drivers/iio/adc/Kconfig | 14 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/twl6030-gpadc.c | 991 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 1006 insertions(+)
create mode 100644 drivers/iio/adc/twl6030-gpadc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..3172461 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -150,6 +150,20 @@ config TI_AM335X_ADC
Say yes here to build support for Texas Instruments ADC
driver which is also a MFD client.

+config TWL6030_GPADC
+ tristate "TWL6030 GPADC (General Purpose A/D Converter) Support"
+ depends on TWL4030_CORE
+ default n
+ help
+ Say yes here if you want support for the TWL6030/TWL6032 General
+ Purpose A/D Converter. This will add support for battery type
+ detection, battery voltage and temperature measurement, die
+ temperature measurement, system supply voltage, audio accessory,
+ USB ID detection.
+
+ This driver can also be built as a module. If so, the module will be
+ called twl6030-gpadc.
+
config VIPERBOARD_ADC
tristate "Viperboard ADC support"
depends on MFD_VIPERBOARD && USB
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..996ba09 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -16,4 +16,5 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
obj-$(CONFIG_MAX1363) += max1363.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
+obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c
new file mode 100644
index 0000000..b42cfd6
--- /dev/null
+++ b/drivers/iio/adc/twl6030-gpadc.c
@@ -0,0 +1,991 @@
+/*
+ * TWL6030 GPADC module driver
+ *
+ * Copyright (C) 2009-2013 Texas Instruments Inc.
+ * Nishant Kamat <[email protected]>
+ * Balaji T K <[email protected]>
+ * Graeme Gregory <[email protected]>
+ * Girish S Ghongdemath <[email protected]>
+ * Ambresh K <[email protected]>
+ * Oleksandr Kozaruk <[email protected]
+ *
+ * Based on twl4030-madc.c
+ * Copyright (C) 2008 Nokia Corporation
+ * Mikko Ylinen <[email protected]>
+ *
+ * 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.
+ *
+ * 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., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/i2c/twl.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define DRIVER_NAME "twl6030_gpadc"
+
+#define TWL6030_GPADC_MAX_CHANNELS 17
+#define TWL6032_GPADC_MAX_CHANNELS 19
+
+#define TWL6030_GPADC_CTRL_P1 0x05
+
+#define TWL6032_GPADC_GPSELECT_ISB 0x07
+#define TWL6032_GPADC_CTRL_P1 0x08
+
+#define TWL6032_GPADC_GPCH0_LSB 0x0d
+#define TWL6032_GPADC_GPCH0_MSB 0x0e
+
+#define TWL6030_GPADC_CTRL_P1_SP1 BIT(3)
+
+#define TWL6030_GPADC_GPCH0_LSB (0x29)
+
+#define TWL6030_GPADC_RT_SW1_EOC_MASK BIT(5)
+
+#define TWL6030_GPADC_TRIM1 0xCD
+
+#define TWL6030_REG_TOGGLE1 0x90
+#define TWL6030_GPADCS BIT(1)
+#define TWL6030_GPADCR BIT(0)
+
+/**
+ * struct twl6030_chnl_calib - channel calibration
+ * @gain: slope coefficient for ideal curve
+ * @gain_error: gain error
+ * @offset_error: offset of the real curve
+ */
+struct twl6030_chnl_calib {
+ s32 gain;
+ s32 gain_error;
+ s32 offset_error;
+};
+
+/**
+ * struct twl6030_ideal_code - GPADC calibration parameters
+ * GPADC is calibrated in two points: close to the beginning and
+ * to the and of the measurable input range
+ *
+ * @code1: ideal code for the input at the beginning
+ * @code2: ideal code for at the end of the range
+ * @volt1: voltage input at the beginning(low voltage)
+ * @volt2: voltage input at the end(high voltage)
+ */
+struct twl6030_ideal_code {
+ u16 code1;
+ u16 code2;
+ u16 volt1;
+ u16 volt2;
+};
+
+struct twl6030_gpadc_data;
+
+/**
+ * struct twl6030_gpadc_platform_data - platform specific data
+ * @nchannels: number of GPADC channels
+ * @iio_channels: iio channels
+ * @twl6030_ideal: pointer to calibration parameters
+ * @start_conversion: pointer to ADC start conversion function
+ * @channel_to_reg pointer to ADC function to convert channel to
+ * register address for reading conversion result
+ * @calibrate: pointer to calibration function
+ */
+struct twl6030_gpadc_platform_data {
+ const int nchannels;
+ const struct iio_chan_spec *iio_channels;
+ const struct twl6030_ideal_code *ideal;
+ int (*start_conversion)(int channel);
+ u8 (*channel_to_reg)(int channel);
+ int (*calibrate)(struct twl6030_gpadc_data *gpadc);
+};
+
+/**
+ * struct twl6030_gpadc_data - GPADC data
+ * @dev: device pointer
+ * @lock: mutual exclusion lock for the structure
+ * @irq_complete: completion to signal end of conversion
+ * @twl6030_cal_tbl: pointer to calibration data for each
+ * channel with gain error and offset
+ * @pdata: pointer to device specific data
+ */
+struct twl6030_gpadc_data {
+ struct device *dev;
+ struct mutex lock;
+ struct completion irq_complete;
+ struct twl6030_chnl_calib *twl6030_cal_tbl;
+ const struct twl6030_gpadc_platform_data *pdata;
+};
+
+/*
+ * channels 11, 12, 13, 15 and 16 have no calibration data
+ * calibration offset is same for channels 1, 3, 4, 5
+ *
+ * The data is taken from GPADC_TRIM registers description.
+ * GPADC_TRIM registers keep difference between the code measured
+ * at volt1 and volt2 input voltages and corresponding code1 and code2
+ */
+static const struct twl6030_ideal_code
+ twl6030_ideal[TWL6030_GPADC_MAX_CHANNELS] = {
+ [0] = { /* ch 0, external, battery type, resistor value */
+ .code1 = 116,
+ .code2 = 745,
+ .volt1 = 141,
+ .volt2 = 910,
+ },
+ [1] = { /* ch 1, external, battery temperature, NTC resistor value */
+ .code1 = 82,
+ .code2 = 900,
+ .volt1 = 100,
+ .volt2 = 1100,
+ },
+ [2] = { /* ch 2, external, audio accessory/general purpose */
+ .code1 = 55,
+ .code2 = 818,
+ .volt1 = 101,
+ .volt2 = 1499,
+ },
+ [3] = { /* ch 3, external, general purpose */
+ .code1 = 82,
+ .code2 = 900,
+ .volt1 = 100,
+ .volt2 = 1100,
+ },
+ [4] = { /* ch 4, external, temperature measurement/general purpose */
+ .code1 = 82,
+ .code2 = 900,
+ .volt1 = 100,
+ .volt2 = 1100,
+ },
+ [5] = { /* ch 5, external, general purpose */
+ .code1 = 82,
+ .code2 = 900,
+ .volt1 = 100,
+ .volt2 = 1100,
+ },
+ [6] = { /* ch 6, external, general purpose */
+ .code1 = 82,
+ .code2 = 900,
+ .volt1 = 100,
+ .volt2 = 1100,
+ },
+ [7] = { /* ch 7, internal, main battery */
+ .code1 = 614,
+ .code2 = 941,
+ .volt1 = 3001,
+ .volt2 = 4599,
+ },
+ [8] = { /* ch 8, internal, backup battery */
+ .code1 = 82,
+ .code2 = 688,
+ .volt1 = 501,
+ .volt2 = 4203,
+ },
+ [9] = { /* ch 9, internal, external charger input */
+ .code1 = 182,
+ .code2 = 818,
+ .volt1 = 2001,
+ .volt2 = 8996,
+ },
+ [10] = { /* ch 10, internal, VBUS */
+ .code1 = 149,
+ .code2 = 818,
+ .volt1 = 1001,
+ .volt2 = 5497,
+ },
+ [11] = {}, /* ch 11, internal, VBUS charging current */
+ [12] = {}, /* ch 12, internal, Die temperature */
+ [13] = {}, /* ch 13, internal, Die temperature */
+ [14] = { /* ch 14, internal, USB ID line */
+ .code1 = 48,
+ .code2 = 714,
+ .volt1 = 323,
+ .volt2 = 4800,
+ },
+ [15] = {}, /* ch 15, internal, test network */
+ [16] = {}, /* ch 16, internal, test network */
+};
+
+static const struct twl6030_ideal_code
+ twl6032_ideal[TWL6032_GPADC_MAX_CHANNELS] = {
+ [0] = { /* ch 0, external, battery type, resistor value */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 440,
+ .volt2 = 1000,
+ },
+ [1] = { /* ch 1, external, battery temperature, NTC resistor value */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 440,
+ .volt2 = 1000,
+ },
+ [2] = { /* ch 2, external, audio accessory/general purpose */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 660,
+ .volt2 = 1500,
+ },
+ [3] = { /* ch 3, external, temperature with external diode/general
+ purpose */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 440,
+ .volt2 = 1000,
+ },
+ [4] = { /* ch 4, external, temperature measurement/general purpose */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 440,
+ .volt2 = 1000,
+ },
+ [5] = { /* ch 5, external, general purpose */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 440,
+ .volt2 = 1000,
+ },
+ [6] = { /* ch 6, external, general purpose */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 440,
+ .volt2 = 1000,
+ },
+ [7] = { /* ch7, internal, system supply */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 2200,
+ .volt2 = 5000,
+ },
+ [8] = { /* ch8, internal, backup battery */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 2200,
+ .volt2 = 5000,
+ },
+ [9] = { /* ch 9, internal, external charger input */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 3960,
+ .volt2 = 9000,
+ },
+ [10] = { /* ch10, internal, VBUS */
+ .code1 = 150,
+ .code2 = 751,
+ .volt1 = 1000,
+ .volt2 = 5000,
+ },
+ [11] = { /* ch 11, internal, VBUS DC-DC output current */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 660,
+ .volt2 = 1500,
+ },
+ [12] = { /* ch 12, internal, Die temperature */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 440,
+ .volt2 = 1000,
+ },
+ [13] = { /* ch 13, internal, Die temperature */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 440,
+ .volt2 = 1000,
+ },
+ [14] = { /* ch 14, internal, USB ID line */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 2420,
+ .volt2 = 5500,
+ },
+ [15] = {}, /* ch 15, internal, test network */
+ [16] = {}, /* ch 16, internal, test network */
+ [17] = {}, /* ch 17, internal, battery charging current */
+ [18] = { /* ch 18, internal, battery voltage */
+ .code1 = 1441,
+ .code2 = 3276,
+ .volt1 = 2200,
+ .volt2 = 5000,
+ },
+};
+
+static inline int twl6030_gpadc_write(u8 reg, u8 val)
+{
+ return twl_i2c_write_u8(TWL6030_MODULE_GPADC, val, reg);
+}
+
+static inline int twl6030_gpadc_read(u8 reg, u8 *val)
+{
+
+ return twl_i2c_read(TWL6030_MODULE_GPADC, val, reg, 2);
+}
+
+static int twl6030_gpadc_enable_irq(u8 mask)
+{
+ int ret;
+
+ ret = twl6030_interrupt_unmask(mask, REG_INT_MSK_LINE_B);
+ if (ret < 0)
+ return ret;
+
+ ret = twl6030_interrupt_unmask(mask, REG_INT_MSK_STS_B);
+
+ return ret;
+}
+
+static void twl6030_gpadc_disable_irq(u8 mask)
+{
+ twl6030_interrupt_mask(mask, REG_INT_MSK_LINE_B);
+ twl6030_interrupt_mask(mask, REG_INT_MSK_STS_B);
+}
+
+static irqreturn_t twl6030_gpadc_irq_handler(int irq, void *indio_dev)
+{
+ struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
+
+ complete(&gpadc->irq_complete);
+
+ return IRQ_HANDLED;
+}
+
+static int twl6030_start_conversion(int channel)
+{
+ return twl6030_gpadc_write(TWL6030_GPADC_CTRL_P1,
+ TWL6030_GPADC_CTRL_P1_SP1);
+}
+
+static int twl6032_start_conversion(int channel)
+{
+ int ret;
+
+ ret = twl6030_gpadc_write(TWL6032_GPADC_GPSELECT_ISB, channel);
+ if (ret)
+ return ret;
+
+ return twl6030_gpadc_write(TWL6032_GPADC_CTRL_P1,
+ TWL6030_GPADC_CTRL_P1_SP1);
+}
+
+static u8 twl6030_channel_to_reg(int channel)
+{
+ return TWL6030_GPADC_GPCH0_LSB + 2 * channel;
+}
+
+static u8 twl6032_channel_to_reg(int channel)
+{
+ /*
+ * for any prior chosen channel, when the conversion is ready
+ * the result is avalable in GPCH0_LSB, GPCH0_MSB.
+ */
+
+ return TWL6032_GPADC_GPCH0_LSB;
+}
+
+static int twl6030_channel_calibrated(const struct twl6030_gpadc_platform_data
+ *pdata, int channel)
+{
+ /* not calibrated channels have 0 in all structure members */
+ return pdata->ideal[channel].code2;
+}
+
+static int twl6030_gpadc_make_correction(struct twl6030_gpadc_data *gpadc,
+ int channel, int raw_code)
+{
+ int corrected_code;
+
+ corrected_code = ((raw_code * 1000) -
+ gpadc->twl6030_cal_tbl[channel].offset_error) /
+ gpadc->twl6030_cal_tbl[channel].gain_error;
+
+ return corrected_code;
+}
+
+static int twl6030_gpadc_get_raw(struct twl6030_gpadc_data *gpadc,
+ int channel, int *res)
+{
+ u8 reg = gpadc->pdata->channel_to_reg(channel);
+ __le16 val;
+ int raw_code;
+ int ret;
+
+ ret = twl6030_gpadc_read(reg, (u8 *)&val);
+ if (ret) {
+ dev_dbg(gpadc->dev, "unable to read register 0x%X\n", reg);
+ return ret;
+ }
+
+ raw_code = le16_to_cpu(val);
+ dev_dbg(gpadc->dev, "GPADC raw code: %d", raw_code);
+
+ if (twl6030_channel_calibrated(gpadc->pdata, channel))
+ *res = twl6030_gpadc_make_correction(gpadc, channel, raw_code);
+ else
+ *res = raw_code;
+
+ return ret;
+}
+
+static int twl6030_gpadc_get_processed(struct twl6030_gpadc_data *gpadc,
+ int channel, int *val)
+{
+ int corrected_code;
+ int channel_value;
+ int ret;
+
+ ret = twl6030_gpadc_get_raw(gpadc, channel, &corrected_code);
+ if (ret)
+ return ret;
+
+ channel_value = corrected_code *
+ gpadc->twl6030_cal_tbl[channel].gain;
+
+ /* Shift back into mV range */
+ channel_value /= 1000;
+
+ dev_dbg(gpadc->dev, "GPADC corrected code: %d", corrected_code);
+ dev_dbg(gpadc->dev, "GPADC value: %d", channel_value);
+
+ *val = channel_value;
+
+ return ret;
+}
+
+static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
+ int ret = -EINVAL;
+ long timeout;
+
+ mutex_lock(&gpadc->lock);
+
+ ret = gpadc->pdata->start_conversion(chan->channel);
+ if (ret) {
+ dev_err(gpadc->dev, "failed to start conversion\n");
+ goto err;
+ }
+ /* wait for conversion to complete */
+ timeout = wait_for_completion_interruptible_timeout(
+ &gpadc->irq_complete, msecs_to_jiffies(5000));
+ if (!timeout) {
+ ret = -ETIMEDOUT;
+ goto err;
+ } else if (timeout < 0) {
+ goto err;
+ ret = -EINTR;
+ }
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = twl6030_gpadc_get_raw(gpadc, chan->channel, val);
+ ret = ret ? -EIO : IIO_VAL_INT;
+ break;
+
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = twl6030_gpadc_get_processed(gpadc, chan->channel, val);
+ ret = ret ? -EIO : IIO_VAL_INT;
+ break;
+
+ default:
+ break;
+ }
+err:
+ mutex_unlock(&gpadc->lock);
+
+ return ret;
+}
+
+/*
+ * The GPADC channels are calibrated using a two point calibration method.
+ * The channels measured with two known values: volt1 and volt2, and
+ * ideal corresponding output codes are known: code1, code2.
+ * The difference(d1, d2) between ideal and measured codes stored in trim
+ * registers.
+ * The goal is to find offset and gain of the real curve for each calibrated
+ * channel.
+ * gain: k = 1 + ((d2 - d1) / (x2 - x1))
+ * offset: b = d1 + (k - 1) * x1
+ */
+static void twl6030_calibrate_channel(struct twl6030_gpadc_data *gpadc,
+ int channel, int d1, int d2)
+{
+ int b, k, gain, x1, x2;
+ const struct twl6030_ideal_code *ideal = gpadc->pdata->ideal;
+
+ /* Gain */
+ gain = ((ideal[channel].volt2 - ideal[channel].volt1) * 1000) /
+ (ideal[channel].code2 - ideal[channel].code1);
+
+ x1 = ideal[channel].code1;
+ x2 = ideal[channel].code2;
+
+ /* k - real curve gain */
+ k = 1000 + (((d2 - d1) * 1000) / (x2 - x1));
+
+ /* b - offset of the real curve gain */
+ b = (d1 * 1000) - (k - 1000) * x1;
+
+ gpadc->twl6030_cal_tbl[channel].gain = gain;
+ gpadc->twl6030_cal_tbl[channel].gain_error = k;
+ gpadc->twl6030_cal_tbl[channel].offset_error = b;
+
+ dev_dbg(gpadc->dev, "GPADC d1 for Chn: %d = %d\n", channel, d1);
+ dev_dbg(gpadc->dev, "GPADC d2 for Chn: %d = %d\n", channel, d2);
+ dev_dbg(gpadc->dev, "GPADC x1 for Chn: %d = %d\n", channel, x1);
+ dev_dbg(gpadc->dev, "GPADC x2 for Chn: %d = %d\n", channel, x2);
+ dev_dbg(gpadc->dev, "GPADC Gain for Chn: %d = %d\n", channel, gain);
+ dev_dbg(gpadc->dev, "GPADC k for Chn: %d = %d\n", channel, k);
+ dev_dbg(gpadc->dev, "GPADC b for Chn: %d = %d\n", channel, b);
+}
+
+static inline int twl6030_gpadc_get_trim_offset(s8 d)
+{
+ /*
+ * XXX NOTE!
+ * bit 0 - sign, bit 7 - reserved, 6..1 - trim value
+ * though, the documentation states that trim value
+ * is absolute value, the correct conversion results are
+ * obtained if the value is interpreted as 2's complement.
+ */
+ __u32 temp = ((d & 0x7f) >> 1) | ((d & 1) << 6);
+
+ return sign_extend32(temp, 6);
+}
+
+static int twl6030_calibration(struct twl6030_gpadc_data *gpadc)
+{
+ int ret;
+ int chn;
+ u8 trim_regs[TWL6030_GPADC_MAX_CHANNELS];
+ s8 d1, d2;
+
+ /*
+ * for calibration two measurements have been performed at
+ * factory, for some channels, during the production test and
+ * have been stored in registers. This two stored values are
+ * used to correct the measurements. The values represent
+ * offsets for the given input from the output on ideal curve.
+ */
+ ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs,
+ TWL6030_GPADC_TRIM1, 16);
+ if (ret < 0) {
+ dev_err(gpadc->dev, "calibration failed\n");
+ return ret;
+ }
+
+ for (chn = 0; chn < TWL6030_GPADC_MAX_CHANNELS; chn++) {
+
+ switch (chn) {
+ case 0:
+ d1 = trim_regs[0];
+ d2 = trim_regs[1];
+ break;
+ case 1:
+ case 3:
+ case 4:
+ case 5:
+ case 6:
+ d1 = trim_regs[4];
+ d2 = trim_regs[5];
+ break;
+ case 2:
+ d1 = trim_regs[12];
+ d2 = trim_regs[13];
+ break;
+ case 7:
+ d1 = trim_regs[6];
+ d2 = trim_regs[7];
+ break;
+ case 8:
+ d1 = trim_regs[2];
+ d2 = trim_regs[3];
+ break;
+ case 9:
+ d1 = trim_regs[8];
+ d2 = trim_regs[9];
+ break;
+ case 10:
+ d1 = trim_regs[10];
+ d2 = trim_regs[11];
+ break;
+ case 14:
+ d1 = trim_regs[14];
+ d2 = trim_regs[15];
+ break;
+ default:
+ continue;
+ }
+
+ d1 = twl6030_gpadc_get_trim_offset(d1);
+ d2 = twl6030_gpadc_get_trim_offset(d2);
+
+ twl6030_calibrate_channel(gpadc, chn, d1, d2);
+ }
+
+ return 0;
+}
+
+static int twl6032_get_trim_value(u8 *trim_regs, unsigned int reg0,
+ unsigned int reg1, unsigned int mask0, unsigned int mask1,
+ unsigned int shift0)
+{
+ int val;
+
+ val = (trim_regs[reg0] & mask0) << shift0;
+ val |= (trim_regs[reg1] & mask1) >> 1;
+ if (trim_regs[reg1] & 0x01)
+ val = -val;
+
+ return val;
+}
+
+static int twl6032_calibration(struct twl6030_gpadc_data *gpadc)
+{
+ int chn, d1 = 0, d2 = 0, temp;
+ u8 trim_regs[17];
+ int ret;
+
+ ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs + 1,
+ TWL6030_GPADC_TRIM1, 16);
+ if (ret < 0) {
+ dev_err(gpadc->dev, "calibration failed\n");
+ return ret;
+ }
+
+ /*
+ * Loop to calculate the value needed for returning voltages from
+ * GPADC not values.
+ *
+ * gain is calculated to 3 decimal places fixed point.
+ */
+ for (chn = 0; chn < TWL6032_GPADC_MAX_CHANNELS; chn++) {
+
+ switch (chn) {
+ case 0:
+ case 1:
+ case 2:
+ case 3:
+ case 4:
+ case 5:
+ case 6:
+ case 11:
+ case 12:
+ case 13:
+ case 14:
+ d1 = twl6032_get_trim_value(trim_regs, 3, 1, 0x1f,
+ 0x06, 2);
+ d2 = twl6032_get_trim_value(trim_regs, 4, 2, 0x3f,
+ 0x06, 2);
+ break;
+ case 8:
+ temp = twl6032_get_trim_value(trim_regs, 3, 1, 0x1f,
+ 0x06, 2);
+ d1 = temp + twl6032_get_trim_value(trim_regs, 8, 7,
+ 0x18, 0x1E, 1);
+
+ temp = twl6032_get_trim_value(trim_regs, 4, 2, 0x3F,
+ 0x06, 2);
+ d2 = temp + twl6032_get_trim_value(trim_regs, 10, 8,
+ 0x1F, 0x06, 2);
+ break;
+ case 9:
+ temp = twl6032_get_trim_value(trim_regs, 3, 1, 0x1f,
+ 0x06, 2);
+ d1 = temp + twl6032_get_trim_value(trim_regs, 14, 12,
+ 0x18, 0x1E, 1);
+
+ temp = twl6032_get_trim_value(trim_regs, 4, 2, 0x3f,
+ 0x06, 2);
+ d2 = temp + twl6032_get_trim_value(trim_regs, 16, 14,
+ 0x1F, 0x06, 1);
+ break;
+ case 10:
+ d1 = twl6032_get_trim_value(trim_regs, 11, 9, 0x0f,
+ 0x0E, 3);
+ d2 = twl6032_get_trim_value(trim_regs, 15, 13, 0x0f,
+ 0x0E, 3);
+ break;
+ case 7:
+ case 18:
+ temp = twl6032_get_trim_value(trim_regs, 3, 1, 0x1f,
+ 0x06, 2);
+
+ d1 = (trim_regs[5] & 0x7E) >> 1;
+ if (trim_regs[5] & 0x01)
+ d1 = -d1;
+ d1 += temp;
+
+ temp = twl6032_get_trim_value(trim_regs, 4, 2, 0x3f,
+ 0x06, 2);
+
+ d2 = (trim_regs[6] & 0xFE) >> 1;
+ if (trim_regs[6] & 0x01)
+ d2 = -d2;
+
+ d2 += temp;
+ break;
+ default:
+ /* No data for other channels */
+ continue;
+ }
+
+ twl6030_calibrate_channel(gpadc, chn, d1, d2);
+ }
+
+ return 0;
+}
+
+#define TWL6030_GPADC_CHAN(chn, _type, chan_info) { \
+ .type = _type, \
+ .channel = chn, \
+ .info_mask_separate = BIT(chan_info), \
+ .indexed = 1, \
+}
+
+/* internal test network channel */
+#define TWL6030_GPADC_TEST_CHAN(chn, chan_info) { \
+ .type = IIO_VOLTAGE, \
+ .channel = chn, \
+ .indexed = 1, \
+}
+
+static const struct iio_chan_spec twl6030_gpadc_iio_channels[] = {
+ TWL6030_GPADC_CHAN(0, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(1, IIO_TEMP, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(2, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(3, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(4, IIO_TEMP, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(5, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(6, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(7, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(8, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(9, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(10, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(11, IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(12, IIO_TEMP, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(13, IIO_TEMP, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(14, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_TEST_CHAN(15, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_TEST_CHAN(16, IIO_CHAN_INFO_RAW),
+};
+
+static const struct iio_chan_spec twl6032_gpadc_iio_channels[] = {
+ TWL6030_GPADC_CHAN(0, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(1, IIO_TEMP, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(2, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(3, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(4, IIO_TEMP, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(5, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(6, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(7, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(8, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(9, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(10, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(11, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_CHAN(12, IIO_TEMP, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(13, IIO_TEMP, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(14, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+ TWL6030_GPADC_TEST_CHAN(15, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_TEST_CHAN(16, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(17, IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+ TWL6030_GPADC_CHAN(18, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
+};
+
+static const struct iio_info twl6030_gpadc_iio_info = {
+ .read_raw = &twl6030_gpadc_read_raw,
+ .driver_module = THIS_MODULE,
+};
+
+static const struct twl6030_gpadc_platform_data twl6030_pdata = {
+ .iio_channels = twl6030_gpadc_iio_channels,
+ .nchannels = TWL6030_GPADC_MAX_CHANNELS,
+ .ideal = twl6030_ideal,
+ .start_conversion = twl6030_start_conversion,
+ .channel_to_reg = twl6030_channel_to_reg,
+ .calibrate = twl6030_calibration,
+};
+
+static const struct twl6030_gpadc_platform_data twl6032_pdata = {
+ .iio_channels = twl6032_gpadc_iio_channels,
+ .nchannels = TWL6032_GPADC_MAX_CHANNELS,
+ .ideal = twl6032_ideal,
+ .start_conversion = twl6032_start_conversion,
+ .channel_to_reg = twl6032_channel_to_reg,
+ .calibrate = twl6032_calibration,
+};
+
+static const struct of_device_id of_twl6030_match_tbl[] = {
+ {
+ .compatible = "ti,twl6030-gpadc",
+ .data = &twl6030_pdata,
+ },
+ {
+ .compatible = "ti,twl6032-gpadc",
+ .data = &twl6032_pdata,
+ },
+ { /* end */ }
+};
+
+static int twl6030_gpadc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct twl6030_gpadc_data *gpadc;
+ const struct twl6030_gpadc_platform_data *pdata;
+ const struct of_device_id *match;
+ struct iio_dev *indio_dev;
+ int irq;
+ int ret;
+
+ match = of_match_device(of_match_ptr(of_twl6030_match_tbl), dev);
+ if (!match)
+ return -EINVAL;
+
+ pdata = match->data;
+
+ indio_dev = iio_device_alloc(sizeof(*gpadc));
+ if (!indio_dev) {
+ dev_err(dev, "failed allocating iio device\n");
+ ret = -ENOMEM;
+ }
+
+ gpadc = iio_priv(indio_dev);
+
+ gpadc->twl6030_cal_tbl = devm_kzalloc(dev,
+ sizeof(struct twl6030_chnl_calib) *
+ pdata->nchannels, GFP_KERNEL);
+ if (!gpadc->twl6030_cal_tbl)
+ goto err_free_device;
+
+ gpadc->dev = dev;
+ gpadc->pdata = pdata;
+
+ platform_set_drvdata(pdev, indio_dev);
+ mutex_init(&gpadc->lock);
+ init_completion(&gpadc->irq_complete);
+
+ ret = pdata->calibrate(gpadc);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to read calibration registers\n");
+ goto err_free_device;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "failed to get irq\n");
+ goto err_free_device;
+ }
+
+ ret = request_threaded_irq(irq, NULL, twl6030_gpadc_irq_handler,
+ IRQF_ONESHOT, "twl6030_gpadc", indio_dev);
+ if (ret) {
+ dev_dbg(&pdev->dev, "could not request irq\n");
+ goto err_free_device;
+ }
+
+ ret = twl6030_gpadc_enable_irq(TWL6030_GPADC_RT_SW1_EOC_MASK);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to enable GPADC interrupt\n");
+ goto err_free_irq;
+ }
+
+ ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, TWL6030_GPADCS,
+ TWL6030_REG_TOGGLE1);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to enable GPADC module\n");
+ goto err_free_irq;
+ }
+
+ indio_dev->name = DRIVER_NAME;
+ indio_dev->dev.parent = dev;
+ indio_dev->info = &twl6030_gpadc_iio_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = pdata->iio_channels;
+ indio_dev->num_channels = pdata->nchannels;
+
+ ret = iio_device_register(indio_dev);
+ if (ret)
+ goto err_free_irq;
+
+ return ret;
+
+err_free_irq:
+ free_irq(irq, indio_dev);
+err_free_device:
+ iio_device_free(indio_dev);
+
+ return ret;
+}
+
+static int twl6030_gpadc_remove(struct platform_device *pdev)
+{
+ struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+ twl6030_gpadc_disable_irq(TWL6030_GPADC_RT_SW1_EOC_MASK);
+ free_irq(platform_get_irq(pdev, 0), indio_dev);
+ iio_device_unregister(indio_dev);
+ iio_device_free(indio_dev);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int twl6030_gpadc_suspend(struct device *pdev)
+{
+ int ret;
+
+ ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, TWL6030_GPADCR,
+ TWL6030_REG_TOGGLE1);
+ if (ret)
+ dev_err(pdev, "error reseting GPADC (%d)!\n", ret);
+
+ return 0;
+};
+
+static int twl6030_gpadc_resume(struct device *pdev)
+{
+ int ret;
+
+ ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, TWL6030_GPADCS,
+ TWL6030_REG_TOGGLE1);
+ if (ret)
+ dev_err(pdev, "error setting GPADC (%d)!\n", ret);
+
+ return 0;
+};
+#endif
+
+static SIMPLE_DEV_PM_OPS(twl6030_gpadc_pm_ops, twl6030_gpadc_suspend,
+ twl6030_gpadc_resume);
+
+static struct platform_driver twl6030_gpadc_driver = {
+ .probe = twl6030_gpadc_probe,
+ .remove = twl6030_gpadc_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ .pm = &twl6030_gpadc_pm_ops,
+ .of_match_table = of_twl6030_match_tbl,
+ },
+};
+
+module_platform_driver(twl6030_gpadc_driver);
+
+MODULE_ALIAS("platform: " DRIVER_NAME);
+MODULE_AUTHOR("Texas Instruments Inc.");
+MODULE_DESCRIPTION("twl6030 ADC driver");
+MODULE_LICENSE("GPL");
--
1.8.1.2

2013-07-19 14:39:50

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] ARM: dts: twl: Add GPADC data to device tree

Hello.

On 19-07-2013 13:27, Oleksandr Kozaruk wrote:

> GPADC is the general purpose ADC present on twl6030.
> The dt data is interrupt used to trigger end of ADC
> conversion.

> Signed-off-by: Oleksandr Kozaruk <[email protected]>
> ---
> arch/arm/boot/dts/twl6030.dtsi | 6 ++++++
> 1 file changed, 6 insertions(+)

> diff --git a/arch/arm/boot/dts/twl6030.dtsi b/arch/arm/boot/dts/twl6030.dtsi
> index 2e3bd31..d7d4c28 100644
> --- a/arch/arm/boot/dts/twl6030.dtsi
> +++ b/arch/arm/boot/dts/twl6030.dtsi
> @@ -103,4 +103,10 @@
> compatible = "ti,twl6030-pwmled";
> #pwm-cells = <2>;
> };
> +
> + adc: gpadc {

Read my lips: the node should be called just "adc", not "gpadc".

WBR, Sergei

2013-07-19 15:42:29

by Grygorii Strashko

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] ARM: dts: twl: Add GPADC data to device tree

On 07/19/2013 05:39 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 19-07-2013 13:27, Oleksandr Kozaruk wrote:
>
>> GPADC is the general purpose ADC present on twl6030.
>> The dt data is interrupt used to trigger end of ADC
>> conversion.
>
>> Signed-off-by: Oleksandr Kozaruk <[email protected]>
>> ---
>> arch/arm/boot/dts/twl6030.dtsi | 6 ++++++
>> 1 file changed, 6 insertions(+)
>
>> diff --git a/arch/arm/boot/dts/twl6030.dtsi
>> b/arch/arm/boot/dts/twl6030.dtsi
>> index 2e3bd31..d7d4c28 100644
>> --- a/arch/arm/boot/dts/twl6030.dtsi
>> +++ b/arch/arm/boot/dts/twl6030.dtsi
>> @@ -103,4 +103,10 @@
>> compatible = "ti,twl6030-pwmled";
>> #pwm-cells = <2>;
>> };
>> +
>> + adc: gpadc {
>
> Read my lips: the node should be called just "adc", not "gpadc".
^^^^^^^^^^^^ Are you sure?

Why? The name was selected according to the documentation on device
"General purpose analog-to-digital converter (GPADC)".

PS. Following your logic - "GPIO" need to renamed to "IO" everywhere ;P

>
> WBR, Sergei
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Regards,
- grygorii

2013-07-19 18:18:53

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] ARM: dts: twl: Add GPADC data to device tree

Hello.

On 07/19/2013 07:40 PM, Grygorii Strashko wrote:

>>> GPADC is the general purpose ADC present on twl6030.
>>> The dt data is interrupt used to trigger end of ADC
>>> conversion.

>>> Signed-off-by: Oleksandr Kozaruk <[email protected]>
>>> ---
>>> arch/arm/boot/dts/twl6030.dtsi | 6 ++++++
>>> 1 file changed, 6 insertions(+)

>>> diff --git a/arch/arm/boot/dts/twl6030.dtsi
>>> b/arch/arm/boot/dts/twl6030.dtsi
>>> index 2e3bd31..d7d4c28 100644
>>> --- a/arch/arm/boot/dts/twl6030.dtsi
>>> +++ b/arch/arm/boot/dts/twl6030.dtsi
>>> @@ -103,4 +103,10 @@
>>> compatible = "ti,twl6030-pwmled";
>>> #pwm-cells = <2>;
>>> };
>>> +
>>> + adc: gpadc {

>> Read my lips: the node should be called just "adc", not "gpadc".
> ^^^^^^^^^^^^ Are you sure?

I didn't know how to express my disappointment from Oleksandr's inability
to understand what I wanted to convey to him from 2 attempts... first, he
changed the label instead of the node name, then he only dropped "twl6030_"
prefix from the name. I should probably have been even more specific before.

> Why? The name was selected according to the documentation on device "General
> purpose analog-to-digital converter (GPADC)".

Sigh, we simply don't care whether this ADC is general-purpose or not.
The main thing it is ADC.

> PS. Following your logic - "GPIO" need to renamed to "IO" everywhere ;P

GPIO is well known and established abbreviation, contrasted to GPADC.
Moreover, ePAPR spec lists "gpio" as a generic node name.

WBR, Sergei

2013-07-20 06:14:38

by Oleksandr Kozaruk

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] ARM: dts: twl: Add GPADC data to device tree

Hi Sergei,

On Fri, Jul 19, 2013 at 1:18 PM, Sergei Shtylyov
<[email protected]> wrote:
> Hello.
>
>
> On 07/19/2013 07:40 PM, Grygorii Strashko wrote:
>
>>>> GPADC is the general purpose ADC present on twl6030.
>>>> The dt data is interrupt used to trigger end of ADC
>>>> conversion.
>
>
>>>> Signed-off-by: Oleksandr Kozaruk <[email protected]>
>>>> ---
>>>> arch/arm/boot/dts/twl6030.dtsi | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>
>
>>>> diff --git a/arch/arm/boot/dts/twl6030.dtsi
>>>> b/arch/arm/boot/dts/twl6030.dtsi
>>>> index 2e3bd31..d7d4c28 100644
>>>> --- a/arch/arm/boot/dts/twl6030.dtsi
>>>> +++ b/arch/arm/boot/dts/twl6030.dtsi
>>>> @@ -103,4 +103,10 @@
>>>> compatible = "ti,twl6030-pwmled";
>>>> #pwm-cells = <2>;
>>>> };
>>>> +
>>>> + adc: gpadc {
>
>
>>> Read my lips: the node should be called just "adc", not "gpadc".
>>
>> ^^^^^^^^^^^^ Are you sure?
>
>
> I didn't know how to express my disappointment from Oleksandr's inability
> to understand what I wanted to convey to him from 2 attempts... first,
How would you comment the following code, v3.10-rc7:

arch/arm/boot/dts/dbx5x0.dtsi, line 375
375 ab8500-gpadc {
376 compatible =
"stericsson,ab8500-gpadc";
377 interrupts = <32
IRQ_TYPE_LEVEL_HIGH
arch/arm/boot/dts/dbx5x0.dtsi: ab8500-gpadc {
arch/arm/boot/dts/dbx5x0.dtsi:
compatible = "stericsson,ab8500-gpadc";
arch/arm/boot/dts/dbx5x0.dtsi:
vddadc-supply = <&ab8500_ldo_tvout_reg>;


arch/arm/boot/dts/sama5d3.dtsi: tsadcc: tsadcc@f8018000 {
arch/arm/boot/dts/sama5d3.dtsi: compatible =
"atmel,at91sam9x5-tsadcc";
arch/arm/boot/dts/sama5d3.dtsi:
atmel,tsadcc_clock = <300000>;

arch/arm/boot/dts/am33xx.dtsi: tscadc: tscadc@44e0d000 {
arch/arm/boot/dts/am33xx.dtsi: compatible = "ti,am3359-tscadc";
arch/arm/boot/dts/am33xx.dtsi: ti,hwmods = "adc_tsc";
arch/arm/boot/dts/am33xx.dtsi: am335x_adc: adc {
arch/arm/boot/dts/am33xx.dtsi: compatible =
"ti,am3359-adc";

Regards,
Sasha.

> changed the label instead of the node name, then he only dropped "twl6030_"
> prefix from the name. I should probably have been even more specific before.
>
>
>> Why? The name was selected according to the documentation on device
>> "General
>> purpose analog-to-digital converter (GPADC)".
>
>
> Sigh, we simply don't care whether this ADC is general-purpose or not.
> The main thing it is ADC.
>
>
>> PS. Following your logic - "GPIO" need to renamed to "IO" everywhere ;P
>
>
> GPIO is well known and established abbreviation, contrasted to GPADC.
> Moreover, ePAPR spec lists "gpio" as a generic node name.
>
>
> WBR, Sergei
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2013-07-20 10:43:52

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

On 07/19/2013 10:27 AM, Oleksandr Kozaruk wrote:
> The GPADC is general purpose ADC found on TWL6030, and TWL6032 PMIC,
> known also as Phoenix and PhoenixLite.
>
> The TWL6030 and TWL6032 have GPADC with 17 and 19 channels
> respectively. Some channels have current source and are used for
> measuring voltage drop on resistive load for detecting battery ID
> resistance, or measuring voltage drop on NTC resistors for external
> temperature measurements. Some channels measure voltage, (i.e. battery
> voltage), and have voltage dividers, thus, capable to scale voltage.
> Some channels are dedicated for measuring die temperature.
>
> Some channels are calibrated in 2 points, having offsets from ideal
> values kept in trim registers. This is used to correct measurements.
>
> The differences between GPADC in TWL6030 and TWL6032:
> - 10 bit vs 12 bit ADC;
> - 17 vs 19 channels;
> - channels have different purpose(i.e. battery voltage
> channel 8 vs channel 18);
> - trim values are interpreted differently.
>
> Based on the driver patched from Balaji TK, Graeme Gregory, Ambresh K,
> Girish S Ghongdemath.
>
> Signed-off-by: Balaji T K <[email protected]>
> Signed-off-by: Graeme Gregory <[email protected]>
> Signed-off-by: Oleksandr Kozaruk <[email protected]>
A few little bits and bobs inline.

My only major query is about the lack of info for the temperature
channels. How do you convert these to useful real world units?

Jonathan

> ---
> drivers/iio/adc/Kconfig | 14 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/twl6030-gpadc.c | 991 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1006 insertions(+)
> create mode 100644 drivers/iio/adc/twl6030-gpadc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index ab0767e6..3172461 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -150,6 +150,20 @@ config TI_AM335X_ADC
> Say yes here to build support for Texas Instruments ADC
> driver which is also a MFD client.
>
> +config TWL6030_GPADC
> + tristate "TWL6030 GPADC (General Purpose A/D Converter) Support"
> + depends on TWL4030_CORE
> + default n
> + help
> + Say yes here if you want support for the TWL6030/TWL6032 General
> + Purpose A/D Converter. This will add support for battery type
> + detection, battery voltage and temperature measurement, die
> + temperature measurement, system supply voltage, audio accessory,
> + USB ID detection.
> +
> + This driver can also be built as a module. If so, the module will be
> + called twl6030-gpadc.
> +
> config VIPERBOARD_ADC
> tristate "Viperboard ADC support"
> depends on MFD_VIPERBOARD && USB
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 0a825be..996ba09 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -16,4 +16,5 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> obj-$(CONFIG_MAX1363) += max1363.o
> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> +obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
> obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c
> new file mode 100644
> index 0000000..b42cfd6
> --- /dev/null
> +++ b/drivers/iio/adc/twl6030-gpadc.c
> @@ -0,0 +1,991 @@
> +/*
> + * TWL6030 GPADC module driver
> + *
> + * Copyright (C) 2009-2013 Texas Instruments Inc.
> + * Nishant Kamat <[email protected]>
> + * Balaji T K <[email protected]>
> + * Graeme Gregory <[email protected]>
> + * Girish S Ghongdemath <[email protected]>
> + * Ambresh K <[email protected]>
> + * Oleksandr Kozaruk <[email protected]
> + *
> + * Based on twl4030-madc.c
> + * Copyright (C) 2008 Nokia Corporation
> + * Mikko Ylinen <[email protected]>
> + *
> + * 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.
> + *
> + * 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., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define DRIVER_NAME "twl6030_gpadc"
> +
> +#define TWL6030_GPADC_MAX_CHANNELS 17
> +#define TWL6032_GPADC_MAX_CHANNELS 19
> +
> +#define TWL6030_GPADC_CTRL_P1 0x05
> +
> +#define TWL6032_GPADC_GPSELECT_ISB 0x07
> +#define TWL6032_GPADC_CTRL_P1 0x08
> +
> +#define TWL6032_GPADC_GPCH0_LSB 0x0d
> +#define TWL6032_GPADC_GPCH0_MSB 0x0e
> +
> +#define TWL6030_GPADC_CTRL_P1_SP1 BIT(3)
> +
> +#define TWL6030_GPADC_GPCH0_LSB (0x29)
> +
> +#define TWL6030_GPADC_RT_SW1_EOC_MASK BIT(5)
> +
> +#define TWL6030_GPADC_TRIM1 0xCD
> +
> +#define TWL6030_REG_TOGGLE1 0x90
> +#define TWL6030_GPADCS BIT(1)
> +#define TWL6030_GPADCR BIT(0)
> +
> +/**
> + * struct twl6030_chnl_calib - channel calibration
> + * @gain: slope coefficient for ideal curve
> + * @gain_error: gain error
> + * @offset_error: offset of the real curve
> + */
> +struct twl6030_chnl_calib {
> + s32 gain;
> + s32 gain_error;
> + s32 offset_error;
> +};
> +
> +/**
> + * struct twl6030_ideal_code - GPADC calibration parameters
> + * GPADC is calibrated in two points: close to the beginning and
> + * to the and of the measurable input range
> + *
> + * @code1: ideal code for the input at the beginning
> + * @code2: ideal code for at the end of the range
> + * @volt1: voltage input at the beginning(low voltage)
> + * @volt2: voltage input at the end(high voltage)
> + */
> +struct twl6030_ideal_code {
> + u16 code1;
> + u16 code2;
> + u16 volt1;
> + u16 volt2;
> +};
> +
> +struct twl6030_gpadc_data;
> +
> +/**
> + * struct twl6030_gpadc_platform_data - platform specific data
> + * @nchannels: number of GPADC channels
> + * @iio_channels: iio channels
> + * @twl6030_ideal: pointer to calibration parameters
> + * @start_conversion: pointer to ADC start conversion function
> + * @channel_to_reg pointer to ADC function to convert channel to
> + * register address for reading conversion result
> + * @calibrate: pointer to calibration function
> + */
> +struct twl6030_gpadc_platform_data {
> + const int nchannels;
> + const struct iio_chan_spec *iio_channels;
> + const struct twl6030_ideal_code *ideal;
> + int (*start_conversion)(int channel);
> + u8 (*channel_to_reg)(int channel);
> + int (*calibrate)(struct twl6030_gpadc_data *gpadc);
> +};
> +
> +/**
> + * struct twl6030_gpadc_data - GPADC data
> + * @dev: device pointer
> + * @lock: mutual exclusion lock for the structure
> + * @irq_complete: completion to signal end of conversion
> + * @twl6030_cal_tbl: pointer to calibration data for each
> + * channel with gain error and offset
> + * @pdata: pointer to device specific data
> + */
> +struct twl6030_gpadc_data {
> + struct device *dev;
> + struct mutex lock;
> + struct completion irq_complete;
> + struct twl6030_chnl_calib *twl6030_cal_tbl;
> + const struct twl6030_gpadc_platform_data *pdata;
> +};
> +
> +/*
> + * channels 11, 12, 13, 15 and 16 have no calibration data
> + * calibration offset is same for channels 1, 3, 4, 5
> + *
> + * The data is taken from GPADC_TRIM registers description.
> + * GPADC_TRIM registers keep difference between the code measured
> + * at volt1 and volt2 input voltages and corresponding code1 and code2
> + */
> +static const struct twl6030_ideal_code
> + twl6030_ideal[TWL6030_GPADC_MAX_CHANNELS] = {
> + [0] = { /* ch 0, external, battery type, resistor value */
> + .code1 = 116,
> + .code2 = 745,
> + .volt1 = 141,
> + .volt2 = 910,
> + },
> + [1] = { /* ch 1, external, battery temperature, NTC resistor value */
> + .code1 = 82,
> + .code2 = 900,
> + .volt1 = 100,
> + .volt2 = 1100,
> + },
> + [2] = { /* ch 2, external, audio accessory/general purpose */
> + .code1 = 55,
> + .code2 = 818,
> + .volt1 = 101,
> + .volt2 = 1499,
> + },
> + [3] = { /* ch 3, external, general purpose */
> + .code1 = 82,
> + .code2 = 900,
> + .volt1 = 100,
> + .volt2 = 1100,
> + },
> + [4] = { /* ch 4, external, temperature measurement/general purpose */
> + .code1 = 82,
> + .code2 = 900,
> + .volt1 = 100,
> + .volt2 = 1100,
> + },
> + [5] = { /* ch 5, external, general purpose */
> + .code1 = 82,
> + .code2 = 900,
> + .volt1 = 100,
> + .volt2 = 1100,
> + },
> + [6] = { /* ch 6, external, general purpose */
> + .code1 = 82,
> + .code2 = 900,
> + .volt1 = 100,
> + .volt2 = 1100,
> + },
> + [7] = { /* ch 7, internal, main battery */
> + .code1 = 614,
> + .code2 = 941,
> + .volt1 = 3001,
> + .volt2 = 4599,
> + },
> + [8] = { /* ch 8, internal, backup battery */
> + .code1 = 82,
> + .code2 = 688,
> + .volt1 = 501,
> + .volt2 = 4203,
> + },
> + [9] = { /* ch 9, internal, external charger input */
> + .code1 = 182,
> + .code2 = 818,
> + .volt1 = 2001,
> + .volt2 = 8996,
> + },
> + [10] = { /* ch 10, internal, VBUS */
> + .code1 = 149,
> + .code2 = 818,
> + .volt1 = 1001,
> + .volt2 = 5497,
> + },
> + [11] = {}, /* ch 11, internal, VBUS charging current */
> + [12] = {}, /* ch 12, internal, Die temperature */
> + [13] = {}, /* ch 13, internal, Die temperature */
> + [14] = { /* ch 14, internal, USB ID line */
> + .code1 = 48,
> + .code2 = 714,
> + .volt1 = 323,
> + .volt2 = 4800,
> + },
> + [15] = {}, /* ch 15, internal, test network */
> + [16] = {}, /* ch 16, internal, test network */
> +};
> +
> +static const struct twl6030_ideal_code
> + twl6032_ideal[TWL6032_GPADC_MAX_CHANNELS] = {
> + [0] = { /* ch 0, external, battery type, resistor value */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 440,
> + .volt2 = 1000,
> + },
> + [1] = { /* ch 1, external, battery temperature, NTC resistor value */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 440,
> + .volt2 = 1000,
> + },
> + [2] = { /* ch 2, external, audio accessory/general purpose */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 660,
> + .volt2 = 1500,
> + },
> + [3] = { /* ch 3, external, temperature with external diode/general
> + purpose */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 440,
> + .volt2 = 1000,
> + },
> + [4] = { /* ch 4, external, temperature measurement/general purpose */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 440,
> + .volt2 = 1000,
> + },
> + [5] = { /* ch 5, external, general purpose */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 440,
> + .volt2 = 1000,
> + },
> + [6] = { /* ch 6, external, general purpose */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 440,
> + .volt2 = 1000,
> + },
> + [7] = { /* ch7, internal, system supply */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 2200,
> + .volt2 = 5000,
> + },
> + [8] = { /* ch8, internal, backup battery */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 2200,
> + .volt2 = 5000,
> + },
> + [9] = { /* ch 9, internal, external charger input */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 3960,
> + .volt2 = 9000,
> + },
> + [10] = { /* ch10, internal, VBUS */
> + .code1 = 150,
> + .code2 = 751,
> + .volt1 = 1000,
> + .volt2 = 5000,
> + },
> + [11] = { /* ch 11, internal, VBUS DC-DC output current */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 660,
> + .volt2 = 1500,
> + },
> + [12] = { /* ch 12, internal, Die temperature */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 440,
> + .volt2 = 1000,
> + },
> + [13] = { /* ch 13, internal, Die temperature */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 440,
> + .volt2 = 1000,
> + },
> + [14] = { /* ch 14, internal, USB ID line */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 2420,
> + .volt2 = 5500,
> + },
> + [15] = {}, /* ch 15, internal, test network */
> + [16] = {}, /* ch 16, internal, test network */
> + [17] = {}, /* ch 17, internal, battery charging current */
> + [18] = { /* ch 18, internal, battery voltage */
> + .code1 = 1441,
> + .code2 = 3276,
> + .volt1 = 2200,
> + .volt2 = 5000,
> + },
> +};
> +
> +static inline int twl6030_gpadc_write(u8 reg, u8 val)
> +{
> + return twl_i2c_write_u8(TWL6030_MODULE_GPADC, val, reg);
> +}
> +
> +static inline int twl6030_gpadc_read(u8 reg, u8 *val)
> +{
> +
> + return twl_i2c_read(TWL6030_MODULE_GPADC, val, reg, 2);
> +}
> +
> +static int twl6030_gpadc_enable_irq(u8 mask)
> +{
> + int ret;
> +
> + ret = twl6030_interrupt_unmask(mask, REG_INT_MSK_LINE_B);
> + if (ret < 0)
> + return ret;
> +
> + ret = twl6030_interrupt_unmask(mask, REG_INT_MSK_STS_B);
> +
> + return ret;
> +}
> +
> +static void twl6030_gpadc_disable_irq(u8 mask)
> +{
> + twl6030_interrupt_mask(mask, REG_INT_MSK_LINE_B);
> + twl6030_interrupt_mask(mask, REG_INT_MSK_STS_B);
> +}
> +
> +static irqreturn_t twl6030_gpadc_irq_handler(int irq, void *indio_dev)
> +{
> + struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
> +
> + complete(&gpadc->irq_complete);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int twl6030_start_conversion(int channel)
> +{
> + return twl6030_gpadc_write(TWL6030_GPADC_CTRL_P1,
> + TWL6030_GPADC_CTRL_P1_SP1);
> +}
> +
> +static int twl6032_start_conversion(int channel)
> +{
> + int ret;
> +
> + ret = twl6030_gpadc_write(TWL6032_GPADC_GPSELECT_ISB, channel);
> + if (ret)
> + return ret;
> +
> + return twl6030_gpadc_write(TWL6032_GPADC_CTRL_P1,
> + TWL6030_GPADC_CTRL_P1_SP1);
> +}
> +
> +static u8 twl6030_channel_to_reg(int channel)
> +{
> + return TWL6030_GPADC_GPCH0_LSB + 2 * channel;
> +}
> +
> +static u8 twl6032_channel_to_reg(int channel)
> +{
> + /*
> + * for any prior chosen channel, when the conversion is ready
> + * the result is avalable in GPCH0_LSB, GPCH0_MSB.
> + */
> +
> + return TWL6032_GPADC_GPCH0_LSB;
> +}
> +
> +static int twl6030_channel_calibrated(const struct twl6030_gpadc_platform_data
> + *pdata, int channel)
> +{
> + /* not calibrated channels have 0 in all structure members */
> + return pdata->ideal[channel].code2;
> +}
> +
> +static int twl6030_gpadc_make_correction(struct twl6030_gpadc_data *gpadc,
> + int channel, int raw_code)
> +{
> + int corrected_code;
> +
> + corrected_code = ((raw_code * 1000) -
> + gpadc->twl6030_cal_tbl[channel].offset_error) /
> + gpadc->twl6030_cal_tbl[channel].gain_error;
> +
> + return corrected_code;
> +}
> +
> +static int twl6030_gpadc_get_raw(struct twl6030_gpadc_data *gpadc,
> + int channel, int *res)
> +{
> + u8 reg = gpadc->pdata->channel_to_reg(channel);
> + __le16 val;
> + int raw_code;
> + int ret;
> +
> + ret = twl6030_gpadc_read(reg, (u8 *)&val);
> + if (ret) {
> + dev_dbg(gpadc->dev, "unable to read register 0x%X\n", reg);
> + return ret;
> + }
> +
> + raw_code = le16_to_cpu(val);
> + dev_dbg(gpadc->dev, "GPADC raw code: %d", raw_code);
> +
> + if (twl6030_channel_calibrated(gpadc->pdata, channel))
> + *res = twl6030_gpadc_make_correction(gpadc, channel, raw_code);
> + else
> + *res = raw_code;
> +
> + return ret;
> +}
> +
> +static int twl6030_gpadc_get_processed(struct twl6030_gpadc_data *gpadc,
> + int channel, int *val)
> +{
> + int corrected_code;
> + int channel_value;
> + int ret;
> +
> + ret = twl6030_gpadc_get_raw(gpadc, channel, &corrected_code);
> + if (ret)
> + return ret;
> +
> + channel_value = corrected_code *
> + gpadc->twl6030_cal_tbl[channel].gain;
> +
> + /* Shift back into mV range */
> + channel_value /= 1000;
> +
> + dev_dbg(gpadc->dev, "GPADC corrected code: %d", corrected_code);
> + dev_dbg(gpadc->dev, "GPADC value: %d", channel_value);
> +
> + *val = channel_value;
> +
> + return ret;
> +}
> +
> +static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
> + int ret = -EINVAL;
I'm suprised you didn't get a warning about the assigment above
being pointless as you overwrite ret just below.

> + long timeout;
> +
> + mutex_lock(&gpadc->lock);
> +
> + ret = gpadc->pdata->start_conversion(chan->channel);
> + if (ret) {
> + dev_err(gpadc->dev, "failed to start conversion\n");
> + goto err;
> + }
> + /* wait for conversion to complete */
> + timeout = wait_for_completion_interruptible_timeout(
> + &gpadc->irq_complete, msecs_to_jiffies(5000));
timeout == 0 would be a little clearer.
> + if (!timeout) {
> + ret = -ETIMEDOUT;
> + goto err;
> + } else if (timeout < 0) {
> + goto err;
> + ret = -EINTR;
> + }
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = twl6030_gpadc_get_raw(gpadc, chan->channel, val);
> + ret = ret ? -EIO : IIO_VAL_INT;
> + break;
> +
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = twl6030_gpadc_get_processed(gpadc, chan->channel, val);
> + ret = ret ? -EIO : IIO_VAL_INT;
> + break;
> +
> + default:
> + break;
> + }
> +err:
> + mutex_unlock(&gpadc->lock);
> +
> + return ret;
> +}
> +
> +/*
> + * The GPADC channels are calibrated using a two point calibration method.
> + * The channels measured with two known values: volt1 and volt2, and
> + * ideal corresponding output codes are known: code1, code2.
> + * The difference(d1, d2) between ideal and measured codes stored in trim
> + * registers.
> + * The goal is to find offset and gain of the real curve for each calibrated
> + * channel.
> + * gain: k = 1 + ((d2 - d1) / (x2 - x1))
> + * offset: b = d1 + (k - 1) * x1
> + */
> +static void twl6030_calibrate_channel(struct twl6030_gpadc_data *gpadc,
> + int channel, int d1, int d2)
> +{
> + int b, k, gain, x1, x2;
> + const struct twl6030_ideal_code *ideal = gpadc->pdata->ideal;
> +
> + /* Gain */
> + gain = ((ideal[channel].volt2 - ideal[channel].volt1) * 1000) /
> + (ideal[channel].code2 - ideal[channel].code1);
> +
> + x1 = ideal[channel].code1;
> + x2 = ideal[channel].code2;
> +
> + /* k - real curve gain */
> + k = 1000 + (((d2 - d1) * 1000) / (x2 - x1));
> +
> + /* b - offset of the real curve gain */
> + b = (d1 * 1000) - (k - 1000) * x1;
> +
> + gpadc->twl6030_cal_tbl[channel].gain = gain;
> + gpadc->twl6030_cal_tbl[channel].gain_error = k;
> + gpadc->twl6030_cal_tbl[channel].offset_error = b;
> +
> + dev_dbg(gpadc->dev, "GPADC d1 for Chn: %d = %d\n", channel, d1);
> + dev_dbg(gpadc->dev, "GPADC d2 for Chn: %d = %d\n", channel, d2);
> + dev_dbg(gpadc->dev, "GPADC x1 for Chn: %d = %d\n", channel, x1);
> + dev_dbg(gpadc->dev, "GPADC x2 for Chn: %d = %d\n", channel, x2);
> + dev_dbg(gpadc->dev, "GPADC Gain for Chn: %d = %d\n", channel, gain);
> + dev_dbg(gpadc->dev, "GPADC k for Chn: %d = %d\n", channel, k);
> + dev_dbg(gpadc->dev, "GPADC b for Chn: %d = %d\n", channel, b);
> +}
> +
> +static inline int twl6030_gpadc_get_trim_offset(s8 d)
> +{
> + /*
> + * XXX NOTE!
> + * bit 0 - sign, bit 7 - reserved, 6..1 - trim value
> + * though, the documentation states that trim value
> + * is absolute value, the correct conversion results are
> + * obtained if the value is interpreted as 2's complement.
> + */
> + __u32 temp = ((d & 0x7f) >> 1) | ((d & 1) << 6);
> +
> + return sign_extend32(temp, 6);
> +}
> +
> +static int twl6030_calibration(struct twl6030_gpadc_data *gpadc)
> +{
> + int ret;
> + int chn;
> + u8 trim_regs[TWL6030_GPADC_MAX_CHANNELS];
> + s8 d1, d2;
> +
> + /*
> + * for calibration two measurements have been performed at
> + * factory, for some channels, during the production test and
> + * have been stored in registers. This two stored values are
> + * used to correct the measurements. The values represent
> + * offsets for the given input from the output on ideal curve.
> + */
> + ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs,
> + TWL6030_GPADC_TRIM1, 16);
> + if (ret < 0) {
> + dev_err(gpadc->dev, "calibration failed\n");
> + return ret;
> + }
> +
> + for (chn = 0; chn < TWL6030_GPADC_MAX_CHANNELS; chn++) {
> +
> + switch (chn) {
> + case 0:
> + d1 = trim_regs[0];
> + d2 = trim_regs[1];
> + break;
> + case 1:
> + case 3:
> + case 4:
> + case 5:
> + case 6:
> + d1 = trim_regs[4];
> + d2 = trim_regs[5];
> + break;
> + case 2:
> + d1 = trim_regs[12];
> + d2 = trim_regs[13];
> + break;
> + case 7:
> + d1 = trim_regs[6];
> + d2 = trim_regs[7];
> + break;
> + case 8:
> + d1 = trim_regs[2];
> + d2 = trim_regs[3];
> + break;
> + case 9:
> + d1 = trim_regs[8];
> + d2 = trim_regs[9];
> + break;
> + case 10:
> + d1 = trim_regs[10];
> + d2 = trim_regs[11];
> + break;
> + case 14:
> + d1 = trim_regs[14];
> + d2 = trim_regs[15];
> + break;
> + default:
> + continue;
> + }
> +
> + d1 = twl6030_gpadc_get_trim_offset(d1);
> + d2 = twl6030_gpadc_get_trim_offset(d2);
> +
> + twl6030_calibrate_channel(gpadc, chn, d1, d2);
> + }
> +
> + return 0;
> +}
> +
> +static int twl6032_get_trim_value(u8 *trim_regs, unsigned int reg0,
> + unsigned int reg1, unsigned int mask0, unsigned int mask1,
> + unsigned int shift0)
> +{
> + int val;
> +
> + val = (trim_regs[reg0] & mask0) << shift0;
> + val |= (trim_regs[reg1] & mask1) >> 1;
> + if (trim_regs[reg1] & 0x01)
> + val = -val;
> +
> + return val;
> +}
> +
> +static int twl6032_calibration(struct twl6030_gpadc_data *gpadc)
> +{
> + int chn, d1 = 0, d2 = 0, temp;
> + u8 trim_regs[17];
Use the nice define you put in above instead of a mystical 17 ;)
> + int ret;
> +
> + ret = twl_i2c_read(TWL6030_MODULE_ID2, trim_regs + 1,
> + TWL6030_GPADC_TRIM1, 16);
> + if (ret < 0) {
> + dev_err(gpadc->dev, "calibration failed\n");
> + return ret;
> + }
> +
> + /*
> + * Loop to calculate the value needed for returning voltages from
> + * GPADC not values.
> + *
> + * gain is calculated to 3 decimal places fixed point.
> + */
> + for (chn = 0; chn < TWL6032_GPADC_MAX_CHANNELS; chn++) {
> +
> + switch (chn) {
> + case 0:
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + case 5:
> + case 6:
> + case 11:
> + case 12:
> + case 13:
> + case 14:
Much tidier.
> + d1 = twl6032_get_trim_value(trim_regs, 3, 1, 0x1f,
> + 0x06, 2);
> + d2 = twl6032_get_trim_value(trim_regs, 4, 2, 0x3f,
> + 0x06, 2);
> + break;
> + case 8:
> + temp = twl6032_get_trim_value(trim_regs, 3, 1, 0x1f,
> + 0x06, 2);
> + d1 = temp + twl6032_get_trim_value(trim_regs, 8, 7,
> + 0x18, 0x1E, 1);
> +
> + temp = twl6032_get_trim_value(trim_regs, 4, 2, 0x3F,
> + 0x06, 2);
> + d2 = temp + twl6032_get_trim_value(trim_regs, 10, 8,
> + 0x1F, 0x06, 2);
> + break;
> + case 9:
> + temp = twl6032_get_trim_value(trim_regs, 3, 1, 0x1f,
> + 0x06, 2);
> + d1 = temp + twl6032_get_trim_value(trim_regs, 14, 12,
> + 0x18, 0x1E, 1);
> +
> + temp = twl6032_get_trim_value(trim_regs, 4, 2, 0x3f,
> + 0x06, 2);
> + d2 = temp + twl6032_get_trim_value(trim_regs, 16, 14,
> + 0x1F, 0x06, 1);
> + break;
> + case 10:
> + d1 = twl6032_get_trim_value(trim_regs, 11, 9, 0x0f,
> + 0x0E, 3);
> + d2 = twl6032_get_trim_value(trim_regs, 15, 13, 0x0f,
> + 0x0E, 3);
> + break;
> + case 7:
> + case 18:
> + temp = twl6032_get_trim_value(trim_regs, 3, 1, 0x1f,
> + 0x06, 2);
> +
> + d1 = (trim_regs[5] & 0x7E) >> 1;
> + if (trim_regs[5] & 0x01)
> + d1 = -d1;
> + d1 += temp;
> +
> + temp = twl6032_get_trim_value(trim_regs, 4, 2, 0x3f,
> + 0x06, 2);
> +
> + d2 = (trim_regs[6] & 0xFE) >> 1;
> + if (trim_regs[6] & 0x01)
> + d2 = -d2;
> +
> + d2 += temp;
> + break;
> + default:
> + /* No data for other channels */
> + continue;
> + }
> +
> + twl6030_calibrate_channel(gpadc, chn, d1, d2);
> + }
> +
> + return 0;
> +}
> +
> +#define TWL6030_GPADC_CHAN(chn, _type, chan_info) { \
> + .type = _type, \
> + .channel = chn, \
> + .info_mask_separate = BIT(chan_info), \
> + .indexed = 1, \
> +}
> +


Why list these at all? I see they are no longer visible from
userspace, but they are still taking up memory etc without I
think ever being used?
> +/* internal test network channel */
> +#define TWL6030_GPADC_TEST_CHAN(chn, chan_info) { \
> + .type = IIO_VOLTAGE, \
> + .channel = chn, \
> + .indexed = 1, \
> +}
> +
> +static const struct iio_chan_spec twl6030_gpadc_iio_channels[] = {
> + TWL6030_GPADC_CHAN(0, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(1, IIO_TEMP, IIO_CHAN_INFO_RAW),
So we have no other information about the temp channels other than
raw adc counts? If so, how are these useful? I guess you might
be intending to use iio-hwmon to get these into hwmon the use
lm-sensors config files to convert to something useful.
Otherwise, you probably want to get the board specific info on
the calibration of these in here to make the data available to userspace
in a useful format...

> + TWL6030_GPADC_CHAN(2, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(3, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(4, IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(5, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(6, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(7, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(8, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(9, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(10, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(11, IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(12, IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(13, IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(14, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_TEST_CHAN(15, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_TEST_CHAN(16, IIO_CHAN_INFO_RAW),
> +};
> +
> +static const struct iio_chan_spec twl6032_gpadc_iio_channels[] = {
> + TWL6030_GPADC_CHAN(0, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(1, IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(2, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(3, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(4, IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(5, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(6, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(7, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(8, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(9, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(10, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(11, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_CHAN(12, IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(13, IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(14, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> + TWL6030_GPADC_TEST_CHAN(15, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_TEST_CHAN(16, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(17, IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TWL6030_GPADC_CHAN(18, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
> +};
> +
> +static const struct iio_info twl6030_gpadc_iio_info = {
> + .read_raw = &twl6030_gpadc_read_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static const struct twl6030_gpadc_platform_data twl6030_pdata = {
> + .iio_channels = twl6030_gpadc_iio_channels,
> + .nchannels = TWL6030_GPADC_MAX_CHANNELS,
> + .ideal = twl6030_ideal,
> + .start_conversion = twl6030_start_conversion,
> + .channel_to_reg = twl6030_channel_to_reg,
> + .calibrate = twl6030_calibration,
> +};
> +
> +static const struct twl6030_gpadc_platform_data twl6032_pdata = {
> + .iio_channels = twl6032_gpadc_iio_channels,
> + .nchannels = TWL6032_GPADC_MAX_CHANNELS,
> + .ideal = twl6032_ideal,
> + .start_conversion = twl6032_start_conversion,
> + .channel_to_reg = twl6032_channel_to_reg,
> + .calibrate = twl6032_calibration,
> +};
> +
> +static const struct of_device_id of_twl6030_match_tbl[] = {
> + {
> + .compatible = "ti,twl6030-gpadc",
> + .data = &twl6030_pdata,
> + },
> + {
> + .compatible = "ti,twl6032-gpadc",
> + .data = &twl6032_pdata,
> + },
> + { /* end */ }
> +};
> +
> +static int twl6030_gpadc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct twl6030_gpadc_data *gpadc;
> + const struct twl6030_gpadc_platform_data *pdata;
> + const struct of_device_id *match;
> + struct iio_dev *indio_dev;
> + int irq;
> + int ret;
> +
> + match = of_match_device(of_match_ptr(of_twl6030_match_tbl), dev);
> + if (!match)
> + return -EINVAL;
> +
> + pdata = match->data;
> +
> + indio_dev = iio_device_alloc(sizeof(*gpadc));
> + if (!indio_dev) {
> + dev_err(dev, "failed allocating iio device\n");
> + ret = -ENOMEM;
> + }
> +
> + gpadc = iio_priv(indio_dev);
> +
> + gpadc->twl6030_cal_tbl = devm_kzalloc(dev,
> + sizeof(struct twl6030_chnl_calib) *
> + pdata->nchannels, GFP_KERNEL);
Nitpick, but the following is easier to verify as correct aas it doesn't
require the reviewer to look up the type of twl6030_cal_tbl ;)

gpadc->twl6030_cal_tbl = devm_kzalloc(dev,
sizeof(*(gpadc->twl6030_cal_tbl) *
pdata->nchannels, GFP_KERNEL);

Random aside, why is there no devm_kcalloc?

> + if (!gpadc->twl6030_cal_tbl)
> + goto err_free_device;
> +
> + gpadc->dev = dev;
> + gpadc->pdata = pdata;
> +
> + platform_set_drvdata(pdev, indio_dev);
> + mutex_init(&gpadc->lock);
> + init_completion(&gpadc->irq_complete);
> +
> + ret = pdata->calibrate(gpadc);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to read calibration registers\n");
> + goto err_free_device;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "failed to get irq\n");
> + goto err_free_device;
> + }
> +
> + ret = request_threaded_irq(irq, NULL, twl6030_gpadc_irq_handler,
> + IRQF_ONESHOT, "twl6030_gpadc", indio_dev);
> + if (ret) {
> + dev_dbg(&pdev->dev, "could not request irq\n");
> + goto err_free_device;
> + }
> +
> + ret = twl6030_gpadc_enable_irq(TWL6030_GPADC_RT_SW1_EOC_MASK);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to enable GPADC interrupt\n");
> + goto err_free_irq;
> + }
> +
> + ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, TWL6030_GPADCS,
> + TWL6030_REG_TOGGLE1);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to enable GPADC module\n");
> + goto err_free_irq;
> + }
> +
> + indio_dev->name = DRIVER_NAME;
> + indio_dev->dev.parent = dev;
> + indio_dev->info = &twl6030_gpadc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = pdata->iio_channels;
> + indio_dev->num_channels = pdata->nchannels;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto err_free_irq;
> +
> + return ret;
> +
> +err_free_irq:
> + free_irq(irq, indio_dev);
> +err_free_device:
> + iio_device_free(indio_dev);
> +
> + return ret;
> +}
> +
> +static int twl6030_gpadc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> + twl6030_gpadc_disable_irq(TWL6030_GPADC_RT_SW1_EOC_MASK);
> + free_irq(platform_get_irq(pdev, 0), indio_dev);
> + iio_device_unregister(indio_dev);
> + iio_device_free(indio_dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int twl6030_gpadc_suspend(struct device *pdev)
> +{
> + int ret;
> +
> + ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, TWL6030_GPADCR,
> + TWL6030_REG_TOGGLE1);
> + if (ret)
> + dev_err(pdev, "error reseting GPADC (%d)!\n", ret);
> +
> + return 0;
> +};
> +
> +static int twl6030_gpadc_resume(struct device *pdev)
> +{
> + int ret;
> +
> + ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, TWL6030_GPADCS,
> + TWL6030_REG_TOGGLE1);
> + if (ret)
> + dev_err(pdev, "error setting GPADC (%d)!\n", ret);
> +
> + return 0;
> +};
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(twl6030_gpadc_pm_ops, twl6030_gpadc_suspend,
> + twl6030_gpadc_resume);
> +
> +static struct platform_driver twl6030_gpadc_driver = {
> + .probe = twl6030_gpadc_probe,
> + .remove = twl6030_gpadc_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .pm = &twl6030_gpadc_pm_ops,
> + .of_match_table = of_twl6030_match_tbl,
> + },
> +};
> +
> +module_platform_driver(twl6030_gpadc_driver);
> +
> +MODULE_ALIAS("platform: " DRIVER_NAME);
> +MODULE_AUTHOR("Texas Instruments Inc.");

I would normally expect an actual person for
the module author. Is this TI policy or simply a case of no clear single
author? Note I believe there is no problem with having multiple
MODULE_AUTHOR lines so that everyone who made a major contribution is
included.

> +MODULE_DESCRIPTION("twl6030 ADC driver");
> +MODULE_LICENSE("GPL");
>

2013-07-22 09:12:16

by Oleksandr Kozaruk

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

Hi Jonathan,

On Sat, Jul 20, 2013 at 1:43 PM, Jonathan Cameron <[email protected]> wrote:
> On 07/19/2013 10:27 AM, Oleksandr Kozaruk wrote:
>> The GPADC is general purpose ADC found on TWL6030, and TWL6032 PMIC,
>> known also as Phoenix and PhoenixLite.
>>
>> The TWL6030 and TWL6032 have GPADC with 17 and 19 channels
>> respectively. Some channels have current source and are used for
>> measuring voltage drop on resistive load for detecting battery ID
>> resistance, or measuring voltage drop on NTC resistors for external
>> temperature measurements. Some channels measure voltage, (i.e. battery
>> voltage), and have voltage dividers, thus, capable to scale voltage.
>> Some channels are dedicated for measuring die temperature.
>>
>> Some channels are calibrated in 2 points, having offsets from ideal
>> values kept in trim registers. This is used to correct measurements.
>>
>> The differences between GPADC in TWL6030 and TWL6032:
>> - 10 bit vs 12 bit ADC;
>> - 17 vs 19 channels;
>> - channels have different purpose(i.e. battery voltage
>> channel 8 vs channel 18);
>> - trim values are interpreted differently.
>>
>> Based on the driver patched from Balaji TK, Graeme Gregory, Ambresh K,
>> Girish S Ghongdemath.
>>
>> Signed-off-by: Balaji T K <[email protected]>
>> Signed-off-by: Graeme Gregory <[email protected]>
>> Signed-off-by: Oleksandr Kozaruk <[email protected]>
> A few little bits and bobs inline.
>
> My only major query is about the lack of info for the temperature
> channels. How do you convert these to useful real world units?

If I get your question right: the ADC channels 1, 4 are dedicated for
measuring resistive value.
The temperature measurement will depend on:
1) the ADC code(provided by the driver);
2) value of the NTC resistor, its characteristics, the way it is
plugged in the circuit,
and may be some calibration data(platform dependent); How the driver can the
drive take care of these?
[...]

>> +static int twl6030_gpadc_read_raw(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct twl6030_gpadc_data *gpadc = iio_priv(indio_dev);
>> + int ret = -EINVAL;
> I'm suprised you didn't get a warning about the assigment above
> being pointless as you overwrite ret just below.
Indeed, ret is overwritten, though, there is no warning from make C=2
and checkpatch is silent.
I'll remove the initialization.
[...]

>> +
>> +#define TWL6030_GPADC_CHAN(chn, _type, chan_info) { \
>> + .type = _type, \
>> + .channel = chn, \
>> + .info_mask_separate = BIT(chan_info), \
>> + .indexed = 1, \
>> +}
>> +
>
>
> Why list these at all? I see they are no longer visible from
> userspace, but they are still taking up memory etc without I
> think ever being used?
I've kept it because for twl6032 there is a gap if I drop channels 15, 16,
as channels 17, 18 are used.

>> +/* internal test network channel */
>> +#define TWL6030_GPADC_TEST_CHAN(chn, chan_info) { \
>> + .type = IIO_VOLTAGE, \
>> + .channel = chn, \
>> + .indexed = 1, \
>> +}
>> +
>> +static const struct iio_chan_spec twl6030_gpadc_iio_channels[] = {
>> + TWL6030_GPADC_CHAN(0, IIO_VOLTAGE, IIO_CHAN_INFO_PROCESSED),
>> + TWL6030_GPADC_CHAN(1, IIO_TEMP, IIO_CHAN_INFO_RAW),
> So we have no other information about the temp channels other than
> raw adc counts? If so, how are these useful? I guess you might
> be intending to use iio-hwmon to get these into hwmon the use
> lm-sensors config files to convert to something useful.
> Otherwise, you probably want to get the board specific info on
> the calibration of these in here to make the data available to userspace
> in a useful format...

Hmm, it seems that info on the NTC type is board specific. And we
should get it from device tree?
I thought the driver just gives the ADC code, and consumer will know
what to do with the ADC data.
So, calculation for converting to temperature should be done in this driver?
I don't know how yet.
[...]

>> +MODULE_AUTHOR("Texas Instruments Inc.");
>
> I would normally expect an actual person for
> the module author. Is this TI policy or simply a case of no clear single
> author? Note I believe there is no problem with having multiple
> MODULE_AUTHOR lines so that everyone who made a major contribution is
> included.

Yes, this is because of having multiple authors. I will change it for
Balaji, Graeme and myself.

Regards,
OK.

2013-07-25 06:09:49

by Oleksandr Kozaruk

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver

On Sat, Jul 20, 2013 at 11:43:42AM +0100, Jonathan Cameron wrote:
> On 07/19/2013 10:27 AM, Oleksandr Kozaruk wrote:
> > The GPADC is general purpose ADC found on TWL6030, and TWL6032 PMIC,
> > known also as Phoenix and PhoenixLite.
> >
> > The TWL6030 and TWL6032 have GPADC with 17 and 19 channels
> > respectively. Some channels have current source and are used for
> > measuring voltage drop on resistive load for detecting battery ID
> > resistance, or measuring voltage drop on NTC resistors for external
> > temperature measurements. Some channels measure voltage, (i.e. battery
> > voltage), and have voltage dividers, thus, capable to scale voltage.
> > Some channels are dedicated for measuring die temperature.
> >
> > Some channels are calibrated in 2 points, having offsets from ideal
> > values kept in trim registers. This is used to correct measurements.
> >
> > The differences between GPADC in TWL6030 and TWL6032:
> > - 10 bit vs 12 bit ADC;
> > - 17 vs 19 channels;
> > - channels have different purpose(i.e. battery voltage
> > channel 8 vs channel 18);
> > - trim values are interpreted differently.
> >
> > Based on the driver patched from Balaji TK, Graeme Gregory, Ambresh K,
> > Girish S Ghongdemath.
> >
> > Signed-off-by: Balaji T K <[email protected]>
> > Signed-off-by: Graeme Gregory <[email protected]>
> > Signed-off-by: Oleksandr Kozaruk <[email protected]>
> A few little bits and bobs inline.
>
> My only major query is about the lack of info for the temperature
> channels. How do you convert these to useful real world units?

On blaze tablet platform(OMAP 4) temperature channels were used by
thermal framework and battery driver.
Thermal was using channel 4 [1]

Here is the comment from the driver:

* NTC termistor (NCP15WB473F) schematic connection for OMAP4460 board:
*
* [Vref]
* |
* $ (Rpu)
* |
* +----+-----------[Vin]
* | |
* [Rt] $ (Rpd)
* | |
* -------- (ground)
*
* NTC termistor resistanse (Rt, k) calculated from following formula:
*
* Rt = Rpd * Rpu * Vin / (Rpd * (Vref - Vin) - Rpu * Vin)
*
* where Vref (GPADC_VREF4) - reference voltage, Vref = 1250 mV;
* Vin (GPADC_IN4) - measuring voltage, Vin = 0...1250 mV;
* Rpu (R1041) - pullup resistor, Rpu = 10 k;
* Rpd (R1043) - pulldown resistor, Rpd = 220 k;
*
* Pcb temp sensor temperature (t, C) calculated from following formula:
*
* t = 1 / (ln(Rt / Rt0) / B + 1 / T0) - 273
*
* where Rt0 - NTC termistor resistance at 25 C, Rt0 = 47 k;
* B - specific constant, B = 4131 K;
* T0 - temperature, T0 = 298 K
[..]

And then there is a table for conversion:
/*
* Temperature values in degrees celsius,
* voltage values from 156 to 1191 milli volts
*/
static s8 mvolt_to_temp[] = {
125, 125, 125, 124, 124, 124, 124, 123, 123, 123, 122, 122,
122, 122, 121, 121, 121, 121, 120, 120, 120, 120, 119, 119,
[..]

The battery driver was using channel 1, dedicated for measuring battery
temperature.
static void twl6030_bci_battery_work(struct work_struct *work)
{
[..]
for (temp = 0; temp < di->platform_data->tblsize; temp++) {
if (adc_code >= di->platform_data->
battery_tmp_tbl[temp])
break;
}

/* first 2 values are for negative temperature */
di->temp_C = (temp - 2) * 10; * /* in tenths of degree Celsius */
[..]
}
static int omap4_batt_table[] = {
/* adc code for temperature in degree C */
929, 925, /* -2 ,-1 */
920, 917, 912, 908, 904, 899, 895, 890, 885, 880, /* 00 - 09 */
875, 869, 864, 858, 853, 847, 841, 835, 829, 823, /* 10 - 19 */
[..]
591, 583, 575, 567, 559, 551, 543, 535, 527, 519, /* 50 - 59 */
511, 504, 496 /* 60 - 62 */
};

So, one driver was using millivolts and the other ADC code,
Though converting to millivolts and the to temperature, seems,
to be redundant, as code can be converted to temperature.
If I recollect correctly the previous driver version was using just
ADC code.

Regards,
Sasha.

[1] http://git.omapzoom.org/?p=kernel/omap.git;a=blob;f=drivers/staging/thermal_framework/sensor/thermistor_sensor.c;h=828d8010579b55ec4a122c49a2e5b547b1e41e63;hb=decb3fd22223ddf207e0118d0d558459acc7094e