> -----Original Message----- > From: Guenter Roeck [mailto:guenter.roeck@xxxxxxxxxxxx] > Sent: Thursday, September 16, 2010 8:48 PM > To: J, KEERTHY > Cc: linux-omap@xxxxxxxxxxxxxxx; lm-sensors@xxxxxxxxxxxxxx; Krishnamoorthy, > Balaji T > Subject: Re: [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@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. > 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@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
