Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Guenter Roeck]

On Sun, Sep 18, 2016 at 07:50:55PM -0500, Timothy Pearson wrote:
> Add a basic driver for the MAX31785, focusing on the fan control
> features but ignoring the temperature and voltage monitoring
> features of the device.
> 
> This driver supports all fan control modes and tachometer / PWM
> readback where applicable.
> 
Could you try using hwmon_device_register_with_info() as available in
linux-next ? Hopefully that should reduce driver size (and give the new API
some test coverage).

Please run the patch through checkpatch --strict. I don't expect you to
fix all the problems is reports, but the following should be addressed.

CHECK: Prefer kernel type 'u8' over 'uint8_t'
CHECK: Prefer kernel type 'u16' over 'uint16_t'
CHECK: Alignment should match open parenthesis
CHECK: Logical continuations should be on the previous line
WARNING: quoted string split across lines

Additional comments inline.

> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
> ---
>  Documentation/hwmon/max31785 |   36 +++
>  drivers/hwmon/Kconfig        |   10 +
>  drivers/hwmon/Makefile       |    1 +
>  drivers/hwmon/max31785.c     |  713 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 760 insertions(+)
>  create mode 100644 Documentation/hwmon/max31785
>  create mode 100644 drivers/hwmon/max31785.c
> 
> diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> new file mode 100644
> index 0000000..34dca74
> --- /dev/null
> +++ b/Documentation/hwmon/max31785
> @@ -0,0 +1,36 @@
> +Kernel driver max31785
> +======================
> +
> +Supported chips:
> +  * Maxim MAX31785
> +    Prefix: 'max31785'
> +    Addresses scanned: 0x52 0x53 0x54 0x55
> +    Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf
> +
> +Author: Timothy Pearson <tpearson@raptorengineering.com>
> +
> +
> +Description
> +-----------
> +
> +This driver implements support for the Maxim MAX31785 chip.
> +
> +The MAX31785 controls the speeds of up to six fans using six independent
> +PWM outputs. The desired fan speeds (or PWM duty cycles) are written
> +through the I2C interface. The outputs drive "4-wire" fans directly,
> +or can be used to modulate the fan's power terminals using an external
> +pass transistor.
> +
> +Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%)
> +monitoring and control of fan RPM as well as detection of fan failure.
> +
> +
> +Sysfs entries
> +-------------
> +
> +fan[1-6]_input           RO  fan tachometer speed in RPM
> +fan[1-6]_fault           RO  fan experienced fault
> +fan[1-6]_target          RW  desired fan speed in RPM
> +fan[1-6]_control_mode    RW  desired control mode: rpm, pwm, or auto

Please use pwm[]_enable (see API)

> +pwm[1-6]_enable          RW  output enabled, 0=disabled, 1=enabled

Per API: 0 = no fan speed control (full speed), 1 = manual fan speed control
enabled (using pwm[1-*]), 2+: automatic fan speed control

> +pwm[1-6]                 RW  fan target duty cycle (0-255)
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index eaf2f91..a202fd5 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -886,6 +886,16 @@ config SENSORS_MAX6697
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called max6697.
>  
> +config SENSORS_MAX31785
> +	tristate "Maxim MAX31785 sensor chip"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for 6-Channel PWM-Output
> +	  Fan RPM Controller.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called max31785.
> +
>  config SENSORS_MAX31790
>  	tristate "Maxim MAX31790 sensor chip"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index fe87d28..44c0c02 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -119,6 +119,7 @@ obj-$(CONFIG_SENSORS_MAX6639)	+= max6639.o
>  obj-$(CONFIG_SENSORS_MAX6642)	+= max6642.o
>  obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
>  obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
> +obj-$(CONFIG_SENSORS_MAX31785)	+= max31785.o
>  obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
>  obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
>  obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
> diff --git a/drivers/hwmon/max31785.c b/drivers/hwmon/max31785.c
> new file mode 100644
> index 0000000..9a23edd
> --- /dev/null
> +++ b/drivers/hwmon/max31785.c
> @@ -0,0 +1,713 @@
> +/*
> + * max31785.c - Part of lm_sensors, Linux kernel modules for hardware
> + *             monitoring.
> + *
> + * (C) 2016 Raptor Engineering, LLC
> + *
> + * 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.
> + *
> + * 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.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +/* MAX31785 registers */
> +#define MAX31785_REG_PAGE			0x00
> +#define MAX31785_PAGE_FAN_CONFIG(ch)		(0x00 + (ch))
> +#define MAX31785_REG_FAN_CONFIG_1_2		0x3a
> +#define MAX31785_REG_FAN_COMMAND_1		0x3b
> +#define MAX31785_REG_STATUS_FANS_1_2		0x81
> +#define MAX31785_REG_FAN_SPEED_1		0x90
> +#define MAX31785_REG_MFR_ID			0x99
> +#define MAX31785_REG_MFR_MODEL			0x9a
> +#define MAX31785_REG_MFR_FAN_CONFIG		0xf1
> +#define MAX31785_REG_READ_FAN_PWM		0xf3
> +
> +/* Fan Config register bits */
> +#define MAX31785_FAN_CFG_PWM_ENABLE		0x80
> +#define MAX31785_FAN_CFG_CONTROL_MODE_RPM	0x40
> +
> +/* Fan Status register bits */
> +#define MAX31785_FAN_STATUS_FAULT_MASK		0x80
> +
> +#define NR_CHANNEL				6
> +
> +/* Addresses to scan */
> +static const unsigned short normal_i2c[] = { 0x52, 0x53, 0x54, 0x55,
> +							I2C_CLIENT_END };
> +
> +/*
> + * Client data (each client gets its own)
> + */
> +struct max31785_data {
> +	struct i2c_client *client;
> +	struct mutex device_lock;
> +	bool valid; /* zero until following fields are valid */
> +	unsigned long last_updated; /* in jiffies */
> +
> +	/* register values */
> +	uint8_t fan_config[NR_CHANNEL];
> +	uint16_t fan_command[NR_CHANNEL];
> +	uint8_t mfr_fan_config[NR_CHANNEL];
> +	uint8_t fault_status[NR_CHANNEL];
> +	uint16_t tach_rpm[NR_CHANNEL];
> +	uint16_t pwm[NR_CHANNEL];
> +};
> +
> +static int max31785_set_page(struct i2c_client *client,
> +				uint8_t page)
> +{
> +	return i2c_smbus_write_byte_data(client,
> +			MAX31785_REG_PAGE,
> +			page);
> +}
> +
> +static int max31785_read_fan_data(struct i2c_client *client,
> +				uint8_t fan, uint8_t reg, uint8_t byte_mode)
> +{
> +	int rv;
> +
> +	rv = max31785_set_page(client, MAX31785_PAGE_FAN_CONFIG(fan));
> +	if (rv < 0)
> +		return rv;
> +
> +	if (byte_mode)
> +		rv = i2c_smbus_read_byte_data(client, reg);
> +	else
> +		rv = i2c_smbus_read_word_data(client, reg);
> +
> +	return rv;
> +}
> +
> +static int max31785_write_fan_data(struct i2c_client *client,
> +				uint8_t fan, uint8_t reg, uint16_t data,
> +				uint8_t byte_mode)
> +{
> +	int err;
> +
> +	err = max31785_set_page(client, MAX31785_PAGE_FAN_CONFIG(fan));
> +	if (err < 0)
> +		return err;
> +
> +	if (byte_mode)
> +		err = i2c_smbus_write_byte_data(client, reg, data);
> +	else
> +		err = i2c_smbus_write_word_data(client, reg, data);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static uint8_t is_automatic_control_mode(struct max31785_data *data,
> +			int index)

Please use bool as return value
> +{
> +	if (data->fan_command[index] > 0x7fff)
> +		return 1;
> +	else
> +		return 0;

	return data->fan_command[index] > 0x7fff;

> +}
> +
> +static struct max31785_data *max31785_update_device(struct device *dev)
> +{
> +	struct max31785_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	struct max31785_data *ret = data;
> +	int i;
> +	int rv;
> +
> +	mutex_lock(&data->device_lock);
> +
> +	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> +		for (i = 0; i < NR_CHANNEL; i++) {
> +			rv = max31785_read_fan_data(client, i,
> +					MAX31785_REG_STATUS_FANS_1_2, 1);
> +			if (rv < 0)
> +				goto abort;
> +			data->fault_status[i] = rv;
> +
> +			rv = max31785_read_fan_data(client, i,
> +					MAX31785_REG_FAN_SPEED_1, 0);
> +			if (rv < 0)
> +				goto abort;
> +			data->tach_rpm[i] = rv;
> +
> +			if ((data->fan_config[i]
> +				& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
> +				|| is_automatic_control_mode(data, i)) {
> +				rv = max31785_read_fan_data(client, i,
> +						MAX31785_REG_READ_FAN_PWM, 0);
> +				if (rv < 0)
> +					goto abort;
> +				data->pwm[i] = rv;
> +			}
> +
> +			if (!is_automatic_control_mode(data, i)) {
> +				/* Poke watchdog for manual fan control */
> +				rv = max31785_write_fan_data(client,
> +					i, MAX31785_REG_FAN_COMMAND_1,
> +					data->fan_command[i], 0);
> +				if (rv < 0)
> +					goto abort;
> +			}
> +		}
> +
> +		data->last_updated = jiffies;
> +		data->valid = true;
> +	}
> +	goto done;
> +
> +abort:
> +	data->valid = false;
> +	ret = ERR_PTR(rv);
> +
> +done:
> +	mutex_unlock(&data->device_lock);
> +
> +	return ret;
> +}
> +
> +static ssize_t get_fan(struct device *dev,
> +		       struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = max31785_update_device(dev);
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	return sprintf(buf, "%d\n", data->tach_rpm[attr->index]);
> +}
> +
> +static ssize_t get_fan_target(struct device *dev,
> +			      struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = max31785_update_device(dev);
> +	int rpm;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	if (data->fan_config[attr->index]
> +		& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
> +		rpm = data->fan_command[attr->index];
> +	else
> +		rpm = data->fan_command[attr->index] / 40; 

Why / 40 ?

> +
> +	return sprintf(buf, "%d\n", rpm);
> +}
> +
> +static ssize_t set_fan_target(struct device *dev,
> +			      struct device_attribute *devattr,
> +			      const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long rpm;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &rpm);
> +	if (err)
> +		return err;
> +
> +	if (rpm > 0x7fff)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->device_lock);
> +
> +	/* Switch fan to RPM mode */
> +	data->fan_config[attr->index] |= MAX31785_FAN_CFG_CONTROL_MODE_RPM;
> +	err = max31785_write_fan_data(client, attr->index,
> +				MAX31785_REG_FAN_CONFIG_1_2,
> +				data->fan_config[attr->index], 1);

We don't usually make such implied changes. The user should be able
to set the target speed and then request the mode change explicitly.

> +
> +	if (err < 0) {
> +		mutex_unlock(&data->device_lock);
> +		return err;
> +	}
> +
> +	/* Write new RPM value */
> +	data->fan_command[attr->index] = rpm;
> +	err = max31785_write_fan_data(client, attr->index,
> +				MAX31785_REG_FAN_COMMAND_1,
> +				data->fan_command[attr->index], 0);
> +
> +	mutex_unlock(&data->device_lock);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return count;
> +}
> +
> +static ssize_t get_pwm(struct device *dev,
> +		       struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = max31785_update_device(dev);
> +	int pwm;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	if ((data->fan_config[attr->index]
> +		& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
> +		|| is_automatic_control_mode(data, attr->index))
> +		pwm = data->pwm[attr->index] / 100;
> +	else
> +		pwm = data->fan_command[attr->index] / 40;

Why / 40 ?

> +
> +	return sprintf(buf, "%d\n", pwm);
> +}
> +
> +static ssize_t set_pwm(struct device *dev,
> +		       struct device_attribute *devattr,
> +		       const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long pwm;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &pwm);
> +	if (err)
> +		return err;
> +
> +	if (pwm > 255)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->device_lock);
> +
> +	/* Switch fan to PWM mode */
> +	data->fan_config[attr->index] &= ~MAX31785_FAN_CFG_CONTROL_MODE_RPM;
> +	err = max31785_write_fan_data(client, attr->index,
> +				MAX31785_REG_FAN_CONFIG_1_2,
> +				data->fan_config[attr->index], 1);
> +
Another implied mode change ...

> +	if (err < 0) {
> +		mutex_unlock(&data->device_lock);
> +		return err;
> +	}
> +
> +	/* Write new PWM value */
> +	data->fan_command[attr->index] = pwm * 40;

Where is the * 40 coming from ?

> +	err = max31785_write_fan_data(client, attr->index,
> +				MAX31785_REG_FAN_COMMAND_1,
> +				data->fan_command[attr->index], 0);
> +
> +	mutex_unlock(&data->device_lock);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return count;
> +}
> +
> +static ssize_t get_pwm_enable(struct device *dev,
> +			      struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = max31785_update_device(dev);
> +	int enabled;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	if (data->fan_config[attr->index] & MAX31785_FAN_CFG_PWM_ENABLE)
> +		enabled = 1;
> +	else
> +		enabled = 0;

	enabled = !!(data->fan_config[attr->index] &
		     MAX31785_FAN_CFG_PWM_ENABLE);
> +
> +	return sprintf(buf, "%d\n", enabled);
> +}
> +
> +static ssize_t set_pwm_enable(struct device *dev,
> +			      struct device_attribute *devattr,
> +			      const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long enabled;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &enabled);
> +	if (err)
> +		return err;
> +
> +	switch (enabled) {
> +	case 0:
> +		data->fan_config[attr->index] =
> +			data->fan_config[attr->index]
> +			& ~MAX31785_FAN_CFG_PWM_ENABLE;
> +		break;
> +	case 1:
> +		data->fan_config[attr->index] =
> +			data->fan_config[attr->index]
> +			 | MAX31785_FAN_CFG_PWM_ENABLE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&data->device_lock);
> +
> +	err = max31785_write_fan_data(client, attr->index,
> +				MAX31785_REG_FAN_CONFIG_1_2,
> +				data->fan_config[attr->index], 1);
> +
> +	mutex_unlock(&data->device_lock);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return count;
> +}
> +
> +static ssize_t get_control_mode(struct device *dev,
> +			      struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = max31785_update_device(dev);
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	if (is_automatic_control_mode(data, attr->index))
> +		return sprintf(buf, "auto\n");
> +	else if (data->fan_config[attr->index]
> +				& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
> +		return sprintf(buf, "rpm\n");
> +
> +	return sprintf(buf, "pwm\n");
> +}
> +
> +static ssize_t set_control_mode(struct device *dev,
> +			      struct device_attribute *devattr,
> +			      const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int err;
> +
> +	if (!strcmp(buf, "auto") || !strcmp(buf, "auto\n")) {
> +		data->fan_command[attr->index] = 0xffff;
> +	} else if (!strcmp(buf, "pwm") || !strcmp(buf, "pwm\n")) {
> +		data->fan_config[attr->index] =
> +			data->fan_config[attr->index]
> +			& ~MAX31785_FAN_CFG_CONTROL_MODE_RPM;
> +	} else if (!strcmp(buf, "rpm") || !strcmp(buf, "rpm\n")) {
> +		data->fan_config[attr->index] =
> +			data->fan_config[attr->index]
> +			| MAX31785_FAN_CFG_CONTROL_MODE_RPM;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&data->device_lock);
> +
> +	if (data->fan_command[attr->index] == 0xffff)
> +		err = max31785_write_fan_data(client, attr->index,
> +					MAX31785_REG_FAN_COMMAND_1,
> +					data->fan_command[attr->index], 0);
> +	else
> +		err = max31785_write_fan_data(client, attr->index,
> +					MAX31785_REG_FAN_CONFIG_1_2,
> +					data->fan_config[attr->index], 1);
> +
> +	mutex_unlock(&data->device_lock);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return count;
> +}
> +
> +static ssize_t get_fan_fault(struct device *dev,
> +			     struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct max31785_data *data = max31785_update_device(dev);
> +	int fault;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	fault = !!(data->fault_status[attr->index]
> +			& MAX31785_FAN_STATUS_FAULT_MASK);
> +
> +	return sprintf(buf, "%d\n", fault);
> +}
> +
> +static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, get_fan, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, get_fan, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan3_input, S_IRUGO, get_fan, NULL, 2);
> +static SENSOR_DEVICE_ATTR(fan4_input, S_IRUGO, get_fan, NULL, 3);
> +static SENSOR_DEVICE_ATTR(fan5_input, S_IRUGO, get_fan, NULL, 4);
> +static SENSOR_DEVICE_ATTR(fan6_input, S_IRUGO, get_fan, NULL, 5);
> +
> +static SENSOR_DEVICE_ATTR(fan1_fault, S_IRUGO, get_fan_fault, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan2_fault, S_IRUGO, get_fan_fault, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan3_fault, S_IRUGO, get_fan_fault, NULL, 2);
> +static SENSOR_DEVICE_ATTR(fan4_fault, S_IRUGO, get_fan_fault, NULL, 3);
> +static SENSOR_DEVICE_ATTR(fan5_fault, S_IRUGO, get_fan_fault, NULL, 4);
> +static SENSOR_DEVICE_ATTR(fan6_fault, S_IRUGO, get_fan_fault, NULL, 5);
> +
> +static SENSOR_DEVICE_ATTR(fan1_target, S_IWUSR | S_IRUGO,
> +		get_fan_target, set_fan_target, 0);
> +static SENSOR_DEVICE_ATTR(fan2_target, S_IWUSR | S_IRUGO,
> +		get_fan_target, set_fan_target, 1);
> +static SENSOR_DEVICE_ATTR(fan3_target, S_IWUSR | S_IRUGO,
> +		get_fan_target, set_fan_target, 2);
> +static SENSOR_DEVICE_ATTR(fan4_target, S_IWUSR | S_IRUGO,
> +		get_fan_target, set_fan_target, 3);
> +static SENSOR_DEVICE_ATTR(fan5_target, S_IWUSR | S_IRUGO,
> +		get_fan_target, set_fan_target, 4);
> +static SENSOR_DEVICE_ATTR(fan6_target, S_IWUSR | S_IRUGO,
> +		get_fan_target, set_fan_target, 5);
> +
> +static SENSOR_DEVICE_ATTR(fan1_control_mode, S_IWUSR | S_IRUGO,
> +		get_control_mode, set_control_mode, 0);
> +static SENSOR_DEVICE_ATTR(fan2_control_mode, S_IWUSR | S_IRUGO,
> +		get_control_mode, set_control_mode, 1);
> +static SENSOR_DEVICE_ATTR(fan3_control_mode, S_IWUSR | S_IRUGO,
> +		get_control_mode, set_control_mode, 2);
> +static SENSOR_DEVICE_ATTR(fan4_control_mode, S_IWUSR | S_IRUGO,
> +		get_control_mode, set_control_mode, 3);
> +static SENSOR_DEVICE_ATTR(fan5_control_mode, S_IWUSR | S_IRUGO,
> +		get_control_mode, set_control_mode, 4);
> +static SENSOR_DEVICE_ATTR(fan6_control_mode, S_IWUSR | S_IRUGO,
> +		get_control_mode, set_control_mode, 5);
> +
> +static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 0);
> +static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 1);
> +static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 2);
> +static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 3);
> +static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 4);
> +static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 5);
> +
> +static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
> +		get_pwm_enable, set_pwm_enable, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
> +		get_pwm_enable, set_pwm_enable, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_enable, S_IWUSR | S_IRUGO,
> +		get_pwm_enable, set_pwm_enable, 2);
> +static SENSOR_DEVICE_ATTR(pwm4_enable, S_IWUSR | S_IRUGO,
> +		get_pwm_enable, set_pwm_enable, 3);
> +static SENSOR_DEVICE_ATTR(pwm5_enable, S_IWUSR | S_IRUGO,
> +		get_pwm_enable, set_pwm_enable, 4);
> +static SENSOR_DEVICE_ATTR(pwm6_enable, S_IWUSR | S_IRUGO,
> +		get_pwm_enable, set_pwm_enable, 5);
> +
> +static struct attribute *max31785_attrs[] = {
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,
> +	&sensor_dev_attr_fan3_input.dev_attr.attr,
> +	&sensor_dev_attr_fan4_input.dev_attr.attr,
> +	&sensor_dev_attr_fan5_input.dev_attr.attr,
> +	&sensor_dev_attr_fan6_input.dev_attr.attr,
> +
> +	&sensor_dev_attr_fan1_fault.dev_attr.attr,
> +	&sensor_dev_attr_fan2_fault.dev_attr.attr,
> +	&sensor_dev_attr_fan3_fault.dev_attr.attr,
> +	&sensor_dev_attr_fan4_fault.dev_attr.attr,
> +	&sensor_dev_attr_fan5_fault.dev_attr.attr,
> +	&sensor_dev_attr_fan6_fault.dev_attr.attr,
> +
> +	&sensor_dev_attr_fan1_target.dev_attr.attr,
> +	&sensor_dev_attr_fan2_target.dev_attr.attr,
> +	&sensor_dev_attr_fan3_target.dev_attr.attr,
> +	&sensor_dev_attr_fan4_target.dev_attr.attr,
> +	&sensor_dev_attr_fan5_target.dev_attr.attr,
> +	&sensor_dev_attr_fan6_target.dev_attr.attr,
> +
> +	&sensor_dev_attr_fan1_control_mode.dev_attr.attr,
> +	&sensor_dev_attr_fan2_control_mode.dev_attr.attr,
> +	&sensor_dev_attr_fan3_control_mode.dev_attr.attr,
> +	&sensor_dev_attr_fan4_control_mode.dev_attr.attr,
> +	&sensor_dev_attr_fan5_control_mode.dev_attr.attr,
> +	&sensor_dev_attr_fan6_control_mode.dev_attr.attr,
> +
> +	&sensor_dev_attr_pwm1.dev_attr.attr,
> +	&sensor_dev_attr_pwm2.dev_attr.attr,
> +	&sensor_dev_attr_pwm3.dev_attr.attr,
> +	&sensor_dev_attr_pwm4.dev_attr.attr,
> +	&sensor_dev_attr_pwm5.dev_attr.attr,
> +	&sensor_dev_attr_pwm6.dev_attr.attr,
> +
> +	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm4_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm5_enable.dev_attr.attr,
> +	&sensor_dev_attr_pwm6_enable.dev_attr.attr,
> +	NULL
> +};
> +
> +static umode_t max31785_attrs_visible(struct kobject *kobj,
> +				     struct attribute *a, int n)
> +{
> +	return a->mode;
> +}
> +
> +static const struct attribute_group max31785_group = {
> +	.attrs = max31785_attrs,
> +	.is_visible = max31785_attrs_visible,
> +};
> +__ATTRIBUTE_GROUPS(max31785);
> +
> +static int max31785_init_client(struct i2c_client *client,
> +				struct max31785_data *data)
> +{
> +	int i, rv;
> +
> +	dev_info(&client->dev, "Reading device state\n");
> +
Noise; please drop.

> +	mutex_lock(&data->device_lock);
> +
This is only called from probe; setting the mutex is unnecessary.

> +	for (i = 0; i < NR_CHANNEL; i++) {
> +		rv = max31785_read_fan_data(client, i,
> +				MAX31785_REG_FAN_CONFIG_1_2, 1);
> +		if (rv < 0)
> +			goto abort;
> +		data->fan_config[i] = rv;
> +
> +		rv = max31785_read_fan_data(client, i,
> +				MAX31785_REG_FAN_COMMAND_1, 0);
> +		if (rv < 0)
> +			goto abort;
> +		data->fan_command[i] = rv;
> +
> +		rv = max31785_read_fan_data(client, i,
> +				MAX31785_REG_MFR_FAN_CONFIG, 1);
> +		if (rv < 0)
> +			goto abort;
> +		data->mfr_fan_config[i] = rv;
> +
> +		if (!((data->fan_config[i]
> +			& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
> +			|| is_automatic_control_mode(data, i))) {
> +			data->pwm[i] = 0;
> +		}
> +	}
> +
> +	dev_info(&client->dev, "Driver initialized\n");

Noise; please drop.

> +
> +abort:
> +	mutex_unlock(&data->device_lock);
> +	return rv;
> +}
> +
> +/* Return 0 if detection is successful, -ENODEV otherwise */
> +static int max31785_detect(struct i2c_client *client,
> +			  struct i2c_board_info *info)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	int rv, manufacturer, model;
> +
> +	if (!i2c_check_functionality(adapter,
> +			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
> +		return -ENODEV;
> +
> +	 dev_info(&adapter->dev, "Probing for MAX31785\n");
> +
Moise; please drop.

> +	/* Probe manufacturer / model registers */
> +	rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_ID);
> +	if (rv < 0)
> +		return -ENODEV;
> +	manufacturer = rv;

The manufacturer variable is unnecessary. Also see below.

	rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_ID);
	if (rv < 0 || rv != 0x4d)
		return -ENODEV;

> +
> +	rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_MODEL);
> +	if (rv < 0)
> +		return -ENODEV;
> +	model = rv;
> +
> +	if (manufacturer != 0x4d)
> +		return -ENODEV;
> +	if (model != 0x53)
> +		return -ENODEV;
> +
> +	dev_info(&adapter->dev,
> +		"Detected MAX31785 at %d,0x%02x with"
> +		" MANUFACTURER: 0x%02x MODEL: %02x\n",
> +		i2c_adapter_id(client->adapter), client->addr,
> +		manufacturer, model);

Output of manufacturer and model is unnecessary. It will always be 0x4d and
0x53.

> +
> +	strlcpy(info->type, "max31785", I2C_NAME_SIZE);
> +
> +	return 0;
> +}
> +
> +static int max31785_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	struct device *dev = &client->dev;
> +	struct max31785_data *data;
> +	struct device *hwmon_dev;
> +	int err;
> +
> +	if (!i2c_check_functionality(adapter,
> +			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(dev, sizeof(struct max31785_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	mutex_init(&data->device_lock);
> +
> +	/*
> +	 * Initialize the max31785 chip
> +	 */
> +	err = max31785_init_client(client, data);
> +	if (err)
> +		return err;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> +			client->name, data, max31785_groups);
> +
As mentioned above, please try using devm_hwmon_device_register_with_info().
You'll find the documentation and some converted drivers in -next.

> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id max31785_id[] = {
> +	{ "max31785", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max31785_id);
> +
> +static struct i2c_driver max31785_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.probe		= max31785_probe,
> +	.driver = {
> +		.name	= "max31785",
> +	},
> +	.id_table	= max31785_id,
> +	.detect		= max31785_detect,
> +	.address_list	= normal_i2c,
> +};
> +
> +module_i2c_driver(max31785_driver);
> +
> +MODULE_AUTHOR("Timothy Pearson <tpearson@raptorengineering.com>");
> +MODULE_DESCRIPTION("MAX31785 sensor driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/18/2016 08:41 PM, Guenter Roeck wrote:
> On Sun, Sep 18, 2016 at 07:50:55PM -0500, Timothy Pearson wrote:
>> Add a basic driver for the MAX31785, focusing on the fan control
>> features but ignoring the temperature and voltage monitoring
>> features of the device.
>>
>> This driver supports all fan control modes and tachometer / PWM
>> readback where applicable.
>>
> Could you try using hwmon_device_register_with_info() as available in
> linux-next ? Hopefully that should reduce driver size (and give the new API
> some test coverage).

Ordinarily I'd be willing to try this, but in this case I wouldn't be
able to test the module on real hardware; it's an embedded system and
uses the 4.7 kernel at the moment.  Would you be OK with merging a fixed
version of this patch and I can throw together an untested variant using
hwmon_device_register_with_info() for future work?

> Please run the patch through checkpatch --strict. I don't expect you to
> fix all the problems is reports, but the following should be addressed.
> 
> CHECK: Prefer kernel type 'u8' over 'uint8_t'
> CHECK: Prefer kernel type 'u16' over 'uint16_t'
> CHECK: Alignment should match open parenthesis
> CHECK: Logical continuations should be on the previous line
> WARNING: quoted string split across lines

My fault, I completely forgot to run checkpatch.  I'll get a V2 together
shortly including addressing the inline comments.

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX4CsUAAoJEK+E3vEXDOFb++AH/i1FxEmOJmNVlSowpV5EHsgQ
xcquFnmeF/pK6Oh00LbcI3WvTCgVRBkUl32WG7m5ktKXLDn5fU9DFEQYQVVhzRFU
jz83JyD8HvDhOKubk9G9mqE518ZBIEnYiREHFI9fVuOnUTVemrZtYdKdoA7ez3wi
5t7B7HJ4Gdcyi1FkNwLu+0/N7eLGyR9WfQAFqM0F64qRfQGOxWtwoi4lYgmbxHCg
NFs7LAoJsj00K4DEPiK/3jjy5usfBpcIgkECAYK7+A/sUtDTq+TrBr1ADZY0mbV3
zqDo85alN5XQEvLTjACWXmauMhCUsV4OqzurtlBAVMJQDW6ZKeCnhxv+iUaDxh8=
=Rr2j
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Guenter Roeck]

On Mon, Sep 19, 2016 at 01:14:49PM -0500, Timothy Pearson wrote:
> On 09/18/2016 08:41 PM, Guenter Roeck wrote:
> > On Sun, Sep 18, 2016 at 07:50:55PM -0500, Timothy Pearson wrote:
> >> Add a basic driver for the MAX31785, focusing on the fan control
> >> features but ignoring the temperature and voltage monitoring
> >> features of the device.
> >>
> >> This driver supports all fan control modes and tachometer / PWM
> >> readback where applicable.
> >>
> > Could you try using hwmon_device_register_with_info() as available in
> > linux-next ? Hopefully that should reduce driver size (and give the new API
> > some test coverage).
> 
> Ordinarily I'd be willing to try this, but in this case I wouldn't be
> able to test the module on real hardware; it's an embedded system and
> uses the 4.7 kernel at the moment.  Would you be OK with merging a fixed
> version of this patch and I can throw together an untested variant using
> hwmon_device_register_with_info() for future work?
> 
After actually looking into the datasheet, I must admit that I don't
understand why you did not model this as a PMBus driver.

Guenter

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/18/2016 08:41 PM, Guenter Roeck wrote:
>> +Sysfs entries
>> +-------------
>> +
>> +fan[1-6]_input           RO  fan tachometer speed in RPM
>> +fan[1-6]_fault           RO  fan experienced fault
>> +fan[1-6]_target          RW  desired fan speed in RPM
>> +fan[1-6]_control_mode    RW  desired control mode: rpm, pwm, or auto
> 
> Please use pwm[]_enable (see API)
> 
>> +pwm[1-6]_enable          RW  output enabled, 0=disabled, 1=enabled
> 
> Per API: 0 = no fan speed control (full speed), 1 = manual fan speed control
> enabled (using pwm[1-*]), 2+: automatic fan speed control
> 
>> +pwm[1-6]                 RW  fan target duty cycle (0-255)

This particular device has a per-output PWM disable bit.  How should
this be handled?  I don't want to globally enable outputs that are not
connector or may possibly be incorrectly connected depending on hardware
design.

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX4DEgAAoJEK+E3vEXDOFbAz4IAKFCx0Mu6Q5HlsA5IloV1YcP
qj5je1vBONSTxM1cfaYjLmZzUBVecbJZojQjhkoUXxrAkrtmrM9otHfw7k0qs/0q
ppoVevrfp2jKMzvoBp3VytHTZy3rK1Oyxko1dIuwBHMvfMNNk1S8tm132jhv34bK
C1z2l16kU7e99NQV1i4HvB8FyrwotHfPXuZuM1ukO+CmfhLsUzXGYcOsXPApKtdg
CHijAp9qa+YmcEqmcNksncJn769IEhKc1aKWkjcqHg2chmXtz1aR9STzNRnHZJO8
RHivo6/9IGVNCOZSuup4HKhF1kle65W9tvS8D2eQnh/no+8rWHtMm+EZ+WEWFIs=
=V8us
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Guenter Roeck]

On Mon, Sep 19, 2016 at 01:40:36PM -0500, Timothy Pearson wrote:
> On 09/18/2016 08:41 PM, Guenter Roeck wrote:
> >> +Sysfs entries
> >> +-------------
> >> +
> >> +fan[1-6]_input           RO  fan tachometer speed in RPM
> >> +fan[1-6]_fault           RO  fan experienced fault
> >> +fan[1-6]_target          RW  desired fan speed in RPM
> >> +fan[1-6]_control_mode    RW  desired control mode: rpm, pwm, or auto
> > 
> > Please use pwm[]_enable (see API)
> > 
> >> +pwm[1-6]_enable          RW  output enabled, 0=disabled, 1=enabled
> > 
> > Per API: 0 = no fan speed control (full speed), 1 = manual fan speed control
> > enabled (using pwm[1-*]), 2+: automatic fan speed control
> > 
> >> +pwm[1-6]                 RW  fan target duty cycle (0-255)
> 
> This particular device has a per-output PWM disable bit.  How should
> this be handled?  I don't want to globally enable outputs that are not
> connector or may possibly be incorrectly connected depending on hardware
> design.
> 
Not sure I understand. pwm[1-6]_enable = 0 means no fan speed control.
Do you mean the (full speed) comment ? 

Having said that, looking into the datasheet, I see that this is actually
a PMBus device. Why don't you just write a PMBus extension driver for it ?
AFAICS drivers/hwmon/pmbus/max34440.c would come pretty close and could
possibly even be extended to support this chip.

Guenter

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/19/2016 03:04 PM, Guenter Roeck wrote:
> On Mon, Sep 19, 2016 at 01:40:36PM -0500, Timothy Pearson wrote:
> Not sure I understand. pwm[1-6]_enable = 0 means no fan speed control.
> Do you mean the (full speed) comment ? 

"no fan speed control (i.e. fan at full speed)".  What I would like to
see is "fan disabled (PWM turned off)".

Is that an acceptable use of the "0" value?

> Having said that, looking into the datasheet, I see that this is actually
> a PMBus device. Why don't you just write a PMBus extension driver for it ?
> AFAICS drivers/hwmon/pmbus/max34440.c would come pretty close and could
> possibly even be extended to support this chip.
> 
> Guenter

The datasheet states that many functions are not available over PMBus.
Rather than fight with potentially incomplete silicon support for PMBus
it was easier to simply write this driver.

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX4EsiAAoJEK+E3vEXDOFbU7sH/1fcxUbs5inK+XL8tdpMnLDK
y6uhsYMQaGzea4f4B6ek9XDDIyGJyMRwkAZy/4v9f+uqhK03DBrwRuY9zAeLPDd5
ilWPKNSJLNqs7vSOHz1f/ooIR20medAdx2oOAVX3QwoQjv/L40Sm9iLAmh/RQJtt
urvLzcnBbhVnTVshuFwc2fwqlszLtaF9XUlPGzEWVZY0d7z+tPXwiseg/3nd444V
LezNTkLgM9zkXwLz402zK9ckO5wABlbO1jd5mssLMnttyw4ilBwEpXgeeUlcsS71
GguTJnU35Wn0tD7DRUbqB76Jx5NX9Su/wHJ8X0hbe8CGAOHRzTYynTEzTHfGVFc=
=XiuV
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Guenter Roeck]

On Mon, Sep 19, 2016 at 03:31:33PM -0500, Timothy Pearson wrote:
> On 09/19/2016 03:04 PM, Guenter Roeck wrote:
> > On Mon, Sep 19, 2016 at 01:40:36PM -0500, Timothy Pearson wrote:
> > Not sure I understand. pwm[1-6]_enable = 0 means no fan speed control.
> > Do you mean the (full speed) comment ? 
> 
> "no fan speed control (i.e. fan at full speed)".  What I would like to
> see is "fan disabled (PWM turned off)".
> 
> Is that an acceptable use of the "0" value?
> 
> > Having said that, looking into the datasheet, I see that this is actually
> > a PMBus device. Why don't you just write a PMBus extension driver for it ?
> > AFAICS drivers/hwmon/pmbus/max34440.c would come pretty close and could
> > possibly even be extended to support this chip.
> > 
> > Guenter
> 
> The datasheet states that many functions are not available over PMBus.
> Rather than fight with potentially incomplete silicon support for PMBus
> it was easier to simply write this driver.
> 
And then you are using all pmbus commands ? Seems odd.

I guess I'l buy an evaluation board if one is available and check if your claim
is correct. I am not inclined to accept a non-pmbus driver for a pmbus
device without good reason.

Guenter

> -- 
> Timothy Pearson
> Raptor Engineering
> +1 (415) 727-8645 (direct line)
> +1 (512) 690-0200 (switchboard)
> https://www.raptorengineering.com

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/19/2016 07:54 PM, Guenter Roeck wrote:
> On Mon, Sep 19, 2016 at 03:31:33PM -0500, Timothy Pearson wrote:
>> On 09/19/2016 03:04 PM, Guenter Roeck wrote:
> And then you are using all pmbus commands ? Seems odd.
> 
> I guess I'l buy an evaluation board if one is available and check if your claim
> is correct. I am not inclined to accept a non-pmbus driver for a pmbus
> device without good reason.
> 
> Guenter

Well, I went back over the datasheet and I may have misinterpreted some
of the introductory text.  In any case I'm not likely to go back and
re-do this as a pmbus driver as the original was done in my free time
and works well enough for our needs at the moment.  When the patch no
longer applies perhaps someone else can pick up the porting effort.

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX4JPYAAoJEK+E3vEXDOFb8koIAK1DQkicpt/CKU5vhGAxw8m9
MYGg+e+fe4s+PD3NmP0WZMAvDLtXOxYCBPRjqZWb4RJdfHNoAjjyJIFlDtG3UkBT
/WIDO3gRADJLopabWSu591IKHfgHr/4v6+mfg8R2ZHrtbNNVnDlj1qnPanfPrskm
h4Nq2Z1PPCBjmGA/HPr9NAkAASQmdcDRKkpO5k1zhUx5L4YL7QNQFphZxRkeiCKp
mpA3hQIYayItCuRCt+PQWqb3dMB1hOkyEQP+xDzaLCkrajTt4wrh+lgjIfPEQZwC
F71wiRuq5FVhZDqg/ko4/lkuwgmiXWR9lOpo7WSHHoFaYg54teFrYX3Z6Ci0Ug4=
=ueAd
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/19/2016 07:54 PM, Guenter Roeck wrote:
>> On 09/19/2016 03:04 PM, Guenter Roeck wrote:
>> And then you are using all pmbus commands ? Seems odd.
> 
> I guess I'l buy an evaluation board if one is available and check if your claim
> is correct. I am not inclined to accept a non-pmbus driver for a pmbus
> device without good reason.
> 
> Guenter

Regarding the PMBus driver, I looked over the documentation available
here: https://www.kernel.org/doc/Documentation/hwmon/pmbus

- From what I can tell PMBus drivers do not support configuring the fan
control parameters, only monitoring the fan status and speed.  Is this
correct, and if not where would I find the correct documentation?

Thank you,

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX4ZECAAoJEK+E3vEXDOFb4XsIALCbk+/dO2LdTvfjDCVMrs9J
2IHDW3heUZn53v5evpMEvJCXEw8J0ARg2cYWxuzx8Ufu5hGzXNS40NW5mUHtkbXG
FNWAqoSdeT1f9aOS8XIi/9+u27ZwJTuI5dZdb7nebfvsjlKc1CKw2FwtqUUBkyZA
Ywa/kjSlEMTO2wGCea4xQXE8aO3UK3JHc0sUwzR3htEOp5nGIn7d8ScDaJvpzW+a
xk0q97U4ezJEm2r3WmXpZGbUQqM2aaOdTSl9iiV7GZBAR6K50LGsacAcRBdanJaj
FOuW9XZt0NjnrZOsMc2N9WIz9wXKYOuo/Y8d/0hVPp6SQ3po5d8coeNrSPDw1qw=
=Q8So
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Guenter Roeck]

On Tue, Sep 20, 2016 at 02:41:56PM -0500, Timothy Pearson wrote:
> On 09/19/2016 07:54 PM, Guenter Roeck wrote:
> >> On 09/19/2016 03:04 PM, Guenter Roeck wrote:
> >> And then you are using all pmbus commands ? Seems odd.
> > 
> > I guess I'l buy an evaluation board if one is available and check if your claim
> > is correct. I am not inclined to accept a non-pmbus driver for a pmbus
> > device without good reason.
> > 
> > Guenter
> 
> Regarding the PMBus driver, I looked over the documentation available
> here: https://www.kernel.org/doc/Documentation/hwmon/pmbus
> 
> From what I can tell PMBus drivers do not support configuring the fan
> control parameters, only monitoring the fan status and speed.  Is this
> correct, and if not where would I find the correct documentation?
> 
So far that wasn't needed. That doesn't mean it can not be added.

I orderd an evaluation board and will likely spend some time on it myself
after I get it.

Guenter

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/20/2016 02:59 PM, Guenter Roeck wrote:
> On Tue, Sep 20, 2016 at 02:41:56PM -0500, Timothy Pearson wrote:
>> On 09/19/2016 07:54 PM, Guenter Roeck wrote:
>>>> On 09/19/2016 03:04 PM, Guenter Roeck wrote:
>>>> And then you are using all pmbus commands ? Seems odd.
>>>
>>> I guess I'l buy an evaluation board if one is available and check if your claim
>>> is correct. I am not inclined to accept a non-pmbus driver for a pmbus
>>> device without good reason.
>>>
>>> Guenter
>>
>> Regarding the PMBus driver, I looked over the documentation available
>> here: https://www.kernel.org/doc/Documentation/hwmon/pmbus
>>
>> From what I can tell PMBus drivers do not support configuring the fan
>> control parameters, only monitoring the fan status and speed.  Is this
>> correct, and if not where would I find the correct documentation?
>>
> So far that wasn't needed. That doesn't mean it can not be added.
> 
> I orderd an evaluation board and will likely spend some time on it myself
> after I get it.
> 
> Guenter

OK, sounds good.  I looked back in the notes for this project and we had
originally considered using a PMBus driver but ran into the kernel
documentation, noted the missing control features, and I subsequently
misinterpreted the datasheet using the kernel docs as a reference.  This
is why the hwmon driver was implemented.

Please let me know if I can be of any assistance.

Thanks!

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX4ZWeAAoJEK+E3vEXDOFbSNkH/3IqLAbvz2aVyPUBhKBFHC72
0uaP/IhBIoDoKAdZ50jJljYQIub0hn6HrQH52SYXvNhkO43+UJwqOxIRliipueDg
X9H2esdeaptjI+O3dXCv5bLyMgy+ohGv1aPkxon+BTo506NWlHOyYlbCscIgmuMu
nsMmHVGQvEyMtbJHlwmT0tjE81IEYxFxc1eLPbPhcrdz1f6yIAY7tHFo5ZjE+wt8
mzU35ZI+zIkKbD88Qfs8Ca/aTfRzt5xDKHDI3//YouyiqlnDK7etoZrAtx2RxtcJ
Qv8k2udTpQuuYwwGobwC0SSWaxeRMlRaumX+ZkIYJz7xSp1krnArx5na2r2cTr8=
=Guwk
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Guenter Roeck]

On 09/20/2016 01:01 PM, Timothy Pearson wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 09/20/2016 02:59 PM, Guenter Roeck wrote:
>> On Tue, Sep 20, 2016 at 02:41:56PM -0500, Timothy Pearson wrote:
>>> On 09/19/2016 07:54 PM, Guenter Roeck wrote:
>>>>> On 09/19/2016 03:04 PM, Guenter Roeck wrote:
>>>>> And then you are using all pmbus commands ? Seems odd.
>>>>
>>>> I guess I'l buy an evaluation board if one is available and check if your claim
>>>> is correct. I am not inclined to accept a non-pmbus driver for a pmbus
>>>> device without good reason.
>>>>
>>>> Guenter
>>>
>>> Regarding the PMBus driver, I looked over the documentation available
>>> here: https://www.kernel.org/doc/Documentation/hwmon/pmbus
>>>
>>> From what I can tell PMBus drivers do not support configuring the fan
>>> control parameters, only monitoring the fan status and speed.  Is this
>>> correct, and if not where would I find the correct documentation?
>>>
>> So far that wasn't needed. That doesn't mean it can not be added.
>>
>> I orderd an evaluation board and will likely spend some time on it myself
>> after I get it.
>>
>> Guenter
>
> OK, sounds good.  I looked back in the notes for this project and we had
> originally considered using a PMBus driver but ran into the kernel
> documentation, noted the missing control features, and I subsequently

It is always useful to talk with the maintainer.

> misinterpreted the datasheet using the kernel docs as a reference.  This
> is why the hwmon driver was implemented.
>

I am still not sure I understand what irked you off in the datasheet.
Is it the "supports a subset of the commands defined in the PMBus
Specfication" ? If so, please keep in mind that every single chip
supporting PMBus will only support a subset of PMBus commands.

> Please let me know if I can be of any assistance.
>
Sure, I'll let you know.

Thanks,
Guenter

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/20/2016 10:04 PM, Guenter Roeck wrote:
> On 09/20/2016 01:01 PM, Timothy Pearson wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 09/20/2016 02:59 PM, Guenter Roeck wrote:
>> OK, sounds good.  I looked back in the notes for this project and we had
>> originally considered using a PMBus driver but ran into the kernel
>> documentation, noted the missing control features, and I subsequently
> 
> It is always useful to talk with the maintainer.

I'll keep that in mind next time.  This particular area of the kernel
seems much more responsive than some of the others that we've had the
misfortune of trying to work with; multiple weeks of delay for a
response to a simple two line patch tends to turn one off of the mailing
lists...

>> misinterpreted the datasheet using the kernel docs as a reference.  This
>> is why the hwmon driver was implemented.
>>
> 
> I am still not sure I understand what irked you off in the datasheet.
> Is it the "supports a subset of the commands defined in the PMBus
> Specfication" ? If so, please keep in mind that every single chip
> supporting PMBus will only support a subset of PMBus commands.

Yes, I definitely understand that now.  This was the line that, when
taken out of context and compared against the kernel documentation,
caused an incorrect assumption on my part.

>> Please let me know if I can be of any assistance.
>>
> Sure, I'll let you know.
> 
> Thanks,
> Guenter
> 


- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX4fnKAAoJEK+E3vEXDOFbyJEH/AqkLDgx6BgxcHRfl63G9YlE
ln97m5X8NrWkGDQajzPHtv4g7imrli4ztTfJgjLnEPKu257clh1pEd32/gWKZYp5
tnewbANOjOg4E/75bM7I1hFSN4LXylwiPGPtNdtPqmHDsKk3RDLSweFn2wuKIvuP
rMJVmZHElbZdM5BenWWBEaqJxkUe8SkWfwb4CcIPNigETMZckvpzP+Db9qm2t5pq
Go3lYbamLK2KcrMngRTtnH3EmJY7KXa0+GsgmQthkxYA7ZvDG9DRBLFpPECEWZfr
HZrld5hwnzGwh2GkTTvO1JUQvv5wrtu1tXAxR5EPCWWDjqHW3dUzG1WgmsSJ+mI=
=xXkP
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/20/2016 10:04 PM, Guenter Roeck wrote:
> On 09/20/2016 01:01 PM, Timothy Pearson wrote:
>> On 09/20/2016 02:59 PM, Guenter Roeck wrote:
>>
>> OK, sounds good.  I looked back in the notes for this project and we had
>> originally considered using a PMBus driver but ran into the kernel
>> documentation, noted the missing control features, and I subsequently
> 
> It is always useful to talk with the maintainer.
> 
>> misinterpreted the datasheet using the kernel docs as a reference.  This
>> is why the hwmon driver was implemented.
>>
> 
> I am still not sure I understand what irked you off in the datasheet.
> Is it the "supports a subset of the commands defined in the PMBus
> Specfication" ? If so, please keep in mind that every single chip
> supporting PMBus will only support a subset of PMBus commands.
> 
>> Please let me know if I can be of any assistance.
>>
> Sure, I'll let you know.
> 
> Thanks,
> Guenter
> 

Just checking back in to see how things are going with this driver.  I
don't really know how involved the surgery to PMBus will be to get this
device working; are we looking at weeks / months or something sooner?

Thanks again!

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX/PmkAAoJEK+E3vEXDOFbYVQIAKhoDT+kU+fR2CML4S3wYRQP
fjI/vJG6D3UPzb8M8IvCpGVSxR4bIl1uLC3pftT5TbF93XHXd6M39MUJYcoWRjnE
iB//9KrYopFXcFxGY6wIKqJ9YwQLDHAMDFAd7GS852z6+gMRxU06L4vOWpCH+Xgv
kNbU8vMuyie0a3SwgC6+gYn/hUyZNzVLUXMtM0AJZm4zpcMoTXXqsHINNc91d82D
RZSHNZs+fHiEqWxAV2GPNGQ+ZnwE+rIYT4yXml6q13AywaUEkJTL45WN86ntt75G
EbCsvzcfgaxz3tlis+UW/yqy6FEaHdVxFICq9FuawAfBjjgeC4rxP54ZD3BHOTM=
=LJ5E
-----END PGP SIGNATURE-----

[PATCH] Add hwmon driver for MAX31875

[Timothy Pearson]

This series adds a new hwmon driver for the MAX31875, which is
used for fan control on Firestone.

This patch is a holdover until a PMBus driver is written at some
point.  Upstream has stated they will not accept unless a PMBus
driver is used for this particular device.

Timothy Pearson (1):
  Initial driver for the MAX31785 intelligent fan controller

 Documentation/hwmon/max31785 |   36 +++
 MAINTAINERS                  |    7 +
 drivers/hwmon/Kconfig        |   10 +
 drivers/hwmon/Makefile       |    1 +
 drivers/hwmon/max31785.c     |  714 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 768 insertions(+)
 create mode 100644 Documentation/hwmon/max31785
 create mode 100644 drivers/hwmon/max31785.c

-- 
1.7.9.5

[PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

Add a basic driver for the MAX31785, focusing on the fan control
features but ignoring the temperature and voltage monitoring
features of the device.

This driver supports all fan control modes and tachometer / PWM
readback where applicable.

Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
---
 Documentation/hwmon/max31785 |   36 +++
 MAINTAINERS                  |    7 +
 drivers/hwmon/Kconfig        |   10 +
 drivers/hwmon/Makefile       |    1 +
 drivers/hwmon/max31785.c     |  714 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 768 insertions(+)
 create mode 100644 Documentation/hwmon/max31785
 create mode 100644 drivers/hwmon/max31785.c

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
new file mode 100644
index 0000000..0911d20
--- /dev/null
+++ b/Documentation/hwmon/max31785
@@ -0,0 +1,36 @@
+Kernel driver max31785
+======================
+
+Supported chips:
+  * Maxim MAX31785
+    Prefix: 'max31785'
+    Addresses scanned: 0x52 0x53 0x54 0x55
+    Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf
+
+Author: Timothy Pearson <tpearson@raptorengineering.com>
+
+
+Description
+-----------
+
+This driver implements support for the Maxim MAX31785 chip.
+
+The MAX31785 controls the speeds of up to six fans using six independent
+PWM outputs. The desired fan speeds (or PWM duty cycles) are written
+through the I2C interface. The outputs drive "4-wire" fans directly,
+or can be used to modulate the fan's power terminals using an external
+pass transistor.
+
+Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%)
+monitoring and control of fan RPM as well as detection of fan failure.
+
+
+Sysfs entries
+-------------
+
+fan[1-6]_input           RO  fan tachometer speed in RPM
+fan[1-6]_fault           RO  fan experienced fault
+fan[1-6]_pulses          RW  tachometer pulses per fan revolution
+fan[1-6]_target          RW  desired fan speed in RPM
+pwm[1-6]_enable          RW  pwm mode, 0=disabled, 1=pwm, 2=rpm, 3=automatic
+pwm[1-6]                 RW  fan target duty cycle (0-255)
diff --git a/MAINTAINERS b/MAINTAINERS
index 6b378a0..f11e425 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7256,6 +7256,13 @@ S:	Maintained
 F:	Documentation/hwmon/max20751
 F:	drivers/hwmon/max20751.c
 
+MAX31785 HARDWARE MONITOR AND FAN CONTROLLER DRIVER
+M:	"Timothy Pearson" <tpearson@raptorengineering.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/max31785
+F:	drivers/hwmon/max31785.c
+
 MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER
 M:	"Hans J. Koch" <hjk@hansjkoch.de>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 3b34ba9..d4de8d5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -874,6 +874,16 @@ config SENSORS_MAX6697
 	  This driver can also be built as a module.  If so, the module
 	  will be called max6697.
 
+config SENSORS_MAX31785
+	tristate "Maxim MAX31785 sensor chip"
+	depends on I2C
+	help
+	  If you say yes here you get support for 6-Channel PWM-Output
+	  Fan RPM Controller.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max31785.
+
 config SENSORS_MAX31790
 	tristate "Maxim MAX31790 sensor chip"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index c0f3201..4455478 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -117,6 +117,7 @@ obj-$(CONFIG_SENSORS_MAX6639)	+= max6639.o
 obj-$(CONFIG_SENSORS_MAX6642)	+= max6642.o
 obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
+obj-$(CONFIG_SENSORS_MAX31785)	+= max31785.o
 obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
 obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
diff --git a/drivers/hwmon/max31785.c b/drivers/hwmon/max31785.c
new file mode 100644
index 0000000..fb7b3f0
--- /dev/null
+++ b/drivers/hwmon/max31785.c
@@ -0,0 +1,714 @@
+/*
+ * max31785.c - Part of lm_sensors, Linux kernel modules for hardware
+ *             monitoring.
+ *
+ * (C) 2016 Raptor Engineering, LLC
+ *
+ * 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.
+ *
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+/* MAX31785 device IDs */
+#define MAX31785_MFR_ID				0x4d
+#define MAX31785_MFR_MODEL			0x53
+
+/* MAX31785 registers */
+#define MAX31785_REG_PAGE			0x00
+#define MAX31785_PAGE_FAN_CONFIG(ch)		(0x00 + (ch))
+#define MAX31785_REG_FAN_CONFIG_1_2		0x3a
+#define MAX31785_REG_FAN_COMMAND_1		0x3b
+#define MAX31785_REG_STATUS_FANS_1_2		0x81
+#define MAX31785_REG_FAN_SPEED_1		0x90
+#define MAX31785_REG_MFR_ID			0x99
+#define MAX31785_REG_MFR_MODEL			0x9a
+#define MAX31785_REG_MFR_FAN_CONFIG		0xf1
+#define MAX31785_REG_READ_FAN_PWM		0xf3
+
+/* Fan Config register bits */
+#define MAX31785_FAN_CFG_PWM_ENABLE		0x80
+#define MAX31785_FAN_CFG_CONTROL_MODE_RPM	0x40
+#define MAX31785_FAN_CFG_PULSE_MASK		0x30
+#define MAX31785_FAN_CFG_PULSE_SHIFT		4
+#define MAX31785_FAN_CFG_PULSE_OFFSET		1
+
+/* Fan Status register bits */
+#define MAX31785_FAN_STATUS_FAULT_MASK		0x80
+
+/* Fan Command constants */
+#define MAX31785_FAN_COMMAND_PWM_RATIO		40
+
+#define NR_CHANNEL				6
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x52, 0x53, 0x54, 0x55,
+							I2C_CLIENT_END };
+
+/*
+ * Client data (each client gets its own)
+ */
+struct max31785_data {
+	struct i2c_client *client;
+	struct mutex device_lock;
+	bool valid; /* zero until following fields are valid */
+	unsigned long last_updated; /* in jiffies */
+
+	/* register values */
+	u8 fan_config[NR_CHANNEL];
+	u16 fan_command[NR_CHANNEL];
+	u8 mfr_fan_config[NR_CHANNEL];
+	u8 fault_status[NR_CHANNEL];
+	u16 tach_rpm[NR_CHANNEL];
+	u16 pwm[NR_CHANNEL];
+};
+
+static int max31785_set_page(struct i2c_client *client,
+				u8 page)
+{
+	return i2c_smbus_write_byte_data(client,
+			MAX31785_REG_PAGE,
+			page);
+}
+
+static int max31785_read_fan_data(struct i2c_client *client,
+				u8 fan, u8 reg, u8 byte_mode)
+{
+	int rv;
+
+	rv = max31785_set_page(client, MAX31785_PAGE_FAN_CONFIG(fan));
+	if (rv < 0)
+		return rv;
+
+	if (byte_mode)
+		rv = i2c_smbus_read_byte_data(client, reg);
+	else
+		rv = i2c_smbus_read_word_data(client, reg);
+
+	return rv;
+}
+
+static int max31785_write_fan_data(struct i2c_client *client,
+				u8 fan, u8 reg, u16 data,
+				u8 byte_mode)
+{
+	int err;
+
+	err = max31785_set_page(client, MAX31785_PAGE_FAN_CONFIG(fan));
+	if (err < 0)
+		return err;
+
+	if (byte_mode)
+		err = i2c_smbus_write_byte_data(client, reg, data);
+	else
+		err = i2c_smbus_write_word_data(client, reg, data);
+
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static bool is_automatic_control_mode(struct max31785_data *data,
+			int index)
+{
+	if (data->fan_command[index] > 0x7fff)
+		return true;
+	else
+		return false;
+}
+
+static struct max31785_data *max31785_update_device(struct device *dev)
+{
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	struct max31785_data *ret = data;
+	int i;
+	int rv;
+
+	mutex_lock(&data->device_lock);
+
+	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
+		for (i = 0; i < NR_CHANNEL; i++) {
+			rv = max31785_read_fan_data(client, i,
+					MAX31785_REG_STATUS_FANS_1_2, 1);
+			if (rv < 0)
+				goto abort;
+			data->fault_status[i] = rv;
+
+			rv = max31785_read_fan_data(client, i,
+					MAX31785_REG_FAN_SPEED_1, 0);
+			if (rv < 0)
+				goto abort;
+			data->tach_rpm[i] = rv;
+
+			if ((data->fan_config[i]
+				& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+				|| is_automatic_control_mode(data, i)) {
+				rv = max31785_read_fan_data(client, i,
+						MAX31785_REG_READ_FAN_PWM, 0);
+				if (rv < 0)
+					goto abort;
+				data->pwm[i] = rv;
+			}
+
+			if (!is_automatic_control_mode(data, i)) {
+				/* Poke watchdog for manual fan control */
+				rv = max31785_write_fan_data(client,
+					i, MAX31785_REG_FAN_COMMAND_1,
+					data->fan_command[i], 0);
+				if (rv < 0)
+					goto abort;
+			}
+		}
+
+		data->last_updated = jiffies;
+		data->valid = true;
+	}
+	goto done;
+
+abort:
+	data->valid = false;
+	ret = ERR_PTR(rv);
+
+done:
+	mutex_unlock(&data->device_lock);
+
+	return ret;
+}
+
+static ssize_t get_fan(struct device *dev,
+		       struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	return sprintf(buf, "%d\n", data->tach_rpm[attr->index]);
+}
+
+static ssize_t get_fan_target(struct device *dev,
+			      struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int rpm;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (data->fan_config[attr->index]
+		& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+		rpm = data->fan_command[attr->index];
+	else
+		rpm = data->fan_command[attr->index]
+					/ MAX31785_FAN_COMMAND_PWM_RATIO;
+
+	return sprintf(buf, "%d\n", rpm);
+}
+
+static ssize_t set_fan_target(struct device *dev,
+			      struct device_attribute *devattr,
+			      const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long rpm;
+	int err;
+
+	err = kstrtoul(buf, 10, &rpm);
+	if (err)
+		return err;
+
+	if (rpm > 0x7fff)
+		return -EINVAL;
+
+	mutex_lock(&data->device_lock);
+
+	/* Write new RPM value */
+	data->fan_command[attr->index] = rpm;
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_COMMAND_1,
+				data->fan_command[attr->index], 0);
+
+	mutex_unlock(&data->device_lock);
+
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t get_fan_pulses(struct device *dev,
+			      struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int pulses;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	pulses = ((data->fan_config[attr->index] & MAX31785_FAN_CFG_PULSE_MASK)
+			>> MAX31785_FAN_CFG_PULSE_SHIFT)
+			+ MAX31785_FAN_CFG_PULSE_OFFSET;
+
+	return sprintf(buf, "%d\n", pulses);
+}
+
+static ssize_t set_fan_pulses(struct device *dev,
+			      struct device_attribute *devattr,
+			      const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long pulses;
+	int err;
+
+	err = kstrtoul(buf, 10, &pulses);
+	if (err)
+		return err;
+
+	if (pulses > 4)
+		return -EINVAL;
+
+	data->fan_config[attr->index] &= MAX31785_FAN_CFG_PULSE_MASK;
+	data->fan_config[attr->index] |=
+				((pulses - MAX31785_FAN_CFG_PULSE_OFFSET)
+				<< MAX31785_FAN_CFG_PULSE_SHIFT);
+
+	mutex_lock(&data->device_lock);
+
+	/* Write new pulse value */
+	data->fan_command[attr->index] = pulses;
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_CONFIG_1_2,
+				data->fan_config[attr->index], 1);
+
+	mutex_unlock(&data->device_lock);
+
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t get_pwm(struct device *dev,
+		       struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int pwm;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if ((data->fan_config[attr->index]
+		& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+		|| is_automatic_control_mode(data, attr->index))
+		pwm = data->pwm[attr->index] / 100;
+	else
+		pwm = data->fan_command[attr->index]
+					/ MAX31785_FAN_COMMAND_PWM_RATIO;
+
+	return sprintf(buf, "%d\n", pwm);
+}
+
+static ssize_t set_pwm(struct device *dev,
+		       struct device_attribute *devattr,
+		       const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long pwm;
+	int err;
+
+	err = kstrtoul(buf, 10, &pwm);
+	if (err)
+		return err;
+
+	if (pwm > 255)
+		return -EINVAL;
+
+	mutex_lock(&data->device_lock);
+
+	/* Write new PWM value */
+	data->fan_command[attr->index] = pwm * MAX31785_FAN_COMMAND_PWM_RATIO;
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_COMMAND_1,
+				data->fan_command[attr->index], 0);
+
+	mutex_unlock(&data->device_lock);
+
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t get_pwm_enable(struct device *dev,
+			      struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int mode;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (!(data->fan_config[attr->index] & MAX31785_FAN_CFG_PWM_ENABLE))
+		mode = 0;
+	else if (is_automatic_control_mode(data, attr->index))
+		mode = 3;
+	else if (data->fan_config[attr->index]
+		& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+		mode = 2;
+	else
+		mode = 1;
+
+	return sprintf(buf, "%d\n", mode);
+}
+
+static ssize_t set_pwm_enable(struct device *dev,
+			      struct device_attribute *devattr,
+			      const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long mode;
+	int err;
+
+	err = kstrtoul(buf, 10, &mode);
+	if (err)
+		return err;
+
+	switch (mode) {
+	case 0:
+		data->fan_config[attr->index] =
+			data->fan_config[attr->index]
+			& ~MAX31785_FAN_CFG_PWM_ENABLE;
+		break;
+	case 1:
+	case 2:
+	case 3:
+		data->fan_config[attr->index] =
+			data->fan_config[attr->index]
+			 | MAX31785_FAN_CFG_PWM_ENABLE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (mode) {
+	case 0:
+		break;
+	case 1:
+		data->fan_config[attr->index] =
+			data->fan_config[attr->index]
+			& ~MAX31785_FAN_CFG_CONTROL_MODE_RPM;
+		break;
+	case 2:
+		data->fan_config[attr->index] =
+			data->fan_config[attr->index]
+			| MAX31785_FAN_CFG_CONTROL_MODE_RPM;
+		break;
+	case 3:
+		data->fan_command[attr->index] = 0xffff;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->device_lock);
+
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_CONFIG_1_2,
+				data->fan_config[attr->index], 1);
+
+	if (err < 0)
+		goto abort;
+
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_COMMAND_1,
+				data->fan_command[attr->index], 0);
+
+abort:
+	mutex_unlock(&data->device_lock);
+
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t get_fan_fault(struct device *dev,
+			     struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int fault;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	fault = !!(data->fault_status[attr->index]
+			& MAX31785_FAN_STATUS_FAULT_MASK);
+
+	return sprintf(buf, "%d\n", fault);
+}
+
+static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, get_fan, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, get_fan, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan3_input, S_IRUGO, get_fan, NULL, 2);
+static SENSOR_DEVICE_ATTR(fan4_input, S_IRUGO, get_fan, NULL, 3);
+static SENSOR_DEVICE_ATTR(fan5_input, S_IRUGO, get_fan, NULL, 4);
+static SENSOR_DEVICE_ATTR(fan6_input, S_IRUGO, get_fan, NULL, 5);
+
+static SENSOR_DEVICE_ATTR(fan1_fault, S_IRUGO, get_fan_fault, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_fault, S_IRUGO, get_fan_fault, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan3_fault, S_IRUGO, get_fan_fault, NULL, 2);
+static SENSOR_DEVICE_ATTR(fan4_fault, S_IRUGO, get_fan_fault, NULL, 3);
+static SENSOR_DEVICE_ATTR(fan5_fault, S_IRUGO, get_fan_fault, NULL, 4);
+static SENSOR_DEVICE_ATTR(fan6_fault, S_IRUGO, get_fan_fault, NULL, 5);
+
+static SENSOR_DEVICE_ATTR(fan1_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 0);
+static SENSOR_DEVICE_ATTR(fan2_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 1);
+static SENSOR_DEVICE_ATTR(fan3_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 2);
+static SENSOR_DEVICE_ATTR(fan4_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 3);
+static SENSOR_DEVICE_ATTR(fan5_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 4);
+static SENSOR_DEVICE_ATTR(fan6_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 5);
+
+static SENSOR_DEVICE_ATTR(fan1_pulses, S_IWUSR | S_IRUGO,
+		get_fan_pulses, set_fan_pulses, 0);
+static SENSOR_DEVICE_ATTR(fan2_pulses, S_IWUSR | S_IRUGO,
+		get_fan_pulses, set_fan_pulses, 1);
+static SENSOR_DEVICE_ATTR(fan3_pulses, S_IWUSR | S_IRUGO,
+		get_fan_pulses, set_fan_pulses, 2);
+static SENSOR_DEVICE_ATTR(fan4_pulses, S_IWUSR | S_IRUGO,
+		get_fan_pulses, set_fan_pulses, 3);
+static SENSOR_DEVICE_ATTR(fan5_pulses, S_IWUSR | S_IRUGO,
+		get_fan_pulses, set_fan_pulses, 4);
+static SENSOR_DEVICE_ATTR(fan6_pulses, S_IWUSR | S_IRUGO,
+		get_fan_pulses, set_fan_pulses, 5);
+
+static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 0);
+static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 1);
+static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 2);
+static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 3);
+static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 4);
+static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 5);
+
+static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 0);
+static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 1);
+static SENSOR_DEVICE_ATTR(pwm3_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 2);
+static SENSOR_DEVICE_ATTR(pwm4_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 3);
+static SENSOR_DEVICE_ATTR(pwm5_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 4);
+static SENSOR_DEVICE_ATTR(pwm6_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 5);
+
+static struct attribute *max31785_attrs[] = {
+	&sensor_dev_attr_fan1_input.dev_attr.attr,
+	&sensor_dev_attr_fan2_input.dev_attr.attr,
+	&sensor_dev_attr_fan3_input.dev_attr.attr,
+	&sensor_dev_attr_fan4_input.dev_attr.attr,
+	&sensor_dev_attr_fan5_input.dev_attr.attr,
+	&sensor_dev_attr_fan6_input.dev_attr.attr,
+
+	&sensor_dev_attr_fan1_fault.dev_attr.attr,
+	&sensor_dev_attr_fan2_fault.dev_attr.attr,
+	&sensor_dev_attr_fan3_fault.dev_attr.attr,
+	&sensor_dev_attr_fan4_fault.dev_attr.attr,
+	&sensor_dev_attr_fan5_fault.dev_attr.attr,
+	&sensor_dev_attr_fan6_fault.dev_attr.attr,
+
+	&sensor_dev_attr_fan1_target.dev_attr.attr,
+	&sensor_dev_attr_fan2_target.dev_attr.attr,
+	&sensor_dev_attr_fan3_target.dev_attr.attr,
+	&sensor_dev_attr_fan4_target.dev_attr.attr,
+	&sensor_dev_attr_fan5_target.dev_attr.attr,
+	&sensor_dev_attr_fan6_target.dev_attr.attr,
+
+	&sensor_dev_attr_fan1_pulses.dev_attr.attr,
+	&sensor_dev_attr_fan2_pulses.dev_attr.attr,
+	&sensor_dev_attr_fan3_pulses.dev_attr.attr,
+	&sensor_dev_attr_fan4_pulses.dev_attr.attr,
+	&sensor_dev_attr_fan5_pulses.dev_attr.attr,
+	&sensor_dev_attr_fan6_pulses.dev_attr.attr,
+
+	&sensor_dev_attr_pwm1.dev_attr.attr,
+	&sensor_dev_attr_pwm2.dev_attr.attr,
+	&sensor_dev_attr_pwm3.dev_attr.attr,
+	&sensor_dev_attr_pwm4.dev_attr.attr,
+	&sensor_dev_attr_pwm5.dev_attr.attr,
+	&sensor_dev_attr_pwm6.dev_attr.attr,
+
+	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm4_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm5_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm6_enable.dev_attr.attr,
+	NULL
+};
+
+static umode_t max31785_attrs_visible(struct kobject *kobj,
+				     struct attribute *a, int n)
+{
+	return a->mode;
+}
+
+static const struct attribute_group max31785_group = {
+	.attrs = max31785_attrs,
+	.is_visible = max31785_attrs_visible,
+};
+__ATTRIBUTE_GROUPS(max31785);
+
+static int max31785_init_client(struct i2c_client *client,
+				struct max31785_data *data)
+{
+	int i, rv;
+
+	for (i = 0; i < NR_CHANNEL; i++) {
+		rv = max31785_read_fan_data(client, i,
+				MAX31785_REG_FAN_CONFIG_1_2, 1);
+		if (rv < 0)
+			return rv;
+		data->fan_config[i] = rv;
+
+		rv = max31785_read_fan_data(client, i,
+				MAX31785_REG_FAN_COMMAND_1, 0);
+		if (rv < 0)
+			return rv;
+		data->fan_command[i] = rv;
+
+		rv = max31785_read_fan_data(client, i,
+				MAX31785_REG_MFR_FAN_CONFIG, 1);
+		if (rv < 0)
+			return rv;
+		data->mfr_fan_config[i] = rv;
+
+		if (!((data->fan_config[i]
+			& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+			|| is_automatic_control_mode(data, i))) {
+			data->pwm[i] = 0;
+		}
+	}
+
+	return rv;
+}
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int max31785_detect(struct i2c_client *client,
+			  struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	int rv;
+
+	if (!i2c_check_functionality(adapter,
+			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	/* Probe manufacturer / model registers */
+	rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_ID);
+	if (rv < 0)
+		return -ENODEV;
+	if (rv != MAX31785_MFR_ID)
+		return -ENODEV;
+
+	rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_MODEL);
+	if (rv < 0)
+		return -ENODEV;
+	if (rv != MAX31785_MFR_MODEL)
+		return -ENODEV;
+
+	strlcpy(info->type, "max31785", I2C_NAME_SIZE);
+
+	return 0;
+}
+
+static int max31785_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	struct device *dev = &client->dev;
+	struct max31785_data *data;
+	struct device *hwmon_dev;
+	int err;
+
+	if (!i2c_check_functionality(adapter,
+			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	data = devm_kzalloc(dev, sizeof(struct max31785_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	mutex_init(&data->device_lock);
+
+	/*
+	 * Initialize the max31785 chip
+	 */
+	err = max31785_init_client(client, data);
+	if (err)
+		return err;
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
+			client->name, data, max31785_groups);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id max31785_id[] = {
+	{ "max31785", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max31785_id);
+
+static struct i2c_driver max31785_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.probe		= max31785_probe,
+	.driver = {
+		.name	= "max31785",
+	},
+	.id_table	= max31785_id,
+	.detect		= max31785_detect,
+	.address_list	= normal_i2c,
+};
+
+module_i2c_driver(max31785_driver);
+
+MODULE_AUTHOR("Timothy Pearson <tpearson@raptorengineering.com>");
+MODULE_DESCRIPTION("MAX31785 sensor driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Joel Stanley]

Hello Timothy,

On Tue, Sep 20, 2016 at 11:22 AM, Timothy Pearson
<tpearson@raptorengineering.com> wrote:
> Add a basic driver for the MAX31785, focusing on the fan control
> features but ignoring the temperature and voltage monitoring
> features of the device.
>
> This driver supports all fan control modes and tachometer / PWM
> readback where applicable.

> This series adds a new hwmon driver for the MAX31875, which is
> used for fan control on Firestone.
>
> This patch is a holdover until a PMBus driver is written at some
> point.  Upstream has stated they will not accept unless a PMBus
> driver is used for this particular device.

I'm happy to carry this in our tree for now.

Do you have a timeline for rewriting and submitting upstream?

>
> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
> ---
>  Documentation/hwmon/max31785 |   36 +++
>  MAINTAINERS                  |    7 +

The change to maintianers should be in a separate patch.

>  drivers/hwmon/Kconfig        |   10 +
>  drivers/hwmon/Makefile       |    1 +
>  drivers/hwmon/max31785.c     |  714 ++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 768 insertions(+)
>  create mode 100644 Documentation/hwmon/max31785
>  create mode 100644 drivers/hwmon/max31785.c
>

> +
> +static const struct i2c_device_id max31785_id[] = {
> +       { "max31785", 0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, max31785_id);

No device tree bindings?

> +
> +static struct i2c_driver max31785_driver = {
> +       .class          = I2C_CLASS_HWMON,
> +       .probe          = max31785_probe,
> +       .driver = {
> +               .name   = "max31785",
> +       },
> +       .id_table       = max31785_id,
> +       .detect         = max31785_detect,
> +       .address_list   = normal_i2c,
> +};
> +
> +module_i2c_driver(max31785_driver);
> +
> +MODULE_AUTHOR("Timothy Pearson <tpearson@raptorengineering.com>");
> +MODULE_DESCRIPTION("MAX31785 sensor driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/23/2016 02:41 AM, Joel Stanley wrote:
> Hello Timothy,
> 
> On Tue, Sep 20, 2016 at 11:22 AM, Timothy Pearson
> <tpearson@raptorengineering.com> wrote:
>> Add a basic driver for the MAX31785, focusing on the fan control
>> features but ignoring the temperature and voltage monitoring
>> features of the device.
>>
>> This driver supports all fan control modes and tachometer / PWM
>> readback where applicable.
> 
>> This series adds a new hwmon driver for the MAX31875, which is
>> used for fan control on Firestone.
>>
>> This patch is a holdover until a PMBus driver is written at some
>> point.  Upstream has stated they will not accept unless a PMBus
>> driver is used for this particular device.
> 
> I'm happy to carry this in our tree for now.
> 
> Do you have a timeline for rewriting and submitting upstream?

At this time upstream needs some significant work; the PMBus core driver
as it currently exists does not support the required functions.
However, the hwmon maintainer (Gunter Roeck) has said he will work on
extending the PMBus driver and might even get something working for this
chip with that core driver, so there is definitely progress being made.
 I wouldn't expect anything upstream until at least kernel 4.9 or later
though.

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJX5USGAAoJEK+E3vEXDOFbuZsIALMWMVl0LK/zGDpD3qVL5tib
M0t9QYXhMfYSX1CbP47lRTTDg779f6YadV4QsVslqVt3pVCVm+cA0q2SuyDGCVJ5
KCfVlocQ8KYjZXUieY/Ry3YLvU3oraCIAy5VLpeuPAZMU8RwA8gctElcmeWQNneV
nywY1wJPe1YjBrIZefVvkF5gJQ/Mzoo498Aeyo8Mi98EB+XSStCp5w9XKF7I7Hja
BL5FZbO13ghvfSoDUPv51uKS4r843Z7SL7yzPH1W4OH+beB8nKXyj5ajo2C21Z2V
ciyuWQQdQMw/rh5I7ejNvAczEUsYV7h8zXc4dMxoZQYmgcagP1P7uahFL/0skZo=
=+1Oo
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Matt Spinler]

Hi,

We're getting closer to needing the hwmon driver for this.  Has anyone 
heard an outlook?

On 9/23/2016 10:04 AM, Timothy Pearson wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 09/23/2016 02:41 AM, Joel Stanley wrote:
>> Hello Timothy,
>>
>> On Tue, Sep 20, 2016 at 11:22 AM, Timothy Pearson
>> <tpearson@raptorengineering.com> wrote:
>>> Add a basic driver for the MAX31785, focusing on the fan control
>>> features but ignoring the temperature and voltage monitoring
>>> features of the device.
>>>
>>> This driver supports all fan control modes and tachometer / PWM
>>> readback where applicable.
>>> This series adds a new hwmon driver for the MAX31875, which is
>>> used for fan control on Firestone.
>>>
>>> This patch is a holdover until a PMBus driver is written at some
>>> point.  Upstream has stated they will not accept unless a PMBus
>>> driver is used for this particular device.
>> I'm happy to carry this in our tree for now.
>>
>> Do you have a timeline for rewriting and submitting upstream?
> At this time upstream needs some significant work; the PMBus core driver
> as it currently exists does not support the required functions.
> However, the hwmon maintainer (Gunter Roeck) has said he will work on
> extending the PMBus driver and might even get something working for this
> chip with that core driver, so there is definitely progress being made.
>   I wouldn't expect anything upstream until at least kernel 4.9 or later
> though.
>
> - -- 
> Timothy Pearson
> Raptor Engineering
> +1 (415) 727-8645 (direct line)
> +1 (512) 690-0200 (switchboard)
> https://www.raptorengineering.com
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQEcBAEBAgAGBQJX5USGAAoJEK+E3vEXDOFbuZsIALMWMVl0LK/zGDpD3qVL5tib
> M0t9QYXhMfYSX1CbP47lRTTDg779f6YadV4QsVslqVt3pVCVm+cA0q2SuyDGCVJ5
> KCfVlocQ8KYjZXUieY/Ry3YLvU3oraCIAy5VLpeuPAZMU8RwA8gctElcmeWQNneV
> nywY1wJPe1YjBrIZefVvkF5gJQ/Mzoo498Aeyo8Mi98EB+XSStCp5w9XKF7I7Hja
> BL5FZbO13ghvfSoDUPv51uKS4r843Z7SL7yzPH1W4OH+beB8nKXyj5ajo2C21Z2V
> ciyuWQQdQMw/rh5I7ejNvAczEUsYV7h8zXc4dMxoZQYmgcagP1P7uahFL/0skZo=
> =+1Oo
> -----END PGP SIGNATURE-----
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Joel Stanley]

Hi Matt,

On Sat, Dec 10, 2016 at 2:55 AM, Matt Spinler
<mspinler@linux.vnet.ibm.com> wrote:
> Hi,
>
> We're getting closer to needing the hwmon driver for this.  Has anyone heard
> an outlook?

It's convention on mailing lists to bottom post. Particularly so when
others in the thread are using this style.

I have not heard any news about this driver.

Have you?

Cheers,

Joel

>
>
> On 9/23/2016 10:04 AM, Timothy Pearson wrote:
>>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 09/23/2016 02:41 AM, Joel Stanley wrote:
>>>
>>> Hello Timothy,
>>>
>>> On Tue, Sep 20, 2016 at 11:22 AM, Timothy Pearson
>>> <tpearson@raptorengineering.com> wrote:
>>>>
>>>> Add a basic driver for the MAX31785, focusing on the fan control
>>>> features but ignoring the temperature and voltage monitoring
>>>> features of the device.
>>>>
>>>> This driver supports all fan control modes and tachometer / PWM
>>>> readback where applicable.
>>>> This series adds a new hwmon driver for the MAX31875, which is
>>>> used for fan control on Firestone.
>>>>
>>>> This patch is a holdover until a PMBus driver is written at some
>>>> point.  Upstream has stated they will not accept unless a PMBus
>>>> driver is used for this particular device.
>>>
>>> I'm happy to carry this in our tree for now.
>>>
>>> Do you have a timeline for rewriting and submitting upstream?
>>
>> At this time upstream needs some significant work; the PMBus core driver
>> as it currently exists does not support the required functions.
>> However, the hwmon maintainer (Gunter Roeck) has said he will work on
>> extending the PMBus driver and might even get something working for this
>> chip with that core driver, so there is definitely progress being made.
>>   I wouldn't expect anything upstream until at least kernel 4.9 or later
>> though.
>>
>> - -- Timothy Pearson
>> Raptor Engineering
>> +1 (415) 727-8645 (direct line)
>> +1 (512) 690-0200 (switchboard)
>> https://www.raptorengineering.com
>> -----BEGIN PGP SIGNATURE-----
>> Version: GnuPG v1.4.11 (GNU/Linux)
>> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>>
>> iQEcBAEBAgAGBQJX5USGAAoJEK+E3vEXDOFbuZsIALMWMVl0LK/zGDpD3qVL5tib
>> M0t9QYXhMfYSX1CbP47lRTTDg779f6YadV4QsVslqVt3pVCVm+cA0q2SuyDGCVJ5
>> KCfVlocQ8KYjZXUieY/Ry3YLvU3oraCIAy5VLpeuPAZMU8RwA8gctElcmeWQNneV
>> nywY1wJPe1YjBrIZefVvkF5gJQ/Mzoo498Aeyo8Mi98EB+XSStCp5w9XKF7I7Hja
>> BL5FZbO13ghvfSoDUPv51uKS4r843Z7SL7yzPH1W4OH+beB8nKXyj5ajo2C21Z2V
>> ciyuWQQdQMw/rh5I7ejNvAczEUsYV7h8zXc4dMxoZQYmgcagP1P7uahFL/0skZo=
>> =+1Oo
>> -----END PGP SIGNATURE-----
>> _______________________________________________
>> openbmc mailing list
>> openbmc@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/openbmc
>
>

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/11/2016 05:36 PM, Joel Stanley wrote:
> Hi Matt,
> 
> On Sat, Dec 10, 2016 at 2:55 AM, Matt Spinler
> <mspinler@linux.vnet.ibm.com> wrote:
>> Hi,
>>
>> We're getting closer to needing the hwmon driver for this.  Has anyone heard
>> an outlook?

There is a functional driver merged into the OpenBMC kernel tree.
Upstream is still working on a replacement driver as far as I know; they
just wanted a different internal implementation of the driver versus the
one in the OpenBMC kernel tree.

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJYTtE7AAoJEK+E3vEXDOFbI/IIAJ7ZnMhqd3EyhBLrFXSBA18c
2z5tAH8iLVYns0/CL9p5vyqs6k5emc5EBAxwfw3NxE+nlXHB2khcZBJgWWA9+ZdR
pbV8ChnmPgyphHg8P2t3UEvEkqwL1hOQHEx7V8pdvC/9JRbWBDgaIsdLJViZL1gQ
AibLhFb0QflLLVKR3txvZadF66w6SyOCN6parT0u/6c/VanxranuQVFOqIn3MVSn
DOpf7fX0MLe3ripJCXe2nZ+OdvjFxlNHfs2rUKl7JVDJzJU36i2PD9xMvkGzadBd
+USdbrD9j6D5QuSHXOyIc6ij2kMVCjNuzW083l0BxUeXZkyolBh3uYu7/S7qf9E=
=k1Ld
-----END PGP SIGNATURE-----

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Matt Spinler]

Hi Tim,


On 12/12/2016 10:32 AM, Timothy Pearson wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 12/11/2016 05:36 PM, Joel Stanley wrote:
>> Hi Matt,
>>
>> On Sat, Dec 10, 2016 at 2:55 AM, Matt Spinler
>> <mspinler@linux.vnet.ibm.com> wrote:
>>> Hi,
>>>
>>> We're getting closer to needing the hwmon driver for this.  Has anyone heard
>>> an outlook?
> There is a functional driver merged into the OpenBMC kernel tree.
> Upstream is still working on a replacement driver as far as I know; they
> just wanted a different internal implementation of the driver versus the
> one in the OpenBMC kernel tree.

Your driver is a hwmon driver, so it will make an entry in 
/sys/class/hwmon?  I realized after
I sent this email that the chip wasn't specified in the device tree, so 
it wasn't going to show
up anywhere even if it was.


> - -- 
> Timothy Pearson
> Raptor Engineering
> +1 (415) 727-8645 (direct line)
> +1 (512) 690-0200 (switchboard)
> https://www.raptorengineering.com
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQEcBAEBAgAGBQJYTtE7AAoJEK+E3vEXDOFbI/IIAJ7ZnMhqd3EyhBLrFXSBA18c
> 2z5tAH8iLVYns0/CL9p5vyqs6k5emc5EBAxwfw3NxE+nlXHB2khcZBJgWWA9+ZdR
> pbV8ChnmPgyphHg8P2t3UEvEkqwL1hOQHEx7V8pdvC/9JRbWBDgaIsdLJViZL1gQ
> AibLhFb0QflLLVKR3txvZadF66w6SyOCN6parT0u/6c/VanxranuQVFOqIn3MVSn
> DOpf7fX0MLe3ripJCXe2nZ+OdvjFxlNHfs2rUKl7JVDJzJU36i2PD9xMvkGzadBd
> +USdbrD9j6D5QuSHXOyIc6ij2kMVCjNuzW083l0BxUeXZkyolBh3uYu7/S7qf9E=
> =k1Ld
> -----END PGP SIGNATURE-----
>

Re: [PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/12/2016 12:17 PM, Matt Spinler wrote:
> Hi Tim,
> 
> 
> On 12/12/2016 10:32 AM, Timothy Pearson wrote:
> On 12/11/2016 05:36 PM, Joel Stanley wrote:
>>>> Hi Matt,
>>>>
>>>> On Sat, Dec 10, 2016 at 2:55 AM, Matt Spinler
>>>> <mspinler@linux.vnet.ibm.com> wrote:
>>>>> Hi,
>>>>>
>>>>> We're getting closer to needing the hwmon driver for this.  Has
>>>>> anyone heard
>>>>> an outlook?
> There is a functional driver merged into the OpenBMC kernel tree.
> Upstream is still working on a replacement driver as far as I know; they
> just wanted a different internal implementation of the driver versus the
> one in the OpenBMC kernel tree.
> 
>> Your driver is a hwmon driver, so it will make an entry in
>> /sys/class/hwmon?  I realized after
>> I sent this email that the chip wasn't specified in the device tree, so
>> it wasn't going to show
>> up anywhere even if it was.

This is correct.  You can read fan speeds and set PWM values, as well as
modifying the automatic fan control modes.

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJYTwD1AAoJEK+E3vEXDOFbcMcH/3BBXUpg4He2kS2T2TPZfgWr
jbWo+2fvdfJ5RZGS/iffrxv26dVw1s6QHocnlnW3xbUhgqQpz7ruXD4YfXtizIgY
+Fa7zWmptQTNqLbM2brOxKRYXBS1qAgRS9Xmciwk7aWsFMtms5cjLZ5JjLWy01cS
tPiRcYX+SQ6RcTgNYZi0QlYIB4gUIwJai4WRs3m0V2sFv4iXJB9CNKuv9Tw7miJY
wNTNQrlV1R0mj05cR8IemgT9S9iONavAZzN71uGSyXHauigMq88BaOVJB/unRtoT
PAI+5+bqGEH8UhfVQnk+epydPkzxTOYkKG5XLkc8VchfCEawontZ3p5dYy97Das=
=doBJ
-----END PGP SIGNATURE-----

[PATCH] Initial driver for the MAX31785 intelligent fan controller

[Timothy Pearson]

Add a basic driver for the MAX31785, focusing on the fan control
features but ignoring the temperature and voltage monitoring
features of the device.

This driver supports all fan control modes and tachometer / PWM
readback where applicable.

Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
---
 Documentation/hwmon/max31785 |   36 +++
 drivers/hwmon/Kconfig        |   10 +
 drivers/hwmon/Makefile       |    1 +
 drivers/hwmon/max31785.c     |  713 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 760 insertions(+)
 create mode 100644 Documentation/hwmon/max31785
 create mode 100644 drivers/hwmon/max31785.c

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
new file mode 100644
index 0000000..34dca74
--- /dev/null
+++ b/Documentation/hwmon/max31785
@@ -0,0 +1,36 @@
+Kernel driver max31785
+======================
+
+Supported chips:
+  * Maxim MAX31785
+    Prefix: 'max31785'
+    Addresses scanned: 0x52 0x53 0x54 0x55
+    Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf
+
+Author: Timothy Pearson <tpearson@raptorengineering.com>
+
+
+Description
+-----------
+
+This driver implements support for the Maxim MAX31785 chip.
+
+The MAX31785 controls the speeds of up to six fans using six independent
+PWM outputs. The desired fan speeds (or PWM duty cycles) are written
+through the I2C interface. The outputs drive "4-wire" fans directly,
+or can be used to modulate the fan's power terminals using an external
+pass transistor.
+
+Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%)
+monitoring and control of fan RPM as well as detection of fan failure.
+
+
+Sysfs entries
+-------------
+
+fan[1-6]_input           RO  fan tachometer speed in RPM
+fan[1-6]_fault           RO  fan experienced fault
+fan[1-6]_target          RW  desired fan speed in RPM
+fan[1-6]_control_mode    RW  desired control mode: rpm, pwm, or auto
+pwm[1-6]_enable          RW  output enabled, 0=disabled, 1=enabled
+pwm[1-6]                 RW  fan target duty cycle (0-255)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index eaf2f91..a202fd5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -886,6 +886,16 @@ config SENSORS_MAX6697
 	  This driver can also be built as a module.  If so, the module
 	  will be called max6697.
 
+config SENSORS_MAX31785
+	tristate "Maxim MAX31785 sensor chip"
+	depends on I2C
+	help
+	  If you say yes here you get support for 6-Channel PWM-Output
+	  Fan RPM Controller.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max31785.
+
 config SENSORS_MAX31790
 	tristate "Maxim MAX31790 sensor chip"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index fe87d28..44c0c02 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_SENSORS_MAX6639)	+= max6639.o
 obj-$(CONFIG_SENSORS_MAX6642)	+= max6642.o
 obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
+obj-$(CONFIG_SENSORS_MAX31785)	+= max31785.o
 obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
 obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
diff --git a/drivers/hwmon/max31785.c b/drivers/hwmon/max31785.c
new file mode 100644
index 0000000..9a23edd
--- /dev/null
+++ b/drivers/hwmon/max31785.c
@@ -0,0 +1,713 @@
+/*
+ * max31785.c - Part of lm_sensors, Linux kernel modules for hardware
+ *             monitoring.
+ *
+ * (C) 2016 Raptor Engineering, LLC
+ *
+ * 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.
+ *
+ * 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.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+/* MAX31785 registers */
+#define MAX31785_REG_PAGE			0x00
+#define MAX31785_PAGE_FAN_CONFIG(ch)		(0x00 + (ch))
+#define MAX31785_REG_FAN_CONFIG_1_2		0x3a
+#define MAX31785_REG_FAN_COMMAND_1		0x3b
+#define MAX31785_REG_STATUS_FANS_1_2		0x81
+#define MAX31785_REG_FAN_SPEED_1		0x90
+#define MAX31785_REG_MFR_ID			0x99
+#define MAX31785_REG_MFR_MODEL			0x9a
+#define MAX31785_REG_MFR_FAN_CONFIG		0xf1
+#define MAX31785_REG_READ_FAN_PWM		0xf3
+
+/* Fan Config register bits */
+#define MAX31785_FAN_CFG_PWM_ENABLE		0x80
+#define MAX31785_FAN_CFG_CONTROL_MODE_RPM	0x40
+
+/* Fan Status register bits */
+#define MAX31785_FAN_STATUS_FAULT_MASK		0x80
+
+#define NR_CHANNEL				6
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x52, 0x53, 0x54, 0x55,
+							I2C_CLIENT_END };
+
+/*
+ * Client data (each client gets its own)
+ */
+struct max31785_data {
+	struct i2c_client *client;
+	struct mutex device_lock;
+	bool valid; /* zero until following fields are valid */
+	unsigned long last_updated; /* in jiffies */
+
+	/* register values */
+	uint8_t fan_config[NR_CHANNEL];
+	uint16_t fan_command[NR_CHANNEL];
+	uint8_t mfr_fan_config[NR_CHANNEL];
+	uint8_t fault_status[NR_CHANNEL];
+	uint16_t tach_rpm[NR_CHANNEL];
+	uint16_t pwm[NR_CHANNEL];
+};
+
+static int max31785_set_page(struct i2c_client *client,
+				uint8_t page)
+{
+	return i2c_smbus_write_byte_data(client,
+			MAX31785_REG_PAGE,
+			page);
+}
+
+static int max31785_read_fan_data(struct i2c_client *client,
+				uint8_t fan, uint8_t reg, uint8_t byte_mode)
+{
+	int rv;
+
+	rv = max31785_set_page(client, MAX31785_PAGE_FAN_CONFIG(fan));
+	if (rv < 0)
+		return rv;
+
+	if (byte_mode)
+		rv = i2c_smbus_read_byte_data(client, reg);
+	else
+		rv = i2c_smbus_read_word_data(client, reg);
+
+	return rv;
+}
+
+static int max31785_write_fan_data(struct i2c_client *client,
+				uint8_t fan, uint8_t reg, uint16_t data,
+				uint8_t byte_mode)
+{
+	int err;
+
+	err = max31785_set_page(client, MAX31785_PAGE_FAN_CONFIG(fan));
+	if (err < 0)
+		return err;
+
+	if (byte_mode)
+		err = i2c_smbus_write_byte_data(client, reg, data);
+	else
+		err = i2c_smbus_write_word_data(client, reg, data);
+
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static uint8_t is_automatic_control_mode(struct max31785_data *data,
+			int index)
+{
+	if (data->fan_command[index] > 0x7fff)
+		return 1;
+	else
+		return 0;
+}
+
+static struct max31785_data *max31785_update_device(struct device *dev)
+{
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	struct max31785_data *ret = data;
+	int i;
+	int rv;
+
+	mutex_lock(&data->device_lock);
+
+	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
+		for (i = 0; i < NR_CHANNEL; i++) {
+			rv = max31785_read_fan_data(client, i,
+					MAX31785_REG_STATUS_FANS_1_2, 1);
+			if (rv < 0)
+				goto abort;
+			data->fault_status[i] = rv;
+
+			rv = max31785_read_fan_data(client, i,
+					MAX31785_REG_FAN_SPEED_1, 0);
+			if (rv < 0)
+				goto abort;
+			data->tach_rpm[i] = rv;
+
+			if ((data->fan_config[i]
+				& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+				|| is_automatic_control_mode(data, i)) {
+				rv = max31785_read_fan_data(client, i,
+						MAX31785_REG_READ_FAN_PWM, 0);
+				if (rv < 0)
+					goto abort;
+				data->pwm[i] = rv;
+			}
+
+			if (!is_automatic_control_mode(data, i)) {
+				/* Poke watchdog for manual fan control */
+				rv = max31785_write_fan_data(client,
+					i, MAX31785_REG_FAN_COMMAND_1,
+					data->fan_command[i], 0);
+				if (rv < 0)
+					goto abort;
+			}
+		}
+
+		data->last_updated = jiffies;
+		data->valid = true;
+	}
+	goto done;
+
+abort:
+	data->valid = false;
+	ret = ERR_PTR(rv);
+
+done:
+	mutex_unlock(&data->device_lock);
+
+	return ret;
+}
+
+static ssize_t get_fan(struct device *dev,
+		       struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	return sprintf(buf, "%d\n", data->tach_rpm[attr->index]);
+}
+
+static ssize_t get_fan_target(struct device *dev,
+			      struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int rpm;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (data->fan_config[attr->index]
+		& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+		rpm = data->fan_command[attr->index];
+	else
+		rpm = data->fan_command[attr->index] / 40;
+
+	return sprintf(buf, "%d\n", rpm);
+}
+
+static ssize_t set_fan_target(struct device *dev,
+			      struct device_attribute *devattr,
+			      const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long rpm;
+	int err;
+
+	err = kstrtoul(buf, 10, &rpm);
+	if (err)
+		return err;
+
+	if (rpm > 0x7fff)
+		return -EINVAL;
+
+	mutex_lock(&data->device_lock);
+
+	/* Switch fan to RPM mode */
+	data->fan_config[attr->index] |= MAX31785_FAN_CFG_CONTROL_MODE_RPM;
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_CONFIG_1_2,
+				data->fan_config[attr->index], 1);
+
+	if (err < 0) {
+		mutex_unlock(&data->device_lock);
+		return err;
+	}
+
+	/* Write new RPM value */
+	data->fan_command[attr->index] = rpm;
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_COMMAND_1,
+				data->fan_command[attr->index], 0);
+
+	mutex_unlock(&data->device_lock);
+
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t get_pwm(struct device *dev,
+		       struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int pwm;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if ((data->fan_config[attr->index]
+		& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+		|| is_automatic_control_mode(data, attr->index))
+		pwm = data->pwm[attr->index] / 100;
+	else
+		pwm = data->fan_command[attr->index] / 40;
+
+	return sprintf(buf, "%d\n", pwm);
+}
+
+static ssize_t set_pwm(struct device *dev,
+		       struct device_attribute *devattr,
+		       const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long pwm;
+	int err;
+
+	err = kstrtoul(buf, 10, &pwm);
+	if (err)
+		return err;
+
+	if (pwm > 255)
+		return -EINVAL;
+
+	mutex_lock(&data->device_lock);
+
+	/* Switch fan to PWM mode */
+	data->fan_config[attr->index] &= ~MAX31785_FAN_CFG_CONTROL_MODE_RPM;
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_CONFIG_1_2,
+				data->fan_config[attr->index], 1);
+
+	if (err < 0) {
+		mutex_unlock(&data->device_lock);
+		return err;
+	}
+
+	/* Write new PWM value */
+	data->fan_command[attr->index] = pwm * 40;
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_COMMAND_1,
+				data->fan_command[attr->index], 0);
+
+	mutex_unlock(&data->device_lock);
+
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t get_pwm_enable(struct device *dev,
+			      struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int enabled;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (data->fan_config[attr->index] & MAX31785_FAN_CFG_PWM_ENABLE)
+		enabled = 1;
+	else
+		enabled = 0;
+
+	return sprintf(buf, "%d\n", enabled);
+}
+
+static ssize_t set_pwm_enable(struct device *dev,
+			      struct device_attribute *devattr,
+			      const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long enabled;
+	int err;
+
+	err = kstrtoul(buf, 10, &enabled);
+	if (err)
+		return err;
+
+	switch (enabled) {
+	case 0:
+		data->fan_config[attr->index] =
+			data->fan_config[attr->index]
+			& ~MAX31785_FAN_CFG_PWM_ENABLE;
+		break;
+	case 1:
+		data->fan_config[attr->index] =
+			data->fan_config[attr->index]
+			 | MAX31785_FAN_CFG_PWM_ENABLE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->device_lock);
+
+	err = max31785_write_fan_data(client, attr->index,
+				MAX31785_REG_FAN_CONFIG_1_2,
+				data->fan_config[attr->index], 1);
+
+	mutex_unlock(&data->device_lock);
+
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t get_control_mode(struct device *dev,
+			      struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (is_automatic_control_mode(data, attr->index))
+		return sprintf(buf, "auto\n");
+	else if (data->fan_config[attr->index]
+				& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+		return sprintf(buf, "rpm\n");
+
+	return sprintf(buf, "pwm\n");
+}
+
+static ssize_t set_control_mode(struct device *dev,
+			      struct device_attribute *devattr,
+			      const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int err;
+
+	if (!strcmp(buf, "auto") || !strcmp(buf, "auto\n")) {
+		data->fan_command[attr->index] = 0xffff;
+	} else if (!strcmp(buf, "pwm") || !strcmp(buf, "pwm\n")) {
+		data->fan_config[attr->index] =
+			data->fan_config[attr->index]
+			& ~MAX31785_FAN_CFG_CONTROL_MODE_RPM;
+	} else if (!strcmp(buf, "rpm") || !strcmp(buf, "rpm\n")) {
+		data->fan_config[attr->index] =
+			data->fan_config[attr->index]
+			| MAX31785_FAN_CFG_CONTROL_MODE_RPM;
+	} else {
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->device_lock);
+
+	if (data->fan_command[attr->index] == 0xffff)
+		err = max31785_write_fan_data(client, attr->index,
+					MAX31785_REG_FAN_COMMAND_1,
+					data->fan_command[attr->index], 0);
+	else
+		err = max31785_write_fan_data(client, attr->index,
+					MAX31785_REG_FAN_CONFIG_1_2,
+					data->fan_config[attr->index], 1);
+
+	mutex_unlock(&data->device_lock);
+
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t get_fan_fault(struct device *dev,
+			     struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct max31785_data *data = max31785_update_device(dev);
+	int fault;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	fault = !!(data->fault_status[attr->index]
+			& MAX31785_FAN_STATUS_FAULT_MASK);
+
+	return sprintf(buf, "%d\n", fault);
+}
+
+static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, get_fan, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, get_fan, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan3_input, S_IRUGO, get_fan, NULL, 2);
+static SENSOR_DEVICE_ATTR(fan4_input, S_IRUGO, get_fan, NULL, 3);
+static SENSOR_DEVICE_ATTR(fan5_input, S_IRUGO, get_fan, NULL, 4);
+static SENSOR_DEVICE_ATTR(fan6_input, S_IRUGO, get_fan, NULL, 5);
+
+static SENSOR_DEVICE_ATTR(fan1_fault, S_IRUGO, get_fan_fault, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_fault, S_IRUGO, get_fan_fault, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan3_fault, S_IRUGO, get_fan_fault, NULL, 2);
+static SENSOR_DEVICE_ATTR(fan4_fault, S_IRUGO, get_fan_fault, NULL, 3);
+static SENSOR_DEVICE_ATTR(fan5_fault, S_IRUGO, get_fan_fault, NULL, 4);
+static SENSOR_DEVICE_ATTR(fan6_fault, S_IRUGO, get_fan_fault, NULL, 5);
+
+static SENSOR_DEVICE_ATTR(fan1_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 0);
+static SENSOR_DEVICE_ATTR(fan2_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 1);
+static SENSOR_DEVICE_ATTR(fan3_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 2);
+static SENSOR_DEVICE_ATTR(fan4_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 3);
+static SENSOR_DEVICE_ATTR(fan5_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 4);
+static SENSOR_DEVICE_ATTR(fan6_target, S_IWUSR | S_IRUGO,
+		get_fan_target, set_fan_target, 5);
+
+static SENSOR_DEVICE_ATTR(fan1_control_mode, S_IWUSR | S_IRUGO,
+		get_control_mode, set_control_mode, 0);
+static SENSOR_DEVICE_ATTR(fan2_control_mode, S_IWUSR | S_IRUGO,
+		get_control_mode, set_control_mode, 1);
+static SENSOR_DEVICE_ATTR(fan3_control_mode, S_IWUSR | S_IRUGO,
+		get_control_mode, set_control_mode, 2);
+static SENSOR_DEVICE_ATTR(fan4_control_mode, S_IWUSR | S_IRUGO,
+		get_control_mode, set_control_mode, 3);
+static SENSOR_DEVICE_ATTR(fan5_control_mode, S_IWUSR | S_IRUGO,
+		get_control_mode, set_control_mode, 4);
+static SENSOR_DEVICE_ATTR(fan6_control_mode, S_IWUSR | S_IRUGO,
+		get_control_mode, set_control_mode, 5);
+
+static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 0);
+static SENSOR_DEVICE_ATTR(pwm2, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 1);
+static SENSOR_DEVICE_ATTR(pwm3, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 2);
+static SENSOR_DEVICE_ATTR(pwm4, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 3);
+static SENSOR_DEVICE_ATTR(pwm5, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 4);
+static SENSOR_DEVICE_ATTR(pwm6, S_IWUSR | S_IRUGO, get_pwm, set_pwm, 5);
+
+static SENSOR_DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 0);
+static SENSOR_DEVICE_ATTR(pwm2_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 1);
+static SENSOR_DEVICE_ATTR(pwm3_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 2);
+static SENSOR_DEVICE_ATTR(pwm4_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 3);
+static SENSOR_DEVICE_ATTR(pwm5_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 4);
+static SENSOR_DEVICE_ATTR(pwm6_enable, S_IWUSR | S_IRUGO,
+		get_pwm_enable, set_pwm_enable, 5);
+
+static struct attribute *max31785_attrs[] = {
+	&sensor_dev_attr_fan1_input.dev_attr.attr,
+	&sensor_dev_attr_fan2_input.dev_attr.attr,
+	&sensor_dev_attr_fan3_input.dev_attr.attr,
+	&sensor_dev_attr_fan4_input.dev_attr.attr,
+	&sensor_dev_attr_fan5_input.dev_attr.attr,
+	&sensor_dev_attr_fan6_input.dev_attr.attr,
+
+	&sensor_dev_attr_fan1_fault.dev_attr.attr,
+	&sensor_dev_attr_fan2_fault.dev_attr.attr,
+	&sensor_dev_attr_fan3_fault.dev_attr.attr,
+	&sensor_dev_attr_fan4_fault.dev_attr.attr,
+	&sensor_dev_attr_fan5_fault.dev_attr.attr,
+	&sensor_dev_attr_fan6_fault.dev_attr.attr,
+
+	&sensor_dev_attr_fan1_target.dev_attr.attr,
+	&sensor_dev_attr_fan2_target.dev_attr.attr,
+	&sensor_dev_attr_fan3_target.dev_attr.attr,
+	&sensor_dev_attr_fan4_target.dev_attr.attr,
+	&sensor_dev_attr_fan5_target.dev_attr.attr,
+	&sensor_dev_attr_fan6_target.dev_attr.attr,
+
+	&sensor_dev_attr_fan1_control_mode.dev_attr.attr,
+	&sensor_dev_attr_fan2_control_mode.dev_attr.attr,
+	&sensor_dev_attr_fan3_control_mode.dev_attr.attr,
+	&sensor_dev_attr_fan4_control_mode.dev_attr.attr,
+	&sensor_dev_attr_fan5_control_mode.dev_attr.attr,
+	&sensor_dev_attr_fan6_control_mode.dev_attr.attr,
+
+	&sensor_dev_attr_pwm1.dev_attr.attr,
+	&sensor_dev_attr_pwm2.dev_attr.attr,
+	&sensor_dev_attr_pwm3.dev_attr.attr,
+	&sensor_dev_attr_pwm4.dev_attr.attr,
+	&sensor_dev_attr_pwm5.dev_attr.attr,
+	&sensor_dev_attr_pwm6.dev_attr.attr,
+
+	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm2_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm4_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm5_enable.dev_attr.attr,
+	&sensor_dev_attr_pwm6_enable.dev_attr.attr,
+	NULL
+};
+
+static umode_t max31785_attrs_visible(struct kobject *kobj,
+				     struct attribute *a, int n)
+{
+	return a->mode;
+}
+
+static const struct attribute_group max31785_group = {
+	.attrs = max31785_attrs,
+	.is_visible = max31785_attrs_visible,
+};
+__ATTRIBUTE_GROUPS(max31785);
+
+static int max31785_init_client(struct i2c_client *client,
+				struct max31785_data *data)
+{
+	int i, rv;
+
+	dev_info(&client->dev, "Reading device state\n");
+
+	mutex_lock(&data->device_lock);
+
+	for (i = 0; i < NR_CHANNEL; i++) {
+		rv = max31785_read_fan_data(client, i,
+				MAX31785_REG_FAN_CONFIG_1_2, 1);
+		if (rv < 0)
+			goto abort;
+		data->fan_config[i] = rv;
+
+		rv = max31785_read_fan_data(client, i,
+				MAX31785_REG_FAN_COMMAND_1, 0);
+		if (rv < 0)
+			goto abort;
+		data->fan_command[i] = rv;
+
+		rv = max31785_read_fan_data(client, i,
+				MAX31785_REG_MFR_FAN_CONFIG, 1);
+		if (rv < 0)
+			goto abort;
+		data->mfr_fan_config[i] = rv;
+
+		if (!((data->fan_config[i]
+			& MAX31785_FAN_CFG_CONTROL_MODE_RPM)
+			|| is_automatic_control_mode(data, i))) {
+			data->pwm[i] = 0;
+		}
+	}
+
+	dev_info(&client->dev, "Driver initialized\n");
+
+abort:
+	mutex_unlock(&data->device_lock);
+	return rv;
+}
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int max31785_detect(struct i2c_client *client,
+			  struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	int rv, manufacturer, model;
+
+	if (!i2c_check_functionality(adapter,
+			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	 dev_info(&adapter->dev, "Probing for MAX31785\n");
+
+	/* Probe manufacturer / model registers */
+	rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_ID);
+	if (rv < 0)
+		return -ENODEV;
+	manufacturer = rv;
+
+	rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_MODEL);
+	if (rv < 0)
+		return -ENODEV;
+	model = rv;
+
+	if (manufacturer != 0x4d)
+		return -ENODEV;
+	if (model != 0x53)
+		return -ENODEV;
+
+	dev_info(&adapter->dev,
+		"Detected MAX31785 at %d,0x%02x with"
+		" MANUFACTURER: 0x%02x MODEL: %02x\n",
+		i2c_adapter_id(client->adapter), client->addr,
+		manufacturer, model);
+
+	strlcpy(info->type, "max31785", I2C_NAME_SIZE);
+
+	return 0;
+}
+
+static int max31785_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	struct device *dev = &client->dev;
+	struct max31785_data *data;
+	struct device *hwmon_dev;
+	int err;
+
+	if (!i2c_check_functionality(adapter,
+			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	data = devm_kzalloc(dev, sizeof(struct max31785_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	mutex_init(&data->device_lock);
+
+	/*
+	 * Initialize the max31785 chip
+	 */
+	err = max31785_init_client(client, data);
+	if (err)
+		return err;
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
+			client->name, data, max31785_groups);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id max31785_id[] = {
+	{ "max31785", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max31785_id);
+
+static struct i2c_driver max31785_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.probe		= max31785_probe,
+	.driver = {
+		.name	= "max31785",
+	},
+	.id_table	= max31785_id,
+	.detect		= max31785_detect,
+	.address_list	= normal_i2c,
+};
+
+module_i2c_driver(max31785_driver);
+
+MODULE_AUTHOR("Timothy Pearson <tpearson@raptorengineering.com>");
+MODULE_DESCRIPTION("MAX31785 sensor driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5