* [PATCH] hwmon: add initial NXP MC34VR500 PMIC monitoring support
@ 2023-01-16 13:42 Mario Kicherer
2023-01-16 15:55 ` Armin Wolf
0 siblings, 1 reply; 4+ messages in thread
From: Mario Kicherer @ 2023-01-16 13:42 UTC (permalink / raw)
To: linux-hwmon; +Cc: jdelvare, linux, Mario Kicherer
This patch adds initial monitoring support for the MC34VR500 PMIC. In its
current form, input voltage and temperature events are reported to hwmon.
Other interrupts only generate a generic entry in the kernel log so far.
The header file is copied from U-Boot and placed next to the C file as the
chip is usually only configured by the bootloader and it is unlikely that
it will be used by another Linux subsystem. If I should remove unused parts
or move the file to another path, let me know.
Datasheet:
- https://www.nxp.com/docs/en/data-sheet/MC34VR500.pdf
Signed-off-by: Mario Kicherer <dev@kicherer.org>
---
drivers/hwmon/Kconfig | 7 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/mc34vr500.c | 382 +++++++++++++++++++++++++++++++++
drivers/hwmon/mc34vr500_pmic.h | 172 +++++++++++++++
4 files changed, 562 insertions(+)
create mode 100644 drivers/hwmon/mc34vr500.c
create mode 100644 drivers/hwmon/mc34vr500_pmic.h
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 3176c33af6c6..b917c2528b62 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2350,6 +2350,13 @@ config SENSORS_INTEL_M10_BMC_HWMON
sensors monitor various telemetry data of different components on the
card, e.g. board temperature, FPGA core temperature/voltage/current.
+config SENSORS_MC34VR500
+ tristate "NXP MC34VR500 hardware monitoring driver"
+ depends on I2C
+ help
+ If you say yes here you get support for the temperature and input
+ voltage sensors of the NXP MC34VR500.
+
if ACPI
comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e2e4e87b282f..99e8cd8275c5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -216,6 +216,7 @@ obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o
obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
+obj-$(CONFIG_SENSORS_MC34VR500) += mc34vr500.o
obj-$(CONFIG_SENSORS_OCC) += occ/
obj-$(CONFIG_SENSORS_PECI) += peci/
diff --git a/drivers/hwmon/mc34vr500.c b/drivers/hwmon/mc34vr500.c
new file mode 100644
index 000000000000..bddf108d05ae
--- /dev/null
+++ b/drivers/hwmon/mc34vr500.c
@@ -0,0 +1,382 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * An hwmon driver for the NXP MC34VR500
+ *
+ * Copyright 2022 Mario Kicherer <dev@kicherer.org>
+ */
+
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include "mc34vr500_pmic.h"
+
+#define MC34VR500_DEVICEID_VALUE 0x14
+
+// INTSENSE0
+#define ENS_BIT (1<<0)
+#define LOWVINS_BIT (1<<1)
+#define THERM110S_BIT (1<<2)
+#define THERM120S_BIT (1<<3)
+#define THERM125S_BIT (1<<4)
+#define THERM130S_BIT (1<<5)
+
+// INTSENSE1
+#define SW1FAULTS1_BIT (1<<0)
+#define SW1FAULTS2_BIT (1<<1)
+#define SW1FAULTS3_BIT (1<<2)
+#define SW2FAULTS_BIT (1<<3)
+#define SW3FAULTS1_BIT (1<<4)
+#define SW3FAULTS2_BIT (1<<5)
+#define SW4FAULTS_BIT (1<<6)
+
+// INTSENSE4
+#define LDO1FAULTS_BIT (1<<1)
+#define LDO2FAULTS_BIT (1<<2)
+#define LDO3FAULTS_BIT (1<<3)
+#define LDO4FAULTS_BIT (1<<4)
+#define LDO5FAULTS_BIT (1<<5)
+
+struct mc34vr500_data {
+ struct i2c_client *client;
+ struct regmap *regmap;
+};
+
+static ssize_t mc34vr500_bool_show(struct device *dev,
+ struct device_attribute *da, char *buf)
+{
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+ struct mc34vr500_data *data = (struct mc34vr500_data *) dev_get_drvdata(dev);
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_read(data->regmap, MC34VR500_INTSENSE0 + (attr->index >> 8), ®);
+ if (ret < 0)
+ return ret;
+
+ reg &= (attr->index & 0xff);
+
+ return sysfs_emit(buf, "%d\n", !!reg);
+}
+
+// INTSENSE0
+static SENSOR_DEVICE_ATTR_RO(in1_alarm, mc34vr500_bool, 0x00 | LOWVINS_BIT);
+static SENSOR_DEVICE_ATTR_RO(temp110_alarm, mc34vr500_bool, 0x00 | THERM110S_BIT);
+static SENSOR_DEVICE_ATTR_RO(temp120_alarm, mc34vr500_bool, 0x00 | THERM120S_BIT);
+static SENSOR_DEVICE_ATTR_RO(temp125_alarm, mc34vr500_bool, 0x00 | THERM125S_BIT);
+static SENSOR_DEVICE_ATTR_RO(temp130_alarm, mc34vr500_bool, 0x00 | THERM130S_BIT);
+
+// INTSENSE1
+static SENSOR_DEVICE_ATTR_RO(curr1_alarm, mc34vr500_bool, 0x300 | SW1FAULTS1_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr2_alarm, mc34vr500_bool, 0x300 | SW1FAULTS2_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr3_alarm, mc34vr500_bool, 0x300 | SW1FAULTS3_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr4_alarm, mc34vr500_bool, 0x300 | SW2FAULTS_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr5_alarm, mc34vr500_bool, 0x300 | SW3FAULTS1_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr6_alarm, mc34vr500_bool, 0x300 | SW3FAULTS2_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr7_alarm, mc34vr500_bool, 0x300 | SW4FAULTS_BIT);
+
+// INTSENSE4
+static SENSOR_DEVICE_ATTR_RO(curr8_alarm, mc34vr500_bool, 0x600 | LDO1FAULTS_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr9_alarm, mc34vr500_bool, 0x600 | LDO2FAULTS_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr10_alarm, mc34vr500_bool, 0x600 | LDO3FAULTS_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr11_alarm, mc34vr500_bool, 0x600 | LDO4FAULTS_BIT);
+static SENSOR_DEVICE_ATTR_RO(curr12_alarm, mc34vr500_bool, 0x600 | LDO5FAULTS_BIT);
+
+static struct attribute *mc34vr500_attrs[] = {
+ &sensor_dev_attr_in1_alarm.dev_attr.attr,
+ &sensor_dev_attr_temp110_alarm.dev_attr.attr,
+ &sensor_dev_attr_temp120_alarm.dev_attr.attr,
+ &sensor_dev_attr_temp125_alarm.dev_attr.attr,
+ &sensor_dev_attr_temp130_alarm.dev_attr.attr,
+
+ &sensor_dev_attr_curr1_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr2_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr3_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr4_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr5_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr6_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr7_alarm.dev_attr.attr,
+
+ &sensor_dev_attr_curr8_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr9_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr10_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr11_alarm.dev_attr.attr,
+ &sensor_dev_attr_curr12_alarm.dev_attr.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(mc34vr500);
+
+static irqreturn_t mc34vr500_process_interrupt(int irq, void *userdata)
+{
+ struct mc34vr500_data *data = (struct mc34vr500_data *) userdata;
+ struct i2c_client *client = data->client;
+ unsigned int reg;
+ int ret;
+
+ pr_debug("mc34vr500: received interrupt\n");
+
+ ret = regmap_read(data->regmap, MC34VR500_INTSTAT0, ®);
+ if (ret < 0) {
+ dev_err(&client->dev, "unable to read mc34vr500 intsense0 register\n");
+ return ret;
+ }
+
+ if (reg) {
+ pr_warn("mc34vr500: interrupt intstat0 %u\n", reg);
+
+ if (reg & LOWVINS_BIT) {
+ ret = hwmon_notify_event(&client->dev, hwmon_in,
+ hwmon_in_alarm, 1);
+ if (ret)
+ dev_err(&client->dev,
+ "mc34vr500: error, hwmon_notify_event() failed: %d\n",
+ ret);
+ }
+
+ if (reg & THERM110S_BIT) {
+ ret = hwmon_notify_event(&client->dev, hwmon_temp,
+ hwmon_temp_alarm, 110);
+ if (ret)
+ dev_err(&client->dev,
+ "mc34vr500: error, hwmon_notify_event() failed: %d\n",
+ ret);
+ }
+
+ reg = 0;
+ ret = regmap_write(data->regmap, MC34VR500_INTSTAT0, reg);
+ if (ret) {
+ dev_err(&client->dev, "unable to enable intmask0 interrupts\n");
+ return ret;
+ }
+ }
+
+ ret = regmap_read(data->regmap, MC34VR500_INTSTAT1, ®);
+ if (ret < 0) {
+ dev_err(&client->dev, "unable to read mc34vr500 intsense1 register\n");
+ return ret;
+ }
+
+ if (reg) {
+ pr_warn("mc34vr500: interrupt intstat1 %u\n", reg);
+
+ reg = 0;
+ ret = regmap_write(data->regmap, MC34VR500_INTSTAT1, reg);
+ if (ret) {
+ dev_err(&client->dev, "unable to enable intmask1 interrupts\n");
+ return ret;
+ }
+ }
+
+ ret = regmap_read(data->regmap, MC34VR500_INTSTAT4, ®);
+ if (ret < 0) {
+ dev_err(&client->dev, "unable to read mc34vr500 intsense4 register\n");
+ return ret;
+ }
+
+ if (reg) {
+ pr_warn("mc34vr500: interrupt intstat4 %u\n", reg);
+
+ reg = 0;
+ ret = regmap_write(data->regmap, MC34VR500_INTSTAT4, reg);
+ if (ret) {
+ dev_err(&client->dev, "unable to enable intmask4 interrupts\n");
+ return ret;
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+static const struct regmap_config mc34vr500_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = MC34VR500_PWRGD_EN,
+};
+
+static int mc34vr500_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct mc34vr500_data *data;
+ struct device *hwmon_dev;
+ int ret;
+ unsigned int reg, revid, fabid;
+ struct regmap *regmap;
+ u32 interrupt_mask;
+
+ regmap = devm_regmap_init_i2c(client, &mc34vr500_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(dev, "failed to allocate register map\n");
+ return PTR_ERR(regmap);
+ }
+
+ data = devm_kzalloc(dev, sizeof(struct mc34vr500_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->regmap = regmap;
+ data->client = client;
+
+ ret = regmap_read(regmap, MC34VR500_DEVICEID, ®);
+ if (ret < 0) {
+ dev_err(dev, "unable to read config register\n");
+
+ return ret;
+ }
+
+ if (reg != MC34VR500_DEVICEID_VALUE) {
+ dev_err(dev, "invalid config register value 0x%x\n", reg);
+
+ return -ENODEV;
+ }
+
+ ret = regmap_read(regmap, MC34VR500_SILICONREVID, &revid);
+ if (ret < 0) {
+ dev_err(dev, "unable to read mc34vr500 revid register\n");
+ return ret;
+ }
+
+ ret = regmap_read(regmap, MC34VR500_FABID, &fabid);
+ if (ret < 0) {
+ dev_err(dev, "unable to read mc34vr500 fabid register\n");
+ return ret;
+ }
+
+ pr_notice("mc34vr500: revid 0x%x fabid 0x%x\n", revid, fabid);
+
+
+ ret = of_property_read_u32(dev->of_node,
+ "interrupt-mask",
+ &interrupt_mask);
+ if (ret == -EINVAL) {
+ interrupt_mask = 0;
+ } else if (ret) {
+ dev_err(dev, "Error reading interrupt_mask\n");
+ return ret;
+ }
+
+ ret = regmap_read(regmap, MC34VR500_INTSTAT0, ®);
+ if (ret < 0) {
+ dev_err(dev, "unable to read mc34vr500 intstat0 register\n");
+ return ret;
+ }
+ reg = reg & (~interrupt_mask);
+
+ if (reg)
+ pr_notice("mc34vr500: intstat0: 0x%x\n", reg);
+
+ if (reg & LOWVINS_BIT)
+ pr_notice("mc34vr500: low input voltage detected\n");
+
+ if (reg & THERM130S_BIT)
+ pr_notice("mc34vr500: temperature >= 130°c\n");
+ else if (reg & THERM125S_BIT)
+ pr_notice("mc34vr500: temperature >= 125°c\n");
+ else if (reg & THERM120S_BIT)
+ pr_notice("mc34vr500: temperature >= 120°c\n");
+ else if (reg & THERM110S_BIT)
+ pr_notice("mc34vr500: temperature >= 110°c\n");
+
+ ret = regmap_read(regmap, MC34VR500_INTSTAT1, ®);
+ if (ret < 0) {
+ dev_err(dev, "unable to read mc34vr500 intstat1 register\n");
+ return ret;
+ }
+ reg = reg & ((~interrupt_mask) >> 8);
+
+ if (reg)
+ pr_notice("mc34vr500: intstat1: 0x%x\n", reg);
+
+ ret = regmap_read(regmap, MC34VR500_INTSTAT4, ®);
+ if (ret < 0) {
+ dev_err(dev, "unable to read mc34vr500 intstat4 register\n");
+ return ret;
+ }
+ reg = reg & ((~interrupt_mask) >> 16);
+
+ if (reg)
+ pr_notice("mc34vr500: intstat4: 0x%x\n", reg);
+
+ hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+ data,
+ mc34vr500_groups);
+
+ if (client->irq) {
+ pr_notice("mc34vr500: enabling IRQ...\n");
+
+ ret = devm_request_threaded_irq(dev, client->irq, NULL,
+ mc34vr500_process_interrupt,
+ IRQF_TRIGGER_RISING |
+ IRQF_ONESHOT |
+ IRQF_SHARED,
+ dev_name(dev),
+ data);
+ if (ret) {
+ dev_err(dev, "Error requesting irq\n");
+ return ret;
+ }
+
+ /* clear interrupts */
+ reg = 0;
+ ret = regmap_write(regmap, MC34VR500_INTSTAT0, reg);
+ if (ret) {
+ dev_err(dev, "unable to write intstat0 register\n");
+ return ret;
+ }
+
+ /* enable interrupts */
+ ret = regmap_write(regmap, MC34VR500_INTMASK0, interrupt_mask & 0xff);
+ if (ret) {
+ dev_err(dev, "unable to enable intmask0 interrupts\n");
+ return ret;
+ }
+
+ reg = 0;
+ ret = regmap_write(regmap, MC34VR500_INTSTAT1, reg);
+ if (ret) {
+ dev_err(dev, "unable to write intstat1 register\n");
+ return ret;
+ }
+
+ ret = regmap_write(regmap, MC34VR500_INTMASK1, (interrupt_mask >> 8) & 0xff);
+ if (ret) {
+ dev_err(dev, "unable to enable intmask1 interrupts\n");
+ return ret;
+ }
+
+ reg = 0;
+ ret = regmap_write(regmap, MC34VR500_INTSTAT4, reg);
+ if (ret) {
+ dev_err(dev, "unable to write intstat1 register\n");
+ return ret;
+ }
+
+ ret = regmap_write(regmap, MC34VR500_INTMASK4, (interrupt_mask >> 16) & 0xff);
+ if (ret) {
+ dev_err(dev, "unable to enable intmask1 interrupts\n");
+ return ret;
+ }
+ }
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id mc34vr500_id[] = {
+ { "mc34vr500", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, mc34vr500_id);
+
+static struct i2c_driver mc34vr500_driver = {
+ .driver = {
+ .name = "mc34vr500",
+ },
+ .probe_new = mc34vr500_probe,
+ .id_table = mc34vr500_id,
+};
+
+module_i2c_driver(mc34vr500_driver);
+
+MODULE_AUTHOR("Mario Kicherer <dev@kicherer.org>");
+
+MODULE_DESCRIPTION("MC34VR500 driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/mc34vr500_pmic.h b/drivers/hwmon/mc34vr500_pmic.h
new file mode 100644
index 000000000000..1543ac692d72
--- /dev/null
+++ b/drivers/hwmon/mc34vr500_pmic.h
@@ -0,0 +1,172 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ * Hou Zhiqiang <Zhiqiang.Hou@freescale.com>
+ */
+
+#ifndef __MC34VR500_H_
+#define __MC34VR500_H_
+
+#define MC34VR500_I2C_ADDR 0x08
+
+/* Drivers name */
+#define MC34VR500_REGULATOR_DRIVER "mc34vr500_regulator"
+
+/* Register map */
+enum {
+ MC34VR500_DEVICEID = 0x00,
+
+ MC34VR500_SILICONREVID = 0x03,
+ MC34VR500_FABID,
+ MC34VR500_INTSTAT0,
+ MC34VR500_INTMASK0,
+ MC34VR500_INTSENSE0,
+ MC34VR500_INTSTAT1,
+ MC34VR500_INTMASK1,
+ MC34VR500_INTSENSE1,
+
+ MC34VR500_INTSTAT4 = 0x11,
+ MC34VR500_INTMASK4,
+ MC34VR500_INTSENSE4,
+
+ MC34VR500_PWRCTL = 0x1B,
+
+ MC34VR500_SW1VOLT = 0x2E,
+ MC34VR500_SW1STBY,
+ MC34VR500_SW1OFF,
+ MC34VR500_SW1MODE,
+ MC34VR500_SW1CONF,
+ MC34VR500_SW2VOLT,
+ MC34VR500_SW2STBY,
+ MC34VR500_SW2OFF,
+ MC34VR500_SW2MODE,
+ MC34VR500_SW2CONF,
+
+ MC34VR500_SW3VOLT = 0x3C,
+ MC34VR500_SW3STBY,
+ MC34VR500_SW3OFF,
+ MC34VR500_SW3MODE,
+ MC34VR500_SW3CONF,
+
+ MC34VR500_SW4VOLT = 0x4A,
+ MC34VR500_SW4STBY,
+ MC34VR500_SW4OFF,
+ MC34VR500_SW4MODE,
+ MC34VR500_SW4CONF,
+
+ MC34VR500_REFOUTCRTRL = 0x6A,
+
+ MC34VR500_LDO1CTL = 0x6D,
+ MC34VR500_LDO2CTL,
+ MC34VR500_LDO3CTL,
+ MC34VR500_LDO4CTL,
+ MC34VR500_LDO5CTL,
+
+ MC34VR500_PAGE_REGISTER = 0x7F,
+
+ /* Internal RAM */
+ MC34VR500_SW1_VOLT = 0xA8,
+ MC34VR500_SW1_SEQ,
+ MC34VR500_SW1_CONFIG,
+
+ MC34VR500_SW2_VOLT = 0xAC,
+ MC34VR500_SW2_SEQ,
+ MC34VR500_SW2_CONFIG,
+
+ MC34VR500_SW3_VOLT = 0xB0,
+ MC34VR500_SW3_SEQ,
+ MC34VR500_SW3_CONFIG,
+
+ MC34VR500_SW4_VOLT = 0xB8,
+ MC34VR500_SW4_SEQ,
+ MC34VR500_SW4_CONFIG,
+
+ MC34VR500_REFOUT_SEQ = 0xC4,
+
+ MC34VR500_LDO1_VOLT = 0xCC,
+ MC34VR500_LDO1_SEQ,
+
+ MC34VR500_LDO2_VOLT = 0xD0,
+ MC34VR500_LDO2_SEQ,
+
+ MC34VR500_LDO3_VOLT = 0xD4,
+ MC34VR500_LDO3_SEQ,
+
+ MC34VR500_LDO4_VOLT = 0xD8,
+ MC34VR500_LDO4_SEQ,
+
+ MC34VR500_LDO5_VOLT = 0xDC,
+ MC34VR500_LDO5_SEQ,
+
+ MC34VR500_PU_CONFIG1 = 0xE0,
+
+ MC34VR500_TBB_POR = 0xE4,
+
+ MC34VR500_PWRGD_EN = 0xE8,
+
+ MC34VR500_NUM_OF_REGS,
+};
+
+/* Registor offset based on SWxVOLT register */
+#define MC34VR500_VOLT_OFFSET 0
+#define MC34VR500_STBY_OFFSET 1
+#define MC34VR500_OFF_OFFSET 2
+#define MC34VR500_MODE_OFFSET 3
+#define MC34VR500_CONF_OFFSET 4
+
+#define SW_MODE_MASK 0xf
+#define SW_MODE_SHIFT 0
+
+#define LDO_VOL_MASK 0xf
+#define LDO_EN (1 << 4)
+#define LDO_MODE_SHIFT 4
+#define LDO_MODE_MASK (1 << 4)
+#define LDO_MODE_OFF 0
+#define LDO_MODE_ON 1
+
+#define REFOUTEN (1 << 4)
+
+/*
+ * Regulator Mode Control
+ *
+ * OFF: The regulator is switched off and the output voltage is discharged.
+ * PFM: In this mode, the regulator is always in PFM mode, which is useful
+ * at light loads for optimized efficiency.
+ * PWM: In this mode, the regulator is always in PWM mode operation
+ * regardless of load conditions.
+ * APS: In this mode, the regulator moves automatically between pulse
+ * skipping mode and PWM mode depending on load conditions.
+ *
+ * SWxMODE[3:0]
+ * Normal Mode | Standby Mode | value
+ * OFF OFF 0x0
+ * PWM OFF 0x1
+ * PFM OFF 0x3
+ * APS OFF 0x4
+ * PWM PWM 0x5
+ * PWM APS 0x6
+ * APS APS 0x8
+ * APS PFM 0xc
+ * PWM PFM 0xd
+ */
+#define OFF_OFF 0x0
+#define PWM_OFF 0x1
+#define PFM_OFF 0x3
+#define APS_OFF 0x4
+#define PWM_PWM 0x5
+#define PWM_APS 0x6
+#define APS_APS 0x8
+#define APS_PFM 0xc
+#define PWM_PFM 0xd
+
+enum swx {
+ SW1 = 0,
+ SW2,
+ SW3,
+ SW4,
+};
+
+int mc34vr500_get_sw_volt(uint8_t sw);
+int mc34vr500_set_sw_volt(uint8_t sw, int sw_volt);
+int power_mc34vr500_init(unsigned char bus);
+#endif /* __MC34VR500_PMIC_H_ */
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: add initial NXP MC34VR500 PMIC monitoring support
2023-01-16 13:42 [PATCH] hwmon: add initial NXP MC34VR500 PMIC monitoring support Mario Kicherer
@ 2023-01-16 15:55 ` Armin Wolf
2023-01-16 16:48 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Armin Wolf @ 2023-01-16 15:55 UTC (permalink / raw)
To: Mario Kicherer, linux-hwmon; +Cc: jdelvare, linux
Am 16.01.23 um 14:42 schrieb Mario Kicherer:
> This patch adds initial monitoring support for the MC34VR500 PMIC. In its
> current form, input voltage and temperature events are reported to hwmon.
> Other interrupts only generate a generic entry in the kernel log so far.
>
> The header file is copied from U-Boot and placed next to the C file as the
> chip is usually only configured by the bootloader and it is unlikely that
> it will be used by another Linux subsystem. If I should remove unused parts
> or move the file to another path, let me know.
>
> Datasheet:
> - https://www.nxp.com/docs/en/data-sheet/MC34VR500.pdf
>
> Signed-off-by: Mario Kicherer <dev@kicherer.org>
> ---
> drivers/hwmon/Kconfig | 7 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/mc34vr500.c | 382 +++++++++++++++++++++++++++++++++
> drivers/hwmon/mc34vr500_pmic.h | 172 +++++++++++++++
> 4 files changed, 562 insertions(+)
> create mode 100644 drivers/hwmon/mc34vr500.c
> create mode 100644 drivers/hwmon/mc34vr500_pmic.h
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 3176c33af6c6..b917c2528b62 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2350,6 +2350,13 @@ config SENSORS_INTEL_M10_BMC_HWMON
> sensors monitor various telemetry data of different components on the
> card, e.g. board temperature, FPGA core temperature/voltage/current.
>
> +config SENSORS_MC34VR500
> + tristate "NXP MC34VR500 hardware monitoring driver"
> + depends on I2C
> + help
> + If you say yes here you get support for the temperature and input
> + voltage sensors of the NXP MC34VR500.
> +
> if ACPI
>
> comment "ACPI drivers"
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e2e4e87b282f..99e8cd8275c5 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -216,6 +216,7 @@ obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o
> obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
> obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
> +obj-$(CONFIG_SENSORS_MC34VR500) += mc34vr500.o
>
> obj-$(CONFIG_SENSORS_OCC) += occ/
> obj-$(CONFIG_SENSORS_PECI) += peci/
> diff --git a/drivers/hwmon/mc34vr500.c b/drivers/hwmon/mc34vr500.c
> new file mode 100644
> index 000000000000..bddf108d05ae
> --- /dev/null
> +++ b/drivers/hwmon/mc34vr500.c
> @@ -0,0 +1,382 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * An hwmon driver for the NXP MC34VR500
> + *
> + * Copyright 2022 Mario Kicherer <dev@kicherer.org>
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include "mc34vr500_pmic.h"
> +
> +#define MC34VR500_DEVICEID_VALUE 0x14
> +
> +// INTSENSE0
> +#define ENS_BIT (1<<0)
> +#define LOWVINS_BIT (1<<1)
> +#define THERM110S_BIT (1<<2)
> +#define THERM120S_BIT (1<<3)
> +#define THERM125S_BIT (1<<4)
> +#define THERM130S_BIT (1<<5)
> +
> +// INTSENSE1
> +#define SW1FAULTS1_BIT (1<<0)
> +#define SW1FAULTS2_BIT (1<<1)
> +#define SW1FAULTS3_BIT (1<<2)
> +#define SW2FAULTS_BIT (1<<3)
> +#define SW3FAULTS1_BIT (1<<4)
> +#define SW3FAULTS2_BIT (1<<5)
> +#define SW4FAULTS_BIT (1<<6)
> +
> +// INTSENSE4
> +#define LDO1FAULTS_BIT (1<<1)
> +#define LDO2FAULTS_BIT (1<<2)
> +#define LDO3FAULTS_BIT (1<<3)
> +#define LDO4FAULTS_BIT (1<<4)
> +#define LDO5FAULTS_BIT (1<<5)
Maybe you could use BIT() here?
> +
> +struct mc34vr500_data {
> + struct i2c_client *client;
> + struct regmap *regmap;
> +};
> +
> +static ssize_t mc34vr500_bool_show(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> + struct mc34vr500_data *data = (struct mc34vr500_data *) dev_get_drvdata(dev);
> + unsigned int reg;
> + int ret;
> +
> + ret = regmap_read(data->regmap, MC34VR500_INTSENSE0 + (attr->index >> 8), ®);
> + if (ret < 0)
> + return ret;
> +
> + reg &= (attr->index & 0xff);
> +
> + return sysfs_emit(buf, "%d\n", !!reg);
> +}
> +
> +// INTSENSE0
> +static SENSOR_DEVICE_ATTR_RO(in1_alarm, mc34vr500_bool, 0x00 | LOWVINS_BIT);
> +static SENSOR_DEVICE_ATTR_RO(temp110_alarm, mc34vr500_bool, 0x00 | THERM110S_BIT);
> +static SENSOR_DEVICE_ATTR_RO(temp120_alarm, mc34vr500_bool, 0x00 | THERM120S_BIT);
> +static SENSOR_DEVICE_ATTR_RO(temp125_alarm, mc34vr500_bool, 0x00 | THERM125S_BIT);
> +static SENSOR_DEVICE_ATTR_RO(temp130_alarm, mc34vr500_bool, 0x00 | THERM130S_BIT);
Does the chip support up to 130 temperature channels? If no, then maybe you could
name them "temp1_*", ..., "temp5_*" and use tempX_label for the labeling.
> +
> +// INTSENSE1
> +static SENSOR_DEVICE_ATTR_RO(curr1_alarm, mc34vr500_bool, 0x300 | SW1FAULTS1_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr2_alarm, mc34vr500_bool, 0x300 | SW1FAULTS2_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr3_alarm, mc34vr500_bool, 0x300 | SW1FAULTS3_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr4_alarm, mc34vr500_bool, 0x300 | SW2FAULTS_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr5_alarm, mc34vr500_bool, 0x300 | SW3FAULTS1_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr6_alarm, mc34vr500_bool, 0x300 | SW3FAULTS2_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr7_alarm, mc34vr500_bool, 0x300 | SW4FAULTS_BIT);
> +
> +// INTSENSE4
> +static SENSOR_DEVICE_ATTR_RO(curr8_alarm, mc34vr500_bool, 0x600 | LDO1FAULTS_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr9_alarm, mc34vr500_bool, 0x600 | LDO2FAULTS_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr10_alarm, mc34vr500_bool, 0x600 | LDO3FAULTS_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr11_alarm, mc34vr500_bool, 0x600 | LDO4FAULTS_BIT);
> +static SENSOR_DEVICE_ATTR_RO(curr12_alarm, mc34vr500_bool, 0x600 | LDO5FAULTS_BIT);
> +
> +static struct attribute *mc34vr500_attrs[] = {
> + &sensor_dev_attr_in1_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp110_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp120_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp125_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp130_alarm.dev_attr.attr,
> +
> + &sensor_dev_attr_curr1_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr2_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr3_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr4_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr5_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr6_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr7_alarm.dev_attr.attr,
> +
> + &sensor_dev_attr_curr8_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr9_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr10_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr11_alarm.dev_attr.attr,
> + &sensor_dev_attr_curr12_alarm.dev_attr.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(mc34vr500);
You should use the new hwmon_device_register_with_info() API, which does
the sysfs handling for you.
> +
> +static irqreturn_t mc34vr500_process_interrupt(int irq, void *userdata)
> +{
> + struct mc34vr500_data *data = (struct mc34vr500_data *) userdata;
> + struct i2c_client *client = data->client;
> + unsigned int reg;
> + int ret;
> +
> + pr_debug("mc34vr500: received interrupt\n");
> +
> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT0, ®);
> + if (ret < 0) {
> + dev_err(&client->dev, "unable to read mc34vr500 intsense0 register\n");
Maybe use some sort of ratelimiting here? Otherwise it could potentially fill up
the kernel log with redundant messages. Or maybe just omit this and all the other
error messages?
> + return ret;
> + }
> +
> + if (reg) {
> + pr_warn("mc34vr500: interrupt intstat0 %u\n", reg);
Same as above, it could fill up the kernel log is the alarm interrupt is triggered fast enough.
> +
> + if (reg & LOWVINS_BIT) {
> + ret = hwmon_notify_event(&client->dev, hwmon_in,
> + hwmon_in_alarm, 1);
> + if (ret)
> + dev_err(&client->dev,
> + "mc34vr500: error, hwmon_notify_event() failed: %d\n",
> + ret);
> + }
> +
> + if (reg & THERM110S_BIT) {
> + ret = hwmon_notify_event(&client->dev, hwmon_temp,
> + hwmon_temp_alarm, 110);
> + if (ret)
> + dev_err(&client->dev,
> + "mc34vr500: error, hwmon_notify_event() failed: %d\n",
> + ret);
> + }
> +
> + reg = 0;
> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT0, reg);
> + if (ret) {
> + dev_err(&client->dev, "unable to enable intmask0 interrupts\n");
> + return ret;
> + }
> + }
> +
> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT1, ®);
> + if (ret < 0) {
> + dev_err(&client->dev, "unable to read mc34vr500 intsense1 register\n");
> + return ret;
> + }
> +
> + if (reg) {
> + pr_warn("mc34vr500: interrupt intstat1 %u\n", reg);
> +
> + reg = 0;
> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT1, reg);
> + if (ret) {
> + dev_err(&client->dev, "unable to enable intmask1 interrupts\n");
> + return ret;
> + }
> + }
> +
> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT4, ®);
> + if (ret < 0) {
> + dev_err(&client->dev, "unable to read mc34vr500 intsense4 register\n");
> + return ret;
> + }
> +
> + if (reg) {
> + pr_warn("mc34vr500: interrupt intstat4 %u\n", reg);
> +
> + reg = 0;
> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT4, reg);
> + if (ret) {
> + dev_err(&client->dev, "unable to enable intmask4 interrupts\n");
> + return ret;
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static const struct regmap_config mc34vr500_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MC34VR500_PWRGD_EN,
> +};
> +
> +static int mc34vr500_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct mc34vr500_data *data;
> + struct device *hwmon_dev;
> + int ret;
> + unsigned int reg, revid, fabid;
> + struct regmap *regmap;
> + u32 interrupt_mask;
> +
> + regmap = devm_regmap_init_i2c(client, &mc34vr500_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(dev, "failed to allocate register map\n");
> + return PTR_ERR(regmap);
> + }
> +
> + data = devm_kzalloc(dev, sizeof(struct mc34vr500_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->regmap = regmap;
> + data->client = client;
> +
> + ret = regmap_read(regmap, MC34VR500_DEVICEID, ®);
> + if (ret < 0) {
> + dev_err(dev, "unable to read config register\n");
> +
> + return ret;
> + }
> +
> + if (reg != MC34VR500_DEVICEID_VALUE) {
> + dev_err(dev, "invalid config register value 0x%x\n", reg);
> +
> + return -ENODEV;
> + }
> +
> + ret = regmap_read(regmap, MC34VR500_SILICONREVID, &revid);
> + if (ret < 0) {
> + dev_err(dev, "unable to read mc34vr500 revid register\n");
> + return ret;
> + }
> +
> + ret = regmap_read(regmap, MC34VR500_FABID, &fabid);
> + if (ret < 0) {
> + dev_err(dev, "unable to read mc34vr500 fabid register\n");
> + return ret;
> + }
> +
> + pr_notice("mc34vr500: revid 0x%x fabid 0x%x\n", revid, fabid);
> +
> +
> + ret = of_property_read_u32(dev->of_node,
> + "interrupt-mask",
> + &interrupt_mask);
> + if (ret == -EINVAL) {
> + interrupt_mask = 0;
> + } else if (ret) {
> + dev_err(dev, "Error reading interrupt_mask\n");
> + return ret;
> + }
> +
> + ret = regmap_read(regmap, MC34VR500_INTSTAT0, ®);
> + if (ret < 0) {
> + dev_err(dev, "unable to read mc34vr500 intstat0 register\n");
> + return ret;
> + }
> + reg = reg & (~interrupt_mask);
> +
> + if (reg)
> + pr_notice("mc34vr500: intstat0: 0x%x\n", reg);
> +
> + if (reg & LOWVINS_BIT)
> + pr_notice("mc34vr500: low input voltage detected\n");
> +
> + if (reg & THERM130S_BIT)
> + pr_notice("mc34vr500: temperature >= 130°c\n");
> + else if (reg & THERM125S_BIT)
> + pr_notice("mc34vr500: temperature >= 125°c\n");
> + else if (reg & THERM120S_BIT)
> + pr_notice("mc34vr500: temperature >= 120°c\n");
> + else if (reg & THERM110S_BIT)
> + pr_notice("mc34vr500: temperature >= 110°c\n");
> +
> + ret = regmap_read(regmap, MC34VR500_INTSTAT1, ®);
> + if (ret < 0) {
> + dev_err(dev, "unable to read mc34vr500 intstat1 register\n");
> + return ret;
> + }
> + reg = reg & ((~interrupt_mask) >> 8);
> +
> + if (reg)
> + pr_notice("mc34vr500: intstat1: 0x%x\n", reg);
> +
> + ret = regmap_read(regmap, MC34VR500_INTSTAT4, ®);
> + if (ret < 0) {
> + dev_err(dev, "unable to read mc34vr500 intstat4 register\n");
> + return ret;
> + }
> + reg = reg & ((~interrupt_mask) >> 16);
> +
> + if (reg)
> + pr_notice("mc34vr500: intstat4: 0x%x\n", reg);
> +
> + hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
> + data,
> + mc34vr500_groups);
> +
> + if (client->irq) {
> + pr_notice("mc34vr500: enabling IRQ...\n");
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + mc34vr500_process_interrupt,
> + IRQF_TRIGGER_RISING |
> + IRQF_ONESHOT |
> + IRQF_SHARED,
> + dev_name(dev),
> + data);
> + if (ret) {
> + dev_err(dev, "Error requesting irq\n");
> + return ret;
> + }
> +
> + /* clear interrupts */
> + reg = 0;
> + ret = regmap_write(regmap, MC34VR500_INTSTAT0, reg);
> + if (ret) {
> + dev_err(dev, "unable to write intstat0 register\n");
> + return ret;
> + }
> +
> + /* enable interrupts */
> + ret = regmap_write(regmap, MC34VR500_INTMASK0, interrupt_mask & 0xff);
> + if (ret) {
> + dev_err(dev, "unable to enable intmask0 interrupts\n");
> + return ret;
> + }
> +
> + reg = 0;
> + ret = regmap_write(regmap, MC34VR500_INTSTAT1, reg);
> + if (ret) {
> + dev_err(dev, "unable to write intstat1 register\n");
> + return ret;
> + }
> +
> + ret = regmap_write(regmap, MC34VR500_INTMASK1, (interrupt_mask >> 8) & 0xff);
> + if (ret) {
> + dev_err(dev, "unable to enable intmask1 interrupts\n");
> + return ret;
> + }
> +
> + reg = 0;
> + ret = regmap_write(regmap, MC34VR500_INTSTAT4, reg);
> + if (ret) {
> + dev_err(dev, "unable to write intstat1 register\n");
> + return ret;
> + }
> +
> + ret = regmap_write(regmap, MC34VR500_INTMASK4, (interrupt_mask >> 16) & 0xff);
> + if (ret) {
> + dev_err(dev, "unable to enable intmask1 interrupts\n");
> + return ret;
> + }
> + }
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id mc34vr500_id[] = {
> + { "mc34vr500", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, mc34vr500_id);
> +
> +static struct i2c_driver mc34vr500_driver = {
> + .driver = {
> + .name = "mc34vr500",
> + },
> + .probe_new = mc34vr500_probe,
> + .id_table = mc34vr500_id,
> +};
> +
> +module_i2c_driver(mc34vr500_driver);
> +
> +MODULE_AUTHOR("Mario Kicherer <dev@kicherer.org>");
> +
> +MODULE_DESCRIPTION("MC34VR500 driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/mc34vr500_pmic.h b/drivers/hwmon/mc34vr500_pmic.h
> new file mode 100644
> index 000000000000..1543ac692d72
> --- /dev/null
> +++ b/drivers/hwmon/mc34vr500_pmic.h
> @@ -0,0 +1,172 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + * Hou Zhiqiang <Zhiqiang.Hou@freescale.com>
> + */
> +
> +#ifndef __MC34VR500_H_
> +#define __MC34VR500_H_
> +
> +#define MC34VR500_I2C_ADDR 0x08
> +
> +/* Drivers name */
> +#define MC34VR500_REGULATOR_DRIVER "mc34vr500_regulator"
> +
> +/* Register map */
> +enum {
> + MC34VR500_DEVICEID = 0x00,
> +
> + MC34VR500_SILICONREVID = 0x03,
> + MC34VR500_FABID,
> + MC34VR500_INTSTAT0,
> + MC34VR500_INTMASK0,
> + MC34VR500_INTSENSE0,
> + MC34VR500_INTSTAT1,
> + MC34VR500_INTMASK1,
> + MC34VR500_INTSENSE1,
> +
> + MC34VR500_INTSTAT4 = 0x11,
> + MC34VR500_INTMASK4,
> + MC34VR500_INTSENSE4,
> +
> + MC34VR500_PWRCTL = 0x1B,
> +
> + MC34VR500_SW1VOLT = 0x2E,
> + MC34VR500_SW1STBY,
> + MC34VR500_SW1OFF,
> + MC34VR500_SW1MODE,
> + MC34VR500_SW1CONF,
> + MC34VR500_SW2VOLT,
> + MC34VR500_SW2STBY,
> + MC34VR500_SW2OFF,
> + MC34VR500_SW2MODE,
> + MC34VR500_SW2CONF,
> +
> + MC34VR500_SW3VOLT = 0x3C,
> + MC34VR500_SW3STBY,
> + MC34VR500_SW3OFF,
> + MC34VR500_SW3MODE,
> + MC34VR500_SW3CONF,
> +
> + MC34VR500_SW4VOLT = 0x4A,
> + MC34VR500_SW4STBY,
> + MC34VR500_SW4OFF,
> + MC34VR500_SW4MODE,
> + MC34VR500_SW4CONF,
> +
> + MC34VR500_REFOUTCRTRL = 0x6A,
> +
> + MC34VR500_LDO1CTL = 0x6D,
> + MC34VR500_LDO2CTL,
> + MC34VR500_LDO3CTL,
> + MC34VR500_LDO4CTL,
> + MC34VR500_LDO5CTL,
> +
> + MC34VR500_PAGE_REGISTER = 0x7F,
> +
> + /* Internal RAM */
> + MC34VR500_SW1_VOLT = 0xA8,
> + MC34VR500_SW1_SEQ,
> + MC34VR500_SW1_CONFIG,
> +
> + MC34VR500_SW2_VOLT = 0xAC,
> + MC34VR500_SW2_SEQ,
> + MC34VR500_SW2_CONFIG,
> +
> + MC34VR500_SW3_VOLT = 0xB0,
> + MC34VR500_SW3_SEQ,
> + MC34VR500_SW3_CONFIG,
> +
> + MC34VR500_SW4_VOLT = 0xB8,
> + MC34VR500_SW4_SEQ,
> + MC34VR500_SW4_CONFIG,
> +
> + MC34VR500_REFOUT_SEQ = 0xC4,
> +
> + MC34VR500_LDO1_VOLT = 0xCC,
> + MC34VR500_LDO1_SEQ,
> +
> + MC34VR500_LDO2_VOLT = 0xD0,
> + MC34VR500_LDO2_SEQ,
> +
> + MC34VR500_LDO3_VOLT = 0xD4,
> + MC34VR500_LDO3_SEQ,
> +
> + MC34VR500_LDO4_VOLT = 0xD8,
> + MC34VR500_LDO4_SEQ,
> +
> + MC34VR500_LDO5_VOLT = 0xDC,
> + MC34VR500_LDO5_SEQ,
> +
> + MC34VR500_PU_CONFIG1 = 0xE0,
> +
> + MC34VR500_TBB_POR = 0xE4,
> +
> + MC34VR500_PWRGD_EN = 0xE8,
> +
> + MC34VR500_NUM_OF_REGS,
> +};
> +
> +/* Registor offset based on SWxVOLT register */
> +#define MC34VR500_VOLT_OFFSET 0
> +#define MC34VR500_STBY_OFFSET 1
> +#define MC34VR500_OFF_OFFSET 2
> +#define MC34VR500_MODE_OFFSET 3
> +#define MC34VR500_CONF_OFFSET 4
> +
> +#define SW_MODE_MASK 0xf
> +#define SW_MODE_SHIFT 0
> +
> +#define LDO_VOL_MASK 0xf
> +#define LDO_EN (1 << 4)
> +#define LDO_MODE_SHIFT 4
> +#define LDO_MODE_MASK (1 << 4)
> +#define LDO_MODE_OFF 0
> +#define LDO_MODE_ON 1
> +
> +#define REFOUTEN (1 << 4)
> +
> +/*
> + * Regulator Mode Control
> + *
> + * OFF: The regulator is switched off and the output voltage is discharged.
> + * PFM: In this mode, the regulator is always in PFM mode, which is useful
> + * at light loads for optimized efficiency.
> + * PWM: In this mode, the regulator is always in PWM mode operation
> + * regardless of load conditions.
> + * APS: In this mode, the regulator moves automatically between pulse
> + * skipping mode and PWM mode depending on load conditions.
> + *
> + * SWxMODE[3:0]
> + * Normal Mode | Standby Mode | value
> + * OFF OFF 0x0
> + * PWM OFF 0x1
> + * PFM OFF 0x3
> + * APS OFF 0x4
> + * PWM PWM 0x5
> + * PWM APS 0x6
> + * APS APS 0x8
> + * APS PFM 0xc
> + * PWM PFM 0xd
> + */
> +#define OFF_OFF 0x0
> +#define PWM_OFF 0x1
> +#define PFM_OFF 0x3
> +#define APS_OFF 0x4
> +#define PWM_PWM 0x5
> +#define PWM_APS 0x6
> +#define APS_APS 0x8
> +#define APS_PFM 0xc
> +#define PWM_PFM 0xd
> +
> +enum swx {
> + SW1 = 0,
> + SW2,
> + SW3,
> + SW4,
> +};
> +
> +int mc34vr500_get_sw_volt(uint8_t sw);
> +int mc34vr500_set_sw_volt(uint8_t sw, int sw_volt);
> +int power_mc34vr500_init(unsigned char bus);
> +#endif /* __MC34VR500_PMIC_H_ */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: add initial NXP MC34VR500 PMIC monitoring support
2023-01-16 15:55 ` Armin Wolf
@ 2023-01-16 16:48 ` Guenter Roeck
2023-01-17 15:55 ` Mario Kicherer
0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2023-01-16 16:48 UTC (permalink / raw)
To: Armin Wolf, Mario Kicherer, linux-hwmon; +Cc: jdelvare
On 1/16/23 07:55, Armin Wolf wrote:
> Am 16.01.23 um 14:42 schrieb Mario Kicherer:
>
>> This patch adds initial monitoring support for the MC34VR500 PMIC. In its
>> current form, input voltage and temperature events are reported to hwmon.
>> Other interrupts only generate a generic entry in the kernel log so far.
>>
This is unacceptable. The driver should neither enable nor log any interrupt
bits it doesn't support.
>> The header file is copied from U-Boot and placed next to the C file as the
>> chip is usually only configured by the bootloader and it is unlikely that
>> it will be used by another Linux subsystem. If I should remove unused parts
>> or move the file to another path, let me know.
>>
>> Datasheet:
>> - https://www.nxp.com/docs/en/data-sheet/MC34VR500.pdf
>>
>> Signed-off-by: Mario Kicherer <dev@kicherer.org>
>> ---
>> drivers/hwmon/Kconfig | 7 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/mc34vr500.c | 382 +++++++++++++++++++++++++++++++++
>> drivers/hwmon/mc34vr500_pmic.h | 172 +++++++++++++++
Documentation/hwmon/mc34vr500 is missing.
Devicetree property documentation is missing (though that doesn't
really matter since the defined property is unacceptable).
>> 4 files changed, 562 insertions(+)
>> create mode 100644 drivers/hwmon/mc34vr500.c
>> create mode 100644 drivers/hwmon/mc34vr500_pmic.h
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 3176c33af6c6..b917c2528b62 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2350,6 +2350,13 @@ config SENSORS_INTEL_M10_BMC_HWMON
>> sensors monitor various telemetry data of different components on the
>> card, e.g. board temperature, FPGA core temperature/voltage/current.
>>
>> +config SENSORS_MC34VR500
>> + tristate "NXP MC34VR500 hardware monitoring driver"
>> + depends on I2C
>> + help
>> + If you say yes here you get support for the temperature and input
>> + voltage sensors of the NXP MC34VR500.
>> +
>> if ACPI
>>
>> comment "ACPI drivers"
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index e2e4e87b282f..99e8cd8275c5 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -216,6 +216,7 @@ obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o
>> obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
>> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
>> obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
>> +obj-$(CONFIG_SENSORS_MC34VR500) += mc34vr500.o
>>
>> obj-$(CONFIG_SENSORS_OCC) += occ/
>> obj-$(CONFIG_SENSORS_PECI) += peci/
>> diff --git a/drivers/hwmon/mc34vr500.c b/drivers/hwmon/mc34vr500.c
>> new file mode 100644
>> index 000000000000..bddf108d05ae
>> --- /dev/null
>> +++ b/drivers/hwmon/mc34vr500.c
>> @@ -0,0 +1,382 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * An hwmon driver for the NXP MC34VR500
>> + *
>> + * Copyright 2022 Mario Kicherer <dev@kicherer.org>
>> + */
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include "mc34vr500_pmic.h"
>> +
>> +#define MC34VR500_DEVICEID_VALUE 0x14
>> +
>> +// INTSENSE0
Please no C++ comments.
>> +#define ENS_BIT (1<<0)
>> +#define LOWVINS_BIT (1<<1)
>> +#define THERM110S_BIT (1<<2)
>> +#define THERM120S_BIT (1<<3)
>> +#define THERM125S_BIT (1<<4)
>> +#define THERM130S_BIT (1<<5)
>> +
>> +// INTSENSE1
>> +#define SW1FAULTS1_BIT (1<<0)
>> +#define SW1FAULTS2_BIT (1<<1)
>> +#define SW1FAULTS3_BIT (1<<2)
>> +#define SW2FAULTS_BIT (1<<3)
>> +#define SW3FAULTS1_BIT (1<<4)
>> +#define SW3FAULTS2_BIT (1<<5)
>> +#define SW4FAULTS_BIT (1<<6)
>> +
>> +// INTSENSE4
>> +#define LDO1FAULTS_BIT (1<<1)
>> +#define LDO2FAULTS_BIT (1<<2)
>> +#define LDO3FAULTS_BIT (1<<3)
>> +#define LDO4FAULTS_BIT (1<<4)
>> +#define LDO5FAULTS_BIT (1<<5)
>
> Maybe you could use BIT() here?
>
Documentation/hwmon/submitting-patches.rst says:
* Please run your patch through 'checkpatch --strict'. There should be no
errors, no warnings, and few if any check messages. If there are any
messages, please be prepared to explain.
With
total: 0 errors, 2 warnings, 43 checks, 574 lines checked
the author will have a lot of explaining to do.
I don't think the author read that document, though, because various
other requirements in that document are not followed either.
Some of the more obvious ones:
* Limit the number of kernel log messages. In general, your driver should not
generate an error message just because a runtime operation failed. Report
errors to user space instead, using an appropriate error code. Keep in mind
that kernel error log messages not only fill up the kernel log, but also are
printed synchronously, most likely with interrupt disabled, often to a serial
console. Excessive logging can seriously affect system performance.
* Use devm_hwmon_device_register_with_info() or, if your driver needs a remove
function, hwmon_device_register_with_info() to register your driver with the
hwmon subsystem. Try using devm_add_action() instead of a remove function if
possible. Do not use hwmon_device_register().
* Document the driver in Documentation/hwmon/<driver_name>.rst.
* Add the driver to Kconfig and Makefile in alphabetical order.
That really makes me wonder if I should create a form letter advising people
to follow the document, and that I'll only review the driver if there are no
violations.
>> +
>> +struct mc34vr500_data {
>> + struct i2c_client *client;
>> + struct regmap *regmap;
>> +};
>> +
>> +static ssize_t mc34vr500_bool_show(struct device *dev,
>> + struct device_attribute *da, char *buf)
>> +{
>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> + struct mc34vr500_data *data = (struct mc34vr500_data *) dev_get_drvdata(dev);
>> + unsigned int reg;
>> + int ret;
>> +
>> + ret = regmap_read(data->regmap, MC34VR500_INTSENSE0 + (attr->index >> 8), ®);
>> + if (ret < 0)
>> + return ret;
>> +
>> + reg &= (attr->index & 0xff);
>> +
>> + return sysfs_emit(buf, "%d\n", !!reg);
>> +}
>> +
>> +// INTSENSE0
>> +static SENSOR_DEVICE_ATTR_RO(in1_alarm, mc34vr500_bool, 0x00 | LOWVINS_BIT);
This should really be either _min_alarm or _lcrit_alarm.
>> +static SENSOR_DEVICE_ATTR_RO(temp110_alarm, mc34vr500_bool, 0x00 | THERM110S_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(temp120_alarm, mc34vr500_bool, 0x00 | THERM120S_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(temp125_alarm, mc34vr500_bool, 0x00 | THERM125S_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(temp130_alarm, mc34vr500_bool, 0x00 | THERM130S_BIT);
>
> Does the chip support up to 130 temperature channels? If no, then maybe you could
> name them "temp1_*", ..., "temp5_*" and use tempX_label for the labeling.
>
No, I checked the datasheet. This reflects temperatures and is an abuse of the hwmon ABI.
There is only a single temperature. The author will have to decide which of the alarm
attributes (_max_alarm, _crit_alarm, _emergency_alarm) to use for the four limits.
>> +
>> +// INTSENSE1
>> +static SENSOR_DEVICE_ATTR_RO(curr1_alarm, mc34vr500_bool, 0x300 | SW1FAULTS1_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr2_alarm, mc34vr500_bool, 0x300 | SW1FAULTS2_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr3_alarm, mc34vr500_bool, 0x300 | SW1FAULTS3_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr4_alarm, mc34vr500_bool, 0x300 | SW2FAULTS_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr5_alarm, mc34vr500_bool, 0x300 | SW3FAULTS1_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr6_alarm, mc34vr500_bool, 0x300 | SW3FAULTS2_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr7_alarm, mc34vr500_bool, 0x300 | SW4FAULTS_BIT);
>> +
>> +// INTSENSE4
>> +static SENSOR_DEVICE_ATTR_RO(curr8_alarm, mc34vr500_bool, 0x600 | LDO1FAULTS_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr9_alarm, mc34vr500_bool, 0x600 | LDO2FAULTS_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr10_alarm, mc34vr500_bool, 0x600 | LDO3FAULTS_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr11_alarm, mc34vr500_bool, 0x600 | LDO4FAULTS_BIT);
>> +static SENSOR_DEVICE_ATTR_RO(curr12_alarm, mc34vr500_bool, 0x600 | LDO5FAULTS_BIT);
>> +
>> +static struct attribute *mc34vr500_attrs[] = {
>> + &sensor_dev_attr_in1_alarm.dev_attr.attr,
>> + &sensor_dev_attr_temp110_alarm.dev_attr.attr,
>> + &sensor_dev_attr_temp120_alarm.dev_attr.attr,
>> + &sensor_dev_attr_temp125_alarm.dev_attr.attr,
>> + &sensor_dev_attr_temp130_alarm.dev_attr.attr,
>> +
>> + &sensor_dev_attr_curr1_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr2_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr3_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr4_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr5_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr6_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr7_alarm.dev_attr.attr,
>> +
>> + &sensor_dev_attr_curr8_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr9_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr10_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr11_alarm.dev_attr.attr,
>> + &sensor_dev_attr_curr12_alarm.dev_attr.attr,
There are only 9 current channels (SW1..SW4 and LDO1..LDO5). Some of them
consume more than one interrupt status bit, but the documentation doesn't
say what the two bits exactly refer to. Maybe to multiple phases ?
Either that (but that would not explain why only two of the four SWx
channels have two status bits) or this is another abuse of the hwmon ABI.
>> + NULL
>> +};
>> +ATTRIBUTE_GROUPS(mc34vr500);
>
> You should use the new hwmon_device_register_with_info() API, which does
> the sysfs handling for you.
>
should -> must. I won't accept new drivers if they don't use the new API.
>> +
>> +static irqreturn_t mc34vr500_process_interrupt(int irq, void *userdata)
>> +{
>> + struct mc34vr500_data *data = (struct mc34vr500_data *) userdata;
>> + struct i2c_client *client = data->client;
>> + unsigned int reg;
>> + int ret;
>> +
>> + pr_debug("mc34vr500: received interrupt\n");
>> +
>> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT0, ®);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "unable to read mc34vr500 intsense0 register\n");
>
> Maybe use some sort of ratelimiting here? Otherwise it could potentially fill up
> the kernel log with redundant messages. Or maybe just omit this and all the other
> error messages?
>
>> + return ret;
>> + }
>> +
>> + if (reg) {
>> + pr_warn("mc34vr500: interrupt intstat0 %u\n", reg);
>
> Same as above, it could fill up the kernel log is the alarm interrupt is triggered fast enough.
>
This isn't acceptable anyway. It logs a message with every interrupts.
All those pr_{warn, notice} messages need to be dropped from the driver.
>> +
>> + if (reg & LOWVINS_BIT) {
>> + ret = hwmon_notify_event(&client->dev, hwmon_in,
>> + hwmon_in_alarm, 1);
Notifications need to be on the hwmon device.
>> + if (ret)
>> + dev_err(&client->dev,
>> + "mc34vr500: error, hwmon_notify_event() failed: %d\n",
>> + ret);
>> + }
>> +
>> + if (reg & THERM110S_BIT) {
>> + ret = hwmon_notify_event(&client->dev, hwmon_temp,
>> + hwmon_temp_alarm, 110);
>> + if (ret)
>> + dev_err(&client->dev,
>> + "mc34vr500: error, hwmon_notify_event() failed: %d\n",
>> + ret);
>> + }
hwmon_notify_event() only fails if bad data is passed to it, ie if the driver
code is bad. Checking and logging it is only necessary if the code was never
tested.
>> +
>> + reg = 0;
>> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT0, reg);
>> + if (ret) {
>> + dev_err(&client->dev, "unable to enable intmask0 interrupts\n");
>> + return ret;
>> + }
>> + }
>> +
>> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT1, ®);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "unable to read mc34vr500 intsense1 register\n");
>> + return ret;
>> + }
>> +
>> + if (reg) {
>> + pr_warn("mc34vr500: interrupt intstat1 %u\n", reg);
>> +
>> + reg = 0;
>> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT1, reg);
>> + if (ret) {
>> + dev_err(&client->dev, "unable to enable intmask1 interrupts\n");
>> + return ret;
>> + }
>> + }
>> +
>> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT4, ®);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "unable to read mc34vr500 intsense4 register\n");
>> + return ret;
>> + }
>> +
>> + if (reg) {
>> + pr_warn("mc34vr500: interrupt intstat4 %u\n", reg);
>> +
>> + reg = 0;
>> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT4, reg);
>> + if (ret) {
>> + dev_err(&client->dev, "unable to enable intmask4 interrupts\n");
>> + return ret;
>> + }
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static const struct regmap_config mc34vr500_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + .max_register = MC34VR500_PWRGD_EN,
>> +};
>> +
>> +static int mc34vr500_probe(struct i2c_client *client)
>> +{
>> + struct device *dev = &client->dev;
>> + struct mc34vr500_data *data;
>> + struct device *hwmon_dev;
>> + int ret;
>> + unsigned int reg, revid, fabid;
>> + struct regmap *regmap;
>> + u32 interrupt_mask;
>> +
>> + regmap = devm_regmap_init_i2c(client, &mc34vr500_regmap_config);
>> + if (IS_ERR(regmap)) {
>> + dev_err(dev, "failed to allocate register map\n");
>> + return PTR_ERR(regmap);
>> + }
>> +
>> + data = devm_kzalloc(dev, sizeof(struct mc34vr500_data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->regmap = regmap;
>> + data->client = client;
>> +
>> + ret = regmap_read(regmap, MC34VR500_DEVICEID, ®);
>> + if (ret < 0) {
>> + dev_err(dev, "unable to read config register\n");
>> +
>> + return ret;
>> + }
>> +
>> + if (reg != MC34VR500_DEVICEID_VALUE) {
>> + dev_err(dev, "invalid config register value 0x%x\n", reg);
>> +
>> + return -ENODEV;
>> + }
This simply reflects that the chip isn't there and should result in -ENODEV
without logging noise.
>> +
>> + ret = regmap_read(regmap, MC34VR500_SILICONREVID, &revid);
>> + if (ret < 0) {
>> + dev_err(dev, "unable to read mc34vr500 revid register\n");
>> + return ret;
>> + }
>> +
>> + ret = regmap_read(regmap, MC34VR500_FABID, &fabid);
>> + if (ret < 0) {
>> + dev_err(dev, "unable to read mc34vr500 fabid register\n");
>> + return ret;
>> + }
>> +
>> + pr_notice("mc34vr500: revid 0x%x fabid 0x%x\n", revid, fabid);
>> +
>> +
>> + ret = of_property_read_u32(dev->of_node,
>> + "interrupt-mask",
>> + &interrupt_mask);
>> + if (ret == -EINVAL) {
>> + interrupt_mask = 0;
>> + } else if (ret) {
>> + dev_err(dev, "Error reading interrupt_mask\n");
>> + return ret;
>> + }
>> +
This is inappropriate. The driver needs to enable the interrupt bits it needs,
and nothing else.
If there is a desire to enable individual channels, that should be done
with a per-channel deviretree file and with the {in, current, temp}X_enable
sysfs attribute.
>> + ret = regmap_read(regmap, MC34VR500_INTSTAT0, ®);
>> + if (ret < 0) {
>> + dev_err(dev, "unable to read mc34vr500 intstat0 register\n");
>> + return ret;
>> + }
>> + reg = reg & (~interrupt_mask);
>> +
>> + if (reg)
>> + pr_notice("mc34vr500: intstat0: 0x%x\n", reg);
>> +
>> + if (reg & LOWVINS_BIT)
>> + pr_notice("mc34vr500: low input voltage detected\n");
>> +
>> + if (reg & THERM130S_BIT)
>> + pr_notice("mc34vr500: temperature >= 130°c\n");
>> + else if (reg & THERM125S_BIT)
>> + pr_notice("mc34vr500: temperature >= 125°c\n");
>> + else if (reg & THERM120S_BIT)
>> + pr_notice("mc34vr500: temperature >= 120°c\n");
>> + else if (reg & THERM110S_BIT)
>> + pr_notice("mc34vr500: temperature >= 110°c\n");
>> +
>> + ret = regmap_read(regmap, MC34VR500_INTSTAT1, ®);
>> + if (ret < 0) {
>> + dev_err(dev, "unable to read mc34vr500 intstat1 register\n");
>> + return ret;
>> + }
>> + reg = reg & ((~interrupt_mask) >> 8);
>> +
>> + if (reg)
>> + pr_notice("mc34vr500: intstat1: 0x%x\n", reg);
>> +
>> + ret = regmap_read(regmap, MC34VR500_INTSTAT4, ®);
>> + if (ret < 0) {
>> + dev_err(dev, "unable to read mc34vr500 intstat4 register\n");
>> + return ret;
>> + }
>> + reg = reg & ((~interrupt_mask) >> 16);
>> +
>> + if (reg)
>> + pr_notice("mc34vr500: intstat4: 0x%x\n", reg);
>> +
Most of the above does not belong in the probe function and needs to be dropped.
It is not the responsibility of the probe function to log interrupt status
values.
>> + hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
>> + data,
>> + mc34vr500_groups);
>> +
>> + if (client->irq) {
>> + pr_notice("mc34vr500: enabling IRQ...\n");
>> +
Example for driver noise. I could try to mark them all but gave up after
realizing how noisy this driver is. This all needs to go.
>> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
>> + mc34vr500_process_interrupt,
>> + IRQF_TRIGGER_RISING |
>> + IRQF_ONESHOT |
>> + IRQF_SHARED,
>> + dev_name(dev),
>> + data);
>> + if (ret) {
>> + dev_err(dev, "Error requesting irq\n");
>> + return ret;
>> + }
>> +
>> + /* clear interrupts */
>> + reg = 0;
>> + ret = regmap_write(regmap, MC34VR500_INTSTAT0, reg);
>> + if (ret) {
>> + dev_err(dev, "unable to write intstat0 register\n");
>> + return ret;
>> + }
>> +
>> + /* enable interrupts */
>> + ret = regmap_write(regmap, MC34VR500_INTMASK0, interrupt_mask & 0xff);
>> + if (ret) {
>> + dev_err(dev, "unable to enable intmask0 interrupts\n");
>> + return ret;
>> + }
>> +
>> + reg = 0;
>> + ret = regmap_write(regmap, MC34VR500_INTSTAT1, reg);
>> + if (ret) {
>> + dev_err(dev, "unable to write intstat1 register\n");
>> + return ret;
>> + }
>> +
>> + ret = regmap_write(regmap, MC34VR500_INTMASK1, (interrupt_mask >> 8) & 0xff);
>> + if (ret) {
>> + dev_err(dev, "unable to enable intmask1 interrupts\n");
>> + return ret;
>> + }
>> +
>> + reg = 0;
>> + ret = regmap_write(regmap, MC34VR500_INTSTAT4, reg);
>> + if (ret) {
>> + dev_err(dev, "unable to write intstat1 register\n");
>> + return ret;
>> + }
>> +
>> + ret = regmap_write(regmap, MC34VR500_INTMASK4, (interrupt_mask >> 16) & 0xff);
>> + if (ret) {
>> + dev_err(dev, "unable to enable intmask1 interrupts\n");
>> + return ret;
>> + }
>> + }
>> +
>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct i2c_device_id mc34vr500_id[] = {
>> + { "mc34vr500", 0 },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, mc34vr500_id);
>> +
>> +static struct i2c_driver mc34vr500_driver = {
>> + .driver = {
>> + .name = "mc34vr500",
>> + },
>> + .probe_new = mc34vr500_probe,
>> + .id_table = mc34vr500_id,
>> +};
>> +
>> +module_i2c_driver(mc34vr500_driver);
>> +
>> +MODULE_AUTHOR("Mario Kicherer <dev@kicherer.org>");
>> +
>> +MODULE_DESCRIPTION("MC34VR500 driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/hwmon/mc34vr500_pmic.h b/drivers/hwmon/mc34vr500_pmic.h
>> new file mode 100644
>> index 000000000000..1543ac692d72
>> --- /dev/null
>> +++ b/drivers/hwmon/mc34vr500_pmic.h
>> @@ -0,0 +1,172 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright 2016 Freescale Semiconductor, Inc.
>> + * Hou Zhiqiang <Zhiqiang.Hou@freescale.com>
>> + */
>> +
>> +#ifndef __MC34VR500_H_
>> +#define __MC34VR500_H_
>> +
>> +#define MC34VR500_I2C_ADDR 0x08
>> +
>> +/* Drivers name */
>> +#define MC34VR500_REGULATOR_DRIVER "mc34vr500_regulator"
>> +
>> +/* Register map */
>> +enum {
>> + MC34VR500_DEVICEID = 0x00,
>> +
>> + MC34VR500_SILICONREVID = 0x03,
>> + MC34VR500_FABID,
>> + MC34VR500_INTSTAT0,
>> + MC34VR500_INTMASK0,
>> + MC34VR500_INTSENSE0,
>> + MC34VR500_INTSTAT1,
>> + MC34VR500_INTMASK1,
>> + MC34VR500_INTSENSE1,
>> +
>> + MC34VR500_INTSTAT4 = 0x11,
>> + MC34VR500_INTMASK4,
>> + MC34VR500_INTSENSE4,
>> +
>> + MC34VR500_PWRCTL = 0x1B,
>> +
>> + MC34VR500_SW1VOLT = 0x2E,
>> + MC34VR500_SW1STBY,
>> + MC34VR500_SW1OFF,
>> + MC34VR500_SW1MODE,
>> + MC34VR500_SW1CONF,
>> + MC34VR500_SW2VOLT,
>> + MC34VR500_SW2STBY,
>> + MC34VR500_SW2OFF,
>> + MC34VR500_SW2MODE,
>> + MC34VR500_SW2CONF,
>> +
>> + MC34VR500_SW3VOLT = 0x3C,
>> + MC34VR500_SW3STBY,
>> + MC34VR500_SW3OFF,
>> + MC34VR500_SW3MODE,
>> + MC34VR500_SW3CONF,
>> +
>> + MC34VR500_SW4VOLT = 0x4A,
>> + MC34VR500_SW4STBY,
>> + MC34VR500_SW4OFF,
>> + MC34VR500_SW4MODE,
>> + MC34VR500_SW4CONF,
>> +
>> + MC34VR500_REFOUTCRTRL = 0x6A,
>> +
>> + MC34VR500_LDO1CTL = 0x6D,
>> + MC34VR500_LDO2CTL,
>> + MC34VR500_LDO3CTL,
>> + MC34VR500_LDO4CTL,
>> + MC34VR500_LDO5CTL,
>> +
>> + MC34VR500_PAGE_REGISTER = 0x7F,
>> +
>> + /* Internal RAM */
>> + MC34VR500_SW1_VOLT = 0xA8,
>> + MC34VR500_SW1_SEQ,
>> + MC34VR500_SW1_CONFIG,
>> +
>> + MC34VR500_SW2_VOLT = 0xAC,
>> + MC34VR500_SW2_SEQ,
>> + MC34VR500_SW2_CONFIG,
>> +
>> + MC34VR500_SW3_VOLT = 0xB0,
>> + MC34VR500_SW3_SEQ,
>> + MC34VR500_SW3_CONFIG,
>> +
>> + MC34VR500_SW4_VOLT = 0xB8,
>> + MC34VR500_SW4_SEQ,
>> + MC34VR500_SW4_CONFIG,
>> +
>> + MC34VR500_REFOUT_SEQ = 0xC4,
>> +
>> + MC34VR500_LDO1_VOLT = 0xCC,
>> + MC34VR500_LDO1_SEQ,
>> +
>> + MC34VR500_LDO2_VOLT = 0xD0,
>> + MC34VR500_LDO2_SEQ,
>> +
>> + MC34VR500_LDO3_VOLT = 0xD4,
>> + MC34VR500_LDO3_SEQ,
>> +
>> + MC34VR500_LDO4_VOLT = 0xD8,
>> + MC34VR500_LDO4_SEQ,
>> +
>> + MC34VR500_LDO5_VOLT = 0xDC,
>> + MC34VR500_LDO5_SEQ,
>> +
>> + MC34VR500_PU_CONFIG1 = 0xE0,
>> +
>> + MC34VR500_TBB_POR = 0xE4,
>> +
>> + MC34VR500_PWRGD_EN = 0xE8,
>> +
>> + MC34VR500_NUM_OF_REGS,
>> +};
>> +
>> +/* Registor offset based on SWxVOLT register */
>> +#define MC34VR500_VOLT_OFFSET 0
>> +#define MC34VR500_STBY_OFFSET 1
>> +#define MC34VR500_OFF_OFFSET 2
>> +#define MC34VR500_MODE_OFFSET 3
>> +#define MC34VR500_CONF_OFFSET 4
>> +
>> +#define SW_MODE_MASK 0xf
>> +#define SW_MODE_SHIFT 0
>> +
>> +#define LDO_VOL_MASK 0xf
>> +#define LDO_EN (1 << 4)
>> +#define LDO_MODE_SHIFT 4
>> +#define LDO_MODE_MASK (1 << 4)
>> +#define LDO_MODE_OFF 0
>> +#define LDO_MODE_ON 1
>> +
>> +#define REFOUTEN (1 << 4)
>> +
>> +/*
>> + * Regulator Mode Control
>> + *
>> + * OFF: The regulator is switched off and the output voltage is discharged.
>> + * PFM: In this mode, the regulator is always in PFM mode, which is useful
>> + * at light loads for optimized efficiency.
>> + * PWM: In this mode, the regulator is always in PWM mode operation
>> + * regardless of load conditions.
>> + * APS: In this mode, the regulator moves automatically between pulse
>> + * skipping mode and PWM mode depending on load conditions.
>> + *
>> + * SWxMODE[3:0]
>> + * Normal Mode | Standby Mode | value
>> + * OFF OFF 0x0
>> + * PWM OFF 0x1
>> + * PFM OFF 0x3
>> + * APS OFF 0x4
>> + * PWM PWM 0x5
>> + * PWM APS 0x6
>> + * APS APS 0x8
>> + * APS PFM 0xc
>> + * PWM PFM 0xd
>> + */
>> +#define OFF_OFF 0x0
>> +#define PWM_OFF 0x1
>> +#define PFM_OFF 0x3
>> +#define APS_OFF 0x4
>> +#define PWM_PWM 0x5
>> +#define PWM_APS 0x6
>> +#define APS_APS 0x8
>> +#define APS_PFM 0xc
>> +#define PWM_PFM 0xd
>> +
>> +enum swx {
>> + SW1 = 0,
>> + SW2,
>> + SW3,
>> + SW4,
>> +};
>> +
>> +int mc34vr500_get_sw_volt(uint8_t sw);
>> +int mc34vr500_set_sw_volt(uint8_t sw, int sw_volt);
>> +int power_mc34vr500_init(unsigned char bus);
>> +#endif /* __MC34VR500_PMIC_H_ */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: add initial NXP MC34VR500 PMIC monitoring support
2023-01-16 16:48 ` Guenter Roeck
@ 2023-01-17 15:55 ` Mario Kicherer
0 siblings, 0 replies; 4+ messages in thread
From: Mario Kicherer @ 2023-01-17 15:55 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Armin Wolf, linux-hwmon, jdelvare
Sorry, I did not know that there is separate submission guide for HWMON.
I only
checked the code without the --strict parameter.
I made all changes and will send a v2 in a few seconds.
Thank you both for the review!
Best regards,
Mario
On 2023-01-16 17:48, Guenter Roeck wrote:
> On 1/16/23 07:55, Armin Wolf wrote:
>> Am 16.01.23 um 14:42 schrieb Mario Kicherer:
>>
>>> This patch adds initial monitoring support for the MC34VR500 PMIC. In
>>> its
>>> current form, input voltage and temperature events are reported to
>>> hwmon.
>>> Other interrupts only generate a generic entry in the kernel log so
>>> far.
>>>
>
> This is unacceptable. The driver should neither enable nor log any
> interrupt
> bits it doesn't support.
>
>>> The header file is copied from U-Boot and placed next to the C file
>>> as the
>>> chip is usually only configured by the bootloader and it is unlikely
>>> that
>>> it will be used by another Linux subsystem. If I should remove unused
>>> parts
>>> or move the file to another path, let me know.
>>>
>>> Datasheet:
>>> - https://www.nxp.com/docs/en/data-sheet/MC34VR500.pdf
>>>
>>> Signed-off-by: Mario Kicherer <dev@kicherer.org>
>>> ---
>>> drivers/hwmon/Kconfig | 7 +
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/mc34vr500.c | 382
>>> +++++++++++++++++++++++++++++++++
>>> drivers/hwmon/mc34vr500_pmic.h | 172 +++++++++++++++
>
> Documentation/hwmon/mc34vr500 is missing.
>
> Devicetree property documentation is missing (though that doesn't
> really matter since the defined property is unacceptable).
>
>>> 4 files changed, 562 insertions(+)
>>> create mode 100644 drivers/hwmon/mc34vr500.c
>>> create mode 100644 drivers/hwmon/mc34vr500_pmic.h
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 3176c33af6c6..b917c2528b62 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -2350,6 +2350,13 @@ config SENSORS_INTEL_M10_BMC_HWMON
>>> sensors monitor various telemetry data of different
>>> components on the
>>> card, e.g. board temperature, FPGA core
>>> temperature/voltage/current.
>>>
>>> +config SENSORS_MC34VR500
>>> + tristate "NXP MC34VR500 hardware monitoring driver"
>>> + depends on I2C
>>> + help
>>> + If you say yes here you get support for the temperature and
>>> input
>>> + voltage sensors of the NXP MC34VR500.
>>> +
>>> if ACPI
>>>
>>> comment "ACPI drivers"
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index e2e4e87b282f..99e8cd8275c5 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -216,6 +216,7 @@ obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o
>>> obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
>>> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
>>> obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
>>> +obj-$(CONFIG_SENSORS_MC34VR500) += mc34vr500.o
>>>
>>> obj-$(CONFIG_SENSORS_OCC) += occ/
>>> obj-$(CONFIG_SENSORS_PECI) += peci/
>>> diff --git a/drivers/hwmon/mc34vr500.c b/drivers/hwmon/mc34vr500.c
>>> new file mode 100644
>>> index 000000000000..bddf108d05ae
>>> --- /dev/null
>>> +++ b/drivers/hwmon/mc34vr500.c
>>> @@ -0,0 +1,382 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * An hwmon driver for the NXP MC34VR500
>>> + *
>>> + * Copyright 2022 Mario Kicherer <dev@kicherer.org>
>>> + */
>>> +
>>> +#include <linux/hwmon.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/regmap.h>
>>> +#include "mc34vr500_pmic.h"
>>> +
>>> +#define MC34VR500_DEVICEID_VALUE 0x14
>>> +
>>> +// INTSENSE0
>
> Please no C++ comments.
>
>>> +#define ENS_BIT (1<<0)
>>> +#define LOWVINS_BIT (1<<1)
>>> +#define THERM110S_BIT (1<<2)
>>> +#define THERM120S_BIT (1<<3)
>>> +#define THERM125S_BIT (1<<4)
>>> +#define THERM130S_BIT (1<<5)
>>> +
>>> +// INTSENSE1
>>> +#define SW1FAULTS1_BIT (1<<0)
>>> +#define SW1FAULTS2_BIT (1<<1)
>>> +#define SW1FAULTS3_BIT (1<<2)
>>> +#define SW2FAULTS_BIT (1<<3)
>>> +#define SW3FAULTS1_BIT (1<<4)
>>> +#define SW3FAULTS2_BIT (1<<5)
>>> +#define SW4FAULTS_BIT (1<<6)
>>> +
>>> +// INTSENSE4
>>> +#define LDO1FAULTS_BIT (1<<1)
>>> +#define LDO2FAULTS_BIT (1<<2)
>>> +#define LDO3FAULTS_BIT (1<<3)
>>> +#define LDO4FAULTS_BIT (1<<4)
>>> +#define LDO5FAULTS_BIT (1<<5)
>>
>> Maybe you could use BIT() here?
>>
>
> Documentation/hwmon/submitting-patches.rst says:
>
> * Please run your patch through 'checkpatch --strict'. There should be
> no
> errors, no warnings, and few if any check messages. If there are any
> messages, please be prepared to explain.
>
> With
>
> total: 0 errors, 2 warnings, 43 checks, 574 lines checked
>
> the author will have a lot of explaining to do.
>
> I don't think the author read that document, though, because various
> other requirements in that document are not followed either.
> Some of the more obvious ones:
>
> * Limit the number of kernel log messages. In general, your driver
> should not
> generate an error message just because a runtime operation failed.
> Report
> errors to user space instead, using an appropriate error code. Keep
> in mind
> that kernel error log messages not only fill up the kernel log, but
> also are
> printed synchronously, most likely with interrupt disabled, often to
> a serial
> console. Excessive logging can seriously affect system performance.
>
> * Use devm_hwmon_device_register_with_info() or, if your driver needs a
> remove
> function, hwmon_device_register_with_info() to register your driver
> with the
> hwmon subsystem. Try using devm_add_action() instead of a remove
> function if
> possible. Do not use hwmon_device_register().
>
> * Document the driver in Documentation/hwmon/<driver_name>.rst.
>
> * Add the driver to Kconfig and Makefile in alphabetical order.
>
> That really makes me wonder if I should create a form letter advising
> people
> to follow the document, and that I'll only review the driver if there
> are no
> violations.
>
>>> +
>>> +struct mc34vr500_data {
>>> + struct i2c_client *client;
>>> + struct regmap *regmap;
>>> +};
>>> +
>>> +static ssize_t mc34vr500_bool_show(struct device *dev,
>>> + struct device_attribute *da, char *buf)
>>> +{
>>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>>> + struct mc34vr500_data *data = (struct mc34vr500_data *)
>>> dev_get_drvdata(dev);
>>> + unsigned int reg;
>>> + int ret;
>>> +
>>> + ret = regmap_read(data->regmap, MC34VR500_INTSENSE0 +
>>> (attr->index >> 8), ®);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + reg &= (attr->index & 0xff);
>>> +
>>> + return sysfs_emit(buf, "%d\n", !!reg);
>>> +}
>>> +
>>> +// INTSENSE0
>>> +static SENSOR_DEVICE_ATTR_RO(in1_alarm, mc34vr500_bool, 0x00 |
>>> LOWVINS_BIT);
>
> This should really be either _min_alarm or _lcrit_alarm.
>
>>> +static SENSOR_DEVICE_ATTR_RO(temp110_alarm, mc34vr500_bool, 0x00 |
>>> THERM110S_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(temp120_alarm, mc34vr500_bool, 0x00 |
>>> THERM120S_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(temp125_alarm, mc34vr500_bool, 0x00 |
>>> THERM125S_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(temp130_alarm, mc34vr500_bool, 0x00 |
>>> THERM130S_BIT);
>>
>> Does the chip support up to 130 temperature channels? If no, then
>> maybe you could
>> name them "temp1_*", ..., "temp5_*" and use tempX_label for the
>> labeling.
>>
>
> No, I checked the datasheet. This reflects temperatures and is an abuse
> of the hwmon ABI.
> There is only a single temperature. The author will have to decide
> which of the alarm
> attributes (_max_alarm, _crit_alarm, _emergency_alarm) to use for the
> four limits.
>
>>> +
>>> +// INTSENSE1
>>> +static SENSOR_DEVICE_ATTR_RO(curr1_alarm, mc34vr500_bool, 0x300 |
>>> SW1FAULTS1_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr2_alarm, mc34vr500_bool, 0x300 |
>>> SW1FAULTS2_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr3_alarm, mc34vr500_bool, 0x300 |
>>> SW1FAULTS3_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr4_alarm, mc34vr500_bool, 0x300 |
>>> SW2FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr5_alarm, mc34vr500_bool, 0x300 |
>>> SW3FAULTS1_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr6_alarm, mc34vr500_bool, 0x300 |
>>> SW3FAULTS2_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr7_alarm, mc34vr500_bool, 0x300 |
>>> SW4FAULTS_BIT);
>>> +
>>> +// INTSENSE4
>>> +static SENSOR_DEVICE_ATTR_RO(curr8_alarm, mc34vr500_bool, 0x600 |
>>> LDO1FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr9_alarm, mc34vr500_bool, 0x600 |
>>> LDO2FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr10_alarm, mc34vr500_bool, 0x600 |
>>> LDO3FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr11_alarm, mc34vr500_bool, 0x600 |
>>> LDO4FAULTS_BIT);
>>> +static SENSOR_DEVICE_ATTR_RO(curr12_alarm, mc34vr500_bool, 0x600 |
>>> LDO5FAULTS_BIT);
>>> +
>>> +static struct attribute *mc34vr500_attrs[] = {
>>> + &sensor_dev_attr_in1_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_temp110_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_temp120_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_temp125_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_temp130_alarm.dev_attr.attr,
>>> +
>>> + &sensor_dev_attr_curr1_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr2_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr3_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr4_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr5_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr6_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr7_alarm.dev_attr.attr,
>>> +
>>> + &sensor_dev_attr_curr8_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr9_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr10_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr11_alarm.dev_attr.attr,
>>> + &sensor_dev_attr_curr12_alarm.dev_attr.attr,
>
> There are only 9 current channels (SW1..SW4 and LDO1..LDO5). Some of
> them
> consume more than one interrupt status bit, but the documentation
> doesn't
> say what the two bits exactly refer to. Maybe to multiple phases ?
> Either that (but that would not explain why only two of the four SWx
> channels have two status bits) or this is another abuse of the hwmon
> ABI.
>
>>> + NULL
>>> +};
>>> +ATTRIBUTE_GROUPS(mc34vr500);
>>
>> You should use the new hwmon_device_register_with_info() API, which
>> does
>> the sysfs handling for you.
>>
>
> should -> must. I won't accept new drivers if they don't use the new
> API.
>
>>> +
>>> +static irqreturn_t mc34vr500_process_interrupt(int irq, void
>>> *userdata)
>>> +{
>>> + struct mc34vr500_data *data = (struct mc34vr500_data *)
>>> userdata;
>>> + struct i2c_client *client = data->client;
>>> + unsigned int reg;
>>> + int ret;
>>> +
>>> + pr_debug("mc34vr500: received interrupt\n");
>>> +
>>> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT0, ®);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "unable to read mc34vr500 intsense0
>>> register\n");
>>
>> Maybe use some sort of ratelimiting here? Otherwise it could
>> potentially fill up
>> the kernel log with redundant messages. Or maybe just omit this and
>> all the other
>> error messages?
>>
>>> + return ret;
>>> + }
>>> +
>>> + if (reg) {
>>> + pr_warn("mc34vr500: interrupt intstat0 %u\n", reg);
>>
>> Same as above, it could fill up the kernel log is the alarm interrupt
>> is triggered fast enough.
>>
>
> This isn't acceptable anyway. It logs a message with every interrupts.
> All those pr_{warn, notice} messages need to be dropped from the
> driver.
>
>>> +
>>> + if (reg & LOWVINS_BIT) {
>>> + ret = hwmon_notify_event(&client->dev, hwmon_in,
>>> + hwmon_in_alarm, 1);
>
> Notifications need to be on the hwmon device.
>
>>> + if (ret)
>>> + dev_err(&client->dev,
>>> + "mc34vr500: error, hwmon_notify_event() failed:
>>> %d\n",
>>> + ret);
>>> + }
>>> +
>>> + if (reg & THERM110S_BIT) {
>>> + ret = hwmon_notify_event(&client->dev, hwmon_temp,
>>> + hwmon_temp_alarm, 110);
>>> + if (ret)
>>> + dev_err(&client->dev,
>>> + "mc34vr500: error, hwmon_notify_event() failed:
>>> %d\n",
>>> + ret);
>>> + }
>
> hwmon_notify_event() only fails if bad data is passed to it, ie if the
> driver
> code is bad. Checking and logging it is only necessary if the code was
> never
> tested.
>
>>> +
>>> + reg = 0;
>>> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT0, reg);
>>> + if (ret) {
>>> + dev_err(&client->dev, "unable to enable intmask0
>>> interrupts\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT1, ®);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "unable to read mc34vr500 intsense1
>>> register\n");
>>> + return ret;
>>> + }
>>> +
>>> + if (reg) {
>>> + pr_warn("mc34vr500: interrupt intstat1 %u\n", reg);
>>> +
>>> + reg = 0;
>>> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT1, reg);
>>> + if (ret) {
>>> + dev_err(&client->dev, "unable to enable intmask1
>>> interrupts\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + ret = regmap_read(data->regmap, MC34VR500_INTSTAT4, ®);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "unable to read mc34vr500 intsense4
>>> register\n");
>>> + return ret;
>>> + }
>>> +
>>> + if (reg) {
>>> + pr_warn("mc34vr500: interrupt intstat4 %u\n", reg);
>>> +
>>> + reg = 0;
>>> + ret = regmap_write(data->regmap, MC34VR500_INTSTAT4, reg);
>>> + if (ret) {
>>> + dev_err(&client->dev, "unable to enable intmask4
>>> interrupts\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static const struct regmap_config mc34vr500_regmap_config = {
>>> + .reg_bits = 8,
>>> + .val_bits = 8,
>>> + .max_register = MC34VR500_PWRGD_EN,
>>> +};
>>> +
>>> +static int mc34vr500_probe(struct i2c_client *client)
>>> +{
>>> + struct device *dev = &client->dev;
>>> + struct mc34vr500_data *data;
>>> + struct device *hwmon_dev;
>>> + int ret;
>>> + unsigned int reg, revid, fabid;
>>> + struct regmap *regmap;
>>> + u32 interrupt_mask;
>>> +
>>> + regmap = devm_regmap_init_i2c(client, &mc34vr500_regmap_config);
>>> + if (IS_ERR(regmap)) {
>>> + dev_err(dev, "failed to allocate register map\n");
>>> + return PTR_ERR(regmap);
>>> + }
>>> +
>>> + data = devm_kzalloc(dev, sizeof(struct mc34vr500_data),
>>> GFP_KERNEL);
>>> + if (!data)
>>> + return -ENOMEM;
>>> +
>>> + data->regmap = regmap;
>>> + data->client = client;
>>> +
>>> + ret = regmap_read(regmap, MC34VR500_DEVICEID, ®);
>>> + if (ret < 0) {
>>> + dev_err(dev, "unable to read config register\n");
>>> +
>>> + return ret;
>>> + }
>>> +
>>> + if (reg != MC34VR500_DEVICEID_VALUE) {
>>> + dev_err(dev, "invalid config register value 0x%x\n", reg);
>>> +
>>> + return -ENODEV;
>>> + }
>
> This simply reflects that the chip isn't there and should result in
> -ENODEV
> without logging noise.
>
>>> +
>>> + ret = regmap_read(regmap, MC34VR500_SILICONREVID, &revid);
>>> + if (ret < 0) {
>>> + dev_err(dev, "unable to read mc34vr500 revid register\n");
>>> + return ret;
>>> + }
>>> +
>>> + ret = regmap_read(regmap, MC34VR500_FABID, &fabid);
>>> + if (ret < 0) {
>>> + dev_err(dev, "unable to read mc34vr500 fabid register\n");
>>> + return ret;
>>> + }
>>> +
>>> + pr_notice("mc34vr500: revid 0x%x fabid 0x%x\n", revid, fabid);
>>> +
>>> +
>>> + ret = of_property_read_u32(dev->of_node,
>>> + "interrupt-mask",
>>> + &interrupt_mask);
>>> + if (ret == -EINVAL) {
>>> + interrupt_mask = 0;
>>> + } else if (ret) {
>>> + dev_err(dev, "Error reading interrupt_mask\n");
>>> + return ret;
>>> + }
>>> +
>
> This is inappropriate. The driver needs to enable the interrupt bits it
> needs,
> and nothing else.
>
> If there is a desire to enable individual channels, that should be done
> with a per-channel deviretree file and with the {in, current,
> temp}X_enable
> sysfs attribute.
>
>>> + ret = regmap_read(regmap, MC34VR500_INTSTAT0, ®);
>>> + if (ret < 0) {
>>> + dev_err(dev, "unable to read mc34vr500 intstat0
>>> register\n");
>>> + return ret;
>>> + }
>>> + reg = reg & (~interrupt_mask);
>>> +
>>> + if (reg)
>>> + pr_notice("mc34vr500: intstat0: 0x%x\n", reg);
>>> +
>>> + if (reg & LOWVINS_BIT)
>>> + pr_notice("mc34vr500: low input voltage detected\n");
>>> +
>>> + if (reg & THERM130S_BIT)
>>> + pr_notice("mc34vr500: temperature >= 130°c\n");
>>> + else if (reg & THERM125S_BIT)
>>> + pr_notice("mc34vr500: temperature >= 125°c\n");
>>> + else if (reg & THERM120S_BIT)
>>> + pr_notice("mc34vr500: temperature >= 120°c\n");
>>> + else if (reg & THERM110S_BIT)
>>> + pr_notice("mc34vr500: temperature >= 110°c\n");
>>> +
>>> + ret = regmap_read(regmap, MC34VR500_INTSTAT1, ®);
>>> + if (ret < 0) {
>>> + dev_err(dev, "unable to read mc34vr500 intstat1
>>> register\n");
>>> + return ret;
>>> + }
>>> + reg = reg & ((~interrupt_mask) >> 8);
>>> +
>>> + if (reg)
>>> + pr_notice("mc34vr500: intstat1: 0x%x\n", reg);
>>> +
>>> + ret = regmap_read(regmap, MC34VR500_INTSTAT4, ®);
>>> + if (ret < 0) {
>>> + dev_err(dev, "unable to read mc34vr500 intstat4
>>> register\n");
>>> + return ret;
>>> + }
>>> + reg = reg & ((~interrupt_mask) >> 16);
>>> +
>>> + if (reg)
>>> + pr_notice("mc34vr500: intstat4: 0x%x\n", reg);
>>> +
>
> Most of the above does not belong in the probe function and needs to be
> dropped.
> It is not the responsibility of the probe function to log interrupt
> status
> values.
>
>>> + hwmon_dev = devm_hwmon_device_register_with_groups(dev,
>>> client->name,
>>> + data,
>>> + mc34vr500_groups);
>>> +
>>> + if (client->irq) {
>>> + pr_notice("mc34vr500: enabling IRQ...\n");
>>> +
>
> Example for driver noise. I could try to mark them all but gave up
> after
> realizing how noisy this driver is. This all needs to go.
>
>>> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
>>> + mc34vr500_process_interrupt,
>>> + IRQF_TRIGGER_RISING |
>>> + IRQF_ONESHOT |
>>> + IRQF_SHARED,
>>> + dev_name(dev),
>>> + data);
>>> + if (ret) {
>>> + dev_err(dev, "Error requesting irq\n");
>>> + return ret;
>>> + }
>>> +
>>> + /* clear interrupts */
>>> + reg = 0;
>>> + ret = regmap_write(regmap, MC34VR500_INTSTAT0, reg);
>>> + if (ret) {
>>> + dev_err(dev, "unable to write intstat0 register\n");
>>> + return ret;
>>> + }
>>> +
>>> + /* enable interrupts */
>>> + ret = regmap_write(regmap, MC34VR500_INTMASK0,
>>> interrupt_mask & 0xff);
>>> + if (ret) {
>>> + dev_err(dev, "unable to enable intmask0 interrupts\n");
>>> + return ret;
>>> + }
>>> +
>>> + reg = 0;
>>> + ret = regmap_write(regmap, MC34VR500_INTSTAT1, reg);
>>> + if (ret) {
>>> + dev_err(dev, "unable to write intstat1 register\n");
>>> + return ret;
>>> + }
>>> +
>>> + ret = regmap_write(regmap, MC34VR500_INTMASK1,
>>> (interrupt_mask >> 8) & 0xff);
>>> + if (ret) {
>>> + dev_err(dev, "unable to enable intmask1 interrupts\n");
>>> + return ret;
>>> + }
>>> +
>>> + reg = 0;
>>> + ret = regmap_write(regmap, MC34VR500_INTSTAT4, reg);
>>> + if (ret) {
>>> + dev_err(dev, "unable to write intstat1 register\n");
>>> + return ret;
>>> + }
>>> +
>>> + ret = regmap_write(regmap, MC34VR500_INTMASK4,
>>> (interrupt_mask >> 16) & 0xff);
>>> + if (ret) {
>>> + dev_err(dev, "unable to enable intmask1 interrupts\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + return PTR_ERR_OR_ZERO(hwmon_dev);
>>> +}
>>> +
>>> +static const struct i2c_device_id mc34vr500_id[] = {
>>> + { "mc34vr500", 0 },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, mc34vr500_id);
>>> +
>>> +static struct i2c_driver mc34vr500_driver = {
>>> + .driver = {
>>> + .name = "mc34vr500",
>>> + },
>>> + .probe_new = mc34vr500_probe,
>>> + .id_table = mc34vr500_id,
>>> +};
>>> +
>>> +module_i2c_driver(mc34vr500_driver);
>>> +
>>> +MODULE_AUTHOR("Mario Kicherer <dev@kicherer.org>");
>>> +
>>> +MODULE_DESCRIPTION("MC34VR500 driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/hwmon/mc34vr500_pmic.h
>>> b/drivers/hwmon/mc34vr500_pmic.h
>>> new file mode 100644
>>> index 000000000000..1543ac692d72
>>> --- /dev/null
>>> +++ b/drivers/hwmon/mc34vr500_pmic.h
>>> @@ -0,0 +1,172 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/*
>>> + * Copyright 2016 Freescale Semiconductor, Inc.
>>> + * Hou Zhiqiang <Zhiqiang.Hou@freescale.com>
>>> + */
>>> +
>>> +#ifndef __MC34VR500_H_
>>> +#define __MC34VR500_H_
>>> +
>>> +#define MC34VR500_I2C_ADDR 0x08
>>> +
>>> +/* Drivers name */
>>> +#define MC34VR500_REGULATOR_DRIVER "mc34vr500_regulator"
>>> +
>>> +/* Register map */
>>> +enum {
>>> + MC34VR500_DEVICEID = 0x00,
>>> +
>>> + MC34VR500_SILICONREVID = 0x03,
>>> + MC34VR500_FABID,
>>> + MC34VR500_INTSTAT0,
>>> + MC34VR500_INTMASK0,
>>> + MC34VR500_INTSENSE0,
>>> + MC34VR500_INTSTAT1,
>>> + MC34VR500_INTMASK1,
>>> + MC34VR500_INTSENSE1,
>>> +
>>> + MC34VR500_INTSTAT4 = 0x11,
>>> + MC34VR500_INTMASK4,
>>> + MC34VR500_INTSENSE4,
>>> +
>>> + MC34VR500_PWRCTL = 0x1B,
>>> +
>>> + MC34VR500_SW1VOLT = 0x2E,
>>> + MC34VR500_SW1STBY,
>>> + MC34VR500_SW1OFF,
>>> + MC34VR500_SW1MODE,
>>> + MC34VR500_SW1CONF,
>>> + MC34VR500_SW2VOLT,
>>> + MC34VR500_SW2STBY,
>>> + MC34VR500_SW2OFF,
>>> + MC34VR500_SW2MODE,
>>> + MC34VR500_SW2CONF,
>>> +
>>> + MC34VR500_SW3VOLT = 0x3C,
>>> + MC34VR500_SW3STBY,
>>> + MC34VR500_SW3OFF,
>>> + MC34VR500_SW3MODE,
>>> + MC34VR500_SW3CONF,
>>> +
>>> + MC34VR500_SW4VOLT = 0x4A,
>>> + MC34VR500_SW4STBY,
>>> + MC34VR500_SW4OFF,
>>> + MC34VR500_SW4MODE,
>>> + MC34VR500_SW4CONF,
>>> +
>>> + MC34VR500_REFOUTCRTRL = 0x6A,
>>> +
>>> + MC34VR500_LDO1CTL = 0x6D,
>>> + MC34VR500_LDO2CTL,
>>> + MC34VR500_LDO3CTL,
>>> + MC34VR500_LDO4CTL,
>>> + MC34VR500_LDO5CTL,
>>> +
>>> + MC34VR500_PAGE_REGISTER = 0x7F,
>>> +
>>> + /* Internal RAM */
>>> + MC34VR500_SW1_VOLT = 0xA8,
>>> + MC34VR500_SW1_SEQ,
>>> + MC34VR500_SW1_CONFIG,
>>> +
>>> + MC34VR500_SW2_VOLT = 0xAC,
>>> + MC34VR500_SW2_SEQ,
>>> + MC34VR500_SW2_CONFIG,
>>> +
>>> + MC34VR500_SW3_VOLT = 0xB0,
>>> + MC34VR500_SW3_SEQ,
>>> + MC34VR500_SW3_CONFIG,
>>> +
>>> + MC34VR500_SW4_VOLT = 0xB8,
>>> + MC34VR500_SW4_SEQ,
>>> + MC34VR500_SW4_CONFIG,
>>> +
>>> + MC34VR500_REFOUT_SEQ = 0xC4,
>>> +
>>> + MC34VR500_LDO1_VOLT = 0xCC,
>>> + MC34VR500_LDO1_SEQ,
>>> +
>>> + MC34VR500_LDO2_VOLT = 0xD0,
>>> + MC34VR500_LDO2_SEQ,
>>> +
>>> + MC34VR500_LDO3_VOLT = 0xD4,
>>> + MC34VR500_LDO3_SEQ,
>>> +
>>> + MC34VR500_LDO4_VOLT = 0xD8,
>>> + MC34VR500_LDO4_SEQ,
>>> +
>>> + MC34VR500_LDO5_VOLT = 0xDC,
>>> + MC34VR500_LDO5_SEQ,
>>> +
>>> + MC34VR500_PU_CONFIG1 = 0xE0,
>>> +
>>> + MC34VR500_TBB_POR = 0xE4,
>>> +
>>> + MC34VR500_PWRGD_EN = 0xE8,
>>> +
>>> + MC34VR500_NUM_OF_REGS,
>>> +};
>>> +
>>> +/* Registor offset based on SWxVOLT register */
>>> +#define MC34VR500_VOLT_OFFSET 0
>>> +#define MC34VR500_STBY_OFFSET 1
>>> +#define MC34VR500_OFF_OFFSET 2
>>> +#define MC34VR500_MODE_OFFSET 3
>>> +#define MC34VR500_CONF_OFFSET 4
>>> +
>>> +#define SW_MODE_MASK 0xf
>>> +#define SW_MODE_SHIFT 0
>>> +
>>> +#define LDO_VOL_MASK 0xf
>>> +#define LDO_EN (1 << 4)
>>> +#define LDO_MODE_SHIFT 4
>>> +#define LDO_MODE_MASK (1 << 4)
>>> +#define LDO_MODE_OFF 0
>>> +#define LDO_MODE_ON 1
>>> +
>>> +#define REFOUTEN (1 << 4)
>>> +
>>> +/*
>>> + * Regulator Mode Control
>>> + *
>>> + * OFF: The regulator is switched off and the output voltage is
>>> discharged.
>>> + * PFM: In this mode, the regulator is always in PFM mode, which is
>>> useful
>>> + * at light loads for optimized efficiency.
>>> + * PWM: In this mode, the regulator is always in PWM mode operation
>>> + * regardless of load conditions.
>>> + * APS: In this mode, the regulator moves automatically between
>>> pulse
>>> + * skipping mode and PWM mode depending on load conditions.
>>> + *
>>> + * SWxMODE[3:0]
>>> + * Normal Mode | Standby Mode | value
>>> + * OFF OFF 0x0
>>> + * PWM OFF 0x1
>>> + * PFM OFF 0x3
>>> + * APS OFF 0x4
>>> + * PWM PWM 0x5
>>> + * PWM APS 0x6
>>> + * APS APS 0x8
>>> + * APS PFM 0xc
>>> + * PWM PFM 0xd
>>> + */
>>> +#define OFF_OFF 0x0
>>> +#define PWM_OFF 0x1
>>> +#define PFM_OFF 0x3
>>> +#define APS_OFF 0x4
>>> +#define PWM_PWM 0x5
>>> +#define PWM_APS 0x6
>>> +#define APS_APS 0x8
>>> +#define APS_PFM 0xc
>>> +#define PWM_PFM 0xd
>>> +
>>> +enum swx {
>>> + SW1 = 0,
>>> + SW2,
>>> + SW3,
>>> + SW4,
>>> +};
>>> +
>>> +int mc34vr500_get_sw_volt(uint8_t sw);
>>> +int mc34vr500_set_sw_volt(uint8_t sw, int sw_volt);
>>> +int power_mc34vr500_init(unsigned char bus);
>>> +#endif /* __MC34VR500_PMIC_H_ */
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-17 15:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-16 13:42 [PATCH] hwmon: add initial NXP MC34VR500 PMIC monitoring support Mario Kicherer
2023-01-16 15:55 ` Armin Wolf
2023-01-16 16:48 ` Guenter Roeck
2023-01-17 15:55 ` Mario Kicherer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).