Re: [PATCH V2] hwmon: twl4030: Driver for twl4030 madc module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 11, 2010 at 5:31 AM, Guenter Roeck
<guenter.roeck@xxxxxxxxxxxx> wrote:
> On Tue, Nov 09, 2010 at 04:20:57AM -0500, Keerthy wrote:
>> Introducing a driver for MADC on TWL4030 powerIC. MADC stands for
> monitoring
>> ADC. 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.
>>
>> Signed-off-by: Keerthy <j-keerthy@xxxxxx>
>
> Again, I am not sure if this driver belongs into hwmon, since it is not
> really a hardware monitoring chip but a generic adc. We'll have to sort
> that out.

Since hwmon is the place where most of the ADC drivers are residing.
MADC is also an ADC. We feel that hwmon is the right place.

>
> Code looks much better than before. Still not a complete review; you
> should
> have a much closer look at error handling. I am sure I missed several
> cases
> where error returns are ignored.
>
> Thanks,
> Guenter
>
>> ---
>>
>> Several people have contributed to this driver on the linux-omap list.
>>
>> V2:
>>
>> Error path correction in probe function.
>> Return checks added.
>> the_madc pointer could not be removed. The external drivers will not be
> knowing
>> device information of the madc.
>> Added another function which takes the channel number alone and returns
>> the channel reading if the caller wants TWL4030_MADC_SW2 method for ADC
> conversion.
>> IOCTL function is removed.
>> Work struct is completely removed since request_threaded_irq is used.
>>
>>  drivers/hwmon/Kconfig            |    6 +
>>  drivers/hwmon/Makefile           |    1 +
>>  drivers/hwmon/twl4030-madc.c     |  573
> ++++++++++++++++++++++++++++++++++++++
>>  include/linux/i2c/twl4030-madc.h |  118 ++++++++
>>  4 files changed, 698 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/hwmon/twl4030-madc.c
>>  create mode 100644 include/linux/i2c/twl4030-madc.h
>>
>> V1:
>>
>> http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg34947.html
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index a56f6ad..fef75f2 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1171,6 +1171,12 @@ config SENSORS_MC13783_ADC
>>          help
>>            Support for the A/D converter on MC13783 PMIC.
>>
>> +config SENSORS_TWL4030_MADC
>> +       tristate
>> +       depends on TWL4030_CORE
>> +       help
>> +         This driver provides support for TWL4030-MADC.
>> +
>
> Besides adding a description, you might alwo want to move this to the
> other
> TI chips.
>

I will move this to the other TI Chips.

>>  if ACPI
>>
>>  comment "ACPI drivers"
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 2479b3d..a54af22 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -100,6 +100,7 @@ obj-$(CONFIG_SENSORS_THMC50)        += thmc50.o
>>  obj-$(CONFIG_SENSORS_TMP102)   += tmp102.o
>>  obj-$(CONFIG_SENSORS_TMP401)   += tmp401.o
>>  obj-$(CONFIG_SENSORS_TMP421)   += tmp421.o
>> +obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc.o
>>  obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>>  obj-$(CONFIG_SENSORS_VIA686A)  += via686a.o
>>  obj-$(CONFIG_SENSORS_VT1211)   += vt1211.o
>> diff --git a/drivers/hwmon/twl4030-madc.c b/drivers/hwmon/twl4030-madc.c
>> new file mode 100644
>> index 0000000..42f7d4a
>> --- /dev/null
>> +++ b/drivers/hwmon/twl4030-madc.c
>> @@ -0,0 +1,573 @@
>> +/*
>> + *
>> + * TWL4030 MADC module driver-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.
>> + *
>> + * Copyright (C) 2010 Texas Instruments Inc.
>> + * J Keerthy <j-keerthy@xxxxxx>
>> + *
>> + * Based on twl4030-madc.c
>> + * Copyright (C) 2008 Nokia Corporation
>> + * Mikko Ylinen <mikko.k.ylinen@xxxxxxxxx>
>> + *
>> + * Amit Kucheria <amit.kucheria@xxxxxxxxxxxxx>
>> + *
>> + * 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/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 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;
>> +       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, "%ld\n", val);
>> +       return status;
>
>
>        if (unlikely(val < 0))
>                return val;
>
>        return sprintf(buf, "%ld\n", req.rbuf[attr->index]);
>
> should do and would be simpler. status is not needed at all.
>

Ok i will remove.

>> +}
>> +
>> +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);
>> +}
>
> Looking into other uses of twl_i2c_write_u8(), most check and return
> errors.
> Not sure if it is a good idea to ignore all errors as it is done here.
> There
> should at least some comment explaining that and why it is done.
>

Ok. Error check will be in place.

>> +
>> +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);
>> +
> Ignoring returned errors and combining them into a random value doesn't
> make much sense.

Return checks will be added.

>
>> +       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;
>> +
> I don't see how this can ever be NULL.

Ok i will remove it.

>
>> +       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);
>
> Content of buf will be more or less random after errors.

Ok i will add a return check for every iteration.

>
>> +                       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);
>
> What if twl4030_madc_read() returned an error ? Try to write a random
> value ?
>
>> +       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);
>
> Returned error ignored.

Return checks will be added for read followed by writes.

>
>> +       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;
>> +       const struct twl4030_madc_conversion_method *method;
>> +       u8 isr_val, imr_val;
>> +       int i, len;
>> +       struct twl4030_madc_request *r;
>> +
>> +       isr_val = twl4030_madc_read(madc, madc->isr);
>> +       imr_val = twl4030_madc_read(madc, madc->imr);
>> +
> Returned errors ignored.

I will add a check.

>
>> +       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;
>> +       }
>> +       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);
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +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);
>
> Returned error ignored.

Return check will be addded.

>
>> +               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);
>> +
>> +       if (req->method < TWL4030_MADC_RT || req->method >
> TWL4030_MADC_SW2) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       /* 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);
>> +
>> +/*
>> + * Return channel value
>> + * Or < 0 on failure.
>> + */
>> +static int twl4030_get_madc_conversion(int channel_no)
>> +{
>> +       struct twl4030_madc_request req;
>> +       int temp = 0;
>> +       int ret;
>> +
>> +       req.channels = (1 << channel_no);
>> +       req.method = TWL4030_MADC_SW2;
>> +       req.active = 0;
>> +       req.func_cb = NULL;
>> +       ret = twl4030_madc_conversion(&req);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       if (req.rbuf[channel_no] > 0)
>> +               temp = req.rbuf[channel_no];
>> +
>> +       return temp;
>> +}
>> +EXPORT_SYMBOL(twl4030_get_madc_conversion);
>> +
>
> EXPORT_SYMBOL and static declaration is a contradiction.
> Actually, this function is not used anywhere. Does this compile without
> warning ?

I will add the function declaration in a header file.

>
>> +static void twl4030_madc_set_current_generator(struct twl4030_madc_data
> *madc,
>> +                                              int chan, int on)
>> +{
>> +       int ret;
>> +       u8 regval;
>> +
>> +       ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
>> +                             &regval, TWL4030_BCI_BCICTL1);
>> +       if (ret) {
>> +               dev_dbg(madc->hwmon_dev,
>> +                       "unable to read register 0x%X\n",
> TWL4030_BCI_BCICTL1);
>> +       }
> You detect the error ... then go ahead and use the random regval anyway.
>

I will add a return value check.

>> +       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);
>> +       if (ret)
>> +               dev_err(madc->hwmon_dev,
>> +                       "unable to write register 0x%X\n",
> TWL4030_BCI_BCICTL1);
>> +
> Unnecessary blank line.

I will reomve it.

>
>> +}
>> +
>> +static void twl4030_madc_set_power(struct twl4030_madc_data *madc, int
> on)
>> +{
>> +       u8 regval;
>> +
>> +       regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
>
> Returned error ignored.

Return check will be added.

>
>> +       if (on)
>> +               regval |= TWL4030_MADC_MADCON;
>> +       else
>> +               regval &= ~TWL4030_MADC_MADCON;
>> +       twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, madc_read, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, madc_read, NULL, 1);
>> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, madc_read, NULL, 2);
>> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, madc_read, NULL, 3);
>> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, madc_read, NULL, 4);
>> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, madc_read, NULL, 5);
>> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, madc_read, NULL, 6);
>> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, madc_read, NULL, 7);
>> +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO, madc_read, NULL, 8);
>> +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO, madc_read, NULL, 9);
>> +static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO, madc_read, NULL, 10);
>> +static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO, madc_read, NULL, 11);
>> +static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO, madc_read, NULL, 12);
>> +static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO, madc_read, NULL, 13);
>> +static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO, madc_read, NULL, 14);
>> +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO, 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 __devinit 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_misc;
>> +       }
> You can check that before allocating memory.

Ok i will do that.

>
>> +       madc->imr = (pdata->irq_line == 1) ?
>> +           TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
>> +       madc->isr = (pdata->irq_line == 1) ?
>> +           TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
>> +       twl4030_madc_set_power(madc, 1);
>> +
>> +       twl4030_madc_set_current_generator(madc, 0, 1);
>> +
>> +       ret = twl_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
>> +                             &regval, TWL4030_BCI_BCICTL1);
>> +       if (ret) {
>> +               dev_err(&pdev->dev,
>> +                       "unable to read register 0x%X\n",
> TWL4030_BCI_BCICTL1);
>
> No action ?

Ok. If read fails subsequent write will not make sense i will return an error.

>
>> +       }
>> +       regval |= TWL4030_BCI_MESBAT;
>> +
> If the read failed above, you are going to write random data.
> Not sure if that makes sense.

Ok i will return an error.

>
>> +       ret = twl_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
>> +                              regval, TWL4030_BCI_BCICTL1);
>> +       if (ret) {
>> +               dev_err(&pdev->dev,
>> +                       "unable to write register 0x%X\n",
> TWL4030_BCI_BCICTL1);
>
> No action ?
>
> It might make sense to bail out if register read/writes fail.

Ok i will return error if it fails.

>
>> +       }
>> +
>> +       platform_set_drvdata(pdev, madc);
>> +       mutex_init(&madc->lock);
>> +
>> +       ret = request_threaded_irq(platform_get_irq(pdev, 0), NULL,
>> +                                  twl4030_madc_irq_handler,
>> +                                  IRQF_TRIGGER_RISING, "twl4030_madc",
> madc);
>> +       if (ret) {
>> +               dev_dbg(&pdev->dev, "could not request irq\n");
>> +               goto err_irq;
>
> Jump to release irq for which the request failed.

Ok i will revert only the previously succeeded steps.

>
>> +       }
>> +
>> +       ret = sysfs_create_group(&pdev->dev.kobj, &twl4030_madc_group);
>> +       if (ret)
>> +               goto err_sysfs;
>
> Jump to remove group which wasn't added.

Ok i will revert only the previously succeeded steps.

>
>> +
>> +       madc->hwmon_dev = hwmon_device_register(&pdev->dev);
>> +       if (IS_ERR(madc->hwmon_dev)) {
>> +               dev_err(&pdev->dev, "hwmon_device_register failed.\n");
>> +               status = PTR_ERR(madc->hwmon_dev);
>> +               goto err_reg;
>
> This jumps to err_reg where you try to unregister the device for which
> registration failed.
>

Ok i will revert only the previously succeeded steps.

>> +       }
>> +
>> +       the_madc = madc;
>> +       return 0;
>> +
>> +err_reg:
>> +       hwmon_device_unregister(&pdev->dev);
>> +err_sysfs:
>> +       sysfs_remove_group(&pdev->dev.kobj, &twl4030_madc_group);
>> +err_irq:
>> +       free_irq(platform_get_irq(pdev, 0), madc);
>> +       platform_set_drvdata(pdev, NULL);
>> +       twl4030_madc_set_power(madc, 0);
>> +       twl4030_madc_set_current_generator(madc, 0, 0);
>> +err_misc:
>> +       kfree(madc);
>> +
>> +       return ret;
>> +}
>> +
>> +static int __devexit twl4030_madc_remove(struct platform_device *pdev)
>> +{
>> +       struct twl4030_madc_data *madc = platform_get_drvdata(pdev);
>> +
>> +       hwmon_device_unregister(&pdev->dev);
>> +       sysfs_remove_group(&pdev->dev.kobj, &twl4030_madc_group);
>> +       free_irq(platform_get_irq(pdev, 0), madc);
>> +       platform_set_drvdata(pdev, NULL);
>> +       twl4030_madc_set_current_generator(madc, 0, 0);
>> +       twl4030_madc_set_power(madc, 0);
>> +       kfree(madc);
>> +
>> +       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("J Keerthy");
>> diff --git a/include/linux/i2c/twl4030-madc.h
> b/include/linux/i2c/twl4030-madc.h
>> new file mode 100644
>> index 0000000..0e4e227
>> --- /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 */
>> +
> Formatting looks off here, and I am sure lines are > 80 columns.
>

Yes formatting is off. I will format it.

>> +#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)
>> +
> Please no tabs after #define.
>

Ok i will remove the tabs.

>> +/* 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
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux