This driver implements support for temperature monitoring of Astera Labs
PT5161L series PCIe retimer chips.
Cosmo Chou (3):
dt-bindings: vendor-prefixes: add asteralabs
dt-bindings: hwmon: pt516xx: add bindings
hwmon: Add driver for Astera Labs PT516XX retimer
.../bindings/hwmon/asteralabs,pt516xx.yaml | 36 +
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/pt516xx.rst | 48 ++
MAINTAINERS | 8 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++
8 files changed, 754 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
create mode 100644 Documentation/hwmon/pt516xx.rst
create mode 100644 drivers/hwmon/pt516xx.c
--
2.34.1
Add dt-bindings for pt516xx temperature monitor.
Signed-off-by: Cosmo Chou <[email protected]>
---
.../bindings/hwmon/asteralabs,pt516xx.yaml | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
new file mode 100644
index 000000000000..5700d4c91a0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
@@ -0,0 +1,36 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/asteralabs,pt516xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PT5161L hwmon sensor
+
+maintainers:
+ - Cosmo Chou <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - asteralabs,pt5161l
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sensor@24 {
+ compatible = "asteralabs,pt5161l";
+ reg = <0x24>;
+ };
+ };
--
2.34.1
Add vendor prefix for Astera Labs, Inc.
https://www.asteralabs.com
Signed-off-by: Cosmo Chou <[email protected]>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 309b94c328c8..5c49f63d4ef0 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -155,6 +155,8 @@ patternProperties:
description: ASPEED Technology Inc.
"^asrock,.*":
description: ASRock Inc.
+ "^asteralabs,.*":
+ description: Astera Labs, Inc.
"^asus,.*":
description: AsusTek Computer Inc.
"^atheros,.*":
--
2.34.1
This driver implements support for temperature monitoring of Astera Labs
PT5161L series PCIe retimer chips.
This driver implementation originates from the CSDK available at
Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
The communication protocol utilized is based on the I2C/SMBus standard.
Signed-off-by: Cosmo Chou <[email protected]>
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/pt516xx.rst | 48 +++
MAINTAINERS | 8 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++++++++++++++++
6 files changed, 716 insertions(+)
create mode 100644 Documentation/hwmon/pt516xx.rst
create mode 100644 drivers/hwmon/pt516xx.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 72f4e6065bae..2c4df18db663 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -181,6 +181,7 @@ Hardware Monitoring Kernel Drivers
pmbus
powerz
powr1220
+ pt516xx
pxe1610
pwm-fan
q54sj108a2
diff --git a/Documentation/hwmon/pt516xx.rst b/Documentation/hwmon/pt516xx.rst
new file mode 100644
index 000000000000..945194f9a804
--- /dev/null
+++ b/Documentation/hwmon/pt516xx.rst
@@ -0,0 +1,48 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver pt516xx
+====================
+
+Supported chips:
+
+ * Astera Labs PT5161L
+
+ Prefix: 'pt5161l'
+
+ Addresses: I2C 0x24
+
+ Datasheet: http://www.asteralabs.com/wp-content/uploads/2021/03/Astera_Labs_PT5161L_Product_Brief.pdf
+
+Authors: Cosmo Chou <[email protected]>
+
+Description
+-----------
+
+This driver implements support for temperature monitoring of Astera Labs
+PT5161L series PCIe retimer chips.
+
+This driver implementation originates from the CSDK available at
+https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
+The communication protocol utilized is based on the I2C/SMBus standard.
+
+For more detailed information and specific implementation details, it is
+recommended to refer to the CSDK source code available at the provided GitHub
+link.
+
+Sysfs entries
+----------------
+
+================ ==============================================
+temp1_input Measured temperature (in millidegrees Celsius)
+================ ==============================================
+
+Debugfs entries
+----------------
+
+================ ====================================
+fw_ver Firmware version of the retimer
+health Health status (8 bits)
+ [0]: Heartbeat Okay (1'b1: OK)
+ [1]: Firmware loaded Okay (1'b1: OK)
+ [7:2]: Reserved
+================ ====================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 788be9ab5b73..492002ffd12c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17482,6 +17482,14 @@ F: fs/pstore/
F: include/linux/pstore*
K: \b(pstore|ramoops)
+PT516XX HARDWARE MONITOR DRIVER
+M: Cosmo Chou <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
+F: Documentation/hwmon/pt516xx.rst
+F: drivers/hwmon/pt516xx.c
+
PTP HARDWARE CLOCK SUPPORT
M: Richard Cochran <[email protected]>
L: [email protected]
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index cf27523eed5a..3965bec7774a 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1703,6 +1703,16 @@ source "drivers/hwmon/peci/Kconfig"
source "drivers/hwmon/pmbus/Kconfig"
+config SENSORS_PT516XX
+ tristate "Astera Labs PT516XX PCIe retimer hardware monitoring"
+ depends on I2C
+ help
+ If you say yes here you get support for temperature monitoring
+ on the Astera Labs PT516XX PCIe retimer.
+
+ This driver can also be built as a module. If so, the module
+ will be called pt516xx.
+
config SENSORS_PWM_FAN
tristate "PWM fan"
depends on (PWM && OF) || COMPILE_TEST
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e84bd9685b5c..1942064cd4e9 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
obj-$(CONFIG_SENSORS_POWERZ) += powerz.o
obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
+obj-$(CONFIG_SENSORS_PT516XX) += pt516xx.o
obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
obj-$(CONFIG_SENSORS_SBTSI) += sbtsi_temp.o
diff --git a/drivers/hwmon/pt516xx.c b/drivers/hwmon/pt516xx.c
new file mode 100644
index 000000000000..824798559fe1
--- /dev/null
+++ b/drivers/hwmon/pt516xx.c
@@ -0,0 +1,648 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+
+/** Main-micro-assisted access command codes */
+// Wide register reads
+#define ARIES_MM_RD_WIDE_REG_2B 0x1d
+#define ARIES_MM_RD_WIDE_REG_3B 0x1e
+#define ARIES_MM_RD_WIDE_REG_4B 0x1f
+#define ARIES_MM_RD_WIDE_REG_5B 0x20
+// Wide register writes
+#define ARIES_MM_WR_WIDE_REG_2B 0x21
+#define ARIES_MM_WR_WIDE_REG_3B 0x22
+#define ARIES_MM_WR_WIDE_REG_4B 0x23
+#define ARIES_MM_WR_WIDE_REG_5B 0x24
+
+/** Temperature measurement constants */
+// Aries current average temp ADC code CSR
+#define ARIES_CURRENT_AVG_TEMP_ADC_CSR 0x42c
+
+// Main Micro Heartbeat reg
+#define ARIES_MM_HEARTBEAT_ADDR 0x923
+
+/** Main SRAM */
+// AL Main SRAM DMEM offset (A0)
+#define AL_MAIN_SRAM_DMEM_OFFSET (64 * 1024)
+// SRAM read command
+#define AL_TG_RD_LOC_IND_SRAM 0x16
+
+/** Micros */
+// Offset for main micro FW info
+#define ARIES_MAIN_MICRO_FW_INFO (96 * 1024 - 128)
+
+/** Path Micro Members */
+// FW Info (Major) offset location in struct
+#define ARIES_MM_FW_VERSION_MAJOR 0
+// FW Info (Minor) offset location in struct
+#define ARIES_MM_FW_VERSION_MINOR 1
+// FW Info (Build no.) offset location in struct
+#define ARIES_MM_FW_VERSION_BUILD 2
+
+/** Offsets for MM assisted accesses */
+// Legacy Address and Data registers (using wide registers)
+// Reg offset to specify Address for MM assisted accesses
+#define ARIES_MM_ASSIST_REG_ADDR_OFFSET 0xd99
+// Reg offset to specify Command
+#define ARIES_MM_ASSIST_CMD_OFFSET 0xd9d
+
+// New Address and Data registers (not using wide registers)
+// Reg offset to MM SPARE 0 used specify Address[7:0]
+#define ARIES_MM_ASSIST_SPARE_0_OFFSET 0xd9f
+// Reg offset to MM SPARE 3 used specify Data Byte 0
+#define ARIES_MM_ASSIST_SPARE_3_OFFSET 0xda2
+
+/** Misc block offsets */
+// Device Load check register
+#define ARIES_CODE_LOAD_REG 0x605
+
+/** Value indicating FW was loaded properly */
+#define ARIES_LOAD_CODE 0xe
+
+/** EEPROM parameters */
+// Time delay between checking MM status of EEPROM write (microseconds)
+#define ARIES_MM_STATUS_TIME 5000
+
+#define ARIES_TEMP_CAL_CODE_DEFAULT 84
+
+/* Struct defining FW version loaded on an Aries device */
+struct pt516xx_fw_ver {
+ u8 major; // FW version major release value
+ u8 minor; // FW version minor release value
+ u16 build; // FW version build release value
+};
+
+/* Each client has this additional data */
+struct pt516xx_data {
+ struct i2c_client *client;
+ struct dentry *debugfs;
+ struct pt516xx_fw_ver fw_ver;
+ struct mutex lock;
+ bool init_done;
+ bool pec_enable; // Enable PEC
+ bool code_load_okay; // indicate if code load reg value is expected
+ bool mm_heartbeat_okay; // indicate if Main Micro heartbeat is good
+ bool mm_wide_reg_valid; // MM assisted Wide Register access
+ u8 temp_cal_code_avg;
+};
+
+static struct dentry *pt516xx_debugfs_dir;
+
+/*
+ * Write multiple data bytes to Aries over I2C
+ */
+static int pt516xx_write_block_data(struct pt516xx_data *data, u32 address,
+ u8 len, u8 *val)
+{
+ struct i2c_client *client = data->client;
+ int ret;
+ u8 remain_len = len;
+ u8 xfer_len, curr_len;
+ u8 buf[16];
+ u8 cmd = 0x0F; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end
+ u8 config = 0x40; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address
+
+ if (data->pec_enable)
+ cmd |= 0x80;
+
+ // If byte count is greater than 4, perform multiple iterations
+ while (remain_len > 0) {
+ if (remain_len > 4) {
+ curr_len = 4;
+ remain_len -= 4;
+ } else {
+ curr_len = remain_len;
+ remain_len = 0;
+ }
+
+ buf[0] = config | (curr_len - 1) << 1 | ((address >> 16) & 0x1);
+ buf[1] = (address >> 8) & 0xff;
+ buf[2] = address & 0xff;
+ memcpy(&buf[3], val, curr_len);
+
+ xfer_len = 3 + curr_len;
+ ret = i2c_smbus_write_block_data(client, cmd, xfer_len, buf);
+ if (ret)
+ return ret;
+
+ val += curr_len;
+ address += curr_len;
+ }
+
+ return 0;
+}
+
+/*
+ * Read multiple data bytes from Aries over I2C
+ */
+static int pt516xx_read_block_data(struct pt516xx_data *data, u32 address,
+ u8 len, u8 *val)
+{
+ struct i2c_client *client = data->client;
+ int ret, tries;
+ u8 remain_len = len;
+ u8 curr_len;
+ u8 wbuf[16], rbuf[24];
+ u8 cmd = 0x08; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end
+ u8 config = 0x00; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address
+
+ if (data->pec_enable)
+ cmd |= 0x80;
+
+ // If byte count is greater than 16, perform multiple iterations
+ while (remain_len > 0) {
+ if (remain_len > 16) {
+ curr_len = 16;
+ remain_len -= 16;
+ } else {
+ curr_len = remain_len;
+ remain_len = 0;
+ }
+
+ wbuf[0] = config | (curr_len - 1) << 1 |
+ ((address >> 16) & 0x1);
+ wbuf[1] = (address >> 8) & 0xff;
+ wbuf[2] = address & 0xff;
+
+ // Perform read operation
+ for (tries = 0; tries < 3; tries++) {
+ ret = i2c_smbus_write_block_data(client, (cmd | 0x2), 3,
+ wbuf);
+ if (ret)
+ return ret;
+
+ ret = i2c_smbus_read_block_data(client, (cmd | 0x1),
+ rbuf);
+ if (ret == curr_len)
+ break;
+ }
+ if (tries >= 3)
+ return -ENODATA;
+
+ memcpy(val, rbuf, curr_len);
+ val += curr_len;
+ address += curr_len;
+ }
+
+ return 0;
+}
+
+static int pt516xx_read_wide_reg(struct pt516xx_data *data, u32 address,
+ u8 width, u8 *val)
+{
+ int ret, tries;
+ u8 buf[8];
+ u8 status;
+
+ if (data->mm_wide_reg_valid) {
+ // Write address (3 bytes)
+ buf[0] = address & 0xff;
+ buf[1] = (address >> 8) & 0xff;
+ buf[2] = (address >> 16) & 0x1;
+ ret = pt516xx_write_block_data(
+ data, ARIES_MM_ASSIST_SPARE_0_OFFSET, 3, buf);
+ if (ret)
+ return ret;
+
+ // Set command based on width
+ switch (width) {
+ case 2:
+ buf[0] = ARIES_MM_RD_WIDE_REG_2B;
+ break;
+ case 3:
+ buf[0] = ARIES_MM_RD_WIDE_REG_3B;
+ break;
+ case 4:
+ buf[0] = ARIES_MM_RD_WIDE_REG_4B;
+ break;
+ case 5:
+ buf[0] = ARIES_MM_RD_WIDE_REG_5B;
+ break;
+ default:
+ return -EINVAL;
+ }
+ ret = pt516xx_write_block_data(data, ARIES_MM_ASSIST_CMD_OFFSET,
+ 1, buf);
+ if (ret)
+ return ret;
+
+ // Check access status
+ status = 0xff;
+ for (tries = 0; tries < 100; tries++) {
+ ret = pt516xx_read_block_data(
+ data, ARIES_MM_ASSIST_CMD_OFFSET, 1, &status);
+ if (ret)
+ return ret;
+
+ if (status == 0)
+ break;
+
+ usleep_range(ARIES_MM_STATUS_TIME,
+ ARIES_MM_STATUS_TIME + 1000);
+ }
+ if (status != 0)
+ return -ETIMEDOUT;
+
+ // Read N bytes of data based on width
+ ret = pt516xx_read_block_data(
+ data, ARIES_MM_ASSIST_SPARE_3_OFFSET, width, val);
+ if (ret)
+ return ret;
+ } else {
+ return pt516xx_read_block_data(data, address, width, val);
+ }
+
+ return 0;
+}
+
+/*
+ * Read multiple (up to eight) data bytes from micro SRAM over I2C
+ */
+static int
+pt516xx_read_block_data_main_micro_indirect(struct pt516xx_data *data,
+ u32 address, u8 len, u8 *val)
+{
+ int ret, tries;
+ u8 buf[8];
+ u8 i, status;
+ u32 uind_offs = ARIES_MM_ASSIST_REG_ADDR_OFFSET;
+ u32 eeprom_base, eeprom_addr;
+
+ // No multi-byte indirect support here. Hence read a byte at a time
+ eeprom_base = address - AL_MAIN_SRAM_DMEM_OFFSET;
+ for (i = 0; i < len; i++) {
+ // Write eeprom addr
+ eeprom_addr = eeprom_base + i;
+ buf[0] = eeprom_addr & 0xff;
+ buf[1] = (eeprom_addr >> 8) & 0xff;
+ buf[2] = (eeprom_addr >> 16) & 0xff;
+ ret = pt516xx_write_block_data(data, uind_offs, 3, buf);
+ if (ret)
+ return ret;
+
+ // Write eeprom cmd
+ buf[0] = AL_TG_RD_LOC_IND_SRAM;
+ ret = pt516xx_write_block_data(data, uind_offs + 4, 1, buf);
+ if (ret)
+ return ret;
+
+ // Test successful access
+ status = 0xff;
+ for (tries = 0; tries < 255; tries++) {
+ ret = pt516xx_read_block_data(data, uind_offs + 4, 1,
+ &status);
+ if (ret)
+ return ret;
+
+ if (status == 0)
+ break;
+ }
+ if (status != 0)
+ return -ETIMEDOUT;
+
+ ret = pt516xx_read_block_data(data, uind_offs + 3, 1, buf);
+ if (ret)
+ return ret;
+
+ val[i] = buf[0];
+ }
+
+ return 0;
+}
+
+/*
+ * Check the status of firmware
+ */
+static int pt516xx_fwsts_check(struct pt516xx_data *data)
+{
+ int ret, tries;
+ u8 buf[8];
+ u8 heartbeat, major = 0, minor = 0;
+ u16 build = 0;
+ bool hb_changed = false;
+
+ // Read Code Load reg
+ ret = pt516xx_read_block_data(data, ARIES_CODE_LOAD_REG, 1, buf);
+ if (ret)
+ return ret;
+
+ if (buf[0] < ARIES_LOAD_CODE) {
+ dev_warn(
+ &data->client->dev,
+ "Code Load reg unexpected. Not all modules are loaded %x\n",
+ buf[0]);
+ data->code_load_okay = false;
+ } else {
+ data->code_load_okay = true;
+ }
+
+ // Check Main Micro heartbeat
+ // If heartbeat value does not change for 100 tries, no MM heartbeat
+ // Else heartbeat present even if one value changes
+ ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, buf);
+ if (ret)
+ return ret;
+
+ heartbeat = buf[0];
+ for (tries = 0; tries < 100; tries++) {
+ ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1,
+ buf);
+ if (ret)
+ return ret;
+
+ if (buf[0] != heartbeat) {
+ hb_changed = true;
+ break;
+ }
+ }
+ data->mm_heartbeat_okay = hb_changed;
+
+ // Read FW version
+ // If heartbeat not there, set default FW values to 0.0.0
+ // and return success
+ if (data->mm_heartbeat_okay) {
+ // Get FW version (major)
+ ret = pt516xx_read_block_data_main_micro_indirect(
+ data,
+ ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MAJOR, 1,
+ &major);
+ if (ret)
+ return ret;
+
+ // Get FW version (minor)
+ ret = pt516xx_read_block_data_main_micro_indirect(
+ data,
+ ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MINOR, 1,
+ &minor);
+ if (ret)
+ return ret;
+
+ // Get FW version (build)
+ ret = pt516xx_read_block_data_main_micro_indirect(
+ data,
+ ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_BUILD, 2,
+ (u8 *)&build);
+ if (ret)
+ return ret;
+ } else {
+ dev_warn(&data->client->dev, "No Main Micro Heartbeat\n");
+ }
+ data->fw_ver.major = major;
+ data->fw_ver.minor = minor;
+ data->fw_ver.build = build;
+
+ return 0;
+}
+
+static int pt516xx_fw_is_at_least(struct pt516xx_data *data, u8 major, u8 minor,
+ u16 build)
+{
+ u32 ver = major << 24 | minor << 16 | build;
+ u32 curr_ver = data->fw_ver.major << 24 | data->fw_ver.minor << 16 |
+ data->fw_ver.build;
+
+ if (curr_ver >= ver)
+ return true;
+
+ return false;
+}
+
+static int pt516xx_init_dev(struct pt516xx_data *data)
+{
+ int ret;
+
+ mutex_lock(&data->lock);
+ ret = pt516xx_fwsts_check(data);
+ mutex_unlock(&data->lock);
+ if (ret)
+ return ret;
+
+ dev_info(&data->client->dev, "fw_ver: %u.%u.%u\n", data->fw_ver.major,
+ data->fw_ver.minor, data->fw_ver.build);
+
+ if (pt516xx_fw_is_at_least(data, 2, 2, 0))
+ data->mm_wide_reg_valid = true;
+
+ data->temp_cal_code_avg = ARIES_TEMP_CAL_CODE_DEFAULT;
+ data->init_done = true;
+
+ return 0;
+}
+
+static int pt516xx_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct pt516xx_data *data = dev_get_drvdata(dev);
+ int ret = 0;
+ long adc_code = 0;
+
+ if (!data->init_done) {
+ ret = pt516xx_init_dev(data);
+ if (ret) {
+ dev_err(dev, "pt516xx_init_dev failed %d\n", ret);
+ return ret;
+ }
+ }
+
+ switch (attr) {
+ case hwmon_temp_input:
+ mutex_lock(&data->lock);
+ ret = pt516xx_read_wide_reg(data,
+ ARIES_CURRENT_AVG_TEMP_ADC_CSR, 4,
+ (u8 *)&adc_code);
+ mutex_unlock(&data->lock);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ if (ret) {
+ dev_err(dev, "Read adc_code failed %d\n", ret);
+ return ret;
+ }
+ if (adc_code == 0 || adc_code >= 0x3ff) {
+ dev_err(dev, "Invalid adc_code %lx\n", adc_code);
+ return -EINVAL;
+ }
+
+ *val = 110000 + ((adc_code - (data->temp_cal_code_avg + 250)) * -320);
+
+ return 0;
+}
+
+static umode_t pt516xx_is_visible(const void *data,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel)
+{
+ switch (attr) {
+ case hwmon_temp_input:
+ return 0444;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static const struct hwmon_channel_info *pt516xx_info[] = {
+ HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+ NULL
+};
+
+static const struct hwmon_ops pt516xx_hwmon_ops = {
+ .is_visible = pt516xx_is_visible,
+ .read = pt516xx_read,
+};
+
+static const struct hwmon_chip_info pt516xx_chip_info = {
+ .ops = &pt516xx_hwmon_ops,
+ .info = pt516xx_info,
+};
+
+static ssize_t pt516xx_debugfs_read_fw_ver(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct pt516xx_data *data = file->private_data;
+ int ret;
+ char ver[32];
+
+ mutex_lock(&data->lock);
+ ret = pt516xx_fwsts_check(data);
+ mutex_unlock(&data->lock);
+ if (ret)
+ return ret;
+
+ ret = snprintf(ver, sizeof(ver), "%u.%u.%u\n", data->fw_ver.major,
+ data->fw_ver.minor, data->fw_ver.build);
+ if (ret < 0)
+ return ret;
+
+ return simple_read_from_buffer(buf, count, ppos, ver, ret + 1);
+}
+
+static const struct file_operations pt516xx_debugfs_ops_fw_ver = {
+ .read = pt516xx_debugfs_read_fw_ver,
+ .open = simple_open,
+};
+
+static ssize_t pt516xx_debugfs_read_health(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct pt516xx_data *data = file->private_data;
+ int ret;
+ u8 status = 0;
+ char health[8];
+
+ mutex_lock(&data->lock);
+ ret = pt516xx_fwsts_check(data);
+ mutex_unlock(&data->lock);
+ if (ret == 0) {
+ status |= data->mm_heartbeat_okay ? 1 : 0; // bit0
+ status |= data->code_load_okay ? 2 : 0; // bit1
+ }
+
+ ret = snprintf(health, sizeof(health), "0x%02x\n", status);
+ if (ret < 0)
+ return ret;
+
+ return simple_read_from_buffer(buf, count, ppos, health, ret + 1);
+}
+
+static const struct file_operations pt516xx_debugfs_ops_health = {
+ .read = pt516xx_debugfs_read_health,
+ .open = simple_open,
+};
+
+static int pt516xx_init_debugfs(struct pt516xx_data *data)
+{
+ if (!pt516xx_debugfs_dir)
+ return -ENOENT;
+
+ data->debugfs = debugfs_create_dir(dev_name(&data->client->dev),
+ pt516xx_debugfs_dir);
+ if (IS_ERR_OR_NULL(data->debugfs))
+ return -ENOENT;
+
+ debugfs_create_file("fw_ver", 0444, data->debugfs, data,
+ &pt516xx_debugfs_ops_fw_ver);
+
+ debugfs_create_file("health", 0444, data->debugfs, data,
+ &pt516xx_debugfs_ops_health);
+
+ return 0;
+}
+
+static int pt516xx_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct device *hwmon_dev;
+ struct pt516xx_data *data;
+
+ data = devm_kzalloc(dev, sizeof(struct pt516xx_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->client = client;
+ mutex_init(&data->lock);
+ pt516xx_init_dev(data);
+
+ hwmon_dev = devm_hwmon_device_register_with_info(
+ dev, client->name, data, &pt516xx_chip_info, NULL);
+
+ if (pt516xx_init_debugfs(data))
+ dev_warn(dev, "Failed to register debugfs\n");
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id pt516xx_id[] = {
+ { "pt5161l", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, pt516xx_id);
+
+static const struct of_device_id __maybe_unused pt516xx_of_match[] = {
+ { .compatible = "asteralabs,pt5161l" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, pt516xx_of_match);
+
+static struct i2c_driver pt516xx_driver = {
+ .class = I2C_CLASS_HWMON,
+ .driver = {
+ .name = "pt516xx",
+ .of_match_table = of_match_ptr(pt516xx_of_match),
+ },
+ .probe = pt516xx_probe,
+ .id_table = pt516xx_id,
+};
+
+module_i2c_driver(pt516xx_driver);
+
+static int __init pt516xx_core_init(void)
+{
+ pt516xx_debugfs_dir = debugfs_create_dir("pt516xx", NULL);
+ if (IS_ERR(pt516xx_debugfs_dir))
+ pt516xx_debugfs_dir = NULL;
+
+ return 0;
+}
+
+static void __exit pt516xx_core_exit(void)
+{
+ debugfs_remove_recursive(pt516xx_debugfs_dir);
+}
+
+module_init(pt516xx_core_init);
+module_exit(pt516xx_core_exit);
+
+MODULE_AUTHOR("Cosmo Chou <[email protected]>");
+MODULE_DESCRIPTION("Hwmon driver for Astera Labs Aries PCIe retimer");
+MODULE_LICENSE("GPL");
--
2.34.1
On Tue, Dec 05, 2023 at 03:47:20PM +0800, Cosmo Chou wrote:
> This driver implements support for temperature monitoring of Astera Labs
> PT5161L series PCIe retimer chips.
>
> Cosmo Chou (3):
> dt-bindings: vendor-prefixes: add asteralabs
> dt-bindings: hwmon: pt516xx: add bindings
> hwmon: Add driver for Astera Labs PT516XX retimer
Please refrain from using wildcards in file, variable, or
function names.
Guenter
On 12/4/23 23:47, Cosmo Chou wrote:
> This driver implements support for temperature monitoring of Astera Labs
> PT5161L series PCIe retimer chips.
>
> This driver implementation originates from the CSDK available at
> Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
> The communication protocol utilized is based on the I2C/SMBus standard.
>
> Signed-off-by: Cosmo Chou <[email protected]>
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/pt516xx.rst | 48 +++
> MAINTAINERS | 8 +
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++++++++++++++++
> 6 files changed, 716 insertions(+)
> create mode 100644 Documentation/hwmon/pt516xx.rst
> create mode 100644 drivers/hwmon/pt516xx.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 72f4e6065bae..2c4df18db663 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -181,6 +181,7 @@ Hardware Monitoring Kernel Drivers
> pmbus
> powerz
> powr1220
> + pt516xx
> pxe1610
> pwm-fan
> q54sj108a2
> diff --git a/Documentation/hwmon/pt516xx.rst b/Documentation/hwmon/pt516xx.rst
> new file mode 100644
> index 000000000000..945194f9a804
> --- /dev/null
> +++ b/Documentation/hwmon/pt516xx.rst
> @@ -0,0 +1,48 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver pt516xx
> +====================
> +
> +Supported chips:
> +
> + * Astera Labs PT5161L
> +
> + Prefix: 'pt5161l'
> +
This should be the driver name and be used for function/variable prefixes.
No idea where XX comes into play. The driver for sure won't support
PT516[0-9][A-Z], and should not suggest that it does.
> + Addresses: I2C 0x24
> +
> + Datasheet: http://www.asteralabs.com/wp-content/uploads/2021/03/Astera_Labs_PT5161L_Product_Brief.pdf
> +
> +Authors: Cosmo Chou <[email protected]>
> +
> +Description
> +-----------
> +
> +This driver implements support for temperature monitoring of Astera Labs
> +PT5161L series PCIe retimer chips.
> +
> +This driver implementation originates from the CSDK available at
> +https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
> +The communication protocol utilized is based on the I2C/SMBus standard.
> +
That strongly suggests that the code has a copyright and license associated
with it. None of it is mentioned here.
> +For more detailed information and specific implementation details, it is
> +recommended to refer to the CSDK source code available at the provided GitHub
> +link.
> +
Linux kernel drivers should be self contained. It is fine to reference documentation,
but not out-of-tree source code.
> +Sysfs entries
> +----------------
> +
> +================ ==============================================
> +temp1_input Measured temperature (in millidegrees Celsius)
> +================ ==============================================
> +
> +Debugfs entries
> +----------------
> +
> +================ ====================================
> +fw_ver Firmware version of the retimer
> +health Health status (8 bits)
> + [0]: Heartbeat Okay (1'b1: OK)
> + [1]: Firmware loaded Okay (1'b1: OK)
This is not from the chip but arbitrarily assigned. It should be
reported in separate debugfs files (or all in a single file with
multiple lines and human readable information).
> + [7:2]: Reserved
> +================ ====================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 788be9ab5b73..492002ffd12c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17482,6 +17482,14 @@ F: fs/pstore/
> F: include/linux/pstore*
> K: \b(pstore|ramoops)
>
> +PT516XX HARDWARE MONITOR DRIVER
> +M: Cosmo Chou <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
> +F: Documentation/hwmon/pt516xx.rst
> +F: drivers/hwmon/pt516xx.c
> +
> PTP HARDWARE CLOCK SUPPORT
> M: Richard Cochran <[email protected]>
> L: [email protected]
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index cf27523eed5a..3965bec7774a 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1703,6 +1703,16 @@ source "drivers/hwmon/peci/Kconfig"
>
> source "drivers/hwmon/pmbus/Kconfig"
>
> +config SENSORS_PT516XX
> + tristate "Astera Labs PT516XX PCIe retimer hardware monitoring"
> + depends on I2C
> + help
> + If you say yes here you get support for temperature monitoring
> + on the Astera Labs PT516XX PCIe retimer.
> +
> + This driver can also be built as a module. If so, the module
> + will be called pt516xx.
> +
> config SENSORS_PWM_FAN
> tristate "PWM fan"
> depends on (PWM && OF) || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e84bd9685b5c..1942064cd4e9 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWERZ) += powerz.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> +obj-$(CONFIG_SENSORS_PT516XX) += pt516xx.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_SBTSI) += sbtsi_temp.o
> diff --git a/drivers/hwmon/pt516xx.c b/drivers/hwmon/pt516xx.c
> new file mode 100644
> index 000000000000..824798559fe1
> --- /dev/null
> +++ b/drivers/hwmon/pt516xx.c
> @@ -0,0 +1,648 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
> +
> +/** Main-micro-assisted access command codes */
This is not a proper kernel documentation. Either case, use C style comments
or C++ style comments, but not both.
> +// Wide register reads
> +#define ARIES_MM_RD_WIDE_REG_2B 0x1d
> +#define ARIES_MM_RD_WIDE_REG_3B 0x1e
> +#define ARIES_MM_RD_WIDE_REG_4B 0x1f
> +#define ARIES_MM_RD_WIDE_REG_5B 0x20
> +// Wide register writes
> +#define ARIES_MM_WR_WIDE_REG_2B 0x21
> +#define ARIES_MM_WR_WIDE_REG_3B 0x22
> +#define ARIES_MM_WR_WIDE_REG_4B 0x23
> +#define ARIES_MM_WR_WIDE_REG_5B 0x24
Please use
#define<space>NAME<tab>value
throughout, please. Also, at least ARIES_MM_WR_WIDE_REG_ defines are
not used. Please refrain from declaring unused defines.
And where does ARIES come from ?
> +
> +/** Temperature measurement constants */
> +// Aries current average temp ADC code CSR
> +#define ARIES_CURRENT_AVG_TEMP_ADC_CSR 0x42c
> +
> +// Main Micro Heartbeat reg
> +#define ARIES_MM_HEARTBEAT_ADDR 0x923
> +
> +/** Main SRAM */
> +// AL Main SRAM DMEM offset (A0)
> +#define AL_MAIN_SRAM_DMEM_OFFSET (64 * 1024)
> +// SRAM read command
> +#define AL_TG_RD_LOC_IND_SRAM 0x16
> +
> +/** Micros */
> +// Offset for main micro FW info
> +#define ARIES_MAIN_MICRO_FW_INFO (96 * 1024 - 128)
> +
> +/** Path Micro Members */
> +// FW Info (Major) offset location in struct
> +#define ARIES_MM_FW_VERSION_MAJOR 0
> +// FW Info (Minor) offset location in struct
> +#define ARIES_MM_FW_VERSION_MINOR 1
> +// FW Info (Build no.) offset location in struct
> +#define ARIES_MM_FW_VERSION_BUILD 2
> +
> +/** Offsets for MM assisted accesses */
> +// Legacy Address and Data registers (using wide registers)
> +// Reg offset to specify Address for MM assisted accesses
> +#define ARIES_MM_ASSIST_REG_ADDR_OFFSET 0xd99
> +// Reg offset to specify Command
> +#define ARIES_MM_ASSIST_CMD_OFFSET 0xd9d
> +
> +// New Address and Data registers (not using wide registers)
> +// Reg offset to MM SPARE 0 used specify Address[7:0]
> +#define ARIES_MM_ASSIST_SPARE_0_OFFSET 0xd9f
> +// Reg offset to MM SPARE 3 used specify Data Byte 0
> +#define ARIES_MM_ASSIST_SPARE_3_OFFSET 0xda2
> +
> +/** Misc block offsets */
> +// Device Load check register
> +#define ARIES_CODE_LOAD_REG 0x605
> +
> +/** Value indicating FW was loaded properly */
> +#define ARIES_LOAD_CODE 0xe
> +
> +/** EEPROM parameters */
> +// Time delay between checking MM status of EEPROM write (microseconds)
> +#define ARIES_MM_STATUS_TIME 5000
> +
> +#define ARIES_TEMP_CAL_CODE_DEFAULT 84
> +
> +/* Struct defining FW version loaded on an Aries device */
> +struct pt516xx_fw_ver {
> + u8 major; // FW version major release value
> + u8 minor; // FW version minor release value
> + u16 build; // FW version build release value
> +};
> +
> +/* Each client has this additional data */
> +struct pt516xx_data {
> + struct i2c_client *client;
> + struct dentry *debugfs;
> + struct pt516xx_fw_ver fw_ver;
> + struct mutex lock;
> + bool init_done;
> + bool pec_enable; // Enable PEC
> + bool code_load_okay; // indicate if code load reg value is expected
> + bool mm_heartbeat_okay; // indicate if Main Micro heartbeat is good
> + bool mm_wide_reg_valid; // MM assisted Wide Register access
> + u8 temp_cal_code_avg;
> +};
> +
> +static struct dentry *pt516xx_debugfs_dir;
> +
> +/*
> + * Write multiple data bytes to Aries over I2C
> + */
> +static int pt516xx_write_block_data(struct pt516xx_data *data, u32 address,
> + u8 len, u8 *val)
> +{
> + struct i2c_client *client = data->client;
> + int ret;
> + u8 remain_len = len;
> + u8 xfer_len, curr_len;
> + u8 buf[16];
> + u8 cmd = 0x0F; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end
> + u8 config = 0x40; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address
> +
> + if (data->pec_enable)
> + cmd |= 0x80;
> +
> + // If byte count is greater than 4, perform multiple iterations
Most of those comments are pointless. This is obvious from the code.
> + while (remain_len > 0) {
> + if (remain_len > 4) {
> + curr_len = 4;
> + remain_len -= 4;
> + } else {
> + curr_len = remain_len;
> + remain_len = 0;
> + }
> +
> + buf[0] = config | (curr_len - 1) << 1 | ((address >> 16) & 0x1);
> + buf[1] = (address >> 8) & 0xff;
> + buf[2] = address & 0xff;
> + memcpy(&buf[3], val, curr_len);
> +
> + xfer_len = 3 + curr_len;
> + ret = i2c_smbus_write_block_data(client, cmd, xfer_len, buf);
> + if (ret)
> + return ret;
> +
> + val += curr_len;
> + address += curr_len;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Read multiple data bytes from Aries over I2C
> + */
> +static int pt516xx_read_block_data(struct pt516xx_data *data, u32 address,
> + u8 len, u8 *val)
> +{
> + struct i2c_client *client = data->client;
> + int ret, tries;
> + u8 remain_len = len;
> + u8 curr_len;
> + u8 wbuf[16], rbuf[24];
> + u8 cmd = 0x08; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end
> + u8 config = 0x00; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address
> +
> + if (data->pec_enable)
> + cmd |= 0x80;
> +
> + // If byte count is greater than 16, perform multiple iterations
> + while (remain_len > 0) {
> + if (remain_len > 16) {
> + curr_len = 16;
> + remain_len -= 16;
> + } else {
> + curr_len = remain_len;
> + remain_len = 0;
> + }
> +
> + wbuf[0] = config | (curr_len - 1) << 1 |
> + ((address >> 16) & 0x1);
> + wbuf[1] = (address >> 8) & 0xff;
> + wbuf[2] = address & 0xff;
> +
> + // Perform read operation
> + for (tries = 0; tries < 3; tries++) {
> + ret = i2c_smbus_write_block_data(client, (cmd | 0x2), 3,
> + wbuf);
> + if (ret)
> + return ret;
> +
> + ret = i2c_smbus_read_block_data(client, (cmd | 0x1),
> + rbuf);
> + if (ret == curr_len)
> + break;
> + }
> + if (tries >= 3)
> + return -ENODATA;
> +
> + memcpy(val, rbuf, curr_len);
> + val += curr_len;
> + address += curr_len;
> + }
> +
> + return 0;
> +}
> +
> +static int pt516xx_read_wide_reg(struct pt516xx_data *data, u32 address,
> + u8 width, u8 *val)
> +{
> + int ret, tries;
> + u8 buf[8];
> + u8 status;
> +
> + if (data->mm_wide_reg_valid) {
> + // Write address (3 bytes)
> + buf[0] = address & 0xff;
> + buf[1] = (address >> 8) & 0xff;
> + buf[2] = (address >> 16) & 0x1;
> + ret = pt516xx_write_block_data(
> + data, ARIES_MM_ASSIST_SPARE_0_OFFSET, 3, buf);
> + if (ret)
> + return ret;
> +
> + // Set command based on width
> + switch (width) {
> + case 2:
> + buf[0] = ARIES_MM_RD_WIDE_REG_2B;
> + break;
> + case 3:
> + buf[0] = ARIES_MM_RD_WIDE_REG_3B;
> + break;
> + case 4:
> + buf[0] = ARIES_MM_RD_WIDE_REG_4B;
> + break;
> + case 5:
> + buf[0] = ARIES_MM_RD_WIDE_REG_5B;
> + break;
> + default:
> + return -EINVAL;
> + }
> + ret = pt516xx_write_block_data(data, ARIES_MM_ASSIST_CMD_OFFSET,
> + 1, buf);
> + if (ret)
> + return ret;
> +
> + // Check access status
> + status = 0xff;
> + for (tries = 0; tries < 100; tries++) {
> + ret = pt516xx_read_block_data(
> + data, ARIES_MM_ASSIST_CMD_OFFSET, 1, &status);
> + if (ret)
> + return ret;
> +
> + if (status == 0)
> + break;
> +
> + usleep_range(ARIES_MM_STATUS_TIME,
> + ARIES_MM_STATUS_TIME + 1000);
> + }
> + if (status != 0)
> + return -ETIMEDOUT;
> +
> + // Read N bytes of data based on width
> + ret = pt516xx_read_block_data(
> + data, ARIES_MM_ASSIST_SPARE_3_OFFSET, width, val);
> + if (ret)
> + return ret;
> + } else {
> + return pt516xx_read_block_data(data, address, width, val);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Read multiple (up to eight) data bytes from micro SRAM over I2C
> + */
> +static int
> +pt516xx_read_block_data_main_micro_indirect(struct pt516xx_data *data,
> + u32 address, u8 len, u8 *val)
> +{
> + int ret, tries;
> + u8 buf[8];
> + u8 i, status;
> + u32 uind_offs = ARIES_MM_ASSIST_REG_ADDR_OFFSET;
> + u32 eeprom_base, eeprom_addr;
> +
> + // No multi-byte indirect support here. Hence read a byte at a time
> + eeprom_base = address - AL_MAIN_SRAM_DMEM_OFFSET;
> + for (i = 0; i < len; i++) {
> + // Write eeprom addr
> + eeprom_addr = eeprom_base + i;
> + buf[0] = eeprom_addr & 0xff;
> + buf[1] = (eeprom_addr >> 8) & 0xff;
> + buf[2] = (eeprom_addr >> 16) & 0xff;
> + ret = pt516xx_write_block_data(data, uind_offs, 3, buf);
> + if (ret)
> + return ret;
> +
> + // Write eeprom cmd
> + buf[0] = AL_TG_RD_LOC_IND_SRAM;
> + ret = pt516xx_write_block_data(data, uind_offs + 4, 1, buf);
> + if (ret)
> + return ret;
> +
> + // Test successful access
> + status = 0xff;
> + for (tries = 0; tries < 255; tries++) {
> + ret = pt516xx_read_block_data(data, uind_offs + 4, 1,
> + &status);
> + if (ret)
> + return ret;
> +
> + if (status == 0)
> + break;
> + }
> + if (status != 0)
> + return -ETIMEDOUT;
> +
> + ret = pt516xx_read_block_data(data, uind_offs + 3, 1, buf);
> + if (ret)
> + return ret;
> +
> + val[i] = buf[0];
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Check the status of firmware
> + */
> +static int pt516xx_fwsts_check(struct pt516xx_data *data)
> +{
> + int ret, tries;
> + u8 buf[8];
> + u8 heartbeat, major = 0, minor = 0;
> + u16 build = 0;
> + bool hb_changed = false;
> +
> + // Read Code Load reg
> + ret = pt516xx_read_block_data(data, ARIES_CODE_LOAD_REG, 1, buf);
> + if (ret)
> + return ret;
> +
> + if (buf[0] < ARIES_LOAD_CODE) {
> + dev_warn(
> + &data->client->dev,
> + "Code Load reg unexpected. Not all modules are loaded %x\n",
> + buf[0]);
> + data->code_load_okay = false;
> + } else {
> + data->code_load_okay = true;
> + }
> +
> + // Check Main Micro heartbeat
> + // If heartbeat value does not change for 100 tries, no MM heartbeat
> + // Else heartbeat present even if one value changes
> + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, buf);
> + if (ret)
> + return ret;
> +
> + heartbeat = buf[0];
> + for (tries = 0; tries < 100; tries++) {
> + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1,
> + buf);
> + if (ret)
> + return ret;
> +
> + if (buf[0] != heartbeat) {
> + hb_changed = true;
> + break;
> + }
> + }
> + data->mm_heartbeat_okay = hb_changed;
> +
> + // Read FW version
> + // If heartbeat not there, set default FW values to 0.0.0
> + // and return success
> + if (data->mm_heartbeat_okay) {
> + // Get FW version (major)
> + ret = pt516xx_read_block_data_main_micro_indirect(
> + data,
> + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MAJOR, 1,
> + &major);
> + if (ret)
> + return ret;
> +
> + // Get FW version (minor)
> + ret = pt516xx_read_block_data_main_micro_indirect(
> + data,
> + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MINOR, 1,
> + &minor);
> + if (ret)
> + return ret;
> +
> + // Get FW version (build)
> + ret = pt516xx_read_block_data_main_micro_indirect(
> + data,
> + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_BUILD, 2,
> + (u8 *)&build);
> + if (ret)
> + return ret;
> + } else {
> + dev_warn(&data->client->dev, "No Main Micro Heartbeat\n");
This and other messages above would create a lot of noise if persistent
since the function is called repeatedly.
> + }
> + data->fw_ver.major = major;
> + data->fw_ver.minor = minor;
> + data->fw_ver.build = build;
> +
> + return 0;
> +}
> +
> +static int pt516xx_fw_is_at_least(struct pt516xx_data *data, u8 major, u8 minor,
> + u16 build)
> +{
> + u32 ver = major << 24 | minor << 16 | build;
> + u32 curr_ver = data->fw_ver.major << 24 | data->fw_ver.minor << 16 |
> + data->fw_ver.build;
> +
> + if (curr_ver >= ver)
> + return true;
> +
> + return false;
> +}
> +
> +static int pt516xx_init_dev(struct pt516xx_data *data)
> +{
> + int ret;
> +
> + mutex_lock(&data->lock);
> + ret = pt516xx_fwsts_check(data);
> + mutex_unlock(&data->lock);
> + if (ret)
> + return ret;
> +
> + dev_info(&data->client->dev, "fw_ver: %u.%u.%u\n", data->fw_ver.major,
> + data->fw_ver.minor, data->fw_ver.build);
> +
Please no such noise
> + if (pt516xx_fw_is_at_least(data, 2, 2, 0))
> + data->mm_wide_reg_valid = true;
> +
> + data->temp_cal_code_avg = ARIES_TEMP_CAL_CODE_DEFAULT;
What is the point of this ? temp_cal_code_avg will never be anything else.
> + data->init_done = true;
> +
> + return 0;
> +}
> +
> +static int pt516xx_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct pt516xx_data *data = dev_get_drvdata(dev);
> + int ret = 0;
> + long adc_code = 0;
> +
This assumes a specific endianness which isn't given. I am quite sure that the
code as written won't work in a big endian system. Also, "long" may be 4 or 8 byte
depending on the CPU architecture.
> + if (!data->init_done) {
> + ret = pt516xx_init_dev(data);
> + if (ret) {
> + dev_err(dev, "pt516xx_init_dev failed %d\n", ret);
> + return ret;
> + }
> + }
This is really the wrong place for this code. It should be checked and initialized
in the probe function, and probe should bail out if it fails.
Also, the only reason to call pt516xx_init_dev() seems to be to determine
if "wide reegister access" is supported. If it isn't, pt516xx_read_block_data()
is used. Why not call pt516xx_read_block_data() below directly and not bother
with wide register access ?
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + mutex_lock(&data->lock);
> + ret = pt516xx_read_wide_reg(data,
> + ARIES_CURRENT_AVG_TEMP_ADC_CSR, 4,
> + (u8 *)&adc_code);
> + mutex_unlock(&data->lock);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + if (ret) {
> + dev_err(dev, "Read adc_code failed %d\n", ret);
> + return ret;
> + }
> + if (adc_code == 0 || adc_code >= 0x3ff) {
> + dev_err(dev, "Invalid adc_code %lx\n", adc_code);
> + return -EINVAL;
I guess this is supposed to cover an error return from pt516xx_read_wide_reg()
returned an error, assuming that adc_code was not overwritten in that case.
This is inappropriate. Error returns from pt516xx_read_wide_reg() should be
handled explicitly (and don't indicate "Invalid argument").
> + }
> +
The above will create logging noise if the chip has a problem. Please make it dev_dbg.
> + *val = 110000 + ((adc_code - (data->temp_cal_code_avg + 250)) * -320);
> +
> + return 0;
> +}
> +
> +static umode_t pt516xx_is_visible(const void *data,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel)
> +{
> + switch (attr) {
> + case hwmon_temp_input:
> + return 0444;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static const struct hwmon_channel_info *pt516xx_info[] = {
> + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> + NULL
> +};
> +
> +static const struct hwmon_ops pt516xx_hwmon_ops = {
> + .is_visible = pt516xx_is_visible,
> + .read = pt516xx_read,
> +};
> +
> +static const struct hwmon_chip_info pt516xx_chip_info = {
> + .ops = &pt516xx_hwmon_ops,
> + .info = pt516xx_info,
> +};
> +
> +static ssize_t pt516xx_debugfs_read_fw_ver(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct pt516xx_data *data = file->private_data;
> + int ret;
> + char ver[32];
> +
> + mutex_lock(&data->lock);
> + ret = pt516xx_fwsts_check(data);
> + mutex_unlock(&data->lock);
What is the point of (re-)reading the firmware version repeatedly ?
> + if (ret)
> + return ret;
> +
> + ret = snprintf(ver, sizeof(ver), "%u.%u.%u\n", data->fw_ver.major,
> + data->fw_ver.minor, data->fw_ver.build);
> + if (ret < 0)
> + return ret;
> +
> + return simple_read_from_buffer(buf, count, ppos, ver, ret + 1);
> +}
> +
> +static const struct file_operations pt516xx_debugfs_ops_fw_ver = {
> + .read = pt516xx_debugfs_read_fw_ver,
> + .open = simple_open,
> +};
> +
> +static ssize_t pt516xx_debugfs_read_health(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct pt516xx_data *data = file->private_data;
> + int ret;
> + u8 status = 0;
> + char health[8];
> +
> + mutex_lock(&data->lock);
> + ret = pt516xx_fwsts_check(data);
> + mutex_unlock(&data->lock);
> + if (ret == 0) {
> + status |= data->mm_heartbeat_okay ? 1 : 0; // bit0
> + status |= data->code_load_okay ? 2 : 0; // bit1
Those should really be separate debugfs files.
> + }
> +
> + ret = snprintf(health, sizeof(health), "0x%02x\n", status);
> + if (ret < 0)
> + return ret;
> +
> + return simple_read_from_buffer(buf, count, ppos, health, ret + 1);
> +}
> +
> +static const struct file_operations pt516xx_debugfs_ops_health = {
> + .read = pt516xx_debugfs_read_health,
> + .open = simple_open,
> +};
> +
> +static int pt516xx_init_debugfs(struct pt516xx_data *data)
> +{
> + if (!pt516xx_debugfs_dir)
> + return -ENOENT;
> +
debugfs functions handle this situation, and return values therefore do not
need to be checked.
> + data->debugfs = debugfs_create_dir(dev_name(&data->client->dev),
> + pt516xx_debugfs_dir);
> + if (IS_ERR_OR_NULL(data->debugfs))
> + return -ENOENT;
> +
Again, unnecessary.
> + debugfs_create_file("fw_ver", 0444, data->debugfs, data,
> + &pt516xx_debugfs_ops_fw_ver);
> +
> + debugfs_create_file("health", 0444, data->debugfs, data,
> + &pt516xx_debugfs_ops_health);
> +
> + return 0;
> +}
> +
> +static int pt516xx_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct device *hwmon_dev;
> + struct pt516xx_data *data;
> +
> + data = devm_kzalloc(dev, sizeof(struct pt516xx_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> + mutex_init(&data->lock);
> + pt516xx_init_dev(data);
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(
> + dev, client->name, data, &pt516xx_chip_info, NULL);
> +
> + if (pt516xx_init_debugfs(data))
> + dev_warn(dev, "Failed to register debugfs\n");
debugfs should fail silently.
debugfs files are not removed if a single device is unloaded. Did you check
what happens in this case ?
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id pt516xx_id[] = {
> + { "pt5161l", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, pt516xx_id);
> +
> +static const struct of_device_id __maybe_unused pt516xx_of_match[] = {
> + { .compatible = "asteralabs,pt5161l" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, pt516xx_of_match);
> +
> +static struct i2c_driver pt516xx_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = "pt516xx",
> + .of_match_table = of_match_ptr(pt516xx_of_match),
This means the driver won't instantiate on an ACPI system if devicetree
support is disabled. Is this intentional ?
> + },
> + .probe = pt516xx_probe,
> + .id_table = pt516xx_id,
> +};
> +
> +module_i2c_driver(pt516xx_driver);
> +
> +static int __init pt516xx_core_init(void)
> +{
> + pt516xx_debugfs_dir = debugfs_create_dir("pt516xx", NULL);
> + if (IS_ERR(pt516xx_debugfs_dir))
> + pt516xx_debugfs_dir = NULL;
> +
> + return 0;
> +}
> +
> +static void __exit pt516xx_core_exit(void)
> +{
> + debugfs_remove_recursive(pt516xx_debugfs_dir);
> +}
> +
> +module_init(pt516xx_core_init);
> +module_exit(pt516xx_core_exit);
> +
> +MODULE_AUTHOR("Cosmo Chou <[email protected]>");
> +MODULE_DESCRIPTION("Hwmon driver for Astera Labs Aries PCIe retimer");
> +MODULE_LICENSE("GPL");
On Tue, Dec 05, 2023 at 03:47:21PM +0800, Cosmo Chou wrote:
> Add vendor prefix for Astera Labs, Inc.
> https://www.asteralabs.com
>
> Signed-off-by: Cosmo Chou <[email protected]>
Acked-by: Conor Dooley <[email protected]>
Cheers,
Conor.
On Tue, Dec 05, 2023 at 03:47:22PM +0800, Cosmo Chou wrote:
> Add dt-bindings for pt516xx temperature monitor.
>
> Signed-off-by: Cosmo Chou <[email protected]>
This can just go into trivial-devices.yaml, no?
Thanks,
Conor.
> ---
> .../bindings/hwmon/asteralabs,pt516xx.yaml | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
> new file mode 100644
> index 000000000000..5700d4c91a0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
> @@ -0,0 +1,36 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/asteralabs,pt516xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: PT5161L hwmon sensor
> +
> +maintainers:
> + - Cosmo Chou <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - asteralabs,pt5161l
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + sensor@24 {
> + compatible = "asteralabs,pt5161l";
> + reg = <0x24>;
> + };
> + };
> --
> 2.34.1
>
Hi Cosmo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v6.7-rc4 next-20231205]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cosmo-Chou/dt-bindings-vendor-prefixes-add-asteralabs/20231205-155020
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20231205074723.3546295-4-chou.cosmo%40gmail.com
patch subject: [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer
reproduce: (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> Documentation/hwmon/pt516xx.rst:4: WARNING: Title underline too short.
vim +4 Documentation/hwmon/pt516xx.rst
2
3 Kernel driver pt516xx
> 4 ====================
5
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
OK. I will change to trivial-devices.yaml.
Thanks
Cosmo
On Tue, 5 Dec 2023 16:46:42 +0000, Conor Dooley <[email protected]> wrote:
>
> On Tue, Dec 05, 2023 at 03:47:22PM +0800, Cosmo Chou wrote:
> > Add dt-bindings for pt516xx temperature monitor.
> >
> > Signed-off-by: Cosmo Chou <[email protected]>
>
> This can just go into trivial-devices.yaml, no?
>
> Thanks,
> Conor.
>
> > ---
> > .../bindings/hwmon/asteralabs,pt516xx.yaml | 36 +++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
> > new file mode 100644
> > index 000000000000..5700d4c91a0d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
> > @@ -0,0 +1,36 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/asteralabs,pt516xx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: PT5161L hwmon sensor
> > +
> > +maintainers:
> > + - Cosmo Chou <[email protected]>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - asteralabs,pt5161l
> > +
> > + reg:
> > + maxItems: 1
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + sensor@24 {
> > + compatible = "asteralabs,pt5161l";
> > + reg = <0x24>;
> > + };
> > + };
> > --
> > 2.34.1
> >
On 12/5/2023 07:55:50 -0800 Guenter Roeck <[email protected]> wrote:
>
> On 12/4/23 23:47, Cosmo Chou wrote:
> > This driver implements support for temperature monitoring of Astera Labs
> > PT5161L series PCIe retimer chips.
> >
> > This driver implementation originates from the CSDK available at
> > Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
> > The communication protocol utilized is based on the I2C/SMBus standard.
> >
> > Signed-off-by: Cosmo Chou <[email protected]>
> > ---
> > Documentation/hwmon/index.rst | 1 +
> > Documentation/hwmon/pt516xx.rst | 48 +++
> > MAINTAINERS | 8 +
> > drivers/hwmon/Kconfig | 10 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++++++++++++++++
> > 6 files changed, 716 insertions(+)
> > create mode 100644 Documentation/hwmon/pt516xx.rst
> > create mode 100644 drivers/hwmon/pt516xx.c
> >
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index 72f4e6065bae..2c4df18db663 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -181,6 +181,7 @@ Hardware Monitoring Kernel Drivers
> > pmbus
> > powerz
> > powr1220
> > + pt516xx
> > pxe1610
> > pwm-fan
> > q54sj108a2
> > diff --git a/Documentation/hwmon/pt516xx.rst b/Documentation/hwmon/pt516xx.rst
> > new file mode 100644
> > index 000000000000..945194f9a804
> > --- /dev/null
> > +++ b/Documentation/hwmon/pt516xx.rst
> > @@ -0,0 +1,48 @@
> > +.. SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +Kernel driver pt516xx
> > +====================
> > +
> > +Supported chips:
> > +
> > + * Astera Labs PT5161L
> > +
> > + Prefix: 'pt5161l'
> > +
>
> This should be the driver name and be used for function/variable prefixes.
> No idea where XX comes into play. The driver for sure won't support
> PT516[0-9][A-Z], and should not suggest that it does.
>
OK, I will correct it to pt5161l.
> > + Addresses: I2C 0x24
> > +
> > + Datasheet: http://www.asteralabs.com/wp-content/uploads/2021/03/Astera_Labs_PT5161L_Product_Brief.pdf
> > +
> > +Authors: Cosmo Chou <[email protected]>
> > +
> > +Description
> > +-----------
> > +
> > +This driver implements support for temperature monitoring of Astera Labs
> > +PT5161L series PCIe retimer chips.
> > +
> > +This driver implementation originates from the CSDK available at
> > +https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
> > +The communication protocol utilized is based on the I2C/SMBus standard.
> > +
>
> That strongly suggests that the code has a copyright and license associated
> with it. None of it is mentioned here.
>
It looks like the base code (the link above) declared GPL2.0+ and Apache2.0.
There is already a declaration in the pt516xx.c: (line 1)
// SPDX-License-Identifier: GPL-2.0-or-later
Could you give some advice about adding the copyright and license? Thanks.
> > +For more detailed information and specific implementation details, it is
> > +recommended to refer to the CSDK source code available at the provided GitHub
> > +link.
> > +
>
> Linux kernel drivers should be self contained. It is fine to reference documentation,
> but not out-of-tree source code.
>
OK. I will remove this paragraph ("For more detailed information...")
> > +Sysfs entries
> > +----------------
> > +
> > +================ ==============================================
> > +temp1_input Measured temperature (in millidegrees Celsius)
> > +================ ==============================================
> > +
> > +Debugfs entries
> > +----------------
> > +
> > +================ ====================================
> > +fw_ver Firmware version of the retimer
> > +health Health status (8 bits)
> > + [0]: Heartbeat Okay (1'b1: OK)
> > + [1]: Firmware loaded Okay (1'b1: OK)
>
> This is not from the chip but arbitrarily assigned. It should be
> reported in separate debugfs files (or all in a single file with
> multiple lines and human readable information).
>
OK. I will separate it to "fw_load_status" and "heartbeat_status".
> > + [7:2]: Reserved
> > +================ ====================================
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 788be9ab5b73..492002ffd12c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17482,6 +17482,14 @@ F: fs/pstore/
> > F: include/linux/pstore*
> > K: \b(pstore|ramoops)
> >
> > +PT516XX HARDWARE MONITOR DRIVER
> > +M: Cosmo Chou <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
> > +F: Documentation/hwmon/pt516xx.rst
> > +F: drivers/hwmon/pt516xx.c
> > +
> > PTP HARDWARE CLOCK SUPPORT
> > M: Richard Cochran <[email protected]>
> > L: [email protected]
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index cf27523eed5a..3965bec7774a 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1703,6 +1703,16 @@ source "drivers/hwmon/peci/Kconfig"
> >
> > source "drivers/hwmon/pmbus/Kconfig"
> >
> > +config SENSORS_PT516XX
> > + tristate "Astera Labs PT516XX PCIe retimer hardware monitoring"
> > + depends on I2C
> > + help
> > + If you say yes here you get support for temperature monitoring
> > + on the Astera Labs PT516XX PCIe retimer.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called pt516xx.
> > +
> > config SENSORS_PWM_FAN
> > tristate "PWM fan"
> > depends on (PWM && OF) || COMPILE_TEST
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e84bd9685b5c..1942064cd4e9 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -179,6 +179,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWERZ) += powerz.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > +obj-$(CONFIG_SENSORS_PT516XX) += pt516xx.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_SBTSI) += sbtsi_temp.o
> > diff --git a/drivers/hwmon/pt516xx.c b/drivers/hwmon/pt516xx.c
> > new file mode 100644
> > index 000000000000..824798559fe1
> > --- /dev/null
> > +++ b/drivers/hwmon/pt516xx.c
> > @@ -0,0 +1,648 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of_device.h>
> > +
> > +/** Main-micro-assisted access command codes */
>
> This is not a proper kernel documentation. Either case, use C style comments
> or C++ style comments, but not both.
>
Got it. I will correct the style of comments.
> > +// Wide register reads
> > +#define ARIES_MM_RD_WIDE_REG_2B 0x1d
> > +#define ARIES_MM_RD_WIDE_REG_3B 0x1e
> > +#define ARIES_MM_RD_WIDE_REG_4B 0x1f
> > +#define ARIES_MM_RD_WIDE_REG_5B 0x20
> > +// Wide register writes
> > +#define ARIES_MM_WR_WIDE_REG_2B 0x21
> > +#define ARIES_MM_WR_WIDE_REG_3B 0x22
> > +#define ARIES_MM_WR_WIDE_REG_4B 0x23
> > +#define ARIES_MM_WR_WIDE_REG_5B 0x24
>
> Please use
>
> #define<space>NAME<tab>value
>
Got it.
> throughout, please. Also, at least ARIES_MM_WR_WIDE_REG_ defines are
> not used. Please refrain from declaring unused defines.
>
Oops... I will remove the unused code.
> And where does ARIES come from ?
>
It's the SoC name of the PCIe retimer.
> > +
> > +/** Temperature measurement constants */
> > +// Aries current average temp ADC code CSR
> > +#define ARIES_CURRENT_AVG_TEMP_ADC_CSR 0x42c
> > +
> > +// Main Micro Heartbeat reg
> > +#define ARIES_MM_HEARTBEAT_ADDR 0x923
> > +
> > +/** Main SRAM */
> > +// AL Main SRAM DMEM offset (A0)
> > +#define AL_MAIN_SRAM_DMEM_OFFSET (64 * 1024)
> > +// SRAM read command
> > +#define AL_TG_RD_LOC_IND_SRAM 0x16
> > +
> > +/** Micros */
> > +// Offset for main micro FW info
> > +#define ARIES_MAIN_MICRO_FW_INFO (96 * 1024 - 128)
> > +
> > +/** Path Micro Members */
> > +// FW Info (Major) offset location in struct
> > +#define ARIES_MM_FW_VERSION_MAJOR 0
> > +// FW Info (Minor) offset location in struct
> > +#define ARIES_MM_FW_VERSION_MINOR 1
> > +// FW Info (Build no.) offset location in struct
> > +#define ARIES_MM_FW_VERSION_BUILD 2
> > +
> > +/** Offsets for MM assisted accesses */
> > +// Legacy Address and Data registers (using wide registers)
> > +// Reg offset to specify Address for MM assisted accesses
> > +#define ARIES_MM_ASSIST_REG_ADDR_OFFSET 0xd99
> > +// Reg offset to specify Command
> > +#define ARIES_MM_ASSIST_CMD_OFFSET 0xd9d
> > +
> > +// New Address and Data registers (not using wide registers)
> > +// Reg offset to MM SPARE 0 used specify Address[7:0]
> > +#define ARIES_MM_ASSIST_SPARE_0_OFFSET 0xd9f
> > +// Reg offset to MM SPARE 3 used specify Data Byte 0
> > +#define ARIES_MM_ASSIST_SPARE_3_OFFSET 0xda2
> > +
> > +/** Misc block offsets */
> > +// Device Load check register
> > +#define ARIES_CODE_LOAD_REG 0x605
> > +
> > +/** Value indicating FW was loaded properly */
> > +#define ARIES_LOAD_CODE 0xe
> > +
> > +/** EEPROM parameters */
> > +// Time delay between checking MM status of EEPROM write (microseconds)
> > +#define ARIES_MM_STATUS_TIME 5000
> > +
> > +#define ARIES_TEMP_CAL_CODE_DEFAULT 84
> > +
> > +/* Struct defining FW version loaded on an Aries device */
> > +struct pt516xx_fw_ver {
> > + u8 major; // FW version major release value
> > + u8 minor; // FW version minor release value
> > + u16 build; // FW version build release value
> > +};
> > +
> > +/* Each client has this additional data */
> > +struct pt516xx_data {
> > + struct i2c_client *client;
> > + struct dentry *debugfs;
> > + struct pt516xx_fw_ver fw_ver;
> > + struct mutex lock;
> > + bool init_done;
> > + bool pec_enable; // Enable PEC
> > + bool code_load_okay; // indicate if code load reg value is expected
> > + bool mm_heartbeat_okay; // indicate if Main Micro heartbeat is good
> > + bool mm_wide_reg_valid; // MM assisted Wide Register access
> > + u8 temp_cal_code_avg;
> > +};
> > +
> > +static struct dentry *pt516xx_debugfs_dir;
> > +
> > +/*
> > + * Write multiple data bytes to Aries over I2C
> > + */
> > +static int pt516xx_write_block_data(struct pt516xx_data *data, u32 address,
> > + u8 len, u8 *val)
> > +{
> > + struct i2c_client *client = data->client;
> > + int ret;
> > + u8 remain_len = len;
> > + u8 xfer_len, curr_len;
> > + u8 buf[16];
> > + u8 cmd = 0x0F; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end
> > + u8 config = 0x40; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address
> > +
> > + if (data->pec_enable)
> > + cmd |= 0x80;
> > +
> > + // If byte count is greater than 4, perform multiple iterations
>
> Most of those comments are pointless. This is obvious from the code.
>
OK. I will reduce the pointless comments.
> > + while (remain_len > 0) {
> > + if (remain_len > 4) {
> > + curr_len = 4;
> > + remain_len -= 4;
> > + } else {
> > + curr_len = remain_len;
> > + remain_len = 0;
> > + }
> > +
> > + buf[0] = config | (curr_len - 1) << 1 | ((address >> 16) & 0x1);
> > + buf[1] = (address >> 8) & 0xff;
> > + buf[2] = address & 0xff;
> > + memcpy(&buf[3], val, curr_len);
> > +
> > + xfer_len = 3 + curr_len;
> > + ret = i2c_smbus_write_block_data(client, cmd, xfer_len, buf);
> > + if (ret)
> > + return ret;
> > +
> > + val += curr_len;
> > + address += curr_len;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Read multiple data bytes from Aries over I2C
> > + */
> > +static int pt516xx_read_block_data(struct pt516xx_data *data, u32 address,
> > + u8 len, u8 *val)
> > +{
> > + struct i2c_client *client = data->client;
> > + int ret, tries;
> > + u8 remain_len = len;
> > + u8 curr_len;
> > + u8 wbuf[16], rbuf[24];
> > + u8 cmd = 0x08; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end
> > + u8 config = 0x00; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address
> > +
> > + if (data->pec_enable)
> > + cmd |= 0x80;
> > +
> > + // If byte count is greater than 16, perform multiple iterations
> > + while (remain_len > 0) {
> > + if (remain_len > 16) {
> > + curr_len = 16;
> > + remain_len -= 16;
> > + } else {
> > + curr_len = remain_len;
> > + remain_len = 0;
> > + }
> > +
> > + wbuf[0] = config | (curr_len - 1) << 1 |
> > + ((address >> 16) & 0x1);
> > + wbuf[1] = (address >> 8) & 0xff;
> > + wbuf[2] = address & 0xff;
> > +
> > + // Perform read operation
> > + for (tries = 0; tries < 3; tries++) {
> > + ret = i2c_smbus_write_block_data(client, (cmd | 0x2), 3,
> > + wbuf);
> > + if (ret)
> > + return ret;
> > +
> > + ret = i2c_smbus_read_block_data(client, (cmd | 0x1),
> > + rbuf);
> > + if (ret == curr_len)
> > + break;
> > + }
> > + if (tries >= 3)
> > + return -ENODATA;
> > +
> > + memcpy(val, rbuf, curr_len);
> > + val += curr_len;
> > + address += curr_len;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int pt516xx_read_wide_reg(struct pt516xx_data *data, u32 address,
> > + u8 width, u8 *val)
> > +{
> > + int ret, tries;
> > + u8 buf[8];
> > + u8 status;
> > +
> > + if (data->mm_wide_reg_valid) {
> > + // Write address (3 bytes)
> > + buf[0] = address & 0xff;
> > + buf[1] = (address >> 8) & 0xff;
> > + buf[2] = (address >> 16) & 0x1;
> > + ret = pt516xx_write_block_data(
> > + data, ARIES_MM_ASSIST_SPARE_0_OFFSET, 3, buf);
> > + if (ret)
> > + return ret;
> > +
> > + // Set command based on width
> > + switch (width) {
> > + case 2:
> > + buf[0] = ARIES_MM_RD_WIDE_REG_2B;
> > + break;
> > + case 3:
> > + buf[0] = ARIES_MM_RD_WIDE_REG_3B;
> > + break;
> > + case 4:
> > + buf[0] = ARIES_MM_RD_WIDE_REG_4B;
> > + break;
> > + case 5:
> > + buf[0] = ARIES_MM_RD_WIDE_REG_5B;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + ret = pt516xx_write_block_data(data, ARIES_MM_ASSIST_CMD_OFFSET,
> > + 1, buf);
> > + if (ret)
> > + return ret;
> > +
> > + // Check access status
> > + status = 0xff;
> > + for (tries = 0; tries < 100; tries++) {
> > + ret = pt516xx_read_block_data(
> > + data, ARIES_MM_ASSIST_CMD_OFFSET, 1, &status);
> > + if (ret)
> > + return ret;
> > +
> > + if (status == 0)
> > + break;
> > +
> > + usleep_range(ARIES_MM_STATUS_TIME,
> > + ARIES_MM_STATUS_TIME + 1000);
> > + }
> > + if (status != 0)
> > + return -ETIMEDOUT;
> > +
> > + // Read N bytes of data based on width
> > + ret = pt516xx_read_block_data(
> > + data, ARIES_MM_ASSIST_SPARE_3_OFFSET, width, val);
> > + if (ret)
> > + return ret;
> > + } else {
> > + return pt516xx_read_block_data(data, address, width, val);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Read multiple (up to eight) data bytes from micro SRAM over I2C
> > + */
> > +static int
> > +pt516xx_read_block_data_main_micro_indirect(struct pt516xx_data *data,
> > + u32 address, u8 len, u8 *val)
> > +{
> > + int ret, tries;
> > + u8 buf[8];
> > + u8 i, status;
> > + u32 uind_offs = ARIES_MM_ASSIST_REG_ADDR_OFFSET;
> > + u32 eeprom_base, eeprom_addr;
> > +
> > + // No multi-byte indirect support here. Hence read a byte at a time
> > + eeprom_base = address - AL_MAIN_SRAM_DMEM_OFFSET;
> > + for (i = 0; i < len; i++) {
> > + // Write eeprom addr
> > + eeprom_addr = eeprom_base + i;
> > + buf[0] = eeprom_addr & 0xff;
> > + buf[1] = (eeprom_addr >> 8) & 0xff;
> > + buf[2] = (eeprom_addr >> 16) & 0xff;
> > + ret = pt516xx_write_block_data(data, uind_offs, 3, buf);
> > + if (ret)
> > + return ret;
> > +
> > + // Write eeprom cmd
> > + buf[0] = AL_TG_RD_LOC_IND_SRAM;
> > + ret = pt516xx_write_block_data(data, uind_offs + 4, 1, buf);
> > + if (ret)
> > + return ret;
> > +
> > + // Test successful access
> > + status = 0xff;
> > + for (tries = 0; tries < 255; tries++) {
> > + ret = pt516xx_read_block_data(data, uind_offs + 4, 1,
> > + &status);
> > + if (ret)
> > + return ret;
> > +
> > + if (status == 0)
> > + break;
> > + }
> > + if (status != 0)
> > + return -ETIMEDOUT;
> > +
> > + ret = pt516xx_read_block_data(data, uind_offs + 3, 1, buf);
> > + if (ret)
> > + return ret;
> > +
> > + val[i] = buf[0];
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Check the status of firmware
> > + */
> > +static int pt516xx_fwsts_check(struct pt516xx_data *data)
> > +{
> > + int ret, tries;
> > + u8 buf[8];
> > + u8 heartbeat, major = 0, minor = 0;
> > + u16 build = 0;
> > + bool hb_changed = false;
> > +
> > + // Read Code Load reg
> > + ret = pt516xx_read_block_data(data, ARIES_CODE_LOAD_REG, 1, buf);
> > + if (ret)
> > + return ret;
> > +
> > + if (buf[0] < ARIES_LOAD_CODE) {
> > + dev_warn(
> > + &data->client->dev,
> > + "Code Load reg unexpected. Not all modules are loaded %x\n",
> > + buf[0]);
> > + data->code_load_okay = false;
> > + } else {
> > + data->code_load_okay = true;
> > + }
> > +
> > + // Check Main Micro heartbeat
> > + // If heartbeat value does not change for 100 tries, no MM heartbeat
> > + // Else heartbeat present even if one value changes
> > + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, buf);
> > + if (ret)
> > + return ret;
> > +
> > + heartbeat = buf[0];
> > + for (tries = 0; tries < 100; tries++) {
> > + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1,
> > + buf);
> > + if (ret)
> > + return ret;
> > +
> > + if (buf[0] != heartbeat) {
> > + hb_changed = true;
> > + break;
> > + }
> > + }
> > + data->mm_heartbeat_okay = hb_changed;
> > +
> > + // Read FW version
> > + // If heartbeat not there, set default FW values to 0.0.0
> > + // and return success
> > + if (data->mm_heartbeat_okay) {
> > + // Get FW version (major)
> > + ret = pt516xx_read_block_data_main_micro_indirect(
> > + data,
> > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MAJOR, 1,
> > + &major);
> > + if (ret)
> > + return ret;
> > +
> > + // Get FW version (minor)
> > + ret = pt516xx_read_block_data_main_micro_indirect(
> > + data,
> > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MINOR, 1,
> > + &minor);
> > + if (ret)
> > + return ret;
> > +
> > + // Get FW version (build)
> > + ret = pt516xx_read_block_data_main_micro_indirect(
> > + data,
> > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_BUILD, 2,
> > + (u8 *)&build);
> > + if (ret)
> > + return ret;
> > + } else {
> > + dev_warn(&data->client->dev, "No Main Micro Heartbeat\n");
>
> This and other messages above would create a lot of noise if persistent
> since the function is called repeatedly.
>
OK. I will reduce the debug messages or change to use dev_dbg().
> > + }
> > + data->fw_ver.major = major;
> > + data->fw_ver.minor = minor;
> > + data->fw_ver.build = build;
> > +
> > + return 0;
> > +}
> > +
> > +static int pt516xx_fw_is_at_least(struct pt516xx_data *data, u8 major, u8 minor,
> > + u16 build)
> > +{
> > + u32 ver = major << 24 | minor << 16 | build;
> > + u32 curr_ver = data->fw_ver.major << 24 | data->fw_ver.minor << 16 |
> > + data->fw_ver.build;
> > +
> > + if (curr_ver >= ver)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +static int pt516xx_init_dev(struct pt516xx_data *data)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&data->lock);
> > + ret = pt516xx_fwsts_check(data);
> > + mutex_unlock(&data->lock);
> > + if (ret)
> > + return ret;
> > +
> > + dev_info(&data->client->dev, "fw_ver: %u.%u.%u\n", data->fw_ver.major,
> > + data->fw_ver.minor, data->fw_ver.build);
> > +
>
> Please no such noise
>
OK.
> > + if (pt516xx_fw_is_at_least(data, 2, 2, 0))
> > + data->mm_wide_reg_valid = true;
> > +
> > + data->temp_cal_code_avg = ARIES_TEMP_CAL_CODE_DEFAULT;
>
> What is the point of this ? temp_cal_code_avg will never be anything else.
>
I will change to directly use ARIES_TEMP_CAL_CODE_DEFAULT when calculating.
> > + data->init_done = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int pt516xx_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + struct pt516xx_data *data = dev_get_drvdata(dev);
> > + int ret = 0;
> > + long adc_code = 0;
> > +
>
> This assumes a specific endianness which isn't given. I am quite sure that the
> code as written won't work in a big endian system. Also, "long" may be 4 or 8 byte
> depending on the CPU architecture.
>
Oops, I will revise it to be compatible with the big endian system.
Using long is just to match the prototype of read(): "long *val".
> > + if (!data->init_done) {
> > + ret = pt516xx_init_dev(data);
> > + if (ret) {
> > + dev_err(dev, "pt516xx_init_dev failed %d\n", ret);
> > + return ret;
> > + }
> > + }
>
> This is really the wrong place for this code. It should be checked and initialized
> in the probe function, and probe should bail out if it fails.
>
> Also, the only reason to call pt516xx_init_dev() seems to be to determine
> if "wide reegister access" is supported. If it isn't, pt516xx_read_block_data()
> is used. Why not call pt516xx_read_block_data() below directly and not bother
> with wide register access ?
>
Previously I followed CSDK using wide_reg to access new FW. Also tried
old FW, but old FW
only support read_block_data. After further testing with the new FW,
it also does support
read_block_data. So I will revise it to use read_block_data directly
for temperature. Thanks.
> > +
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + mutex_lock(&data->lock);
> > + ret = pt516xx_read_wide_reg(data,
> > + ARIES_CURRENT_AVG_TEMP_ADC_CSR, 4,
> > + (u8 *)&adc_code);
> > + mutex_unlock(&data->lock);
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > + if (ret) {
> > + dev_err(dev, "Read adc_code failed %d\n", ret);
> > + return ret;
> > + }
The error return has been checked here.
> > + if (adc_code == 0 || adc_code >= 0x3ff) {
> > + dev_err(dev, "Invalid adc_code %lx\n", adc_code);
> > + return -EINVAL;
>
OK. I will change to use -ENODATA here.
> I guess this is supposed to cover an error return from pt516xx_read_wide_reg()
> returned an error, assuming that adc_code was not overwritten in that case.
> This is inappropriate. Error returns from pt516xx_read_wide_reg() should be
> handled explicitly (and don't indicate "Invalid argument").
>
> > + }
> > +
>
> The above will create logging noise if the chip has a problem. Please make it dev_dbg.
>
OK.
> > + *val = 110000 + ((adc_code - (data->temp_cal_code_avg + 250)) * -320);
> > +
> > + return 0;
> > +}
> > +
> > +static umode_t pt516xx_is_visible(const void *data,
> > + enum hwmon_sensor_types type, u32 attr,
> > + int channel)
> > +{
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + return 0444;
> > + default:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct hwmon_channel_info *pt516xx_info[] = {
> > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops pt516xx_hwmon_ops = {
> > + .is_visible = pt516xx_is_visible,
> > + .read = pt516xx_read,
> > +};
> > +
> > +static const struct hwmon_chip_info pt516xx_chip_info = {
> > + .ops = &pt516xx_hwmon_ops,
> > + .info = pt516xx_info,
> > +};
> > +
> > +static ssize_t pt516xx_debugfs_read_fw_ver(struct file *file, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct pt516xx_data *data = file->private_data;
> > + int ret;
> > + char ver[32];
> > +
> > + mutex_lock(&data->lock);
> > + ret = pt516xx_fwsts_check(data);
> > + mutex_unlock(&data->lock);
>
> What is the point of (re-)reading the firmware version repeatedly ?
>
The PCIe retimer uses normal power rail, that is, it's turned off when
the system is powered-off.
Consider that the PCIe retimer is used on a server system, and
monitored by a BMC. (an
independent management controller, with embedded Linux) The BMC is
still working when
the system is powered-off but remains standby power rail. When the
retimer FW is updated
and reload, it can read the correct FW version.
> > + if (ret)
> > + return ret;
> > +
> > + ret = snprintf(ver, sizeof(ver), "%u.%u.%u\n", data->fw_ver.major,
> > + data->fw_ver.minor, data->fw_ver.build);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return simple_read_from_buffer(buf, count, ppos, ver, ret + 1);
> > +}
> > +
> > +static const struct file_operations pt516xx_debugfs_ops_fw_ver = {
> > + .read = pt516xx_debugfs_read_fw_ver,
> > + .open = simple_open,
> > +};
> > +
> > +static ssize_t pt516xx_debugfs_read_health(struct file *file, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct pt516xx_data *data = file->private_data;
> > + int ret;
> > + u8 status = 0;
> > + char health[8];
> > +
> > + mutex_lock(&data->lock);
> > + ret = pt516xx_fwsts_check(data);
> > + mutex_unlock(&data->lock);
> > + if (ret == 0) {
> > + status |= data->mm_heartbeat_okay ? 1 : 0; // bit0
> > + status |= data->code_load_okay ? 2 : 0; // bit1
>
> Those should really be separate debugfs files.
>
OK.
> > + }
> > +
> > + ret = snprintf(health, sizeof(health), "0x%02x\n", status);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return simple_read_from_buffer(buf, count, ppos, health, ret + 1);
> > +}
> > +
> > +static const struct file_operations pt516xx_debugfs_ops_health = {
> > + .read = pt516xx_debugfs_read_health,
> > + .open = simple_open,
> > +};
> > +
> > +static int pt516xx_init_debugfs(struct pt516xx_data *data)
> > +{
> > + if (!pt516xx_debugfs_dir)
> > + return -ENOENT;
> > +
>
> debugfs functions handle this situation, and return values therefore do not
> need to be checked.
>
Got it. I will remove it.
> > + data->debugfs = debugfs_create_dir(dev_name(&data->client->dev),
> > + pt516xx_debugfs_dir);
> > + if (IS_ERR_OR_NULL(data->debugfs))
> > + return -ENOENT;
> > +
> Again, unnecessary.
>
Got it.
> > + debugfs_create_file("fw_ver", 0444, data->debugfs, data,
> > + &pt516xx_debugfs_ops_fw_ver);
> > +
> > + debugfs_create_file("health", 0444, data->debugfs, data,
> > + &pt516xx_debugfs_ops_health);
> > +
> > + return 0;
> > +}
> > +
> > +static int pt516xx_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct device *hwmon_dev;
> > + struct pt516xx_data *data;
> > +
> > + data = devm_kzalloc(dev, sizeof(struct pt516xx_data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->client = client;
> > + mutex_init(&data->lock);
> > + pt516xx_init_dev(data);
> > +
> > + hwmon_dev = devm_hwmon_device_register_with_info(
> > + dev, client->name, data, &pt516xx_chip_info, NULL);
> > +
> > + if (pt516xx_init_debugfs(data))
> > + dev_warn(dev, "Failed to register debugfs\n");
>
> debugfs should fail silently.
>
> debugfs files are not removed if a single device is unloaded. Did you check
> what happens in this case ?
>
Oops, I will add the remove callback function to remove the debugfs files.
> > +
> > + return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static const struct i2c_device_id pt516xx_id[] = {
> > + { "pt5161l", 0 },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, pt516xx_id);
> > +
> > +static const struct of_device_id __maybe_unused pt516xx_of_match[] = {
> > + { .compatible = "asteralabs,pt5161l" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, pt516xx_of_match);
> > +
> > +static struct i2c_driver pt516xx_driver = {
> > + .class = I2C_CLASS_HWMON,
> > + .driver = {
> > + .name = "pt516xx",
> > + .of_match_table = of_match_ptr(pt516xx_of_match),
>
> This means the driver won't instantiate on an ACPI system if devicetree
> support is disabled. Is this intentional ?
>
OK. I will add the acpi_match_table.
> > + },
> > + .probe = pt516xx_probe,
> > + .id_table = pt516xx_id,
> > +};
> > +
> > +module_i2c_driver(pt516xx_driver);
> > +
> > +static int __init pt516xx_core_init(void)
> > +{
> > + pt516xx_debugfs_dir = debugfs_create_dir("pt516xx", NULL);
> > + if (IS_ERR(pt516xx_debugfs_dir))
> > + pt516xx_debugfs_dir = NULL;
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit pt516xx_core_exit(void)
> > +{
> > + debugfs_remove_recursive(pt516xx_debugfs_dir);
> > +}
> > +
> > +module_init(pt516xx_core_init);
> > +module_exit(pt516xx_core_exit);
> > +
> > +MODULE_AUTHOR("Cosmo Chou <[email protected]>");
> > +MODULE_DESCRIPTION("Hwmon driver for Astera Labs Aries PCIe retimer");
> > +MODULE_LICENSE("GPL");
>
On Tue, Dec 5, 2023 at 1:49 AM Cosmo Chou <[email protected]> wrote:
>
> This driver implements support for temperature monitoring of Astera Labs
> PT5161L series PCIe retimer chips.
>
> This driver implementation originates from the CSDK available at
> Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
> The communication protocol utilized is based on the I2C/SMBus standard.
>
> Signed-off-by: Cosmo Chou <[email protected]>
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/pt516xx.rst | 48 +++
> MAINTAINERS | 8 +
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++++++++++++++++
> 6 files changed, 716 insertions(+)
> create mode 100644 Documentation/hwmon/pt516xx.rst
> create mode 100644 drivers/hwmon/pt516xx.c
> diff --git a/drivers/hwmon/pt516xx.c b/drivers/hwmon/pt516xx.c
> new file mode 100644
> index 000000000000..824798559fe1
> --- /dev/null
> +++ b/drivers/hwmon/pt516xx.c
> @@ -0,0 +1,648 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
You probably don't need this header and the implicit includes it makes
are dropped now in linux-next. Please check what you actually need and
make them explicit.
Rob