| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c893384d-4134-4510-be87-11a2c9ba6cc7@kernel.org>
Date: Wed, 2 Jul 2025 10:35:25 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Álvaro Fernández Rojas <noltari@...il.com>,
jdelvare@...e.com, linux@...ck-us.net, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, corbet@....net, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH] drivers: hwmon: add EMC2101 driver
On 01/07/2025 20:12, Álvaro Fernández Rojas wrote:
> The Microchip EMC2101 is a SMBus 2.0 fan controller with temperature
> monitoring.
> It supports up to 1 fan, 1 internal temperature sensor, 1 external
> temperature sensor and an 8 entry look up table to create a
> programmable temperature response.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
> ---
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/emc2101.c | 2175 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 2186 insertions(+)
> create mode 100644 drivers/hwmon/emc2101.c
>
> v2: multiple improvements:
> - Remove FAN_RPM_MIN definition.
> - Rename FAN_FALSE_READ to FAN_MIN_READ.
> - pwm_auto_point_temp_hyst_store(): simplify function.
> - emc2101_fan_min_read(): add missing FAN_MIN_READ condition.
> - emc2101_fan_min_write(): fix tach_count calculation.
> - emc2101_init(): fix REG_TACH_MIN value.
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 079620dd4286..360b9f66275c 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2002,6 +2002,16 @@ config SENSORS_EMC1403
> Threshold values can be configured using sysfs.
> Data from the different diodes are accessible via sysfs.
>
> +config SENSORS_EMC2101
> + tristate "SMSC EMC2101"
> + depends on I2C
> + help
> + If you say yes here you get support for the SMSC EMC2101
> + fan controller chips.
> +
> + This driver can also be built as a module. If so, the module
> + will be called emc2101.
> +
> config SENSORS_EMC2103
> tristate "SMSC EMC2103"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 48e5866c0c9a..70e95096c6f2 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -73,6 +73,7 @@ obj-$(CONFIG_SENSORS_DRIVETEMP) += drivetemp.o
> obj-$(CONFIG_SENSORS_DS620) += ds620.o
> obj-$(CONFIG_SENSORS_DS1621) += ds1621.o
> obj-$(CONFIG_SENSORS_EMC1403) += emc1403.o
> +obj-$(CONFIG_SENSORS_EMC2101) += emc2101.o
> obj-$(CONFIG_SENSORS_EMC2103) += emc2103.o
> obj-$(CONFIG_SENSORS_EMC2305) += emc2305.o
> obj-$(CONFIG_SENSORS_EMC6W201) += emc6w201.o
> diff --git a/drivers/hwmon/emc2101.c b/drivers/hwmon/emc2101.c
> new file mode 100644
> index 000000000000..65f2eff27aaf
> --- /dev/null
> +++ b/drivers/hwmon/emc2101.c
> @@ -0,0 +1,2176 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for Microchip EMC2101 fan controller.
> + *
> + * Copyright 2025 Álvaro Fernández Rojas <noltari@...il.com>
> + */
> +
> +#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>
> +
> +#define REG_TEMP_INT 0x00
> +#define REG_TEMP_EXT_HI 0x01
> +#define REG_STATUS 0x02
> +#define ADC_BUSY BIT(7)
> +#define TEMP_INT_HIGH BIT(6)
> +#define EEPROM_ERROR BIT(5)
> +#define TEMP_EXT_HIGH BIT(4)
> +#define TEMP_EXT_LOW BIT(3)
> +#define TEMP_EXT_FAULT BIT(2)
> +#define TEMP_EXT_CRIT BIT(1)
> +#define TACH_LOW BIT(0)
> +#define REG_CONFIG 0x03
> +#define ALERT_IRQ_ACK BIT(7)
> +#define FAN_STANDBY_ENABLE BIT(6)
> +#define FAN_STANDBY_MODE BIT(5)
> +#define FAN_MODE_DAC BIT(4)
> +#define SMBUS_TOUT_DISABLE BIT(3)
> +#define PIN_FUNC_TACH BIT(2)
> +#define TEMP_EXT_CRIT_UNLOCK BIT(1)
> +#define PIN_ASSERT_3_EXC BIT(0)
> +#define REG_CONV_RATE 0x04
> +#define CONV_RATE_MASK 0xf
> +#define REG_TEMP_INT_MAX 0x05
> +#define REG_TEMP_EXT_MAX_HI 0x07
> +#define REG_TEMP_EXT_MIN_HI 0x08
> +#define REG_TEMP_EXT_FORCE 0x0c
> +#define REG_ONE_SHOT 0x0f
> +#define REG_TEMP_EXT_LO 0x10
> +#define REG_SCRATCHPAD_1 0x11
> +#define REG_SCRATCHPAD_2 0x12
> +#define REG_TEMP_EXT_MAX_LO 0x13
> +#define REG_TEMP_EXT_MIN_LO 0x14
> +#define REG_ALERT_MASK 0x16
> +#define IRQ_TEMP_INT_MAX_DISABLE BIT(6)
> +#define IRQ_TEMP_EXT_MAX_DISABLE BIT(4)
> +#define IRQ_TEMP_EXT_MIN_DISABLE BIT(3)
> +#define IRQ_TEMP_EXT_CRIT_DISABLE BIT(1)
> +#define IRQ_TACH_MIN_DISABLE BIT(0)
> +#define REG_EXT_IDEALITY 0x17
> +#define EXT_IDEALITY_START 9846
> +#define EXT_IDEALITY_STEP 13
> +#define EXT_IDEALITY_VAL(x) (EXT_IDEALITY_START + \
> + ((x) * EXT_IDEALITY_STEP))
> +#define EXT_IDEALITY_MASK 0x3f
> +#define REG_BETA_COMP 0x18
> +#define BETA_COMP_AUTO BIT(3)
> +#define BETA_COMP_DISABLE 7
> +#define BETA_COMP_2_33 6
> +#define BETA_COMP_1_00 5
> +#define BETA_COMP_0_43 4
> +#define BETA_COMP_0_33 3
> +#define BETA_COMP_0_25 2
> +#define BETA_COMP_0_18 1
> +#define BETA_COMP_0_11 0
> +#define BETA_COMP_MASK 0x7
> +#define REG_TEMP_EXT_CRIT 0x19
> +#define REG_TEMP_EXT_CRIT_HYST 0x21
> +#define REG_TACH_LO 0x46
> +#define REG_TACH_HI 0x47
> +#define REG_TACH_MIN_LO 0x48
> +#define REG_TACH_MIN_HI 0x49
> +#define REG_FAN_CONFIG 0x4a
> +#define FAN_EXT_TEMP_FORCE BIT(6)
> +#define FAN_LUT_DISABLE BIT(5)
> +#define FAN_POL_INV BIT(4)
> +#define FAN_CLK_SEL BIT(3)
> +#define FAN_CLK_OVR BIT(2)
> +#define TACH_FALSE_READ_DISABLE BIT(0)
> +#define TACH_FALSE_READ_MASK 0x3
> +#define REG_FAN_SPIN 0x4b
> +#define FAN_SPIN_UP_ABORT BIT(5)
> +#define FAN_SPIN_UP_POWER_SHIFT 3
> +#define FAN_SPIN_UP_POWER_100 (3 << FAN_SPIN_UP_POWER_SHIFT)
> +#define FAN_SPIN_UP_POWER_75 (2 << FAN_SPIN_UP_POWER_SHIFT)
> +#define FAN_SPIN_UP_POWER_50 (1 << FAN_SPIN_UP_POWER_SHIFT)
> +#define FAN_SPIN_UP_POWER_0 (0 << FAN_SPIN_UP_POWER_SHIFT)
> +#define FAN_SPIN_UP_POWER_MASK (0x3 << FAN_SPIN_UP_POWER_SHIFT)
> +#define FAN_SPIN_UP_TIME_3200 7
> +#define FAN_SPIN_UP_TIME_1600 6
> +#define FAN_SPIN_UP_TIME_800 5
> +#define FAN_SPIN_UP_TIME_400 4
> +#define FAN_SPIN_UP_TIME_200 3
> +#define FAN_SPIN_UP_TIME_100 2
> +#define FAN_SPIN_UP_TIME_50 1
> +#define FAN_SPIN_UP_TIME_0 0
> +#define FAN_SPIN_UP_TIME_MASK 0x7
> +#define REG_FAN_SET 0x4c
> +#define FAN_SET_MASK 0x3f
> +#define REG_PWM_FREQ 0x4d
> +#define PWM_FREQ_MASK 0x1f
> +#define REG_PWM_FREQ_DIV 0x4e
> +#define REG_FAN_LUT_HYST 0x4f
> +#define FAN_LUT_HYST_MASK 0x1f
> +#define REG_FAN_LUT_TEMP(x) (0x50 + (0x2 * (x)))
> +/* Write only with FAN_LUT_DISABLE */
> +#define FAN_LUT_TEMP_MASK 0x7f
> +#define REG_FAN_LUT_SPEED(x) (0x51 + (0x2 * (x)))
> +/* Write only with FAN_LUT_DISABLE */
> +#define FAN_LUT_SPEED_MASK 0x3f
> +#define REG_AVG_FILTER 0xbf
> +#define FILTER_SHIFT 1
> +#define FILTER_L2 (3 << FILTER_SHIFT)
> +#define FILTER_L1 (1 << FILTER_SHIFT)
> +#define FILTER_NONE (0 << FILTER_SHIFT)
> +#define FILTER_MASK (0x3 << FILTER_SHIFT)
> +#define ALERT_PIN_TEMP_COMP BIT(0)
> +#define REG_PRODUCT_ID 0xfd
> +#define REG_MANUFACTURER_ID 0xfe
> +#define REG_REVISION 0xff
> +
> +#define CLK_FREQ_ALT 1400
> +#define CLK_FREQ_BASE 360000
> +
> +#define FAN_LUT_COUNT 8
> +#define FAN_LUT_HYST_DEF 4
> +#define FAN_LUT_HYST_MIN 0
> +#define FAN_LUT_HYST_MAX 31
> +#define FAN_MIN_READ 0xffff
> +#define FAN_RPM_FACTOR 5400000
> +
> +#define MANUFACTURER_ID 0x5d
> +
> +#define TEMP_EXT_HI_FAULT 0x7f
> +#define TEMP_EXT_LO_FAULT_OPEN 0x00
> +#define TEMP_EXT_LO_FAULT_SHORT 0xe0
> +
> +#define TEMP_LO_FRAC 125
> +#define TEMP_LO_SHIFT 5
> +#define TEMP_LO_MASK (0x3 << TEMP_LO_SHIFT)
> +
> +#define TEMP_MIN -64
> +#define TEMP_MAX 127
> +#define TEMP_MAX_FRAC 750
> +
> +enum emc2101_fan_spin_up_abort {
> + EMC2101_FAN_SPIN_ABORT_DISABLE = 0,
> + EMC2101_FAN_SPIN_ABORT_ENABLE
> +};
> +
> +enum emc2101_fan_standby {
> + EMC2101_FAN_STBY_DISABLE = 0,
> + EMC2101_FAN_STBY_ENABLE
> +};
> +
> +enum emc2101_mode {
> + EMC2101_MODE_PWM = 0,
> + EMC2101_MODE_DAC
> +};
> +
> +enum ecm2101_product_id {
> + EMC2101 = 0x16,
> + EMC2101_R = 0x28
> +};
> +
> +enum emc2101_pwm_enable {
> + EMC2101_PWM_MANUAL = 1,
> + EMC2101_PWM_LUT = 2
> +};
> +
> +enum emc2101_pwm_polarity {
> + EMC2101_POL_NORMAL = 0,
> + EMC2101_POL_INVERTED
> +};
> +
> +enum emc2101_temp_channels {
> + EMC2101_TC_INT = 0,
> + EMC2101_TC_EXT,
> + EMC2101_TC_FORCE,
> + EMC2101_TC_NUM
> +};
> +
> +enum emc2101_temp_diode {
> + EMC2101_TD_CPU = 1,
> + EMC2101_TD_2N3904 = 2
> +};
> +
> +struct emc2101_data {
> + struct i2c_client *client;
> + struct device *dev;
> + struct mutex mutex;
Add a comment describing what you are protecting here. It looks so far
like you could just use regmap and drop the mutex, but I didn't check
thoroughly.
> +};
...
> +
> +static int emc2101_probe(struct i2c_client *client)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + struct device *dev = &client->dev;
> + struct emc2101_data *data;
> + struct device *hwmon_dev;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -EIO;
> +
> + data = devm_kzalloc(dev, sizeof(struct emc2101_data), GFP_KERNEL);
sizeof(*)
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> + data->dev = dev;
> + mutex_init(&data->mutex);
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
> + &emc2101_chip_info,
> + emc2101_hwmon_groups);
> + if (IS_ERR(hwmon_dev))
> + return PTR_ERR(hwmon_dev);
> +
> + dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
Drivers should be silent oon success. We already know that this probed
based on sysfs.
> +
> + return emc2101_init(data);
> +}
> +
> +static int emc2101_detect(struct i2c_client *client, struct i2c_board_info *info)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + s32 manufacturer, product, revision;
> + struct device *dev = &adapter->dev;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -ENODEV;
> +
> + manufacturer = i2c_smbus_read_byte_data(client, REG_MANUFACTURER_ID);
> + if (manufacturer != MANUFACTURER_ID)
> + return -ENODEV;
> +
> + product = i2c_smbus_read_byte_data(client, REG_PRODUCT_ID);
> + switch (product) {
> + case EMC2101:
> + strscpy(info->type, "emc2101", I2C_NAME_SIZE);
> + break;
> + case EMC2101_R:
> + strscpy(info->type, "emc2101-r", I2C_NAME_SIZE);
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + revision = i2c_smbus_read_byte_data(client, REG_REVISION);
> +
> + dev_info(dev, "Found %s at 0x%02x (rev 0x%02x).\n",
> + info->type, client->addr, revision);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id emc2101_ids[] = {
> + { "emc2101" },
> + { "emc2101-r" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, emc2101_ids);
> +
> +static const struct of_device_id emc2101_of_match_table[] = {
> + { .compatible = "microchip,emc2101", },
> + { .compatible = "microchip,emc2101-r", },
Devices are compatible then? Express it in the bindings and drop this entry.
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, emc2101_of_match_table);
> +
> +static const unsigned short emc2101_address_list[] = {
> + 0x4c, I2C_CLIENT_END
> +};
> +
Best regards,
Krzysztof
Powered by blists - more mailing lists