Re: [PATCH V2] hwmon: twl4030: Driver for twl4030 madc module
On Thu, Nov 11, 2010 at 5:31 AM, Guenter Roeck
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
>
> 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@vger.kernel.org/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 000..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
>> + *
>> + * Based on twl4030-madc.c
>> + * Copyright (C) 2008 Nokia Corporation
>> + * Mikko Ylinen
>> + *
>> + * Amit Kucheria
>> + *
>> + * 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
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +struct twl4030_madc_data {
>> + struct device *hwmon_dev;
>> + struct mutex lock;
>> + struct twl4030_madc_request requests[TWL40
Re: [PATCH V2] hwmon: twl4030: Driver for twl4030 madc module
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
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.
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@vger.kernel.org/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.
> 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 000..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
> + *
> + * Based on twl4030-madc.c
> + * Copyright (C) 2008 Nokia Corporation
> + * Mikko Ylinen
> + *
> + * Amit Kucheria
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +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;
> +
RE: [PATCH V2] hwmon: twl4030: Driver for twl4030 madc module
Hello Anand, -Original Message- From: Gadiyar, Anand Sent: Tuesday, November 09, 2010 7:10 PM To: J, KEERTHY; lm-sens...@lm-sensors.org; guenter.ro...@ericsson.com; sa...@linux.intel.com; kh...@linux-fr.org Cc: mikko.k.yli...@nokia.com; Balbi, Felipe; amit.kuche...@canonical.com; linux-omap@vger.kernel.org; Krishnamoorthy, Balaji T Subject: RE: [PATCH V2] hwmon: twl4030: Driver for twl4030 madc module > 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 ... > 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 You need to provide some label text here, otherwise this option will not show up in the kernel configuration tools, and hence cannot be turned on or off from there. Ok. I get it. I will add a label. - Anand Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2] hwmon: twl4030: Driver for twl4030 madc module
> 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 ... > 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 You need to provide some label text here, otherwise this option will not show up in the kernel configuration tools, and hence cannot be turned on or off from there. - Anand -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
