* [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module
@ 2010-09-16 10:35 ` Keerthy
0 siblings, 0 replies; 12+ messages in thread
From: Keerthy @ 2010-09-16 10:23 UTC (permalink / raw)
To: linux-omap, lm-sensors; +Cc: j-keerthy, balajitk
MADC driver is added under hwmon drivers. This driver monitors the real time
conversion of analog signals like battery temperature,
battery type, battery level etc. User can also ask for the conversion on a
particular channel using the sysfs nodes.
Several people have contributed to this driver on the linux-omap list.
Signed-off-by: Keerthy <j-keerthy@ti.com>
---
drivers/hwmon/twl4030-madc.c | 584 ++++++++++++++++++++++++++++++++++++++
include/linux/i2c/twl4030-madc.h | 118 ++++++++
2 files changed, 702 insertions(+), 0 deletions(-)
create mode 100644 drivers/hwmon/twl4030-madc.c
create mode 100644 include/linux/i2c/twl4030-madc.h
diff --git a/drivers/hwmon/twl4030-madc.c b/drivers/hwmon/twl4030-madc.c
new file mode 100644
index 0000000..b96fccd
--- /dev/null
+++ b/drivers/hwmon/twl4030-madc.c
@@ -0,0 +1,584 @@
+/*
+ * 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/types.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/platform_device.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/i2c/twl.h>
+#include <linux/i2c/twl4030-madc.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+#include <linux/uaccess.h>
+
+
+struct twl4030_madc_data {
+ struct device *hwmon_dev;
+ struct mutex lock;
+ struct work_struct ws;
+ struct twl4030_madc_request requests[TWL4030_MADC_NUM_METHODS];
+ int imr;
+ int isr;
+};
+
+static struct twl4030_madc_data *the_madc;
+
+static ssize_t madc_read(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ struct twl4030_madc_request req;
+ int status;
+ unsigned long val;
+
+ req.channels = (1 << attr->index);
+ req.method = TWL4030_MADC_SW2;
+ req.func_cb = NULL;
+
+ val = twl4030_madc_conversion(&req);
+ if (likely(val >= 0))
+ val = req.rbuf[attr->index];
+ else
+ return val;
+ status = sprintf(buf, "%lu\n", val);
+ return status;
+}
+
+static
+const struct twl4030_madc_conversion_method twl4030_conversion_methods[] = {
+ [TWL4030_MADC_RT] = {
+ .sel = TWL4030_MADC_RTSELECT_LSB,
+ .avg = TWL4030_MADC_RTAVERAGE_LSB,
+ .rbase = TWL4030_MADC_RTCH0_LSB,
+ },
+ [TWL4030_MADC_SW1] = {
+ .sel = TWL4030_MADC_SW1SELECT_LSB,
+ .avg = TWL4030_MADC_SW1AVERAGE_LSB,
+ .rbase = TWL4030_MADC_GPCH0_LSB,
+ .ctrl = TWL4030_MADC_CTRL_SW1,
+ },
+ [TWL4030_MADC_SW2] = {
+ .sel = TWL4030_MADC_SW2SELECT_LSB,
+ .avg = TWL4030_MADC_SW2AVERAGE_LSB,
+ .rbase = TWL4030_MADC_GPCH0_LSB,
+ .ctrl = TWL4030_MADC_CTRL_SW2,
+ },
+};
+
+static int twl4030_madc_read(struct twl4030_madc_data *madc, u8 reg)
+{
+ int ret;
+ u8 val;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &val, reg);
+ if (ret) {
+ dev_dbg(madc->hwmon_dev, "unable to read register 0x%X\n", reg);
+ return ret;
+ }
+
+ return val;
+}
+
+static void twl4030_madc_write(struct twl4030_madc_data *madc, u8 reg, u8 val)
+{
+ int ret;
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, reg);
+ if (ret)
+ dev_err(madc->hwmon_dev,
+ "unable to write register 0x%X\n", reg);
+}
+
+static int twl4030_madc_channel_raw_read(struct twl4030_madc_data *madc, u8 reg)
+{
+ u8 msb, lsb;
+
+ /*
+ *For each ADC channel, we have MSB and LSB register pair. MSB address
+ *is always LSB address+1. reg parameter is the addr of LSB register
+ */
+ msb = twl4030_madc_read(madc, reg + 1);
+ lsb = twl4030_madc_read(madc, reg);
+
+ return (int)(((msb << 8) | lsb) >> 6);
+}
+
+static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
+ u8 reg_base, u16 channels, int *buf)
+{
+ int count = 0;
+ u8 reg, i;
+
+ if (unlikely(!buf))
+ return 0;
+
+ for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
+ if (channels & (1<<i)) {
+ reg = reg_base + 2*i;
+ buf[i] = twl4030_madc_channel_raw_read(madc, reg);
+ count++;
+ }
+ }
+ return count;
+}
+
+static void twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int id)
+{
+ u8 val;
+
+ val = twl4030_madc_read(madc, madc->imr);
+ val &= ~(1 << id);
+ twl4030_madc_write(madc, madc->imr, val);
+}
+
+static void twl4030_madc_disable_irq(struct twl4030_madc_data *madc, int id)
+{
+ u8 val;
+
+ val = twl4030_madc_read(madc, madc->imr);
+ val |= (1 << id);
+ twl4030_madc_write(madc, madc->imr, val);
+}
+
+static irqreturn_t twl4030_madc_irq_handler(int irq, void *_madc)
+{
+ struct twl4030_madc_data *madc = _madc;
+ u8 isr_val, imr_val;
+ int i;
+
+ isr_val = twl4030_madc_read(madc, madc->isr);
+ imr_val = twl4030_madc_read(madc, madc->imr);
+
+ isr_val &= ~imr_val;
+
+ for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
+
+ if (!(isr_val & (1<<i)))
+ continue;
+
+ twl4030_madc_disable_irq(madc, i);
+ madc->requests[i].result_pending = 1;
+ }
+
+ schedule_work(&madc->ws);
+
+ return IRQ_HANDLED;
+}
+
+static void twl4030_madc_work(struct work_struct *ws)
+{
+ const struct twl4030_madc_conversion_method *method;
+ struct twl4030_madc_data *madc;
+ struct twl4030_madc_request *r;
+ int len, i;
+
+ madc = container_of(ws, struct twl4030_madc_data, ws);
+ mutex_lock(&madc->lock);
+
+ for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
+
+ r = &madc->requests[i];
+
+ /* No pending results for this method, move to next one */
+ if (!r->result_pending)
+ continue;
+
+ method = &twl4030_conversion_methods[r->method];
+
+ /* Read results */
+ len = twl4030_madc_read_channels(madc, method->rbase,
+ r->channels, r->rbuf);
+
+ /* Return results to caller */
+ if (r->func_cb != NULL) {
+ r->func_cb(len, r->channels, r->rbuf);
+ r->func_cb = NULL;
+ }
+
+ /* Free request */
+ r->result_pending = 0;
+ r->active = 0;
+ }
+
+ mutex_unlock(&madc->lock);
+}
+
+static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
+ struct twl4030_madc_request *req)
+{
+ struct twl4030_madc_request *p;
+
+ p = &madc->requests[req->method];
+
+ memcpy(p, req, sizeof *req);
+
+ twl4030_madc_enable_irq(madc, req->method);
+
+ return 0;
+}
+
+static inline void twl4030_madc_start_conversion(struct twl4030_madc_data *madc,
+ int conv_method)
+{
+ const struct twl4030_madc_conversion_method *method;
+
+ method = &twl4030_conversion_methods[conv_method];
+
+ switch (conv_method) {
+ case TWL4030_MADC_SW1:
+ case TWL4030_MADC_SW2:
+ twl4030_madc_write(madc, method->ctrl, TWL4030_MADC_SW_START);
+ break;
+ case TWL4030_MADC_RT:
+ default:
+ break;
+ }
+}
+
+static int twl4030_madc_wait_conversion_ready(
+ struct twl4030_madc_data *madc,
+ unsigned int timeout_ms, u8 status_reg)
+{
+ unsigned long timeout;
+
+ timeout = jiffies + msecs_to_jiffies(timeout_ms);
+ do {
+ u8 reg;
+
+ reg = twl4030_madc_read(madc, status_reg);
+ if (!(reg & TWL4030_MADC_BUSY) && (reg & TWL4030_MADC_EOC_SW))
+ return 0;
+ } while (!time_after(jiffies, timeout));
+
+ return -EAGAIN;
+}
+
+int twl4030_madc_conversion(struct twl4030_madc_request *req)
+{
+ const struct twl4030_madc_conversion_method *method;
+ u8 ch_msb, ch_lsb;
+ int ret;
+
+ if (unlikely(!req))
+ return -EINVAL;
+
+ mutex_lock(&the_madc->lock);
+
+ /* Do we have a conversion request ongoing */
+ if (the_madc->requests[req->method].active) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ ch_msb = (req->channels >> 8) & 0xff;
+ ch_lsb = req->channels & 0xff;
+
+ method = &twl4030_conversion_methods[req->method];
+
+ /* Select channels to be converted */
+ twl4030_madc_write(the_madc, method->sel + 1, ch_msb);
+ twl4030_madc_write(the_madc, method->sel, ch_lsb);
+
+ /* Select averaging for all channels if do_avg is set */
+ if (req->do_avg) {
+ twl4030_madc_write(the_madc, method->avg + 1, ch_msb);
+ twl4030_madc_write(the_madc, method->avg, ch_lsb);
+ }
+
+ if ((req->type == TWL4030_MADC_IRQ_ONESHOT) && (req->func_cb != NULL)) {
+ twl4030_madc_set_irq(the_madc, req);
+ twl4030_madc_start_conversion(the_madc, req->method);
+ the_madc->requests[req->method].active = 1;
+ ret = 0;
+ goto out;
+ }
+
+ /* With RT method we should not be here anymore */
+ if (req->method == TWL4030_MADC_RT) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ twl4030_madc_start_conversion(the_madc, req->method);
+ the_madc->requests[req->method].active = 1;
+
+ /* Wait until conversion is ready (ctrl register returns EOC) */
+ ret = twl4030_madc_wait_conversion_ready(the_madc, 5, method->ctrl);
+ if (ret) {
+ dev_dbg(the_madc->hwmon_dev, "conversion timeout!\n");
+ the_madc->requests[req->method].active = 0;
+ goto out;
+ }
+
+ ret = twl4030_madc_read_channels(the_madc, method->rbase, req->channels,
+ req->rbuf);
+
+ the_madc->requests[req->method].active = 0;
+
+out:
+ mutex_unlock(&the_madc->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(twl4030_madc_conversion);
+
+static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
+ int chan, int on)
+{
+ int ret;
+ u8 regval;
+
+ /*
+ * Current generator is only available for ADCIN0 and ADCIN1. NB:
+ * ADCIN1 current generator only works when AC or VBUS is present
+ */
+ if (chan > 1)
+ return EINVAL;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
+ ®val, TWL4030_BCI_BCICTL1);
+ if (on)
+ regval |= (chan) ? TWL4030_BCI_ITHEN : TWL4030_BCI_TYPEN;
+ else
+ regval &= (chan) ? ~TWL4030_BCI_ITHEN : ~TWL4030_BCI_TYPEN;
+ ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
+ regval, TWL4030_BCI_BCICTL1);
+
+ return ret;
+}
+
+static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int on)
+{
+ u8 regval;
+
+ regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
+ if (on)
+ regval |= TWL4030_MADC_MADCON;
+ else
+ regval &= ~TWL4030_MADC_MADCON;
+ twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
+
+ return 0;
+}
+
+static long twl4030_madc_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct twl4030_madc_user_parms par;
+ int val, ret;
+
+ ret = copy_from_user(&par, (void __user *) arg, sizeof(par));
+ if (ret) {
+ dev_dbg(the_madc->hwmon_dev, "copy_from_user: %d\n", ret);
+ return -EACCES;
+ }
+
+ switch (cmd) {
+ case TWL4030_MADC_IOCX_ADC_RAW_READ: {
+ struct twl4030_madc_request req;
+ if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
+ return -EINVAL;
+
+ req.channels = (1 << par.channel);
+ req.do_avg = par.average;
+ req.method = TWL4030_MADC_SW1;
+ req.func_cb = NULL;
+
+ val = twl4030_madc_conversion(&req);
+ if (val <= 0) {
+ par.status = -1;
+ } else {
+ par.status = 0;
+ par.result = (u16)req.rbuf[par.channel];
+ }
+ break;
+ }
+ default:
+ return -EINVAL;
+ }
+
+ ret = copy_to_user((void __user *) arg, &par, sizeof(par));
+ if (ret) {
+ dev_dbg(the_madc->hwmon_dev, "copy_to_user: %d\n", ret);
+ return -EACCES;
+ }
+
+ return 0;
+}
+
+static const struct file_operations twl4030_madc_fileops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = twl4030_madc_ioctl
+};
+
+static struct miscdevice twl4030_madc_device = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "twl4030-madc",
+ .fops = &twl4030_madc_fileops
+};
+
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO|S_IWUSR , madc_read, NULL, 0);
+static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO|S_IWUSR , madc_read, NULL, 1);
+static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO|S_IWUSR , madc_read, NULL, 2);
+static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO|S_IWUSR , madc_read, NULL, 3);
+static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO|S_IWUSR , madc_read, NULL, 4);
+static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO|S_IWUSR , madc_read, NULL, 5);
+static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO|S_IWUSR , madc_read, NULL, 6);
+static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO|S_IWUSR , madc_read, NULL, 7);
+static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO|S_IWUSR , madc_read, NULL, 8);
+static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO|S_IWUSR , madc_read, NULL, 9);
+static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO|S_IWUSR , madc_read, NULL, 10);
+static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO|S_IWUSR , madc_read, NULL, 11);
+static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO|S_IWUSR , madc_read, NULL, 12);
+static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO|S_IWUSR , madc_read, NULL, 13);
+static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO|S_IWUSR , madc_read, NULL, 14);
+static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO|S_IWUSR , madc_read, NULL, 15);
+
+static struct attribute *twl4030_madc_attributes[] = {
+ &sensor_dev_attr_in0_input.dev_attr.attr,
+ &sensor_dev_attr_in1_input.dev_attr.attr,
+ &sensor_dev_attr_in2_input.dev_attr.attr,
+ &sensor_dev_attr_in3_input.dev_attr.attr,
+ &sensor_dev_attr_in4_input.dev_attr.attr,
+ &sensor_dev_attr_in5_input.dev_attr.attr,
+ &sensor_dev_attr_in6_input.dev_attr.attr,
+ &sensor_dev_attr_in7_input.dev_attr.attr,
+ &sensor_dev_attr_in8_input.dev_attr.attr,
+ &sensor_dev_attr_in9_input.dev_attr.attr,
+ &sensor_dev_attr_in10_input.dev_attr.attr,
+ &sensor_dev_attr_in11_input.dev_attr.attr,
+ &sensor_dev_attr_in12_input.dev_attr.attr,
+ &sensor_dev_attr_in13_input.dev_attr.attr,
+ &sensor_dev_attr_in14_input.dev_attr.attr,
+ &sensor_dev_attr_in15_input.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group twl4030_madc_group = {
+ .attrs = twl4030_madc_attributes,
+};
+
+static int __init twl4030_madc_probe(struct platform_device *pdev)
+{
+ struct twl4030_madc_data *madc;
+ struct twl4030_madc_platform_data *pdata = pdev->dev.platform_data;
+ int ret;
+ int status;
+ u8 regval;
+
+ madc = kzalloc(sizeof *madc, GFP_KERNEL);
+ if (!madc)
+ return -ENOMEM;
+
+ if (!pdata) {
+ dev_dbg(&pdev->dev, "platform_data not available\n");
+ ret = -EINVAL;
+ goto err_pdata;
+ }
+ madc->hwmon_dev = &pdev->dev;
+ madc->imr = (pdata->irq_line == 1) ?
+ TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
+ madc->isr = (pdata->irq_line == 1) ?
+ TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
+ madc->hwmon_dev = hwmon_device_register(&pdev->dev);
+ twl4030_madc_set_power(madc, 1);
+ twl4030_madc_set_current_generator(madc, 0, 1);
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
+ ®val, TWL4030_BCI_BCICTL1);
+
+ regval |= TWL4030_BCI_MESBAT;
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
+ regval, TWL4030_BCI_BCICTL1);
+
+ ret = request_threaded_irq(platform_get_irq(pdev, 0), NULL,
+ twl4030_madc_irq_handler , IRQF_TRIGGER_FALLING |
+ IRQF_TRIGGER_RISING, "twl4030_madc", madc);
+ if (ret) {
+ dev_dbg(&pdev->dev, "could not request irq\n");
+ goto err_irq;
+ }
+
+ platform_set_drvdata(pdev, madc);
+ mutex_init(&madc->lock);
+ ret = sysfs_create_group(&pdev->dev.kobj, &twl4030_madc_group);
+ if (ret)
+ goto err_misc;
+
+ if (IS_ERR(madc->hwmon_dev)) {
+ dev_err(&pdev->dev, "hwmon_device_register failed.\n");
+ status = PTR_ERR(madc->hwmon_dev);
+ goto err_misc;
+ }
+ INIT_WORK(&madc->ws, twl4030_madc_work);
+ the_madc = madc;
+
+ return 0;
+
+err_irq:
+err_misc:
+err_pdata:
+ kfree(madc);
+
+ return ret;
+}
+
+static int __exit twl4030_madc_remove(struct platform_device *pdev)
+{
+ struct twl4030_madc_data *madc = platform_get_drvdata(pdev);
+
+ twl4030_madc_set_power(madc, 0);
+ twl4030_madc_set_current_generator(madc, 0, 0);
+ free_irq(platform_get_irq(pdev, 0), madc);
+ cancel_work_sync(&madc->ws);
+
+ return 0;
+}
+
+static struct platform_driver twl4030_madc_driver = {
+ .probe = twl4030_madc_probe,
+ .remove = __exit_p(twl4030_madc_remove),
+ .driver = {
+ .name = "twl4030_madc",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init twl4030_madc_init(void)
+{
+ return platform_driver_register(&twl4030_madc_driver);
+}
+module_init(twl4030_madc_init);
+
+static void __exit twl4030_madc_exit(void)
+{
+ platform_driver_unregister(&twl4030_madc_driver);
+}
+module_exit(twl4030_madc_exit);
+
+MODULE_DESCRIPTION("twl4030 ADC driver");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/i2c/twl4030-madc.h b/include/linux/i2c/twl4030-madc.h
new file mode 100644
index 0000000..1e46ff1
--- /dev/null
+++ b/include/linux/i2c/twl4030-madc.h
@@ -0,0 +1,118 @@
+/*
+ * 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
+ *
+ */
+
+#ifndef _TWL4030_MADC_H
+#define _TWL4030_MADC_H
+
+struct twl4030_madc_conversion_method {
+ u8 sel;
+ u8 avg;
+ u8 rbase;
+ u8 ctrl;
+};
+
+#define TWL4030_MADC_MAX_CHANNELS 16
+
+struct twl4030_madc_request {
+ u16 channels;
+ u16 do_avg;
+ u16 method;
+ u16 type;
+ bool active;
+ bool result_pending;
+ int rbuf[TWL4030_MADC_MAX_CHANNELS];
+ void (*func_cb)(int len, int channels, int *buf);
+};
+
+enum conversion_methods {
+ TWL4030_MADC_RT,
+ TWL4030_MADC_SW1,
+ TWL4030_MADC_SW2,
+ TWL4030_MADC_NUM_METHODS
+};
+
+enum sample_type {
+ TWL4030_MADC_WAIT,
+ TWL4030_MADC_IRQ_ONESHOT,
+ TWL4030_MADC_IRQ_REARM
+};
+
+#define TWL4030_MADC_CTRL1 0x00
+#define TWL4030_MADC_CTRL2 0x01
+
+#define TWL4030_MADC_RTSELECT_LSB 0x02
+#define TWL4030_MADC_SW1SELECT_LSB 0x06
+#define TWL4030_MADC_SW2SELECT_LSB 0x0A
+
+#define TWL4030_MADC_RTAVERAGE_LSB 0x04
+#define TWL4030_MADC_SW1AVERAGE_LSB 0x08
+#define TWL4030_MADC_SW2AVERAGE_LSB 0x0C
+
+#define TWL4030_MADC_CTRL_SW1 0x12
+#define TWL4030_MADC_CTRL_SW2 0x13
+
+#define TWL4030_MADC_RTCH0_LSB 0x17
+#define TWL4030_MADC_GPCH0_LSB 0x37
+
+#define TWL4030_MADC_MADCON (1<<0) /* MADC power on */
+#define TWL4030_MADC_BUSY (1<<0) /* MADC busy */
+#define TWL4030_MADC_EOC_SW (1<<1) /* MADC conversion completion */
+#define TWL4030_MADC_SW_START (1<<5) /* MADC SWx start conversion */
+
+#define TWL4030_MADC_ADCIN0 (1<<0)
+#define TWL4030_MADC_ADCIN1 (1<<1)
+#define TWL4030_MADC_ADCIN2 (1<<2)
+#define TWL4030_MADC_ADCIN3 (1<<3)
+#define TWL4030_MADC_ADCIN4 (1<<4)
+#define TWL4030_MADC_ADCIN5 (1<<5)
+#define TWL4030_MADC_ADCIN6 (1<<6)
+#define TWL4030_MADC_ADCIN7 (1<<7)
+#define TWL4030_MADC_ADCIN8 (1<<8)
+#define TWL4030_MADC_ADCIN9 (1<<9)
+#define TWL4030_MADC_ADCIN10 (1<<10)
+#define TWL4030_MADC_ADCIN11 (1<<11)
+#define TWL4030_MADC_ADCIN12 (1<<12)
+#define TWL4030_MADC_ADCIN13 (1<<13)
+#define TWL4030_MADC_ADCIN14 (1<<14)
+#define TWL4030_MADC_ADCIN15 (1<<15)
+
+/* Fixed channels */
+#define TWL4030_MADC_BTEMP TWL4030_MADC_ADCIN1
+#define TWL4030_MADC_VBUS TWL4030_MADC_ADCIN8
+#define TWL4030_MADC_VBKB TWL4030_MADC_ADCIN9
+#define TWL4030_MADC_ICHG TWL4030_MADC_ADCIN10
+#define TWL4030_MADC_VCHG TWL4030_MADC_ADCIN11
+#define TWL4030_MADC_VBAT TWL4030_MADC_ADCIN12
+
+#define TWL4030_BCI_BCICTL1 0x23
+#define TWL4030_BCI_MESBAT (1<<1)
+#define TWL4030_BCI_TYPEN (1<<4)
+#define TWL4030_BCI_ITHEN (1<<3)
+
+#define TWL4030_MADC_IOC_MAGIC '`'
+#define TWL4030_MADC_IOCX_ADC_RAW_READ _IO(TWL4030_MADC_IOC_MAGIC, 0)
+
+struct twl4030_madc_user_parms {
+ int channel;
+ int average;
+ int status;
+ u16 result;
+};
+
+int twl4030_madc_conversion(struct twl4030_madc_request *conv);
+
+#endif
--
1.7.0.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc
@ 2010-09-16 10:35 ` Keerthy
0 siblings, 0 replies; 12+ messages in thread
From: Keerthy @ 2010-09-16 10:35 UTC (permalink / raw)
To: linux-omap, lm-sensors; +Cc: j-keerthy, balajitk
MADC driver is added under hwmon drivers. This driver monitors the real time
conversion of analog signals like battery temperature,
battery type, battery level etc. User can also ask for the conversion on a
particular channel using the sysfs nodes.
Several people have contributed to this driver on the linux-omap list.
Signed-off-by: Keerthy <j-keerthy@ti.com>
---
drivers/hwmon/twl4030-madc.c | 584 ++++++++++++++++++++++++++++++++++++++
include/linux/i2c/twl4030-madc.h | 118 ++++++++
2 files changed, 702 insertions(+), 0 deletions(-)
create mode 100644 drivers/hwmon/twl4030-madc.c
create mode 100644 include/linux/i2c/twl4030-madc.h
diff --git a/drivers/hwmon/twl4030-madc.c b/drivers/hwmon/twl4030-madc.c
new file mode 100644
index 0000000..b96fccd
--- /dev/null
+++ b/drivers/hwmon/twl4030-madc.c
@@ -0,0 +1,584 @@
+/*
+ * 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/types.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/platform_device.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/i2c/twl.h>
+#include <linux/i2c/twl4030-madc.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+#include <linux/uaccess.h>
+
+
+struct twl4030_madc_data {
+ struct device *hwmon_dev;
+ struct mutex lock;
+ struct work_struct ws;
+ struct twl4030_madc_request requests[TWL4030_MADC_NUM_METHODS];
+ int imr;
+ int isr;
+};
+
+static struct twl4030_madc_data *the_madc;
+
+static ssize_t madc_read(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ struct twl4030_madc_request req;
+ int status;
+ unsigned long val;
+
+ req.channels = (1 << attr->index);
+ req.method = TWL4030_MADC_SW2;
+ req.func_cb = NULL;
+
+ val = twl4030_madc_conversion(&req);
+ if (likely(val >= 0))
+ val = req.rbuf[attr->index];
+ else
+ return val;
+ status = sprintf(buf, "%lu\n", val);
+ return status;
+}
+
+static
+const struct twl4030_madc_conversion_method twl4030_conversion_methods[] = {
+ [TWL4030_MADC_RT] = {
+ .sel = TWL4030_MADC_RTSELECT_LSB,
+ .avg = TWL4030_MADC_RTAVERAGE_LSB,
+ .rbase = TWL4030_MADC_RTCH0_LSB,
+ },
+ [TWL4030_MADC_SW1] = {
+ .sel = TWL4030_MADC_SW1SELECT_LSB,
+ .avg = TWL4030_MADC_SW1AVERAGE_LSB,
+ .rbase = TWL4030_MADC_GPCH0_LSB,
+ .ctrl = TWL4030_MADC_CTRL_SW1,
+ },
+ [TWL4030_MADC_SW2] = {
+ .sel = TWL4030_MADC_SW2SELECT_LSB,
+ .avg = TWL4030_MADC_SW2AVERAGE_LSB,
+ .rbase = TWL4030_MADC_GPCH0_LSB,
+ .ctrl = TWL4030_MADC_CTRL_SW2,
+ },
+};
+
+static int twl4030_madc_read(struct twl4030_madc_data *madc, u8 reg)
+{
+ int ret;
+ u8 val;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &val, reg);
+ if (ret) {
+ dev_dbg(madc->hwmon_dev, "unable to read register 0x%X\n", reg);
+ return ret;
+ }
+
+ return val;
+}
+
+static void twl4030_madc_write(struct twl4030_madc_data *madc, u8 reg, u8 val)
+{
+ int ret;
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, reg);
+ if (ret)
+ dev_err(madc->hwmon_dev,
+ "unable to write register 0x%X\n", reg);
+}
+
+static int twl4030_madc_channel_raw_read(struct twl4030_madc_data *madc, u8 reg)
+{
+ u8 msb, lsb;
+
+ /*
+ *For each ADC channel, we have MSB and LSB register pair. MSB address
+ *is always LSB address+1. reg parameter is the addr of LSB register
+ */
+ msb = twl4030_madc_read(madc, reg + 1);
+ lsb = twl4030_madc_read(madc, reg);
+
+ return (int)(((msb << 8) | lsb) >> 6);
+}
+
+static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
+ u8 reg_base, u16 channels, int *buf)
+{
+ int count = 0;
+ u8 reg, i;
+
+ if (unlikely(!buf))
+ return 0;
+
+ for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
+ if (channels & (1<<i)) {
+ reg = reg_base + 2*i;
+ buf[i] = twl4030_madc_channel_raw_read(madc, reg);
+ count++;
+ }
+ }
+ return count;
+}
+
+static void twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int id)
+{
+ u8 val;
+
+ val = twl4030_madc_read(madc, madc->imr);
+ val &= ~(1 << id);
+ twl4030_madc_write(madc, madc->imr, val);
+}
+
+static void twl4030_madc_disable_irq(struct twl4030_madc_data *madc, int id)
+{
+ u8 val;
+
+ val = twl4030_madc_read(madc, madc->imr);
+ val |= (1 << id);
+ twl4030_madc_write(madc, madc->imr, val);
+}
+
+static irqreturn_t twl4030_madc_irq_handler(int irq, void *_madc)
+{
+ struct twl4030_madc_data *madc = _madc;
+ u8 isr_val, imr_val;
+ int i;
+
+ isr_val = twl4030_madc_read(madc, madc->isr);
+ imr_val = twl4030_madc_read(madc, madc->imr);
+
+ isr_val &= ~imr_val;
+
+ for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
+
+ if (!(isr_val & (1<<i)))
+ continue;
+
+ twl4030_madc_disable_irq(madc, i);
+ madc->requests[i].result_pending = 1;
+ }
+
+ schedule_work(&madc->ws);
+
+ return IRQ_HANDLED;
+}
+
+static void twl4030_madc_work(struct work_struct *ws)
+{
+ const struct twl4030_madc_conversion_method *method;
+ struct twl4030_madc_data *madc;
+ struct twl4030_madc_request *r;
+ int len, i;
+
+ madc = container_of(ws, struct twl4030_madc_data, ws);
+ mutex_lock(&madc->lock);
+
+ for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
+
+ r = &madc->requests[i];
+
+ /* No pending results for this method, move to next one */
+ if (!r->result_pending)
+ continue;
+
+ method = &twl4030_conversion_methods[r->method];
+
+ /* Read results */
+ len = twl4030_madc_read_channels(madc, method->rbase,
+ r->channels, r->rbuf);
+
+ /* Return results to caller */
+ if (r->func_cb != NULL) {
+ r->func_cb(len, r->channels, r->rbuf);
+ r->func_cb = NULL;
+ }
+
+ /* Free request */
+ r->result_pending = 0;
+ r->active = 0;
+ }
+
+ mutex_unlock(&madc->lock);
+}
+
+static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
+ struct twl4030_madc_request *req)
+{
+ struct twl4030_madc_request *p;
+
+ p = &madc->requests[req->method];
+
+ memcpy(p, req, sizeof *req);
+
+ twl4030_madc_enable_irq(madc, req->method);
+
+ return 0;
+}
+
+static inline void twl4030_madc_start_conversion(struct twl4030_madc_data *madc,
+ int conv_method)
+{
+ const struct twl4030_madc_conversion_method *method;
+
+ method = &twl4030_conversion_methods[conv_method];
+
+ switch (conv_method) {
+ case TWL4030_MADC_SW1:
+ case TWL4030_MADC_SW2:
+ twl4030_madc_write(madc, method->ctrl, TWL4030_MADC_SW_START);
+ break;
+ case TWL4030_MADC_RT:
+ default:
+ break;
+ }
+}
+
+static int twl4030_madc_wait_conversion_ready(
+ struct twl4030_madc_data *madc,
+ unsigned int timeout_ms, u8 status_reg)
+{
+ unsigned long timeout;
+
+ timeout = jiffies + msecs_to_jiffies(timeout_ms);
+ do {
+ u8 reg;
+
+ reg = twl4030_madc_read(madc, status_reg);
+ if (!(reg & TWL4030_MADC_BUSY) && (reg & TWL4030_MADC_EOC_SW))
+ return 0;
+ } while (!time_after(jiffies, timeout));
+
+ return -EAGAIN;
+}
+
+int twl4030_madc_conversion(struct twl4030_madc_request *req)
+{
+ const struct twl4030_madc_conversion_method *method;
+ u8 ch_msb, ch_lsb;
+ int ret;
+
+ if (unlikely(!req))
+ return -EINVAL;
+
+ mutex_lock(&the_madc->lock);
+
+ /* Do we have a conversion request ongoing */
+ if (the_madc->requests[req->method].active) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ ch_msb = (req->channels >> 8) & 0xff;
+ ch_lsb = req->channels & 0xff;
+
+ method = &twl4030_conversion_methods[req->method];
+
+ /* Select channels to be converted */
+ twl4030_madc_write(the_madc, method->sel + 1, ch_msb);
+ twl4030_madc_write(the_madc, method->sel, ch_lsb);
+
+ /* Select averaging for all channels if do_avg is set */
+ if (req->do_avg) {
+ twl4030_madc_write(the_madc, method->avg + 1, ch_msb);
+ twl4030_madc_write(the_madc, method->avg, ch_lsb);
+ }
+
+ if ((req->type = TWL4030_MADC_IRQ_ONESHOT) && (req->func_cb != NULL)) {
+ twl4030_madc_set_irq(the_madc, req);
+ twl4030_madc_start_conversion(the_madc, req->method);
+ the_madc->requests[req->method].active = 1;
+ ret = 0;
+ goto out;
+ }
+
+ /* With RT method we should not be here anymore */
+ if (req->method = TWL4030_MADC_RT) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ twl4030_madc_start_conversion(the_madc, req->method);
+ the_madc->requests[req->method].active = 1;
+
+ /* Wait until conversion is ready (ctrl register returns EOC) */
+ ret = twl4030_madc_wait_conversion_ready(the_madc, 5, method->ctrl);
+ if (ret) {
+ dev_dbg(the_madc->hwmon_dev, "conversion timeout!\n");
+ the_madc->requests[req->method].active = 0;
+ goto out;
+ }
+
+ ret = twl4030_madc_read_channels(the_madc, method->rbase, req->channels,
+ req->rbuf);
+
+ the_madc->requests[req->method].active = 0;
+
+out:
+ mutex_unlock(&the_madc->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(twl4030_madc_conversion);
+
+static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
+ int chan, int on)
+{
+ int ret;
+ u8 regval;
+
+ /*
+ * Current generator is only available for ADCIN0 and ADCIN1. NB:
+ * ADCIN1 current generator only works when AC or VBUS is present
+ */
+ if (chan > 1)
+ return EINVAL;
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
+ ®val, TWL4030_BCI_BCICTL1);
+ if (on)
+ regval |= (chan) ? TWL4030_BCI_ITHEN : TWL4030_BCI_TYPEN;
+ else
+ regval &= (chan) ? ~TWL4030_BCI_ITHEN : ~TWL4030_BCI_TYPEN;
+ ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
+ regval, TWL4030_BCI_BCICTL1);
+
+ return ret;
+}
+
+static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int on)
+{
+ u8 regval;
+
+ regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
+ if (on)
+ regval |= TWL4030_MADC_MADCON;
+ else
+ regval &= ~TWL4030_MADC_MADCON;
+ twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
+
+ return 0;
+}
+
+static long twl4030_madc_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct twl4030_madc_user_parms par;
+ int val, ret;
+
+ ret = copy_from_user(&par, (void __user *) arg, sizeof(par));
+ if (ret) {
+ dev_dbg(the_madc->hwmon_dev, "copy_from_user: %d\n", ret);
+ return -EACCES;
+ }
+
+ switch (cmd) {
+ case TWL4030_MADC_IOCX_ADC_RAW_READ: {
+ struct twl4030_madc_request req;
+ if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
+ return -EINVAL;
+
+ req.channels = (1 << par.channel);
+ req.do_avg = par.average;
+ req.method = TWL4030_MADC_SW1;
+ req.func_cb = NULL;
+
+ val = twl4030_madc_conversion(&req);
+ if (val <= 0) {
+ par.status = -1;
+ } else {
+ par.status = 0;
+ par.result = (u16)req.rbuf[par.channel];
+ }
+ break;
+ }
+ default:
+ return -EINVAL;
+ }
+
+ ret = copy_to_user((void __user *) arg, &par, sizeof(par));
+ if (ret) {
+ dev_dbg(the_madc->hwmon_dev, "copy_to_user: %d\n", ret);
+ return -EACCES;
+ }
+
+ return 0;
+}
+
+static const struct file_operations twl4030_madc_fileops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = twl4030_madc_ioctl
+};
+
+static struct miscdevice twl4030_madc_device = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "twl4030-madc",
+ .fops = &twl4030_madc_fileops
+};
+
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO|S_IWUSR , madc_read, NULL, 0);
+static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO|S_IWUSR , madc_read, NULL, 1);
+static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO|S_IWUSR , madc_read, NULL, 2);
+static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO|S_IWUSR , madc_read, NULL, 3);
+static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO|S_IWUSR , madc_read, NULL, 4);
+static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO|S_IWUSR , madc_read, NULL, 5);
+static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO|S_IWUSR , madc_read, NULL, 6);
+static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO|S_IWUSR , madc_read, NULL, 7);
+static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO|S_IWUSR , madc_read, NULL, 8);
+static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO|S_IWUSR , madc_read, NULL, 9);
+static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO|S_IWUSR , madc_read, NULL, 10);
+static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO|S_IWUSR , madc_read, NULL, 11);
+static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO|S_IWUSR , madc_read, NULL, 12);
+static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO|S_IWUSR , madc_read, NULL, 13);
+static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO|S_IWUSR , madc_read, NULL, 14);
+static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO|S_IWUSR , madc_read, NULL, 15);
+
+static struct attribute *twl4030_madc_attributes[] = {
+ &sensor_dev_attr_in0_input.dev_attr.attr,
+ &sensor_dev_attr_in1_input.dev_attr.attr,
+ &sensor_dev_attr_in2_input.dev_attr.attr,
+ &sensor_dev_attr_in3_input.dev_attr.attr,
+ &sensor_dev_attr_in4_input.dev_attr.attr,
+ &sensor_dev_attr_in5_input.dev_attr.attr,
+ &sensor_dev_attr_in6_input.dev_attr.attr,
+ &sensor_dev_attr_in7_input.dev_attr.attr,
+ &sensor_dev_attr_in8_input.dev_attr.attr,
+ &sensor_dev_attr_in9_input.dev_attr.attr,
+ &sensor_dev_attr_in10_input.dev_attr.attr,
+ &sensor_dev_attr_in11_input.dev_attr.attr,
+ &sensor_dev_attr_in12_input.dev_attr.attr,
+ &sensor_dev_attr_in13_input.dev_attr.attr,
+ &sensor_dev_attr_in14_input.dev_attr.attr,
+ &sensor_dev_attr_in15_input.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group twl4030_madc_group = {
+ .attrs = twl4030_madc_attributes,
+};
+
+static int __init twl4030_madc_probe(struct platform_device *pdev)
+{
+ struct twl4030_madc_data *madc;
+ struct twl4030_madc_platform_data *pdata = pdev->dev.platform_data;
+ int ret;
+ int status;
+ u8 regval;
+
+ madc = kzalloc(sizeof *madc, GFP_KERNEL);
+ if (!madc)
+ return -ENOMEM;
+
+ if (!pdata) {
+ dev_dbg(&pdev->dev, "platform_data not available\n");
+ ret = -EINVAL;
+ goto err_pdata;
+ }
+ madc->hwmon_dev = &pdev->dev;
+ madc->imr = (pdata->irq_line = 1) ?
+ TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
+ madc->isr = (pdata->irq_line = 1) ?
+ TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
+ madc->hwmon_dev = hwmon_device_register(&pdev->dev);
+ twl4030_madc_set_power(madc, 1);
+ twl4030_madc_set_current_generator(madc, 0, 1);
+
+ ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
+ ®val, TWL4030_BCI_BCICTL1);
+
+ regval |= TWL4030_BCI_MESBAT;
+
+ ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
+ regval, TWL4030_BCI_BCICTL1);
+
+ ret = request_threaded_irq(platform_get_irq(pdev, 0), NULL,
+ twl4030_madc_irq_handler , IRQF_TRIGGER_FALLING |
+ IRQF_TRIGGER_RISING, "twl4030_madc", madc);
+ if (ret) {
+ dev_dbg(&pdev->dev, "could not request irq\n");
+ goto err_irq;
+ }
+
+ platform_set_drvdata(pdev, madc);
+ mutex_init(&madc->lock);
+ ret = sysfs_create_group(&pdev->dev.kobj, &twl4030_madc_group);
+ if (ret)
+ goto err_misc;
+
+ if (IS_ERR(madc->hwmon_dev)) {
+ dev_err(&pdev->dev, "hwmon_device_register failed.\n");
+ status = PTR_ERR(madc->hwmon_dev);
+ goto err_misc;
+ }
+ INIT_WORK(&madc->ws, twl4030_madc_work);
+ the_madc = madc;
+
+ return 0;
+
+err_irq:
+err_misc:
+err_pdata:
+ kfree(madc);
+
+ return ret;
+}
+
+static int __exit twl4030_madc_remove(struct platform_device *pdev)
+{
+ struct twl4030_madc_data *madc = platform_get_drvdata(pdev);
+
+ twl4030_madc_set_power(madc, 0);
+ twl4030_madc_set_current_generator(madc, 0, 0);
+ free_irq(platform_get_irq(pdev, 0), madc);
+ cancel_work_sync(&madc->ws);
+
+ return 0;
+}
+
+static struct platform_driver twl4030_madc_driver = {
+ .probe = twl4030_madc_probe,
+ .remove = __exit_p(twl4030_madc_remove),
+ .driver = {
+ .name = "twl4030_madc",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init twl4030_madc_init(void)
+{
+ return platform_driver_register(&twl4030_madc_driver);
+}
+module_init(twl4030_madc_init);
+
+static void __exit twl4030_madc_exit(void)
+{
+ platform_driver_unregister(&twl4030_madc_driver);
+}
+module_exit(twl4030_madc_exit);
+
+MODULE_DESCRIPTION("twl4030 ADC driver");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/i2c/twl4030-madc.h b/include/linux/i2c/twl4030-madc.h
new file mode 100644
index 0000000..1e46ff1
--- /dev/null
+++ b/include/linux/i2c/twl4030-madc.h
@@ -0,0 +1,118 @@
+/*
+ * 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
+ *
+ */
+
+#ifndef _TWL4030_MADC_H
+#define _TWL4030_MADC_H
+
+struct twl4030_madc_conversion_method {
+ u8 sel;
+ u8 avg;
+ u8 rbase;
+ u8 ctrl;
+};
+
+#define TWL4030_MADC_MAX_CHANNELS 16
+
+struct twl4030_madc_request {
+ u16 channels;
+ u16 do_avg;
+ u16 method;
+ u16 type;
+ bool active;
+ bool result_pending;
+ int rbuf[TWL4030_MADC_MAX_CHANNELS];
+ void (*func_cb)(int len, int channels, int *buf);
+};
+
+enum conversion_methods {
+ TWL4030_MADC_RT,
+ TWL4030_MADC_SW1,
+ TWL4030_MADC_SW2,
+ TWL4030_MADC_NUM_METHODS
+};
+
+enum sample_type {
+ TWL4030_MADC_WAIT,
+ TWL4030_MADC_IRQ_ONESHOT,
+ TWL4030_MADC_IRQ_REARM
+};
+
+#define TWL4030_MADC_CTRL1 0x00
+#define TWL4030_MADC_CTRL2 0x01
+
+#define TWL4030_MADC_RTSELECT_LSB 0x02
+#define TWL4030_MADC_SW1SELECT_LSB 0x06
+#define TWL4030_MADC_SW2SELECT_LSB 0x0A
+
+#define TWL4030_MADC_RTAVERAGE_LSB 0x04
+#define TWL4030_MADC_SW1AVERAGE_LSB 0x08
+#define TWL4030_MADC_SW2AVERAGE_LSB 0x0C
+
+#define TWL4030_MADC_CTRL_SW1 0x12
+#define TWL4030_MADC_CTRL_SW2 0x13
+
+#define TWL4030_MADC_RTCH0_LSB 0x17
+#define TWL4030_MADC_GPCH0_LSB 0x37
+
+#define TWL4030_MADC_MADCON (1<<0) /* MADC power on */
+#define TWL4030_MADC_BUSY (1<<0) /* MADC busy */
+#define TWL4030_MADC_EOC_SW (1<<1) /* MADC conversion completion */
+#define TWL4030_MADC_SW_START (1<<5) /* MADC SWx start conversion */
+
+#define TWL4030_MADC_ADCIN0 (1<<0)
+#define TWL4030_MADC_ADCIN1 (1<<1)
+#define TWL4030_MADC_ADCIN2 (1<<2)
+#define TWL4030_MADC_ADCIN3 (1<<3)
+#define TWL4030_MADC_ADCIN4 (1<<4)
+#define TWL4030_MADC_ADCIN5 (1<<5)
+#define TWL4030_MADC_ADCIN6 (1<<6)
+#define TWL4030_MADC_ADCIN7 (1<<7)
+#define TWL4030_MADC_ADCIN8 (1<<8)
+#define TWL4030_MADC_ADCIN9 (1<<9)
+#define TWL4030_MADC_ADCIN10 (1<<10)
+#define TWL4030_MADC_ADCIN11 (1<<11)
+#define TWL4030_MADC_ADCIN12 (1<<12)
+#define TWL4030_MADC_ADCIN13 (1<<13)
+#define TWL4030_MADC_ADCIN14 (1<<14)
+#define TWL4030_MADC_ADCIN15 (1<<15)
+
+/* Fixed channels */
+#define TWL4030_MADC_BTEMP TWL4030_MADC_ADCIN1
+#define TWL4030_MADC_VBUS TWL4030_MADC_ADCIN8
+#define TWL4030_MADC_VBKB TWL4030_MADC_ADCIN9
+#define TWL4030_MADC_ICHG TWL4030_MADC_ADCIN10
+#define TWL4030_MADC_VCHG TWL4030_MADC_ADCIN11
+#define TWL4030_MADC_VBAT TWL4030_MADC_ADCIN12
+
+#define TWL4030_BCI_BCICTL1 0x23
+#define TWL4030_BCI_MESBAT (1<<1)
+#define TWL4030_BCI_TYPEN (1<<4)
+#define TWL4030_BCI_ITHEN (1<<3)
+
+#define TWL4030_MADC_IOC_MAGIC '`'
+#define TWL4030_MADC_IOCX_ADC_RAW_READ _IO(TWL4030_MADC_IOC_MAGIC, 0)
+
+struct twl4030_madc_user_parms {
+ int channel;
+ int average;
+ int status;
+ u16 result;
+};
+
+int twl4030_madc_conversion(struct twl4030_madc_request *conv);
+
+#endif
--
1.7.0.4
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module
2010-09-16 10:35 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc Keerthy
@ 2010-09-16 15:17 ` Guenter Roeck
-1 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2010-09-16 15:17 UTC (permalink / raw)
To: Keerthy; +Cc: linux-omap, lm-sensors, balajitk
On Thu, Sep 16, 2010 at 06:23:24AM -0400, Keerthy wrote:
> MADC driver is added under hwmon drivers. This driver monitors the real time
> conversion of analog signals like battery temperature,
> battery type, battery level etc. User can also ask for the conversion on a
> particular channel using the sysfs nodes.
>
Number of comments inline. This is not a complete review; I think there
is some cleanup to be done before that is possible.
Please run the driver through Lindent. While I understand that people prefer
their personal touch, it makes it easier to have a single coding style in the kernel.
I commented on this a couple of times below - the driver mixes generic ADC
reading functions with hwmon functionality. Generic ADC reading functionality
should be moved into another driver, possibly to mfd.
> Several people have contributed to this driver on the linux-omap list.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
> drivers/hwmon/twl4030-madc.c | 584 ++++++++++++++++++++++++++++++++++++++
> include/linux/i2c/twl4030-madc.h | 118 ++++++++
> 2 files changed, 702 insertions(+), 0 deletions(-)
> create mode 100644 drivers/hwmon/twl4030-madc.c
> create mode 100644 include/linux/i2c/twl4030-madc.h
>
> diff --git a/drivers/hwmon/twl4030-madc.c b/drivers/hwmon/twl4030-madc.c
> new file mode 100644
> index 0000000..b96fccd
> --- /dev/null
> +++ b/drivers/hwmon/twl4030-madc.c
> @@ -0,0 +1,584 @@
> +/*
It is customary to add a brief description as well as the author here.
> + * 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/types.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
> +#include <linux/slab.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/i2c/twl4030-madc.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include <linux/uaccess.h>
> +
> +
Extra blank line. Lindent will remove it.
> +struct twl4030_madc_data {
> + struct device *hwmon_dev;
> + struct mutex lock;
> + struct work_struct ws;
> + struct twl4030_madc_request requests[TWL4030_MADC_NUM_METHODS];
> + int imr;
> + int isr;
> +};
> +
> +static struct twl4030_madc_data *the_madc;
> +
This variable should not exist. See other comments later.
> +static ssize_t madc_read(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + struct twl4030_madc_request req;
> + int status;
> + unsigned long val;
> +
> + req.channels = (1 << attr->index);
> + req.method = TWL4030_MADC_SW2;
> + req.func_cb = NULL;
> +
> + val = twl4030_madc_conversion(&req);
> + if (likely(val >= 0))
> + val = req.rbuf[attr->index];
> + else
> + return val;
> + status = sprintf(buf, "%lu\n", val);
> + return status;
> +}
> +
> +static
> +const struct twl4030_madc_conversion_method twl4030_conversion_methods[] = {
> + [TWL4030_MADC_RT] = {
> + .sel = TWL4030_MADC_RTSELECT_LSB,
> + .avg = TWL4030_MADC_RTAVERAGE_LSB,
> + .rbase = TWL4030_MADC_RTCH0_LSB,
> + },
> + [TWL4030_MADC_SW1] = {
> + .sel = TWL4030_MADC_SW1SELECT_LSB,
> + .avg = TWL4030_MADC_SW1AVERAGE_LSB,
> + .rbase = TWL4030_MADC_GPCH0_LSB,
> + .ctrl = TWL4030_MADC_CTRL_SW1,
> + },
> + [TWL4030_MADC_SW2] = {
> + .sel = TWL4030_MADC_SW2SELECT_LSB,
> + .avg = TWL4030_MADC_SW2AVERAGE_LSB,
> + .rbase = TWL4030_MADC_GPCH0_LSB,
> + .ctrl = TWL4030_MADC_CTRL_SW2,
> + },
> +};
> +
> +static int twl4030_madc_read(struct twl4030_madc_data *madc, u8 reg)
> +{
> + int ret;
> + u8 val;
> +
> + ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &val, reg);
Structural comment. If there is a i2c master, why don't you have an I2C
master driver instead of calling a platform dependent i2c function ?
I see this function called from all over the place, so maybe there is a
reason. Just looks odd, though.
> + if (ret) {
> + dev_dbg(madc->hwmon_dev, "unable to read register 0x%X\n", reg);
> + return ret;
> + }
> +
> + return val;
> +}
> +
> +static void twl4030_madc_write(struct twl4030_madc_data *madc, u8 reg, u8 val)
> +{
> + int ret;
> +
> + ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, reg);
> + if (ret)
> + dev_err(madc->hwmon_dev,
> + "unable to write register 0x%X\n", reg);
> +}
> +
> +static int twl4030_madc_channel_raw_read(struct twl4030_madc_data *madc, u8 reg)
> +{
> + u8 msb, lsb;
> +
> + /*
> + *For each ADC channel, we have MSB and LSB register pair. MSB address
> + *is always LSB address+1. reg parameter is the addr of LSB register
> + */
> + msb = twl4030_madc_read(madc, reg + 1);
> + lsb = twl4030_madc_read(madc, reg);
> +
> + return (int)(((msb << 8) | lsb) >> 6);
> +}
> +
> +static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
> + u8 reg_base, u16 channels, int *buf)
> +{
> + int count = 0;
> + u8 reg, i;
> +
> + if (unlikely(!buf))
> + return 0;
> +
> + for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
> + if (channels & (1<<i)) {
> + reg = reg_base + 2*i;
> + buf[i] = twl4030_madc_channel_raw_read(madc, reg);
> + count++;
> + }
> + }
> + return count;
> +}
> +
> +static void twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int id)
> +{
> + u8 val;
> +
> + val = twl4030_madc_read(madc, madc->imr);
> + val &= ~(1 << id);
> + twl4030_madc_write(madc, madc->imr, val);
> +}
> +
> +static void twl4030_madc_disable_irq(struct twl4030_madc_data *madc, int id)
> +{
> + u8 val;
> +
> + val = twl4030_madc_read(madc, madc->imr);
> + val |= (1 << id);
> + twl4030_madc_write(madc, madc->imr, val);
> +}
> +
> +static irqreturn_t twl4030_madc_irq_handler(int irq, void *_madc)
> +{
> + struct twl4030_madc_data *madc = _madc;
> + u8 isr_val, imr_val;
> + int i;
> +
> + isr_val = twl4030_madc_read(madc, madc->isr);
> + imr_val = twl4030_madc_read(madc, madc->imr);
> +
> + isr_val &= ~imr_val;
> +
> + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> +
> + if (!(isr_val & (1<<i)))
> + continue;
> +
> + twl4030_madc_disable_irq(madc, i);
> + madc->requests[i].result_pending = 1;
> + }
> +
> + schedule_work(&madc->ws);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void twl4030_madc_work(struct work_struct *ws)
> +{
> + const struct twl4030_madc_conversion_method *method;
> + struct twl4030_madc_data *madc;
> + struct twl4030_madc_request *r;
> + int len, i;
> +
> + madc = container_of(ws, struct twl4030_madc_data, ws);
> + mutex_lock(&madc->lock);
> +
> + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> +
> + r = &madc->requests[i];
> +
> + /* No pending results for this method, move to next one */
> + if (!r->result_pending)
> + continue;
> +
> + method = &twl4030_conversion_methods[r->method];
> +
> + /* Read results */
> + len = twl4030_madc_read_channels(madc, method->rbase,
> + r->channels, r->rbuf);
> +
> + /* Return results to caller */
> + if (r->func_cb != NULL) {
> + r->func_cb(len, r->channels, r->rbuf);
> + r->func_cb = NULL;
> + }
> +
> + /* Free request */
> + r->result_pending = 0;
> + r->active = 0;
> + }
> +
> + mutex_unlock(&madc->lock);
> +}
> +
> +static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
> + struct twl4030_madc_request *req)
> +{
> + struct twl4030_madc_request *p;
> +
> + p = &madc->requests[req->method];
> +
> + memcpy(p, req, sizeof *req);
> +
> + twl4030_madc_enable_irq(madc, req->method);
> +
> + return 0;
> +}
> +
> +static inline void twl4030_madc_start_conversion(struct twl4030_madc_data *madc,
> + int conv_method)
> +{
> + const struct twl4030_madc_conversion_method *method;
> +
> + method = &twl4030_conversion_methods[conv_method];
> +
> + switch (conv_method) {
> + case TWL4030_MADC_SW1:
> + case TWL4030_MADC_SW2:
> + twl4030_madc_write(madc, method->ctrl, TWL4030_MADC_SW_START);
> + break;
> + case TWL4030_MADC_RT:
> + default:
> + break;
> + }
> +}
> +
> +static int twl4030_madc_wait_conversion_ready(
> + struct twl4030_madc_data *madc,
> + unsigned int timeout_ms, u8 status_reg)
> +{
> + unsigned long timeout;
> +
> + timeout = jiffies + msecs_to_jiffies(timeout_ms);
> + do {
> + u8 reg;
> +
> + reg = twl4030_madc_read(madc, status_reg);
> + if (!(reg & TWL4030_MADC_BUSY) && (reg & TWL4030_MADC_EOC_SW))
> + return 0;
> + } while (!time_after(jiffies, timeout));
> +
Does this have to be a hard wait ?
> + return -EAGAIN;
> +}
> +
> +int twl4030_madc_conversion(struct twl4030_madc_request *req)
> +{
> + const struct twl4030_madc_conversion_method *method;
> + u8 ch_msb, ch_lsb;
> + int ret;
> +
> + if (unlikely(!req))
> + return -EINVAL;
> +
> + mutex_lock(&the_madc->lock);
> +
> + /* Do we have a conversion request ongoing */
> + if (the_madc->requests[req->method].active) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + ch_msb = (req->channels >> 8) & 0xff;
> + ch_lsb = req->channels & 0xff;
> +
> + method = &twl4030_conversion_methods[req->method];
> +
> + /* Select channels to be converted */
> + twl4030_madc_write(the_madc, method->sel + 1, ch_msb);
> + twl4030_madc_write(the_madc, method->sel, ch_lsb);
> +
> + /* Select averaging for all channels if do_avg is set */
> + if (req->do_avg) {
> + twl4030_madc_write(the_madc, method->avg + 1, ch_msb);
> + twl4030_madc_write(the_madc, method->avg, ch_lsb);
> + }
> +
> + if ((req->type == TWL4030_MADC_IRQ_ONESHOT) && (req->func_cb != NULL)) {
Extra and unnecessary (). Please remove.
> + twl4030_madc_set_irq(the_madc, req);
> + twl4030_madc_start_conversion(the_madc, req->method);
> + the_madc->requests[req->method].active = 1;
> + ret = 0;
> + goto out;
> + }
> +
> + /* With RT method we should not be here anymore */
> + if (req->method == TWL4030_MADC_RT) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + twl4030_madc_start_conversion(the_madc, req->method);
> + the_madc->requests[req->method].active = 1;
> +
> + /* Wait until conversion is ready (ctrl register returns EOC) */
> + ret = twl4030_madc_wait_conversion_ready(the_madc, 5, method->ctrl);
> + if (ret) {
> + dev_dbg(the_madc->hwmon_dev, "conversion timeout!\n");
> + the_madc->requests[req->method].active = 0;
> + goto out;
> + }
> +
> + ret = twl4030_madc_read_channels(the_madc, method->rbase, req->channels,
> + req->rbuf);
> +
> + the_madc->requests[req->method].active = 0;
> +
> +out:
> + mutex_unlock(&the_madc->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(twl4030_madc_conversion);
> +
If this function is going to be called from external code, it should not
really be defined here. I would suggest to move it to a global location such as
mfd instead, including all related functions.
The existence of this function export indicates that another non-hwmon
driver depends on this one, which should not really be the case. Another
reason to have a separate common driver instead, and mfd might just be the
place for it.
> +static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
> + int chan, int on)
> +{
> + int ret;
> + u8 regval;
> +
> + /*
> + * Current generator is only available for ADCIN0 and ADCIN1. NB:
> + * ADCIN1 current generator only works when AC or VBUS is present
> + */
> + if (chan > 1)
> + return EINVAL;
> +
So you don't trust your own code and return EINVAL even though this is the same module,
but then the caller doesn't check the return value. That doesn't make any sense.
First, if you don't trust your code, make this condition a BUG.
Second, check the return value if you return an error.
> + ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> + ®val, TWL4030_BCI_BCICTL1);
> + if (on)
> + regval |= (chan) ? TWL4030_BCI_ITHEN : TWL4030_BCI_TYPEN;
> + else
> + regval &= (chan) ? ~TWL4030_BCI_ITHEN : ~TWL4030_BCI_TYPEN;
(chan) in () does not provide any value. Please remove the extra ().
> + ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> + regval, TWL4030_BCI_BCICTL1);
> +
> + return ret;
> +}
> +
> +static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int on)
> +{
> + u8 regval;
> +
> + regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
> + if (on)
> + regval |= TWL4030_MADC_MADCON;
> + else
> + regval &= ~TWL4030_MADC_MADCON;
> + twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
> +
> + return 0;
> +}
> +
> +static long twl4030_madc_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
Highly unusual function for a hwmon driver. Another indication that there
should be a non-hwmon generic component instead to provide adc readings.
> +{
> + struct twl4030_madc_user_parms par;
> + int val, ret;
> +
> + ret = copy_from_user(&par, (void __user *) arg, sizeof(par));
> + if (ret) {
> + dev_dbg(the_madc->hwmon_dev, "copy_from_user: %d\n", ret);
> + return -EACCES;
> + }
> +
> + switch (cmd) {
> + case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> + struct twl4030_madc_request req;
> + if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
> + return -EINVAL;
> +
> + req.channels = (1 << par.channel);
> + req.do_avg = par.average;
> + req.method = TWL4030_MADC_SW1;
> + req.func_cb = NULL;
> +
> + val = twl4030_madc_conversion(&req);
> + if (val <= 0) {
> + par.status = -1;
> + } else {
> + par.status = 0;
> + par.result = (u16)req.rbuf[par.channel];
> + }
> + break;
> + }
> + default:
> + return -EINVAL;
> + }
> +
> + ret = copy_to_user((void __user *) arg, &par, sizeof(par));
> + if (ret) {
> + dev_dbg(the_madc->hwmon_dev, "copy_to_user: %d\n", ret);
> + return -EACCES;
> + }
> +
> + return 0;
> +}
> +
> +static const struct file_operations twl4030_madc_fileops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = twl4030_madc_ioctl
> +};
> +
> +static struct miscdevice twl4030_madc_device = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "twl4030-madc",
> + .fops = &twl4030_madc_fileops
> +};
> +
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO|S_IWUSR , madc_read, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO|S_IWUSR , madc_read, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO|S_IWUSR , madc_read, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO|S_IWUSR , madc_read, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO|S_IWUSR , madc_read, NULL, 4);
> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO|S_IWUSR , madc_read, NULL, 5);
> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO|S_IWUSR , madc_read, NULL, 6);
> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO|S_IWUSR , madc_read, NULL, 7);
> +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO|S_IWUSR , madc_read, NULL, 8);
> +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO|S_IWUSR , madc_read, NULL, 9);
> +static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO|S_IWUSR , madc_read, NULL, 10);
> +static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO|S_IWUSR , madc_read, NULL, 11);
> +static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO|S_IWUSR , madc_read, NULL, 12);
> +static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO|S_IWUSR , madc_read, NULL, 13);
> +static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO|S_IWUSR , madc_read, NULL, 14);
> +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO|S_IWUSR , madc_read, NULL, 15);
" , " --> ", "
> +
There is no write function. Why S_IWUSR ?
> +static struct attribute *twl4030_madc_attributes[] = {
> + &sensor_dev_attr_in0_input.dev_attr.attr,
> + &sensor_dev_attr_in1_input.dev_attr.attr,
> + &sensor_dev_attr_in2_input.dev_attr.attr,
> + &sensor_dev_attr_in3_input.dev_attr.attr,
> + &sensor_dev_attr_in4_input.dev_attr.attr,
> + &sensor_dev_attr_in5_input.dev_attr.attr,
> + &sensor_dev_attr_in6_input.dev_attr.attr,
> + &sensor_dev_attr_in7_input.dev_attr.attr,
> + &sensor_dev_attr_in8_input.dev_attr.attr,
> + &sensor_dev_attr_in9_input.dev_attr.attr,
> + &sensor_dev_attr_in10_input.dev_attr.attr,
> + &sensor_dev_attr_in11_input.dev_attr.attr,
> + &sensor_dev_attr_in12_input.dev_attr.attr,
> + &sensor_dev_attr_in13_input.dev_attr.attr,
> + &sensor_dev_attr_in14_input.dev_attr.attr,
> + &sensor_dev_attr_in15_input.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group twl4030_madc_group = {
> + .attrs = twl4030_madc_attributes,
> +};
> +
> +static int __init twl4030_madc_probe(struct platform_device *pdev)
> +{
> + struct twl4030_madc_data *madc;
> + struct twl4030_madc_platform_data *pdata = pdev->dev.platform_data;
> + int ret;
> + int status;
> + u8 regval;
> +
> + madc = kzalloc(sizeof *madc, GFP_KERNEL);
> + if (!madc)
> + return -ENOMEM;
> +
> + if (!pdata) {
> + dev_dbg(&pdev->dev, "platform_data not available\n");
> + ret = -EINVAL;
> + goto err_pdata;
> + }
> + madc->hwmon_dev = &pdev->dev;
> + madc->imr = (pdata->irq_line == 1) ?
> + TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
> + madc->isr = (pdata->irq_line == 1) ?
> + TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
> + madc->hwmon_dev = hwmon_device_register(&pdev->dev);
That is usually done last, after everything else works.
And you don't remove it if anything else failed. There should at least
be a hwmon_device_unregister() in the error path.
> + twl4030_madc_set_power(madc, 1);
Function is defines as int and can presumably return an error. So you should check it.
If there is no reason to return an error, please define function as void.
> + twl4030_madc_set_current_generator(madc, 0, 1);
> +
Same here.
> + ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> + ®val, TWL4030_BCI_BCICTL1);
> +
Return value check missing.
> + regval |= TWL4030_BCI_MESBAT;
> +
> + ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> + regval, TWL4030_BCI_BCICTL1);
Return value check missing ? If you don't want to check it, there is
no reason to assign it to ret.
> +
> + ret = request_threaded_irq(platform_get_irq(pdev, 0), NULL,
> + twl4030_madc_irq_handler , IRQF_TRIGGER_FALLING |
" , " --> ", "
> + IRQF_TRIGGER_RISING, "twl4030_madc", madc);
> + if (ret) {
> + dev_dbg(&pdev->dev, "could not request irq\n");
> + goto err_irq;
> + }
> +
> + platform_set_drvdata(pdev, madc);
> + mutex_init(&madc->lock);
> + ret = sysfs_create_group(&pdev->dev.kobj, &twl4030_madc_group);
> + if (ret)
> + goto err_misc;
> +
> + if (IS_ERR(madc->hwmon_dev)) {
> + dev_err(&pdev->dev, "hwmon_device_register failed.\n");
> + status = PTR_ERR(madc->hwmon_dev);
> + goto err_misc;
> + }
You abort installation if hwmon registration failed, but you don't clean
up, leaving the sysfs files existing but free madc. At the very least
you'll have dangling sysfs files, which will cause a NULL pointer
access when the sysfs read functions access use the_madc which will be NULL.
Please have a look at the error path and clean it up.
> + INIT_WORK(&madc->ws, twl4030_madc_work);
> + the_madc = madc;
> +
The use of the_madc instead of using platform_get_drvdata() is quite unusual
and suggests some structural problem with the driver.
Please check if you can use platform_get_drvdata() instead. If you can
use it, please do. If you can not, I would suggest to restructure the code
so you can.
> + return 0;
> +
> +err_irq:
> +err_misc:
> +err_pdata:
Multiple labels pointing to the same location does not make sense. Please use only one label.
Or add cleanup functionality to the others if needed.
> + kfree(madc);
> +
> + return ret;
> +}
> +
> +static int __exit twl4030_madc_remove(struct platform_device *pdev)
> +{
> + struct twl4030_madc_data *madc = platform_get_drvdata(pdev);
> +
> + twl4030_madc_set_power(madc, 0);
> + twl4030_madc_set_current_generator(madc, 0, 0);
> + free_irq(platform_get_irq(pdev, 0), madc);
> + cancel_work_sync(&madc->ws);
> +
hwmon_device_unregister() missing. sysfs attribute files not removed.
And how do you determine if any other module needing the exported function
is still loaded ?
> + return 0;
> +}
> +
> +static struct platform_driver twl4030_madc_driver = {
> + .probe = twl4030_madc_probe,
> + .remove = __exit_p(twl4030_madc_remove),
> + .driver = {
> + .name = "twl4030_madc",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init twl4030_madc_init(void)
> +{
> + return platform_driver_register(&twl4030_madc_driver);
> +}
> +module_init(twl4030_madc_init);
> +
> +static void __exit twl4030_madc_exit(void)
> +{
> + platform_driver_unregister(&twl4030_madc_driver);
> +}
> +module_exit(twl4030_madc_exit);
> +
> +MODULE_DESCRIPTION("twl4030 ADC driver");
> +MODULE_LICENSE("GPL");
MODULE_AUTHOR would be nice to have too.
> +
Extra blank line here, causing a whitespace error when integrating. Please remove.
> diff --git a/include/linux/i2c/twl4030-madc.h b/include/linux/i2c/twl4030-madc.h
> new file mode 100644
> index 0000000..1e46ff1
> --- /dev/null
> +++ b/include/linux/i2c/twl4030-madc.h
> @@ -0,0 +1,118 @@
> +/*
> + * 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
> + *
> + */
> +
> +#ifndef _TWL4030_MADC_H
> +#define _TWL4030_MADC_H
> +
> +struct twl4030_madc_conversion_method {
> + u8 sel;
> + u8 avg;
> + u8 rbase;
> + u8 ctrl;
> +};
> +
> +#define TWL4030_MADC_MAX_CHANNELS 16
> +
> +struct twl4030_madc_request {
> + u16 channels;
> + u16 do_avg;
> + u16 method;
> + u16 type;
> + bool active;
> + bool result_pending;
> + int rbuf[TWL4030_MADC_MAX_CHANNELS];
> + void (*func_cb)(int len, int channels, int *buf);
> +};
> +
> +enum conversion_methods {
> + TWL4030_MADC_RT,
> + TWL4030_MADC_SW1,
> + TWL4030_MADC_SW2,
> + TWL4030_MADC_NUM_METHODS
> +};
> +
> +enum sample_type {
> + TWL4030_MADC_WAIT,
> + TWL4030_MADC_IRQ_ONESHOT,
> + TWL4030_MADC_IRQ_REARM
> +};
> +
> +#define TWL4030_MADC_CTRL1 0x00
> +#define TWL4030_MADC_CTRL2 0x01
> +
> +#define TWL4030_MADC_RTSELECT_LSB 0x02
> +#define TWL4030_MADC_SW1SELECT_LSB 0x06
> +#define TWL4030_MADC_SW2SELECT_LSB 0x0A
> +
> +#define TWL4030_MADC_RTAVERAGE_LSB 0x04
> +#define TWL4030_MADC_SW1AVERAGE_LSB 0x08
> +#define TWL4030_MADC_SW2AVERAGE_LSB 0x0C
> +
> +#define TWL4030_MADC_CTRL_SW1 0x12
> +#define TWL4030_MADC_CTRL_SW2 0x13
> +
> +#define TWL4030_MADC_RTCH0_LSB 0x17
> +#define TWL4030_MADC_GPCH0_LSB 0x37
> +
> +#define TWL4030_MADC_MADCON (1<<0) /* MADC power on */
> +#define TWL4030_MADC_BUSY (1<<0) /* MADC busy */
> +#define TWL4030_MADC_EOC_SW (1<<1) /* MADC conversion completion */
> +#define TWL4030_MADC_SW_START (1<<5) /* MADC SWx start conversion */
> +
> +#define TWL4030_MADC_ADCIN0 (1<<0)
> +#define TWL4030_MADC_ADCIN1 (1<<1)
> +#define TWL4030_MADC_ADCIN2 (1<<2)
> +#define TWL4030_MADC_ADCIN3 (1<<3)
> +#define TWL4030_MADC_ADCIN4 (1<<4)
> +#define TWL4030_MADC_ADCIN5 (1<<5)
> +#define TWL4030_MADC_ADCIN6 (1<<6)
> +#define TWL4030_MADC_ADCIN7 (1<<7)
> +#define TWL4030_MADC_ADCIN8 (1<<8)
> +#define TWL4030_MADC_ADCIN9 (1<<9)
> +#define TWL4030_MADC_ADCIN10 (1<<10)
> +#define TWL4030_MADC_ADCIN11 (1<<11)
> +#define TWL4030_MADC_ADCIN12 (1<<12)
> +#define TWL4030_MADC_ADCIN13 (1<<13)
> +#define TWL4030_MADC_ADCIN14 (1<<14)
> +#define TWL4030_MADC_ADCIN15 (1<<15)
> +
> +/* Fixed channels */
> +#define TWL4030_MADC_BTEMP TWL4030_MADC_ADCIN1
> +#define TWL4030_MADC_VBUS TWL4030_MADC_ADCIN8
> +#define TWL4030_MADC_VBKB TWL4030_MADC_ADCIN9
> +#define TWL4030_MADC_ICHG TWL4030_MADC_ADCIN10
> +#define TWL4030_MADC_VCHG TWL4030_MADC_ADCIN11
> +#define TWL4030_MADC_VBAT TWL4030_MADC_ADCIN12
> +
> +#define TWL4030_BCI_BCICTL1 0x23
> +#define TWL4030_BCI_MESBAT (1<<1)
> +#define TWL4030_BCI_TYPEN (1<<4)
> +#define TWL4030_BCI_ITHEN (1<<3)
> +
> +#define TWL4030_MADC_IOC_MAGIC '`'
> +#define TWL4030_MADC_IOCX_ADC_RAW_READ _IO(TWL4030_MADC_IOC_MAGIC, 0)
> +
> +struct twl4030_madc_user_parms {
> + int channel;
> + int average;
> + int status;
> + u16 result;
> +};
> +
> +int twl4030_madc_conversion(struct twl4030_madc_request *conv);
> +
> +#endif
> --
> 1.7.0.4
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
@ 2010-09-16 15:17 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2010-09-16 15:17 UTC (permalink / raw)
To: Keerthy; +Cc: linux-omap, lm-sensors, balajitk
On Thu, Sep 16, 2010 at 06:23:24AM -0400, Keerthy wrote:
> MADC driver is added under hwmon drivers. This driver monitors the real time
> conversion of analog signals like battery temperature,
> battery type, battery level etc. User can also ask for the conversion on a
> particular channel using the sysfs nodes.
>
Number of comments inline. This is not a complete review; I think there
is some cleanup to be done before that is possible.
Please run the driver through Lindent. While I understand that people prefer
their personal touch, it makes it easier to have a single coding style in the kernel.
I commented on this a couple of times below - the driver mixes generic ADC
reading functions with hwmon functionality. Generic ADC reading functionality
should be moved into another driver, possibly to mfd.
> Several people have contributed to this driver on the linux-omap list.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
> drivers/hwmon/twl4030-madc.c | 584 ++++++++++++++++++++++++++++++++++++++
> include/linux/i2c/twl4030-madc.h | 118 ++++++++
> 2 files changed, 702 insertions(+), 0 deletions(-)
> create mode 100644 drivers/hwmon/twl4030-madc.c
> create mode 100644 include/linux/i2c/twl4030-madc.h
>
> diff --git a/drivers/hwmon/twl4030-madc.c b/drivers/hwmon/twl4030-madc.c
> new file mode 100644
> index 0000000..b96fccd
> --- /dev/null
> +++ b/drivers/hwmon/twl4030-madc.c
> @@ -0,0 +1,584 @@
> +/*
It is customary to add a brief description as well as the author here.
> + * 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/types.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
> +#include <linux/slab.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/i2c/twl4030-madc.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include <linux/uaccess.h>
> +
> +
Extra blank line. Lindent will remove it.
> +struct twl4030_madc_data {
> + struct device *hwmon_dev;
> + struct mutex lock;
> + struct work_struct ws;
> + struct twl4030_madc_request requests[TWL4030_MADC_NUM_METHODS];
> + int imr;
> + int isr;
> +};
> +
> +static struct twl4030_madc_data *the_madc;
> +
This variable should not exist. See other comments later.
> +static ssize_t madc_read(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + struct twl4030_madc_request req;
> + int status;
> + unsigned long val;
> +
> + req.channels = (1 << attr->index);
> + req.method = TWL4030_MADC_SW2;
> + req.func_cb = NULL;
> +
> + val = twl4030_madc_conversion(&req);
> + if (likely(val >= 0))
> + val = req.rbuf[attr->index];
> + else
> + return val;
> + status = sprintf(buf, "%lu\n", val);
> + return status;
> +}
> +
> +static
> +const struct twl4030_madc_conversion_method twl4030_conversion_methods[] = {
> + [TWL4030_MADC_RT] = {
> + .sel = TWL4030_MADC_RTSELECT_LSB,
> + .avg = TWL4030_MADC_RTAVERAGE_LSB,
> + .rbase = TWL4030_MADC_RTCH0_LSB,
> + },
> + [TWL4030_MADC_SW1] = {
> + .sel = TWL4030_MADC_SW1SELECT_LSB,
> + .avg = TWL4030_MADC_SW1AVERAGE_LSB,
> + .rbase = TWL4030_MADC_GPCH0_LSB,
> + .ctrl = TWL4030_MADC_CTRL_SW1,
> + },
> + [TWL4030_MADC_SW2] = {
> + .sel = TWL4030_MADC_SW2SELECT_LSB,
> + .avg = TWL4030_MADC_SW2AVERAGE_LSB,
> + .rbase = TWL4030_MADC_GPCH0_LSB,
> + .ctrl = TWL4030_MADC_CTRL_SW2,
> + },
> +};
> +
> +static int twl4030_madc_read(struct twl4030_madc_data *madc, u8 reg)
> +{
> + int ret;
> + u8 val;
> +
> + ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &val, reg);
Structural comment. If there is a i2c master, why don't you have an I2C
master driver instead of calling a platform dependent i2c function ?
I see this function called from all over the place, so maybe there is a
reason. Just looks odd, though.
> + if (ret) {
> + dev_dbg(madc->hwmon_dev, "unable to read register 0x%X\n", reg);
> + return ret;
> + }
> +
> + return val;
> +}
> +
> +static void twl4030_madc_write(struct twl4030_madc_data *madc, u8 reg, u8 val)
> +{
> + int ret;
> +
> + ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, reg);
> + if (ret)
> + dev_err(madc->hwmon_dev,
> + "unable to write register 0x%X\n", reg);
> +}
> +
> +static int twl4030_madc_channel_raw_read(struct twl4030_madc_data *madc, u8 reg)
> +{
> + u8 msb, lsb;
> +
> + /*
> + *For each ADC channel, we have MSB and LSB register pair. MSB address
> + *is always LSB address+1. reg parameter is the addr of LSB register
> + */
> + msb = twl4030_madc_read(madc, reg + 1);
> + lsb = twl4030_madc_read(madc, reg);
> +
> + return (int)(((msb << 8) | lsb) >> 6);
> +}
> +
> +static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
> + u8 reg_base, u16 channels, int *buf)
> +{
> + int count = 0;
> + u8 reg, i;
> +
> + if (unlikely(!buf))
> + return 0;
> +
> + for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
> + if (channels & (1<<i)) {
> + reg = reg_base + 2*i;
> + buf[i] = twl4030_madc_channel_raw_read(madc, reg);
> + count++;
> + }
> + }
> + return count;
> +}
> +
> +static void twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int id)
> +{
> + u8 val;
> +
> + val = twl4030_madc_read(madc, madc->imr);
> + val &= ~(1 << id);
> + twl4030_madc_write(madc, madc->imr, val);
> +}
> +
> +static void twl4030_madc_disable_irq(struct twl4030_madc_data *madc, int id)
> +{
> + u8 val;
> +
> + val = twl4030_madc_read(madc, madc->imr);
> + val |= (1 << id);
> + twl4030_madc_write(madc, madc->imr, val);
> +}
> +
> +static irqreturn_t twl4030_madc_irq_handler(int irq, void *_madc)
> +{
> + struct twl4030_madc_data *madc = _madc;
> + u8 isr_val, imr_val;
> + int i;
> +
> + isr_val = twl4030_madc_read(madc, madc->isr);
> + imr_val = twl4030_madc_read(madc, madc->imr);
> +
> + isr_val &= ~imr_val;
> +
> + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> +
> + if (!(isr_val & (1<<i)))
> + continue;
> +
> + twl4030_madc_disable_irq(madc, i);
> + madc->requests[i].result_pending = 1;
> + }
> +
> + schedule_work(&madc->ws);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void twl4030_madc_work(struct work_struct *ws)
> +{
> + const struct twl4030_madc_conversion_method *method;
> + struct twl4030_madc_data *madc;
> + struct twl4030_madc_request *r;
> + int len, i;
> +
> + madc = container_of(ws, struct twl4030_madc_data, ws);
> + mutex_lock(&madc->lock);
> +
> + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> +
> + r = &madc->requests[i];
> +
> + /* No pending results for this method, move to next one */
> + if (!r->result_pending)
> + continue;
> +
> + method = &twl4030_conversion_methods[r->method];
> +
> + /* Read results */
> + len = twl4030_madc_read_channels(madc, method->rbase,
> + r->channels, r->rbuf);
> +
> + /* Return results to caller */
> + if (r->func_cb != NULL) {
> + r->func_cb(len, r->channels, r->rbuf);
> + r->func_cb = NULL;
> + }
> +
> + /* Free request */
> + r->result_pending = 0;
> + r->active = 0;
> + }
> +
> + mutex_unlock(&madc->lock);
> +}
> +
> +static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
> + struct twl4030_madc_request *req)
> +{
> + struct twl4030_madc_request *p;
> +
> + p = &madc->requests[req->method];
> +
> + memcpy(p, req, sizeof *req);
> +
> + twl4030_madc_enable_irq(madc, req->method);
> +
> + return 0;
> +}
> +
> +static inline void twl4030_madc_start_conversion(struct twl4030_madc_data *madc,
> + int conv_method)
> +{
> + const struct twl4030_madc_conversion_method *method;
> +
> + method = &twl4030_conversion_methods[conv_method];
> +
> + switch (conv_method) {
> + case TWL4030_MADC_SW1:
> + case TWL4030_MADC_SW2:
> + twl4030_madc_write(madc, method->ctrl, TWL4030_MADC_SW_START);
> + break;
> + case TWL4030_MADC_RT:
> + default:
> + break;
> + }
> +}
> +
> +static int twl4030_madc_wait_conversion_ready(
> + struct twl4030_madc_data *madc,
> + unsigned int timeout_ms, u8 status_reg)
> +{
> + unsigned long timeout;
> +
> + timeout = jiffies + msecs_to_jiffies(timeout_ms);
> + do {
> + u8 reg;
> +
> + reg = twl4030_madc_read(madc, status_reg);
> + if (!(reg & TWL4030_MADC_BUSY) && (reg & TWL4030_MADC_EOC_SW))
> + return 0;
> + } while (!time_after(jiffies, timeout));
> +
Does this have to be a hard wait ?
> + return -EAGAIN;
> +}
> +
> +int twl4030_madc_conversion(struct twl4030_madc_request *req)
> +{
> + const struct twl4030_madc_conversion_method *method;
> + u8 ch_msb, ch_lsb;
> + int ret;
> +
> + if (unlikely(!req))
> + return -EINVAL;
> +
> + mutex_lock(&the_madc->lock);
> +
> + /* Do we have a conversion request ongoing */
> + if (the_madc->requests[req->method].active) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + ch_msb = (req->channels >> 8) & 0xff;
> + ch_lsb = req->channels & 0xff;
> +
> + method = &twl4030_conversion_methods[req->method];
> +
> + /* Select channels to be converted */
> + twl4030_madc_write(the_madc, method->sel + 1, ch_msb);
> + twl4030_madc_write(the_madc, method->sel, ch_lsb);
> +
> + /* Select averaging for all channels if do_avg is set */
> + if (req->do_avg) {
> + twl4030_madc_write(the_madc, method->avg + 1, ch_msb);
> + twl4030_madc_write(the_madc, method->avg, ch_lsb);
> + }
> +
> + if ((req->type = TWL4030_MADC_IRQ_ONESHOT) && (req->func_cb != NULL)) {
Extra and unnecessary (). Please remove.
> + twl4030_madc_set_irq(the_madc, req);
> + twl4030_madc_start_conversion(the_madc, req->method);
> + the_madc->requests[req->method].active = 1;
> + ret = 0;
> + goto out;
> + }
> +
> + /* With RT method we should not be here anymore */
> + if (req->method = TWL4030_MADC_RT) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + twl4030_madc_start_conversion(the_madc, req->method);
> + the_madc->requests[req->method].active = 1;
> +
> + /* Wait until conversion is ready (ctrl register returns EOC) */
> + ret = twl4030_madc_wait_conversion_ready(the_madc, 5, method->ctrl);
> + if (ret) {
> + dev_dbg(the_madc->hwmon_dev, "conversion timeout!\n");
> + the_madc->requests[req->method].active = 0;
> + goto out;
> + }
> +
> + ret = twl4030_madc_read_channels(the_madc, method->rbase, req->channels,
> + req->rbuf);
> +
> + the_madc->requests[req->method].active = 0;
> +
> +out:
> + mutex_unlock(&the_madc->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(twl4030_madc_conversion);
> +
If this function is going to be called from external code, it should not
really be defined here. I would suggest to move it to a global location such as
mfd instead, including all related functions.
The existence of this function export indicates that another non-hwmon
driver depends on this one, which should not really be the case. Another
reason to have a separate common driver instead, and mfd might just be the
place for it.
> +static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
> + int chan, int on)
> +{
> + int ret;
> + u8 regval;
> +
> + /*
> + * Current generator is only available for ADCIN0 and ADCIN1. NB:
> + * ADCIN1 current generator only works when AC or VBUS is present
> + */
> + if (chan > 1)
> + return EINVAL;
> +
So you don't trust your own code and return EINVAL even though this is the same module,
but then the caller doesn't check the return value. That doesn't make any sense.
First, if you don't trust your code, make this condition a BUG.
Second, check the return value if you return an error.
> + ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> + ®val, TWL4030_BCI_BCICTL1);
> + if (on)
> + regval |= (chan) ? TWL4030_BCI_ITHEN : TWL4030_BCI_TYPEN;
> + else
> + regval &= (chan) ? ~TWL4030_BCI_ITHEN : ~TWL4030_BCI_TYPEN;
(chan) in () does not provide any value. Please remove the extra ().
> + ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> + regval, TWL4030_BCI_BCICTL1);
> +
> + return ret;
> +}
> +
> +static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int on)
> +{
> + u8 regval;
> +
> + regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
> + if (on)
> + regval |= TWL4030_MADC_MADCON;
> + else
> + regval &= ~TWL4030_MADC_MADCON;
> + twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
> +
> + return 0;
> +}
> +
> +static long twl4030_madc_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
Highly unusual function for a hwmon driver. Another indication that there
should be a non-hwmon generic component instead to provide adc readings.
> +{
> + struct twl4030_madc_user_parms par;
> + int val, ret;
> +
> + ret = copy_from_user(&par, (void __user *) arg, sizeof(par));
> + if (ret) {
> + dev_dbg(the_madc->hwmon_dev, "copy_from_user: %d\n", ret);
> + return -EACCES;
> + }
> +
> + switch (cmd) {
> + case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> + struct twl4030_madc_request req;
> + if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
> + return -EINVAL;
> +
> + req.channels = (1 << par.channel);
> + req.do_avg = par.average;
> + req.method = TWL4030_MADC_SW1;
> + req.func_cb = NULL;
> +
> + val = twl4030_madc_conversion(&req);
> + if (val <= 0) {
> + par.status = -1;
> + } else {
> + par.status = 0;
> + par.result = (u16)req.rbuf[par.channel];
> + }
> + break;
> + }
> + default:
> + return -EINVAL;
> + }
> +
> + ret = copy_to_user((void __user *) arg, &par, sizeof(par));
> + if (ret) {
> + dev_dbg(the_madc->hwmon_dev, "copy_to_user: %d\n", ret);
> + return -EACCES;
> + }
> +
> + return 0;
> +}
> +
> +static const struct file_operations twl4030_madc_fileops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = twl4030_madc_ioctl
> +};
> +
> +static struct miscdevice twl4030_madc_device = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "twl4030-madc",
> + .fops = &twl4030_madc_fileops
> +};
> +
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO|S_IWUSR , madc_read, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO|S_IWUSR , madc_read, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO|S_IWUSR , madc_read, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO|S_IWUSR , madc_read, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO|S_IWUSR , madc_read, NULL, 4);
> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO|S_IWUSR , madc_read, NULL, 5);
> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO|S_IWUSR , madc_read, NULL, 6);
> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO|S_IWUSR , madc_read, NULL, 7);
> +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO|S_IWUSR , madc_read, NULL, 8);
> +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO|S_IWUSR , madc_read, NULL, 9);
> +static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO|S_IWUSR , madc_read, NULL, 10);
> +static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO|S_IWUSR , madc_read, NULL, 11);
> +static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO|S_IWUSR , madc_read, NULL, 12);
> +static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO|S_IWUSR , madc_read, NULL, 13);
> +static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO|S_IWUSR , madc_read, NULL, 14);
> +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO|S_IWUSR , madc_read, NULL, 15);
" , " --> ", "
> +
There is no write function. Why S_IWUSR ?
> +static struct attribute *twl4030_madc_attributes[] = {
> + &sensor_dev_attr_in0_input.dev_attr.attr,
> + &sensor_dev_attr_in1_input.dev_attr.attr,
> + &sensor_dev_attr_in2_input.dev_attr.attr,
> + &sensor_dev_attr_in3_input.dev_attr.attr,
> + &sensor_dev_attr_in4_input.dev_attr.attr,
> + &sensor_dev_attr_in5_input.dev_attr.attr,
> + &sensor_dev_attr_in6_input.dev_attr.attr,
> + &sensor_dev_attr_in7_input.dev_attr.attr,
> + &sensor_dev_attr_in8_input.dev_attr.attr,
> + &sensor_dev_attr_in9_input.dev_attr.attr,
> + &sensor_dev_attr_in10_input.dev_attr.attr,
> + &sensor_dev_attr_in11_input.dev_attr.attr,
> + &sensor_dev_attr_in12_input.dev_attr.attr,
> + &sensor_dev_attr_in13_input.dev_attr.attr,
> + &sensor_dev_attr_in14_input.dev_attr.attr,
> + &sensor_dev_attr_in15_input.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group twl4030_madc_group = {
> + .attrs = twl4030_madc_attributes,
> +};
> +
> +static int __init twl4030_madc_probe(struct platform_device *pdev)
> +{
> + struct twl4030_madc_data *madc;
> + struct twl4030_madc_platform_data *pdata = pdev->dev.platform_data;
> + int ret;
> + int status;
> + u8 regval;
> +
> + madc = kzalloc(sizeof *madc, GFP_KERNEL);
> + if (!madc)
> + return -ENOMEM;
> +
> + if (!pdata) {
> + dev_dbg(&pdev->dev, "platform_data not available\n");
> + ret = -EINVAL;
> + goto err_pdata;
> + }
> + madc->hwmon_dev = &pdev->dev;
> + madc->imr = (pdata->irq_line = 1) ?
> + TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
> + madc->isr = (pdata->irq_line = 1) ?
> + TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
> + madc->hwmon_dev = hwmon_device_register(&pdev->dev);
That is usually done last, after everything else works.
And you don't remove it if anything else failed. There should at least
be a hwmon_device_unregister() in the error path.
> + twl4030_madc_set_power(madc, 1);
Function is defines as int and can presumably return an error. So you should check it.
If there is no reason to return an error, please define function as void.
> + twl4030_madc_set_current_generator(madc, 0, 1);
> +
Same here.
> + ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> + ®val, TWL4030_BCI_BCICTL1);
> +
Return value check missing.
> + regval |= TWL4030_BCI_MESBAT;
> +
> + ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> + regval, TWL4030_BCI_BCICTL1);
Return value check missing ? If you don't want to check it, there is
no reason to assign it to ret.
> +
> + ret = request_threaded_irq(platform_get_irq(pdev, 0), NULL,
> + twl4030_madc_irq_handler , IRQF_TRIGGER_FALLING |
" , " --> ", "
> + IRQF_TRIGGER_RISING, "twl4030_madc", madc);
> + if (ret) {
> + dev_dbg(&pdev->dev, "could not request irq\n");
> + goto err_irq;
> + }
> +
> + platform_set_drvdata(pdev, madc);
> + mutex_init(&madc->lock);
> + ret = sysfs_create_group(&pdev->dev.kobj, &twl4030_madc_group);
> + if (ret)
> + goto err_misc;
> +
> + if (IS_ERR(madc->hwmon_dev)) {
> + dev_err(&pdev->dev, "hwmon_device_register failed.\n");
> + status = PTR_ERR(madc->hwmon_dev);
> + goto err_misc;
> + }
You abort installation if hwmon registration failed, but you don't clean
up, leaving the sysfs files existing but free madc. At the very least
you'll have dangling sysfs files, which will cause a NULL pointer
access when the sysfs read functions access use the_madc which will be NULL.
Please have a look at the error path and clean it up.
> + INIT_WORK(&madc->ws, twl4030_madc_work);
> + the_madc = madc;
> +
The use of the_madc instead of using platform_get_drvdata() is quite unusual
and suggests some structural problem with the driver.
Please check if you can use platform_get_drvdata() instead. If you can
use it, please do. If you can not, I would suggest to restructure the code
so you can.
> + return 0;
> +
> +err_irq:
> +err_misc:
> +err_pdata:
Multiple labels pointing to the same location does not make sense. Please use only one label.
Or add cleanup functionality to the others if needed.
> + kfree(madc);
> +
> + return ret;
> +}
> +
> +static int __exit twl4030_madc_remove(struct platform_device *pdev)
> +{
> + struct twl4030_madc_data *madc = platform_get_drvdata(pdev);
> +
> + twl4030_madc_set_power(madc, 0);
> + twl4030_madc_set_current_generator(madc, 0, 0);
> + free_irq(platform_get_irq(pdev, 0), madc);
> + cancel_work_sync(&madc->ws);
> +
hwmon_device_unregister() missing. sysfs attribute files not removed.
And how do you determine if any other module needing the exported function
is still loaded ?
> + return 0;
> +}
> +
> +static struct platform_driver twl4030_madc_driver = {
> + .probe = twl4030_madc_probe,
> + .remove = __exit_p(twl4030_madc_remove),
> + .driver = {
> + .name = "twl4030_madc",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init twl4030_madc_init(void)
> +{
> + return platform_driver_register(&twl4030_madc_driver);
> +}
> +module_init(twl4030_madc_init);
> +
> +static void __exit twl4030_madc_exit(void)
> +{
> + platform_driver_unregister(&twl4030_madc_driver);
> +}
> +module_exit(twl4030_madc_exit);
> +
> +MODULE_DESCRIPTION("twl4030 ADC driver");
> +MODULE_LICENSE("GPL");
MODULE_AUTHOR would be nice to have too.
> +
Extra blank line here, causing a whitespace error when integrating. Please remove.
> diff --git a/include/linux/i2c/twl4030-madc.h b/include/linux/i2c/twl4030-madc.h
> new file mode 100644
> index 0000000..1e46ff1
> --- /dev/null
> +++ b/include/linux/i2c/twl4030-madc.h
> @@ -0,0 +1,118 @@
> +/*
> + * 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
> + *
> + */
> +
> +#ifndef _TWL4030_MADC_H
> +#define _TWL4030_MADC_H
> +
> +struct twl4030_madc_conversion_method {
> + u8 sel;
> + u8 avg;
> + u8 rbase;
> + u8 ctrl;
> +};
> +
> +#define TWL4030_MADC_MAX_CHANNELS 16
> +
> +struct twl4030_madc_request {
> + u16 channels;
> + u16 do_avg;
> + u16 method;
> + u16 type;
> + bool active;
> + bool result_pending;
> + int rbuf[TWL4030_MADC_MAX_CHANNELS];
> + void (*func_cb)(int len, int channels, int *buf);
> +};
> +
> +enum conversion_methods {
> + TWL4030_MADC_RT,
> + TWL4030_MADC_SW1,
> + TWL4030_MADC_SW2,
> + TWL4030_MADC_NUM_METHODS
> +};
> +
> +enum sample_type {
> + TWL4030_MADC_WAIT,
> + TWL4030_MADC_IRQ_ONESHOT,
> + TWL4030_MADC_IRQ_REARM
> +};
> +
> +#define TWL4030_MADC_CTRL1 0x00
> +#define TWL4030_MADC_CTRL2 0x01
> +
> +#define TWL4030_MADC_RTSELECT_LSB 0x02
> +#define TWL4030_MADC_SW1SELECT_LSB 0x06
> +#define TWL4030_MADC_SW2SELECT_LSB 0x0A
> +
> +#define TWL4030_MADC_RTAVERAGE_LSB 0x04
> +#define TWL4030_MADC_SW1AVERAGE_LSB 0x08
> +#define TWL4030_MADC_SW2AVERAGE_LSB 0x0C
> +
> +#define TWL4030_MADC_CTRL_SW1 0x12
> +#define TWL4030_MADC_CTRL_SW2 0x13
> +
> +#define TWL4030_MADC_RTCH0_LSB 0x17
> +#define TWL4030_MADC_GPCH0_LSB 0x37
> +
> +#define TWL4030_MADC_MADCON (1<<0) /* MADC power on */
> +#define TWL4030_MADC_BUSY (1<<0) /* MADC busy */
> +#define TWL4030_MADC_EOC_SW (1<<1) /* MADC conversion completion */
> +#define TWL4030_MADC_SW_START (1<<5) /* MADC SWx start conversion */
> +
> +#define TWL4030_MADC_ADCIN0 (1<<0)
> +#define TWL4030_MADC_ADCIN1 (1<<1)
> +#define TWL4030_MADC_ADCIN2 (1<<2)
> +#define TWL4030_MADC_ADCIN3 (1<<3)
> +#define TWL4030_MADC_ADCIN4 (1<<4)
> +#define TWL4030_MADC_ADCIN5 (1<<5)
> +#define TWL4030_MADC_ADCIN6 (1<<6)
> +#define TWL4030_MADC_ADCIN7 (1<<7)
> +#define TWL4030_MADC_ADCIN8 (1<<8)
> +#define TWL4030_MADC_ADCIN9 (1<<9)
> +#define TWL4030_MADC_ADCIN10 (1<<10)
> +#define TWL4030_MADC_ADCIN11 (1<<11)
> +#define TWL4030_MADC_ADCIN12 (1<<12)
> +#define TWL4030_MADC_ADCIN13 (1<<13)
> +#define TWL4030_MADC_ADCIN14 (1<<14)
> +#define TWL4030_MADC_ADCIN15 (1<<15)
> +
> +/* Fixed channels */
> +#define TWL4030_MADC_BTEMP TWL4030_MADC_ADCIN1
> +#define TWL4030_MADC_VBUS TWL4030_MADC_ADCIN8
> +#define TWL4030_MADC_VBKB TWL4030_MADC_ADCIN9
> +#define TWL4030_MADC_ICHG TWL4030_MADC_ADCIN10
> +#define TWL4030_MADC_VCHG TWL4030_MADC_ADCIN11
> +#define TWL4030_MADC_VBAT TWL4030_MADC_ADCIN12
> +
> +#define TWL4030_BCI_BCICTL1 0x23
> +#define TWL4030_BCI_MESBAT (1<<1)
> +#define TWL4030_BCI_TYPEN (1<<4)
> +#define TWL4030_BCI_ITHEN (1<<3)
> +
> +#define TWL4030_MADC_IOC_MAGIC '`'
> +#define TWL4030_MADC_IOCX_ADC_RAW_READ _IO(TWL4030_MADC_IOC_MAGIC, 0)
> +
> +struct twl4030_madc_user_parms {
> + int channel;
> + int average;
> + int status;
> + u16 result;
> +};
> +
> +int twl4030_madc_conversion(struct twl4030_madc_request *conv);
> +
> +#endif
> --
> 1.7.0.4
>
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module
2010-09-16 15:17 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 Guenter Roeck
@ 2010-09-20 10:46 ` J, KEERTHY
-1 siblings, 0 replies; 12+ messages in thread
From: J, KEERTHY @ 2010-09-20 10:34 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-omap, lm-sensors, Krishnamoorthy, Balaji T
> -----Original Message-----
> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> Sent: Thursday, September 16, 2010 8:48 PM
> To: J, KEERTHY
> Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy,
> Balaji T
> Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
> madc module
>
> On Thu, Sep 16, 2010 at 06:23:24AM -0400, Keerthy wrote:
> > MADC driver is added under hwmon drivers. This driver monitors the real
> time
> > conversion of analog signals like battery temperature,
> > battery type, battery level etc. User can also ask for the conversion on
> a
> > particular channel using the sysfs nodes.
> >
> Number of comments inline. This is not a complete review; I think there
> is some cleanup to be done before that is possible.
>
> Please run the driver through Lindent. While I understand that people
> prefer
> their personal touch, it makes it easier to have a single coding style in
> the kernel.
>
I will run Lindent.
> I commented on this a couple of times below - the driver mixes generic ADC
> reading functions with hwmon functionality. Generic ADC reading
> functionality
> should be moved into another driver, possibly to mfd.
>
I am addressing the comment below.
> > Several people have contributed to this driver on the linux-omap list.
> >
> > Signed-off-by: Keerthy <j-keerthy@ti.com>
> > ---
> > drivers/hwmon/twl4030-madc.c | 584
> ++++++++++++++++++++++++++++++++++++++
> > include/linux/i2c/twl4030-madc.h | 118 ++++++++
> > 2 files changed, 702 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/hwmon/twl4030-madc.c
> > create mode 100644 include/linux/i2c/twl4030-madc.h
> >
> > diff --git a/drivers/hwmon/twl4030-madc.c b/drivers/hwmon/twl4030-madc.c
> > new file mode 100644
> > index 0000000..b96fccd
> > --- /dev/null
> > +++ b/drivers/hwmon/twl4030-madc.c
> > @@ -0,0 +1,584 @@
> > +/*
>
> It is customary to add a brief description as well as the author here.
>
I will add it.
> > + * 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/types.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/fs.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c/twl.h>
> > +#include <linux/i2c/twl4030-madc.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +
> > +#include <linux/uaccess.h>
> > +
> > +
> Extra blank line. Lindent will remove it.
>
I will run the Lindent.
> > +struct twl4030_madc_data {
> > + struct device *hwmon_dev;
> > + struct mutex lock;
> > + struct work_struct ws;
> > + struct twl4030_madc_request
> requests[TWL4030_MADC_NUM_METHODS];
> > + int imr;
> > + int isr;
> > +};
> > +
> > +static struct twl4030_madc_data *the_madc;
> > +
> This variable should not exist. See other comments later.
>
> > +static ssize_t madc_read(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + struct sensor_device_attribute *attr =
> to_sensor_dev_attr(devattr);
> > + struct twl4030_madc_request req;
> > + int status;
> > + unsigned long val;
> > +
> > + req.channels = (1 << attr->index);
> > + req.method = TWL4030_MADC_SW2;
> > + req.func_cb = NULL;
> > +
> > + val = twl4030_madc_conversion(&req);
> > + if (likely(val >= 0))
> > + val = req.rbuf[attr->index];
> > + else
> > + return val;
> > + status = sprintf(buf, "%lu\n", val);
> > + return status;
> > +}
> > +
> > +static
> > +const struct twl4030_madc_conversion_method
> twl4030_conversion_methods[] = {
> > + [TWL4030_MADC_RT] = {
> > + .sel = TWL4030_MADC_RTSELECT_LSB,
> > + .avg = TWL4030_MADC_RTAVERAGE_LSB,
> > + .rbase = TWL4030_MADC_RTCH0_LSB,
> > + },
> > + [TWL4030_MADC_SW1] = {
> > + .sel = TWL4030_MADC_SW1SELECT_LSB,
> > + .avg = TWL4030_MADC_SW1AVERAGE_LSB,
> > + .rbase = TWL4030_MADC_GPCH0_LSB,
> > + .ctrl = TWL4030_MADC_CTRL_SW1,
> > + },
> > + [TWL4030_MADC_SW2] = {
> > + .sel = TWL4030_MADC_SW2SELECT_LSB,
> > + .avg = TWL4030_MADC_SW2AVERAGE_LSB,
> > + .rbase = TWL4030_MADC_GPCH0_LSB,
> > + .ctrl = TWL4030_MADC_CTRL_SW2,
> > + },
> > +};
> > +
> > +static int twl4030_madc_read(struct twl4030_madc_data *madc, u8 reg)
> > +{
> > + int ret;
> > + u8 val;
> > +
> > + ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &val, reg);
>
> Structural comment. If there is a i2c master, why don't you have an I2C
> master driver instead of calling a platform dependent i2c function ?
> I see this function called from all over the place, so maybe there is a
> reason. Just looks odd, though.
>
There are many small modules in the same i2c slave address 0x4A.
twl4030_keypad is one such module which uses the above function.
All the drivers call the above function with their individual offsets.
> > + if (ret) {
> > + dev_dbg(madc->hwmon_dev, "unable to read register
> 0x%X\n", reg);
> > + return ret;
> > + }
> > +
> > + return val;
> > +}
> > +
> > +static void twl4030_madc_write(struct twl4030_madc_data *madc, u8 reg,
> u8 val)
> > +{
> > + int ret;
> > +
> > + ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, reg);
> > + if (ret)
> > + dev_err(madc->hwmon_dev,
> > + "unable to write register 0x%X\n", reg);
> > +}
> > +
> > +static int twl4030_madc_channel_raw_read(struct twl4030_madc_data
> *madc, u8 reg)
> > +{
> > + u8 msb, lsb;
> > +
> > + /*
> > + *For each ADC channel, we have MSB and LSB register pair. MSB
> address
> > + *is always LSB address+1. reg parameter is the addr of LSB
> register
> > + */
> > + msb = twl4030_madc_read(madc, reg + 1);
> > + lsb = twl4030_madc_read(madc, reg);
> > +
> > + return (int)(((msb << 8) | lsb) >> 6);
> > +}
> > +
> > +static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
> > + u8 reg_base, u16 channels, int *buf)
> > +{
> > + int count = 0;
> > + u8 reg, i;
> > +
> > + if (unlikely(!buf))
> > + return 0;
> > +
> > + for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
> > + if (channels & (1<<i)) {
> > + reg = reg_base + 2*i;
> > + buf[i] = twl4030_madc_channel_raw_read(madc,
> reg);
> > + count++;
> > + }
> > + }
> > + return count;
> > +}
> > +
> > +static void twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int
> id)
> > +{
> > + u8 val;
> > +
> > + val = twl4030_madc_read(madc, madc->imr);
> > + val &= ~(1 << id);
> > + twl4030_madc_write(madc, madc->imr, val);
> > +}
> > +
> > +static void twl4030_madc_disable_irq(struct twl4030_madc_data *madc,
> int id)
> > +{
> > + u8 val;
> > +
> > + val = twl4030_madc_read(madc, madc->imr);
> > + val |= (1 << id);
> > + twl4030_madc_write(madc, madc->imr, val);
> > +}
> > +
> > +static irqreturn_t twl4030_madc_irq_handler(int irq, void *_madc)
> > +{
> > + struct twl4030_madc_data *madc = _madc;
> > + u8 isr_val, imr_val;
> > + int i;
> > +
> > + isr_val = twl4030_madc_read(madc, madc->isr);
> > + imr_val = twl4030_madc_read(madc, madc->imr);
> > +
> > + isr_val &= ~imr_val;
> > +
> > + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> > +
> > + if (!(isr_val & (1<<i)))
> > + continue;
> > +
> > + twl4030_madc_disable_irq(madc, i);
> > + madc->requests[i].result_pending = 1;
> > + }
> > +
> > + schedule_work(&madc->ws);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void twl4030_madc_work(struct work_struct *ws)
> > +{
> > + const struct twl4030_madc_conversion_method *method;
> > + struct twl4030_madc_data *madc;
> > + struct twl4030_madc_request *r;
> > + int len, i;
> > +
> > + madc = container_of(ws, struct twl4030_madc_data, ws);
> > + mutex_lock(&madc->lock);
> > +
> > + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> > +
> > + r = &madc->requests[i];
> > +
> > + /* No pending results for this method, move to next one
> */
> > + if (!r->result_pending)
> > + continue;
> > +
> > + method = &twl4030_conversion_methods[r->method];
> > +
> > + /* Read results */
> > + len = twl4030_madc_read_channels(madc, method->rbase,
> > + r->channels, r->rbuf);
> > +
> > + /* Return results to caller */
> > + if (r->func_cb != NULL) {
> > + r->func_cb(len, r->channels, r->rbuf);
> > + r->func_cb = NULL;
> > + }
> > +
> > + /* Free request */
> > + r->result_pending = 0;
> > + r->active = 0;
> > + }
> > +
> > + mutex_unlock(&madc->lock);
> > +}
> > +
> > +static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
> > + struct twl4030_madc_request *req)
> > +{
> > + struct twl4030_madc_request *p;
> > +
> > + p = &madc->requests[req->method];
> > +
> > + memcpy(p, req, sizeof *req);
> > +
> > + twl4030_madc_enable_irq(madc, req->method);
> > +
> > + return 0;
> > +}
> > +
> > +static inline void twl4030_madc_start_conversion(struct
> twl4030_madc_data *madc,
> > + int conv_method)
> > +{
> > + const struct twl4030_madc_conversion_method *method;
> > +
> > + method = &twl4030_conversion_methods[conv_method];
> > +
> > + switch (conv_method) {
> > + case TWL4030_MADC_SW1:
> > + case TWL4030_MADC_SW2:
> > + twl4030_madc_write(madc, method->ctrl,
> TWL4030_MADC_SW_START);
> > + break;
> > + case TWL4030_MADC_RT:
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +static int twl4030_madc_wait_conversion_ready(
> > + struct twl4030_madc_data *madc,
> > + unsigned int timeout_ms, u8 status_reg)
> > +{
> > + unsigned long timeout;
> > +
> > + timeout = jiffies + msecs_to_jiffies(timeout_ms);
> > + do
> > + u8 reg;
> > +
> > + reg = twl4030_madc_read(madc, status_reg);
> > + if (!(reg & TWL4030_MADC_BUSY) && (reg &
> TWL4030_MADC_EOC_SW))
> > + return 0;
> > + } while (!time_after(jiffies, timeout));
> > +
> Does this have to be a hard wait ?
>
It is of the order of 5ms. Conversion takes less than 5s.
Otherwise there will be a time out. If needed we can add
Delay.
> > + return -EAGAIN;
> > +}
> > +
> > +int twl4030_madc_conversion(struct twl4030_madc_request *req)
> > +{
> > + const struct twl4030_madc_conversion_method *method;
> > + u8 ch_msb, ch_lsb;
> > + int ret;
> > +
> > + if (unlikely(!req))
> > + return -EINVAL;
> > +
> > + mutex_lock(&the_madc->lock);
> > +
> > + /* Do we have a conversion request ongoing */
> > + if (the_madc->requests[req->method].active) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > +
> > + ch_msb = (req->channels >> 8) & 0xff;
> > + ch_lsb = req->channels & 0xff;
> > +
> > + method = &twl4030_conversion_methods[req->method];
> > +
> > + /* Select channels to be converted */
> > + twl4030_madc_write(the_madc, method->sel + 1, ch_msb);
> > + twl4030_madc_write(the_madc, method->sel, ch_lsb);
> > +
> > + /* Select averaging for all channels if do_avg is set */
> > + if (req->do_avg) {
> > + twl4030_madc_write(the_madc, method->avg + 1, ch_msb);
> > + twl4030_madc_write(the_madc, method->avg, ch_lsb);
> > + }
> > +
> > + if ((req->type == TWL4030_MADC_IRQ_ONESHOT) && (req->func_cb !=
> NULL)) {
>
> Extra and unnecessary (). Please remove.
>
Will be removed.
> > + twl4030_madc_set_irq(the_madc, req);
> > + twl4030_madc_start_conversion(the_madc, req->method);
> > + the_madc->requests[req->method].active = 1;
> > + ret = 0;
> > + goto out;
> > + }
> > +
> > + /* With RT method we should not be here anymore */
> > + if (req->method == TWL4030_MADC_RT) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + twl4030_madc_start_conversion(the_madc, req->method);
> > + the_madc->requests[req->method].active = 1;
> > +
> > + /* Wait until conversion is ready (ctrl register returns EOC) */
> > + ret = twl4030_madc_wait_conversion_ready(the_madc, 5, method-
> >ctrl);
> > + if (ret) {
> > + dev_dbg(the_madc->hwmon_dev, "conversion timeout!\n");
> > + the_madc->requests[req->method].active = 0;
> > + goto out;
> > + }
> > +
> > + ret = twl4030_madc_read_channels(the_madc, method->rbase, req-
> >channels,
> > + req->rbuf);
> > +
> > + the_madc->requests[req->method].active = 0;
> > +
> > +out:
> > + mutex_unlock(&the_madc->lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(twl4030_madc_conversion);
> > +
> If this function is going to be called from external code, it should not
> really be defined here. I would suggest to move it to a global location
> such as
> mfd instead, including all related functions.
>
> The existence of this function export indicates that another non-hwmon
> driver depends on this one, which should not really be the case. Another
> reason to have a separate common driver instead, and mfd might just be the
> place for it.
Few kernel modules need to perform ADC conversion to measure battery
voltage, battery temperature, VBUS voltage via twl4030_madc_conversion.
the_madc is needed as those drivers will not have this device pointer.
>
> > +static int twl4030_madc_set_current_generator(struct twl4030_madc_data
> *madc,
> > + int chan, int on)
> > +{
> > + int ret;
> > + u8 regval;
> > +
> > + /*
> > + * Current generator is only available for ADCIN0 and ADCIN1.
> NB:
> > + * ADCIN1 current generator only works when AC or VBUS is
> present
> > + */
> > + if (chan > 1)
> > + return EINVAL;
> > +
> So you don't trust your own code and return EINVAL even though this is the
> same module,
> but then the caller doesn't check the return value. That doesn't make any
> sense.
> First, if you don't trust your code, make this condition a BUG.
> Second, check the return value if you return an error.
This check is not required and it will be removed.
>
> > + ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> > + ®val, TWL4030_BCI_BCICTL1);
> > + if (on)
> > + regval |= (chan) ? TWL4030_BCI_ITHEN :
> TWL4030_BCI_TYPEN;
> > + else
> > + regval &= (chan) ? ~TWL4030_BCI_ITHEN :
> ~TWL4030_BCI_TYPEN;
>
> (chan) in () does not provide any value. Please remove the extra ().
>
It will be removed.
> > + ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> > + regval, TWL4030_BCI_BCICTL1);
> > +
> > + return ret;
> > +}
> > +
> > +static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int
> on)
> > +{
> > + u8 regval;
> > +
> > + regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
> > + if (on)
> > + regval |= TWL4030_MADC_MADCON;
> > + else
> > + regval &= ~TWL4030_MADC_MADCON;
> > + twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
> > +
> > + return 0;
> > +}
> > +
> > +static long twl4030_madc_ioctl(struct file *filp, unsigned int cmd,
> > + unsigned long arg)
>
> Highly unusual function for a hwmon driver. Another indication that there
> should be a non-hwmon generic component instead to provide adc readings.
>
Is there a generic IOCTL provided for Hwmon class of drivers?
So that we can replace the specific one by the generic IOCTL.
> > +{
> > + struct twl4030_madc_user_parms par;
> > + int val, ret;
> > +
> > + ret = copy_from_user(&par, (void __user *) arg, sizeof(par));
> > + if (ret) {
> > + dev_dbg(the_madc->hwmon_dev, "copy_from_user: %d\n",
> ret);
> > + return -EACCES;
> > + }
> > +
> > + switch (cmd) {
> > + case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> > + struct twl4030_madc_request req;
> > + if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
> > + return -EINVAL;
> > +
> > + req.channels = (1 << par.channel);
> > + req.do_avg = par.average;
> > + req.method = TWL4030_MADC_SW1;
> > + req.func_cb = NULL;
> > +
> > + val = twl4030_madc_conversion(&req);
> > + if (val <= 0) {
> > + par.status = -1;
> > + } else {
> > + par.status = 0;
> > + par.result = (u16)req.rbuf[par.channel];
> > + }
> > + break;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = copy_to_user((void __user *) arg, &par, sizeof(par));
> > + if (ret) {
> > + dev_dbg(the_madc->hwmon_dev, "copy_to_user: %d\n", ret);
> > + return -EACCES;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct file_operations twl4030_madc_fileops = {
> > + .owner = THIS_MODULE,
> > + .unlocked_ioctl = twl4030_madc_ioctl
> > +};
> > +
> > +static struct miscdevice twl4030_madc_device = {
> > + .minor = MISC_DYNAMIC_MINOR,
> > + .name = "twl4030-madc",
> > + .fops = &twl4030_madc_fileops
> > +};
> > +
> > +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 0);
> > +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 1);
> > +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 2);
> > +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 3);
> > +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 4);
> > +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 5);
> > +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 6);
> > +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 7);
> > +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 8);
> > +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 9);
> > +static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 10);
> > +static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 11);
> > +static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 12);
> > +static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 13);
> > +static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 14);
> > +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 15);
>
> " , " --> ", "
> > +
> There is no write function. Why S_IWUSR ?
>
Yes S_IWUSR is unnecessary. I will remove this.
> > +static struct attribute *twl4030_madc_attributes[] = {
> > + &sensor_dev_attr_in0_input.dev_attr.attr,
> > + &sensor_dev_attr_in1_input.dev_attr.attr,
> > + &sensor_dev_attr_in2_input.dev_attr.attr,
> > + &sensor_dev_attr_in3_input.dev_attr.attr,
> > + &sensor_dev_attr_in4_input.dev_attr.attr,
> > + &sensor_dev_attr_in5_input.dev_attr.attr,
> > + &sensor_dev_attr_in6_input.dev_attr.attr,
> > + &sensor_dev_attr_in7_input.dev_attr.attr,
> > + &sensor_dev_attr_in8_input.dev_attr.attr,
> > + &sensor_dev_attr_in9_input.dev_attr.attr,
> > + &sensor_dev_attr_in10_input.dev_attr.attr,
> > + &sensor_dev_attr_in11_input.dev_attr.attr,
> > + &sensor_dev_attr_in12_input.dev_attr.attr,
> > + &sensor_dev_attr_in13_input.dev_attr.attr,
> > + &sensor_dev_attr_in14_input.dev_attr.attr,
> > + &sensor_dev_attr_in15_input.dev_attr.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group twl4030_madc_group = {
> > + .attrs = twl4030_madc_attributes,
> > +};
> > +
> > +static int __init twl4030_madc_probe(struct platform_device *pdev)
> > +{
> > + struct twl4030_madc_data *madc;
> > + struct twl4030_madc_platform_data *pdata = pdev-
> >dev.platform_data;
> > + int ret;
> > + int status;
> > + u8 regval;
> > +
> > + madc = kzalloc(sizeof *madc, GFP_KERNEL);
> > + if (!madc)
> > + return -ENOMEM;
> > +
> > + if (!pdata) {
> > + dev_dbg(&pdev->dev, "platform_data not available\n");
> > + ret = -EINVAL;
> > + goto err_pdata;
> > + }
> > + madc->hwmon_dev = &pdev->dev;
> > + madc->imr = (pdata->irq_line == 1) ?
> > + TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
> > + madc->isr = (pdata->irq_line == 1) ?
> > + TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
> > + madc->hwmon_dev = hwmon_device_register(&pdev->dev);
>
> That is usually done last, after everything else works.
> And you don't remove it if anything else failed. There should at least
> be a hwmon_device_unregister() in the error path.
>
Yes I will call the register in the end and unregister when required.
>
> > + twl4030_madc_set_power(madc, 1);
>
> Function is defines as int and can presumably return an error. So you
> should check it.
> If there is no reason to return an error, please define function as void.
>
Error check will be done.
> > + twl4030_madc_set_current_generator(madc, 0, 1);
> > +
> Same here.
>
> > + ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> > + ®val, TWL4030_BCI_BCICTL1);
> > +
> Return value check missing.
>
> > + regval |= TWL4030_BCI_MESBAT;
> > +
> > + ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> > + regval, TWL4030_BCI_BCICTL1);
>
> Return value check missing ? If you don't want to check it, there is
> no reason to assign it to ret.
I will check the return value.
>
> > +
> > + ret = request_threaded_irq(platform_get_irq(pdev, 0), NULL,
> > + twl4030_madc_irq_handler , IRQF_TRIGGER_FALLING |
>
> " , " --> ", "
>
I will run the Lindent.
> > + IRQF_TRIGGER_RISING, "twl4030_madc", madc);
> > + if (ret) {
> > + dev_dbg(&pdev->dev, "could not request irq\n");
> > + goto err_irq;
> > + }
> > +
> > + platform_set_drvdata(pdev, madc);
> > + mutex_init(&madc->lock);
> > + ret = sysfs_create_group(&pdev->dev.kobj, &twl4030_madc_group);
> > + if (ret)
> > + goto err_misc;
> > +
> > + if (IS_ERR(madc->hwmon_dev)) {
> > + dev_err(&pdev->dev, "hwmon_device_register failed.\n");
> > + status = PTR_ERR(madc->hwmon_dev);
> > + goto err_misc;
> > + }
> You abort installation if hwmon registration failed, but you don't clean
> up, leaving the sysfs files existing but free madc. At the very least
> you'll have dangling sysfs files, which will cause a NULL pointer
> access when the sysfs read functions access use the_madc which will be
> NULL.
>
> Please have a look at the error path and clean it up.
>
I will clean up.
> > + INIT_WORK(&madc->ws, twl4030_madc_work);
> > + the_madc = madc;
> > +
> The use of the_madc instead of using platform_get_drvdata() is quite
> unusual
> and suggests some structural problem with the driver.
> Please check if you can use platform_get_drvdata() instead. If you can
> use it, please do. If you can not, I would suggest to restructure the code
> so you can.
>
> > + return 0;
> > +
> > +err_irq:
> > +err_misc:
> > +err_pdata:
>
> Multiple labels pointing to the same location does not make sense. Please
> use only one label.
> Or add cleanup functionality to the others if needed.
>
I will cleanup.
> > + kfree(madc);
> > +
> > + return ret;
> > +}
> > +
> > +static int __exit twl4030_madc_remove(struct platform_device *pdev)
> > +{
> > + struct twl4030_madc_data *madc = platform_get_drvdata(pdev);
> > +
> > + twl4030_madc_set_power(madc, 0);
> > + twl4030_madc_set_current_generator(madc, 0, 0);
> > + free_irq(platform_get_irq(pdev, 0), madc);
> > + cancel_work_sync(&madc->ws);
> > +
> hwmon_device_unregister() missing. sysfs attribute files not removed.
>
I will add the hwmon_device_unregister() call.
> And how do you determine if any other module needing the exported function
> is still loaded ?
>
> > + return 0;
> > +}
> > +
> > +static struct platform_driver twl4030_madc_driver = {
> > + .probe = twl4030_madc_probe,
> > + .remove = __exit_p(twl4030_madc_remove),
> > + .driver = {
> > + .name = "twl4030_madc",
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +static int __init twl4030_madc_init(void)
> > +{
> > + return platform_driver_register(&twl4030_madc_driver);
> > +}
> > +module_init(twl4030_madc_init);
> > +
> > +static void __exit twl4030_madc_exit(void)
> > +{
> > + platform_driver_unregister(&twl4030_madc_driver);
> > +}
> > +module_exit(twl4030_madc_exit);
> > +
> > +MODULE_DESCRIPTION("twl4030 ADC driver");
> > +MODULE_LICENSE("GPL");
>
> MODULE_AUTHOR would be nice to have too.
>
I will add it.
> > +
>
> Extra blank line here, causing a whitespace error when integrating. Please
> remove.
>
> > diff --git a/include/linux/i2c/twl4030-madc.h
> b/include/linux/i2c/twl4030-madc.h
> > new file mode 100644
> > index 0000000..1e46ff1
> > --- /dev/null
> > +++ b/include/linux/i2c/twl4030-madc.h
> > @@ -0,0 +1,118 @@
> > +/*
> > + * 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
> > + *
> > + */
> > +
> > +#ifndef _TWL4030_MADC_H
> > +#define _TWL4030_MADC_H
> > +
> > +struct twl4030_madc_conversion_method {
> > + u8 sel;
> > + u8 avg;
> > + u8 rbase;
> > + u8 ctrl;
> > +};
> > +
> > +#define TWL4030_MADC_MAX_CHANNELS 16
> > +
> > +struct twl4030_madc_request {
> > + u16 channels;
> > + u16 do_avg;
> > + u16 method;
> > + u16 type;
> > + bool active;
> > + bool result_pending;
> > + int rbuf[TWL4030_MADC_MAX_CHANNELS];
> > + void (*func_cb)(int len, int channels, int *buf);
> > +};
> > +
> > +enum conversion_methods {
> > + TWL4030_MADC_RT,
> > + TWL4030_MADC_SW1,
> > + TWL4030_MADC_SW2,
> > + TWL4030_MADC_NUM_METHODS
> > +};
> > +
> > +enum sample_type {
> > + TWL4030_MADC_WAIT,
> > + TWL4030_MADC_IRQ_ONESHOT,
> > + TWL4030_MADC_IRQ_REARM
> > +};
> > +
> > +#define TWL4030_MADC_CTRL1 0x00
> > +#define TWL4030_MADC_CTRL2 0x01
> > +
> > +#define TWL4030_MADC_RTSELECT_LSB 0x02
> > +#define TWL4030_MADC_SW1SELECT_LSB 0x06
> > +#define TWL4030_MADC_SW2SELECT_LSB 0x0A
> > +
> > +#define TWL4030_MADC_RTAVERAGE_LSB 0x04
> > +#define TWL4030_MADC_SW1AVERAGE_LSB 0x08
> > +#define TWL4030_MADC_SW2AVERAGE_LSB 0x0C
> > +
> > +#define TWL4030_MADC_CTRL_SW1 0x12
> > +#define TWL4030_MADC_CTRL_SW2 0x13
> > +
> > +#define TWL4030_MADC_RTCH0_LSB 0x17
> > +#define TWL4030_MADC_GPCH0_LSB 0x37
> > +
> > +#define TWL4030_MADC_MADCON (1<<0) /* MADC power on */
> > +#define TWL4030_MADC_BUSY (1<<0) /* MADC busy */
> > +#define TWL4030_MADC_EOC_SW (1<<1) /* MADC conversion
> completion */
> > +#define TWL4030_MADC_SW_START (1<<5) /* MADC SWx start
> conversion */
> > +
> > +#define TWL4030_MADC_ADCIN0 (1<<0)
> > +#define TWL4030_MADC_ADCIN1 (1<<1)
> > +#define TWL4030_MADC_ADCIN2 (1<<2)
> > +#define TWL4030_MADC_ADCIN3 (1<<3)
> > +#define TWL4030_MADC_ADCIN4 (1<<4)
> > +#define TWL4030_MADC_ADCIN5 (1<<5)
> > +#define TWL4030_MADC_ADCIN6 (1<<6)
> > +#define TWL4030_MADC_ADCIN7 (1<<7)
> > +#define TWL4030_MADC_ADCIN8 (1<<8)
> > +#define TWL4030_MADC_ADCIN9 (1<<9)
> > +#define TWL4030_MADC_ADCIN10 (1<<10)
> > +#define TWL4030_MADC_ADCIN11 (1<<11)
> > +#define TWL4030_MADC_ADCIN12 (1<<12)
> > +#define TWL4030_MADC_ADCIN13 (1<<13)
> > +#define TWL4030_MADC_ADCIN14 (1<<14)
> > +#define TWL4030_MADC_ADCIN15 (1<<15)
> > +
> > +/* Fixed channels */
> > +#define TWL4030_MADC_BTEMP TWL4030_MADC_ADCIN1
> > +#define TWL4030_MADC_VBUS TWL4030_MADC_ADCIN8
> > +#define TWL4030_MADC_VBKB TWL4030_MADC_ADCIN9
> > +#define TWL4030_MADC_ICHG TWL4030_MADC_ADCIN10
> > +#define TWL4030_MADC_VCHG TWL4030_MADC_ADCIN11
> > +#define TWL4030_MADC_VBAT TWL4030_MADC_ADCIN12
> > +
> > +#define TWL4030_BCI_BCICTL1 0x23
> > +#define TWL4030_BCI_MESBAT (1<<1)
> > +#define TWL4030_BCI_TYPEN (1<<4)
> > +#define TWL4030_BCI_ITHEN (1<<3)
> > +
> > +#define TWL4030_MADC_IOC_MAGIC '`'
> > +#define TWL4030_MADC_IOCX_ADC_RAW_READ
> _IO(TWL4030_MADC_IOC_MAGIC, 0)
> > +
> > +struct twl4030_madc_user_parms {
> > + int channel;
> > + int average;
> > + int status;
> > + u16 result;
> > +};
> > +
> > +int twl4030_madc_conversion(struct twl4030_madc_request *conv);
> > +
> > +#endif
> > --
> > 1.7.0.4
> >
> >
> > _______________________________________________
> > lm-sensors mailing list
> > lm-sensors@lm-sensors.org
> > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
@ 2010-09-20 10:46 ` J, KEERTHY
0 siblings, 0 replies; 12+ messages in thread
From: J, KEERTHY @ 2010-09-20 10:46 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-omap, lm-sensors, Krishnamoorthy, Balaji T
> -----Original Message-----
> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> Sent: Thursday, September 16, 2010 8:48 PM
> To: J, KEERTHY
> Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy,
> Balaji T
> Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
> madc module
>
> On Thu, Sep 16, 2010 at 06:23:24AM -0400, Keerthy wrote:
> > MADC driver is added under hwmon drivers. This driver monitors the real
> time
> > conversion of analog signals like battery temperature,
> > battery type, battery level etc. User can also ask for the conversion on
> a
> > particular channel using the sysfs nodes.
> >
> Number of comments inline. This is not a complete review; I think there
> is some cleanup to be done before that is possible.
>
> Please run the driver through Lindent. While I understand that people
> prefer
> their personal touch, it makes it easier to have a single coding style in
> the kernel.
>
I will run Lindent.
> I commented on this a couple of times below - the driver mixes generic ADC
> reading functions with hwmon functionality. Generic ADC reading
> functionality
> should be moved into another driver, possibly to mfd.
>
I am addressing the comment below.
> > Several people have contributed to this driver on the linux-omap list.
> >
> > Signed-off-by: Keerthy <j-keerthy@ti.com>
> > ---
> > drivers/hwmon/twl4030-madc.c | 584
> ++++++++++++++++++++++++++++++++++++++
> > include/linux/i2c/twl4030-madc.h | 118 ++++++++
> > 2 files changed, 702 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/hwmon/twl4030-madc.c
> > create mode 100644 include/linux/i2c/twl4030-madc.h
> >
> > diff --git a/drivers/hwmon/twl4030-madc.c b/drivers/hwmon/twl4030-madc.c
> > new file mode 100644
> > index 0000000..b96fccd
> > --- /dev/null
> > +++ b/drivers/hwmon/twl4030-madc.c
> > @@ -0,0 +1,584 @@
> > +/*
>
> It is customary to add a brief description as well as the author here.
>
I will add it.
> > + * 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/types.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/fs.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c/twl.h>
> > +#include <linux/i2c/twl4030-madc.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +
> > +#include <linux/uaccess.h>
> > +
> > +
> Extra blank line. Lindent will remove it.
>
I will run the Lindent.
> > +struct twl4030_madc_data {
> > + struct device *hwmon_dev;
> > + struct mutex lock;
> > + struct work_struct ws;
> > + struct twl4030_madc_request
> requests[TWL4030_MADC_NUM_METHODS];
> > + int imr;
> > + int isr;
> > +};
> > +
> > +static struct twl4030_madc_data *the_madc;
> > +
> This variable should not exist. See other comments later.
>
> > +static ssize_t madc_read(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + struct sensor_device_attribute *attr > to_sensor_dev_attr(devattr);
> > + struct twl4030_madc_request req;
> > + int status;
> > + unsigned long val;
> > +
> > + req.channels = (1 << attr->index);
> > + req.method = TWL4030_MADC_SW2;
> > + req.func_cb = NULL;
> > +
> > + val = twl4030_madc_conversion(&req);
> > + if (likely(val >= 0))
> > + val = req.rbuf[attr->index];
> > + else
> > + return val;
> > + status = sprintf(buf, "%lu\n", val);
> > + return status;
> > +}
> > +
> > +static
> > +const struct twl4030_madc_conversion_method
> twl4030_conversion_methods[] = {
> > + [TWL4030_MADC_RT] = {
> > + .sel = TWL4030_MADC_RTSELECT_LSB,
> > + .avg = TWL4030_MADC_RTAVERAGE_LSB,
> > + .rbase = TWL4030_MADC_RTCH0_LSB,
> > + },
> > + [TWL4030_MADC_SW1] = {
> > + .sel = TWL4030_MADC_SW1SELECT_LSB,
> > + .avg = TWL4030_MADC_SW1AVERAGE_LSB,
> > + .rbase = TWL4030_MADC_GPCH0_LSB,
> > + .ctrl = TWL4030_MADC_CTRL_SW1,
> > + },
> > + [TWL4030_MADC_SW2] = {
> > + .sel = TWL4030_MADC_SW2SELECT_LSB,
> > + .avg = TWL4030_MADC_SW2AVERAGE_LSB,
> > + .rbase = TWL4030_MADC_GPCH0_LSB,
> > + .ctrl = TWL4030_MADC_CTRL_SW2,
> > + },
> > +};
> > +
> > +static int twl4030_madc_read(struct twl4030_madc_data *madc, u8 reg)
> > +{
> > + int ret;
> > + u8 val;
> > +
> > + ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, &val, reg);
>
> Structural comment. If there is a i2c master, why don't you have an I2C
> master driver instead of calling a platform dependent i2c function ?
> I see this function called from all over the place, so maybe there is a
> reason. Just looks odd, though.
>
There are many small modules in the same i2c slave address 0x4A.
twl4030_keypad is one such module which uses the above function.
All the drivers call the above function with their individual offsets.
> > + if (ret) {
> > + dev_dbg(madc->hwmon_dev, "unable to read register
> 0x%X\n", reg);
> > + return ret;
> > + }
> > +
> > + return val;
> > +}
> > +
> > +static void twl4030_madc_write(struct twl4030_madc_data *madc, u8 reg,
> u8 val)
> > +{
> > + int ret;
> > +
> > + ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, reg);
> > + if (ret)
> > + dev_err(madc->hwmon_dev,
> > + "unable to write register 0x%X\n", reg);
> > +}
> > +
> > +static int twl4030_madc_channel_raw_read(struct twl4030_madc_data
> *madc, u8 reg)
> > +{
> > + u8 msb, lsb;
> > +
> > + /*
> > + *For each ADC channel, we have MSB and LSB register pair. MSB
> address
> > + *is always LSB address+1. reg parameter is the addr of LSB
> register
> > + */
> > + msb = twl4030_madc_read(madc, reg + 1);
> > + lsb = twl4030_madc_read(madc, reg);
> > +
> > + return (int)(((msb << 8) | lsb) >> 6);
> > +}
> > +
> > +static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
> > + u8 reg_base, u16 channels, int *buf)
> > +{
> > + int count = 0;
> > + u8 reg, i;
> > +
> > + if (unlikely(!buf))
> > + return 0;
> > +
> > + for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
> > + if (channels & (1<<i)) {
> > + reg = reg_base + 2*i;
> > + buf[i] = twl4030_madc_channel_raw_read(madc,
> reg);
> > + count++;
> > + }
> > + }
> > + return count;
> > +}
> > +
> > +static void twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int
> id)
> > +{
> > + u8 val;
> > +
> > + val = twl4030_madc_read(madc, madc->imr);
> > + val &= ~(1 << id);
> > + twl4030_madc_write(madc, madc->imr, val);
> > +}
> > +
> > +static void twl4030_madc_disable_irq(struct twl4030_madc_data *madc,
> int id)
> > +{
> > + u8 val;
> > +
> > + val = twl4030_madc_read(madc, madc->imr);
> > + val |= (1 << id);
> > + twl4030_madc_write(madc, madc->imr, val);
> > +}
> > +
> > +static irqreturn_t twl4030_madc_irq_handler(int irq, void *_madc)
> > +{
> > + struct twl4030_madc_data *madc = _madc;
> > + u8 isr_val, imr_val;
> > + int i;
> > +
> > + isr_val = twl4030_madc_read(madc, madc->isr);
> > + imr_val = twl4030_madc_read(madc, madc->imr);
> > +
> > + isr_val &= ~imr_val;
> > +
> > + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> > +
> > + if (!(isr_val & (1<<i)))
> > + continue;
> > +
> > + twl4030_madc_disable_irq(madc, i);
> > + madc->requests[i].result_pending = 1;
> > + }
> > +
> > + schedule_work(&madc->ws);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void twl4030_madc_work(struct work_struct *ws)
> > +{
> > + const struct twl4030_madc_conversion_method *method;
> > + struct twl4030_madc_data *madc;
> > + struct twl4030_madc_request *r;
> > + int len, i;
> > +
> > + madc = container_of(ws, struct twl4030_madc_data, ws);
> > + mutex_lock(&madc->lock);
> > +
> > + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> > +
> > + r = &madc->requests[i];
> > +
> > + /* No pending results for this method, move to next one
> */
> > + if (!r->result_pending)
> > + continue;
> > +
> > + method = &twl4030_conversion_methods[r->method];
> > +
> > + /* Read results */
> > + len = twl4030_madc_read_channels(madc, method->rbase,
> > + r->channels, r->rbuf);
> > +
> > + /* Return results to caller */
> > + if (r->func_cb != NULL) {
> > + r->func_cb(len, r->channels, r->rbuf);
> > + r->func_cb = NULL;
> > + }
> > +
> > + /* Free request */
> > + r->result_pending = 0;
> > + r->active = 0;
> > + }
> > +
> > + mutex_unlock(&madc->lock);
> > +}
> > +
> > +static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
> > + struct twl4030_madc_request *req)
> > +{
> > + struct twl4030_madc_request *p;
> > +
> > + p = &madc->requests[req->method];
> > +
> > + memcpy(p, req, sizeof *req);
> > +
> > + twl4030_madc_enable_irq(madc, req->method);
> > +
> > + return 0;
> > +}
> > +
> > +static inline void twl4030_madc_start_conversion(struct
> twl4030_madc_data *madc,
> > + int conv_method)
> > +{
> > + const struct twl4030_madc_conversion_method *method;
> > +
> > + method = &twl4030_conversion_methods[conv_method];
> > +
> > + switch (conv_method) {
> > + case TWL4030_MADC_SW1:
> > + case TWL4030_MADC_SW2:
> > + twl4030_madc_write(madc, method->ctrl,
> TWL4030_MADC_SW_START);
> > + break;
> > + case TWL4030_MADC_RT:
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +static int twl4030_madc_wait_conversion_ready(
> > + struct twl4030_madc_data *madc,
> > + unsigned int timeout_ms, u8 status_reg)
> > +{
> > + unsigned long timeout;
> > +
> > + timeout = jiffies + msecs_to_jiffies(timeout_ms);
> > + do
> > + u8 reg;
> > +
> > + reg = twl4030_madc_read(madc, status_reg);
> > + if (!(reg & TWL4030_MADC_BUSY) && (reg &
> TWL4030_MADC_EOC_SW))
> > + return 0;
> > + } while (!time_after(jiffies, timeout));
> > +
> Does this have to be a hard wait ?
>
It is of the order of 5ms. Conversion takes less than 5s.
Otherwise there will be a time out. If needed we can add
Delay.
> > + return -EAGAIN;
> > +}
> > +
> > +int twl4030_madc_conversion(struct twl4030_madc_request *req)
> > +{
> > + const struct twl4030_madc_conversion_method *method;
> > + u8 ch_msb, ch_lsb;
> > + int ret;
> > +
> > + if (unlikely(!req))
> > + return -EINVAL;
> > +
> > + mutex_lock(&the_madc->lock);
> > +
> > + /* Do we have a conversion request ongoing */
> > + if (the_madc->requests[req->method].active) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > +
> > + ch_msb = (req->channels >> 8) & 0xff;
> > + ch_lsb = req->channels & 0xff;
> > +
> > + method = &twl4030_conversion_methods[req->method];
> > +
> > + /* Select channels to be converted */
> > + twl4030_madc_write(the_madc, method->sel + 1, ch_msb);
> > + twl4030_madc_write(the_madc, method->sel, ch_lsb);
> > +
> > + /* Select averaging for all channels if do_avg is set */
> > + if (req->do_avg) {
> > + twl4030_madc_write(the_madc, method->avg + 1, ch_msb);
> > + twl4030_madc_write(the_madc, method->avg, ch_lsb);
> > + }
> > +
> > + if ((req->type = TWL4030_MADC_IRQ_ONESHOT) && (req->func_cb !> NULL)) {
>
> Extra and unnecessary (). Please remove.
>
Will be removed.
> > + twl4030_madc_set_irq(the_madc, req);
> > + twl4030_madc_start_conversion(the_madc, req->method);
> > + the_madc->requests[req->method].active = 1;
> > + ret = 0;
> > + goto out;
> > + }
> > +
> > + /* With RT method we should not be here anymore */
> > + if (req->method = TWL4030_MADC_RT) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + twl4030_madc_start_conversion(the_madc, req->method);
> > + the_madc->requests[req->method].active = 1;
> > +
> > + /* Wait until conversion is ready (ctrl register returns EOC) */
> > + ret = twl4030_madc_wait_conversion_ready(the_madc, 5, method-
> >ctrl);
> > + if (ret) {
> > + dev_dbg(the_madc->hwmon_dev, "conversion timeout!\n");
> > + the_madc->requests[req->method].active = 0;
> > + goto out;
> > + }
> > +
> > + ret = twl4030_madc_read_channels(the_madc, method->rbase, req-
> >channels,
> > + req->rbuf);
> > +
> > + the_madc->requests[req->method].active = 0;
> > +
> > +out:
> > + mutex_unlock(&the_madc->lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(twl4030_madc_conversion);
> > +
> If this function is going to be called from external code, it should not
> really be defined here. I would suggest to move it to a global location
> such as
> mfd instead, including all related functions.
>
> The existence of this function export indicates that another non-hwmon
> driver depends on this one, which should not really be the case. Another
> reason to have a separate common driver instead, and mfd might just be the
> place for it.
Few kernel modules need to perform ADC conversion to measure battery
voltage, battery temperature, VBUS voltage via twl4030_madc_conversion.
the_madc is needed as those drivers will not have this device pointer.
>
> > +static int twl4030_madc_set_current_generator(struct twl4030_madc_data
> *madc,
> > + int chan, int on)
> > +{
> > + int ret;
> > + u8 regval;
> > +
> > + /*
> > + * Current generator is only available for ADCIN0 and ADCIN1.
> NB:
> > + * ADCIN1 current generator only works when AC or VBUS is
> present
> > + */
> > + if (chan > 1)
> > + return EINVAL;
> > +
> So you don't trust your own code and return EINVAL even though this is the
> same module,
> but then the caller doesn't check the return value. That doesn't make any
> sense.
> First, if you don't trust your code, make this condition a BUG.
> Second, check the return value if you return an error.
This check is not required and it will be removed.
>
> > + ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> > + ®val, TWL4030_BCI_BCICTL1);
> > + if (on)
> > + regval |= (chan) ? TWL4030_BCI_ITHEN :
> TWL4030_BCI_TYPEN;
> > + else
> > + regval &= (chan) ? ~TWL4030_BCI_ITHEN :
> ~TWL4030_BCI_TYPEN;
>
> (chan) in () does not provide any value. Please remove the extra ().
>
It will be removed.
> > + ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> > + regval, TWL4030_BCI_BCICTL1);
> > +
> > + return ret;
> > +}
> > +
> > +static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int
> on)
> > +{
> > + u8 regval;
> > +
> > + regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
> > + if (on)
> > + regval |= TWL4030_MADC_MADCON;
> > + else
> > + regval &= ~TWL4030_MADC_MADCON;
> > + twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
> > +
> > + return 0;
> > +}
> > +
> > +static long twl4030_madc_ioctl(struct file *filp, unsigned int cmd,
> > + unsigned long arg)
>
> Highly unusual function for a hwmon driver. Another indication that there
> should be a non-hwmon generic component instead to provide adc readings.
>
Is there a generic IOCTL provided for Hwmon class of drivers?
So that we can replace the specific one by the generic IOCTL.
> > +{
> > + struct twl4030_madc_user_parms par;
> > + int val, ret;
> > +
> > + ret = copy_from_user(&par, (void __user *) arg, sizeof(par));
> > + if (ret) {
> > + dev_dbg(the_madc->hwmon_dev, "copy_from_user: %d\n",
> ret);
> > + return -EACCES;
> > + }
> > +
> > + switch (cmd) {
> > + case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> > + struct twl4030_madc_request req;
> > + if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
> > + return -EINVAL;
> > +
> > + req.channels = (1 << par.channel);
> > + req.do_avg = par.average;
> > + req.method = TWL4030_MADC_SW1;
> > + req.func_cb = NULL;
> > +
> > + val = twl4030_madc_conversion(&req);
> > + if (val <= 0) {
> > + par.status = -1;
> > + } else {
> > + par.status = 0;
> > + par.result = (u16)req.rbuf[par.channel];
> > + }
> > + break;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = copy_to_user((void __user *) arg, &par, sizeof(par));
> > + if (ret) {
> > + dev_dbg(the_madc->hwmon_dev, "copy_to_user: %d\n", ret);
> > + return -EACCES;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct file_operations twl4030_madc_fileops = {
> > + .owner = THIS_MODULE,
> > + .unlocked_ioctl = twl4030_madc_ioctl
> > +};
> > +
> > +static struct miscdevice twl4030_madc_device = {
> > + .minor = MISC_DYNAMIC_MINOR,
> > + .name = "twl4030-madc",
> > + .fops = &twl4030_madc_fileops
> > +};
> > +
> > +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 0);
> > +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 1);
> > +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 2);
> > +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 3);
> > +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 4);
> > +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 5);
> > +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 6);
> > +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 7);
> > +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 8);
> > +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO|S_IWUSR , madc_read, NULL,
> 9);
> > +static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 10);
> > +static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 11);
> > +static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 12);
> > +static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 13);
> > +static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 14);
> > +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO|S_IWUSR , madc_read,
> NULL, 15);
>
> " , " --> ", "
> > +
> There is no write function. Why S_IWUSR ?
>
Yes S_IWUSR is unnecessary. I will remove this.
> > +static struct attribute *twl4030_madc_attributes[] = {
> > + &sensor_dev_attr_in0_input.dev_attr.attr,
> > + &sensor_dev_attr_in1_input.dev_attr.attr,
> > + &sensor_dev_attr_in2_input.dev_attr.attr,
> > + &sensor_dev_attr_in3_input.dev_attr.attr,
> > + &sensor_dev_attr_in4_input.dev_attr.attr,
> > + &sensor_dev_attr_in5_input.dev_attr.attr,
> > + &sensor_dev_attr_in6_input.dev_attr.attr,
> > + &sensor_dev_attr_in7_input.dev_attr.attr,
> > + &sensor_dev_attr_in8_input.dev_attr.attr,
> > + &sensor_dev_attr_in9_input.dev_attr.attr,
> > + &sensor_dev_attr_in10_input.dev_attr.attr,
> > + &sensor_dev_attr_in11_input.dev_attr.attr,
> > + &sensor_dev_attr_in12_input.dev_attr.attr,
> > + &sensor_dev_attr_in13_input.dev_attr.attr,
> > + &sensor_dev_attr_in14_input.dev_attr.attr,
> > + &sensor_dev_attr_in15_input.dev_attr.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group twl4030_madc_group = {
> > + .attrs = twl4030_madc_attributes,
> > +};
> > +
> > +static int __init twl4030_madc_probe(struct platform_device *pdev)
> > +{
> > + struct twl4030_madc_data *madc;
> > + struct twl4030_madc_platform_data *pdata = pdev-
> >dev.platform_data;
> > + int ret;
> > + int status;
> > + u8 regval;
> > +
> > + madc = kzalloc(sizeof *madc, GFP_KERNEL);
> > + if (!madc)
> > + return -ENOMEM;
> > +
> > + if (!pdata) {
> > + dev_dbg(&pdev->dev, "platform_data not available\n");
> > + ret = -EINVAL;
> > + goto err_pdata;
> > + }
> > + madc->hwmon_dev = &pdev->dev;
> > + madc->imr = (pdata->irq_line = 1) ?
> > + TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
> > + madc->isr = (pdata->irq_line = 1) ?
> > + TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
> > + madc->hwmon_dev = hwmon_device_register(&pdev->dev);
>
> That is usually done last, after everything else works.
> And you don't remove it if anything else failed. There should at least
> be a hwmon_device_unregister() in the error path.
>
Yes I will call the register in the end and unregister when required.
>
> > + twl4030_madc_set_power(madc, 1);
>
> Function is defines as int and can presumably return an error. So you
> should check it.
> If there is no reason to return an error, please define function as void.
>
Error check will be done.
> > + twl4030_madc_set_current_generator(madc, 0, 1);
> > +
> Same here.
>
> > + ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> > + ®val, TWL4030_BCI_BCICTL1);
> > +
> Return value check missing.
>
> > + regval |= TWL4030_BCI_MESBAT;
> > +
> > + ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> > + regval, TWL4030_BCI_BCICTL1);
>
> Return value check missing ? If you don't want to check it, there is
> no reason to assign it to ret.
I will check the return value.
>
> > +
> > + ret = request_threaded_irq(platform_get_irq(pdev, 0), NULL,
> > + twl4030_madc_irq_handler , IRQF_TRIGGER_FALLING |
>
> " , " --> ", "
>
I will run the Lindent.
> > + IRQF_TRIGGER_RISING, "twl4030_madc", madc);
> > + if (ret) {
> > + dev_dbg(&pdev->dev, "could not request irq\n");
> > + goto err_irq;
> > + }
> > +
> > + platform_set_drvdata(pdev, madc);
> > + mutex_init(&madc->lock);
> > + ret = sysfs_create_group(&pdev->dev.kobj, &twl4030_madc_group);
> > + if (ret)
> > + goto err_misc;
> > +
> > + if (IS_ERR(madc->hwmon_dev)) {
> > + dev_err(&pdev->dev, "hwmon_device_register failed.\n");
> > + status = PTR_ERR(madc->hwmon_dev);
> > + goto err_misc;
> > + }
> You abort installation if hwmon registration failed, but you don't clean
> up, leaving the sysfs files existing but free madc. At the very least
> you'll have dangling sysfs files, which will cause a NULL pointer
> access when the sysfs read functions access use the_madc which will be
> NULL.
>
> Please have a look at the error path and clean it up.
>
I will clean up.
> > + INIT_WORK(&madc->ws, twl4030_madc_work);
> > + the_madc = madc;
> > +
> The use of the_madc instead of using platform_get_drvdata() is quite
> unusual
> and suggests some structural problem with the driver.
> Please check if you can use platform_get_drvdata() instead. If you can
> use it, please do. If you can not, I would suggest to restructure the code
> so you can.
>
> > + return 0;
> > +
> > +err_irq:
> > +err_misc:
> > +err_pdata:
>
> Multiple labels pointing to the same location does not make sense. Please
> use only one label.
> Or add cleanup functionality to the others if needed.
>
I will cleanup.
> > + kfree(madc);
> > +
> > + return ret;
> > +}
> > +
> > +static int __exit twl4030_madc_remove(struct platform_device *pdev)
> > +{
> > + struct twl4030_madc_data *madc = platform_get_drvdata(pdev);
> > +
> > + twl4030_madc_set_power(madc, 0);
> > + twl4030_madc_set_current_generator(madc, 0, 0);
> > + free_irq(platform_get_irq(pdev, 0), madc);
> > + cancel_work_sync(&madc->ws);
> > +
> hwmon_device_unregister() missing. sysfs attribute files not removed.
>
I will add the hwmon_device_unregister() call.
> And how do you determine if any other module needing the exported function
> is still loaded ?
>
> > + return 0;
> > +}
> > +
> > +static struct platform_driver twl4030_madc_driver = {
> > + .probe = twl4030_madc_probe,
> > + .remove = __exit_p(twl4030_madc_remove),
> > + .driver = {
> > + .name = "twl4030_madc",
> > + .owner = THIS_MODULE,
> > + },
> > +};
> > +
> > +static int __init twl4030_madc_init(void)
> > +{
> > + return platform_driver_register(&twl4030_madc_driver);
> > +}
> > +module_init(twl4030_madc_init);
> > +
> > +static void __exit twl4030_madc_exit(void)
> > +{
> > + platform_driver_unregister(&twl4030_madc_driver);
> > +}
> > +module_exit(twl4030_madc_exit);
> > +
> > +MODULE_DESCRIPTION("twl4030 ADC driver");
> > +MODULE_LICENSE("GPL");
>
> MODULE_AUTHOR would be nice to have too.
>
I will add it.
> > +
>
> Extra blank line here, causing a whitespace error when integrating. Please
> remove.
>
> > diff --git a/include/linux/i2c/twl4030-madc.h
> b/include/linux/i2c/twl4030-madc.h
> > new file mode 100644
> > index 0000000..1e46ff1
> > --- /dev/null
> > +++ b/include/linux/i2c/twl4030-madc.h
> > @@ -0,0 +1,118 @@
> > +/*
> > + * 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
> > + *
> > + */
> > +
> > +#ifndef _TWL4030_MADC_H
> > +#define _TWL4030_MADC_H
> > +
> > +struct twl4030_madc_conversion_method {
> > + u8 sel;
> > + u8 avg;
> > + u8 rbase;
> > + u8 ctrl;
> > +};
> > +
> > +#define TWL4030_MADC_MAX_CHANNELS 16
> > +
> > +struct twl4030_madc_request {
> > + u16 channels;
> > + u16 do_avg;
> > + u16 method;
> > + u16 type;
> > + bool active;
> > + bool result_pending;
> > + int rbuf[TWL4030_MADC_MAX_CHANNELS];
> > + void (*func_cb)(int len, int channels, int *buf);
> > +};
> > +
> > +enum conversion_methods {
> > + TWL4030_MADC_RT,
> > + TWL4030_MADC_SW1,
> > + TWL4030_MADC_SW2,
> > + TWL4030_MADC_NUM_METHODS
> > +};
> > +
> > +enum sample_type {
> > + TWL4030_MADC_WAIT,
> > + TWL4030_MADC_IRQ_ONESHOT,
> > + TWL4030_MADC_IRQ_REARM
> > +};
> > +
> > +#define TWL4030_MADC_CTRL1 0x00
> > +#define TWL4030_MADC_CTRL2 0x01
> > +
> > +#define TWL4030_MADC_RTSELECT_LSB 0x02
> > +#define TWL4030_MADC_SW1SELECT_LSB 0x06
> > +#define TWL4030_MADC_SW2SELECT_LSB 0x0A
> > +
> > +#define TWL4030_MADC_RTAVERAGE_LSB 0x04
> > +#define TWL4030_MADC_SW1AVERAGE_LSB 0x08
> > +#define TWL4030_MADC_SW2AVERAGE_LSB 0x0C
> > +
> > +#define TWL4030_MADC_CTRL_SW1 0x12
> > +#define TWL4030_MADC_CTRL_SW2 0x13
> > +
> > +#define TWL4030_MADC_RTCH0_LSB 0x17
> > +#define TWL4030_MADC_GPCH0_LSB 0x37
> > +
> > +#define TWL4030_MADC_MADCON (1<<0) /* MADC power on */
> > +#define TWL4030_MADC_BUSY (1<<0) /* MADC busy */
> > +#define TWL4030_MADC_EOC_SW (1<<1) /* MADC conversion
> completion */
> > +#define TWL4030_MADC_SW_START (1<<5) /* MADC SWx start
> conversion */
> > +
> > +#define TWL4030_MADC_ADCIN0 (1<<0)
> > +#define TWL4030_MADC_ADCIN1 (1<<1)
> > +#define TWL4030_MADC_ADCIN2 (1<<2)
> > +#define TWL4030_MADC_ADCIN3 (1<<3)
> > +#define TWL4030_MADC_ADCIN4 (1<<4)
> > +#define TWL4030_MADC_ADCIN5 (1<<5)
> > +#define TWL4030_MADC_ADCIN6 (1<<6)
> > +#define TWL4030_MADC_ADCIN7 (1<<7)
> > +#define TWL4030_MADC_ADCIN8 (1<<8)
> > +#define TWL4030_MADC_ADCIN9 (1<<9)
> > +#define TWL4030_MADC_ADCIN10 (1<<10)
> > +#define TWL4030_MADC_ADCIN11 (1<<11)
> > +#define TWL4030_MADC_ADCIN12 (1<<12)
> > +#define TWL4030_MADC_ADCIN13 (1<<13)
> > +#define TWL4030_MADC_ADCIN14 (1<<14)
> > +#define TWL4030_MADC_ADCIN15 (1<<15)
> > +
> > +/* Fixed channels */
> > +#define TWL4030_MADC_BTEMP TWL4030_MADC_ADCIN1
> > +#define TWL4030_MADC_VBUS TWL4030_MADC_ADCIN8
> > +#define TWL4030_MADC_VBKB TWL4030_MADC_ADCIN9
> > +#define TWL4030_MADC_ICHG TWL4030_MADC_ADCIN10
> > +#define TWL4030_MADC_VCHG TWL4030_MADC_ADCIN11
> > +#define TWL4030_MADC_VBAT TWL4030_MADC_ADCIN12
> > +
> > +#define TWL4030_BCI_BCICTL1 0x23
> > +#define TWL4030_BCI_MESBAT (1<<1)
> > +#define TWL4030_BCI_TYPEN (1<<4)
> > +#define TWL4030_BCI_ITHEN (1<<3)
> > +
> > +#define TWL4030_MADC_IOC_MAGIC '`'
> > +#define TWL4030_MADC_IOCX_ADC_RAW_READ
> _IO(TWL4030_MADC_IOC_MAGIC, 0)
> > +
> > +struct twl4030_madc_user_parms {
> > + int channel;
> > + int average;
> > + int status;
> > + u16 result;
> > +};
> > +
> > +int twl4030_madc_conversion(struct twl4030_madc_request *conv);
> > +
> > +#endif
> > --
> > 1.7.0.4
> >
> >
> > _______________________________________________
> > lm-sensors mailing list
> > lm-sensors@lm-sensors.org
> > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module
2010-09-20 10:46 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 J, KEERTHY
@ 2010-09-20 14:09 ` Guenter Roeck
-1 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2010-09-20 14:09 UTC (permalink / raw)
To: J, KEERTHY; +Cc: linux-omap, lm-sensors, Krishnamoorthy, Balaji T
Hi,
On Mon, Sep 20, 2010 at 06:34:24AM -0400, J, KEERTHY wrote:
>
>
> > -----Original Message-----
> > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> > Sent: Thursday, September 16, 2010 8:48 PM
> > To: J, KEERTHY
> > Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy,
> > Balaji T
> > Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
> > madc module
> >
[...]
> > > +EXPORT_SYMBOL(twl4030_madc_conversion);
> > > +
> > If this function is going to be called from external code, it should not
> > really be defined here. I would suggest to move it to a global location
> > such as
> > mfd instead, including all related functions.
> >
> > The existence of this function export indicates that another non-hwmon
> > driver depends on this one, which should not really be the case. Another
> > reason to have a separate common driver instead, and mfd might just be the
> > place for it.
> Few kernel modules need to perform ADC conversion to measure battery
> voltage, battery temperature, VBUS voltage via twl4030_madc_conversion.
> the_madc is needed as those drivers will not have this device pointer.
>
The point I was trying to make is that this function (as well as the ioctl)
should not be in this driver in the first place. hwmon is about providing
hwmon information, not about providing adc readings to another driver.
Or, in other words, hwmon should be a consumer of information from other drivers,
not a producer of information to other drivers.
There should be a higher level driver (presumably a mfd driver) to provide
adc readings to all consumers, ie to all callers of twl4030_madc_conversion().
This driver can provide all data and information needed by more than one driver,
and would also be the logical place for the ioctl providing raw adc readings
to the user.
Guenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
@ 2010-09-20 14:09 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2010-09-20 14:09 UTC (permalink / raw)
To: J, KEERTHY; +Cc: linux-omap, lm-sensors, Krishnamoorthy, Balaji T
Hi,
On Mon, Sep 20, 2010 at 06:34:24AM -0400, J, KEERTHY wrote:
>
>
> > -----Original Message-----
> > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> > Sent: Thursday, September 16, 2010 8:48 PM
> > To: J, KEERTHY
> > Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy,
> > Balaji T
> > Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
> > madc module
> >
[...]
> > > +EXPORT_SYMBOL(twl4030_madc_conversion);
> > > +
> > If this function is going to be called from external code, it should not
> > really be defined here. I would suggest to move it to a global location
> > such as
> > mfd instead, including all related functions.
> >
> > The existence of this function export indicates that another non-hwmon
> > driver depends on this one, which should not really be the case. Another
> > reason to have a separate common driver instead, and mfd might just be the
> > place for it.
> Few kernel modules need to perform ADC conversion to measure battery
> voltage, battery temperature, VBUS voltage via twl4030_madc_conversion.
> the_madc is needed as those drivers will not have this device pointer.
>
The point I was trying to make is that this function (as well as the ioctl)
should not be in this driver in the first place. hwmon is about providing
hwmon information, not about providing adc readings to another driver.
Or, in other words, hwmon should be a consumer of information from other drivers,
not a producer of information to other drivers.
There should be a higher level driver (presumably a mfd driver) to provide
adc readings to all consumers, ie to all callers of twl4030_madc_conversion().
This driver can provide all data and information needed by more than one driver,
and would also be the logical place for the ioctl providing raw adc readings
to the user.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module
2010-09-20 14:09 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 Guenter Roeck
@ 2010-09-20 14:38 ` Guenter Roeck
-1 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2010-09-20 14:38 UTC (permalink / raw)
To: J, KEERTHY; +Cc: linux-omap, Krishnamoorthy, Balaji T, lm-sensors
On Mon, Sep 20, 2010 at 10:09:05AM -0400, Guenter Roeck wrote:
> Hi,
> On Mon, Sep 20, 2010 at 06:34:24AM -0400, J, KEERTHY wrote:
> >
> >
> > > -----Original Message-----
> > > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> > > Sent: Thursday, September 16, 2010 8:48 PM
> > > To: J, KEERTHY
> > > Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy,
> > > Balaji T
> > > Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
> > > madc module
> > >
> [...]
> > > > +EXPORT_SYMBOL(twl4030_madc_conversion);
> > > > +
> > > If this function is going to be called from external code, it should not
> > > really be defined here. I would suggest to move it to a global location
> > > such as
> > > mfd instead, including all related functions.
> > >
> > > The existence of this function export indicates that another non-hwmon
> > > driver depends on this one, which should not really be the case. Another
> > > reason to have a separate common driver instead, and mfd might just be the
> > > place for it.
> > Few kernel modules need to perform ADC conversion to measure battery
> > voltage, battery temperature, VBUS voltage via twl4030_madc_conversion.
> > the_madc is needed as those drivers will not have this device pointer.
> >
> The point I was trying to make is that this function (as well as the ioctl)
> should not be in this driver in the first place. hwmon is about providing
> hwmon information, not about providing adc readings to another driver.
>
> Or, in other words, hwmon should be a consumer of information from other drivers,
> not a producer of information to other drivers.
>
> There should be a higher level driver (presumably a mfd driver) to provide
> adc readings to all consumers, ie to all callers of twl4030_madc_conversion().
> This driver can provide all data and information needed by more than one driver,
> and would also be the logical place for the ioctl providing raw adc readings
> to the user.
>
I just noticed that there already is a mfd core driver for tw4030. You might want
to consider moving the functionality to read adc values into that driver.
Guenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
@ 2010-09-20 14:38 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2010-09-20 14:38 UTC (permalink / raw)
To: J, KEERTHY; +Cc: linux-omap, Krishnamoorthy, Balaji T, lm-sensors
On Mon, Sep 20, 2010 at 10:09:05AM -0400, Guenter Roeck wrote:
> Hi,
> On Mon, Sep 20, 2010 at 06:34:24AM -0400, J, KEERTHY wrote:
> >
> >
> > > -----Original Message-----
> > > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> > > Sent: Thursday, September 16, 2010 8:48 PM
> > > To: J, KEERTHY
> > > Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy,
> > > Balaji T
> > > Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
> > > madc module
> > >
> [...]
> > > > +EXPORT_SYMBOL(twl4030_madc_conversion);
> > > > +
> > > If this function is going to be called from external code, it should not
> > > really be defined here. I would suggest to move it to a global location
> > > such as
> > > mfd instead, including all related functions.
> > >
> > > The existence of this function export indicates that another non-hwmon
> > > driver depends on this one, which should not really be the case. Another
> > > reason to have a separate common driver instead, and mfd might just be the
> > > place for it.
> > Few kernel modules need to perform ADC conversion to measure battery
> > voltage, battery temperature, VBUS voltage via twl4030_madc_conversion.
> > the_madc is needed as those drivers will not have this device pointer.
> >
> The point I was trying to make is that this function (as well as the ioctl)
> should not be in this driver in the first place. hwmon is about providing
> hwmon information, not about providing adc readings to another driver.
>
> Or, in other words, hwmon should be a consumer of information from other drivers,
> not a producer of information to other drivers.
>
> There should be a higher level driver (presumably a mfd driver) to provide
> adc readings to all consumers, ie to all callers of twl4030_madc_conversion().
> This driver can provide all data and information needed by more than one driver,
> and would also be the logical place for the ioctl providing raw adc readings
> to the user.
>
I just noticed that there already is a mfd core driver for tw4030. You might want
to consider moving the functionality to read adc values into that driver.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module
2010-09-20 14:38 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 Guenter Roeck
@ 2010-09-27 9:31 ` Keerthy
-1 siblings, 0 replies; 12+ messages in thread
From: Keerthy @ 2010-09-27 9:19 UTC (permalink / raw)
To: sameo
Cc: J, KEERTHY, linux-omap, Krishnamoorthy, Balaji T, lm-sensors,
Guenter Roeck
Hello Sameo,
twl4030-madc driver patch can be found here:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg34947.html
Based on the received inputs.
Can the twl4030-madc driver or part of the driver reside under mfd?
Regards,
Keerthy
On Monday 20 September 2010 08:08 PM, Guenter Roeck wrote:
> On Mon, Sep 20, 2010 at 10:09:05AM -0400, Guenter Roeck wrote:
>
>> Hi,
>> On Mon, Sep 20, 2010 at 06:34:24AM -0400, J, KEERTHY wrote:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
>>>> Sent: Thursday, September 16, 2010 8:48 PM
>>>> To: J, KEERTHY
>>>> Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy,
>>>> Balaji T
>>>> Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
>>>> madc module
>>>>
>>>>
>> [...]
>>
>>>>> +EXPORT_SYMBOL(twl4030_madc_conversion);
>>>>> +
>>>>>
>>>> If this function is going to be called from external code, it should not
>>>> really be defined here. I would suggest to move it to a global location
>>>> such as
>>>> mfd instead, including all related functions.
>>>>
>>>> The existence of this function export indicates that another non-hwmon
>>>> driver depends on this one, which should not really be the case. Another
>>>> reason to have a separate common driver instead, and mfd might just be the
>>>> place for it.
>>>>
>>> Few kernel modules need to perform ADC conversion to measure battery
>>> voltage, battery temperature, VBUS voltage via twl4030_madc_conversion.
>>> the_madc is needed as those drivers will not have this device pointer.
>>>
>>>
>> The point I was trying to make is that this function (as well as the ioctl)
>> should not be in this driver in the first place. hwmon is about providing
>> hwmon information, not about providing adc readings to another driver.
>>
>> Or, in other words, hwmon should be a consumer of information from other drivers,
>> not a producer of information to other drivers.
>>
>> There should be a higher level driver (presumably a mfd driver) to provide
>> adc readings to all consumers, ie to all callers of twl4030_madc_conversion().
>> This driver can provide all data and information needed by more than one driver,
>> and would also be the logical place for the ioctl providing raw adc readings
>> to the user.
>>
>>
> I just noticed that there already is a mfd core driver for tw4030. You might want
> to consider moving the functionality to read adc values into that driver.
>
> Guenter
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
@ 2010-09-27 9:31 ` Keerthy
0 siblings, 0 replies; 12+ messages in thread
From: Keerthy @ 2010-09-27 9:31 UTC (permalink / raw)
To: sameo
Cc: J, KEERTHY, linux-omap, Krishnamoorthy, Balaji T, lm-sensors,
Guenter Roeck
Hello Sameo,
twl4030-madc driver patch can be found here:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg34947.html
Based on the received inputs.
Can the twl4030-madc driver or part of the driver reside under mfd?
Regards,
Keerthy
On Monday 20 September 2010 08:08 PM, Guenter Roeck wrote:
> On Mon, Sep 20, 2010 at 10:09:05AM -0400, Guenter Roeck wrote:
>
>> Hi,
>> On Mon, Sep 20, 2010 at 06:34:24AM -0400, J, KEERTHY wrote:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
>>>> Sent: Thursday, September 16, 2010 8:48 PM
>>>> To: J, KEERTHY
>>>> Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy,
>>>> Balaji T
>>>> Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030
>>>> madc module
>>>>
>>>>
>> [...]
>>
>>>>> +EXPORT_SYMBOL(twl4030_madc_conversion);
>>>>> +
>>>>>
>>>> If this function is going to be called from external code, it should not
>>>> really be defined here. I would suggest to move it to a global location
>>>> such as
>>>> mfd instead, including all related functions.
>>>>
>>>> The existence of this function export indicates that another non-hwmon
>>>> driver depends on this one, which should not really be the case. Another
>>>> reason to have a separate common driver instead, and mfd might just be the
>>>> place for it.
>>>>
>>> Few kernel modules need to perform ADC conversion to measure battery
>>> voltage, battery temperature, VBUS voltage via twl4030_madc_conversion.
>>> the_madc is needed as those drivers will not have this device pointer.
>>>
>>>
>> The point I was trying to make is that this function (as well as the ioctl)
>> should not be in this driver in the first place. hwmon is about providing
>> hwmon information, not about providing adc readings to another driver.
>>
>> Or, in other words, hwmon should be a consumer of information from other drivers,
>> not a producer of information to other drivers.
>>
>> There should be a higher level driver (presumably a mfd driver) to provide
>> adc readings to all consumers, ie to all callers of twl4030_madc_conversion().
>> This driver can provide all data and information needed by more than one driver,
>> and would also be the logical place for the ioctl providing raw adc readings
>> to the user.
>>
>>
> I just noticed that there already is a mfd core driver for tw4030. You might want
> to consider moving the functionality to read adc values into that driver.
>
> Guenter
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2010-09-27 9:31 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-16 10:23 [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module Keerthy
2010-09-16 10:35 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc Keerthy
2010-09-16 15:17 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module Guenter Roeck
2010-09-16 15:17 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 Guenter Roeck
2010-09-20 10:34 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module J, KEERTHY
2010-09-20 10:46 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 J, KEERTHY
2010-09-20 14:09 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module Guenter Roeck
2010-09-20 14:09 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 Guenter Roeck
2010-09-20 14:38 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module Guenter Roeck
2010-09-20 14:38 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 Guenter Roeck
2010-09-27 9:19 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module Keerthy
2010-09-27 9:31 ` [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 Keerthy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.