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@xxxxxx>
> ---
> 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@xxxxxxxxxxxxxx
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
