[PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Carsten Spieß]

Driver for Renesas ISL28022 power monitor with I2C interface.
The device monitors voltage, current via shunt resistor
and calculated power.

Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
---
 Documentation/hwmon/index.rst    |   1 +
 Documentation/hwmon/isl28022.rst |  56 +++++
 MAINTAINERS                      |   7 +
 drivers/hwmon/Kconfig            |  11 +
 drivers/hwmon/Makefile           |   1 +
 drivers/hwmon/isl28022.c         | 405 +++++++++++++++++++++++++++++++
 6 files changed, 481 insertions(+)
 create mode 100644 Documentation/hwmon/isl28022.rst
 create mode 100644 drivers/hwmon/isl28022.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index d11924667f76..c9548fc5c40e 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -90,6 +90,7 @@ Hardware Monitoring Kernel Drivers
    ir35221
    ir38064
    ir36021
+   isl28022
    isl68137
    it87
    jc42
diff --git a/Documentation/hwmon/isl28022.rst b/Documentation/hwmon/isl28022.rst
new file mode 100644
index 000000000000..84c27ddcd33e
--- /dev/null
+++ b/Documentation/hwmon/isl28022.rst
@@ -0,0 +1,56 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver isl28022
+======================
+
+Supported chips:
+
+  * Renesas ISL28022
+
+    Prefix: 'isl28022'
+
+    Addresses scanned: none
+
+    Datasheet: Publicly available at the Renesas website
+
+	       https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
+
+Author:
+    Carsten Spieß <mail@carsten-spiess.de>
+
+Description
+-----------
+
+The ISL28022 is a power monitor with I2C interface. The device monitors
+voltage, current via shunt resistor and calculated power.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+device explicitly. Please see Documentation/i2c/instantiating-devices.rst for
+details.
+
+The shunt value in micro-ohms, shunt gain and averaging can be set
+via device tree at compile-time.
+Please refer to the Documentation/devicetree/bindings/hwmon/isl,isl28022.yaml
+for bindings if the device tree is used.
+
+Sysfs entries
+-------------
+
+The following attributes are supported. All attributes are read-only.
+
+======================= =======================================================
+in0_input		shunt voltage (micro Volt)
+in1_input		bus voltage (milli Volt)
+
+curr1_input		current (milli Ampere)
+power1_input		power (micro Watt)
+
+note			current and power attributes are supported only when
+			shunt value is configured via device tree
+
+			shunt voltage is in micro Volt, not milli Volt,
+			to get useful values
+======================= =======================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 7abb5710e1bb..c61aa688cd11 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11072,6 +11072,13 @@ S:	Maintained
 F:	Documentation/filesystems/isofs.rst
 F:	fs/isofs/
 
+ISL28022 HARDWARE MONITORING DRIVER
+M:	Carsten Spieß <mail@carsten-spiess.de>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/isl28022.rst
+F:	drivers/hwmon/isl28022.c
+
 IT87 HARDWARE MONITORING DRIVER
 M:	Jean Delvare <jdelvare@suse.com>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 2913299c2c9e..3049c519e6ac 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -800,6 +800,17 @@ config SENSORS_CORETEMP
 	  sensor inside your CPU. Most of the family 6 CPUs
 	  are supported. Check Documentation/hwmon/coretemp.rst for details.
 
+config SENSORS_ISL28022
+	tristate "Renesas ISL28022"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for ISL28022 power monitor.
+	  Check Documentation/hwmon/isl28022.rst for details.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called isl28022.
+
 config SENSORS_IT87
 	tristate "ITE IT87xx and compatibles"
 	depends on !PPC
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index ff6bfd109c72..4046fed7f48d 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_SENSORS_INA2XX)	+= ina2xx.o
 obj-$(CONFIG_SENSORS_INA238)	+= ina238.o
 obj-$(CONFIG_SENSORS_INA3221)	+= ina3221.o
 obj-$(CONFIG_SENSORS_INTEL_M10_BMC_HWMON) += intel-m10-bmc-hwmon.o
+obj-$(CONFIG_SENSORS_ISL28022)	+= isl28022.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
new file mode 100644
index 000000000000..67cf47c31600
--- /dev/null
+++ b/drivers/hwmon/isl28022.c
@@ -0,0 +1,405 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * isl28022.c - driver for Renesas ISL28022 power monitor chip
+ *	 monitoring
+ * Copyright (c) 2023 Carsten Spieß <mail@carsten-spiess.de>
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/util_macros.h>
+#include <linux/regulator/consumer.h>
+
+/* ISL28022 registers */
+#define ISL28022_REG_CONFIG	0x00
+#define ISL28022_REG_SHUNT	0x01
+#define ISL28022_REG_BUS	0x02
+#define ISL28022_REG_POWER	0x03
+#define ISL28022_REG_CURRENT	0x04
+#define ISL28022_REG_CALIB	0x05
+#define ISL28022_REG_SHUNT_THR	0x06
+#define ISL28022_REG_BUS_THR	0x07
+#define ISL28022_REG_INT	0x08
+#define ISL28022_REG_AUX	0x09
+#define ISL28022_REG_MAX	ISL28022_REG_AUX
+
+/* ISL28022 config flags */
+/* mode flags */
+#define ISL28022_MODE_SHIFT	0
+#define ISL28022_MODE_MASK	0x0007
+
+#define ISL28022_MODE_PWR_DOWN	0x0
+#define ISL28022_MODE_TRG_S	0x1
+#define ISL28022_MODE_TRG_B	0x2
+#define ISL28022_MODE_TRG_SB	0x3
+#define ISL28022_MODE_ADC_OFF	0x4
+#define ISL28022_MODE_CONT_S	0x5
+#define ISL28022_MODE_CONT_B	0x6
+#define ISL28022_MODE_CONT_SB	0x7
+
+/* shunt ADC settings */
+#define ISL28022_SADC_SHIFT	3
+#define ISL28022_SADC_MASK	0x0078
+
+#define ISL28022_BADC_SHIFT	7
+#define ISL28022_BADC_MASK	0x0780
+
+#define ISL28022_ADC_12		0x0	/* 12 bit ADC */
+#define ISL28022_ADC_13		0x1	/* 13 bit ADC */
+#define ISL28022_ADC_14		0x2	/* 14 bit ADC */
+#define ISL28022_ADC_15		0x3	/* 15 bit ADC */
+#define ISL28022_ADC_15_1	0x8	/* 15 bit ADC, 1 sample */
+#define ISL28022_ADC_15_2	0x9	/* 15 bit ADC, 2 samples */
+#define ISL28022_ADC_15_4	0xA	/* 15 bit ADC, 4 samples */
+#define ISL28022_ADC_15_8	0xB	/* 15 bit ADC, 8 samples */
+#define ISL28022_ADC_15_16	0xC	/* 15 bit ADC, 16 samples */
+#define ISL28022_ADC_15_32	0xD	/* 15 bit ADC, 32 samples */
+#define ISL28022_ADC_15_64	0xE	/* 15 bit ADC, 64 samples */
+#define ISL28022_ADC_15_128	0xF	/* 15 bit ADC, 128 samples */
+
+/* shunt voltage range */
+#define ISL28022_PG_SHIFT	11
+#define ISL28022_PG_MASK	0x1800
+
+#define ISL28022_PG_40		0x0	/* +/-40 mV */
+#define ISL28022_PG_80		0x1	/* +/-80 mV */
+#define ISL28022_PG_160		0x2	/* +/-160 mV */
+#define ISL28022_PG_320		0x3	/* +/-3200 mV */
+
+/* bus voltage range */
+#define ISL28022_BRNG_SHIFT	13
+#define ISL28022_BRNG_MASK	0x6000
+
+#define ISL28022_BRNG_16	0x0	/* 16 V */
+#define ISL28022_BRNG_32	0x1	/* 32 V */
+#define ISL28022_BRNG_60	0x3	/* 60 V */
+
+/* reset */
+#define ISL28022_RESET		0x8000
+
+struct isl28022_data {
+	struct i2c_client	*client;
+	struct regmap		*regmap;
+	u32			shunt;
+	u32			gain;
+	u32			average;
+	u16			config;
+	u16			calib;
+};
+
+static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long *val)
+{
+	struct isl28022_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	int err;
+
+	switch (type) {
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			err = regmap_read(data->regmap,
+					  ISL28022_REG_SHUNT + channel, &regval);
+			if (err < 0)
+				return err;
+			*val = (channel == 0) ?
+					(long)((s16)((u16)regval)) * 10 :
+					(long)(((u16)regval) & 0xFFFC);
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			err = regmap_read(data->regmap,
+					  ISL28022_REG_CURRENT, &regval);
+			if (err < 0)
+				return err;
+			if (!data->shunt)
+				return -EINVAL;
+			*val = ((long)regval * 10000L * (long)data->gain) /
+				(long)(8 * data->shunt);
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+			err = regmap_read(data->regmap,
+					  ISL28022_REG_POWER, &regval);
+			if (err < 0)
+				return err;
+			if (!data->shunt)
+				return -EINVAL;
+			*val = ((long)regval * 409600000L * (long)data->gain) /
+				(long)(8 * data->shunt);
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static umode_t isl28022_is_visible(const void *data, enum hwmon_sensor_types type,
+				   u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			return 0444;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			return 0444;
+		}
+		break;
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+			return 0444;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static const struct hwmon_channel_info *isl28022_info[] = {
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
+			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
+	NULL
+};
+
+static const struct hwmon_channel_info *isl28022_info_shunt[] = {
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
+			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT), /* channel 1: current (mA) */
+	HWMON_CHANNEL_INFO(power,
+			   HWMON_P_INPUT),	/* channel 1: power (µW) */
+	NULL
+};
+
+static const struct hwmon_ops isl28022_hwmon_ops = {
+	.is_visible = isl28022_is_visible,
+	.read = isl28022_read,
+};
+
+static const struct hwmon_chip_info isl28022_chip_info = {
+	.ops = &isl28022_hwmon_ops,
+	.info = isl28022_info,
+};
+
+static const struct hwmon_chip_info isl28022_chip_info_shunt = {
+	.ops = &isl28022_hwmon_ops,
+	.info = isl28022_info_shunt,
+};
+
+static bool isl28022_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ISL28022_REG_CONFIG:
+	case ISL28022_REG_CALIB:
+	case ISL28022_REG_SHUNT_THR:
+	case ISL28022_REG_BUS_THR:
+	case ISL28022_REG_INT:
+	case ISL28022_REG_AUX:
+		return true;
+	}
+
+	return false;
+}
+
+static bool isl28022_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case ISL28022_REG_CONFIG:
+	case ISL28022_REG_SHUNT:
+	case ISL28022_REG_BUS:
+	case ISL28022_REG_POWER:
+	case ISL28022_REG_CURRENT:
+	case ISL28022_REG_INT:
+	case ISL28022_REG_AUX:
+		return true;
+	}
+	return true;
+}
+
+static const struct regmap_config isl28022_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = ISL28022_REG_MAX,
+	.writeable_reg = isl28022_is_writeable_reg,
+	.volatile_reg = isl28022_is_volatile_reg,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.cache_type = REGCACHE_RBTREE,
+	.use_single_read = true,
+	.use_single_write = true,
+};
+
+static const struct i2c_device_id isl28022_ids[] = {
+	{ "isl28022", 0},
+	{ /* LIST END */ }
+};
+MODULE_DEVICE_TABLE(i2c, isl28022_ids);
+
+static int isl28022_config(struct device *dev)
+{
+	struct isl28022_data *data = dev_get_drvdata(dev);
+
+	if (!dev || !data)
+		return -EINVAL;
+				;
+	data->config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) |
+			(ISL28022_BRNG_60 << ISL28022_BRNG_SHIFT);
+
+	switch (data->gain) {
+	case 1:
+		data->config |= (ISL28022_PG_40 << ISL28022_PG_SHIFT);
+		break;
+	case 2:
+		data->config |= (ISL28022_PG_80 << ISL28022_PG_SHIFT);
+		break;
+	case 4:
+		data->config |= (ISL28022_PG_160 << ISL28022_PG_SHIFT);
+		break;
+	default:
+		data->config |= (ISL28022_PG_320 << ISL28022_PG_SHIFT);
+		data->gain = 8;
+		break;
+	}
+
+	data->calib = data->shunt ? 0x8000 / data->gain : 0;
+
+	switch (data->average) {
+	case 128:
+		data->config |= (ISL28022_ADC_15_128 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_128 << ISL28022_BADC_SHIFT);
+		break;
+	case 64:
+		data->config |= (ISL28022_ADC_15_64 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_64 << ISL28022_BADC_SHIFT);
+		break;
+	case 32:
+		data->config |= (ISL28022_ADC_15_32 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_32 << ISL28022_BADC_SHIFT);
+		break;
+	case 16:
+		data->config |= (ISL28022_ADC_15_16 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_16 << ISL28022_BADC_SHIFT);
+		break;
+	case 8:
+		data->config |= (ISL28022_ADC_15_8 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_8 << ISL28022_BADC_SHIFT);
+		break;
+	case 4:
+		data->config |= (ISL28022_ADC_15_4 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_4 << ISL28022_BADC_SHIFT);
+		break;
+	case 2:
+		data->config |= (ISL28022_ADC_15_2 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_2 << ISL28022_BADC_SHIFT);
+		break;
+	case 1:
+		data->config |= (ISL28022_ADC_15_1 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15_1 << ISL28022_BADC_SHIFT);
+		break;
+	default:
+		data->config |= (ISL28022_ADC_15 << ISL28022_SADC_SHIFT) |
+				(ISL28022_ADC_15 << ISL28022_BADC_SHIFT);
+		data->average = 0;
+		break;
+	}
+
+	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
+	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);
+
+	return 0;
+}
+
+static int isl28022_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct isl28022_data *data;
+	int status;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -EIO;
+
+	data = devm_kzalloc(dev, sizeof(struct isl28022_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+
+	of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &data->shunt);
+	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
+	of_property_read_u32(dev->of_node, "average", &data->average);
+
+	data->regmap = devm_regmap_init_i2c(client, &isl28022_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "isl28022_hwmon",
+							 data, data->shunt ?
+							 &isl28022_chip_info_shunt :
+							 &isl28022_chip_info, NULL);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	status = isl28022_config(hwmon_dev);
+	if (status)
+		return status;
+
+	dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused isl28022_of_match[] = {
+	{ .compatible = "renesas,isl28022"},
+	{ /* LIST END */ }
+};
+MODULE_DEVICE_TABLE(of, isl28022_of_match);
+
+static struct i2c_driver isl28022_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "isl28022",
+		.of_match_table = of_match_ptr(isl28022_of_match),
+	},
+	.probe_new	= isl28022_probe,
+	.id_table	= isl28022_ids,
+};
+
+module_i2c_driver(isl28022_driver);
+
+MODULE_AUTHOR("Carsten Spiess <mail@carsten-spiess.de>");
+MODULE_DESCRIPTION("ISL28022 driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Carsten Spieß]

Add dt-bindings for Renesase ISL28022 power monitor.

Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
---
 .../bindings/hwmon/renesas,isl28022.yaml      | 67 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
new file mode 100644
index 000000000000..5ecf892db269
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/renesas,isl28022.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas ISL28022 power monitor
+
+maintainers:
+  - Carsten Spieß <mail@carsten-spiess.de>
+
+description: |
+  The ISL28022 is a power monitor with I2C interface. The device monitors
+  voltage, current via shunt resistor and calculated power.
+
+  Datasheets:
+    https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
+
+Required properties:
+  compatible:
+    enum:
+      - renesas,isl28022
+
+  reg:
+    maxItems: 1
+
+Optional properties:
+  shunt-resistor-micro-ohms:
+    description:
+      Shunt resistor value in micro-Ohm
+      defaults to <0> when not set
+      monitoring of current and power not supported when <0>
+
+  shunt-gain:
+    description:
+      Shunt gain to scale maximal shunt voltage to
+      40mV, 80mV, 160mV, 320mV
+      defaults to <8> (320mV) when not set
+    enum: [1, 2, 4, 8]
+
+  average:
+    description: |
+      Number of samples to be used to report voltage, current and power values.
+      defaults to <0> when not set
+    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-monitor@40 {
+            compatible = "renesas,isl28022";
+            reg = <0x10>;
+            shunt-resistor-micro-ohms = <8000>;
+            shunt-gain = <1>;
+            average = <128>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index c61aa688cd11..ec9b97ace50b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11076,6 +11076,7 @@ ISL28022 HARDWARE MONITORING DRIVER
 M:	Carsten Spieß <mail@carsten-spiess.de>
 L:	linux-hwmon@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
 F:	Documentation/hwmon/isl28022.rst
 F:	drivers/hwmon/isl28022.c
 
-- 
2.34.1

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Krzysztof Kozlowski]

On 26/07/2023 17:22, Carsten Spieß wrote:
> Add dt-bindings for Renesase ISL28022 power monitor.
> 
> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
> ---

Thank you for your patch. There is something to discuss/improve.


>  .../bindings/hwmon/renesas,isl28022.yaml      | 67 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> new file mode 100644
> index 000000000000..5ecf892db269
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +

No blank line.

> +$id: http://devicetree.org/schemas/hwmon/renesas,isl28022.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas ISL28022 power monitor
> +
> +maintainers:
> +  - Carsten Spieß <mail@carsten-spiess.de>
> +
> +description: |
> +  The ISL28022 is a power monitor with I2C interface. The device monitors
> +  voltage, current via shunt resistor and calculated power.
> +
> +  Datasheets:
> +    https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
> +
> +Required properties:
> +  compatible:
> +    enum:
> +      - renesas,isl28022
> +
> +  reg:
> +    maxItems: 1
> +
> +Optional properties:

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

These are "properties"

> +  shunt-resistor-micro-ohms:
> +    description:
> +      Shunt resistor value in micro-Ohm
> +      defaults to <0> when not set
> +      monitoring of current and power not supported when <0>
> +
> +  shunt-gain:

1. Missing vendor prefix (does not look like generic property)
2. -microvolt

> +    description:
> +      Shunt gain to scale maximal shunt voltage to
> +      40mV, 80mV, 160mV, 320mV
> +      defaults to <8> (320mV) when not set

And then enum is for 40, 80, 160 and 320.

> +    enum: [1, 2, 4, 8]
> +
> +  average:
> +    description: |
> +      Number of samples to be used to report voltage, current and power values.
> +      defaults to <0> when not set
> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]

I am sure hwmon has some property for this. Are you sure it is called
"average"?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +


Best regards,
Krzysztof

Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Guenter Roeck]

On 7/26/23 08:22, Carsten Spieß wrote:
> Driver for Renesas ISL28022 power monitor with I2C interface.
> The device monitors voltage, current via shunt resistor
> and calculated power.
> 
> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
> ---
>   Documentation/hwmon/index.rst    |   1 +
>   Documentation/hwmon/isl28022.rst |  56 +++++
>   MAINTAINERS                      |   7 +
>   drivers/hwmon/Kconfig            |  11 +
>   drivers/hwmon/Makefile           |   1 +
>   drivers/hwmon/isl28022.c         | 405 +++++++++++++++++++++++++++++++
>   6 files changed, 481 insertions(+)
>   create mode 100644 Documentation/hwmon/isl28022.rst
>   create mode 100644 drivers/hwmon/isl28022.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index d11924667f76..c9548fc5c40e 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -90,6 +90,7 @@ Hardware Monitoring Kernel Drivers
>      ir35221
>      ir38064
>      ir36021
> +   isl28022
>      isl68137
>      it87
>      jc42
> diff --git a/Documentation/hwmon/isl28022.rst b/Documentation/hwmon/isl28022.rst
> new file mode 100644
> index 000000000000..84c27ddcd33e
> --- /dev/null
> +++ b/Documentation/hwmon/isl28022.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver isl28022
> +======================
> +
> +Supported chips:
> +
> +  * Renesas ISL28022
> +
> +    Prefix: 'isl28022'
> +
> +    Addresses scanned: none
> +
> +    Datasheet: Publicly available at the Renesas website
> +
> +	       https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
> +
> +Author:
> +    Carsten Spieß <mail@carsten-spiess.de>
> +
> +Description
> +-----------
> +
> +The ISL28022 is a power monitor with I2C interface. The device monitors
> +voltage, current via shunt resistor and calculated power.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +device explicitly. Please see Documentation/i2c/instantiating-devices.rst for
> +details.
> +
> +The shunt value in micro-ohms, shunt gain and averaging can be set
> +via device tree at compile-time.

At _compile-time_ ?

> +Please refer to the Documentation/devicetree/bindings/hwmon/isl,isl28022.yaml
> +for bindings if the device tree is used.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. All attributes are read-only.
> +
> +======================= =======================================================
> +in0_input		shunt voltage (micro Volt)

No. You must not change the ABI like that.

> +in1_input		bus voltage (milli Volt)
> +
> +curr1_input		current (milli Ampere)
> +power1_input		power (micro Watt)
> +
> +note			current and power attributes are supported only when
> +			shunt value is configured via device tree

No. Use a reasonable default if there are no devicetree properties.
> +
> +			shunt voltage is in micro Volt, not milli Volt,
> +			to get useful values

I'd argue that shunt voltage is all but useless, but if you want to have it supported
it _has_ to be in mV.

Why not support limit attributes ?

> +======================= =======================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7abb5710e1bb..c61aa688cd11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11072,6 +11072,13 @@ S:	Maintained
>   F:	Documentation/filesystems/isofs.rst
>   F:	fs/isofs/
>   
> +ISL28022 HARDWARE MONITORING DRIVER
> +M:	Carsten Spieß <mail@carsten-spiess.de>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/isl28022.rst
> +F:	drivers/hwmon/isl28022.c
> +
>   IT87 HARDWARE MONITORING DRIVER
>   M:	Jean Delvare <jdelvare@suse.com>
>   L:	linux-hwmon@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2913299c2c9e..3049c519e6ac 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -800,6 +800,17 @@ config SENSORS_CORETEMP
>   	  sensor inside your CPU. Most of the family 6 CPUs
>   	  are supported. Check Documentation/hwmon/coretemp.rst for details.
>   
> +config SENSORS_ISL28022
> +	tristate "Renesas ISL28022"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for ISL28022 power monitor.
> +	  Check Documentation/hwmon/isl28022.rst for details.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called isl28022.
> +
>   config SENSORS_IT87
>   	tristate "ITE IT87xx and compatibles"
>   	depends on !PPC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index ff6bfd109c72..4046fed7f48d 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_SENSORS_INA2XX)	+= ina2xx.o
>   obj-$(CONFIG_SENSORS_INA238)	+= ina238.o
>   obj-$(CONFIG_SENSORS_INA3221)	+= ina3221.o
>   obj-$(CONFIG_SENSORS_INTEL_M10_BMC_HWMON) += intel-m10-bmc-hwmon.o
> +obj-$(CONFIG_SENSORS_ISL28022)	+= isl28022.o
>   obj-$(CONFIG_SENSORS_IT87)	+= it87.o
>   obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
>   obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
> diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
> new file mode 100644
> index 000000000000..67cf47c31600
> --- /dev/null
> +++ b/drivers/hwmon/isl28022.c
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * isl28022.c - driver for Renesas ISL28022 power monitor chip
> + *	 monitoring
> + * Copyright (c) 2023 Carsten Spieß <mail@carsten-spiess.de>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>
> +#include <linux/regulator/consumer.h>
> +
> +/* ISL28022 registers */
> +#define ISL28022_REG_CONFIG	0x00
> +#define ISL28022_REG_SHUNT	0x01
> +#define ISL28022_REG_BUS	0x02
> +#define ISL28022_REG_POWER	0x03
> +#define ISL28022_REG_CURRENT	0x04
> +#define ISL28022_REG_CALIB	0x05
> +#define ISL28022_REG_SHUNT_THR	0x06
> +#define ISL28022_REG_BUS_THR	0x07
> +#define ISL28022_REG_INT	0x08
> +#define ISL28022_REG_AUX	0x09
> +#define ISL28022_REG_MAX	ISL28022_REG_AUX
> +
> +/* ISL28022 config flags */
> +/* mode flags */
> +#define ISL28022_MODE_SHIFT	0
> +#define ISL28022_MODE_MASK	0x0007
> +
> +#define ISL28022_MODE_PWR_DOWN	0x0
> +#define ISL28022_MODE_TRG_S	0x1
> +#define ISL28022_MODE_TRG_B	0x2
> +#define ISL28022_MODE_TRG_SB	0x3
> +#define ISL28022_MODE_ADC_OFF	0x4
> +#define ISL28022_MODE_CONT_S	0x5
> +#define ISL28022_MODE_CONT_B	0x6
> +#define ISL28022_MODE_CONT_SB	0x7
> +
> +/* shunt ADC settings */
> +#define ISL28022_SADC_SHIFT	3
> +#define ISL28022_SADC_MASK	0x0078
> +
> +#define ISL28022_BADC_SHIFT	7
> +#define ISL28022_BADC_MASK	0x0780
> +
> +#define ISL28022_ADC_12		0x0	/* 12 bit ADC */
> +#define ISL28022_ADC_13		0x1	/* 13 bit ADC */
> +#define ISL28022_ADC_14		0x2	/* 14 bit ADC */
> +#define ISL28022_ADC_15		0x3	/* 15 bit ADC */
> +#define ISL28022_ADC_15_1	0x8	/* 15 bit ADC, 1 sample */
> +#define ISL28022_ADC_15_2	0x9	/* 15 bit ADC, 2 samples */
> +#define ISL28022_ADC_15_4	0xA	/* 15 bit ADC, 4 samples */
> +#define ISL28022_ADC_15_8	0xB	/* 15 bit ADC, 8 samples */
> +#define ISL28022_ADC_15_16	0xC	/* 15 bit ADC, 16 samples */
> +#define ISL28022_ADC_15_32	0xD	/* 15 bit ADC, 32 samples */
> +#define ISL28022_ADC_15_64	0xE	/* 15 bit ADC, 64 samples */
> +#define ISL28022_ADC_15_128	0xF	/* 15 bit ADC, 128 samples */
> +
> +/* shunt voltage range */
> +#define ISL28022_PG_SHIFT	11
> +#define ISL28022_PG_MASK	0x1800
> +
> +#define ISL28022_PG_40		0x0	/* +/-40 mV */
> +#define ISL28022_PG_80		0x1	/* +/-80 mV */
> +#define ISL28022_PG_160		0x2	/* +/-160 mV */
> +#define ISL28022_PG_320		0x3	/* +/-3200 mV */
> +
> +/* bus voltage range */
> +#define ISL28022_BRNG_SHIFT	13
> +#define ISL28022_BRNG_MASK	0x6000
> +
> +#define ISL28022_BRNG_16	0x0	/* 16 V */
> +#define ISL28022_BRNG_32	0x1	/* 32 V */
> +#define ISL28022_BRNG_60	0x3	/* 60 V */
> +
> +/* reset */
> +#define ISL28022_RESET		0x8000
> +
> +struct isl28022_data {
> +	struct i2c_client	*client;
> +	struct regmap		*regmap;
> +	u32			shunt;
> +	u32			gain;
> +	u32			average;
> +	u16			config;
> +	u16			calib;
> +};
> +
> +static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long *val)
> +{
> +	struct isl28022_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_SHUNT + channel, &regval);

That never reads REG_BUS.

> +			if (err < 0)
> +				return err;
> +			*val = (channel == 0) ?
> +					(long)((s16)((u16)regval)) * 10 :
> +					(long)(((u16)regval) & 0xFFFC);

I don't think the sign extensions are correct based on the datasheet.
This will have to use sign_extend.

> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_CURRENT, &regval);
> +			if (err < 0)
> +				return err;
> +			if (!data->shunt)
> +				return -EINVAL;

Getting an error message each time the "sensors" command is executed ?
Unacceptable.

> +			*val = ((long)regval * 10000L * (long)data->gain) /
> +				(long)(8 * data->shunt);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_POWER, &regval);
> +			if (err < 0)
> +				return err;
> +			if (!data->shunt)
> +				return -EINVAL;

Unacceptable.

> +			*val = ((long)regval * 409600000L * (long)data->gain) /
> +				(long)(8 * data->shunt);

I don't think this was checked for overflows.

> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static umode_t isl28022_is_visible(const void *data, enum hwmon_sensor_types type,
> +				   u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			return 0444;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *isl28022_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
> +			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
> +	NULL
> +};
> +
> +static const struct hwmon_channel_info *isl28022_info_shunt[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
> +			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT), /* channel 1: current (mA) */
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT),	/* channel 1: power (µW) */
> +	NULL
> +};
> +
> +static const struct hwmon_ops isl28022_hwmon_ops = {
> +	.is_visible = isl28022_is_visible,
> +	.read = isl28022_read,
> +};
> +
> +static const struct hwmon_chip_info isl28022_chip_info = {
> +	.ops = &isl28022_hwmon_ops,
> +	.info = isl28022_info,
> +};
> +
> +static const struct hwmon_chip_info isl28022_chip_info_shunt = {
> +	.ops = &isl28022_hwmon_ops,
> +	.info = isl28022_info_shunt,
> +};
> +
> +static bool isl28022_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ISL28022_REG_CONFIG:
> +	case ISL28022_REG_CALIB:
> +	case ISL28022_REG_SHUNT_THR:
> +	case ISL28022_REG_BUS_THR:
> +	case ISL28022_REG_INT:
> +	case ISL28022_REG_AUX:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool isl28022_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ISL28022_REG_CONFIG:
> +	case ISL28022_REG_SHUNT:
> +	case ISL28022_REG_BUS:
> +	case ISL28022_REG_POWER:
> +	case ISL28022_REG_CURRENT:
> +	case ISL28022_REG_INT:
> +	case ISL28022_REG_AUX:
> +		return true;
> +	}
> +	return true;
> +}
> +
> +static const struct regmap_config isl28022_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = ISL28022_REG_MAX,
> +	.writeable_reg = isl28022_is_writeable_reg,
> +	.volatile_reg = isl28022_is_volatile_reg,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +	.cache_type = REGCACHE_RBTREE,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +};
> +
> +static const struct i2c_device_id isl28022_ids[] = {
> +	{ "isl28022", 0},
> +	{ /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, isl28022_ids);
> +
> +static int isl28022_config(struct device *dev)
> +{
> +	struct isl28022_data *data = dev_get_drvdata(dev);
> +
> +	if (!dev || !data)
> +		return -EINVAL;

How would this ever happen ?

> +				;
> +	data->config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) |
> +			(ISL28022_BRNG_60 << ISL28022_BRNG_SHIFT);
> +
> +	switch (data->gain) {
> +	case 1:
> +		data->config |= (ISL28022_PG_40 << ISL28022_PG_SHIFT);
> +		break;
> +	case 2:
> +		data->config |= (ISL28022_PG_80 << ISL28022_PG_SHIFT);
> +		break;
> +	case 4:
> +		data->config |= (ISL28022_PG_160 << ISL28022_PG_SHIFT);
> +		break;
> +	default:
> +		data->config |= (ISL28022_PG_320 << ISL28022_PG_SHIFT);
> +		data->gain = 8;
> +		break;
> +	}
> +
> +	data->calib = data->shunt ? 0x8000 / data->gain : 0;
> +
> +	switch (data->average) {
> +	case 128:
> +		data->config |= (ISL28022_ADC_15_128 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_128 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 64:
> +		data->config |= (ISL28022_ADC_15_64 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_64 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 32:
> +		data->config |= (ISL28022_ADC_15_32 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_32 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 16:
> +		data->config |= (ISL28022_ADC_15_16 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_16 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 8:
> +		data->config |= (ISL28022_ADC_15_8 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_8 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 4:
> +		data->config |= (ISL28022_ADC_15_4 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_4 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 2:
> +		data->config |= (ISL28022_ADC_15_2 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_2 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 1:
> +		data->config |= (ISL28022_ADC_15_1 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_1 << ISL28022_BADC_SHIFT);
> +		break;
> +	default:
> +		data->config |= (ISL28022_ADC_15 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15 << ISL28022_BADC_SHIFT);
> +		data->average = 0;
> +		break;
> +	}
> +
> +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
> +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);

Error checking needed.

> +
> +	return 0;
> +}
> +
> +static int isl28022_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct isl28022_data *data;
> +	int status;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -EIO;

This is not an IO error. Return -ENODEV as most other drivers do.

> +
> +	data = devm_kzalloc(dev, sizeof(struct isl28022_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &data->shunt);
> +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
> +	of_property_read_u32(dev->of_node, "average", &data->average);

Check for errors and provide defaults if properties are not set.
Also please use device_property_read_u32() to enable use from non-devicetree
systems.

> +
> +	data->regmap = devm_regmap_init_i2c(client, &isl28022_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "isl28022_hwmon",
> +							 data, data->shunt ?
> +							 &isl28022_chip_info_shunt :
> +							 &isl28022_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	status = isl28022_config(hwmon_dev);
> +	if (status)
> +		return status;

That has to happen before the call to devm_hwmon_device_register_with_info()
to avoid race conditions.

> +
> +	dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused isl28022_of_match[] = {
> +	{ .compatible = "renesas,isl28022"},
> +	{ /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(of, isl28022_of_match);
> +
> +static struct i2c_driver isl28022_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "isl28022",
> +		.of_match_table = of_match_ptr(isl28022_of_match),

Drop of_match_ptr()

> +	},
> +	.probe_new	= isl28022_probe,
> +	.id_table	= isl28022_ids,
> +};
> +
> +module_i2c_driver(isl28022_driver);
> +
> +MODULE_AUTHOR("Carsten Spiess <mail@carsten-spiess.de>");
> +MODULE_DESCRIPTION("ISL28022 driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Guenter Roeck]

On 7/26/23 08:22, Carsten Spieß wrote:
> Add dt-bindings for Renesase ISL28022 power monitor.
> 
> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
> ---
>   .../bindings/hwmon/renesas,isl28022.yaml      | 67 +++++++++++++++++++
>   MAINTAINERS                                   |  1 +
>   2 files changed, 68 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> new file mode 100644
> index 000000000000..5ecf892db269
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/renesas,isl28022.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas ISL28022 power monitor
> +
> +maintainers:
> +  - Carsten Spieß <mail@carsten-spiess.de>
> +
> +description: |
> +  The ISL28022 is a power monitor with I2C interface. The device monitors
> +  voltage, current via shunt resistor and calculated power.
> +
> +  Datasheets:
> +    https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
> +
> +Required properties:
> +  compatible:
> +    enum:
> +      - renesas,isl28022
> +
> +  reg:
> +    maxItems: 1
> +
> +Optional properties:
> +  shunt-resistor-micro-ohms:
> +    description:
> +      Shunt resistor value in micro-Ohm
> +      defaults to <0> when not set
> +      monitoring of current and power not supported when <0>
> +

Should not default to 0 (disabled).

> +  shunt-gain:
> +    description:
> +      Shunt gain to scale maximal shunt voltage to
> +      40mV, 80mV, 160mV, 320mV
> +      defaults to <8> (320mV) when not set
> +    enum: [1, 2, 4, 8]
> +
> +  average:
> +    description: |
> +      Number of samples to be used to report voltage, current and power values.
> +      defaults to <0> when not set
> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]

Should not default to 0 (disabled).

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        power-monitor@40 {
> +            compatible = "renesas,isl28022";
> +            reg = <0x10>;
> +            shunt-resistor-micro-ohms = <8000>;
> +            shunt-gain = <1>;
> +            average = <128>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c61aa688cd11..ec9b97ace50b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11076,6 +11076,7 @@ ISL28022 HARDWARE MONITORING DRIVER
>   M:	Carsten Spieß <mail@carsten-spiess.de>
>   L:	linux-hwmon@vger.kernel.org
>   S:	Maintained
> +F:	Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>   F:	Documentation/hwmon/isl28022.rst
>   F:	drivers/hwmon/isl28022.c
>   

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Krzysztof Kozlowski]

On 26/07/2023 18:14, Krzysztof Kozlowski wrote:
> 
>> +  shunt-resistor-micro-ohms:
>> +    description:
>> +      Shunt resistor value in micro-Ohm
>> +      defaults to <0> when not set
>> +      monitoring of current and power not supported when <0>
>> +
>> +  shunt-gain:
> 
> 1. Missing vendor prefix (does not look like generic property)
> 2. -microvolt
> 
>> +    description:
>> +      Shunt gain to scale maximal shunt voltage to
>> +      40mV, 80mV, 160mV, 320mV
>> +      defaults to <8> (320mV) when not set
> 
> And then enum is for 40, 80, 160 and 320.

Also use "default: X" to choose default, instead of free-form text.

Best regards,
Krzysztof

Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Guenter Roeck]

On 7/26/23 08:22, Carsten Spieß wrote:
> Driver for Renesas ISL28022 power monitor with I2C interface.
> The device monitors voltage, current via shunt resistor
> and calculated power.
> 
> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>

Please provide a register dump (using i2cdump) for this chip. I strongly suspect
that the conversions will result in overflows and that they are not always correct.
I'll want to write unit test code before accepting the driver.

Guenter

> ---
>   Documentation/hwmon/index.rst    |   1 +
>   Documentation/hwmon/isl28022.rst |  56 +++++
>   MAINTAINERS                      |   7 +
>   drivers/hwmon/Kconfig            |  11 +
>   drivers/hwmon/Makefile           |   1 +
>   drivers/hwmon/isl28022.c         | 405 +++++++++++++++++++++++++++++++
>   6 files changed, 481 insertions(+)
>   create mode 100644 Documentation/hwmon/isl28022.rst
>   create mode 100644 drivers/hwmon/isl28022.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index d11924667f76..c9548fc5c40e 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -90,6 +90,7 @@ Hardware Monitoring Kernel Drivers
>      ir35221
>      ir38064
>      ir36021
> +   isl28022
>      isl68137
>      it87
>      jc42
> diff --git a/Documentation/hwmon/isl28022.rst b/Documentation/hwmon/isl28022.rst
> new file mode 100644
> index 000000000000..84c27ddcd33e
> --- /dev/null
> +++ b/Documentation/hwmon/isl28022.rst
> @@ -0,0 +1,56 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver isl28022
> +======================
> +
> +Supported chips:
> +
> +  * Renesas ISL28022
> +
> +    Prefix: 'isl28022'
> +
> +    Addresses scanned: none
> +
> +    Datasheet: Publicly available at the Renesas website
> +
> +	       https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
> +
> +Author:
> +    Carsten Spieß <mail@carsten-spiess.de>
> +
> +Description
> +-----------
> +
> +The ISL28022 is a power monitor with I2C interface. The device monitors
> +voltage, current via shunt resistor and calculated power.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +device explicitly. Please see Documentation/i2c/instantiating-devices.rst for
> +details.
> +
> +The shunt value in micro-ohms, shunt gain and averaging can be set
> +via device tree at compile-time.
> +Please refer to the Documentation/devicetree/bindings/hwmon/isl,isl28022.yaml
> +for bindings if the device tree is used.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. All attributes are read-only.
> +
> +======================= =======================================================
> +in0_input		shunt voltage (micro Volt)
> +in1_input		bus voltage (milli Volt)
> +
> +curr1_input		current (milli Ampere)
> +power1_input		power (micro Watt)
> +
> +note			current and power attributes are supported only when
> +			shunt value is configured via device tree
> +
> +			shunt voltage is in micro Volt, not milli Volt,
> +			to get useful values
> +======================= =======================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7abb5710e1bb..c61aa688cd11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11072,6 +11072,13 @@ S:	Maintained
>   F:	Documentation/filesystems/isofs.rst
>   F:	fs/isofs/
>   
> +ISL28022 HARDWARE MONITORING DRIVER
> +M:	Carsten Spieß <mail@carsten-spiess.de>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/isl28022.rst
> +F:	drivers/hwmon/isl28022.c
> +
>   IT87 HARDWARE MONITORING DRIVER
>   M:	Jean Delvare <jdelvare@suse.com>
>   L:	linux-hwmon@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2913299c2c9e..3049c519e6ac 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -800,6 +800,17 @@ config SENSORS_CORETEMP
>   	  sensor inside your CPU. Most of the family 6 CPUs
>   	  are supported. Check Documentation/hwmon/coretemp.rst for details.
>   
> +config SENSORS_ISL28022
> +	tristate "Renesas ISL28022"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for ISL28022 power monitor.
> +	  Check Documentation/hwmon/isl28022.rst for details.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called isl28022.
> +
>   config SENSORS_IT87
>   	tristate "ITE IT87xx and compatibles"
>   	depends on !PPC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index ff6bfd109c72..4046fed7f48d 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_SENSORS_INA2XX)	+= ina2xx.o
>   obj-$(CONFIG_SENSORS_INA238)	+= ina238.o
>   obj-$(CONFIG_SENSORS_INA3221)	+= ina3221.o
>   obj-$(CONFIG_SENSORS_INTEL_M10_BMC_HWMON) += intel-m10-bmc-hwmon.o
> +obj-$(CONFIG_SENSORS_ISL28022)	+= isl28022.o
>   obj-$(CONFIG_SENSORS_IT87)	+= it87.o
>   obj-$(CONFIG_SENSORS_JC42)	+= jc42.o
>   obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
> diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
> new file mode 100644
> index 000000000000..67cf47c31600
> --- /dev/null
> +++ b/drivers/hwmon/isl28022.c
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * isl28022.c - driver for Renesas ISL28022 power monitor chip
> + *	 monitoring
> + * Copyright (c) 2023 Carsten Spieß <mail@carsten-spiess.de>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>
> +#include <linux/regulator/consumer.h>
> +
> +/* ISL28022 registers */
> +#define ISL28022_REG_CONFIG	0x00
> +#define ISL28022_REG_SHUNT	0x01
> +#define ISL28022_REG_BUS	0x02
> +#define ISL28022_REG_POWER	0x03
> +#define ISL28022_REG_CURRENT	0x04
> +#define ISL28022_REG_CALIB	0x05
> +#define ISL28022_REG_SHUNT_THR	0x06
> +#define ISL28022_REG_BUS_THR	0x07
> +#define ISL28022_REG_INT	0x08
> +#define ISL28022_REG_AUX	0x09
> +#define ISL28022_REG_MAX	ISL28022_REG_AUX
> +
> +/* ISL28022 config flags */
> +/* mode flags */
> +#define ISL28022_MODE_SHIFT	0
> +#define ISL28022_MODE_MASK	0x0007
> +
> +#define ISL28022_MODE_PWR_DOWN	0x0
> +#define ISL28022_MODE_TRG_S	0x1
> +#define ISL28022_MODE_TRG_B	0x2
> +#define ISL28022_MODE_TRG_SB	0x3
> +#define ISL28022_MODE_ADC_OFF	0x4
> +#define ISL28022_MODE_CONT_S	0x5
> +#define ISL28022_MODE_CONT_B	0x6
> +#define ISL28022_MODE_CONT_SB	0x7
> +
> +/* shunt ADC settings */
> +#define ISL28022_SADC_SHIFT	3
> +#define ISL28022_SADC_MASK	0x0078
> +
> +#define ISL28022_BADC_SHIFT	7
> +#define ISL28022_BADC_MASK	0x0780
> +
> +#define ISL28022_ADC_12		0x0	/* 12 bit ADC */
> +#define ISL28022_ADC_13		0x1	/* 13 bit ADC */
> +#define ISL28022_ADC_14		0x2	/* 14 bit ADC */
> +#define ISL28022_ADC_15		0x3	/* 15 bit ADC */
> +#define ISL28022_ADC_15_1	0x8	/* 15 bit ADC, 1 sample */
> +#define ISL28022_ADC_15_2	0x9	/* 15 bit ADC, 2 samples */
> +#define ISL28022_ADC_15_4	0xA	/* 15 bit ADC, 4 samples */
> +#define ISL28022_ADC_15_8	0xB	/* 15 bit ADC, 8 samples */
> +#define ISL28022_ADC_15_16	0xC	/* 15 bit ADC, 16 samples */
> +#define ISL28022_ADC_15_32	0xD	/* 15 bit ADC, 32 samples */
> +#define ISL28022_ADC_15_64	0xE	/* 15 bit ADC, 64 samples */
> +#define ISL28022_ADC_15_128	0xF	/* 15 bit ADC, 128 samples */
> +
> +/* shunt voltage range */
> +#define ISL28022_PG_SHIFT	11
> +#define ISL28022_PG_MASK	0x1800
> +
> +#define ISL28022_PG_40		0x0	/* +/-40 mV */
> +#define ISL28022_PG_80		0x1	/* +/-80 mV */
> +#define ISL28022_PG_160		0x2	/* +/-160 mV */
> +#define ISL28022_PG_320		0x3	/* +/-3200 mV */
> +
> +/* bus voltage range */
> +#define ISL28022_BRNG_SHIFT	13
> +#define ISL28022_BRNG_MASK	0x6000
> +
> +#define ISL28022_BRNG_16	0x0	/* 16 V */
> +#define ISL28022_BRNG_32	0x1	/* 32 V */
> +#define ISL28022_BRNG_60	0x3	/* 60 V */
> +
> +/* reset */
> +#define ISL28022_RESET		0x8000
> +
> +struct isl28022_data {
> +	struct i2c_client	*client;
> +	struct regmap		*regmap;
> +	u32			shunt;
> +	u32			gain;
> +	u32			average;
> +	u16			config;
> +	u16			calib;
> +};
> +
> +static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long *val)
> +{
> +	struct isl28022_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_SHUNT + channel, &regval);
> +			if (err < 0)
> +				return err;
> +			*val = (channel == 0) ?
> +					(long)((s16)((u16)regval)) * 10 :
> +					(long)(((u16)regval) & 0xFFFC);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_CURRENT, &regval);
> +			if (err < 0)
> +				return err;
> +			if (!data->shunt)
> +				return -EINVAL;
> +			*val = ((long)regval * 10000L * (long)data->gain) /
> +				(long)(8 * data->shunt);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			err = regmap_read(data->regmap,
> +					  ISL28022_REG_POWER, &regval);
> +			if (err < 0)
> +				return err;
> +			if (!data->shunt)
> +				return -EINVAL;
> +			*val = ((long)regval * 409600000L * (long)data->gain) /
> +				(long)(8 * data->shunt);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static umode_t isl28022_is_visible(const void *data, enum hwmon_sensor_types type,
> +				   u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			return 0444;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			return 0444;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *isl28022_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
> +			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
> +	NULL
> +};
> +
> +static const struct hwmon_channel_info *isl28022_info_shunt[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,	/* channel 0: shunt voltage (µV) */
> +			   HWMON_I_INPUT),	/* channel 1: bus voltage (mV) */
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT), /* channel 1: current (mA) */
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT),	/* channel 1: power (µW) */
> +	NULL
> +};
> +
> +static const struct hwmon_ops isl28022_hwmon_ops = {
> +	.is_visible = isl28022_is_visible,
> +	.read = isl28022_read,
> +};
> +
> +static const struct hwmon_chip_info isl28022_chip_info = {
> +	.ops = &isl28022_hwmon_ops,
> +	.info = isl28022_info,
> +};
> +
> +static const struct hwmon_chip_info isl28022_chip_info_shunt = {
> +	.ops = &isl28022_hwmon_ops,
> +	.info = isl28022_info_shunt,
> +};
> +
> +static bool isl28022_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ISL28022_REG_CONFIG:
> +	case ISL28022_REG_CALIB:
> +	case ISL28022_REG_SHUNT_THR:
> +	case ISL28022_REG_BUS_THR:
> +	case ISL28022_REG_INT:
> +	case ISL28022_REG_AUX:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool isl28022_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case ISL28022_REG_CONFIG:
> +	case ISL28022_REG_SHUNT:
> +	case ISL28022_REG_BUS:
> +	case ISL28022_REG_POWER:
> +	case ISL28022_REG_CURRENT:
> +	case ISL28022_REG_INT:
> +	case ISL28022_REG_AUX:
> +		return true;
> +	}
> +	return true;
> +}
> +
> +static const struct regmap_config isl28022_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = ISL28022_REG_MAX,
> +	.writeable_reg = isl28022_is_writeable_reg,
> +	.volatile_reg = isl28022_is_volatile_reg,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +	.cache_type = REGCACHE_RBTREE,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +};
> +
> +static const struct i2c_device_id isl28022_ids[] = {
> +	{ "isl28022", 0},
> +	{ /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, isl28022_ids);
> +
> +static int isl28022_config(struct device *dev)
> +{
> +	struct isl28022_data *data = dev_get_drvdata(dev);
> +
> +	if (!dev || !data)
> +		return -EINVAL;
> +				;
> +	data->config = (ISL28022_MODE_CONT_SB << ISL28022_MODE_SHIFT) |
> +			(ISL28022_BRNG_60 << ISL28022_BRNG_SHIFT);
> +
> +	switch (data->gain) {
> +	case 1:
> +		data->config |= (ISL28022_PG_40 << ISL28022_PG_SHIFT);
> +		break;
> +	case 2:
> +		data->config |= (ISL28022_PG_80 << ISL28022_PG_SHIFT);
> +		break;
> +	case 4:
> +		data->config |= (ISL28022_PG_160 << ISL28022_PG_SHIFT);
> +		break;
> +	default:
> +		data->config |= (ISL28022_PG_320 << ISL28022_PG_SHIFT);
> +		data->gain = 8;
> +		break;
> +	}
> +
> +	data->calib = data->shunt ? 0x8000 / data->gain : 0;
> +
> +	switch (data->average) {
> +	case 128:
> +		data->config |= (ISL28022_ADC_15_128 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_128 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 64:
> +		data->config |= (ISL28022_ADC_15_64 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_64 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 32:
> +		data->config |= (ISL28022_ADC_15_32 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_32 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 16:
> +		data->config |= (ISL28022_ADC_15_16 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_16 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 8:
> +		data->config |= (ISL28022_ADC_15_8 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_8 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 4:
> +		data->config |= (ISL28022_ADC_15_4 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_4 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 2:
> +		data->config |= (ISL28022_ADC_15_2 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_2 << ISL28022_BADC_SHIFT);
> +		break;
> +	case 1:
> +		data->config |= (ISL28022_ADC_15_1 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15_1 << ISL28022_BADC_SHIFT);
> +		break;
> +	default:
> +		data->config |= (ISL28022_ADC_15 << ISL28022_SADC_SHIFT) |
> +				(ISL28022_ADC_15 << ISL28022_BADC_SHIFT);
> +		data->average = 0;
> +		break;
> +	}
> +
> +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
> +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);
> +
> +	return 0;
> +}
> +
> +static int isl28022_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct isl28022_data *data;
> +	int status;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -EIO;
> +
> +	data = devm_kzalloc(dev, sizeof(struct isl28022_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &data->shunt);
> +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
> +	of_property_read_u32(dev->of_node, "average", &data->average);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &isl28022_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "isl28022_hwmon",
> +							 data, data->shunt ?
> +							 &isl28022_chip_info_shunt :
> +							 &isl28022_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	status = isl28022_config(hwmon_dev);
> +	if (status)
> +		return status;
> +
> +	dev_info(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev), client->name);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused isl28022_of_match[] = {
> +	{ .compatible = "renesas,isl28022"},
> +	{ /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(of, isl28022_of_match);
> +
> +static struct i2c_driver isl28022_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "isl28022",
> +		.of_match_table = of_match_ptr(isl28022_of_match),
> +	},
> +	.probe_new	= isl28022_probe,
> +	.id_table	= isl28022_ids,
> +};
> +
> +module_i2c_driver(isl28022_driver);
> +
> +MODULE_AUTHOR("Carsten Spiess <mail@carsten-spiess.de>");
> +MODULE_DESCRIPTION("ISL28022 driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Rob Herring]

On Wed, 26 Jul 2023 17:22:34 +0200, Carsten Spieß wrote:
> Add dt-bindings for Renesase ISL28022 power monitor.
> 
> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
> ---
>  .../bindings/hwmon/renesas,isl28022.yaml      | 67 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml: 'anyOf' conditional failed, one must be fixed:
	'type' is a required property
	'properties' is a required property
	'patternProperties' is a required property
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml: 'Required properties' is not one of ['$id', '$schema', 'title', 'description', 'examples', 'required', 'allOf', 'anyOf', 'oneOf', 'definitions', '$defs', 'additionalProperties', 'dependencies', 'dependentRequired', 'dependentSchemas', 'patternProperties', 'properties', 'not', 'if', 'then', 'else', 'unevaluatedProperties', 'deprecated', 'maintainers', 'select', '$ref']
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml: 'Optional properties' is not one of ['$id', '$schema', 'title', 'description', 'examples', 'required', 'allOf', 'anyOf', 'oneOf', 'definitions', '$defs', 'additionalProperties', 'dependencies', 'dependentRequired', 'dependentSchemas', 'patternProperties', 'properties', 'not', 'if', 'then', 'else', 'unevaluatedProperties', 'deprecated', 'maintainers', 'select', '$ref']
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml: 'anyOf' conditional failed, one must be fixed:
	'properties' is a required property
	'patternProperties' is a required property
	hint: Metaschema for devicetree binding documentation
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
Documentation/devicetree/bindings/hwmon/renesas,isl28022.example.dts:22.30-28.15: Warning (i2c_bus_reg): /example-0/i2c/power-monitor@40: I2C bus unit address format error, expected "10"

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230726152235.249569-3-mail@carsten-spiess.de

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Guenter Roeck]

On 7/26/23 09:14, Krzysztof Kozlowski wrote:
> On 26/07/2023 17:22, Carsten Spieß wrote:
>> Add dt-bindings for Renesase ISL28022 power monitor.
>>
>> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
>> ---
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> 
>>   .../bindings/hwmon/renesas,isl28022.yaml      | 67 +++++++++++++++++++
>>   MAINTAINERS                                   |  1 +
>>   2 files changed, 68 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>> new file mode 100644
>> index 000000000000..5ecf892db269
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>> @@ -0,0 +1,67 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +
> 
> No blank line.
> 
>> +$id: http://devicetree.org/schemas/hwmon/renesas,isl28022.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Renesas ISL28022 power monitor
>> +
>> +maintainers:
>> +  - Carsten Spieß <mail@carsten-spiess.de>
>> +
>> +description: |
>> +  The ISL28022 is a power monitor with I2C interface. The device monitors
>> +  voltage, current via shunt resistor and calculated power.
>> +
>> +  Datasheets:
>> +    https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
>> +
>> +Required properties:
>> +  compatible:
>> +    enum:
>> +      - renesas,isl28022
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +Optional properties:
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
> 
> These are "properties"
> 
>> +  shunt-resistor-micro-ohms:
>> +    description:
>> +      Shunt resistor value in micro-Ohm
>> +      defaults to <0> when not set
>> +      monitoring of current and power not supported when <0>
>> +
>> +  shunt-gain:
> 
> 1. Missing vendor prefix (does not look like generic property)
> 2. -microvolt
> 
>> +    description:
>> +      Shunt gain to scale maximal shunt voltage to
>> +      40mV, 80mV, 160mV, 320mV
>> +      defaults to <8> (320mV) when not set
> 
> And then enum is for 40, 80, 160 and 320.
> 
>> +    enum: [1, 2, 4, 8]
>> +
>> +  average:
>> +    description: |
>> +      Number of samples to be used to report voltage, current and power values.
>> +      defaults to <0> when not set
>> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]
> 
> I am sure hwmon has some property for this. Are you sure it is called
> "average"?
> 

Something with samples. adi,power-sample-average is similar. Others
use average-samples, qcom,avg-samples, touchscreen-average-samples.

Guenter

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Krzysztof Kozlowski]

On 26/07/2023 20:19, Guenter Roeck wrote:
>>
>>> +    enum: [1, 2, 4, 8]
>>> +
>>> +  average:
>>> +    description: |
>>> +      Number of samples to be used to report voltage, current and power values.
>>> +      defaults to <0> when not set
>>> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]
>>
>> I am sure hwmon has some property for this. Are you sure it is called
>> "average"?
>>
> 
> Something with samples. adi,power-sample-average is similar. Others
> use average-samples, qcom,avg-samples, touchscreen-average-samples.

So probably it's a time to come with something generic, e.g.:
average-samples in some hwmon.yaml

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Guenter Roeck]

On 7/26/23 11:27, Krzysztof Kozlowski wrote:
> On 26/07/2023 20:19, Guenter Roeck wrote:
>>>
>>>> +    enum: [1, 2, 4, 8]
>>>> +
>>>> +  average:
>>>> +    description: |
>>>> +      Number of samples to be used to report voltage, current and power values.
>>>> +      defaults to <0> when not set
>>>> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]
>>>
>>> I am sure hwmon has some property for this. Are you sure it is called
>>> "average"?
>>>
>>
>> Something with samples. adi,power-sample-average is similar. Others
>> use average-samples, qcom,avg-samples, touchscreen-average-samples.
> 
> So probably it's a time to come with something generic, e.g.:
> average-samples in some hwmon.yaml
> 

Maybe, but that is going to take a lot of time. Many configuration options
of hardware monitoring chips (such as the thermal sensor type, the ideality
factor of a thermal diode, or the number of pulses per revolution reported
by fans) are actually properties of the connected device, and Rob has made
clear that he expects such properties to be associated with the actual
device (fan, thermal sensor, etc). I don't even know how to model that,
and I really don't have the time (nor, really, the expertise) to come up
with acceptable generic bindings.

Guenter

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Krzysztof Kozlowski]

On 26/07/2023 20:59, Guenter Roeck wrote:
> On 7/26/23 11:27, Krzysztof Kozlowski wrote:
>> On 26/07/2023 20:19, Guenter Roeck wrote:
>>>>
>>>>> +    enum: [1, 2, 4, 8]
>>>>> +
>>>>> +  average:
>>>>> +    description: |
>>>>> +      Number of samples to be used to report voltage, current and power values.
>>>>> +      defaults to <0> when not set
>>>>> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]
>>>>
>>>> I am sure hwmon has some property for this. Are you sure it is called
>>>> "average"?
>>>>
>>>
>>> Something with samples. adi,power-sample-average is similar. Others
>>> use average-samples, qcom,avg-samples, touchscreen-average-samples.
>>
>> So probably it's a time to come with something generic, e.g.:
>> average-samples in some hwmon.yaml
>>
> 
> Maybe, but that is going to take a lot of time. Many configuration options
> of hardware monitoring chips (such as the thermal sensor type, the ideality
> factor of a thermal diode, or the number of pulses per revolution reported
> by fans) are actually properties of the connected device, and Rob has made
> clear that he expects such properties to be associated with the actual
> device (fan, thermal sensor, etc). 

Sure, we can go then with usual vendor like property, so:
renesas,average-samples


Best regards,
Krzysztof

Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Randy Dunlap]

On 7/26/23 08:22, Carsten Spieß wrote:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7abb5710e1bb..c61aa688cd11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11072,6 +11072,13 @@ S:	Maintained
>  F:	Documentation/filesystems/isofs.rst
>  F:	fs/isofs/
>  

New entry is not quite in the correct place for alphabetical order.
It should be just before the ISOFS entry, not just after it.

> +ISL28022 HARDWARE MONITORING DRIVER
> +M:	Carsten Spieß <mail@carsten-spiess.de>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/isl28022.rst
> +F:	drivers/hwmon/isl28022.c
> +
>  IT87 HARDWARE MONITORING DRIVER
>  M:	Jean Delvare <jdelvare@suse.com>
>  L:	linux-hwmon@vger.kernel.org

-- 
~Randy

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Carsten Spieß]

[-- Attachment #1: Type: text/plain, Size: 3002 bytes --]

thanks all for your input.
I conclude here in one answer:

On 7/26/23 18:14, Krzysztof Kozlowski wrote:
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
sorry i missed that, fixed for [Patch v2].


On 7/26/23 18:14, Krzysztof Kozlowski wrote:
>> +  shunt-gain:  
> 1. Missing vendor prefix (does not look like generic property)
> 2. -microvolt
> And then enum is for 40, 80, 160 and 320.
replaced with
  renesas,shunt-range-milli-volts:
    description: |
      maximal shunt voltage range of 40mV, 80mV, 160mV or 320mV
    $ref: /schemas/types.yaml#/definitions/uint32-array
    default: 320
    enum: [40, 80, 160, 320]

On 26/07/2023 18:16, Guenter Roeck wrote:
>> +  shunt-resistor-micro-ohms:
>> +    description:
>> +      Shunt resistor value in micro-Ohm
>> +      defaults to <0> when not set
>> +      monitoring of current and power not supported when <0>
> +  
> Should not default to 0 (disabled).
When shunt resistor value is not known, current and power calculation can not be done,
but the shunt voltage still can be measured.
As the resistor is externaly connected to the ISL28022 chip there is no default.
When i set default to any value (e.g 1000 micro Ohm), current and power calculation
will give wrong results (except when the shunt _is_ 1000 micro Ohm)

On 26/07/2023 18:16, Guenter Roeck wrote:
>> +  average:
>> +    description: |
>> +      Number of samples to be used to report voltage, current and power values.
>> +      defaults to <0> when not set
>> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]  
> Should not default to 0 (disabled).
From datasheet table 6 on page 15 there are two similar ADC modes:
  3 - without samples
  8 - with one? sample
I intended enum 0 to ADC mode 3, 1 to ADC mode 8,
but i can live without ADC mode 3.

On 26/07/2023 20:59, Guenter Roeck wrote:
> On 7/26/23 11:27, Krzysztof Kozlowski wrote:  
>> On 26/07/2023 20:19, Guenter Roeck wrote:  
>>>>> +  average:
>>>>> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]  
>>>>
>>>> I am sure hwmon has some property for this. Are you sure it is called
>>>> "average"?
>>>>  
>>>
>>> Something with samples. adi,power-sample-average is similar. Others
>>> use average-samples, qcom,avg-samples, touchscreen-average-samples.  
>>
>> So probably it's a time to come with something generic, e.g.:
>> average-samples in some hwmon.yaml

As there's no generic yet, replaced with
  renesas,average-samples:
    description: |
      Number of samples to be used to report voltage, current and power values.
    default: 1
    $ref: /schemas/types.yaml#/definitions/uint32-array
    enum: [1, 2, 4, 8, 16, 32, 64, 128]



On 26/07/2023 19:13, Rob Herring wrote:
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
Fixed for [Patch v2], runs without errors or warnings

Regards, Carsten

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Carsten Spieß]

[-- Attachment #1: Type: text/plain, Size: 4603 bytes --]


On 7/26/23 18:14, Gueter Roeck wrote:
> On 7/26/23 08:22, Carsten Spieß wrote:
> > +The shunt value in micro-ohms, shunt gain and averaging can be set
> > +via device tree at compile-time.  
> 
> At _compile-time_ ?
How to explain better that it isn't set at runtime?
Other drivers use the same term.

> I'd argue that shunt voltage is all but useless, but if you want to have it supported
> it _has_ to be in mV.
That's a problem.

In my case the ER-6P has a 8 milli Ohm (or 8000 micro Ohm) shunt and a powersupply with 
max. 2.5 A. This gives a max shunt voltage of 20 mV at 2.5 A.
The device normaly consumes between 200 and 500 mA. (typ ~250 mA).
This results in shunt voltage of 1.6 to 4.0 mV (typ ~2mV).
Having no fractions will make it useless. 

Unfortunately there is no possibility to give a scaling factor.
Or returning float values (i know, this can't and shouldn't be changed)

> Why not support limit attributes ?
Due to limited time, left for later enhancement.


> > +#define ISL28022_REG_SHUNT	0x01
> > +#define ISL28022_REG_BUS	0x02


> > +	switch (type) {
> > +	case hwmon_in:
> > +		switch (attr) {
> > +		case hwmon_in_input:
> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_SHUNT + channel, &regval);  
> 
> That never reads REG_BUS.
Hmm, 
channel 0: ISL28022_REG_SHUNT + 0 == 0x01
channel 1: ISL28022_REG_SHUNT + 1 == 0x02 == ISL28022_REG_BUS
or do i miss something?

> > +			if (err < 0)
> > +				return err;
> > +			*val = (channel == 0) ?
> > +					(long)((s16)((u16)regval)) * 10 :
> > +					(long)(((u16)regval) & 0xFFFC);  
> 
> I don't think the sign extensions are correct based on the datasheet.
> This will have to use sign_extend.
From my understading (see table 11 on page 16 of the ISL28022 datasheet)
shunt value is already sign extended, (D15-D12 is sign)
bus value (table 12 on page 16) is unsigned.

> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_CURRENT, &regval);
> > +			if (err < 0)
> > +				return err;
> > +			if (!data->shunt)
> > +				return -EINVAL;  
> 
> Getting an error message each time the "sensors" command is executed ?
> Unacceptable.
o.k., will change to set *val = 0;

> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_POWER, &regval);
> > +			if (err < 0)
> > +				return err;
> > +			if (!data->shunt)
> > +				return -EINVAL;  
> 
> Unacceptable.
o.k., as above

> > +			*val = ((long)regval * 409600000L * (long)data->gain) /
> > +				(long)(8 * data->shunt);  
> 
> I don't think this was checked for overflows.
Yes, i must first divide then multiply.
I have to think about how to keep accuracy on high shunt resistor values.

> > +static int isl28022_config(struct device *dev)
> > +{
> > +	struct isl28022_data *data = dev_get_drvdata(dev);
> > +
> > +	if (!dev || !data)
> > +		return -EINVAL;  
> 
> How would this ever happen ?
Shouldn't, but i'm carefully (i had it once during development due to an error
(using dev instead of hwmon_dev) on calling this function
 
> > +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
> > +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);  
> 
> Error checking needed.
o.k. will add.

> > +static int isl28022_probe(struct i2c_client *client)
> > +{

> > +	if (!i2c_check_functionality(client->adapter,
> > +				     I2C_FUNC_SMBUS_BYTE_DATA |
> > +				     I2C_FUNC_SMBUS_WORD_DATA))
> > +		return -EIO;  
> 
> This is not an IO error. Return -ENODEV as most other drivers do.
o.k.

> > +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
> > +	of_property_read_u32(dev->of_node, "average", &data->average);  
> 
> Check for errors and provide defaults if properties are not set.
o.k.

> Also please use device_property_read_u32() to enable use from non-devicetree
> systems.
o.k. Never used this, have to look for an example on using it.
Many (old) drivers are using the of_property_* functions, is it intended to replace it.
What about backporting this driver to e.g. 5.15, will it affect it?

> > +	status = isl28022_config(hwmon_dev);
> > +	if (status)
> > +		return status;  
> 
> That has to happen before the call to devm_hwmon_device_register_with_info()
> to avoid race conditions.
o.k.

> > +static struct i2c_driver isl28022_driver = {
> > +	.class		= I2C_CLASS_HWMON,
> > +	.driver = {
> > +		.name	= "isl28022",
> > +		.of_match_table = of_match_ptr(isl28022_of_match),  
> 
> Drop of_match_ptr()
Most drivers have this, why drop?

Regards, Carsten

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Carsten Spieß]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

 On 7/26/23 18:19, Guenter Roeck wrote:
> Please provide a register dump (using i2cdump) for this chip. 

# i2cdump -y -r 0-9 1 0x40 w
     0,8  1,9  2,a  3,b  4,c  5,d  6,e  7,f
00: ff67 ca00 a25d c803 5006 0080 817f 00ff 
08: 0000 0000  

Please note that due to big vs.- little endian bytes are swapped,
should be read as:
00: 67ff 00ca 5da2 03c8 0650 8000 7f81 ff00
08: 0000 0000  

corresponding sensor values are about (not read at same time):
in0:           1.99 V  
in1:          23.97 V  
power1:        6.10 W  
curr1:       248.00 mA 

in0 should be read as mV

Regards, Carsten

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On Thu, Jul 27, 2023 at 09:35:28AM +0200, Carsten Spieß wrote:

> On 7/26/23 18:14, Krzysztof Kozlowski wrote:
> >> +  shunt-gain:  
> > 1. Missing vendor prefix (does not look like generic property)
> > 2. -microvolt
> > And then enum is for 40, 80, 160 and 320.
> replaced with
>   renesas,shunt-range-milli-volts:
>     description: |
>       maximal shunt voltage range of 40mV, 80mV, 160mV or 320mV
>     $ref: /schemas/types.yaml#/definitions/uint32-array
>     default: 320
>     enum: [40, 80, 160, 320]

The point of using -microvolt is that it is a standard suffix:
https://github.com/devicetree-org/dt-schema/blob/c1e73ebea9fc07f7d7848f304301f755f1278a67/dtschema/schemas/property-units.yaml#L101

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Krzysztof Kozlowski]

On 27/07/2023 09:35, Carsten Spieß wrote:
> thanks all for your input.
> I conclude here in one answer:
> 
> On 7/26/23 18:14, Krzysztof Kozlowski wrote:
>> It does not look like you tested the bindings, at least after quick
>> look. Please run `make dt_binding_check` (see
> sorry i missed that, fixed for [Patch v2].
> 
> 
> On 7/26/23 18:14, Krzysztof Kozlowski wrote:
>>> +  shunt-gain:  
>> 1. Missing vendor prefix (does not look like generic property)
>> 2. -microvolt
>> And then enum is for 40, 80, 160 and 320.
> replaced with
>   renesas,shunt-range-milli-volts:

That's not correct unit in DT, so again please use -microvolt:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

>     description: |
>       maximal shunt voltage range of 40mV, 80mV, 160mV or 320mV
>     $ref: /schemas/types.yaml#/definitions/uint32-array

And then drop this line.


Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Carsten Spieß]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]


On 7/27/23 10:25, Krzysztof Kozlowski wrote:  

> > replaced with
> >   renesas,shunt-range-milli-volts:  
> 
> That's not correct unit in DT, so again please use -microvolt:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
a bit unhandy but o.k., applied to [Patch v2]

> >     description: |
> >       maximal shunt voltage range of 40mV, 80mV, 160mV or 320mV
> >     $ref: /schemas/types.yaml#/definitions/uint32-array  
>
> And then drop this line.
Applied.

Regards, Carsten

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Guenter Roeck]

On 7/27/23 00:35, Carsten Spieß wrote:
> thanks all for your input.
> I conclude here in one answer:
> 
> On 7/26/23 18:14, Krzysztof Kozlowski wrote:
>> It does not look like you tested the bindings, at least after quick
>> look. Please run `make dt_binding_check` (see
> sorry i missed that, fixed for [Patch v2].
> 
> 
> On 7/26/23 18:14, Krzysztof Kozlowski wrote:
>>> +  shunt-gain:
>> 1. Missing vendor prefix (does not look like generic property)
>> 2. -microvolt
>> And then enum is for 40, 80, 160 and 320.
> replaced with
>    renesas,shunt-range-milli-volts:
>      description: |
>        maximal shunt voltage range of 40mV, 80mV, 160mV or 320mV
>      $ref: /schemas/types.yaml#/definitions/uint32-array
>      default: 320
>      enum: [40, 80, 160, 320]
> 
> On 26/07/2023 18:16, Guenter Roeck wrote:
>>> +  shunt-resistor-micro-ohms:
>>> +    description:
>>> +      Shunt resistor value in micro-Ohm
>>> +      defaults to <0> when not set
>>> +      monitoring of current and power not supported when <0>
>> +
>> Should not default to 0 (disabled).
> When shunt resistor value is not known, current and power calculation can not be done,
> but the shunt voltage still can be measured.
> As the resistor is externaly connected to the ISL28022 chip there is no default.
> When i set default to any value (e.g 1000 micro Ohm), current and power calculation
> will give wrong results (except when the shunt _is_ 1000 micro Ohm)
> 

Just like many other hardware monitoring drivers do. That is why /etc/sensors3.conf
exists. That is not a valid argument and not a reason to effectively make the driver
useless on systems where devicetree or the matching ACPI data is not provided.

> On 26/07/2023 18:16, Guenter Roeck wrote:
>>> +  average:
>>> +    description: |
>>> +      Number of samples to be used to report voltage, current and power values.
>>> +      defaults to <0> when not set
>>> +    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]
>> Should not default to 0 (disabled).
>  From datasheet table 6 on page 15 there are two similar ADC modes:
>    3 - without samples
>    8 - with one? sample
> I intended enum 0 to ADC mode 3, 1 to ADC mode 8,
> but i can live without ADC mode 3.
> 

Looking at the datasheet, 3 and 8 seem to be identical. There is actually no
"0 samples", which is why I assumed you wanted to use that to disable the sensor
(though, thinking about it, that doesn't really make sense). What is missing
is that 0..3 actually control the sensor accuracy (12..15 bit).

Guenter

Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Guenter Roeck]

On 7/27/23 00:35, Carsten Spieß wrote:
> 
> On 7/26/23 18:14, Gueter Roeck wrote:
>> On 7/26/23 08:22, Carsten Spieß wrote:
>>> +The shunt value in micro-ohms, shunt gain and averaging can be set
>>> +via device tree at compile-time.
>>
>> At _compile-time_ ?
> How to explain better that it isn't set at runtime?
> Other drivers use the same term.

You mean it is ok to exceed the speed limit because others do it
as well ?

[ Sorry, I have heard the "Other drivers do it" pseudo-argument too many times.
   That doesn't mean it is the right thing to do. ]

Why not just leave it off ? What value does it add ? Besides,
even "via devicetree" isn't really correct because it can also
be set via ACPI or by a platform driver when using device_ properties.
I would suggest "can be set with device properties".

> 
>> I'd argue that shunt voltage is all but useless, but if you want to have it supported
>> it _has_ to be in mV.
> That's a problem.
> 
> In my case the ER-6P has a 8 milli Ohm (or 8000 micro Ohm) shunt and a powersupply with
> max. 2.5 A. This gives a max shunt voltage of 20 mV at 2.5 A.
> The device normaly consumes between 200 and 500 mA. (typ ~250 mA).
> This results in shunt voltage of 1.6 to 4.0 mV (typ ~2mV).
> Having no fractions will make it useless.
> 
> Unfortunately there is no possibility to give a scaling factor.
> Or returning float values (i know, this can't and shouldn't be changed)
> 

Just like the ABI must not be changed. The sensors command would display your
4mV shunt voltage as 4V, which is just as useless.

In practice, the shunt voltage _is_ useless for hardware monitoring purpose
because it can be calculated from current and shunt resistor value.
I'd say if you really want it, provide it as debugfs attribute. As hwmon
attribute it has to be in mV.

>> Why not support limit attributes ?
> Due to limited time, left for later enhancement.
> 
> 
>>> +#define ISL28022_REG_SHUNT	0x01
>>> +#define ISL28022_REG_BUS	0x02
> 
> 
>>> +	switch (type) {
>>> +	case hwmon_in:
>>> +		switch (attr) {
>>> +		case hwmon_in_input:
>>> +			err = regmap_read(data->regmap,
>>> +					  ISL28022_REG_SHUNT + channel, &regval);
>>
>> That never reads REG_BUS.
> Hmm,
> channel 0: ISL28022_REG_SHUNT + 0 == 0x01
> channel 1: ISL28022_REG_SHUNT + 1 == 0x02 == ISL28022_REG_BUS
> or do i miss something?
> 

No, I missed the "+ channel".

>>> +			if (err < 0)
>>> +				return err;
>>> +			*val = (channel == 0) ?
>>> +					(long)((s16)((u16)regval)) * 10 :
>>> +					(long)(((u16)regval) & 0xFFFC);
>>
>> I don't think the sign extensions are correct based on the datasheet.
>> This will have to use sign_extend.
>  From my understading (see table 11 on page 16 of the ISL28022 datasheet)
> shunt value is already sign extended, (D15-D12 is sign)
> bus value (table 12 on page 16) is unsigned.
> 

Not really. For the shunt voltage, 0xf000 has different meanings depending on scale
and range settings. LSB for bus voltage is 4 mV and starts at bit 2 or 3 depending
on BRNG. The above just happens to be correct if BRNG = 10 OR 11 per datasheet.
If that is intentional, it needs to get a comment.

>>> +			err = regmap_read(data->regmap,
>>> +					  ISL28022_REG_CURRENT, &regval);
>>> +			if (err < 0)
>>> +				return err;
>>> +			if (!data->shunt)
>>> +				return -EINVAL;
>>
>> Getting an error message each time the "sensors" command is executed ?
>> Unacceptable.
> o.k., will change to set *val = 0;
> 
Still unacceptable.

>>> +			err = regmap_read(data->regmap,
>>> +					  ISL28022_REG_POWER, &regval);
>>> +			if (err < 0)
>>> +				return err;
>>> +			if (!data->shunt)
>>> +				return -EINVAL;
>>
>> Unacceptable.
> o.k., as above
> 
>>> +			*val = ((long)regval * 409600000L * (long)data->gain) /
>>> +				(long)(8 * data->shunt);
>>
>> I don't think this was checked for overflows.
> Yes, i must first divide then multiply.
> I have to think about how to keep accuracy on high shunt resistor values.
> 
>>> +static int isl28022_config(struct device *dev)
>>> +{
>>> +	struct isl28022_data *data = dev_get_drvdata(dev);
>>> +
>>> +	if (!dev || !data)
>>> +		return -EINVAL;
>>
>> How would this ever happen ?
> Shouldn't, but i'm carefully (i had it once during development due to an error
> (using dev instead of hwmon_dev) on calling this function
>   

Parameter checks are only acceptable on API functions. This is not an API function.
Local functions are expected to be consistent. If this function is called with
a bad argument, that needs to be fixed during development.

>>> +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
>>> +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);
>>
>> Error checking needed.
> o.k. will add.
> 
>>> +static int isl28022_probe(struct i2c_client *client)
>>> +{
> 
>>> +	if (!i2c_check_functionality(client->adapter,
>>> +				     I2C_FUNC_SMBUS_BYTE_DATA |
>>> +				     I2C_FUNC_SMBUS_WORD_DATA))
>>> +		return -EIO;
>>
>> This is not an IO error. Return -ENODEV as most other drivers do.
> o.k.
> 
>>> +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
>>> +	of_property_read_u32(dev->of_node, "average", &data->average);
>>
>> Check for errors and provide defaults if properties are not set.
> o.k.
> 
>> Also please use device_property_read_u32() to enable use from non-devicetree
>> systems.
> o.k. Never used this, have to look for an example on using it.
> Many (old) drivers are using the of_property_* functions, is it intended to replace it.

This is not an old driver. It is more generic than of_ functions and
should be used in new drivers.

> What about backporting this driver to e.g. 5.15, will it affect it?
> 

Device property callback functions existed for a long time. The function
exists in v4.14.y, which is the oldest supported kernel. Either case,
lack of support in an older kernel version is not an argument for avoiding
a new API. Anyone who backports a driver to an older kernel is responsible
for handling the backport, which may include new API functions.

Specific example: Your driver will have to use the .probe callback.
That has one argument in the latest kernel. In v5.15.y, it has two arguments.
You'll have to use the .probe_new callback there. Yes, technically you could
try sneaking in the use of .probe_new in your driver, but that callback will
be removed in v6.6. So you'll _have_ to do some backport, if you want it or not.

>>> +	status = isl28022_config(hwmon_dev);
>>> +	if (status)
>>> +		return status;
>>
>> That has to happen before the call to devm_hwmon_device_register_with_info()
>> to avoid race conditions.
> o.k.
> 
>>> +static struct i2c_driver isl28022_driver = {
>>> +	.class		= I2C_CLASS_HWMON,
>>> +	.driver = {
>>> +		.name	= "isl28022",
>>> +		.of_match_table = of_match_ptr(isl28022_of_match),
>>
>> Drop of_match_ptr()
> Most drivers have this, why drop?
> 

It is needed for device_property_read_u32() to work. And, again, "other drivers
do it" is not a valid argument.

Guenter

Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Carsten Spieß]

[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]


On 7/27/23 16:30, Gueter Roeck wrote:  
> >> On 7/26/23 08:22, Carsten Spieß wrote:  
> >> At _compile-time_ ?  
> > How to explain better that it isn't set at runtime?
> I would suggest "can be set with device properties".
Yeah, i like that.

> >> I'd argue that shunt voltage is all but useless, but if you want to have it supported
> >> it _has_ to be in mV.  
> > That's a problem.
> > 
> > In my case the ER-6P has a 8 milli Ohm (or 8000 micro Ohm) shunt and a powersupply with
> > max. 2.5 A. This gives a max shunt voltage of 20 mV at 2.5 A.
> > The device normaly consumes between 200 and 500 mA. (typ ~250 mA).
> > This results in shunt voltage of 1.6 to 4.0 mV (typ ~2mV).
> > Having no fractions will make it useless.
> > 
> > Unfortunately there is no possibility to give a scaling factor.
> > Or returning float values (i know, this can't and shouldn't be changed)
> >   
> 
> Just like the ABI must not be changed. The sensors command would display your
> 4mV shunt voltage as 4V, which is just as useless.
> 
> In practice, the shunt voltage _is_ useless for hardware monitoring purpose
> because it can be calculated from current and shunt resistor value.
> I'd say if you really want it, provide it as debugfs attribute. As hwmon
> attribute it has to be in mV.
O.k. will move to debugfs.

> >> I don't think the sign extensions are correct based on the datasheet.
> >> This will have to use sign_extend.  
> >  From my understading (see table 11 on page 16 of the ISL28022 datasheet)
> > shunt value is already sign extended, (D15-D12 is sign)
> > bus value (table 12 on page 16) is unsigned.
> >   
> 
> Not really. For the shunt voltage, 0xf000 has different meanings depending on scale
> and range settings. 
Sorry, i don't agree, 0xf000 is -40.96 mV on all scale settings.

> LSB for bus voltage is 4 mV and starts at bit 2 or 3 depending
> on BRNG. The above just happens to be correct if BRNG = 10 OR 11 per datasheet.
> If that is intentional, it needs to get a comment.
Yes, will add comment.

> >> Getting an error message each time the "sensors" command is executed ?
> >> Unacceptable.  
> > o.k., will change to set *val = 0;
> >   
> Still unacceptable.
O.k. i will limit shunt-resistor-milli-ohms to a minimal value > 0 and drop check here.

> >>> +	if (!dev || !data)
> >>> +		return -EINVAL;  
> >>
> >> How would this ever happen ?  
> > Shouldn't, but i'm carefully (i had it once during development due to an error
> > (using dev instead of hwmon_dev) on calling this function
> >     
> 
> Parameter checks are only acceptable on API functions. This is not an API function.
> Local functions are expected to be consistent. If this function is called with
> a bad argument, that needs to be fixed during development.
O.k., removed.

> >>> +static struct i2c_driver isl28022_driver = {
> >>> +	.class		= I2C_CLASS_HWMON,
> >>> +	.driver = {
> >>> +		.name	= "isl28022",
> >>> +		.of_match_table = of_match_ptr(isl28022_of_match),  
> >>
> >> Drop of_match_ptr()  
> > Most drivers have this, why drop?
> >   
> 
> It is needed for device_property_read_u32() to work. 
O.k. dropped, i wasn't familiar with device_property_read functions.

Regards Carsten


[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

[Carsten Spieß]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]


> On 7/26/23 18:19, Guenter Roeck wrote:
> I strongly suspect that the conversions will result in overflows and that they are not always correct.
> I'll want to write unit test code before accepting the driver.
I changed the conversion for current to 
				*val = ((long)regval * 1250L * (long)data->gain) /
					(long)data->shunt;

The term (1250 * gain) will be 10000 max, fits to 14 bit.
So no risk for 32bit overflow when multiply with 16bit value.

And changed the conversion for power to 
				*val = ((51200000L * ((long)data->gain)) /
					(long)data->shunt) * (long)regval;

The first term (51200000 * gain / shunt) will be larger than 16bit when 
(shunt/gain) is less than ~800. So min values for shunt are 
- 6400 µOhm for 320 mV range
- 3200 µOhm for 160 mV range
- 1600 µOhm for 80 mV range
- 800 µOhm for 40 mV range
Can i set this conditionally in the .yaml file?

Regards Carsten

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Carsten Spieß]

Add dt-bindings for Renesas ISL28022 power monitor.

Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
---
 .../bindings/hwmon/renesas,isl28022.yaml      | 65 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
new file mode 100644
index 000000000000..1e0971287941
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/renesas,isl28022.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas ISL28022 power monitor
+
+maintainers:
+  - Carsten Spieß <mail@carsten-spiess.de>
+
+description: |
+  The ISL28022 is a power monitor with I2C interface. The device monitors
+  voltage, current via shunt resistor and calculated power.
+
+  Datasheets:
+    https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
+
+properties:
+  compatible:
+    enum:
+      - renesas,isl28022
+
+  reg:
+    maxItems: 1
+
+  shunt-resistor-micro-ohms:
+    description: |
+      Shunt resistor value in micro-Ohm
+    minimum: 800
+    default: 10000
+
+  renesas,shunt-range-microvolt:
+    description: |
+      Maximal shunt voltage range of +/- 40 mV, 80 mV, 160 mV or 320 mV
+    default: 320000
+    enum: [40000, 80000, 160000, 320000]
+
+  renesas,average-samples:
+    description: |
+      Number of samples to be used to report voltage, current and power values.
+    default: 1
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 8, 16, 32, 64, 128]
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-monitor@40 {
+            compatible = "renesas,isl28022";
+            reg = <0x40>;
+            shunt-resistor-micro-ohms = <8000>;
+            renesas,shunt-range-microvolt = <40000>;
+            renesas,average-samples = <128>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index b02e3b991676..23b8e8183ece 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11069,6 +11069,7 @@ ISL28022 HARDWARE MONITORING DRIVER
 M:	Carsten Spieß <mail@carsten-spiess.de>
 L:	linux-hwmon@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
 F:	Documentation/hwmon/isl28022.rst
 F:	drivers/hwmon/isl28022.c
 
-- 
2.34.1