Message-ID: <20090728162658.0ac35362@hyperion.delvare>
Date:	Tue, 28 Jul 2009 16:26:58 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>, linux-kernel@...r.kernel.org,
	lm-sensors@...sensors.org
Subject: Re: [PATCH] hwmon: WM831x PMIC hardware monitoring driver

Hi Mark,

On Tue, 28 Jul 2009 15:11:00 +0100, Mark Brown wrote:
> This driver adds support for the hardware monitoring features of
> the WM831x PMICs to the hwmon API. Monitoring is provided for
> the system voltages supported natively by the WM831x, the chip
> temperature, the battery temperature and the auxiliary inputs
> of the WM831x.
> 
> Currently no alarms are supported, though digital comparators on
> the WM831x devices would allow these to be provided.
> 
> Since the auxiliary and battery temperature input scaling depends
> on the system configuration the value is reported as a voltage to
> userspace.
> 
> Signed-off-by: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> Cc: Jean Delvare <khali@...ux-fr.org>
> Cc: lm-sensors@...sensors.org
> ---
> 
> Changes from review from Jean:
>  - Hook up backup battery monitoring.
>  - Use DIV_ROUND_CLOSEST().
>  - Report battery temperature as a voltage.
>  - Various formatting cleanups.
> 
>  Documentation/hwmon/wm831x   |   37 +++++++
>  drivers/hwmon/Kconfig        |   11 ++
>  drivers/hwmon/Makefile       |    1 +
>  drivers/hwmon/wm831x-hwmon.c |  240 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 289 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/hwmon/wm831x
>  create mode 100644 drivers/hwmon/wm831x-hwmon.c
> 
> diff --git a/Documentation/hwmon/wm831x b/Documentation/hwmon/wm831x
> new file mode 100644
> index 0000000..ab10068
> --- /dev/null
> +++ b/Documentation/hwmon/wm831x
> @@ -0,0 +1,37 @@
> +Kernel driver wm831x-hwmon
> +==========================
> +
> +Supported chips:
> +  * Wolfson Microelectronics WM831x PMICs
> +    Prefix: 'wm831x'
> +    Datasheet:
> +	http://www.wolfsonmicro.com/products/WM8310
> +	http://www.wolfsonmicro.com/products/WM8311
> +	http://www.wolfsonmicro.com/products/WM8312
> +
> +Authors: Mark Brown <broonie@...nsource.wolfsonmicro.com>
> +
> +Description
> +-----------
> +
> +The WM831x series of PMICs include an AUXADC which can be used to
> +monitor a range of system operating parameters, including the voltages
> +of the major supplies within the system.  Currently the driver provides
> +reporting of all the input values but does not provide any alarms.
> +
> +Voltage Monitoring
> +------------------
> +
> +Voltages are sampled by a 12 bit ADC.  Voltages in milivolts are 1.465
> +times the ADC value.
> +
> +Temperature Monitoring
> +----------------------
> +
> +Temperatures are sampled by a 12 bit ADC.  Chip and battery temperatures
> +are available.  The chip temperature is calculated as:
> +
> +	Degrees celsius = (512.18 - data) / 1.0983
> +
> +while the battery temperature calculation will depend on the NTC
> +termistor component.

I think the correct spelling is thermistor.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 8d17135..ed85909 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -930,6 +930,17 @@ config SENSORS_W83627EHF
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called w83627ehf.
>  
> +config SENSORS_WM831X
> +	tristate "WM831x PMICs"
> +	depends on MFD_WM831X
> +	help
> +	  If you say yes here you get support for the hardware
> +	  monitoring functionality of the Wolfson Microelectronics
> +	  WM831x series of PMICs.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called wm831x-hwmon.
> +
>  config SENSORS_WM8350
>  	tristate "Wolfson Microelectronics WM835x"
>  	depends on MFD_WM8350
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 6126257..a7e7eb7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -90,6 +90,7 @@ obj-$(CONFIG_SENSORS_VT8231)	+= vt8231.o
>  obj-$(CONFIG_SENSORS_W83627EHF)	+= w83627ehf.o
>  obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
>  obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
> +obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
>  obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
>  
>  ifeq ($(CONFIG_HWMON_DEBUG_CHIP),y)
> diff --git a/drivers/hwmon/wm831x-hwmon.c b/drivers/hwmon/wm831x-hwmon.c
> new file mode 100644
> index 0000000..a3a9b78
> --- /dev/null
> +++ b/drivers/hwmon/wm831x-hwmon.c
> @@ -0,0 +1,240 @@
> +/*
> + * drivers/hwmon/wm831x-hwmon.c - Wolfson Microelectronics WM831x PMIC
> + *                                hardware monitoring features.
> + *
> + * Copyright (C) 2009 Wolfson Microelectronics plc
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License v2 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 Street, Fifth Floor, Boston, MA  02110-1301, USA
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include <linux/mfd/wm831x/core.h>
> +#include <linux/mfd/wm831x/auxadc.h>
> +
> +struct wm831x_hwmon {
> +	struct wm831x *wm831x;
> +	struct device *classdev;
> +};
> +
> +static ssize_t show_name(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "wm831x\n");
> +}
> +
> +static const char *input_names[] = {
> +	[WM831X_AUX_SYSVDD]    = "SYSVDD",
> +	[WM831X_AUX_USB]       = "USB",
> +	[WM831X_AUX_BKUP_BATT] = "Backup battery",
> +	[WM831X_AUX_BATT]      = "Battery",
> +	[WM831X_AUX_WALL]      = "WALL",
> +	[WM831X_AUX_CHIP_TEMP] = "PMIC",
> +	[WM831X_AUX_BATT_TEMP] = "Battery",
> +};
> +
> +
> +static ssize_t show_voltage(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct wm831x_hwmon *hwmon = dev_get_drvdata(dev);
> +	int channel = to_sensor_dev_attr(attr)->index;
> +	int ret;
> +
> +	ret = wm831x_auxadc_read_uv(hwmon->wm831x, channel);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", DIV_ROUND_CLOSEST(ret, 1000));
> +}
> +
> +static ssize_t show_chip_temp(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct wm831x_hwmon *hwmon = dev_get_drvdata(dev);
> +	int channel = to_sensor_dev_attr(attr)->index;
> +	int ret;
> +
> +	ret = wm831x_auxadc_read(hwmon->wm831x, channel);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Degrees celsius = (512.18-ret) / 1.0983 */
> +	ret = 512180 - (ret * 1000);
> +	ret = DIV_ROUND_CLOSEST(ret * 10000, 10983);
> +
> +	return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t show_batt_temp(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct wm831x_hwmon *hwmon = dev_get_drvdata(dev);
> +	int channel = to_sensor_dev_attr(attr)->index;
> +	int ret;
> +
> +	ret = wm831x_auxadc_read_uv(hwmon->wm831x, channel);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* The conversion depends on the battery, leave to userspace but
> +	 * report as voltage for ABI reasons. */
> +	return sprintf(buf, "%d\n", DIV_ROUND_CLOSEST(ret, 1000));
> +}

If I am not mistaken, the above function is an exact copy of
show_voltage(), so you might as well use it?

> +
> +static ssize_t show_label(struct device *dev,
> +			  struct device_attribute *attr, char *buf)
> +{
> +	int channel = to_sensor_dev_attr(attr)->index;
> +
> +	return sprintf(buf, "%s\n", input_names[channel]);
> +}
> +
> +#define WM831X_VOLTAGE(id, name) \
> +	static SENSOR_DEVICE_ATTR(in##id##_input, S_IRUGO, show_voltage, \
> +				  NULL, name)
> +
> +#define WM831X_NAMED_VOLTAGE(id, name) \
> +	WM831X_VOLTAGE(id, name); \
> +	static SENSOR_DEVICE_ATTR(in##id##_label, S_IRUGO, show_label,	\
> +				  NULL, name)
> +
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +
> +WM831X_VOLTAGE(0, WM831X_AUX_AUX1);
> +WM831X_VOLTAGE(1, WM831X_AUX_AUX2);
> +WM831X_VOLTAGE(2, WM831X_AUX_AUX3);
> +WM831X_VOLTAGE(3, WM831X_AUX_AUX4);
> +
> +WM831X_NAMED_VOLTAGE(4, WM831X_AUX_SYSVDD);
> +WM831X_NAMED_VOLTAGE(5, WM831X_AUX_USB);
> +WM831X_NAMED_VOLTAGE(6, WM831X_AUX_BATT);
> +WM831X_NAMED_VOLTAGE(7, WM831X_AUX_WALL);
> +WM831X_NAMED_VOLTAGE(8, WM831X_AUX_BKUP_BATT);
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_chip_temp, NULL,
> +			  WM831X_AUX_CHIP_TEMP);
> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL,
> +			  WM831X_AUX_CHIP_TEMP);
> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_batt_temp, NULL,
> +			  WM831X_AUX_BATT_TEMP);
> +static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO, show_label, NULL,
> +			  WM831X_AUX_BATT_TEMP);
> +
> +static struct attribute *wm831x_attributes[] = {
> +	&dev_attr_name.attr,
> +
> +	&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_in4_label.dev_attr.attr,
> +	&sensor_dev_attr_in5_input.dev_attr.attr,
> +	&sensor_dev_attr_in5_label.dev_attr.attr,
> +	&sensor_dev_attr_in6_input.dev_attr.attr,
> +	&sensor_dev_attr_in6_label.dev_attr.attr,
> +	&sensor_dev_attr_in7_input.dev_attr.attr,
> +	&sensor_dev_attr_in7_label.dev_attr.attr,
> +	&sensor_dev_attr_in8_input.dev_attr.attr,
> +	&sensor_dev_attr_in8_label.dev_attr.attr,
> +
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_label.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_label.dev_attr.attr,
> +
> +	NULL
> +};
> +
> +static const struct attribute_group wm831x_attr_group = {
> +	.attrs	= wm831x_attributes,
> +};
> +
> +static int __devinit wm831x_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
> +	struct wm831x_hwmon *hwmon;
> +	int ret;
> +
> +	hwmon = kzalloc(sizeof(struct wm831x_hwmon), GFP_KERNEL);
> +	if (!hwmon)
> +		return -ENOMEM;
> +
> +	hwmon->wm831x = wm831x;
> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &wm831x_attr_group);
> +	if (ret)
> +		goto err;
> +
> +	hwmon->classdev = hwmon_device_register(&pdev->dev);
> +	if (IS_ERR(hwmon->classdev)) {
> +		ret = PTR_ERR(hwmon->classdev);
> +		goto err_sysfs;
> +	}
> +
> +	platform_set_drvdata(pdev, hwmon);
> +
> +	return 0;
> +
> +err_sysfs:
> +	sysfs_remove_group(&pdev->dev.kobj, &wm831x_attr_group);
> +err:
> +	kfree(hwmon);
> +	return ret;
> +}
> +
> +static int __devexit wm831x_hwmon_remove(struct platform_device *pdev)
> +{
> +	struct wm831x_hwmon *hwmon = platform_get_drvdata(pdev);
> +
> +	hwmon_device_unregister(hwmon->classdev);
> +	sysfs_remove_group(&pdev->dev.kobj, &wm831x_attr_group);
> +	kfree(hwmon);
> +	platform_set_drvdata(pdev, NULL);

Preferably the other way around (yes I am nitpicking ;))

> +
> +	return 0;
> +}
> +
> +static struct platform_driver wm831x_hwmon_driver = {
> +	.probe = wm831x_hwmon_probe,
> +	.remove = __devexit_p(wm831x_hwmon_remove),
> +	.driver = {
> +		.name = "wm831x-hwmon",
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init wm831x_hwmon_init(void)
> +{
> +	return platform_driver_register(&wm831x_hwmon_driver);
> +}
> +module_init(wm831x_hwmon_init);
> +
> +static void __exit wm831x_hwmon_exit(void)
> +{
> +	platform_driver_unregister(&wm831x_hwmon_driver);
> +}
> +module_exit(wm831x_hwmon_exit);
> +
> +MODULE_AUTHOR("Mark Brown <broonie@...nsource.wolfsonmicro.com>");
> +MODULE_DESCRIPTION("WM831x Hardware Monitoring");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:wm831x-hwmon");

All the rest looks good to me now, so:

Acked-by: Jean Delvare <khali@...ux-fr.org>

And as I said before for the wm8350-hwmon driver: I can pick this patch
and push it to Linux in 2.6.32 if you want, but if you want it to take
a different route this is equally fine with me. Just tell me if you
want me to pick it.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists