diff mbox

[PATCHv2] hwmon: Add tc654 driver

Message ID 20161007013845.10949-1-chris.packham@alliedtelesis.co.nz (mailing list archive)
State Changes Requested
Headers show

Commit Message

Chris Packham Oct. 7, 2016, 1:38 a.m. UTC
Add support for the tc654 and tc655 fan controllers from Microchip.

http://ww1.microchip.com/downloads/en/DeviceDoc/20001734C.pdf

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Changes in v2:
- Add Documentation/hwmon/tc654
- Incorporate most of the review comments from Guenter. Additional error
  handling is added. Unused/unnecessary code is removed. I decided not
  to go down the regmap path yet. I may circle back to it when I look at
  using regmap in the adm9240 driver.

 .../devicetree/bindings/i2c/trivial-devices.txt    |   2 +
 Documentation/hwmon/tc654                          |  26 ++
 drivers/hwmon/Kconfig                              |  11 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/tc654.c                              | 513 +++++++++++++++++++++
 5 files changed, 553 insertions(+)
 create mode 100644 Documentation/hwmon/tc654
 create mode 100644 drivers/hwmon/tc654.c

Comments

Guenter Roeck Oct. 7, 2016, 6:29 p.m. UTC | #1
On Fri, Oct 07, 2016 at 02:38:44PM +1300, Chris Packham wrote:
> Add support for the tc654 and tc655 fan controllers from Microchip.
> 
> http://ww1.microchip.com/downloads/en/DeviceDoc/20001734C.pdf
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Changes in v2:
> - Add Documentation/hwmon/tc654
> - Incorporate most of the review comments from Guenter. Additional error
>   handling is added. Unused/unnecessary code is removed. I decided not
>   to go down the regmap path yet. I may circle back to it when I look at
>   using regmap in the adm9240 driver.
> 
>  .../devicetree/bindings/i2c/trivial-devices.txt    |   2 +
>  Documentation/hwmon/tc654                          |  26 ++
>  drivers/hwmon/Kconfig                              |  11 +
>  drivers/hwmon/Makefile                             |   1 +
>  drivers/hwmon/tc654.c                              | 513 +++++++++++++++++++++
>  5 files changed, 553 insertions(+)
>  create mode 100644 Documentation/hwmon/tc654
>  create mode 100644 drivers/hwmon/tc654.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> index 1416c6a0d2cd..833fb9f133d3 100644
> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> @@ -122,6 +122,8 @@ microchip,mcp4662-502	Microchip 8-bit Dual I2C Digital Potentiometer with NV Mem
>  microchip,mcp4662-103	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (10k)
>  microchip,mcp4662-503	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (50k)
>  microchip,mcp4662-104	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (100k)
> +microchip,tc654		PWM Fan Speed Controller With Fan Fault Detection
> +microchip,tc655		PWM Fan Speed Controller With Fan Fault Detection
>  national,lm63		Temperature sensor with integrated fan control
>  national,lm75		I2C TEMP SENSOR
>  national,lm80		Serial Interface ACPI-Compatible Microprocessor System Hardware Monitor
> diff --git a/Documentation/hwmon/tc654 b/Documentation/hwmon/tc654
> new file mode 100644
> index 000000000000..93796c5c7e79
> --- /dev/null
> +++ b/Documentation/hwmon/tc654
> @@ -0,0 +1,26 @@
> +Kernel driver tc654
> +===================
> +
> +Supported chips:
> +  * Microship TC654 and TC655
> +    Prefix: 'tc654'
> +    Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/20001734C.pdf
> +
> +Authors:
> +        Chris Packham <chris.packham@alliedtelesis.co.nz>
> +        Masahiko Iwamoto <iwamoto@allied-telesis.co.jp>
> +
> +Description
> +-----------
> +This driver implements support for the Microchip TC654 and TC655.
> +
> +The TC654 used the 2-wire interface compatible with the SMBUS 2.0

uses

> +specification. The TC654 has two (2) inputs for measuring fan RPM and
> +one (1) PWM output which can be used for fan control.
> +
> +Configuration Notes
> +-------------------
> +Ordinarily the pwm1_mode ABI is used for controlling the pwm output
> +mode.  However, for this chip the output is always pwm, and the
> +pwm1_mode determines if the pwm output is controlled via the pwm1 value
> +or via the Vin analog input.

Please describe the supported values here.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 45cef3d2c75c..8681bc65cde5 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -907,6 +907,17 @@ config SENSORS_MCP3021
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called mcp3021.
>  
> +config SENSORS_TC654
> +	tristate "Microchip TC654/TC655 and compatibles"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for TC654 and TC655.
> +	  The TC654 and TC655 are PWM mode fan speed controllers with
> +	  FanSense technology for use with brushless DC fans.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tc654.
> +
>  config SENSORS_MENF21BMC_HWMON
>  	tristate "MEN 14F021P00 BMC Hardware Monitoring"
>  	depends on MFD_MENF21BMC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index aecf4ba17460..c651f0f1d047 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -122,6 +122,7 @@ obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
>  obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
>  obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
>  obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
> +obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
>  obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
>  obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
>  obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
> diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
> new file mode 100644
> index 000000000000..cba31cbd3383
> --- /dev/null
> +++ b/drivers/hwmon/tc654.c
> @@ -0,0 +1,513 @@
> +/*
> + * tc654.c - Linux kernel modules for fan speed controller
> + *
> + * Copyright (C) 2016 Allied Telesis Labs NZ
> + *
> + * 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/bitops.h>

> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/jiffies.h>
> +#include <linux/util_macros.h>

Please order include files alphabetically.

> +
> +enum tc654_regs {
> +	TC654_REG_RPM1 = 0x00,	/* RPM Output 1 */
> +	TC654_REG_RPM2 = 0x01,	/* RPM Output 2 */
> +	TC654_REG_FAN_FAULT1 = 0x02,	/* Fan Fault 1 Threshold */
> +	TC654_REG_FAN_FAULT2 = 0x03,	/* Fan Fault 2 Threshold */
> +	TC654_REG_CONFIG = 0x04,	/* Configuration */
> +	TC654_REG_STATUS = 0x05,	/* Status */
> +	TC654_REG_DUTY_CYCLE = 0x06,	/* Fan Speed Duty Cycle */
> +	TC654_REG_MFR_ID = 0x07,	/* Manufacturer Identification */
> +	TC654_REG_VER_ID = 0x08,	/* Version Identification */
> +};
> +
> +/* Macros to easily index the registers */
> +#define TC654_REG_RPM(idx) (TC654_REG_RPM1 + (idx))
> +#define TC654_REG_FAN_FAULT(idx) (TC654_REG_FAN_FAULT1 + (idx))
> +
> +/* Config register bits */
> +#define TC654_REG_CONFIG_RES 0x40	/* Resolution Selection */
> +#define TC654_REG_CONFIG_DUTYC 0x20	/* Duty Cycle Control Method */
> +#define TC654_REG_CONFIG_SDM 0x01	/* Shutdown Mode */
> +
> +/* Status register bits */
> +#define TC654_REG_STATUS_F2F 0x02	/* Fan 2 Fault */
> +#define TC654_REG_STATUS_F1F 0x01	/* Fan 1 Fault */
> +

Didn't notice earlier ... those are bits, so it would be better to use
the BIT() macro.

> +/* RPM resolution for RPM Output registers */
> +#define TC654_HIGH_RPM_RESOLUTION 25	/* 25 RPM resolution */
> +#define TC654_LOW_RPM_RESOLUTION 50	/* 50 RPM resolution */
> +
> +/* Convert to the fan fault RPM threshold from register value */
> +#define TC654_FAN_FAULT_FROM_REG(val) ((val) * 50)	/* 50 RPM resolution */
> +
> +/* Convert to register value from the fan fault RPM threshold */
> +#define TC654_FAN_FAULT_TO_REG(val) (((val) / 50) & 0xff)
> +
> +/* Register data is read (and cached) at most once per second. */
> +#define TC654_UPDATE_INTERVAL	HZ
> +
> +struct tc654_data {
> +	struct i2c_client *client;
> +	struct device *hwmon_dev;

No longer needed. Just keep the variable local in the probe function.

> +
> +	/* update mutex */
> +	struct mutex update_lock;
> +
> +	/* tc654 register cache */
> +	bool valid;
> +	unsigned long last_updated;	/* in jiffies */
> +
> +	u8 rpm_output[2];	/* The fan RPM data for fans 1 and 2 is then
> +				 * written to registers RPM1 and RPM2
> +				 */
> +	u8 fan_fault[2];	/* The Fan Fault Threshold Registers are used to
> +				 * set the fan fault threshold levels for fan 1
> +				 * and fan 2
> +				 */
> +	u8 config;	/* The Configuration Register is an 8-bit read/
> +			 * writable multi-function control register
> +			 *   7: Fan Fault Clear
> +			 *      1 = Clear Fan Fault
> +			 *      0 = Normal Operation (default)
> +			 *   6: Resolution Selection for RPM Output Registers
> +			 *      RPM Output Registers (RPM1 and RPM2) will be
> +			 *      set for
> +			 *      1 = 25 RPM (9-bit) resolution
> +			 *      0 = 50 RPM (8-bit) resolution (default)
> +			 *   5: Duty Cycle Control Method
> +			 *      The V OUT duty cycle will be controlled via
> +			 *      1 = the SMBus interface.
> +			 *      0 = via the V IN analog input pin. (default)
> +			 * 4,3: Fan 2 Pulses Per Rotation
> +			 *      00 = 1
> +			 *      01 = 2 (default)
> +			 *      10 = 4
> +			 *      11 = 8
> +			 * 2,1: Fan 1 Pulses Per Rotation
> +			 *      00 = 1
> +			 *      01 = 2 (default)
> +			 *      10 = 4
> +			 *      11 = 8
> +			 *   0: Shutdown Mode
> +			 *      1 = Shutdown mode.
> +			 *      0 = Normal operation. (default)
> +			 */
> +	u8 status;	/* The Status register provides all the information
> +			 * about what is going on within the TC654/TC655
> +			 * devices.
> +			 * 7,6: Unimplemented, Read as '0'
> +			 *   5: Over-Temperature Fault Condition
> +			 *      1 = Over-Temperature condition has occurred
> +			 *      0 = Normal operation. V IN is less than 2.6V
> +			 *   4: RPM2 Counter Overflow
> +			 *      1 = Fault condition
> +			 *      0 = Normal operation
> +			 *   3: RPM1 Counter Overflow
> +			 *      1 = Fault condition
> +			 *      0 = Normal operation
> +			 *   2: V IN Input Status
> +			 *      1 = V IN is open
> +			 *      0 = Normal operation. voltage present at V IN
> +			 *   1: Fan 2 Fault
> +			 *      1 = Fault condition
> +			 *      0 = Normal operation
> +			 *   0: Fan 1 Fault
> +			 *      1 = Fault condition
> +			 *      0 = Normal operation
> +			 */
> +	u8 duty_cycle;	/* The DUTY_CYCLE register is a 4-bit read/
> +			 * writable register used to control the duty
> +			 * cycle of the V OUT output.
> +			 */
> +};
> +
> +/* helper to grab and cache data, at most one time per second */
> +static struct tc654_data *tc654_update_client(struct device *dev)
> +{
> +	struct tc654_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret = 0;
> +
> +	mutex_lock(&data->update_lock);
> +	if (time_before(jiffies, data->last_updated + TC654_UPDATE_INTERVAL) &&
> +	    likely(data->valid))
> +		goto out;
> +
> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_RPM(0));
> +	if (ret < 0)
> +		goto out;
> +	data->rpm_output[0] = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_RPM(1));
> +	if (ret < 0)
> +		goto out;
> +	data->rpm_output[1] = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_FAN_FAULT(0));
> +	if (ret < 0)
> +		goto out;
> +	data->fan_fault[0] = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_FAN_FAULT(1));
> +	if (ret < 0)
> +		goto out;
> +	data->fan_fault[1] = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_CONFIG);
> +	if (ret < 0)
> +		goto out;
> +	data->config = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_STATUS);
> +	if (ret < 0)
> +		goto out;
> +	data->status = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_DUTY_CYCLE);
> +	if (ret < 0)
> +		goto out;
> +	data->duty_cycle = ret;

Maybe make it
	data->duty_cycle = ret & 0x0f;

While that should not be necessary, it doesn't hurt, and the datasheet isn't
entirely clear if the upper bits are guaranteed to be 0.

> +
> +	data->last_updated = jiffies;
> +	data->valid = true;
> +out:
> +	mutex_unlock(&data->update_lock);
> +
> +	if (ret < 0)		/* upon error, encode it in return value */
> +		data = ERR_PTR(ret);
> +
> +	return data;
> +}
> +
> +/*
> + * sysfs attributes
> + */
> +
> +static ssize_t show_fan(struct device *dev, struct device_attribute *da,
> +			char *buf)
> +{
> +	int nr = to_sensor_dev_attr(da)->index;
> +	struct tc654_data *data = tc654_update_client(dev);
> +	int val;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	if (data->config & TC654_REG_CONFIG_RES)
> +		val = data->rpm_output[nr] * TC654_HIGH_RPM_RESOLUTION;
> +	else
> +		val = data->rpm_output[nr] * TC654_LOW_RPM_RESOLUTION;
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t show_fan_min(struct device *dev, struct device_attribute *da,
> +			    char *buf)
> +{
> +	int nr = to_sensor_dev_attr(da)->index;
> +	struct tc654_data *data = tc654_update_client(dev);
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	return sprintf(buf, "%d\n",
> +		       TC654_FAN_FAULT_FROM_REG(data->fan_fault[nr]));
> +}
> +
> +static ssize_t set_fan_min(struct device *dev, struct device_attribute *da,
> +			   const char *buf, size_t count)
> +{
> +	int nr = to_sensor_dev_attr(da)->index;
> +	struct tc654_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long val;
> +	int ret;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	clamp_val(val, 0, 12750);

	val = clamp_val(val, 0, 12750);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	data->fan_fault[nr] = TC654_FAN_FAULT_TO_REG(val);
> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_FAN_FAULT(nr),
> +				  data->fan_fault[nr]);

Hmmm ... the reason for asking you to align continuation lines with '('
is that it makes the code more uniform and helps me review it. I understand
that people sometimes don't like it, but please keep in mind that it helps
with the code review.

> +
> +	mutex_unlock(&data->update_lock);
> +	return ret < 0 ? ret : count;
> +}
> +
> +static ssize_t show_fan_alarm(struct device *dev, struct device_attribute *da,
> +			      char *buf)
> +{
> +	int nr = to_sensor_dev_attr(da)->index;
> +	struct tc654_data *data = tc654_update_client(dev);
> +	int val;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	if (nr == 0)
> +		val = !!(data->status & TC654_REG_STATUS_F1F);
> +	else
> +		val = !!(data->status & TC654_REG_STATUS_F2F);
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static const u8 TC654_FAN_PULSE_SHIFT[] = { 1, 3 };
> +
> +static ssize_t show_fan_pulses(struct device *dev, struct device_attribute *da,
> +			      char *buf)
> +{
> +	int nr = to_sensor_dev_attr(da)->index;
> +	struct tc654_data *data = tc654_update_client(dev);
> +	u8 val;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	val = BIT((data->config >> TC654_FAN_PULSE_SHIFT[nr]) & 0x03);
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t set_fan_pulses(struct device *dev, struct device_attribute *da,
> +			     const char *buf, size_t count)
> +{
> +	int nr = to_sensor_dev_attr(da)->index;
> +	struct tc654_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	u8 config;
> +	unsigned long val;
> +	int ret;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	switch (val) {
> +	case 1:
> +		config = 0;
> +		break;
> +	case 2:
> +		config = 1;
> +		break;
> +	case 4:
> +		config = 2;
> +		break;
> +	case 8:
> +		config = 3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&data->update_lock);
> +
> +	data->config &= ~(0x03 << TC654_FAN_PULSE_SHIFT[nr]);
> +	data->config |= (config << TC654_FAN_PULSE_SHIFT[nr]);
> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +
> +	mutex_unlock(&data->update_lock);
> +	return ret < 0 ? ret : count;
> +}
> +
> +static ssize_t show_pwm_mode(struct device *dev,
> +			     struct device_attribute *da, char *buf)
> +{
> +	struct tc654_data *data = tc654_update_client(dev);
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	return sprintf(buf, "%d\n", data->config & TC654_REG_CONFIG_DUTYC);

Should be
				!!(data->config & TC654_REG_CONFIG_DUTYC)
otherwise it displays 0 or 32.

> +}
> +
> +static ssize_t set_pwm_mode(struct device *dev,
> +			    struct device_attribute *da,
> +			    const char *buf, size_t count)
> +{
> +	struct tc654_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long val;
> +	int ret;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	if (val != 0 && val != 1)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (val)
> +		data->config |= TC654_REG_CONFIG_DUTYC;
> +	else
> +		data->config &= ~TC654_REG_CONFIG_DUTYC;
> +
> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +
> +	mutex_unlock(&data->update_lock);
> +	return ret < 0 ? ret : count;
> +}
> +
> +static const int tc654_pwm_map[16] = { 76, 88, 100, 112, 124, 141, 147, 171,
> +		183, 195, 207, 219, 231, 243, 255 };
> +

This lists 15 entries for an array of size 16, leaving the last entry
at 0. Is there an entry missing ?

Also, 141 yields 55.29%, which doesn't match the datasheet.

I ended up spending some time to match the numbers:

map	%	datasheet
76	29.8	30.0
88	34.5	34.67
100	39.21	39.33
112	43.92	44.0
124	48.62	48.67
141	55.29	53.33	off (136 would be 53.33%)
147	57.64	58.0	148 would be 58.03%
??	??	62.67	missing (160 would be 62.67%)
171	67.05	67.33	172 would be 67.45%
183	71.76	72.0	184 would be 72.15%
195	76.47	76.67
207	81.17	81.33
219	85.88	86.0
231	90.58	90.67
243	95.29	95.33
255	100	100

> +static ssize_t show_pwm(struct device *dev, struct device_attribute *da,
> +			       char *buf)
> +{
> +	struct tc654_data *data = tc654_update_client(dev);
> +	int pwm;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	if (data->config & TC654_REG_CONFIG_SDM)
> +		pwm = 0;
> +	else
> +		pwm = tc654_pwm_map[data->duty_cycle];
> +
> +	return sprintf(buf, "%d\n", pwm);
> +}
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *da,
> +			      const char *buf, size_t count)
> +{
> +	struct tc654_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	unsigned long val;
> +	int ret;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +	if (val > 255)
> +		return -EINVAL;
> +
> +	if (val == 0)
> +		data->config |= TC654_REG_CONFIG_SDM;
> +	else
> +		data->config &= ~TC654_REG_CONFIG_SDM;
> +
> +	data->duty_cycle = find_closest(val, tc654_pwm_map,
> +					ARRAY_SIZE(tc654_pwm_map));
> +
> +	mutex_lock(&data->update_lock);
> +
> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_DUTY_CYCLE,
> +				  data->duty_cycle);
> +	if (ret < 0)
> +		goto out;
> +
> +out:
> +	mutex_unlock(&data->update_lock);
> +	return ret < 0 ? ret : count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_fan, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, show_fan, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan_min,
> +			  set_fan_min, 0);
> +static SENSOR_DEVICE_ATTR(fan2_min, S_IWUSR | S_IRUGO, show_fan_min,
> +			  set_fan_min, 1);
> +static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO, show_fan_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO, show_fan_alarm, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan1_pulses, S_IWUSR | S_IRUGO, show_fan_pulses,
> +			  set_fan_pulses, 0);
> +static SENSOR_DEVICE_ATTR(fan2_pulses, S_IWUSR | S_IRUGO, show_fan_pulses,
> +			  set_fan_pulses, 1);
> +static SENSOR_DEVICE_ATTR(pwm1_mode, S_IWUSR | S_IRUGO,
> +			  show_pwm_mode, set_pwm_mode, 0);
> +static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm,
> +			  set_pwm, 0);
> +
> +/* Driver data */
> +static struct attribute *tc654_attrs[] = {
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,
> +	&sensor_dev_attr_fan1_min.dev_attr.attr,
> +	&sensor_dev_attr_fan2_min.dev_attr.attr,
> +	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
> +	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
> +	&sensor_dev_attr_fan1_pulses.dev_attr.attr,
> +	&sensor_dev_attr_fan2_pulses.dev_attr.attr,
> +	&sensor_dev_attr_pwm1_mode.dev_attr.attr,
> +	&sensor_dev_attr_pwm1.dev_attr.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(tc654);
> +
> +/*
> + * device probe and removal
> + */
> +
> +static int tc654_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct tc654_data *data;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(dev, sizeof(struct tc654_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	i2c_set_clientdata(client, data);

No longer needed.

> +	mutex_init(&data->update_lock);
> +
> +	data->hwmon_dev =
> +	    devm_hwmon_device_register_with_groups(dev, client->name, data,
> +						   tc654_groups);
> +	if (IS_ERR(data->hwmon_dev))
> +		return PTR_ERR(data->hwmon_dev);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id tc654_id[] = {
> +	{"tc654", 0},
> +	{"tc655", 0},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, tc654_id);
> +
> +static struct i2c_driver tc654_driver = {
> +	.driver = {
> +		   .name = "tc654",
> +		   .owner = THIS_MODULE,

Not needed (see Julia's patch)

> +		   },
> +	.probe = tc654_probe,
> +	.id_table = tc654_id,
> +};
> +
> +module_i2c_driver(tc654_driver);
> +
> +MODULE_AUTHOR("Allied Telesis Labs");
> +MODULE_DESCRIPTION("Microchip TC654/TC655 driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.10.0.479.g7c56b16
> 
--
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
Chris Packham Oct. 9, 2016, 9:21 p.m. UTC | #2
Hi Gunter,

Thanks for the review. v3 on it's way some responses below.

On 10/08/2016 07:29 AM, Guenter Roeck wrote:
> On Fri, Oct 07, 2016 at 02:38:44PM +1300, Chris Packham wrote:
>> Add support for the tc654 and tc655 fan controllers from Microchip.
>>
>> http://ww1.microchip.com/downloads/en/DeviceDoc/20001734C.pdf
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Changes in v2:
>> - Add Documentation/hwmon/tc654
>> - Incorporate most of the review comments from Guenter. Additional error
>>   handling is added. Unused/unnecessary code is removed. I decided not
>>   to go down the regmap path yet. I may circle back to it when I look at
>>   using regmap in the adm9240 driver.
>>
>>  .../devicetree/bindings/i2c/trivial-devices.txt    |   2 +
>>  Documentation/hwmon/tc654                          |  26 ++
>>  drivers/hwmon/Kconfig                              |  11 +
>>  drivers/hwmon/Makefile                             |   1 +
>>  drivers/hwmon/tc654.c                              | 513 +++++++++++++++++++++
>>  5 files changed, 553 insertions(+)
>>  create mode 100644 Documentation/hwmon/tc654
>>  create mode 100644 drivers/hwmon/tc654.c
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>> index 1416c6a0d2cd..833fb9f133d3 100644
>> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>> @@ -122,6 +122,8 @@ microchip,mcp4662-502	Microchip 8-bit Dual I2C Digital Potentiometer with NV Mem
>>  microchip,mcp4662-103	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (10k)
>>  microchip,mcp4662-503	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (50k)
>>  microchip,mcp4662-104	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (100k)
>> +microchip,tc654		PWM Fan Speed Controller With Fan Fault Detection
>> +microchip,tc655		PWM Fan Speed Controller With Fan Fault Detection
>>  national,lm63		Temperature sensor with integrated fan control
>>  national,lm75		I2C TEMP SENSOR
>>  national,lm80		Serial Interface ACPI-Compatible Microprocessor System Hardware Monitor
>> diff --git a/Documentation/hwmon/tc654 b/Documentation/hwmon/tc654
>> new file mode 100644
>> index 000000000000..93796c5c7e79
>> --- /dev/null
>> +++ b/Documentation/hwmon/tc654
>> @@ -0,0 +1,26 @@
>> +Kernel driver tc654
>> +===================
>> +
>> +Supported chips:
>> +  * Microship TC654 and TC655
>> +    Prefix: 'tc654'
>> +    Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/20001734C.pdf
>> +
>> +Authors:
>> +        Chris Packham <chris.packham@alliedtelesis.co.nz>
>> +        Masahiko Iwamoto <iwamoto@allied-telesis.co.jp>
>> +
>> +Description
>> +-----------
>> +This driver implements support for the Microchip TC654 and TC655.
>> +
>> +The TC654 used the 2-wire interface compatible with the SMBUS 2.0
>
> uses
>

Done.

>> +specification. The TC654 has two (2) inputs for measuring fan RPM and
>> +one (1) PWM output which can be used for fan control.
>> +
>> +Configuration Notes
>> +-------------------
>> +Ordinarily the pwm1_mode ABI is used for controlling the pwm output
>> +mode.  However, for this chip the output is always pwm, and the
>> +pwm1_mode determines if the pwm output is controlled via the pwm1 value
>> +or via the Vin analog input.
>
> Please describe the supported values here.
>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 45cef3d2c75c..8681bc65cde5 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -907,6 +907,17 @@ config SENSORS_MCP3021
>>  	  This driver can also be built as a module.  If so, the module
>>  	  will be called mcp3021.
>>
>> +config SENSORS_TC654
>> +	tristate "Microchip TC654/TC655 and compatibles"
>> +	depends on I2C
>> +	help
>> +	  If you say yes here you get support for TC654 and TC655.
>> +	  The TC654 and TC655 are PWM mode fan speed controllers with
>> +	  FanSense technology for use with brushless DC fans.
>> +
>> +	  This driver can also be built as a module.  If so, the module
>> +	  will be called tc654.
>> +
>>  config SENSORS_MENF21BMC_HWMON
>>  	tristate "MEN 14F021P00 BMC Hardware Monitoring"
>>  	depends on MFD_MENF21BMC
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index aecf4ba17460..c651f0f1d047 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -122,6 +122,7 @@ obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
>>  obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
>>  obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
>>  obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
>> +obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
>>  obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
>>  obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
>>  obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
>> diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
>> new file mode 100644
>> index 000000000000..cba31cbd3383
>> --- /dev/null
>> +++ b/drivers/hwmon/tc654.c
>> @@ -0,0 +1,513 @@
>> +/*
>> + * tc654.c - Linux kernel modules for fan speed controller
>> + *
>> + * Copyright (C) 2016 Allied Telesis Labs NZ
>> + *
>> + * 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/bitops.h>
>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/i2c.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/util_macros.h>
>
> Please order include files alphabetically.
>

Done.

>> +
>> +enum tc654_regs {
>> +	TC654_REG_RPM1 = 0x00,	/* RPM Output 1 */
>> +	TC654_REG_RPM2 = 0x01,	/* RPM Output 2 */
>> +	TC654_REG_FAN_FAULT1 = 0x02,	/* Fan Fault 1 Threshold */
>> +	TC654_REG_FAN_FAULT2 = 0x03,	/* Fan Fault 2 Threshold */
>> +	TC654_REG_CONFIG = 0x04,	/* Configuration */
>> +	TC654_REG_STATUS = 0x05,	/* Status */
>> +	TC654_REG_DUTY_CYCLE = 0x06,	/* Fan Speed Duty Cycle */
>> +	TC654_REG_MFR_ID = 0x07,	/* Manufacturer Identification */
>> +	TC654_REG_VER_ID = 0x08,	/* Version Identification */
>> +};
>> +
>> +/* Macros to easily index the registers */
>> +#define TC654_REG_RPM(idx) (TC654_REG_RPM1 + (idx))
>> +#define TC654_REG_FAN_FAULT(idx) (TC654_REG_FAN_FAULT1 + (idx))
>> +
>> +/* Config register bits */
>> +#define TC654_REG_CONFIG_RES 0x40	/* Resolution Selection */
>> +#define TC654_REG_CONFIG_DUTYC 0x20	/* Duty Cycle Control Method */
>> +#define TC654_REG_CONFIG_SDM 0x01	/* Shutdown Mode */
>> +
>> +/* Status register bits */
>> +#define TC654_REG_STATUS_F2F 0x02	/* Fan 2 Fault */
>> +#define TC654_REG_STATUS_F1F 0x01	/* Fan 1 Fault */
>> +
>
> Didn't notice earlier ... those are bits, so it would be better to use
> the BIT() macro.
>
>> +/* RPM resolution for RPM Output registers */
>> +#define TC654_HIGH_RPM_RESOLUTION 25	/* 25 RPM resolution */
>> +#define TC654_LOW_RPM_RESOLUTION 50	/* 50 RPM resolution */
>> +
>> +/* Convert to the fan fault RPM threshold from register value */
>> +#define TC654_FAN_FAULT_FROM_REG(val) ((val) * 50)	/* 50 RPM resolution */
>> +
>> +/* Convert to register value from the fan fault RPM threshold */
>> +#define TC654_FAN_FAULT_TO_REG(val) (((val) / 50) & 0xff)
>> +
>> +/* Register data is read (and cached) at most once per second. */
>> +#define TC654_UPDATE_INTERVAL	HZ
>> +
>> +struct tc654_data {
>> +	struct i2c_client *client;
>> +	struct device *hwmon_dev;
>
> No longer needed. Just keep the variable local in the probe function.
>
>> +
>> +	/* update mutex */
>> +	struct mutex update_lock;
>> +
>> +	/* tc654 register cache */
>> +	bool valid;
>> +	unsigned long last_updated;	/* in jiffies */
>> +
>> +	u8 rpm_output[2];	/* The fan RPM data for fans 1 and 2 is then
>> +				 * written to registers RPM1 and RPM2
>> +				 */
>> +	u8 fan_fault[2];	/* The Fan Fault Threshold Registers are used to
>> +				 * set the fan fault threshold levels for fan 1
>> +				 * and fan 2
>> +				 */
>> +	u8 config;	/* The Configuration Register is an 8-bit read/
>> +			 * writable multi-function control register
>> +			 *   7: Fan Fault Clear
>> +			 *      1 = Clear Fan Fault
>> +			 *      0 = Normal Operation (default)
>> +			 *   6: Resolution Selection for RPM Output Registers
>> +			 *      RPM Output Registers (RPM1 and RPM2) will be
>> +			 *      set for
>> +			 *      1 = 25 RPM (9-bit) resolution
>> +			 *      0 = 50 RPM (8-bit) resolution (default)
>> +			 *   5: Duty Cycle Control Method
>> +			 *      The V OUT duty cycle will be controlled via
>> +			 *      1 = the SMBus interface.
>> +			 *      0 = via the V IN analog input pin. (default)
>> +			 * 4,3: Fan 2 Pulses Per Rotation
>> +			 *      00 = 1
>> +			 *      01 = 2 (default)
>> +			 *      10 = 4
>> +			 *      11 = 8
>> +			 * 2,1: Fan 1 Pulses Per Rotation
>> +			 *      00 = 1
>> +			 *      01 = 2 (default)
>> +			 *      10 = 4
>> +			 *      11 = 8
>> +			 *   0: Shutdown Mode
>> +			 *      1 = Shutdown mode.
>> +			 *      0 = Normal operation. (default)
>> +			 */
>> +	u8 status;	/* The Status register provides all the information
>> +			 * about what is going on within the TC654/TC655
>> +			 * devices.
>> +			 * 7,6: Unimplemented, Read as '0'
>> +			 *   5: Over-Temperature Fault Condition
>> +			 *      1 = Over-Temperature condition has occurred
>> +			 *      0 = Normal operation. V IN is less than 2.6V
>> +			 *   4: RPM2 Counter Overflow
>> +			 *      1 = Fault condition
>> +			 *      0 = Normal operation
>> +			 *   3: RPM1 Counter Overflow
>> +			 *      1 = Fault condition
>> +			 *      0 = Normal operation
>> +			 *   2: V IN Input Status
>> +			 *      1 = V IN is open
>> +			 *      0 = Normal operation. voltage present at V IN
>> +			 *   1: Fan 2 Fault
>> +			 *      1 = Fault condition
>> +			 *      0 = Normal operation
>> +			 *   0: Fan 1 Fault
>> +			 *      1 = Fault condition
>> +			 *      0 = Normal operation
>> +			 */
>> +	u8 duty_cycle;	/* The DUTY_CYCLE register is a 4-bit read/
>> +			 * writable register used to control the duty
>> +			 * cycle of the V OUT output.
>> +			 */
>> +};
>> +
>> +/* helper to grab and cache data, at most one time per second */
>> +static struct tc654_data *tc654_update_client(struct device *dev)
>> +{
>> +	struct tc654_data *data = dev_get_drvdata(dev);
>> +	struct i2c_client *client = data->client;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&data->update_lock);
>> +	if (time_before(jiffies, data->last_updated + TC654_UPDATE_INTERVAL) &&
>> +	    likely(data->valid))
>> +		goto out;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_RPM(0));
>> +	if (ret < 0)
>> +		goto out;
>> +	data->rpm_output[0] = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_RPM(1));
>> +	if (ret < 0)
>> +		goto out;
>> +	data->rpm_output[1] = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_FAN_FAULT(0));
>> +	if (ret < 0)
>> +		goto out;
>> +	data->fan_fault[0] = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_FAN_FAULT(1));
>> +	if (ret < 0)
>> +		goto out;
>> +	data->fan_fault[1] = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_CONFIG);
>> +	if (ret < 0)
>> +		goto out;
>> +	data->config = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_STATUS);
>> +	if (ret < 0)
>> +		goto out;
>> +	data->status = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, TC654_REG_DUTY_CYCLE);
>> +	if (ret < 0)
>> +		goto out;
>> +	data->duty_cycle = ret;
>
> Maybe make it
> 	data->duty_cycle = ret & 0x0f;
>
> While that should not be necessary, it doesn't hurt, and the datasheet isn't
> entirely clear if the upper bits are guaranteed to be 0.
>

Done.

>> +
>> +	data->last_updated = jiffies;
>> +	data->valid = true;
>> +out:
>> +	mutex_unlock(&data->update_lock);
>> +
>> +	if (ret < 0)		/* upon error, encode it in return value */
>> +		data = ERR_PTR(ret);
>> +
>> +	return data;
>> +}
>> +
>> +/*
>> + * sysfs attributes
>> + */
>> +
>> +static ssize_t show_fan(struct device *dev, struct device_attribute *da,
>> +			char *buf)
>> +{
>> +	int nr = to_sensor_dev_attr(da)->index;
>> +	struct tc654_data *data = tc654_update_client(dev);
>> +	int val;
>> +
>> +	if (IS_ERR(data))
>> +		return PTR_ERR(data);
>> +
>> +	if (data->config & TC654_REG_CONFIG_RES)
>> +		val = data->rpm_output[nr] * TC654_HIGH_RPM_RESOLUTION;
>> +	else
>> +		val = data->rpm_output[nr] * TC654_LOW_RPM_RESOLUTION;
>> +
>> +	return sprintf(buf, "%d\n", val);
>> +}
>> +
>> +static ssize_t show_fan_min(struct device *dev, struct device_attribute *da,
>> +			    char *buf)
>> +{
>> +	int nr = to_sensor_dev_attr(da)->index;
>> +	struct tc654_data *data = tc654_update_client(dev);
>> +
>> +	if (IS_ERR(data))
>> +		return PTR_ERR(data);
>> +
>> +	return sprintf(buf, "%d\n",
>> +		       TC654_FAN_FAULT_FROM_REG(data->fan_fault[nr]));
>> +}
>> +
>> +static ssize_t set_fan_min(struct device *dev, struct device_attribute *da,
>> +			   const char *buf, size_t count)
>> +{
>> +	int nr = to_sensor_dev_attr(da)->index;
>> +	struct tc654_data *data = dev_get_drvdata(dev);
>> +	struct i2c_client *client = data->client;
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	if (kstrtoul(buf, 10, &val))
>> +		return -EINVAL;
>> +
>> +	clamp_val(val, 0, 12750);
>
> 	val = clamp_val(val, 0, 12750);
>> +
>> +	mutex_lock(&data->update_lock);
>> +
>> +	data->fan_fault[nr] = TC654_FAN_FAULT_TO_REG(val);
>> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_FAN_FAULT(nr),
>> +				  data->fan_fault[nr]);
>
> Hmmm ... the reason for asking you to align continuation lines with '('
> is that it makes the code more uniform and helps me review it. I understand
> that people sometimes don't like it, but please keep in mind that it helps
> with the code review.
>

Sorry about that. Didn't re-align when I added the 'ret ='.

>> +
>> +	mutex_unlock(&data->update_lock);
>> +	return ret < 0 ? ret : count;
>> +}
>> +
>> +static ssize_t show_fan_alarm(struct device *dev, struct device_attribute *da,
>> +			      char *buf)
>> +{
>> +	int nr = to_sensor_dev_attr(da)->index;
>> +	struct tc654_data *data = tc654_update_client(dev);
>> +	int val;
>> +
>> +	if (IS_ERR(data))
>> +		return PTR_ERR(data);
>> +
>> +	if (nr == 0)
>> +		val = !!(data->status & TC654_REG_STATUS_F1F);
>> +	else
>> +		val = !!(data->status & TC654_REG_STATUS_F2F);
>> +
>> +	return sprintf(buf, "%d\n", val);
>> +}
>> +
>> +static const u8 TC654_FAN_PULSE_SHIFT[] = { 1, 3 };
>> +
>> +static ssize_t show_fan_pulses(struct device *dev, struct device_attribute *da,
>> +			      char *buf)
>> +{
>> +	int nr = to_sensor_dev_attr(da)->index;
>> +	struct tc654_data *data = tc654_update_client(dev);
>> +	u8 val;
>> +
>> +	if (IS_ERR(data))
>> +		return PTR_ERR(data);
>> +
>> +	val = BIT((data->config >> TC654_FAN_PULSE_SHIFT[nr]) & 0x03);
>> +	return sprintf(buf, "%d\n", val);
>> +}
>> +
>> +static ssize_t set_fan_pulses(struct device *dev, struct device_attribute *da,
>> +			     const char *buf, size_t count)
>> +{
>> +	int nr = to_sensor_dev_attr(da)->index;
>> +	struct tc654_data *data = dev_get_drvdata(dev);
>> +	struct i2c_client *client = data->client;
>> +	u8 config;
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	if (kstrtoul(buf, 10, &val))
>> +		return -EINVAL;
>> +
>> +	switch (val) {
>> +	case 1:
>> +		config = 0;
>> +		break;
>> +	case 2:
>> +		config = 1;
>> +		break;
>> +	case 4:
>> +		config = 2;
>> +		break;
>> +	case 8:
>> +		config = 3;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&data->update_lock);
>> +
>> +	data->config &= ~(0x03 << TC654_FAN_PULSE_SHIFT[nr]);
>> +	data->config |= (config << TC654_FAN_PULSE_SHIFT[nr]);
>> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
>> +
>> +	mutex_unlock(&data->update_lock);
>> +	return ret < 0 ? ret : count;
>> +}
>> +
>> +static ssize_t show_pwm_mode(struct device *dev,
>> +			     struct device_attribute *da, char *buf)
>> +{
>> +	struct tc654_data *data = tc654_update_client(dev);
>> +
>> +	if (IS_ERR(data))
>> +		return PTR_ERR(data);
>> +
>> +	return sprintf(buf, "%d\n", data->config & TC654_REG_CONFIG_DUTYC);
>
> Should be
> 				!!(data->config & TC654_REG_CONFIG_DUTYC)
> otherwise it displays 0 or 32.
>

Done.

>> +}
>> +
>> +static ssize_t set_pwm_mode(struct device *dev,
>> +			    struct device_attribute *da,
>> +			    const char *buf, size_t count)
>> +{
>> +	struct tc654_data *data = dev_get_drvdata(dev);
>> +	struct i2c_client *client = data->client;
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	if (kstrtoul(buf, 10, &val))
>> +		return -EINVAL;
>> +
>> +	if (val != 0 && val != 1)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&data->update_lock);
>> +
>> +	if (val)
>> +		data->config |= TC654_REG_CONFIG_DUTYC;
>> +	else
>> +		data->config &= ~TC654_REG_CONFIG_DUTYC;
>> +
>> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
>> +
>> +	mutex_unlock(&data->update_lock);
>> +	return ret < 0 ? ret : count;
>> +}
>> +
>> +static const int tc654_pwm_map[16] = { 76, 88, 100, 112, 124, 141, 147, 171,
>> +		183, 195, 207, 219, 231, 243, 255 };
>> +
>
> This lists 15 entries for an array of size 16, leaving the last entry
> at 0. Is there an entry missing ?
>
> Also, 141 yields 55.29%, which doesn't match the datasheet.
>
> I ended up spending some time to match the numbers:
>
> map	%	datasheet
> 76	29.8	30.0
> 88	34.5	34.67
> 100	39.21	39.33
> 112	43.92	44.0
> 124	48.62	48.67
> 141	55.29	53.33	off (136 would be 53.33%)
> 147	57.64	58.0	148 would be 58.03%
> ??	??	62.67	missing (160 would be 62.67%)
> 171	67.05	67.33	172 would be 67.45%
> 183	71.76	72.0	184 would be 72.15%
> 195	76.47	76.67
> 207	81.17	81.33
> 219	85.88	86.0
> 231	90.58	90.67
> 243	95.29	95.33
> 255	100	100
>

Thanks for (re-)doing my math. I initially did these by hand so I'm not 
surprised I missed one. I've put all the values through a spreadsheet 
and used rounding to come up with the following

map	%	datasheet
77	30.20%	30.00%
88	34.51%	34.67%
102	40.00%	39.93%
112	43.92%	44.00%
124	48.63%	48.67%
136	53.33%	53.33%
148	58.04%	58.00%
160	62.75%	62.67%
172	67.45%	67.33%
184	72.16%	72.00%
196	76.86%	76.67%
207	81.18%	81.33%
219	85.88%	86.00%
231	90.59%	90.67%
243	95.29%	95.33%
255	100.00%	100.00%

Differences are due to rounding.

>> +static ssize_t show_pwm(struct device *dev, struct device_attribute *da,
>> +			       char *buf)
>> +{
>> +	struct tc654_data *data = tc654_update_client(dev);
>> +	int pwm;
>> +
>> +	if (IS_ERR(data))
>> +		return PTR_ERR(data);
>> +
>> +	if (data->config & TC654_REG_CONFIG_SDM)
>> +		pwm = 0;
>> +	else
>> +		pwm = tc654_pwm_map[data->duty_cycle];
>> +
>> +	return sprintf(buf, "%d\n", pwm);
>> +}
>> +
>> +static ssize_t set_pwm(struct device *dev, struct device_attribute *da,
>> +			      const char *buf, size_t count)
>> +{
>> +	struct tc654_data *data = dev_get_drvdata(dev);
>> +	struct i2c_client *client = data->client;
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	if (kstrtoul(buf, 10, &val))
>> +		return -EINVAL;
>> +	if (val > 255)
>> +		return -EINVAL;
>> +
>> +	if (val == 0)
>> +		data->config |= TC654_REG_CONFIG_SDM;
>> +	else
>> +		data->config &= ~TC654_REG_CONFIG_SDM;
>> +
>> +	data->duty_cycle = find_closest(val, tc654_pwm_map,
>> +					ARRAY_SIZE(tc654_pwm_map));
>> +
>> +	mutex_lock(&data->update_lock);
>> +
>> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	ret = i2c_smbus_write_byte_data(client, TC654_REG_DUTY_CYCLE,
>> +				  data->duty_cycle);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +out:
>> +	mutex_unlock(&data->update_lock);
>> +	return ret < 0 ? ret : count;
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_fan, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, show_fan, NULL, 1);
>> +static SENSOR_DEVICE_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan_min,
>> +			  set_fan_min, 0);
>> +static SENSOR_DEVICE_ATTR(fan2_min, S_IWUSR | S_IRUGO, show_fan_min,
>> +			  set_fan_min, 1);
>> +static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO, show_fan_alarm, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO, show_fan_alarm, NULL, 1);
>> +static SENSOR_DEVICE_ATTR(fan1_pulses, S_IWUSR | S_IRUGO, show_fan_pulses,
>> +			  set_fan_pulses, 0);
>> +static SENSOR_DEVICE_ATTR(fan2_pulses, S_IWUSR | S_IRUGO, show_fan_pulses,
>> +			  set_fan_pulses, 1);
>> +static SENSOR_DEVICE_ATTR(pwm1_mode, S_IWUSR | S_IRUGO,
>> +			  show_pwm_mode, set_pwm_mode, 0);
>> +static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm,
>> +			  set_pwm, 0);
>> +
>> +/* Driver data */
>> +static struct attribute *tc654_attrs[] = {
>> +	&sensor_dev_attr_fan1_input.dev_attr.attr,
>> +	&sensor_dev_attr_fan2_input.dev_attr.attr,
>> +	&sensor_dev_attr_fan1_min.dev_attr.attr,
>> +	&sensor_dev_attr_fan2_min.dev_attr.attr,
>> +	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_fan1_pulses.dev_attr.attr,
>> +	&sensor_dev_attr_fan2_pulses.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1_mode.dev_attr.attr,
>> +	&sensor_dev_attr_pwm1.dev_attr.attr,
>> +	NULL
>> +};
>> +
>> +ATTRIBUTE_GROUPS(tc654);
>> +
>> +/*
>> + * device probe and removal
>> + */
>> +
>> +static int tc654_probe(struct i2c_client *client,
>> +		       const struct i2c_device_id *id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct tc654_data *data;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>> +		return -ENODEV;
>> +
>> +	data = devm_kzalloc(dev, sizeof(struct tc654_data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->client = client;
>> +	i2c_set_clientdata(client, data);
>
> No longer needed.

Done.

>
>> +	mutex_init(&data->update_lock);
>> +
>> +	data->hwmon_dev =
>> +	    devm_hwmon_device_register_with_groups(dev, client->name, data,
>> +						   tc654_groups);
>> +	if (IS_ERR(data->hwmon_dev))
>> +		return PTR_ERR(data->hwmon_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id tc654_id[] = {
>> +	{"tc654", 0},
>> +	{"tc655", 0},
>> +	{}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, tc654_id);
>> +
>> +static struct i2c_driver tc654_driver = {
>> +	.driver = {
>> +		   .name = "tc654",
>> +		   .owner = THIS_MODULE,
>
> Not needed (see Julia's patch)
>

Will incorporate those changes too.

>> +		   },
>> +	.probe = tc654_probe,
>> +	.id_table = tc654_id,
>> +};
>> +
>> +module_i2c_driver(tc654_driver);
>> +
>> +MODULE_AUTHOR("Allied Telesis Labs");
>> +MODULE_DESCRIPTION("Microchip TC654/TC655 driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.10.0.479.g7c56b16
>>
>

--
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
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index 1416c6a0d2cd..833fb9f133d3 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -122,6 +122,8 @@  microchip,mcp4662-502	Microchip 8-bit Dual I2C Digital Potentiometer with NV Mem
 microchip,mcp4662-103	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (10k)
 microchip,mcp4662-503	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (50k)
 microchip,mcp4662-104	Microchip 8-bit Dual I2C Digital Potentiometer with NV Memory (100k)
+microchip,tc654		PWM Fan Speed Controller With Fan Fault Detection
+microchip,tc655		PWM Fan Speed Controller With Fan Fault Detection
 national,lm63		Temperature sensor with integrated fan control
 national,lm75		I2C TEMP SENSOR
 national,lm80		Serial Interface ACPI-Compatible Microprocessor System Hardware Monitor
diff --git a/Documentation/hwmon/tc654 b/Documentation/hwmon/tc654
new file mode 100644
index 000000000000..93796c5c7e79
--- /dev/null
+++ b/Documentation/hwmon/tc654
@@ -0,0 +1,26 @@ 
+Kernel driver tc654
+===================
+
+Supported chips:
+  * Microship TC654 and TC655
+    Prefix: 'tc654'
+    Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/20001734C.pdf
+
+Authors:
+        Chris Packham <chris.packham@alliedtelesis.co.nz>
+        Masahiko Iwamoto <iwamoto@allied-telesis.co.jp>
+
+Description
+-----------
+This driver implements support for the Microchip TC654 and TC655.
+
+The TC654 used the 2-wire interface compatible with the SMBUS 2.0
+specification. The TC654 has two (2) inputs for measuring fan RPM and
+one (1) PWM output which can be used for fan control.
+
+Configuration Notes
+-------------------
+Ordinarily the pwm1_mode ABI is used for controlling the pwm output
+mode.  However, for this chip the output is always pwm, and the
+pwm1_mode determines if the pwm output is controlled via the pwm1 value
+or via the Vin analog input.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 45cef3d2c75c..8681bc65cde5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -907,6 +907,17 @@  config SENSORS_MCP3021
 	  This driver can also be built as a module.  If so, the module
 	  will be called mcp3021.
 
+config SENSORS_TC654
+	tristate "Microchip TC654/TC655 and compatibles"
+	depends on I2C
+	help
+	  If you say yes here you get support for TC654 and TC655.
+	  The TC654 and TC655 are PWM mode fan speed controllers with
+	  FanSense technology for use with brushless DC fans.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called tc654.
+
 config SENSORS_MENF21BMC_HWMON
 	tristate "MEN 14F021P00 BMC Hardware Monitoring"
 	depends on MFD_MENF21BMC
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index aecf4ba17460..c651f0f1d047 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -122,6 +122,7 @@  obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
 obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
 obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
+obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
 obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
 obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
 obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c
new file mode 100644
index 000000000000..cba31cbd3383
--- /dev/null
+++ b/drivers/hwmon/tc654.c
@@ -0,0 +1,513 @@ 
+/*
+ * tc654.c - Linux kernel modules for fan speed controller
+ *
+ * Copyright (C) 2016 Allied Telesis Labs NZ
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/jiffies.h>
+#include <linux/util_macros.h>
+
+enum tc654_regs {
+	TC654_REG_RPM1 = 0x00,	/* RPM Output 1 */
+	TC654_REG_RPM2 = 0x01,	/* RPM Output 2 */
+	TC654_REG_FAN_FAULT1 = 0x02,	/* Fan Fault 1 Threshold */
+	TC654_REG_FAN_FAULT2 = 0x03,	/* Fan Fault 2 Threshold */
+	TC654_REG_CONFIG = 0x04,	/* Configuration */
+	TC654_REG_STATUS = 0x05,	/* Status */
+	TC654_REG_DUTY_CYCLE = 0x06,	/* Fan Speed Duty Cycle */
+	TC654_REG_MFR_ID = 0x07,	/* Manufacturer Identification */
+	TC654_REG_VER_ID = 0x08,	/* Version Identification */
+};
+
+/* Macros to easily index the registers */
+#define TC654_REG_RPM(idx) (TC654_REG_RPM1 + (idx))
+#define TC654_REG_FAN_FAULT(idx) (TC654_REG_FAN_FAULT1 + (idx))
+
+/* Config register bits */
+#define TC654_REG_CONFIG_RES 0x40	/* Resolution Selection */
+#define TC654_REG_CONFIG_DUTYC 0x20	/* Duty Cycle Control Method */
+#define TC654_REG_CONFIG_SDM 0x01	/* Shutdown Mode */
+
+/* Status register bits */
+#define TC654_REG_STATUS_F2F 0x02	/* Fan 2 Fault */
+#define TC654_REG_STATUS_F1F 0x01	/* Fan 1 Fault */
+
+/* RPM resolution for RPM Output registers */
+#define TC654_HIGH_RPM_RESOLUTION 25	/* 25 RPM resolution */
+#define TC654_LOW_RPM_RESOLUTION 50	/* 50 RPM resolution */
+
+/* Convert to the fan fault RPM threshold from register value */
+#define TC654_FAN_FAULT_FROM_REG(val) ((val) * 50)	/* 50 RPM resolution */
+
+/* Convert to register value from the fan fault RPM threshold */
+#define TC654_FAN_FAULT_TO_REG(val) (((val) / 50) & 0xff)
+
+/* Register data is read (and cached) at most once per second. */
+#define TC654_UPDATE_INTERVAL	HZ
+
+struct tc654_data {
+	struct i2c_client *client;
+	struct device *hwmon_dev;
+
+	/* update mutex */
+	struct mutex update_lock;
+
+	/* tc654 register cache */
+	bool valid;
+	unsigned long last_updated;	/* in jiffies */
+
+	u8 rpm_output[2];	/* The fan RPM data for fans 1 and 2 is then
+				 * written to registers RPM1 and RPM2
+				 */
+	u8 fan_fault[2];	/* The Fan Fault Threshold Registers are used to
+				 * set the fan fault threshold levels for fan 1
+				 * and fan 2
+				 */
+	u8 config;	/* The Configuration Register is an 8-bit read/
+			 * writable multi-function control register
+			 *   7: Fan Fault Clear
+			 *      1 = Clear Fan Fault
+			 *      0 = Normal Operation (default)
+			 *   6: Resolution Selection for RPM Output Registers
+			 *      RPM Output Registers (RPM1 and RPM2) will be
+			 *      set for
+			 *      1 = 25 RPM (9-bit) resolution
+			 *      0 = 50 RPM (8-bit) resolution (default)
+			 *   5: Duty Cycle Control Method
+			 *      The V OUT duty cycle will be controlled via
+			 *      1 = the SMBus interface.
+			 *      0 = via the V IN analog input pin. (default)
+			 * 4,3: Fan 2 Pulses Per Rotation
+			 *      00 = 1
+			 *      01 = 2 (default)
+			 *      10 = 4
+			 *      11 = 8
+			 * 2,1: Fan 1 Pulses Per Rotation
+			 *      00 = 1
+			 *      01 = 2 (default)
+			 *      10 = 4
+			 *      11 = 8
+			 *   0: Shutdown Mode
+			 *      1 = Shutdown mode.
+			 *      0 = Normal operation. (default)
+			 */
+	u8 status;	/* The Status register provides all the information
+			 * about what is going on within the TC654/TC655
+			 * devices.
+			 * 7,6: Unimplemented, Read as '0'
+			 *   5: Over-Temperature Fault Condition
+			 *      1 = Over-Temperature condition has occurred
+			 *      0 = Normal operation. V IN is less than 2.6V
+			 *   4: RPM2 Counter Overflow
+			 *      1 = Fault condition
+			 *      0 = Normal operation
+			 *   3: RPM1 Counter Overflow
+			 *      1 = Fault condition
+			 *      0 = Normal operation
+			 *   2: V IN Input Status
+			 *      1 = V IN is open
+			 *      0 = Normal operation. voltage present at V IN
+			 *   1: Fan 2 Fault
+			 *      1 = Fault condition
+			 *      0 = Normal operation
+			 *   0: Fan 1 Fault
+			 *      1 = Fault condition
+			 *      0 = Normal operation
+			 */
+	u8 duty_cycle;	/* The DUTY_CYCLE register is a 4-bit read/
+			 * writable register used to control the duty
+			 * cycle of the V OUT output.
+			 */
+};
+
+/* helper to grab and cache data, at most one time per second */
+static struct tc654_data *tc654_update_client(struct device *dev)
+{
+	struct tc654_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret = 0;
+
+	mutex_lock(&data->update_lock);
+	if (time_before(jiffies, data->last_updated + TC654_UPDATE_INTERVAL) &&
+	    likely(data->valid))
+		goto out;
+
+	ret = i2c_smbus_read_byte_data(client, TC654_REG_RPM(0));
+	if (ret < 0)
+		goto out;
+	data->rpm_output[0] = ret;
+
+	ret = i2c_smbus_read_byte_data(client, TC654_REG_RPM(1));
+	if (ret < 0)
+		goto out;
+	data->rpm_output[1] = ret;
+
+	ret = i2c_smbus_read_byte_data(client, TC654_REG_FAN_FAULT(0));
+	if (ret < 0)
+		goto out;
+	data->fan_fault[0] = ret;
+
+	ret = i2c_smbus_read_byte_data(client, TC654_REG_FAN_FAULT(1));
+	if (ret < 0)
+		goto out;
+	data->fan_fault[1] = ret;
+
+	ret = i2c_smbus_read_byte_data(client, TC654_REG_CONFIG);
+	if (ret < 0)
+		goto out;
+	data->config = ret;
+
+	ret = i2c_smbus_read_byte_data(client, TC654_REG_STATUS);
+	if (ret < 0)
+		goto out;
+	data->status = ret;
+
+	ret = i2c_smbus_read_byte_data(client, TC654_REG_DUTY_CYCLE);
+	if (ret < 0)
+		goto out;
+	data->duty_cycle = ret;
+
+	data->last_updated = jiffies;
+	data->valid = true;
+out:
+	mutex_unlock(&data->update_lock);
+
+	if (ret < 0)		/* upon error, encode it in return value */
+		data = ERR_PTR(ret);
+
+	return data;
+}
+
+/*
+ * sysfs attributes
+ */
+
+static ssize_t show_fan(struct device *dev, struct device_attribute *da,
+			char *buf)
+{
+	int nr = to_sensor_dev_attr(da)->index;
+	struct tc654_data *data = tc654_update_client(dev);
+	int val;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (data->config & TC654_REG_CONFIG_RES)
+		val = data->rpm_output[nr] * TC654_HIGH_RPM_RESOLUTION;
+	else
+		val = data->rpm_output[nr] * TC654_LOW_RPM_RESOLUTION;
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t show_fan_min(struct device *dev, struct device_attribute *da,
+			    char *buf)
+{
+	int nr = to_sensor_dev_attr(da)->index;
+	struct tc654_data *data = tc654_update_client(dev);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	return sprintf(buf, "%d\n",
+		       TC654_FAN_FAULT_FROM_REG(data->fan_fault[nr]));
+}
+
+static ssize_t set_fan_min(struct device *dev, struct device_attribute *da,
+			   const char *buf, size_t count)
+{
+	int nr = to_sensor_dev_attr(da)->index;
+	struct tc654_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val;
+	int ret;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	clamp_val(val, 0, 12750);
+
+	mutex_lock(&data->update_lock);
+
+	data->fan_fault[nr] = TC654_FAN_FAULT_TO_REG(val);
+	ret = i2c_smbus_write_byte_data(client, TC654_REG_FAN_FAULT(nr),
+				  data->fan_fault[nr]);
+
+	mutex_unlock(&data->update_lock);
+	return ret < 0 ? ret : count;
+}
+
+static ssize_t show_fan_alarm(struct device *dev, struct device_attribute *da,
+			      char *buf)
+{
+	int nr = to_sensor_dev_attr(da)->index;
+	struct tc654_data *data = tc654_update_client(dev);
+	int val;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (nr == 0)
+		val = !!(data->status & TC654_REG_STATUS_F1F);
+	else
+		val = !!(data->status & TC654_REG_STATUS_F2F);
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static const u8 TC654_FAN_PULSE_SHIFT[] = { 1, 3 };
+
+static ssize_t show_fan_pulses(struct device *dev, struct device_attribute *da,
+			      char *buf)
+{
+	int nr = to_sensor_dev_attr(da)->index;
+	struct tc654_data *data = tc654_update_client(dev);
+	u8 val;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	val = BIT((data->config >> TC654_FAN_PULSE_SHIFT[nr]) & 0x03);
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t set_fan_pulses(struct device *dev, struct device_attribute *da,
+			     const char *buf, size_t count)
+{
+	int nr = to_sensor_dev_attr(da)->index;
+	struct tc654_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	u8 config;
+	unsigned long val;
+	int ret;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	switch (val) {
+	case 1:
+		config = 0;
+		break;
+	case 2:
+		config = 1;
+		break;
+	case 4:
+		config = 2;
+		break;
+	case 8:
+		config = 3;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->update_lock);
+
+	data->config &= ~(0x03 << TC654_FAN_PULSE_SHIFT[nr]);
+	data->config |= (config << TC654_FAN_PULSE_SHIFT[nr]);
+	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
+
+	mutex_unlock(&data->update_lock);
+	return ret < 0 ? ret : count;
+}
+
+static ssize_t show_pwm_mode(struct device *dev,
+			     struct device_attribute *da, char *buf)
+{
+	struct tc654_data *data = tc654_update_client(dev);
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	return sprintf(buf, "%d\n", data->config & TC654_REG_CONFIG_DUTYC);
+}
+
+static ssize_t set_pwm_mode(struct device *dev,
+			    struct device_attribute *da,
+			    const char *buf, size_t count)
+{
+	struct tc654_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val;
+	int ret;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	if (val)
+		data->config |= TC654_REG_CONFIG_DUTYC;
+	else
+		data->config &= ~TC654_REG_CONFIG_DUTYC;
+
+	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
+
+	mutex_unlock(&data->update_lock);
+	return ret < 0 ? ret : count;
+}
+
+static const int tc654_pwm_map[16] = { 76, 88, 100, 112, 124, 141, 147, 171,
+		183, 195, 207, 219, 231, 243, 255 };
+
+static ssize_t show_pwm(struct device *dev, struct device_attribute *da,
+			       char *buf)
+{
+	struct tc654_data *data = tc654_update_client(dev);
+	int pwm;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	if (data->config & TC654_REG_CONFIG_SDM)
+		pwm = 0;
+	else
+		pwm = tc654_pwm_map[data->duty_cycle];
+
+	return sprintf(buf, "%d\n", pwm);
+}
+
+static ssize_t set_pwm(struct device *dev, struct device_attribute *da,
+			      const char *buf, size_t count)
+{
+	struct tc654_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val;
+	int ret;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val > 255)
+		return -EINVAL;
+
+	if (val == 0)
+		data->config |= TC654_REG_CONFIG_SDM;
+	else
+		data->config &= ~TC654_REG_CONFIG_SDM;
+
+	data->duty_cycle = find_closest(val, tc654_pwm_map,
+					ARRAY_SIZE(tc654_pwm_map));
+
+	mutex_lock(&data->update_lock);
+
+	ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
+	if (ret < 0)
+		goto out;
+
+	ret = i2c_smbus_write_byte_data(client, TC654_REG_DUTY_CYCLE,
+				  data->duty_cycle);
+	if (ret < 0)
+		goto out;
+
+out:
+	mutex_unlock(&data->update_lock);
+	return ret < 0 ? ret : count;
+}
+
+static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_fan, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_input, S_IRUGO, show_fan, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan1_min, S_IWUSR | S_IRUGO, show_fan_min,
+			  set_fan_min, 0);
+static SENSOR_DEVICE_ATTR(fan2_min, S_IWUSR | S_IRUGO, show_fan_min,
+			  set_fan_min, 1);
+static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO, show_fan_alarm, NULL, 0);
+static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO, show_fan_alarm, NULL, 1);
+static SENSOR_DEVICE_ATTR(fan1_pulses, S_IWUSR | S_IRUGO, show_fan_pulses,
+			  set_fan_pulses, 0);
+static SENSOR_DEVICE_ATTR(fan2_pulses, S_IWUSR | S_IRUGO, show_fan_pulses,
+			  set_fan_pulses, 1);
+static SENSOR_DEVICE_ATTR(pwm1_mode, S_IWUSR | S_IRUGO,
+			  show_pwm_mode, set_pwm_mode, 0);
+static SENSOR_DEVICE_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm,
+			  set_pwm, 0);
+
+/* Driver data */
+static struct attribute *tc654_attrs[] = {
+	&sensor_dev_attr_fan1_input.dev_attr.attr,
+	&sensor_dev_attr_fan2_input.dev_attr.attr,
+	&sensor_dev_attr_fan1_min.dev_attr.attr,
+	&sensor_dev_attr_fan2_min.dev_attr.attr,
+	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
+	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
+	&sensor_dev_attr_fan1_pulses.dev_attr.attr,
+	&sensor_dev_attr_fan2_pulses.dev_attr.attr,
+	&sensor_dev_attr_pwm1_mode.dev_attr.attr,
+	&sensor_dev_attr_pwm1.dev_attr.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(tc654);
+
+/*
+ * device probe and removal
+ */
+
+static int tc654_probe(struct i2c_client *client,
+		       const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct tc654_data *data;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	data = devm_kzalloc(dev, sizeof(struct tc654_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	i2c_set_clientdata(client, data);
+	mutex_init(&data->update_lock);
+
+	data->hwmon_dev =
+	    devm_hwmon_device_register_with_groups(dev, client->name, data,
+						   tc654_groups);
+	if (IS_ERR(data->hwmon_dev))
+		return PTR_ERR(data->hwmon_dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id tc654_id[] = {
+	{"tc654", 0},
+	{"tc655", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, tc654_id);
+
+static struct i2c_driver tc654_driver = {
+	.driver = {
+		   .name = "tc654",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = tc654_probe,
+	.id_table = tc654_id,
+};
+
+module_i2c_driver(tc654_driver);
+
+MODULE_AUTHOR("Allied Telesis Labs");
+MODULE_DESCRIPTION("Microchip TC654/TC655 driver");
+MODULE_LICENSE("GPL");