[lm-sensors] [RFC, PATCH] hwmon: add support for GMT G760A fan speed PWM controller
Jean Delvare
khali at linux-fr.org
Sun Nov 25 18:47:26 CET 2007
Hi Herbert,
On Sat, 24 Nov 2007 20:25:44 +0100, Herbert Valerio Riedel wrote:
> This controller can be found on the D-Link DNS-323 for instance,
> where it is to be configured via static i2c_board_info in the
> board-specific mach-orion/dns323-setup.c; this driver supports only
> the new-style driver model.
>
> Signed-off-by: Herbert Valerio Riedel <hvr at gnu.org>
> Cc: Jean Delvare <khali at linux-fr.org>
> ---
> wrt to the previous g760a patch, this one is significantly smaller, since I
> removed all legacy code; I also changed the pwm1 sysfs file to reflect the
> convention of 0x00 being lowest and 0xff being maximum fanspeed;
>
> in the code I assume somehow a kind of fan-divider of 2 pulses per rotation;
That's OK, all our drivers do that.
> as that is the case on the dns323, and as the g760a has no "fan1_div" register
> in hardware, I didn't implement a virtual fan1_div sysfs object either...
fan1_div is only indirectly related to the number of pulse per
revolutions emitted by the fan. Please read:
http://www.lm-sensors.org/browser/lm-sensors/trunk/doc/fan-divisors
>
> Documentation/hwmon/g760a | 37 +++++++
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/g760a.c | 260 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 308 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/hwmon/g760a
> create mode 100644 drivers/hwmon/g760a.c
>
> diff --git a/Documentation/hwmon/g760a b/Documentation/hwmon/g760a
> new file mode 100644
> index 0000000..d3f32a5
> --- /dev/null
> +++ b/Documentation/hwmon/g760a
> @@ -0,0 +1,37 @@
> +Kernel driver g760a
> +===================
> +
> +Supported chips:
> + * Global Mixed-mode Technology Inc. G760A
> + Prefix: 'g760a'
> + Datasheet: Publicly available at the GMT website
> + http://www.gmt.com.tw/datasheet/g760a.pdf
> +
> +Author: Herbert Valerio Riedel <hvr at gnu.org>
> +
> +Description
> +-----------
> +
> +The GMT G760A Fan Speed PWM Controller is connected directly to a fan
> +and performs closed-loop control of the fan speed.
> +
> +The fan speed is programmed by setting the period via 'pwm1' of two
> +consecutive speed pulses. The period is defined in terms of clock
> +cycle counts of an assumed 32kHz clock source.
> +
> +Setting a period of 0 stops the fan; setting the period to 255 sets
> +fan to maximum speed.
> +
> +The measured fan rotation speed returned via 'fan1_input' is derived
> +from the measured speed pulse period by assuming again a 32kHz clock
> +source and a 2 pulse-per-revolution fan.
> +
> +The 'alarms' file provides access to the two alarm bits provided by
> +the G760A chip's status register: Bit 0 is set when the actual fan
> +speed differs more than 20% with respect to the programmed fan speed;
> +bit 1 is set when fan speed is below 1920 rpm.
This doesn't match the code (and the code is right).
Note that I think that the documentation is a bit misleading about the
conditions in which bit 1 of the alarm register is set. The chip itself
has no idea about the actual fan speed, all it knows is a number of
clock cycles per half-revolution. For a 32 kHz clock, the max register
value (255) corresponds to 1920 RPMs, but for a different clock, the
limit would be different. What the chip really report is the register
overflow, not a specific speed limit.
I invite you to export the speed limit to user-space as fan1_min
(read-only), otherwise the user might wonder what the alarm flag
corresponds to.
> +
> +The g760a driver will not update its values more frequently than every
> +other second; reading them more often will do no harm, but will return
> +'old' values.
This doesn't match the code, which lets the user update the values every
second (and quite rightly so.)
> +
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index a0445be..6ce8c12 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -291,6 +291,16 @@ config SENSORS_FSCHMD
> This driver can also be built as a module. If so, the module
> will be called fschmd.
>
> +config SENSORS_G760A
> + tristate "GMT G760A"
> + depends on I2C
> + help
> + If you say yes here you get support for Global Mixed-mode
> + Technology Inc G760A fan speed pwm controller chips.
> +
> + This driver can also be built as a module. If so, the module
> + will be called g760a.
> +
> config SENSORS_GL518SM
> tristate "Genesys Logic GL518SM"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 55595f6..93d78a8 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_SENSORS_F75375S) += f75375s.o
> obj-$(CONFIG_SENSORS_FSCHER) += fscher.o
> obj-$(CONFIG_SENSORS_FSCHMD) += fschmd.o
> obj-$(CONFIG_SENSORS_FSCPOS) += fscpos.o
> +obj-$(CONFIG_SENSORS_G760A) += g760a.o
> obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
> obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
> obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
> diff --git a/drivers/hwmon/g760a.c b/drivers/hwmon/g760a.c
> new file mode 100644
> index 0000000..dc503c4
> --- /dev/null
> +++ b/drivers/hwmon/g760a.c
> @@ -0,0 +1,260 @@
> +/*
> + g760a - Driver for the Global Mixed-mode Technology Inc. G760A
> + fan speed PWM controller chip
> +
> + Copyright (C) 2007 Herbert Valerio Riedel <hvr at gnu.org>
> +
> + Complete datasheet is available at GMT's website:
> + http://www.gmt.com.tw/datasheet/g760a.pdf
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 2 of the License, or
> + (at your option) any later version.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>
> +
> +enum g760a_regs {
> + G760A_REG_SET_CNT = 0x00,
> + G760A_REG_ACT_CNT = 0x01,
> + G760A_REG_FAN_STA = 0x02
Please add a trailing comma at the end of the last line. Rationale:
this minimizes the changes when more values need to be added later.
> +};
> +
> +#define G760A_REG_FAN_STA_RPM_OFF 0x1 /* 20% off */
> +#define G760A_REG_FAN_STA_RPM_LOW 0x2 /* below 1920rpm */
> +
> +/* register data is read (and cached) at most once per second */
> +#define G760A_UPDATE_INTERVAL (HZ)
> +
> +struct g760a_data {
> + struct i2c_client *client;
> + struct device *hwmon_dev;
> + struct mutex update_lock;
> +
> + /* board specific parameters */
> + u32 clk; /* default 32kHz */
> + u16 fan_div; /* default P=2 */
> +
> + /* g760a register cache */
> + int valid:1;
> + unsigned long last_updated; /* In jiffies */
> +
> + u8 set_cnt; /* PWM (period) count number; 0xff stops fan */
> + u8 act_cnt; /* formula: cnt = (CLK * 30)/(rpm * P) */
> + u8 fan_sta; /* bit 0: set when actual fan speed more than %20
> + * outside requested fan speed
> + * bit 1: set when fan speed below 1920 rpm */
> +};
> +
> +#define G760A_DEFAULT_CLK 32768
> +#define G760A_DEFAULT_FAN_DIV 2
> +
> +#define RPM_FROM_REG(val, clk, div) \
> + ((0x00 == (val)) ? 0 : (((clk)*30)/((val)*(div))))
I suggest using an inline function instead. Macros evaluating their
parameters more than once can be tricky.
> +
> +#define PWM_FROM_CNT(cnt) (0xff-(cnt))
> +#define PWM_TO_CNT(pwm) (0xff-(pwm))
> +
> +/* new-style driver model */
> +static int g760a_probe(struct i2c_client *client);
> +static int g760a_remove(struct i2c_client *client);
> +
> +static struct i2c_driver g760a_driver = {
> + .driver = {
> + .name = "g760a",
> + },
> + .probe = g760a_probe,
> + .remove = g760a_remove,
> +};
You could also move this driver declaration to the end of the file, so
that you don't need to forward-declare g760a_probe and g760a_remove.
> +
> +/* read/write wrappers */
> +static int g760a_read_value(struct i2c_client *client, enum g760a_regs reg)
> +{
> + return i2c_smbus_read_byte_data(client, reg);
> +}
> +
> +static int g760a_write_value(struct i2c_client *client, enum g760a_regs reg,
> + u16 value)
> +{
> + return i2c_smbus_write_byte_data(client, reg, value);
> +}
I can't see any value to these wrappers. Why don't you call
i2c_smbus_read_byte_data and i2c_smbus_write_byte_data directly?
> +
> +/****************************************************************************
> + * sysfs attributes
> + */
> +
> +static struct g760a_data *g760a_update_client(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct g760a_data *data = i2c_get_clientdata(client);
> +
> + mutex_lock(&data->update_lock);
> +
> + if (time_after(jiffies, data->last_updated + G760A_UPDATE_INTERVAL)
> + || !data->valid) {
> + dev_dbg(&client->dev, "Starting g760a update\n");
> +
> + data->set_cnt = g760a_read_value(client, G760A_REG_SET_CNT);
> + data->act_cnt = g760a_read_value(client, G760A_REG_ACT_CNT);
> + data->fan_sta = g760a_read_value(client, G760A_REG_FAN_STA);
> +
> + data->last_updated = jiffies;
> + data->valid = 1;
> + }
> +
> + mutex_unlock(&data->update_lock);
> +
> + return data;
> +}
> +
> +static ssize_t show_fan(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + struct g760a_data *data = g760a_update_client(dev);
> + int rpm = 0;
> +
> + mutex_lock(&data->update_lock);
> + if (!(data->fan_sta & G760A_REG_FAN_STA_RPM_LOW))
> + rpm = RPM_FROM_REG(data->act_cnt, data->clk, data->fan_div);
> + mutex_unlock(&data->update_lock);
> +
> + return sprintf(buf, "%d\n", rpm);
> +}
> +
> +static ssize_t show_fan_alarm(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + struct g760a_data *data = g760a_update_client(dev);
> +
> + int fan_alarm = (data->fan_sta & G760A_REG_FAN_STA_RPM_OFF) ? 1 : 0;
> +
> + return sprintf(buf, "%d\n", fan_alarm);
> +}
> +
> +static ssize_t get_pwm(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + struct g760a_data *data = g760a_update_client(dev);
> +
> + return sprintf(buf, "%d\n", PWM_FROM_CNT(data->set_cnt));
> +}
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *da,
> + const char *buf, size_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct g760a_data *data = g760a_update_client(dev);
No need to call g760a_update_client() for an attribute write,
i2c_get_clientdata() is enough.
> +
> + const long val = simple_strtol(buf, NULL, 10);
> +
> + mutex_lock(&data->update_lock);
> + data->set_cnt = PWM_TO_CNT(SENSORS_LIMIT(val, 0, 255));
> + g760a_write_value(client, G760A_REG_SET_CNT, data->set_cnt);
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
What your driver (and this chip) implements is NOT pwm1. pwm1 would be
expressing a "raw" PWM duty cycle, while what the chip does is
closed-loop fan control. This is more powerful. In our standard
interface, this corresponds to fan1_target. This is also more
user-friendly, as the fan1_target value is expressed directly in RPM.
> +
> +static DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, get_pwm, set_pwm);
> +static DEVICE_ATTR(fan1_input, S_IRUGO, show_fan, NULL);
> +static DEVICE_ATTR(fan1_alarm, S_IRUGO, show_fan_alarm, NULL);
> +
> +static struct attribute *g760a_attributes[] = {
> + &dev_attr_pwm1.attr,
> + &dev_attr_fan1_input.attr,
> + &dev_attr_fan1_alarm.attr,
> + NULL
> +};
> +
> +static const struct attribute_group g760a_group = {
> + .attrs = g760a_attributes,
> +};
> +
> +/****************************************************************************
> + * new-style driver model code
> + */
> +
> +static int g760a_probe(struct i2c_client *client)
> +{
> + struct g760a_data *data;
> + int err;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA))
> + return -EIO;
> +
> + data = kzalloc(sizeof(struct g760a_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, data);
> +
> + data->client = client;
> + mutex_init(&data->update_lock);
> +
> + /* setup default configuration for now */
> + data->fan_div = G760A_DEFAULT_FAN_DIV;
> + data->clk = G760A_DEFAULT_CLK;
> +
> + /* Register sysfs hooks */
> + err = sysfs_create_group(&client->dev.kobj, &g760a_group);
> + if (err)
> + goto error_sysfs_create_group;
> +
> + data->hwmon_dev = hwmon_device_register(&client->dev);
> + if (IS_ERR(data->hwmon_dev)) {
> + err = PTR_ERR(data->hwmon_dev);
> + goto error_hwmon_device_register;
> + }
> +
> + return 0;
> +
> +error_hwmon_device_register:
> + sysfs_remove_group(&client->dev.kobj, &g760a_group);
> +error_sysfs_create_group:
> + kfree(data);
> + i2c_set_clientdata(client, NULL);
> +
> + return err;
> +}
> +
> +static int g760a_remove(struct i2c_client *client)
> +{
> + struct g760a_data *data = i2c_get_clientdata(client);
> +
> + hwmon_device_unregister(data->hwmon_dev);
> + sysfs_remove_group(&client->dev.kobj, &g760a_group);
> + kfree(data);
> + i2c_set_clientdata(client, NULL);
> +
> + return 0;
> +}
> +
> +/* module management */
> +
> +static int __init g760a_init(void)
> +{
> + return i2c_add_driver(&g760a_driver);
> +}
> +
> +static void __exit g760a_exit(void)
> +{
> + i2c_del_driver(&g760a_driver);
> +}
> +
> +MODULE_AUTHOR("Herbert Valerio Riedel <hvr at gnu.org>");
> +MODULE_DESCRIPTION("GMT G760A driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(g760a_init);
> +module_exit(g760a_exit);
The rest of the code looks alright to me. Pretty impressive for a first
contribution. Well done!
--
Jean Delvare
More information about the lm-sensors
mailing list