Re: [PATCH 2/2] hwmon: twl4030: Hwmon Driver for TWL4030 MADC

[Guenter Roeck <guenter.roeck@ericsson.com>]

On Wed, Feb 16, 2011 at 07:56:57AM -0500, Keerthy wrote:
> This driver exposes the sysfs nodes of the TWL4030 MADC module.
> All the channel values are expressed in terms of mV. Channel 13
> and channel 14 are reserved. There are channels which represent
> temperature and current. Even they output raw voltage in mV.
> 
Why ?

> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
> 
> The previous discussion concluded in keeping the generic ADC
> functionality and the hwmon separate. The discussion can be found here:
> 
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg41805.html
> 
>  Documentation/hwmon/twl4030-madc-hwmon |   45 +++++++++
>  drivers/hwmon/Kconfig                  |   10 ++
>  drivers/hwmon/Makefile                 |    1 +
>  drivers/hwmon/twl4030-madc-hwmon.c     |  155 ++++++++++++++++++++++++++++++++
>  4 files changed, 211 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/hwmon/twl4030-madc-hwmon
>  create mode 100644 drivers/hwmon/twl4030-madc-hwmon.c
> 
> diff --git a/Documentation/hwmon/twl4030-madc-hwmon b/Documentation/hwmon/twl4030-madc-hwmon
> new file mode 100644
> index 0000000..2cf1cc2
> --- /dev/null
> +++ b/Documentation/hwmon/twl4030-madc-hwmon
> @@ -0,0 +1,45 @@
> +Kernel driver twl4030-madc
> +=========================
> +
> +Supported chips:
> +	* Texas Instruments TWL4030
> +	Prefix: 'twl4030-madc'
> +
> +
> +Authors:
> +	J Keerthy <j-keerthy@ti.com>
> +
> +Description
> +-----------
> +
> +The Texas Instruments TWL4030 is a Power Management and Audio Circuit. Among
> +other things it contains a 10-bit A/D converter MADC. The converter has 16
> +channels which can be used in different modes.
> +
> +
> +See this table for the meaning of the different channels
> +
> +Channel Signal
> +------------------------------------------
> +0	Battery type(BTYPE)
> +1	BCI: Battery temperature (BTEMP)
> +2	GP analog input
> +3	GP analog input
> +4	GP analog input
> +5	GP analog input
> +6	GP analog input
> +7	GP analog input
> +8	BCI: VBUS voltage(VBUS)
> +9	Backup Battery voltage (VBKP)
> +10	BCI: Battery charger current (ICHG)
> +11	BCI: Battery charger voltage (VCHG)
> +12	BCI: Main battery voltage (VBAT)
> +13	Reserved
> +14	Reserved
> +15	VRUSB Supply/Speaker left/Speaker right polarization level
> +
> +
> +The Sysfs nodes will represent the voltage in the units of mV,
> +the temperature channel shows the converted raw voltage in mV.
> +The Battery charging current channel represents raw voltage mV.

Doesn't really make sense to me to export values known to be current and temperature
as voltages. You'll have to add a lot more information for that to make sense.

> +Channel 13 and 14 are reserved.

You already said that above.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 773e484..11ddde3 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -940,6 +940,16 @@ config SENSORS_TMP421
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tmp421.
>  
> +config SENSORS_TWL4030_MADC
> +	tristate "Texas Instruments TWL4030 MADC Hwmon"
> +	depends on TWL4030_MADC
> +	help
> +	This driver provides hwmon support for triton TWL4030-MADC.
> +	This driver can be built as part of kernel or can be built
> +	as a module.

	Kernel is obvious. Please stick with the usual text

	"This driver can also be built as a module.  If so, the module
         will be called XXXX"

At least please tell users how the module is named.

> +		This driver exposes the various voltage values to
> +	the user space.
> +
Not sure if this provides any value. Either case, it should be before "built as module".

>  config SENSORS_VIA_CPUTEMP
>  	tristate "VIA CPU temperature sensor"
>  	depends on X86
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index dde02d9..bc7d740 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -102,6 +102,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-hwmon.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-hwmon.c b/drivers/hwmon/twl4030-madc-hwmon.c
> new file mode 100644
> index 0000000..4a61e8a
> --- /dev/null
> +++ b/drivers/hwmon/twl4030-madc-hwmon.c
> @@ -0,0 +1,155 @@
> +/*
> + *
> + * TWL4030 MADC Hwmon driver-This driver monitors the real time
> + * conversion of analog signals like battery temperature,
> + * battery type, battery level etc. User can ask for the conversion on a
> + * particular channel using the sysfs nodes.
> + *
> + * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com/

2011 ?

> + * J Keerthy <j-keerthy@ti.com>
> + *
> + * 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/platform_device.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/i2c/twl4030-madc.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +/*
> + * sysfs hook function
> + */
> +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;
> +	long val;
> +
> +	req.channels = (1 << attr->index);
> +	req.method = TWL4030_MADC_SW2;
> +	req.func_cb = NULL;
> +	val = twl4030_madc_conversion(&req);
> +	if (val >= 0)
> +		val = req.rbuf[attr->index];
> +	else
> +		return val;
> +
Seems to be a bit complicated.

	if (val < 0)
		return val;

	return sprintf(buf, "%ld\n", req.rbuf[attr->index]);

would be more straightforward.

> +	return sprintf(buf, "%ld\n", val);
> +}
> +
> +/* sysfs nodes to read individual channels from user side */
> +static SENSOR_DEVICE_ATTR(in0_battery_type, S_IRUGO, madc_read, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_battery_temp, S_IRUGO, madc_read, NULL, 1);

I don't know why people always try to introduce new sysfs attributes
for no good reason. Use a label if you want to describe the sensor.
And use tempX_input for temperatures, and currX_input for currents.

I am not inclined to accept drivers introducing new sysfs attributes,
unless the new attribute has either been discussed and added to the ABI,
or if a _really_ good reason for the non-standard attribute is provided.
Neither is the case here. 

> +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_vbus, S_IRUGO, madc_read, NULL, 8);
> +static SENSOR_DEVICE_ATTR(in9_vbkp, S_IRUGO, madc_read, NULL, 9);
> +static SENSOR_DEVICE_ATTR(in10_battery_charger_current,
> +			S_IRUGO, madc_read, NULL, 10);
> +static SENSOR_DEVICE_ATTR(in11_battery_charger_voltage,
> +			S_IRUGO, madc_read, NULL, 11);
> +static SENSOR_DEVICE_ATTR(in12_main_battery_voltage,
> +			S_IRUGO, madc_read, NULL, 12);

... and again ...

> +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO, madc_read, NULL, 15);
> +

Number sequence doesn't have to match chip channel number.
Sure you want to leave a hole here ?

Either case, you should describe in the documentation how chip channels
match attribute names.

> +static struct attribute *twl4030_madc_attributes[] = {
> +	&sensor_dev_attr_in0_battery_type.dev_attr.attr,
> +	&sensor_dev_attr_in1_battery_temp.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_vbus.dev_attr.attr,
> +	&sensor_dev_attr_in9_vbkp.dev_attr.attr,
> +	&sensor_dev_attr_in10_battery_charger_current.dev_attr.attr,
> +	&sensor_dev_attr_in11_battery_charger_voltage.dev_attr.attr,
> +	&sensor_dev_attr_in12_main_battery_voltage.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_hwmon_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	int status;
> +	struct device *hwmon;
> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &twl4030_madc_group);
> +	if (ret)
> +		goto err_sysfs;
> +	hwmon = hwmon_device_register(&pdev->dev);
> +	if (IS_ERR(hwmon)) {
> +		dev_err(&pdev->dev, "hwmon_device_register failed.\n");
> +		status = PTR_ERR(hwmon);
> +		goto err_reg;
> +	}
> +
> +	return 0;
> +
> +err_reg:
> +	sysfs_remove_group(&pdev->dev.kobj, &twl4030_madc_group);
> +err_sysfs:
> +
> +	return ret;
> +}
> +
> +static int __devexit twl4030_madc_hwmon_remove(struct platform_device *pdev)
> +{
> +	hwmon_device_unregister(&pdev->dev);
> +	sysfs_remove_group(&pdev->dev.kobj, &twl4030_madc_group);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver twl4030_madc_hwmon_driver = {
> +	.probe = twl4030_madc_hwmon_probe,
> +	.remove = __exit_p(twl4030_madc_hwmon_remove),
> +	.driver = {
> +		   .name = "twl4030_madc_hwmon",
> +		   .owner = THIS_MODULE,
> +		   },
> +};
> +
> +static int __init twl4030_madc_hwmon_init(void)
> +{
> +	return platform_driver_register(&twl4030_madc_hwmon_driver);
> +}
> +
> +module_init(twl4030_madc_hwmon_init);
> +
> +static void __exit twl4030_madc_hwmon_exit(void)
> +{
> +	platform_driver_unregister(&twl4030_madc_hwmon_driver);
> +}
> +
> +module_exit(twl4030_madc_hwmon_exit);
> +
> +MODULE_DESCRIPTION("TWL4030 ADC Hwmon driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("J Keerthy");
> +MODULE_ALIAS("twl4030_madc_hwmon");
> -- 
> 1.7.0.4
>