* Re: [PATCHv1] hwmon: Add tc654 driver
@ 2016-10-06 22:57 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2016-10-06 22:57 UTC (permalink / raw)
To: Chris Packham
Cc: linux-hwmon, iwamoto, Joshua.Scott, Kevin Tsai, Wolfram Sang,
Rob Herring, Mark Rutland, Jean Delvare, linux-i2c, devicetree,
linux-kernel
On Fri, Oct 07, 2016 at 10:36:47AM +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>
> ---
>
> Hi Gunter,
>
> I realise this isn't using the new hwmon registration API. This is
> essentially a forward port from an older kernel. I can attempt a
> conversion as a follow up patch but I loose the ability to actually test
> the driver.
>
No problem.
Can you send me a register dump for this chip ?
> .../devicetree/bindings/i2c/trivial-devices.txt | 2 +
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tc654.c | 532 +++++++++++++++++++++
Please also add Documentation/hwmon/tc654.
> 4 files changed, 546 insertions(+)
> 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/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..31e5f065183f
> --- /dev/null
> +++ b/drivers/hwmon/tc654.c
> @@ -0,0 +1,532 @@
> +/*
> + * 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_FFCLR 0x80 /* Fan Fault Clear */
> +#define TC654_REG_CONFIG_RES 0x40 /* Resolution Selection */
> +#define TC654_REG_CONFIG_DUTYC 0x20 /* Duty Cycle Control Method */
> +#define TC654_REG_CONFIG_F2PPR 0x18 /* Fan 2 Pulses Per Rotation */
> +#define TC654_REG_CONFIG_F1PPR 0x06 /* Fan 1 Pulses Per Rotation */
> +#define TC654_REG_CONFIG_SDM 0x01 /* Shutdown Mode */
> +
> +/* Status register bits */
> +#define TC654_REG_STATUS_OTF 0x20 /* Over-Temperature Fault Condition */
> +#define TC654_REG_STATUS_R2CO 0x10 /* RPM2 Counter Overflow */
> +#define TC654_REG_STATUS_R1CO 0x08 /* RPM1 Counter Overflow */
> +#define TC654_REG_STATUS_VSTAT 0x04 /* V IN Input Status */
> +#define TC654_REG_STATUS_F2F 0x02 /* Fan 2 Fault */
> +#define TC654_REG_STATUS_F1F 0x01 /* Fan 1 Fault */
> +
Many of those defines are not used. Please drop the unused ones
(or use them).
> +/* 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
> +
Another possibility would be to use regmap and let it cache the
static registers.
> +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;
> + long val;
> +
> + if (kstrtol(buf, 10, &val))
> + return -EINVAL;
kstrtoul ?
> + if (val < 0 || val > 255)
It would be better to use clamp_val() here.
Is that really the speed in rpm (0 .. 255) ? Seems unlikely.
Looking into the code, the limit should probably be 255 * 50.
> + return -EINVAL;
> +
> + mutex_lock(&data->update_lock);
> +
> + data->fan_fault[nr] = TC654_FAN_FAULT_TO_REG(val);
> + i2c_smbus_write_byte_data(client, TC654_REG_FAN_FAULT(nr),
> + data->fan_fault[nr]);
> +
Error check (here and elsewhere) ?
> + mutex_unlock(&data->update_lock);
> + return 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) ? 1 : 0;
val = !!(data->status & TC654_REG_STATUS_F1F);
> + else
> + val = (data->status & TC654_REG_STATUS_F2F) ? 1 : 0;
> +
> + 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 = (1 << ((data->config >> TC654_FAN_PULSE_SHIFT[nr]) & 0x03));
Extra ( ). Also, you could use BIT() here.
> + 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;
> + long val;
> +
> + if (kstrtol(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]);
> + i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +
> + mutex_unlock(&data->update_lock);
> + return count;
> +}
> +
> +static ssize_t show_pwm_mode(struct device *dev,
> + struct device_attribute *da, char *buf)
Please align continuation lines with '('.
> +{
> + 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;
> + long val;
> +
> + if (kstrtol(buf, 10, &val))
kstrtoul ?
> + return -EINVAL;
> +
> + if (val != 0 && val != 1)
> +
The error return is missing here.
> + mutex_lock(&data->update_lock);
> +
> + if (val)
> + data->config |= TC654_REG_CONFIG_DUTYC;
> + else
> + data->config &= ~TC654_REG_CONFIG_DUTYC;
> +
This is different to the intended ABI. The ABI expects the _output_ to be
configured with this attribute. However, for this chip the output is always
pwm, and the bit determines if the pwm output is controlled via register
or via the Vin analog input.
It is ok with me, but it should be documented.
> + i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +
> + mutex_unlock(&data->update_lock);
> + return 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;
> + long val;
> +
> + if (kstrtol(buf, 10, &val))
kstrtoul ?
> + return -EINVAL;
> +
> + val = clamp_val(val, 0, 255);
Please use strict value checking here. pwm value range is well defined.
> +
> + 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);
> +
> + i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +
> + i2c_smbus_write_byte_data(client, TC654_REG_DUTY_CYCLE,
> + data->duty_cycle);
> +
> + mutex_unlock(&data->update_lock);
> + return 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 =
> + hwmon_device_register_with_groups(dev, client->name, data,
> + tc654_groups);
Please use the devm_ function (and you can drop the remove function).
> + if (IS_ERR(data->hwmon_dev))
> + return PTR_ERR(data->hwmon_dev);
> +
> + dev_info(dev, "%s: sensor '%s'\n",
> + dev_name(data->hwmon_dev), client->name);
dev_info() already displays the device name.
> +
> + return 0;
> +}
> +
> +static int tc654_remove(struct i2c_client *client)
> +{
> + struct tc654_data *data = i2c_get_clientdata(client);
> +
> + hwmon_device_unregister(data->hwmon_dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id tc654_dt_match[] = {
> + {.compatible = "microchip,tc654"},
> + {.compatible = "microchip,tc655"},
> + {},
> +};
> +#endif
> +
Not needed.
> +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,
> + .of_match_table = of_match_ptr(tc654_dt_match),
Not needed.
> + },
> + .probe = tc654_probe,
> + .remove = tc654_remove,
> + .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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv1] hwmon: Add tc654 driver
@ 2016-10-06 22:57 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2016-10-06 22:57 UTC (permalink / raw)
To: Chris Packham
Cc: linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
iwamoto-sK/J6oeM9AhgKrQr38906+qrae++aQT8,
Joshua.Scott-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu, Kevin Tsai,
Wolfram Sang, Rob Herring, Mark Rutland, Jean Delvare,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Fri, Oct 07, 2016 at 10:36:47AM +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-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org>
> ---
>
> Hi Gunter,
>
> I realise this isn't using the new hwmon registration API. This is
> essentially a forward port from an older kernel. I can attempt a
> conversion as a follow up patch but I loose the ability to actually test
> the driver.
>
No problem.
Can you send me a register dump for this chip ?
> .../devicetree/bindings/i2c/trivial-devices.txt | 2 +
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tc654.c | 532 +++++++++++++++++++++
Please also add Documentation/hwmon/tc654.
> 4 files changed, 546 insertions(+)
> 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/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..31e5f065183f
> --- /dev/null
> +++ b/drivers/hwmon/tc654.c
> @@ -0,0 +1,532 @@
> +/*
> + * 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_FFCLR 0x80 /* Fan Fault Clear */
> +#define TC654_REG_CONFIG_RES 0x40 /* Resolution Selection */
> +#define TC654_REG_CONFIG_DUTYC 0x20 /* Duty Cycle Control Method */
> +#define TC654_REG_CONFIG_F2PPR 0x18 /* Fan 2 Pulses Per Rotation */
> +#define TC654_REG_CONFIG_F1PPR 0x06 /* Fan 1 Pulses Per Rotation */
> +#define TC654_REG_CONFIG_SDM 0x01 /* Shutdown Mode */
> +
> +/* Status register bits */
> +#define TC654_REG_STATUS_OTF 0x20 /* Over-Temperature Fault Condition */
> +#define TC654_REG_STATUS_R2CO 0x10 /* RPM2 Counter Overflow */
> +#define TC654_REG_STATUS_R1CO 0x08 /* RPM1 Counter Overflow */
> +#define TC654_REG_STATUS_VSTAT 0x04 /* V IN Input Status */
> +#define TC654_REG_STATUS_F2F 0x02 /* Fan 2 Fault */
> +#define TC654_REG_STATUS_F1F 0x01 /* Fan 1 Fault */
> +
Many of those defines are not used. Please drop the unused ones
(or use them).
> +/* 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
> +
Another possibility would be to use regmap and let it cache the
static registers.
> +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;
> + long val;
> +
> + if (kstrtol(buf, 10, &val))
> + return -EINVAL;
kstrtoul ?
> + if (val < 0 || val > 255)
It would be better to use clamp_val() here.
Is that really the speed in rpm (0 .. 255) ? Seems unlikely.
Looking into the code, the limit should probably be 255 * 50.
> + return -EINVAL;
> +
> + mutex_lock(&data->update_lock);
> +
> + data->fan_fault[nr] = TC654_FAN_FAULT_TO_REG(val);
> + i2c_smbus_write_byte_data(client, TC654_REG_FAN_FAULT(nr),
> + data->fan_fault[nr]);
> +
Error check (here and elsewhere) ?
> + mutex_unlock(&data->update_lock);
> + return 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) ? 1 : 0;
val = !!(data->status & TC654_REG_STATUS_F1F);
> + else
> + val = (data->status & TC654_REG_STATUS_F2F) ? 1 : 0;
> +
> + 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 = (1 << ((data->config >> TC654_FAN_PULSE_SHIFT[nr]) & 0x03));
Extra ( ). Also, you could use BIT() here.
> + 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;
> + long val;
> +
> + if (kstrtol(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]);
> + i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +
> + mutex_unlock(&data->update_lock);
> + return count;
> +}
> +
> +static ssize_t show_pwm_mode(struct device *dev,
> + struct device_attribute *da, char *buf)
Please align continuation lines with '('.
> +{
> + 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;
> + long val;
> +
> + if (kstrtol(buf, 10, &val))
kstrtoul ?
> + return -EINVAL;
> +
> + if (val != 0 && val != 1)
> +
The error return is missing here.
> + mutex_lock(&data->update_lock);
> +
> + if (val)
> + data->config |= TC654_REG_CONFIG_DUTYC;
> + else
> + data->config &= ~TC654_REG_CONFIG_DUTYC;
> +
This is different to the intended ABI. The ABI expects the _output_ to be
configured with this attribute. However, for this chip the output is always
pwm, and the bit determines if the pwm output is controlled via register
or via the Vin analog input.
It is ok with me, but it should be documented.
> + i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +
> + mutex_unlock(&data->update_lock);
> + return 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;
> + long val;
> +
> + if (kstrtol(buf, 10, &val))
kstrtoul ?
> + return -EINVAL;
> +
> + val = clamp_val(val, 0, 255);
Please use strict value checking here. pwm value range is well defined.
> +
> + 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);
> +
> + i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config);
> +
> + i2c_smbus_write_byte_data(client, TC654_REG_DUTY_CYCLE,
> + data->duty_cycle);
> +
> + mutex_unlock(&data->update_lock);
> + return 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 =
> + hwmon_device_register_with_groups(dev, client->name, data,
> + tc654_groups);
Please use the devm_ function (and you can drop the remove function).
> + if (IS_ERR(data->hwmon_dev))
> + return PTR_ERR(data->hwmon_dev);
> +
> + dev_info(dev, "%s: sensor '%s'\n",
> + dev_name(data->hwmon_dev), client->name);
dev_info() already displays the device name.
> +
> + return 0;
> +}
> +
> +static int tc654_remove(struct i2c_client *client)
> +{
> + struct tc654_data *data = i2c_get_clientdata(client);
> +
> + hwmon_device_unregister(data->hwmon_dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id tc654_dt_match[] = {
> + {.compatible = "microchip,tc654"},
> + {.compatible = "microchip,tc655"},
> + {},
> +};
> +#endif
> +
Not needed.
> +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,
> + .of_match_table = of_match_ptr(tc654_dt_match),
Not needed.
> + },
> + .probe = tc654_probe,
> + .remove = tc654_remove,
> + .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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv1] hwmon: Add tc654 driver
2016-10-06 22:57 ` Guenter Roeck
(?)
@ 2016-10-06 23:13 ` Chris Packham
-1 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2016-10-06 23:13 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-hwmon, Masahiko Iwamoto, Joshua Scott, Kevin Tsai,
Wolfram Sang, Rob Herring, Mark Rutland, Jean Delvare, linux-i2c,
devicetree, linux-kernel
On 10/07/2016 11:57 AM, Guenter Roeck wrote:
> On Fri, Oct 07, 2016 at 10:36:47AM +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>
>> ---
>>
>> Hi Gunter,
>>
>> I realise this isn't using the new hwmon registration API. This is
>> essentially a forward port from an older kernel. I can attempt a
>> conversion as a follow up patch but I loose the ability to actually test
>> the driver.
>>
> No problem.
>
> Can you send me a register dump for this chip ?
Here you go
[root@awplus /flash]# i2cdump /dev/i2c-0 0x1b
Dump of device 0x1b on /dev/i2c-0
0: 76 00 0a 0a 2a 02 00 54 00 b3 31 01 ff ff ff 00 v...*..T..1.....
I'll read through the rest of your comments and reply with a v2 later.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCHv2] hwmon: Add tc654 driver
2016-10-06 22:57 ` Guenter Roeck
(?)
(?)
@ 2016-10-07 1:38 ` Chris Packham
2016-10-07 18:29 ` Guenter Roeck
-1 siblings, 1 reply; 16+ messages in thread
From: Chris Packham @ 2016-10-07 1:38 UTC (permalink / raw)
To: linux, linux-hwmon
Cc: iwamoto, Joshua.Scott, Chris Packham, Kevin Tsai, Wolfram Sang,
Rob Herring, Mark Rutland, Jean Delvare, Jonathan Corbet,
linux-i2c, devicetree, linux-kernel, linux-doc
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
+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");
--
2.10.0.479.g7c56b16
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCHv2] hwmon: Add tc654 driver
2016-10-07 1:38 ` [PATCHv2] " Chris Packham
@ 2016-10-07 18:29 ` Guenter Roeck
2016-10-09 21:21 ` Chris Packham
2016-10-09 22:12 ` [PATCHv3] " Chris Packham
0 siblings, 2 replies; 16+ messages in thread
From: Guenter Roeck @ 2016-10-07 18:29 UTC (permalink / raw)
To: Chris Packham
Cc: linux-hwmon, iwamoto, Joshua.Scott, Kevin Tsai, Wolfram Sang,
Rob Herring, Mark Rutland, Jean Delvare, Jonathan Corbet,
linux-i2c, devicetree, linux-kernel, linux-doc
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv2] hwmon: Add tc654 driver
2016-10-07 18:29 ` Guenter Roeck
@ 2016-10-09 21:21 ` Chris Packham
2016-10-09 22:12 ` [PATCHv3] " Chris Packham
1 sibling, 0 replies; 16+ messages in thread
From: Chris Packham @ 2016-10-09 21:21 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-hwmon, Masahiko Iwamoto, Joshua Scott, Kevin Tsai,
Wolfram Sang, Rob Herring, Mark Rutland, Jean Delvare,
Jonathan Corbet, linux-i2c, devicetree, linux-kernel, linux-doc
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
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCHv3] hwmon: Add tc654 driver
2016-10-07 18:29 ` Guenter Roeck
2016-10-09 21:21 ` Chris Packham
@ 2016-10-09 22:12 ` Chris Packham
2016-10-10 13:21 ` Guenter Roeck
1 sibling, 1 reply; 16+ messages in thread
From: Chris Packham @ 2016-10-09 22:12 UTC (permalink / raw)
To: linux, linux-hwmon
Cc: iwamoto, Joshua.Scott, Chris Packham, Kevin Tsai, Wolfram Sang,
Rob Herring, Mark Rutland, Jean Delvare, Jonathan Corbet,
linux-i2c, devicetree, linux-kernel, linux-doc
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 v3:
- typofix in documentation
- add missing value to tc654_pwm_map, re-generate based on datasheet.
- remove unnecessary hwmon_dev member from struct tc654_data
- bug fixes in set_fan_min() and show_pwm_mode()
- miscellaneous style fixes
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 | 31 ++
drivers/hwmon/Kconfig | 11 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/tc654.c | 509 +++++++++++++++++++++
5 files changed, 554 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..91a2843f5f98
--- /dev/null
+++ b/Documentation/hwmon/tc654
@@ -0,0 +1,31 @@
+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 uses 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.
+
+
+Setting pwm1_mode to 1 will cause the pwm output to be driven based on
+the pwm1 value. Setting pwm1_mode to 0 will cause the pwm output to be
+driven based on the Vin 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..456e0bb9f94f
--- /dev/null
+++ b/drivers/hwmon/tc654.c
@@ -0,0 +1,509 @@
+/*
+ * 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/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/mutex.h>
+#include <linux/slab.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 BIT(6) /* Resolution Selection */
+#define TC654_REG_CONFIG_DUTYC BIT(5) /* Duty Cycle Control Method */
+#define TC654_REG_CONFIG_SDM BIT(0) /* Shutdown Mode */
+
+/* Status register bits */
+#define TC654_REG_STATUS_F2F BIT(1) /* Fan 2 Fault */
+#define TC654_REG_STATUS_F1F BIT(0) /* 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;
+
+ /* 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 & 0x0f;
+
+ 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;
+
+ 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]);
+
+ 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] = { 77, 88, 102, 112, 124, 136, 148, 160,
+ 172, 184, 196, 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;
+ struct device *hwmon_dev;
+
+ 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;
+ mutex_init(&data->update_lock);
+
+ hwmon_dev =
+ devm_hwmon_device_register_with_groups(dev, client->name, data,
+ tc654_groups);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+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",
+ },
+ .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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCHv3] hwmon: Add tc654 driver
@ 2016-10-10 13:21 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2016-10-10 13:21 UTC (permalink / raw)
To: Chris Packham, linux-hwmon
Cc: iwamoto, Joshua.Scott, Kevin Tsai, Wolfram Sang, Rob Herring,
Mark Rutland, Jean Delvare, Jonathan Corbet, linux-i2c,
devicetree, linux-kernel, linux-doc
On 10/09/2016 03:12 PM, 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 v3:
> - typofix in documentation
> - add missing value to tc654_pwm_map, re-generate based on datasheet.
> - remove unnecessary hwmon_dev member from struct tc654_data
> - bug fixes in set_fan_min() and show_pwm_mode()
> - miscellaneous style fixes
>
> 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 | 31 ++
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tc654.c | 509 +++++++++++++++++++++
> 5 files changed, 554 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..91a2843f5f98
> --- /dev/null
> +++ b/Documentation/hwmon/tc654
> @@ -0,0 +1,31 @@
> +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 uses 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.
> +
> +
> +Setting pwm1_mode to 1 will cause the pwm output to be driven based on
> +the pwm1 value. Setting pwm1_mode to 0 will cause the pwm output to be
> +driven based on the Vin 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..456e0bb9f94f
> --- /dev/null
> +++ b/drivers/hwmon/tc654.c
> @@ -0,0 +1,509 @@
> +/*
> + * 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/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/mutex.h>
> +#include <linux/slab.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 BIT(6) /* Resolution Selection */
> +#define TC654_REG_CONFIG_DUTYC BIT(5) /* Duty Cycle Control Method */
> +#define TC654_REG_CONFIG_SDM BIT(0) /* Shutdown Mode */
> +
> +/* Status register bits */
> +#define TC654_REG_STATUS_F2F BIT(1) /* Fan 2 Fault */
> +#define TC654_REG_STATUS_F1F BIT(0) /* 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)
> +
Nitpick, but the above defines are difficult to read. Can you tab-align the values ?
> +/* Register data is read (and cached) at most once per second. */
> +#define TC654_UPDATE_INTERVAL HZ
> +
> +struct tc654_data {
> + struct i2c_client *client;
> +
> + /* 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 & 0x0f;
> +
> + 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;
> +
> + 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]);
> +
> + 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;
I just realized that this won't work as intended. Problem is that you
only fill data->config when reading an attribute. So, if a set function
is called prior to reading an attribute, data->config will be 0, and
you end up overwriting the original configuration.
> +
> + 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] = { 77, 88, 102, 112, 124, 136, 148, 160,
> + 172, 184, 196, 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);
> +
The lock has to be earlier, before writing duty_cycle.
> + 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;
This goto is not necessary.
> +
> +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;
> + struct device *hwmon_dev;
> +
> + 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;
> + mutex_init(&data->update_lock);
> +
> + hwmon_dev =
> + devm_hwmon_device_register_with_groups(dev, client->name, data,
> + tc654_groups);
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +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",
> + },
> + .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");
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] hwmon: Add tc654 driver
@ 2016-10-10 13:21 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2016-10-10 13:21 UTC (permalink / raw)
To: Chris Packham, linux-hwmon-u79uwXL29TY76Z2rM5mHXA
Cc: iwamoto-sK/J6oeM9AhgKrQr38906+qrae++aQT8,
Joshua.Scott-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu, Kevin Tsai,
Wolfram Sang, Rob Herring, Mark Rutland, Jean Delvare,
Jonathan Corbet, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
On 10/09/2016 03:12 PM, 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-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org>
> ---
> Changes in v3:
> - typofix in documentation
> - add missing value to tc654_pwm_map, re-generate based on datasheet.
> - remove unnecessary hwmon_dev member from struct tc654_data
> - bug fixes in set_fan_min() and show_pwm_mode()
> - miscellaneous style fixes
>
> 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 | 31 ++
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tc654.c | 509 +++++++++++++++++++++
> 5 files changed, 554 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..91a2843f5f98
> --- /dev/null
> +++ b/Documentation/hwmon/tc654
> @@ -0,0 +1,31 @@
> +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-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org>
> + Masahiko Iwamoto <iwamoto-sK/J6oeM9AhgKrQr38906+qrae++aQT8@public.gmane.org>
> +
> +Description
> +-----------
> +This driver implements support for the Microchip TC654 and TC655.
> +
> +The TC654 uses 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.
> +
> +
> +Setting pwm1_mode to 1 will cause the pwm output to be driven based on
> +the pwm1 value. Setting pwm1_mode to 0 will cause the pwm output to be
> +driven based on the Vin 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..456e0bb9f94f
> --- /dev/null
> +++ b/drivers/hwmon/tc654.c
> @@ -0,0 +1,509 @@
> +/*
> + * 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/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/mutex.h>
> +#include <linux/slab.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 BIT(6) /* Resolution Selection */
> +#define TC654_REG_CONFIG_DUTYC BIT(5) /* Duty Cycle Control Method */
> +#define TC654_REG_CONFIG_SDM BIT(0) /* Shutdown Mode */
> +
> +/* Status register bits */
> +#define TC654_REG_STATUS_F2F BIT(1) /* Fan 2 Fault */
> +#define TC654_REG_STATUS_F1F BIT(0) /* 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)
> +
Nitpick, but the above defines are difficult to read. Can you tab-align the values ?
> +/* Register data is read (and cached) at most once per second. */
> +#define TC654_UPDATE_INTERVAL HZ
> +
> +struct tc654_data {
> + struct i2c_client *client;
> +
> + /* 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 & 0x0f;
> +
> + 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;
> +
> + 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]);
> +
> + 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;
I just realized that this won't work as intended. Problem is that you
only fill data->config when reading an attribute. So, if a set function
is called prior to reading an attribute, data->config will be 0, and
you end up overwriting the original configuration.
> +
> + 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] = { 77, 88, 102, 112, 124, 136, 148, 160,
> + 172, 184, 196, 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);
> +
The lock has to be earlier, before writing duty_cycle.
> + 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;
This goto is not necessary.
> +
> +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;
> + struct device *hwmon_dev;
> +
> + 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;
> + mutex_init(&data->update_lock);
> +
> + hwmon_dev =
> + devm_hwmon_device_register_with_groups(dev, client->name, data,
> + tc654_groups);
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +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",
> + },
> + .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");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] hwmon: Add tc654 driver
2016-10-10 13:21 ` Guenter Roeck
(?)
@ 2016-10-10 20:08 ` Chris Packham
2016-10-10 20:59 ` Guenter Roeck
-1 siblings, 1 reply; 16+ messages in thread
From: Chris Packham @ 2016-10-10 20:08 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: Masahiko Iwamoto, Joshua Scott, Kevin Tsai, Wolfram Sang,
Rob Herring, Mark Rutland, Jean Delvare, Jonathan Corbet,
linux-i2c, devicetree, linux-kernel, linux-doc
On 10/11/2016 02:22 AM, Guenter Roeck wrote:
>> + if (val)
>> > + data->config |= TC654_REG_CONFIG_DUTYC;
>> > + else
>> > + data->config &= ~TC654_REG_CONFIG_DUTYC;
> I just realized that this won't work as intended. Problem is that you
> only fill data->config when reading an attribute. So, if a set function
> is called prior to reading an attribute, data->config will be 0, and
> you end up overwriting the original configuration.
>
Should I just read it in the probe function or fill it in with the
documented hardware defaults?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv3] hwmon: Add tc654 driver
2016-10-10 20:08 ` Chris Packham
@ 2016-10-10 20:59 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2016-10-10 20:59 UTC (permalink / raw)
To: Chris Packham
Cc: linux-hwmon, Masahiko Iwamoto, Joshua Scott, Kevin Tsai,
Wolfram Sang, Rob Herring, Mark Rutland, Jean Delvare,
Jonathan Corbet, linux-i2c, devicetree, linux-kernel, linux-doc
On Mon, Oct 10, 2016 at 08:08:14PM +0000, Chris Packham wrote:
> On 10/11/2016 02:22 AM, Guenter Roeck wrote:
> >> + if (val)
> >> > + data->config |= TC654_REG_CONFIG_DUTYC;
> >> > + else
> >> > + data->config &= ~TC654_REG_CONFIG_DUTYC;
> > I just realized that this won't work as intended. Problem is that you
> > only fill data->config when reading an attribute. So, if a set function
> > is called prior to reading an attribute, data->config will be 0, and
> > you end up overwriting the original configuration.
> >
>
> Should I just read it in the probe function or fill it in with the
> documented hardware defaults?
Reading it would be better - that leaves the option open that it was configured
by ROMMON or BIOS.
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCHv4] hwmon: Add tc654 driver
2016-10-10 13:21 ` Guenter Roeck
(?)
(?)
@ 2016-10-10 21:26 ` Chris Packham
2016-10-10 21:54 ` Rob Herring
2016-10-12 13:03 ` Guenter Roeck
-1 siblings, 2 replies; 16+ messages in thread
From: Chris Packham @ 2016-10-10 21:26 UTC (permalink / raw)
To: linux, linux-hwmon
Cc: iwamoto, Joshua.Scott, Chris Packham, Kevin Tsai, Wolfram Sang,
Rob Herring, Mark Rutland, Jean Delvare, Jonathan Corbet,
linux-i2c, devicetree, linux-kernel, linux-doc
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 v4:
- tab-align values in #defines
- ensure locking in set_pwm covers updating cached values
- populate the cached value for the config register in tc654_probe()
Changes in v3:
- typofix in documentation
- add missing value to tc654_pwm_map, re-generate based on datasheet.
- remove unnecessary hwmon_dev member from struct tc654_data
- bug fixes in set_fan_min() and show_pwm_mode()
- miscellaneous style fixes
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 | 31 ++
drivers/hwmon/Kconfig | 11 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/tc654.c | 514 +++++++++++++++++++++
5 files changed, 559 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..91a2843f5f98
--- /dev/null
+++ b/Documentation/hwmon/tc654
@@ -0,0 +1,31 @@
+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 uses 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.
+
+
+Setting pwm1_mode to 1 will cause the pwm output to be driven based on
+the pwm1 value. Setting pwm1_mode to 0 will cause the pwm output to be
+driven based on the Vin 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..04485f6d6983
--- /dev/null
+++ b/drivers/hwmon/tc654.c
@@ -0,0 +1,514 @@
+/*
+ * 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/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/mutex.h>
+#include <linux/slab.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 BIT(6) /* Resolution Selection */
+#define TC654_REG_CONFIG_DUTYC BIT(5) /* Duty Cycle Control */
+#define TC654_REG_CONFIG_SDM BIT(0) /* Shutdown Mode */
+
+/* Status register bits */
+#define TC654_REG_STATUS_F2F BIT(1) /* Fan 2 Fault */
+#define TC654_REG_STATUS_F1F BIT(0) /* 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;
+
+ /* 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 & 0x0f;
+
+ 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;
+
+ 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]);
+
+ 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] = { 77, 88, 102, 112, 124, 136, 148, 160,
+ 172, 184, 196, 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;
+
+ mutex_lock(&data->update_lock);
+
+ 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));
+
+ 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);
+
+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;
+ struct device *hwmon_dev;
+ int ret;
+
+ 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;
+ mutex_init(&data->update_lock);
+
+ ret = i2c_smbus_read_byte_data(client, TC654_REG_CONFIG);
+ if (ret < 0)
+ return ret;
+
+ data->config = ret;
+
+ hwmon_dev =
+ devm_hwmon_device_register_with_groups(dev, client->name, data,
+ tc654_groups);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+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",
+ },
+ .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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCHv4] hwmon: Add tc654 driver
2016-10-10 21:26 ` [PATCHv4] " Chris Packham
@ 2016-10-10 21:54 ` Rob Herring
2016-10-12 13:03 ` Guenter Roeck
1 sibling, 0 replies; 16+ messages in thread
From: Rob Herring @ 2016-10-10 21:54 UTC (permalink / raw)
To: Chris Packham
Cc: linux, linux-hwmon, iwamoto, Joshua.Scott, Kevin Tsai,
Wolfram Sang, Mark Rutland, Jean Delvare, Jonathan Corbet,
linux-i2c, devicetree, linux-kernel, linux-doc
On Tue, Oct 11, 2016 at 10:26:31AM +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 v4:
> - tab-align values in #defines
> - ensure locking in set_pwm covers updating cached values
> - populate the cached value for the config register in tc654_probe()
>
> Changes in v3:
> - typofix in documentation
> - add missing value to tc654_pwm_map, re-generate based on datasheet.
> - remove unnecessary hwmon_dev member from struct tc654_data
> - bug fixes in set_fan_min() and show_pwm_mode()
> - miscellaneous style fixes
>
> 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 +
Expect a merge conflict with the IIO tree.
Acked-by: Rob Herring <robh@kernel.org>
> Documentation/hwmon/tc654 | 31 ++
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tc654.c | 514 +++++++++++++++++++++
> 5 files changed, 559 insertions(+)
> create mode 100644 Documentation/hwmon/tc654
> create mode 100644 drivers/hwmon/tc654.c
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv4] hwmon: Add tc654 driver
2016-10-10 21:26 ` [PATCHv4] " Chris Packham
2016-10-10 21:54 ` Rob Herring
@ 2016-10-12 13:03 ` Guenter Roeck
2016-10-12 20:17 ` Chris Packham
1 sibling, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2016-10-12 13:03 UTC (permalink / raw)
To: Chris Packham
Cc: linux-hwmon, iwamoto, Joshua.Scott, Kevin Tsai, Wolfram Sang,
Rob Herring, Mark Rutland, Jean Delvare, Jonathan Corbet,
linux-i2c, devicetree, linux-kernel, linux-doc
On Tue, Oct 11, 2016 at 10:26:31AM +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>
> Acked-by: Rob Herring <robh@kernel.org>
Applied to -next (after fixing continuation line alignments).
Guenter
> ---
> Changes in v4:
> - tab-align values in #defines
> - ensure locking in set_pwm covers updating cached values
> - populate the cached value for the config register in tc654_probe()
>
> Changes in v3:
> - typofix in documentation
> - add missing value to tc654_pwm_map, re-generate based on datasheet.
> - remove unnecessary hwmon_dev member from struct tc654_data
> - bug fixes in set_fan_min() and show_pwm_mode()
> - miscellaneous style fixes
>
> 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 | 31 ++
> drivers/hwmon/Kconfig | 11 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tc654.c | 514 +++++++++++++++++++++
> 5 files changed, 559 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..91a2843f5f98
> --- /dev/null
> +++ b/Documentation/hwmon/tc654
> @@ -0,0 +1,31 @@
> +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 uses 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.
> +
> +
> +Setting pwm1_mode to 1 will cause the pwm output to be driven based on
> +the pwm1 value. Setting pwm1_mode to 0 will cause the pwm output to be
> +driven based on the Vin 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..04485f6d6983
> --- /dev/null
> +++ b/drivers/hwmon/tc654.c
> @@ -0,0 +1,514 @@
> +/*
> + * 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/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/mutex.h>
> +#include <linux/slab.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 BIT(6) /* Resolution Selection */
> +#define TC654_REG_CONFIG_DUTYC BIT(5) /* Duty Cycle Control */
> +#define TC654_REG_CONFIG_SDM BIT(0) /* Shutdown Mode */
> +
> +/* Status register bits */
> +#define TC654_REG_STATUS_F2F BIT(1) /* Fan 2 Fault */
> +#define TC654_REG_STATUS_F1F BIT(0) /* 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;
> +
> + /* 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 & 0x0f;
> +
> + 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;
> +
> + 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]);
> +
> + 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] = { 77, 88, 102, 112, 124, 136, 148, 160,
> + 172, 184, 196, 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;
> +
> + mutex_lock(&data->update_lock);
> +
> + 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));
> +
> + 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);
> +
> +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;
> + struct device *hwmon_dev;
> + int ret;
> +
> + 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;
> + mutex_init(&data->update_lock);
> +
> + ret = i2c_smbus_read_byte_data(client, TC654_REG_CONFIG);
> + if (ret < 0)
> + return ret;
> +
> + data->config = ret;
> +
> + hwmon_dev =
> + devm_hwmon_device_register_with_groups(dev, client->name, data,
> + tc654_groups);
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +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",
> + },
> + .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");
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv4] hwmon: Add tc654 driver
2016-10-12 13:03 ` Guenter Roeck
@ 2016-10-12 20:17 ` Chris Packham
0 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2016-10-12 20:17 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-hwmon, Masahiko Iwamoto, Joshua Scott, Kevin Tsai,
Wolfram Sang, Rob Herring, Mark Rutland, Jean Delvare,
Jonathan Corbet, linux-i2c, devicetree, linux-kernel, linux-doc
On 10/13/2016 02:03 AM, Guenter Roeck wrote:
> On Tue, Oct 11, 2016 at 10:26:31AM +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>
>> > Acked-by: Rob Herring <robh@kernel.org>
> Applied to -next (after fixing continuation line alignments).
>
Sorry about those. I'll try to do better next time.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCHv4] hwmon: Add tc654 driver
@ 2016-10-12 20:17 ` Chris Packham
0 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2016-10-12 20:17 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-hwmon-u79uwXL29TY76Z2rM5mHXA, Masahiko Iwamoto,
Joshua Scott, Kevin Tsai, Wolfram Sang, Rob Herring,
Mark Rutland, Jean Delvare, Jonathan Corbet,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
On 10/13/2016 02:03 AM, Guenter Roeck wrote:
> On Tue, Oct 11, 2016 at 10:26:31AM +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-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org>
>> > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Applied to -next (after fixing continuation line alignments).
>
Sorry about those. I'll try to do better next time.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-10-12 20:17 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-06 22:57 [PATCHv1] hwmon: Add tc654 driver Guenter Roeck
2016-10-06 22:57 ` Guenter Roeck
2016-10-06 23:13 ` Chris Packham
2016-10-07 1:38 ` [PATCHv2] " Chris Packham
2016-10-07 18:29 ` Guenter Roeck
2016-10-09 21:21 ` Chris Packham
2016-10-09 22:12 ` [PATCHv3] " Chris Packham
2016-10-10 13:21 ` Guenter Roeck
2016-10-10 13:21 ` Guenter Roeck
2016-10-10 20:08 ` Chris Packham
2016-10-10 20:59 ` Guenter Roeck
2016-10-10 21:26 ` [PATCHv4] " Chris Packham
2016-10-10 21:54 ` Rob Herring
2016-10-12 13:03 ` Guenter Roeck
2016-10-12 20:17 ` Chris Packham
2016-10-12 20:17 ` Chris Packham
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.