Thank you for the details. As suggested, have versioned and submitted the updated patch separately along with the change log for review. Regards, Arun Saravanan -----Original Message----- From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck Sent: Thursday, August 12, 2021 8:16 PM To: Balac, Arun Saravanan Cc: jdelvare@xxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx Subject: Re: [PATCH] hwmon: Add Maxim MAX6620 hardware monitoring driver [EXTERNAL EMAIL] On Thu, Aug 12, 2021 at 02:11:58PM +0000, Balac, Arun Saravanan wrote: > From: Arun Saravanan Balachandran <Arun_Saravanan_Balac@xxxxxxxx> > > Add hardware monitoring driver for Maxim MAX6620 Fan controller > > Originally-from: L. Grunenberg <contact@xxxxxxxxxxxxxx> > Originally-from: Cumulus Networks <support@xxxxxxxxxxxxxxxxxxx> > Originally-from: Shuotian Cheng <shuche@xxxxxxxxxxxxx> > Signed-off-by: Arun Saravanan Balachandran <Arun_Saravanan_Balac@xxxxxxxx> > --- Updated patches must be sent as new patch and be versioned, not as reply to a previous version. Also, <Formletter> Change log goes here. If it is missing, I won't know what changed. That means I will have to dig out older patch versions to compare. That costs time and would hold up both this patch as well as all other patches which I still have to review. For this reason, I will not review patches without change log. </Formletter> Guenter > Documentation/hwmon/max6620.rst | 46 +++ > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/max6620.c | 510 ++++++++++++++++++++++++++++++++ > 4 files changed, 567 insertions(+) > create mode 100644 Documentation/hwmon/max6620.rst > create mode 100644 drivers/hwmon/max6620.c > > diff --git a/Documentation/hwmon/max6620.rst b/Documentation/hwmon/max6620.rst > new file mode 100644 > index 000000000000..84c1c44d3de4 > --- /dev/null > +++ b/Documentation/hwmon/max6620.rst > @@ -0,0 +1,46 @@ > +.. SPDX-License-Identifier: GPL-2.0-or-later > + > +Kernel driver max6620 > +===================== > + > +Supported chips: > + > + Maxim MAX6620 > + > + Prefix: 'max6620' > + > + Addresses scanned: none > + > + Datasheet: https://urldefense.com/v3/__http://pdfserv.maxim-ic.com/en/ds/MAX6620.pdf__;!!LpKI!yvnJNNxy30vWyMVuJ11QlMK86iBKRvlY7jHoLh9pERUOIN1bd4Yp6IUzBjBjhn4IHlJS2w$ [pdfserv[.]maxim-ic[.]com] > + > +Authors: > + - L\. Grunenberg <contact@xxxxxxxxxxxxxx> > + - Cumulus Networks <support@xxxxxxxxxxxxxxxxxxx> > + - Shuotian Cheng <shuche@xxxxxxxxxxxxx> > + - Arun Saravanan Balachandran <Arun_Saravanan_Balac@xxxxxxxx> > + > +Description > +----------- > + > +This driver implements support for Maxim MAX6620 fan controller. > + > +The driver configures the fan controller in RPM mode. To give the readings more > +range or accuracy, the desired value can be set by a programmable register > +(1, 2, 4, 8, 16 or 32). Set higher values for larger speeds. > + > +The driver provides the following sensor access in sysfs: > + > +================ ======= ===================================================== > +fan[1-4]_alarm ro Fan alarm. > +fan[1-4]_div rw Sets the nominal RPM range of the fan. Valid values > + are 1, 2, 4, 8, 16 and 32. > +fan[1-4]_input ro Fan speed in RPM. > +fan[1-4]_target rw Desired fan speed in RPM. > +================ ======= ===================================================== > + > +Usage notes > +----------- > + > +This driver does not auto-detect devices. You will have to instantiate the > +devices explicitly. Please see Documentation/i2c/instantiating-devices.rst for > +details. > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index e3675377bc5d..74811fbaa266 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1022,6 +1022,16 @@ config SENSORS_MAX31730 > This driver can also be built as a module. If so, the module > will be called max31730. > > +config SENSORS_MAX6620 > + tristate "Maxim MAX6620 fan controller" > + depends on I2C > + help > + If you say yes here you get support for the MAX6620 > + fan controller. > + > + This driver can also be built as a module. If so, the module > + will be called max6620. > + > config SENSORS_MAX6621 > tristate "Maxim MAX6621 sensor chip" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index d712c61c1f5e..9e50ff903931 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -135,6 +135,7 @@ obj-$(CONFIG_SENSORS_MAX1668) += max1668.o > obj-$(CONFIG_SENSORS_MAX197) += max197.o > obj-$(CONFIG_SENSORS_MAX31722) += max31722.o > obj-$(CONFIG_SENSORS_MAX31730) += max31730.o > +obj-$(CONFIG_SENSORS_MAX6620) += max6620.o > obj-$(CONFIG_SENSORS_MAX6621) += max6621.o > obj-$(CONFIG_SENSORS_MAX6639) += max6639.o > obj-$(CONFIG_SENSORS_MAX6642) += max6642.o > diff --git a/drivers/hwmon/max6620.c b/drivers/hwmon/max6620.c > new file mode 100644 > index 000000000000..1b709f0fcb7f > --- /dev/null > +++ b/drivers/hwmon/max6620.c > @@ -0,0 +1,510 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Hardware monitoring driver for Maxim MAX6620 > + * > + * (C) 2012 by L. Grunenberg <contact@xxxxxxxxxxxxxx> > + * > + * based on code written by : > + * 2007 by Hans J. Koch <hjk@xxxxxxxxxxxx> > + * John Morris <john.morris@xxxxxxxxxxxxxx> > + * Copyright (c) 2003 Spirent Communications > + * and Claus Gindhart <claus.gindhart@xxxxxxxxxxx> > + * > + * This module has only been tested with the MAX6620 chip. > + * > + * The datasheet was last seen at: > + * > + * https://urldefense.com/v3/__http://pdfserv.maxim-ic.com/en/ds/MAX6620.pdf__;!!LpKI!yvnJNNxy30vWyMVuJ11QlMK86iBKRvlY7jHoLh9pERUOIN1bd4Yp6IUzBjBjhn4IHlJS2w$ [pdfserv[.]maxim-ic[.]com] > + * > + */ > + > +#include <linux/err.h> > +#include <linux/hwmon.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/jiffies.h> > +#include <linux/module.h> > +#include <linux/slab.h> > + > +/* > + * MAX 6620 registers > + */ > + > +#define MAX6620_REG_CONFIG 0x00 > +#define MAX6620_REG_FAULT 0x01 > +#define MAX6620_REG_CONF_FAN0 0x02 > +#define MAX6620_REG_CONF_FAN1 0x03 > +#define MAX6620_REG_CONF_FAN2 0x04 > +#define MAX6620_REG_CONF_FAN3 0x05 > +#define MAX6620_REG_DYN_FAN0 0x06 > +#define MAX6620_REG_DYN_FAN1 0x07 > +#define MAX6620_REG_DYN_FAN2 0x08 > +#define MAX6620_REG_DYN_FAN3 0x09 > +#define MAX6620_REG_TACH0 0x10 > +#define MAX6620_REG_TACH1 0x12 > +#define MAX6620_REG_TACH2 0x14 > +#define MAX6620_REG_TACH3 0x16 > +#define MAX6620_REG_VOLT0 0x18 > +#define MAX6620_REG_VOLT1 0x1A > +#define MAX6620_REG_VOLT2 0x1C > +#define MAX6620_REG_VOLT3 0x1E > +#define MAX6620_REG_TAR0 0x20 > +#define MAX6620_REG_TAR1 0x22 > +#define MAX6620_REG_TAR2 0x24 > +#define MAX6620_REG_TAR3 0x26 > +#define MAX6620_REG_DAC0 0x28 > +#define MAX6620_REG_DAC1 0x2A > +#define MAX6620_REG_DAC2 0x2C > +#define MAX6620_REG_DAC3 0x2E > + > +/* > + * Config register bits > + */ > + > +#define MAX6620_CFG_RUN BIT(7) > +#define MAX6620_CFG_POR BIT(6) > +#define MAX6620_CFG_TIMEOUT BIT(5) > +#define MAX6620_CFG_FULLFAN BIT(4) > +#define MAX6620_CFG_OSC BIT(3) > +#define MAX6620_CFG_WD_MASK (BIT(2) | BIT(1)) > +#define MAX6620_CFG_WD_2 BIT(1) > +#define MAX6620_CFG_WD_6 BIT(2) > +#define MAX6620_CFG_WD10 (BIT(2) | BIT(1)) > +#define MAX6620_CFG_WD BIT(0) > + > +/* > + * Failure status register bits > + */ > + > +#define MAX6620_FAIL_TACH0 BIT(4) > +#define MAX6620_FAIL_TACH1 BIT(5) > +#define MAX6620_FAIL_TACH2 BIT(6) > +#define MAX6620_FAIL_TACH3 BIT(7) > +#define MAX6620_FAIL_MASK0 BIT(0) > +#define MAX6620_FAIL_MASK1 BIT(1) > +#define MAX6620_FAIL_MASK2 BIT(2) > +#define MAX6620_FAIL_MASK3 BIT(3) > + > +#define MAX6620_CLOCK_FREQ 8192 /* Clock frequency in Hz */ > +#define MAX6620_PULSE_PER_REV 2 /* Tachometer pulses per revolution */ > + > +/* Minimum and maximum values of the FAN-RPM */ > +#define FAN_RPM_MIN 240 > +#define FAN_RPM_MAX 30000 > + > +static const u8 config_reg[] = { > + MAX6620_REG_CONF_FAN0, > + MAX6620_REG_CONF_FAN1, > + MAX6620_REG_CONF_FAN2, > + MAX6620_REG_CONF_FAN3, > +}; > + > +static const u8 dyn_reg[] = { > + MAX6620_REG_DYN_FAN0, > + MAX6620_REG_DYN_FAN1, > + MAX6620_REG_DYN_FAN2, > + MAX6620_REG_DYN_FAN3, > +}; > + > +static const u8 tach_reg[] = { > + MAX6620_REG_TACH0, > + MAX6620_REG_TACH1, > + MAX6620_REG_TACH2, > + MAX6620_REG_TACH3, > +}; > + > +static const u8 target_reg[] = { > + MAX6620_REG_TAR0, > + MAX6620_REG_TAR1, > + MAX6620_REG_TAR2, > + MAX6620_REG_TAR3, > +}; > + > +/* > + * Client data (each client gets its own) > + */ > + > +struct max6620_data { > + struct i2c_client *client; > + struct mutex update_lock; > + bool valid; /* false until following fields are valid */ > + unsigned long last_updated; /* in jiffies */ > + > + /* register values */ > + u8 fancfg[4]; > + u8 fandyn[4]; > + u8 fault; > + u16 tach[4]; > + u16 target[4]; > +}; > + > +static u8 max6620_fan_div_from_reg(u8 val) > +{ > + return 1 << ((val & 0xE0) >> 5); > +} > + > +static int max6620_update_device(struct device *dev) > +{ > + struct max6620_data *data = dev_get_drvdata(dev); > + struct i2c_client *client = data->client; > + int i, reg, regval1, regval2; > + int ret = 0; > + > + mutex_lock(&data->update_lock); > + > + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { > + for (i = 0; i < 4; i++) { > + reg = i2c_smbus_read_byte_data(client, config_reg[i]); > + if (reg < 0) { > + ret = reg; > + goto error; > + } > + data->fancfg[i] = reg; > + > + reg = i2c_smbus_read_byte_data(client, dyn_reg[i]); > + if (reg < 0) { > + ret = reg; > + goto error; > + } > + data->fandyn[i] = reg; > + > + regval1 = i2c_smbus_read_byte_data(client, tach_reg[i]); > + if (regval1 < 0) { > + ret = regval1; > + goto error; > + } > + regval2 = i2c_smbus_read_byte_data(client, tach_reg[i] + 1); > + if (regval2 < 0) { > + ret = regval2; > + goto error; > + } > + data->tach[i] = ((regval1 << 3) & 0x7f8) | ((regval2 >> 5) & 0x7); > + > + regval1 = i2c_smbus_read_byte_data(client, target_reg[i]); > + if (regval1 < 0) { > + ret = regval1; > + goto error; > + } > + regval2 = i2c_smbus_read_byte_data(client, target_reg[i] + 1); > + if (regval2 < 0) { > + ret = regval2; > + goto error; > + } > + data->target[i] = ((regval1 << 3) & 0x7f8) | ((regval2 >> 5) & 0x7); > + } > + > + /* > + * Alarms are cleared on read in case the condition that > + * caused the alarm is removed. Keep the value latched here > + * for providing the register through different alarm files. > + */ > + reg = i2c_smbus_read_byte_data(client, MAX6620_REG_FAULT); > + if (reg < 0) { > + ret = reg; > + goto error; > + } > + data->fault |= (reg >> 4) & (reg & 0x0F); > + > + data->last_updated = jiffies; > + data->valid = true; > + } > + > +error: > + mutex_unlock(&data->update_lock); > + return ret; > +} > + > +static umode_t > +max6620_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr, > + int channel) > +{ > + switch (type) { > + case hwmon_fan: > + switch (attr) { > + case hwmon_fan_alarm: > + case hwmon_fan_input: > + return 0444; > + case hwmon_fan_div: > + case hwmon_fan_target: > + return 0644; > + default: > + break; > + } > + > + default: > + break; > + } > + > + return 0; > +} > + > +static int > +max6620_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, > + int channel, long *val) > +{ > + struct max6620_data *data; > + struct i2c_client *client; > + int ret = 0; > + u8 div; > + u8 val1; > + u8 val2; > + > + ret = max6620_update_device(dev); > + if (ret < 0) > + return ret; > + data = dev_get_drvdata(dev); > + client = data->client; > + > + switch (type) { > + case hwmon_fan: > + switch (attr) { > + case hwmon_fan_alarm: > + *val = !!(data->fault & BIT(channel)); > + > + /* Setting TACH count to re-enable fan fault detection */ > + if (*val == 1) { > + val1 = (data->target[channel] >> 3) & 0xff; > + val2 = (data->target[channel] << 5) & 0xe0; > + ret = i2c_smbus_write_byte_data(client, > + target_reg[channel], val1); > + if (ret < 0) > + return ret; > + ret = i2c_smbus_write_byte_data(client, > + target_reg[channel] + 1, val2); > + if (ret < 0) > + return ret; > + > + mutex_lock(&data->update_lock); > + data->fault &= ~(BIT(channel)); > + mutex_unlock(&data->update_lock); > + } > + > + break; > + case hwmon_fan_div: > + *val = max6620_fan_div_from_reg(data->fandyn[channel]); > + break; > + case hwmon_fan_input: > + if (data->tach[channel] == 0) { > + *val = 0; > + } else { > + div = max6620_fan_div_from_reg(data->fandyn[channel]); > + *val = (60 * div * MAX6620_CLOCK_FREQ) / (data->tach[channel] > + * MAX6620_PULSE_PER_REV); > + } > + break; > + case hwmon_fan_target: > + if (data->target[channel] == 0) { > + *val = 0; > + } else { > + div = max6620_fan_div_from_reg(data->fandyn[channel]); > + *val = (60 * div * MAX6620_CLOCK_FREQ) / (data->target[channel] > + * MAX6620_PULSE_PER_REV); > + } > + break; > + default: > + return -EOPNOTSUPP; > + } > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int > +max6620_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, > + int channel, long val) > +{ > + struct max6620_data *data; > + struct i2c_client *client; > + int ret = 0; > + u8 div; > + u16 tach; > + u8 val1; > + u8 val2; > + > + ret = max6620_update_device(dev); > + if (ret < 0) > + return ret; > + data = dev_get_drvdata(dev); > + client = data->client; > + mutex_lock(&data->update_lock); > + > + switch (type) { > + case hwmon_fan: > + switch (attr) { > + case hwmon_fan_div: > + switch (val) { > + case 1: > + div = 0; > + break; > + case 2: > + div = 1; > + break; > + case 4: > + div = 2; > + break; > + case 8: > + div = 3; > + break; > + case 16: > + div = 4; > + break; > + case 32: > + div = 5; > + break; > + default: > + ret = -EINVAL; > + goto error; > + } > + data->fandyn[channel] &= 0x1F; > + data->fandyn[channel] |= div << 5; > + ret = i2c_smbus_write_byte_data(client, dyn_reg[channel], > + data->fandyn[channel]); > + break; > + case hwmon_fan_target: > + val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX); > + div = max6620_fan_div_from_reg(data->fandyn[channel]); > + tach = (60 * div * MAX6620_CLOCK_FREQ) / (val * MAX6620_PULSE_PER_REV); > + val1 = (tach >> 3) & 0xff; > + val2 = (tach << 5) & 0xe0; > + ret = i2c_smbus_write_byte_data(client, target_reg[channel], val1); > + if (ret < 0) > + break; > + ret = i2c_smbus_write_byte_data(client, target_reg[channel] + 1, val2); > + if (ret < 0) > + break; > + > + /* Setting TACH count re-enables fan fault detection */ > + data->fault &= ~(BIT(channel)); > + > + break; > + default: > + ret = -EOPNOTSUPP; > + break; > + } > + break; > + > + default: > + ret = -EOPNOTSUPP; > + break; > + } > + > +error: > + mutex_unlock(&data->update_lock); > + return ret; > +} > + > +static const struct hwmon_channel_info *max6620_info[] = { > + HWMON_CHANNEL_INFO(fan, > + HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_TARGET | HWMON_F_ALARM, > + HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_TARGET | HWMON_F_ALARM, > + HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_TARGET | HWMON_F_ALARM, > + HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_TARGET | HWMON_F_ALARM), > + NULL > +}; > + > +static const struct hwmon_ops max6620_hwmon_ops = { > + .read = max6620_read, > + .write = max6620_write, > + .is_visible = max6620_is_visible, > +}; > + > +static const struct hwmon_chip_info max6620_chip_info = { > + .ops = &max6620_hwmon_ops, > + .info = max6620_info, > +}; > + > +static int max6620_init_client(struct max6620_data *data) > +{ > + struct i2c_client *client = data->client; > + int config; > + int err; > + int i; > + int reg; > + > + config = i2c_smbus_read_byte_data(client, MAX6620_REG_CONFIG); > + if (config < 0) { > + dev_err(&client->dev, "Error reading config, aborting.\n"); > + return config; > + } > + > + /* > + * Set bit 4, disable other fans from going full speed on a fail > + * failure. > + */ > + err = i2c_smbus_write_byte_data(client, MAX6620_REG_CONFIG, config | 0x10); > + if (err < 0) { > + dev_err(&client->dev, "Config write error, aborting.\n"); > + return err; > + } > + > + for (i = 0; i < 4; i++) { > + reg = i2c_smbus_read_byte_data(client, config_reg[i]); > + if (reg < 0) > + return reg; > + data->fancfg[i] = reg; > + > + /* Enable RPM mode */ > + data->fancfg[i] |= 0xa8; > + err = i2c_smbus_write_byte_data(client, config_reg[i], data->fancfg[i]); > + if (err < 0) > + return err; > + > + /* 2 counts (001) and Rate change 100 (0.125 secs) */ > + data->fandyn[i] = 0x30; > + err = i2c_smbus_write_byte_data(client, dyn_reg[i], data->fandyn[i]); > + if (err < 0) > + return err; > + } > + return 0; > +} > + > +static int max6620_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct max6620_data *data; > + struct device *hwmon_dev; > + int err; > + > + data = devm_kzalloc(dev, sizeof(struct max6620_data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->client = client; > + mutex_init(&data->update_lock); > + > + err = max6620_init_client(data); > + if (err) > + return err; > + > + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, > + data, > + &max6620_chip_info, > + NULL); > + > + return PTR_ERR_OR_ZERO(hwmon_dev); > +} > + > +static const struct i2c_device_id max6620_id[] = { > + { "max6620", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, max6620_id); > + > +static struct i2c_driver max6620_driver = { > + .class = I2C_CLASS_HWMON, > + .driver = { > + .name = "max6620", > + }, > + .probe_new = max6620_probe, > + .id_table = max6620_id, > +}; > + > +module_i2c_driver(max6620_driver); > + > +MODULE_AUTHOR("Lucas Grunenberg"); > +MODULE_DESCRIPTION("MAX6620 sensor driver"); > +MODULE_LICENSE("GPL"); > > base-commit: ff1176468d368232b684f75e82563369208bc371 > -- > 2.32.0 > > Please find above the updated patch. > > Thanks, > Arun Saravanan > -----Original Message----- > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > Sent: Wednesday, July 28, 2021 9:25 PM > To: Balac, Arun Saravanan; jdelvare@xxxxxxxx > Cc: linux-hwmon@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] hwmon: Add Maxim MAX6620 hardware monitoring driver > > > [EXTERNAL EMAIL] > > On 7/28/21 7:11 AM, Balac, Arun Saravanan wrote: > > From: Arun Saravanan Balachandran <Arun_Saravanan_Balac@xxxxxxxx> > > > > Add hardware monitoring driver for Maxim MAX6620 Fan controller > > > > Originally-from: L. Grunenberg <contact@xxxxxxxxxxxxxx> > > Originally-from: Cumulus Networks <support@xxxxxxxxxxxxxxxxxxx> > > Originally-from: Shuotian Cheng <shuche@xxxxxxxxxxxxx> > > Signed-off-by: Arun Saravanan Balachandran <Arun_Saravanan_Balac@xxxxxxxx> > > --- > > drivers/hwmon/Kconfig | 10 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/max6620.c | 464 ++++++++++++++++++++++++++++++++++++++++ > > Documentation missing. > > > 3 files changed, 475 insertions(+) > > create mode 100644 drivers/hwmon/max6620.c > > > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index e3675377bc5d..7bb2fbd72db4 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -1022,6 +1022,16 @@ config SENSORS_MAX31730 > > This driver can also be built as a module. If so, the module > > will be called max31730. > > > > +config SENSORS_MAX6620 > > + tristate "Maxim MAX6620 sensor chip" > > + depends on I2C > > + help > > + If you say yes here you get support for the MAX6620 > > + sensor chips. > > This is not a sensor, it is a fan controller. > > > + > > + This driver can also be built as a module. If so, the module > > + will be called max6620. > > + > > config SENSORS_MAX6621 > > tristate "Maxim MAX6621 sensor chip" > > depends on I2C > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index d712c61c1f5e..9e50ff903931 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -135,6 +135,7 @@ obj-$(CONFIG_SENSORS_MAX1668) += max1668.o > > obj-$(CONFIG_SENSORS_MAX197) += max197.o > > obj-$(CONFIG_SENSORS_MAX31722) += max31722.o > > obj-$(CONFIG_SENSORS_MAX31730) += max31730.o > > +obj-$(CONFIG_SENSORS_MAX6620) += max6620.o > > obj-$(CONFIG_SENSORS_MAX6621) += max6621.o > > obj-$(CONFIG_SENSORS_MAX6639) += max6639.o > > obj-$(CONFIG_SENSORS_MAX6642) += max6642.o > > diff --git a/drivers/hwmon/max6620.c b/drivers/hwmon/max6620.c > > new file mode 100644 > > index 000000000000..05f6fdc80343 > > --- /dev/null > > +++ b/drivers/hwmon/max6620.c > > @@ -0,0 +1,464 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * max6620.c - Linux Kernel module for hardware monitoring. > > Pointless. > > > + * > > + * (C) 2012 by L. Grunenberg <contact@xxxxxxxxxxxxxx> > > + * > > + * based on code written by : > > + * 2007 by Hans J. Koch <hjk@xxxxxxxxxxxx> > > + * John Morris <john.morris@xxxxxxxxxxxxxx> > > + * Copyright (c) 2003 Spirent Communications > > + * and Claus Gindhart <claus.gindhart@xxxxxxxxxxx> > > + * > > + * This module has only been tested with the MAX6620 chip. > > + * > > + * The datasheet was last seen at: > > + * > > + * https://urldefense.com/v3/__http://pdfserv.maxim-ic.com/en/ds/MAX6620.pdf__;!!LpKI!1VdQjyy5Q-_FrmWBxhCaB4bhmElQ75SkuBrVm_q99Rjt5Ejt_AMjK94gP2c_gd_tRYx1Ow$ [pdfserv[.]maxim-ic[.]com] > > + * > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/init.h> > > +#include <linux/slab.h> > > +#include <linux/jiffies.h> > > +#include <linux/i2c.h> > > +#include <linux/hwmon.h> > > +#include <linux/hwmon-sysfs.h> > > Not needed. > > > +#include <linux/err.h> > > Alphabetic include file order, please. > > > + > > + > > +/* > > + * MAX 6620 registers > > + */ > > + > > +#define MAX6620_REG_CONFIG 0x00 > > +#define MAX6620_REG_FAULT 0x01 > > +#define MAX6620_REG_CONF_FAN0 0x02 > > +#define MAX6620_REG_CONF_FAN1 0x03 > > +#define MAX6620_REG_CONF_FAN2 0x04 > > +#define MAX6620_REG_CONF_FAN3 0x05 > > +#define MAX6620_REG_DYN_FAN0 0x06 > > +#define MAX6620_REG_DYN_FAN1 0x07 > > +#define MAX6620_REG_DYN_FAN2 0x08 > > +#define MAX6620_REG_DYN_FAN3 0x09 > > +#define MAX6620_REG_TACH0 0x10 > > +#define MAX6620_REG_TACH1 0x12 > > +#define MAX6620_REG_TACH2 0x14 > > +#define MAX6620_REG_TACH3 0x16 > > +#define MAX6620_REG_VOLT0 0x18 > > +#define MAX6620_REG_VOLT1 0x1A > > +#define MAX6620_REG_VOLT2 0x1C > > +#define MAX6620_REG_VOLT3 0x1E > > +#define MAX6620_REG_TAR0 0x20 > > +#define MAX6620_REG_TAR1 0x22 > > +#define MAX6620_REG_TAR2 0x24 > > +#define MAX6620_REG_TAR3 0x26 > > +#define MAX6620_REG_DAC0 0x28 > > +#define MAX6620_REG_DAC1 0x2A > > +#define MAX6620_REG_DAC2 0x2C > > +#define MAX6620_REG_DAC3 0x2E > > + > > +/* > > + * Config register bits > > + */ > > + > > +#define MAX6620_CFG_RUN 0x80 > > +#define MAX6620_CFG_POR 0x40 > > +#define MAX6620_CFG_TIMEOUT 0x20 > > +#define MAX6620_CFG_FULLFAN 0x10 > > +#define MAX6620_CFG_OSC 0x08 > > +#define MAX6620_CFG_WD_MASK 0x06 > > +#define MAX6620_CFG_WD_2 0x02 > > +#define MAX6620_CFG_WD_6 0x04 > > +#define MAX6620_CFG_WD10 0x06 > > +#define MAX6620_CFG_WD 0x01 > > Please use BIT(). > > > > + > > + > > +/* > > + * Failure status register bits > > + */ > > + > > +#define MAX6620_FAIL_TACH0 0x10 > > +#define MAX6620_FAIL_TACH1 0x20 > > +#define MAX6620_FAIL_TACH2 0x40 > > +#define MAX6620_FAIL_TACH3 0x80 > > +#define MAX6620_FAIL_MASK0 0x01 > > +#define MAX6620_FAIL_MASK1 0x02 > > +#define MAX6620_FAIL_MASK2 0x04 > > +#define MAX6620_FAIL_MASK3 0x08 > > + > > + > > +/* Minimum and maximum values of the FAN-RPM */ > > +#define FAN_RPM_MIN 240 > > +#define FAN_RPM_MAX 30000 > > #define<space>DEFINE<tab>value > > > + > > +/* > > + * Insmod parameters > > + */ > > + > > + > > +/* clock: The clock frequency of the chip the driver should assume */ > > +static int clock = 8192; > > +static u32 np = 2; > > 'clock' is always 8192. np is the number of pulses per revolution, > and always 2. Please use defines for both. > > > + > > +module_param(clock, int, 0444); > > + > > +static const u8 config_reg[] = { > > + MAX6620_REG_CONF_FAN0, > > + MAX6620_REG_CONF_FAN1, > > + MAX6620_REG_CONF_FAN2, > > + MAX6620_REG_CONF_FAN3, > > +}; > > + > > +static const u8 dyn_reg[] = { > > + MAX6620_REG_DYN_FAN0, > > + MAX6620_REG_DYN_FAN1, > > + MAX6620_REG_DYN_FAN2, > > + MAX6620_REG_DYN_FAN3, > > +}; > > + > > +static const u8 tach_reg[] = { > > + MAX6620_REG_TACH0, > > + MAX6620_REG_TACH1, > > + MAX6620_REG_TACH2, > > + MAX6620_REG_TACH3, > > +}; > > + > > +static const u8 target_reg[] = { > > + MAX6620_REG_TAR0, > > + MAX6620_REG_TAR1, > > + MAX6620_REG_TAR2, > > + MAX6620_REG_TAR3, > > +}; > > + > > +/* > > + * Client data (each client gets its own) > > + */ > > + > > +struct max6620_data { > > + struct i2c_client *client; > > + struct mutex update_lock; > > + char valid; /* zero until following fields are valid */ > > bool. However, I would strongly suggest to drop caching > except for the fault register. > > > + unsigned long last_updated; /* in jiffies */ > > + > > + /* register values */ > > + u8 config; > > + u8 fancfg[4]; > > + u8 fandyn[4]; > > + u8 fault; > > + u16 tach[4]; > > + u16 target[4]; > > +}; > > + > > +static u8 max6620_fan_div_from_reg(u8 val) > > +{ > > + return 1 << ((val & 0xE0) >> 5); > > +} > > + > > +static struct max6620_data *max6620_update_device(struct device *dev) > > +{ > > + struct max6620_data *data = dev_get_drvdata(dev); > > + struct i2c_client *client = data->client; > > + int i; > > + u8 fault_reg, regval1, regval2; > > + > > + mutex_lock(&data->update_lock); > > + > > + if (time_after(jiffies, data->last_updated + HZ) || !data->valid) { > > + > > + for (i = 0; i < 4; i++) { > > + data->fancfg[i] = i2c_smbus_read_byte_data(client, config_reg[i]); > > + data->fandyn[i] = i2c_smbus_read_byte_data(client, dyn_reg[i]); > > + regval1 = i2c_smbus_read_byte_data(client, tach_reg[i]); > > + regval2 = i2c_smbus_read_byte_data(client, tach_reg[i] + 1); > > + data->tach[i] = ((regval1 << 3) & 0x7f8) | ((regval2 >> 5) & 0x7); > > + regval1 = i2c_smbus_read_byte_data(client, target_reg[i]); > > + regval2 = i2c_smbus_read_byte_data(client, target_reg[i] + 1); > > + data->target[i] = ((regval1 << 3) & 0x7f8) | ((regval2 >> 5) & 0x7); > > + } > > + > > + > > + /* > > + * Alarms are cleared on read in case the condition that > > + * caused the alarm is removed. Keep the value latched here > > + * for providing the register through different alarm files. > > + */ > > + fault_reg = i2c_smbus_read_byte_data(client, MAX6620_REG_FAULT); > > + data->fault |= (fault_reg >> 4) & (fault_reg & 0x0F); > > + > > + data->last_updated = jiffies; > > + data->valid = 1; > > + } > > + > > + mutex_unlock(&data->update_lock); > > + > > + return data; > > +} > > + > > +static umode_t > > +max6620_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr, > > + int channel) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + switch (attr) { > > + case hwmon_fan_alarm: > > + case hwmon_fan_input: > > + return 0444; > > + case hwmon_fan_div: > > + case hwmon_fan_target: > > + return 0644; > > + default: > > + break; > > + } > > + > > + default: > > + break; > > + } > > + > > + return 0; > > +} > > + > > +static int > > +max6620_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, > > + int channel, long *val) > > +{ > > + struct max6620_data *data = max6620_update_device(dev); > > + int alarm = 0; > > + u8 div; > > + > > + switch (type) { > > + case hwmon_fan: > > + switch (attr) { > > + case hwmon_fan_alarm: > > + mutex_lock(&data->update_lock); > > + if (data->fault & (1 << channel)) > > + alarm = 1; > > + > > + mutex_unlock(&data->update_lock); > > Locking is pointless here. > *val = !!(data->fault & BIT(channel)); > does the same. > > This code does not clear alarms after reading the attribute, > or re-enable alarms. Clearing faults by relying on a write > to fan_target is not appropriate. Alarms should be cleared > and re-armed after reading an alarm attribute. > > > + *val = alarm; > > + > > + break; > > + case hwmon_fan_div: > > + *val = max6620_fan_div_from_reg(data->fandyn[channel]); > > + break; > > + case hwmon_fan_input: > > + if (data->tach[channel] == 0) > > + *val = 0; > > + else { > > if and else branch both need to use { }. > > Please run checkpatch --strict and fix what it reports. > > > + div = max6620_fan_div_from_reg(data->fandyn[channel]); > > + *val = (60 * div * clock)/(data->tach[channel] * np); > > + } > > + break; > > + case hwmon_fan_target: > > + if (data->target[channel] == 0) > > + *val = 0; > > + else { > > + div = max6620_fan_div_from_reg(data->fandyn[channel]); > > + *val = (60 * div * clock)/(data->target[channel] * np); > > + } > > + break; > > + default: > > + return -EOPNOTSUPP; > > + } > > + break; > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int > > +max6620_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, > > + int channel, long val) > > +{ > > + struct max6620_data *data = dev_get_drvdata(dev); > > + struct i2c_client *client = data->client; > > + u8 div; > > + u16 tach; > > + u8 val1; > > + u8 val2; > > + > > + switch (type) { > > + case hwmon_fan: > > + switch (attr) { > > + case hwmon_fan_div: > > + mutex_lock(&data->update_lock); > > The lock is pointless here. Move it after the switch statement to > simplify the code. > > > + switch (val) { > > + case 1: > > + div = 0; > > + break; > > + case 2: > > + div = 1; > > + break; > > + case 4: > > + div = 2; > > + break; > > + case 8: > > + div = 3; > > + break; > > + case 16: > > + div = 4; > > + break; > > + case 32: > > + div = 5; > > + break; > > + default: > > + mutex_unlock(&data->update_lock); > > + return -EINVAL; > > + } > > + data->fandyn[channel] &= 0x1F; > > + data->fandyn[channel] |= div << 5; > > + i2c_smbus_write_byte_data(client, dyn_reg[channel], > > + data->fandyn[channel]); > > Please do not ignore errors (everywhere). > > > + mutex_unlock(&data->update_lock); > > + > > + break; > > + case hwmon_fan_target: > > + val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX); > > + mutex_lock(&data->update_lock); > > + > > + div = max6620_fan_div_from_reg(data->fandyn[channel]); > > + tach = (60 * div * clock)/(val * np); > > + val1 = (tach >> 3) & 0xff; > > + val2 = (tach << 5) & 0xe0; > > + i2c_smbus_write_byte_data(client, target_reg[channel], val1); > > + i2c_smbus_write_byte_data(client, target_reg[channel] + 1, val2); > > + > > + /* Setting TACH count re-enables fan fault detection */ > > + data->fault &= ~(1 << channel); > > Maybe, but expecting the user to write to this register to re-arm alarms > is not appropriate. > > > + > > + mutex_unlock(&data->update_lock); > > + > > + break; > > + default: > > + return -EOPNOTSUPP; > > + } > > + break; > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static const u32 max6620_fan_config[] = { > > + HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_TARGET | HWMON_F_ALARM, > > + HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_TARGET | HWMON_F_ALARM, > > + HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_TARGET | HWMON_F_ALARM, > > + HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_TARGET | HWMON_F_ALARM, > > + 0 > > +}; > > + > > +static const struct hwmon_channel_info max6620_fan = { > > + .type = hwmon_fan, > > + .config = max6620_fan_config, > > +}; > > + > > +static const struct hwmon_channel_info *max6620_info[] = { > > + &max6620_fan, > > + NULL > > +}; > > Please use the HWMON_CHANNEL_INFO() macro. > > > + > > +static const struct hwmon_ops max6620_hwmon_ops = { > > + .read = max6620_read, > > + .write = max6620_write, > > + .is_visible = max6620_is_visible, > > +}; > > + > > +static const struct hwmon_chip_info max6620_chip_info = { > > + .ops = &max6620_hwmon_ops, > > + .info = max6620_info, > > +}; > > + > > +static int max6620_init_client(struct i2c_client *client) > > +{ > > + struct max6620_data *data = i2c_get_clientdata(client); > > + int config; > > + int err = -EIO; > > + int i; > > + > > + config = i2c_smbus_read_byte_data(client, MAX6620_REG_CONFIG); > > + > Unnecessary empty line > > > + if (config < 0) { > > + dev_err(&client->dev, "Error reading config, aborting.\n"); > > + return err; > > Please do not overwrite error codes. This should return config. > > > + } > > + > > + /* > > + * Set bit 4, disable other fans from going full speed on a fail > > + * failure. > > + */ > > + if (i2c_smbus_write_byte_data(client, MAX6620_REG_CONFIG, config | 0x10)) { > > + dev_err(&client->dev, "Config write error, aborting.\n"); > > + return err; > > Please return the error code from i2c_smbus_write_byte_data(). > > > + } > > + > > + data->config = config; > > data->config is not used anywhere. > > > + for (i = 0; i < 4; i++) { > > + data->fancfg[i] = i2c_smbus_read_byte_data(client, config_reg[i]); > > + /* Enable RPM mode */ > > + data->fancfg[i] |= 0xa8; > > + i2c_smbus_write_byte_data(client, config_reg[i], data->fancfg[i]); > > + data->fandyn[i] = i2c_smbus_read_byte_data(client, dyn_reg[i]); > > + /* 2 counts (001) and Rate change 100 (0.125 secs) */ > > + data->fandyn[i] = 0x30; > > + i2c_smbus_write_byte_data(client, dyn_reg[i], data->fandyn[i]); > > Again, please do not ignore error codes. Also, this mostly duplicates > max6620_update_device(). > > > + } > > + return 0; > > +} > > + > > +static int max6620_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct max6620_data *data; > > + struct device *hwmon_dev; > > + int err; > > + > > + data = devm_kzalloc(dev, sizeof(struct max6620_data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + i2c_set_clientdata(client, data); > > The only reason for this is to use it in max6620_init_client(). > Just pass 'data' to that function instead. > > > + data->client = client; > > + mutex_init(&data->update_lock); > > + > > + /* > > + * Initialize the max6620 chip > > + */ > > Pointless comment. > > > + err = max6620_init_client(client); > > + if (err) > > + return err; > > + > > + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, > > + data, > > + &max6620_chip_info, > > + NULL); > > + > > + return PTR_ERR_OR_ZERO(hwmon_dev); > > +} > > + > > +static const struct i2c_device_id max6620_id[] = { > > + { "max6620", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, max6620_id); > > + > > +static struct i2c_driver max6620_driver = { > > + .class = I2C_CLASS_HWMON, > > + .driver = { > > + .name = "max6620", > > + }, > > + .probe_new = max6620_probe, > > + .id_table = max6620_id, > > +}; > > + > > +module_i2c_driver(max6620_driver); > > + > > +MODULE_AUTHOR("Lucas Grunenberg"); > > +MODULE_DESCRIPTION("MAX6620 sensor driver"); > > +MODULE_LICENSE("GPL"); > > > > base-commit: ff1176468d368232b684f75e82563369208bc371 > > >
