Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On Mon, Jul 15, 2013 at 01:33:53PM +0200, Lars-Peter Clausen wrote:
On 07/15/2013 01:09 PM, Kozaruk, Oleksandr wrote:
[...]
+ ret = devm_request_threaded_irq(dev, irq, NULL,
+ twl6030_gpadc_irq_handler,
+ IRQF_ONESHOT, twl6030_gpadc, gpadc);
You access memory in the interrupt handler which is freed before
the interrupt
handler is freed.
Thanks for pointing this. devm_* will free memory for irq after the
driver
is removed and memory for the device is freed. I took me awhile to
understand
this. Is there going to be something like devm_iio_device_alloc?
whould it be helpfull?
Yes, I think it certainly makes sense to add a
devm_iio_device_alloc(), care
to send a patch?
Anything like this? (of course it's not a patch)
struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
{
struct iio_dev *indio_dev;
size_t alloc_size;
alloc_size = sizeof(struct iio_dev);
if (sizeof_priv) {
alloc_size = ALIGN(alloc_size, IIO_ALIGN);
alloc_size += sizeof_priv;
}
/* ensure 32-byte alignment of whole construct ? */
alloc_size += IIO_ALIGN - 1;
indio_dev = devm_kzalloc(dev, alloc_size, GFP_KERNEL);
if (indio_dev) {
indio_dev-dev.groups = indio_dev-groups;
indio_dev-dev.type = iio_device_type;
indio_dev-dev.bus = iio_bus_type;
device_initialize(indio_dev-dev);
dev_set_drvdata(indio_dev-dev, (void *)indio_dev);
mutex_init(indio_dev-mlock);
mutex_init(indio_dev-info_exist_lock);
INIT_LIST_HEAD(indio_dev-channel_attr_list);
indio_dev-id = ida_simple_get(iio_ida, 0, 0, GFP_KERNEL);
if (indio_dev-id 0) {
/* cannot use a dev_err as the name isn't available */
printk(KERN_ERR Failed to get id\n);
kfree(dev);
return NULL;
}
dev_set_name(indio_dev-dev, iio:device%d, indio_dev-id);
INIT_LIST_HEAD(indio_dev-buffer_list);
}
return indio_dev;
}
EXPORT_SYMBOL(devm_iio_device_alloc);
Regards,
OK
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/17/2013 03:45 PM, Oleksandr Kozaruk wrote:
On Mon, Jul 15, 2013 at 01:33:53PM +0200, Lars-Peter Clausen wrote:
On 07/15/2013 01:09 PM, Kozaruk, Oleksandr wrote:
[...]
+ ret = devm_request_threaded_irq(dev, irq, NULL,
+ twl6030_gpadc_irq_handler,
+ IRQF_ONESHOT, twl6030_gpadc, gpadc);
You access memory in the interrupt handler which is freed before the
interrupt
handler is freed.
Thanks for pointing this. devm_* will free memory for irq after the driver
is removed and memory for the device is freed. I took me awhile to
understand
this. Is there going to be something like devm_iio_device_alloc? whould
it be helpfull?
Yes, I think it certainly makes sense to add a devm_iio_device_alloc(), care
to send a patch?
Anything like this? (of course it's not a patch)
No. I think you can for example use devm_regulator_get() as a template. But
instead of regulator_get() and regulator_put() use iio_device_alloc() and
iio_device_free().
struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
{
struct iio_dev *indio_dev;
size_t alloc_size;
alloc_size = sizeof(struct iio_dev);
if (sizeof_priv) {
alloc_size = ALIGN(alloc_size, IIO_ALIGN);
alloc_size += sizeof_priv;
}
/* ensure 32-byte alignment of whole construct ? */
alloc_size += IIO_ALIGN - 1;
indio_dev = devm_kzalloc(dev, alloc_size, GFP_KERNEL);
if (indio_dev) {
indio_dev-dev.groups = indio_dev-groups;
indio_dev-dev.type = iio_device_type;
indio_dev-dev.bus = iio_bus_type;
device_initialize(indio_dev-dev);
dev_set_drvdata(indio_dev-dev, (void *)indio_dev);
mutex_init(indio_dev-mlock);
mutex_init(indio_dev-info_exist_lock);
INIT_LIST_HEAD(indio_dev-channel_attr_list);
indio_dev-id = ida_simple_get(iio_ida, 0, 0, GFP_KERNEL);
if (indio_dev-id 0) {
/* cannot use a dev_err as the name isn't available */
printk(KERN_ERR Failed to get id\n);
kfree(dev);
return NULL;
}
dev_set_name(indio_dev-dev, iio:device%d, indio_dev-id);
INIT_LIST_HEAD(indio_dev-buffer_list);
}
return indio_dev;
}
EXPORT_SYMBOL(devm_iio_device_alloc);
Regards,
OK
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
Hello Lars-Peter,
Thank you for the review.
diff --git a/drivers/iio/adc/twl6030-gpadc.c
b/drivers/iio/adc/twl6030-gpadc.c
new file mode 100644
index 000..6ceb789
--- /dev/null
+++ b/drivers/iio/adc/twl6030-gpadc.c
@@ -0,0 +1,1019 @@
[...]
+static u8 twl6032_channel_to_reg(int channel)
+{
+ return TWL6032_GPADC_GPCH0_LSB;
There is more than one channel, isn't there?
Yes. But for twl6032 channel of interest is chosen first. When the conversion
is ready tre result is available in GPCH0_LSB/GPCH1_MSB for any cosen
channel. For twl6030 there are as many result register pairs as many of
channels.
+ ret = devm_request_threaded_irq(dev, irq, NULL,
+ twl6030_gpadc_irq_handler,
+ IRQF_ONESHOT, twl6030_gpadc, gpadc);
You access memory in the interrupt handler which is freed before the interrupt
handler is freed.
Thanks for pointing this. devm_* will free memory for irq after the driver
is removed and memory for the device is freed. I took me awhile to understand
this. Is there going to be something like devm_iio_device_alloc? whould it be
helpfull?--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/15/2013 01:09 PM, Kozaruk, Oleksandr wrote: [...] + ret = devm_request_threaded_irq(dev, irq, NULL, + twl6030_gpadc_irq_handler, + IRQF_ONESHOT, twl6030_gpadc, gpadc); You access memory in the interrupt handler which is freed before the interrupt handler is freed. Thanks for pointing this. devm_* will free memory for irq after the driver is removed and memory for the device is freed. I took me awhile to understand this. Is there going to be something like devm_iio_device_alloc? whould it be helpfull? Yes, I think it certainly makes sense to add a devm_iio_device_alloc(), care to send a patch? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
Hi All,
I have a question regarding this patch and IIO in general
- Does IIO provide sync mechanism with system wide suspend/resume or
this should be handled by each driver itself?
What if during system suspend iio_read_channel_raw() (or any other
consumer API) will be called after gpadc driver have been suspended
already? (I did some investigation and seems it's possible - Am I right?)
If no, could info_exist_lock be reused for such purposes?
Regards,
-grygorii
On 07/12/2013 10:56 PM, Lars-Peter Clausen wrote:
A couple of comments inline.
On 07/12/2013 09:18 AM, Oleksandr Kozaruk wrote:
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..87d699e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -157,4 +157,12 @@ config VIPERBOARD_ADC
Say yes here to access the ADC part of the Nano River
Technologies Viperboard.
+config TWL6030_GPADC
+tristate TWL6030 GPADC (General Purpose A/D Convertor) Support
+depends on TWL4030_CORE
+default n
+help
+ Say yes here if you want support for the TWL6030 General Purpose
+ A/D Convertor.
+
Keep it in alphabetical order
endmenu
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8b05633 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
+obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
Same here.
diff --git a/drivers/iio/adc/twl6030-gpadc.c
b/drivers/iio/adc/twl6030-gpadc.c
new file mode 100644
index 000..6ceb789
--- /dev/null
+++ b/drivers/iio/adc/twl6030-gpadc.c
@@ -0,0 +1,1019 @@
[...]
+static u8 twl6032_channel_to_reg(int channel)
+{
+return TWL6032_GPADC_GPCH0_LSB;
There is more than one channel, isn't there?
[...]
+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;
+
+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 */
+wait_for_completion_interruptible_timeout(gpadc-irq_complete,
+msecs_to_jiffies(5000));
wait_for_completion_interruptible_timeout() can return either a negative
error code if it was interrupted, 0 if it timed out, or a positive value
if it was successfully completed. You need to handle all three cases.
Have a look at other existing drivers to see how to do this properly.
+
+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;
+}
+}
+
[...]
+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 = 0;
+
+match = of_match_device(of_match_ptr(of_twl6030_match_tbl), dev);
+pdata = match ? match-data : dev-platform_data;
+
+if (!pdata)
+return -EINVAL;
+
+indio_dev = iio_device_alloc(sizeof(struct twl6030_gpadc_data));
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;
+
+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;
+}
+
+irq = platform_get_irq(pdev, 0);
+if (irq 0) {
+dev_err(pdev-dev, failed to get irq\n);
+goto err;
+}
+
+ret = devm_request_threaded_irq(dev, irq, NULL,
+twl6030_gpadc_irq_handler,
+IRQF_ONESHOT, twl6030_gpadc, gpadc);
You access memory in the interrupt handler which is freed before the
interrupt handler is freed.
+if (ret)
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/15/2013 01:56 PM, Grygorii Strashko wrote:
Hi All,
I have a question regarding this patch and IIO in general
- Does IIO provide sync mechanism with system wide suspend/resume or this
should be handled by each driver itself?
What if during system suspend iio_read_channel_raw() (or any other consumer
API) will be called after gpadc driver have been suspended already? (I did
some investigation and seems it's possible - Am I right?)
If no, could info_exist_lock be reused for such purposes?
I think you can use the mlock for this purpose. Usually you'd have a flag in
your driver struct which you'd set to true in suspend and to false in
resume. And in the read_raw callback you'd check that flag before accessing
the hardware. If it turns out that this is a common pattern it probably
makes sense to add infrastructure for this to the IIO core. Something along
the lines of:
static int drv_suspend(...)
{
...
iio_device_set_suspended(iio_dev, true);
...
}
static int drv_suspend(...)
{
...
iio_device_set_suspended(iio_dev, false);
...
}
and then have the IIO core check that flag before calling the read_raw callback.
- Lars
Regards,
-grygorii
On 07/12/2013 10:56 PM, Lars-Peter Clausen wrote:
A couple of comments inline.
On 07/12/2013 09:18 AM, Oleksandr Kozaruk wrote:
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..87d699e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -157,4 +157,12 @@ config VIPERBOARD_ADC
Say yes here to access the ADC part of the Nano River
Technologies Viperboard.
+config TWL6030_GPADC
+tristate TWL6030 GPADC (General Purpose A/D Convertor) Support
+depends on TWL4030_CORE
+default n
+help
+ Say yes here if you want support for the TWL6030 General Purpose
+ A/D Convertor.
+
Keep it in alphabetical order
endmenu
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8b05633 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
+obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
Same here.
diff --git a/drivers/iio/adc/twl6030-gpadc.c
b/drivers/iio/adc/twl6030-gpadc.c
new file mode 100644
index 000..6ceb789
--- /dev/null
+++ b/drivers/iio/adc/twl6030-gpadc.c
@@ -0,0 +1,1019 @@
[...]
+static u8 twl6032_channel_to_reg(int channel)
+{
+return TWL6032_GPADC_GPCH0_LSB;
There is more than one channel, isn't there?
[...]
+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;
+
+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 */
+wait_for_completion_interruptible_timeout(gpadc-irq_complete,
+msecs_to_jiffies(5000));
wait_for_completion_interruptible_timeout() can return either a negative
error code if it was interrupted, 0 if it timed out, or a positive value
if it was successfully completed. You need to handle all three cases.
Have a look at other existing drivers to see how to do this properly.
+
+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;
+}
+}
+
[...]
+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 = 0;
+
+match = of_match_device(of_match_ptr(of_twl6030_match_tbl), dev);
+pdata = match ? match-data : dev-platform_data;
+
+if (!pdata)
+return -EINVAL;
+
+indio_dev = iio_device_alloc(sizeof(struct twl6030_gpadc_data));
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
RE: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
Hello Jonathan,
Thanks for the review.
Couple of things:
1) It looks from the driver that a lot of the channels are not measuring
voltages but rather temperature or currents etc. If so then their
types in the channel mask should be appropriately set. Also if some
of the channels are entirely internal test networks, what is the benefit
of exposing them to userspace as readable channels?
If channels are merely 'suggested' as being used for temperatures etc
then it is fine to keep them as voltages.
There are two kinds of channel for measuring temperature: die temperature
and those that measure temperature indirectly measuring voltage on resistive
load(NTC sensor).
die temperature is calculated by some formulas which convert ADC code to
temperature. This is not implemented in the driver.
Channels intended to measure temperature with NTC sensor have inbuilt current
sources. Voltage measured by this channels and specific NTC could be converted
to temperature.
This is the reason they chosen to be voltage channels.
As for test network I'll remove them from exposing to userspace.
[...]
+static int twl6030_gpadc_get_raw(struct twl6030_gpadc_data *gpadc,
+ int channel, int *res)
+{
+ u8 reg = gpadc-pdata-channel_to_reg(channel);
+ u8 val[2];
le16 val;
+ int ret;
+
ret = twl6030_gpadc_read(val, reg);
(note that (reg, val) ordering of parameters would be a more common choice)
+ ret = twl6030_gpadc_read(val, reg);
+ if (ret) {
+ dev_dbg(gpadc-dev, unable to read register 0x%X\n, reg);
+ return ret;
+ }
+
res = le16_to_cpup(val);
+ *res = (val[1] 8) | val[0];
+
+ return ret;
+}
+
You mean something like this:
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 ret;
ret = twl6030_gpadc_read(reg, (u8 *)val);
if (ret) {
dev_dbg(gpadc-dev, unable to read register 0x%X\n, reg);
return ret;
}
*res = le16_to_cpup(val);
return ret;
}
Note, that twl6030_gpadc_read() is just wrapper for twl_i2c_read() which takes
an array of num_bytes containing data to be read
I like the original implementation better then this(if I did it correctly)
Do you insist on this change?
[...]
+static int twl6030_gpadc_get_processed(struct twl6030_gpadc_data *gpadc,
+ int channel, int *val)
+{
+ int raw_code;
+ int corrected_code;
+ int channel_value;
+ int ret;
+
+
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 15/07/13 14:30, Kozaruk, Oleksandr wrote:
Hello Jonathan,
Thanks for the review.
Couple of things:
1) It looks from the driver that a lot of the channels are not measuring
voltages but rather temperature or currents etc. If so then their
types in the channel mask should be appropriately set. Also if some
of the channels are entirely internal test networks, what is the benefit
of exposing them to userspace as readable channels?
If channels are merely 'suggested' as being used for temperatures etc
then it is fine to keep them as voltages.
There are two kinds of channel for measuring temperature: die temperature
and those that measure temperature indirectly measuring voltage on resistive
load(NTC sensor).
die temperature is calculated by some formulas which convert ADC code to
temperature. This is not implemented in the driver.
Channels intended to measure temperature with NTC sensor have inbuilt current
sources. Voltage measured by this channels and specific NTC could be converted
to temperature.
This is the reason they chosen to be voltage channels.
As for test network I'll remove them from exposing to userspace.
[...]
+static int twl6030_gpadc_get_raw(struct twl6030_gpadc_data *gpadc,
+ int channel, int *res)
+{
+ u8 reg = gpadc-pdata-channel_to_reg(channel);
+ u8 val[2];
le16 val;
+ int ret;
+
ret = twl6030_gpadc_read(val, reg);
(note that (reg, val) ordering of parameters would be a more common choice)
+ ret = twl6030_gpadc_read(val, reg);
+ if (ret) {
+ dev_dbg(gpadc-dev, unable to read register 0x%X\n, reg);
+ return ret;
+ }
+
res = le16_to_cpup(val);
+ *res = (val[1] 8) | val[0];
+
+ return ret;
+}
+
You mean something like this:
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 ret;
ret = twl6030_gpadc_read(reg, (u8 *)val);
if (ret) {
dev_dbg(gpadc-dev, unable to read register 0x%X\n, reg);
return ret;
}
*res = le16_to_cpup(val);
return ret;
}
Note, that twl6030_gpadc_read() is just wrapper for twl_i2c_read() which takes
an array of num_bytes containing data to be read
I like the original implementation better then this(if I did it correctly)
Do you insist on this change?
[...]
+static int twl6030_gpadc_get_processed(struct twl6030_gpadc_data *gpadc,
+ int channel, int *val)
+{
+
[PATCH v3 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. The driver exports function returning converted value for requested channels. Based on the driver patched from Balaji TK, Graeme Gregory, Ambresh K, Girish S Ghongdemath. Signed-off-by: Balaji T K balaj...@ti.com Graeme Gregory g...@slimlogic.co.uk Signed-off-by: Oleksandr Kozaruk oleksandr.koza...@ti.com --- drivers/iio/adc/Kconfig |8 + drivers/iio/adc/Makefile|1 + drivers/iio/adc/twl6030-gpadc.c | 1019 +++ 3 files changed, 1028 insertions(+) create mode 100644 drivers/iio/adc/twl6030-gpadc.c diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index ab0767e6..87d699e 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -157,4 +157,12 @@ config VIPERBOARD_ADC Say yes here to access the ADC part of the Nano River Technologies Viperboard. +config TWL6030_GPADC + tristate TWL6030 GPADC (General Purpose A/D Convertor) Support + depends on TWL4030_CORE + default n + help + Say yes here if you want support for the TWL6030 General Purpose + A/D Convertor. + endmenu diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 0a825be..8b05633 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o +obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c new file mode 100644 index 000..6ceb789 --- /dev/null +++ b/drivers/iio/adc/twl6030-gpadc.c @@ -0,0 +1,1019 @@ +/* + * TWL6030 GPADC module driver + * + * Copyright (C) 2009-2013 Texas Instruments Inc. + * Nishant Kamat nska...@ti.com + * Balaji T K balaj...@ti.com + * Graeme Gregory g...@slimlogic.co.uk + * Girish S Ghongdemath giris...@ti.com + * Ambresh K ambr...@ti.com + * Oleksandr Kozaruk oleksandr.koza...@ti.com + * + * Based on twl4030-madc.c + * Copyright (C) 2008 Nokia Corporation + * Mikko Ylinen mikko.k.yli...@nokia.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. + * + * 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_NAMEtwl6030_gpadc + +#define TWL6030_GPADC_MAX_CHANNELS 17 +#define TWL6032_GPADC_MAX_CHANNELS 19 +/* Define this as the biggest of all chips using this driver */ +#define GPADC_MAX_CHANNELS TWL6032_GPADC_MAX_CHANNELS + +#define TWL6030_GPADC_CTRL 0x00/* 0x2e */ +#define TWL6030_GPADC_CTRL20x01/* 0x2f */ + +#define TWL6030_GPADC_CTRL_P1 0x05 +#define TWL6030_GPADC_CTRL_P2 0x06 + +#define TWL6032_GPADC_GPSELECT_ISB 0x07 +#define TWL6032_GPADC_CTRL_P1 0x08 + +#define TWL6032_GPADC_RTCH0_LSB0x09 +#define TWL6032_GPADC_RTCH0_MSB0x0a +#define TWL6032_GPADC_RTCH1_LSB0x0b +#define TWL6032_GPADC_RTCH1_MSB0x0c +#define TWL6032_GPADC_GPCH0_LSB0x0d +#define TWL6032_GPADC_GPCH0_MSB0x0e + +#define
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
On 07/12/2013 08:18 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. The driver exports function returning converted value for requested channels. Based on the driver patched from Balaji TK, Graeme Gregory, Ambresh K, Girish S Ghongdemath. Couple of things: 1) It looks from the driver that a lot of the channels are not measuring voltages but rather temperature or currents etc. If so then their types in the channel mask should be appropriately set. Also if some of the channels are entirely internal test networks, what is the benefit of exposing them to userspace as readable channels? If channels are merely 'suggested' as being used for temperatures etc then it is fine to keep them as voltages. 2) You have my sympathy when it comes to the way those trim values are packed into the registers. That is truely hideous ;) Various trivial little bits but all in all a nice clean driver. Thanks, Jonathan Signed-off-by: Balaji T K balaj...@ti.com Graeme Gregory g...@slimlogic.co.uk Signed-off-by: Oleksandr Kozaruk oleksandr.koza...@ti.com --- drivers/iio/adc/Kconfig |8 + drivers/iio/adc/Makefile|1 + drivers/iio/adc/twl6030-gpadc.c | 1019 +++ 3 files changed, 1028 insertions(+) create mode 100644 drivers/iio/adc/twl6030-gpadc.c diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index ab0767e6..87d699e 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -157,4 +157,12 @@ config VIPERBOARD_ADC Say yes here to access the ADC part of the Nano River Technologies Viperboard. +config TWL6030_GPADC + tristate TWL6030 GPADC (General Purpose A/D Convertor) Support + depends on TWL4030_CORE + default n + help + Say yes here if you want support for the TWL6030 General Purpose + A/D Convertor. Perhaps a little more detail for such a complex device? + endmenu diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 0a825be..8b05633 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o +obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c new file mode 100644 index 000..6ceb789 --- /dev/null +++ b/drivers/iio/adc/twl6030-gpadc.c @@ -0,0 +1,1019 @@ +/* + * TWL6030 GPADC module driver + * + * Copyright (C) 2009-2013 Texas Instruments Inc. + * Nishant Kamat nska...@ti.com + * Balaji T K balaj...@ti.com + * Graeme Gregory g...@slimlogic.co.uk + * Girish S Ghongdemath giris...@ti.com + * Ambresh K ambr...@ti.com + * Oleksandr Kozaruk oleksandr.koza...@ti.com + * + * Based on twl4030-madc.c + * Copyright (C) 2008 Nokia Corporation + * Mikko Ylinen mikko.k.yli...@nokia.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. + * + * 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
Re: [PATCH v3 2/2] iio: twl6030-gpadc: TWL6030, TWL6032 GPADC driver
A couple of comments inline.
On 07/12/2013 09:18 AM, Oleksandr Kozaruk wrote:
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..87d699e 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -157,4 +157,12 @@ config VIPERBOARD_ADC
Say yes here to access the ADC part of the Nano River
Technologies Viperboard.
+config TWL6030_GPADC
+ tristate TWL6030 GPADC (General Purpose A/D Convertor) Support
+ depends on TWL4030_CORE
+ default n
+ help
+ Say yes here if you want support for the TWL6030 General Purpose
+ A/D Convertor.
+
Keep it in alphabetical order
endmenu
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..8b05633 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_MAX1363) += max1363.o
obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
+obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
Same here.
diff --git a/drivers/iio/adc/twl6030-gpadc.c b/drivers/iio/adc/twl6030-gpadc.c
new file mode 100644
index 000..6ceb789
--- /dev/null
+++ b/drivers/iio/adc/twl6030-gpadc.c
@@ -0,0 +1,1019 @@
[...]
+static u8 twl6032_channel_to_reg(int channel)
+{
+ return TWL6032_GPADC_GPCH0_LSB;
There is more than one channel, isn't there?
[...]
+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;
+
+ 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 */
+ wait_for_completion_interruptible_timeout(gpadc-irq_complete,
+ msecs_to_jiffies(5000));
wait_for_completion_interruptible_timeout() can return either a negative error
code if it was interrupted, 0 if it timed out, or a positive value if it was
successfully completed. You need to handle all three cases. Have a look at
other existing drivers to see how to do this properly.
+
+ 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;
+}
+}
+
[...]
+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 = 0;
+
+ match = of_match_device(of_match_ptr(of_twl6030_match_tbl), dev);
+ pdata = match ? match-data : dev-platform_data;
+
+ if (!pdata)
+ return -EINVAL;
+
+ indio_dev = iio_device_alloc(sizeof(struct twl6030_gpadc_data));
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;
+
+ 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;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq 0) {
+ dev_err(pdev-dev, failed to get irq\n);
+ goto err;
+ }
+
+ ret = devm_request_threaded_irq(dev, irq, NULL,
+ twl6030_gpadc_irq_handler,
+ IRQF_ONESHOT, twl6030_gpadc, gpadc);
You access memory in the interrupt handler which is freed before the interrupt
handler is freed.
+ if (ret) {
+ dev_dbg(pdev-dev, could not request irq\n);
+ goto err;
+ }
+
+ 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;
+
